Re: [PATCH/RFC 0/2] bisect per-worktree

Re: [PATCH/RFC 0/2] bisect per-worktree

[David Turner]

On Sat, 2015-08-01 at 06:04 +0200, Christian Couder wrote:
> 
> Le 1 août 2015 09:01, "David Turner" <dturner@twopensource.com> a
> écrit :
> >
> > This is RFC because I'm not sure why show-ref only works on refs/
> (and
> > whether it should learn to look in worktree-refs/).  I'm also not
> sure
> > whether there are other changes I should make to refs.c to handle
> > per-worktree refs; I basically did the simplest thing I could think
> of
> > to start with.
> 
> (Sorry, I am answering from my phone, as I am having a vacation. )

Enjoy your vacation.

> I wonder what would happen if people upgrade in the middle of a
> bisection or if they have scripts using "bisect/bad" for example.

Junio told me not to worry about this, so I didn't.  

[PATCH/RFC 0/2] bisect per-worktree

[David Turner]

This is RFC because I'm not sure why show-ref only works on refs/ (and
whether it should learn to look in worktree-refs/).  I'm also not sure
whether there are other changes I should make to refs.c to handle
per-worktree refs; I basically did the simplest thing I could think of
to start with.

Re: [PATCH/RFC 0/2] bisect per-worktree

[Michael Haggerty]

On 08/01/2015 01:56 AM, David Turner wrote:
> This is RFC because I'm not sure why show-ref only works on refs/ (and
> whether it should learn to look in worktree-refs/).  I'm also not sure
> whether there are other changes I should make to refs.c to handle
> per-worktree refs; I basically did the simplest thing I could think of
> to start with.

It seems to me that adding a new top-level "worktree-refs" directory is
pretty traumatic. Lots of people and tools will have made the assumption
that all "normal" references live under "refs/".

Each worktree has a name, right? What if worktree-specific refs were
stored under "refs/worktree/<name>/" or something? This would require
the name to be a valid reference name component, but I think that is a
prudent idea anyway.

Of course that doesn't answer the question: where should the refs for
the main repository go? I suppose either in the current place, or maybe
the main repository should have a name too (e.g. "main") and its
references should be stored under "refs/worktree/main/".

These worktree-specific refs should presumably be deleted automatically
when the worktree is deleted.

Either way, there's also the question of who should know how to find the
worktree-specific references--the program that wants to access them, or
should there be a secret invisible mapping that is done on lookup, and
that knows the full list of references that want to be worktree-specific
(for example, that in a linked worktree, "refs/bisect/*" should be
silently redirected to "refs/worktree/<name>/bisect/*")?

It's all a bit frightening, frankly.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH/RFC 0/2] bisect per-worktree

[Junio C Hamano]

On Fri, Jul 31, 2015 at 8:59 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>
> It seems to me that adding a new top-level "worktree-refs" directory is
> pretty traumatic. Lots of people and tools will have made the assumption
> that all "normal" references live under "refs/".
> ...
> It's all a bit frightening, frankly.

I actually feel the prospect of pluggable ref backend more frightening,
frankly ;-). These bisect refs are just like FETCH_HEAD and MERGE_HEAD,
not about the primary purpose of the "repository" to grow the history of refs
(branches), but about ephemeral pointers into the history used to help keep
track of what is being done in the worktree upstairs. There is no need for
these to be visible across worktrees. If we use the real refs that are grobal
in the repository (as opposed to per-worktree ones), we would hit the backend
databas with transactions to update these ephemeral things, which somehow
makes me feel stupid.

I wish we could just make refs/bisect/* (or whatever the current bisect
code uses) automatically per worktree. I know David dismissed it saying
that the current code is not set up to allow it easily, but is it
really a fundamental
limitation, or is it just a matter of coding a few more dozens of lines?

If we can keep using the same location under refs/ and transparently make
them appear per-worktree, "what is the name of the main one?", and "do we
even need to call the one and the only one 'main'?" will automatically
disappear.
Of course, "git bisect" and "gitk --bisect" does not have to change if
we go that
route.

And there will not be any backward compatibility worries. If you are not
using multiple worktrees, you will see them as refs/bisect/*, just at the
same location as you are familiar with.

Re: [PATCH/RFC 0/2] bisect per-worktree

[David Turner]

On Fri, 2015-07-31 at 22:12 -0700, Junio C Hamano wrote:
> On Fri, Jul 31, 2015 at 8:59 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> >
> > It seems to me that adding a new top-level "worktree-refs" directory is
> > pretty traumatic. Lots of people and tools will have made the assumption
> > that all "normal" references live under "refs/".
> > ...
> > It's all a bit frightening, frankly.
> 
> I actually feel the prospect of pluggable ref backend more frightening,
> frankly ;-). These bisect refs are just like FETCH_HEAD and MERGE_HEAD,
> not about the primary purpose of the "repository" to grow the history of refs
> (branches), but about ephemeral pointers into the history used to help keep
> track of what is being done in the worktree upstairs. There is no need for
> these to be visible across worktrees. If we use the real refs that are grobal
> in the repository (as opposed to per-worktree ones), we would hit the backend
> databas with transactions to update these ephemeral things, which somehow
> makes me feel stupid.

Agreed, I think it's a mistake to complicate the global ref namespace
like that.

> I wish we could just make refs/bisect/* (or whatever the current bisect
> code uses) automatically per worktree. I know David dismissed it saying
> that the current code is not set up to allow it easily, but is it
> really a fundamental
> limitation, or is it just a matter of coding a few more dozens of lines?

We still need the packed-refs and is_per_worktree_ref change; we need a
different and more complicated loose-refs loading change, and we need
changes to path.c to treat refs/bisect different from refs/.  Probably a
few dozen lines, yeah.  And it's sort of ugly to make the refs/bisect
special case into a fundamental part of the repository structure.  

I'm worried that we'll discover a need for more of these.  But I can do
the rewrite and see how it looks (probably on Monday).

Do we need to worry about reflogs for these?  If so, a few more lines,
probably.

> If we can keep using the same location under refs/ and transparently make
> them appear per-worktree, "what is the name of the main one?", and "do we
> even need to call the one and the only one 'main'?" will automatically
> disappear.
> Of course, "git bisect" and "gitk --bisect" does not have to change if
> we go that
> route.
> 
> And there will not be any backward compatibility worries. If you are not
> using multiple worktrees, you will see them as refs/bisect/*, just at the
> same location as you are familiar with.

Yes, this is a good point.

Re: [PATCH/RFC 0/2] bisect per-worktree

[Michael Haggerty]

On 08/01/2015 07:12 AM, Junio C Hamano wrote:
> On Fri, Jul 31, 2015 at 8:59 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>
>> It seems to me that adding a new top-level "worktree-refs" directory is
>> pretty traumatic. Lots of people and tools will have made the assumption
>> that all "normal" references live under "refs/".
>> ...
>> It's all a bit frightening, frankly.
> 
> I actually feel the prospect of pluggable ref backend more frightening,
> frankly ;-). These bisect refs are just like FETCH_HEAD and MERGE_HEAD,
> not about the primary purpose of the "repository" to grow the history of refs
> (branches), but about ephemeral pointers into the history used to help keep
> track of what is being done in the worktree upstairs. There is no need for
> these to be visible across worktrees. If we use the real refs that are grobal
> in the repository (as opposed to per-worktree ones), we would hit the backend
> databas with transactions to update these ephemeral things, which somehow
> makes me feel stupid.

Hmm, ok, so you are thinking of a remote database with high latency. I
was thinking more of something like LMDB, with latency comparable to
filesystem storage.

These worktree-specific references might be ephemeral, but they also
imply reachability, which means that they need to be visible at least
during object pruning. Moreover, if the references don't live in the
same database with the rest of the references, then we have to deal with
races due to updating references in different places without atomicity.

The refs+object store is the most important thing for maintaining the
integrity of a repo and avoiding races. To me it seems easier to do so
if there is a single refs+objects store than if we have some references
over here on the file system, some over there in a LMDB, etc. So my gut
feeling is for the primary reference storage to be in a single reference
namespace that (at least in principle) can be stored in a single ACID
database.

For each worktree, we could then create a different view of the
references by splicing parts of the full reference namespace together.
This could even be based on config settings so that we don't have to
hardcode information like "refs/bisect/* is worktree-specific" deep in
the references module. Suppose we could write

[worktree.refs]
	map = refs/worktrees/*:
	map = refs/bisect/*:refs/worktrees/[worktree]/refs/bisect/*

which would mean (a) hide the references under refs/worktrees", and (b)
make it look as if the references under
refs/worktrees/[worktree]/refs/bisect actually appear under refs/bisect
(where "[worktree]" is replaced with the current worktree's name). By
making these settings configurable, we allow other projects to define
their own worktree-specific reference namespaces too.

The corresponding main repo might hide "refs/worktrees/*" but leave its
refs/bisect namespace exposed in the usual place.

"git prune" would see the whole namespace as it really is so that it can
compute reachability correctly.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH/RFC 0/2] bisect per-worktree

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Hmm, ok, so you are thinking of a remote database with high latency. I
> was thinking more of something like LMDB, with latency comparable to
> filesystem storage.

Not necessarily.  The comment was more from conceptual point: "Why
share what needs not to be shared?"

> These worktree-specific references might be ephemeral, but they also
> imply reachability, which means that they need to be visible at least
> during object pruning.

True, but for bisect in particular, that is moot.  You are in
sightseer mode, marking various (older) points in time of an
existing history that is already anchored by branches and tags.

> Moreover, if the references don't live in the
> same database with the rest of the references, then we have to deal with
> races due to updating references in different places without atomicity.

Does that still hold true in the context of bisection?  You do not
even want atomicity to get in the way when you found this old commit
is bad and are about to mark the next one for testing by checking it
out.  You still want to mark the "bad" one (as it being bad is already
an established fact after testing), even if the subsequent checkout
of another commit (i.e. update to "HEAD") fails.

But these two comments above do take advantage of the fact that we
are discussing "bisect" and nothing else.  As a set of points to
consider in a more general context, I do agree that there are refs
and ref-like things that are per-worktree but still has to be part
of reachability, which "HEAD" is the prime example ;-), and some of
them may want to be part of a transaction.

I however think what David has been calling pseudo-refs falls more
in the "ephemeral and unnecessary to participate in reachabliity or
transaction" category (e.g. CHERRY_PICK_HEAD).  And I think the refs
used during bisection falls into the same category.

> For each worktree, we could then create a different view of the
> references by splicing parts of the full reference namespace together.
> ...
> "git prune" would see the whole namespace as it really is so that it can
> compute reachability correctly.

I really like this as a mechanism to include refs/ hierarchy, some
part of which is shared and some part of which is private.

I do not think bisect or pseudorefs needs that, though.

Re: [PATCH/RFC 0/2] bisect per-worktree

[Duy Nguyen]

On Sat, Aug 1, 2015 at 1:51 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> For each worktree, we could then create a different view of the
> references by splicing parts of the full reference namespace together.
> This could even be based on config settings so that we don't have to
> hardcode information like "refs/bisect/* is worktree-specific" deep in
> the references module. Suppose we could write
>
> [worktree.refs]
>         map = refs/worktrees/*:
>         map = refs/bisect/*:refs/worktrees/[worktree]/refs/bisect/*

Perhaps the destination should be worktrees/[worktree]/refs/bisect/*.
Does it have to start with "refs/"?

> which would mean (a) hide the references under refs/worktrees", and (b)
> make it look as if the references under
> refs/worktrees/[worktree]/refs/bisect actually appear under refs/bisect
> (where "[worktree]" is replaced with the current worktree's name). By
> making these settings configurable, we allow other projects to define
> their own worktree-specific reference namespaces too.
>
> The corresponding main repo might hide "refs/worktrees/*" but leave its
> refs/bisect namespace exposed in the usual place.
>
> "git prune" would see the whole namespace as it really is so that it can
> compute reachability correctly.

For multiple wortrees, first we basically remap paths in .git,
relocate some to worktrees/[worktree]/ (already done), then we're
going to remap config keys (submodule support, just proof of concept
so far), remapping ref paths sounds like the logical next step if some
under refs/ needs to be worktree-specific. I wonder if current ref
namespace code could be extended to do this?
-- 
Duy

Re: [PATCH/RFC 0/2] bisect per-worktree

[David Turner]

On Sat, 2015-08-01 at 08:51 +0200, Michael Haggerty wrote:
> On 08/01/2015 07:12 AM, Junio C Hamano wrote:
> > On Fri, Jul 31, 2015 at 8:59 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> >>
> >> It seems to me that adding a new top-level "worktree-refs" directory is
> >> pretty traumatic. Lots of people and tools will have made the assumption
> >> that all "normal" references live under "refs/".
> >> ...
> >> It's all a bit frightening, frankly.
> > 
> > I actually feel the prospect of pluggable ref backend more frightening,
> > frankly ;-). These bisect refs are just like FETCH_HEAD and MERGE_HEAD,
> > not about the primary purpose of the "repository" to grow the history of refs
> > (branches), but about ephemeral pointers into the history used to help keep
> > track of what is being done in the worktree upstairs. There is no need for
> > these to be visible across worktrees. If we use the real refs that are grobal
> > in the repository (as opposed to per-worktree ones), we would hit the backend
> > databas with transactions to update these ephemeral things, which somehow
> > makes me feel stupid.
> 
> Hmm, ok, so you are thinking of a remote database with high latency. I
> was thinking more of something like LMDB, with latency comparable to
> filesystem storage.
> 
> These worktree-specific references might be ephemeral, but they also
> imply reachability, which means that they need to be visible at least
> during object pruning. Moreover, if the references don't live in the
> same database with the rest of the references, then we have to deal with
> races due to updating references in different places without atomicity.
> 
> The refs+object store is the most important thing for maintaining the
> integrity of a repo and avoiding races. To me it seems easier to do so
> if there is a single refs+objects store than if we have some references
> over here on the file system, some over there in a LMDB, etc. So my gut
> feeling is for the primary reference storage to be in a single reference
> namespace that (at least in principle) can be stored in a single ACID
> database.
>
> For each worktree, we could then create a different view of the
> references by splicing parts of the full reference namespace together.
> This could even be based on config settings so that we don't have to
> hardcode information like "refs/bisect/* is worktree-specific" deep in
> the references module. Suppose we could write
> 
> [worktree.refs]
> 	map = refs/worktrees/*:
> 	map = refs/bisect/*:refs/worktrees/[worktree]/refs/bisect/*
> 
> which would mean (a) hide the references under refs/worktrees", and (b)
> make it look as if the references under
> refs/worktrees/[worktree]/refs/bisect actually appear under refs/bisect
> (where "[worktree]" is replaced with the current worktree's name). By
> making these settings configurable, we allow other projects to define
> their own worktree-specific reference namespaces too.
> 
> The corresponding main repo might hide "refs/worktrees/*" but leave its
> refs/bisect namespace exposed in the usual place.
> 
> "git prune" would see the whole namespace as it really is so that it can
> compute reachability correctly.

I think making this configurable is (a) overkill and (b) dangerous.
It's dangerous because the semantics of which refs are per-worktree is
important to the correct operation of git, and allowing users to mess
with it seems like a big mistake.  Instead, we should figure out a
simple scheme and define it globally.

I think refs/worktree -> refs/worktrees/[worktree]/ would do fine as a
fixed scheme, if we go that route.

We would need two separate views of the refs hierarchy, though: one used
by prune (and pack-refs) that is non-mapped (that is, includes
per-worktree refs for each worktree), and one for general use that is
mapped.   Maybe this is just a flag to the ref traversal functions.

But I'm not sure that this is really the right way to go.

As I understand it, we don't presently do many transactions that include
both pseudorefs or per-worktree refs and other refs.  And we definitely
don't want to move pseudorefs into the database since there's so much
code that assumes they're files.  Also, the vast majority of refs are
common, rather than per-worktree.  In fact, the only per-worktree refs
I've seen mentioned so far are the bisect refs and NOTES_MERGE_REF and
HEAD.  Of these, only HEAD is needed for pruning. Are there more that I
haven't thought of?

So I'm not sure the gain from moving per-worktree refs into the database
is that great.

There are some downsides of moving per-worktree refs into the database:

1. More operations in one worktree can now contend with operations in
another worktree for the database.  LMDB only allows a single write
transaction at a time.  

2. The refs API would be more complicated: it would need to deal with
remapped vs raw ref paths.  Refs backends would need to have functions
to prune per-worktree data when a worktree is destroyed. 

4. We would still need to deal with pseudorefs, so there's still some
missing transactional safety, and still the complication of dealing with
files on the filesystem.

Simply treating refs/worktree as per-worktree, while the rest of refs/
is not, would be a few dozen lines of code.  The full remapping approach
is likely to be a lot more. I've already got the lmdb backend working
with something like this approach.  If we decide on a complicated
approach, I am likely to run out of time to work on pluggable backends.

Re: [PATCH/RFC 0/2] bisect per-worktree

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> I think making this configurable is (a) overkill and (b) dangerous.
> It's dangerous because the semantics of which refs are per-worktree is
> important to the correct operation of git, and allowing users to mess
> with it seems like a big mistake.  Instead, we should figure out a
> simple scheme and define it globally.
>
> I think refs/worktree -> refs/worktrees/[worktree]/ would do fine as a
> fixed scheme, if we go that route.

OK.

> We would need two separate views of the refs hierarchy, though: one used
> by prune (and pack-refs) that is non-mapped (that is, includes
> per-worktree refs for each worktree), and one for general use that is
> mapped.   Maybe this is just a flag to the ref traversal functions.

True.  Alternatively we could just view refs/worktree/* as if they
are symbolic refs that point into refs/worktrees/$my_worktree/*, but
that would imply making the latter always visible to all worktrees,
which would hurt when people use it to interact with outside world
(namely, refs in other people's private area should probably not be
advertised).

> As I understand it, we don't presently do many transactions that include
> both pseudorefs or per-worktree refs and other refs.  And we definitely
> don't want to move pseudorefs into the database since there's so much
> code that assumes they're files.  Also, the vast majority of refs are
> common, rather than per-worktree.  In fact, the only per-worktree refs
> I've seen mentioned so far are the bisect refs and NOTES_MERGE_REF and
> HEAD.  Of these, only HEAD is needed for pruning. Are there more that I
> haven't thought of?

I myself have come up with nothing other than the above.  Let's hear
from others.

> So I'm not sure the gain from moving per-worktree refs into the database
> is that great.

I am on the same wavelength as you are on this.

> There are some downsides of moving per-worktree refs into the database:
> ...

All good points except #3, which I cannot judge if it is good or bad.

> ...
> Simply treating refs/worktree as per-worktree, while the rest of refs/
> is not, would be a few dozen lines of code.  The full remapping approach
> is likely to be a lot more.

We may be over-engineering with Michael's and even with the more
simpler refs/worktree/* -> refs/worktrees/$mine/* fixed mapping;
I tend to agree with you.

Re: [PATCH/RFC 0/2] bisect per-worktree

[Duy Nguyen]

On Tue, Aug 4, 2015 at 2:49 AM, David Turner <dturner@twopensource.com> wrote:
> Simply treating refs/worktree as per-worktree, while the rest of refs/
> is not, would be a few dozen lines of code.  The full remapping approach
> is likely to be a lot more. I've already got the lmdb backend working
> with something like this approach.  If we decide on a complicated
> approach, I am likely to run out of time to work on pluggable backends.

I think you still have another option: decide that lmdb backend does
not (yet) support multiple worktrees (which means make "git worktree
add" reject when lmdb backend is used). That would simplify some for
you and we can continue on at a later time.
-- 
Duy

Re: [PATCH/RFC 0/2] bisect per-worktree

[David Turner]

On Tue, 2015-08-04 at 06:09 +0700, Duy Nguyen wrote:
> On Tue, Aug 4, 2015 at 2:49 AM, David Turner <dturner@twopensource.com> wrote:
> > Simply treating refs/worktree as per-worktree, while the rest of refs/
> > is not, would be a few dozen lines of code.  The full remapping approach
> > is likely to be a lot more. I've already got the lmdb backend working
> > with something like this approach.  If we decide on a complicated
> > approach, I am likely to run out of time to work on pluggable backends.
> 
> I think you still have another option: decide that lmdb backend does
> not (yet) support multiple worktrees (which means make "git worktree
> add" reject when lmdb backend is used). That would simplify some for
> you and we can continue on at a later time.

Some of our developers are pretty excited about multiple worktrees, so I
don't really want to do that.  Also, it's easier to develop when more of
the tests pass under the LMDB backend (no need to investigate whether
worktrees are the reason for failures when there are no failures).

Re: [PATCH/RFC 0/2] bisect per-worktree

[Duy Nguyen]

On Sat, Aug 1, 2015 at 10:59 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Either way, there's also the question of who should know how to find the
> worktree-specific references--the program that wants to access them, or
> should there be a secret invisible mapping that is done on lookup, and
> that knows the full list of references that want to be worktree-specific
> (for example, that in a linked worktree, "refs/bisect/*" should be
> silently redirected to "refs/worktree/<name>/bisect/*")?
>
> It's all a bit frightening, frankly.

I would think all work to make pluggable backends happen should allow
us to do this kind of transparent mapping. We can add a new file-based
backend with just this redirection, can't we? I haven't read the
updated refs.c though, need to do it now..
-- 
Duy

Re: [PATCH/RFC 0/2] bisect per-worktree

[Duy Nguyen]

On Mon, Aug 3, 2015 at 8:02 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sat, Aug 1, 2015 at 10:59 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Either way, there's also the question of who should know how to find the
>> worktree-specific references--the program that wants to access them, or
>> should there be a secret invisible mapping that is done on lookup, and

After re-reading some code, I think this invisible mapping on lookup
is already done for our file-based ref backend. Take
resolve_ref_unsafe_1 for example, the call strbuf_git_path() may
return ".git/worktrees/foo/HEAD", not ".git/HEAD", given the input ref
"HEAD". So ref names are already separated from where they are stored.

>> that knows the full list of references that want to be worktree-specific
>> (for example, that in a linked worktree, "refs/bisect/*" should be
>> silently redirected to "refs/worktree/<name>/bisect/*")?

My decision to hard code these paths may turn out a bad thing (it's
common_list[] in path.c). Perhaps I should let the user extend them,
but we will not allow to override a small subset of paths because that
may break stuff horribly.

>> It's all a bit frightening, frankly.
>
> I would think all work to make pluggable backends happen should allow
> us to do this kind of transparent mapping. We can add a new file-based
> backend with just this redirection, can't we? I haven't read the
> updated refs.c though, need to do it now..
-- 
Duy