Re: What's cooking in git.git (May 2019, #05; Thu, 30)
On Thu, May 30, 2019 at 5:20 PM Junio C Hamano wrote: > > * nb/branch-show-other-worktrees-head (2019-05-07) 3 commits > - branch: add worktree info on verbose output > - branch: update output to include worktree info > - ref-filter: add worktreepath atom > > "git branch --list" learned to show branches that are checked out > in other worktrees connected to the same repository prefixed with > '+', similar to the way the currently checked out branch is shown > with '*' in front. > I see that this has been in 'pu' for some time now. Is there something holding it up from going into 'next'?
Re: [PATCH v10 0/3]
Sorry, I'm not very accustomed to mailing list development. I had assumed that this was being threaded with the other messages in the series, hence leaving the subject blank and only putting new info in the body. In the future I'll add in an appropriate subject and brief re-hash in the body. Thanks for bringing it up. Nickolai On Mon, Apr 29, 2019 at 1:57 PM Johannes Schindelin wrote: > > Hi, > > am I the only person who is puzzled every time this patch series with a > completely empty subject and without any further explanation about the > intent in the mail body is sent? > > Ciao, > Johannes > > > On Sun, 28 Apr 2019, nbelakov...@gmail.com wrote: > > > From: Nickolai Belakovski > > > > Added test_i18ncmp per instructions from Szeder > > > > Is there some other part of the infrastructure that's testing for this? > > Because it did not fail in any of my Travis CI builds. > > > > Travis CI results: https://travis-ci.org/nbelakovski/git/builds/525801210 > > > > Nickolai Belakovski (3): > > ref-filter: add worktreepath atom > > branch: update output to include worktree info > > branch: add worktree info on verbose output > > > > Documentation/git-branch.txt | 12 -- > > Documentation/git-for-each-ref.txt | 5 +++ > > builtin/branch.c | 16 ++-- > > ref-filter.c | 78 > > ++ > > t/t3200-branch.sh | 16 +--- > > t/t3203-branch-output.sh | 43 - > > t/t6302-for-each-ref-filter.sh | 13 +++ > > 7 files changed, 168 insertions(+), 15 deletions(-) > > > > -- > > 2.14.2 > > > >
Re: [PATCH v10 0/3]
> Testing with GETTEXT_POISON was broken since 6cdccfce (i18n: make > GETTEXT_POISON a runtime option, 2018-11-08), and didn't catch any of > these issues. See: > > > https://public-inbox.org/git/xmqqlg0bvppc.fsf...@gitster-ct.c.googlers.com/T/#u > > The fix f88b9cb603 (gettext tests: export the restored > GIT_TEST_GETTEXT_POISON, 2019-04-15) was merged to master just last > week. > Ah I see, thanks for the info.
Re: [PATCH v8 0/3]
> > Patch 1 looks good to me. Given that we're on v8 and most of the other > comments are for patches 2 and 3, I think we might consider graduating > it separately if the other two are not ready soon. It's independently > useful, IMHO. Patch 2 was my main motivation, so it would be nice to get it in together with 1 :) Patch 3, like I said in the thread on that one I don't have strong feeling about it. It was an attempt to provide a connection between the new cyan output and its intent, as opposed to having the user guess, but I think anyone who's using a worktree will figure it out sooner or later, and anyone not using a worktree will be unaffected. I'm willing to keep going with comments on patch 2. I can't imagine it would take many more revisions as it's much more straightforward than patch 1, it's basically just modifying one line of branch.c. If we decide to drop patch 3 fine with me. I'll send in v9 with the latest changes for both 2 and 3 sometime tomorrow unless I hear otherwise. Thanks for the feedback.
Re: [PATCH v8 3/3] branch: add worktree info on verbose output
On Thu, Feb 21, 2019 at 4:59 AM Jeff King wrote:
>
> On Tue, Feb 19, 2019 at 05:31:23PM +0900, nbelakov...@gmail.com wrote:
>
> > From: Nickolai Belakovski
> >
> > To display worktree path for refs checked out in a linked worktree
>
> This would be a good place to describe why this is useful. :)
>
> I do not have an opinion myself. Patch 2 makes a lot of sense to me, but
> I don't know if people would like this one or not. I don't use "-v"
> myself, though, so what do I know. :)
I threw this one in because I thought it wouldn't be clear to the
average user why some
branches are in cyan. By putting the worktree path in cyan on the next
level of output
I thought this would help the user make the connection, but actually I
don't have strong
feelings about this one.
>
> > diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> > index f2e5a07d64..326a45f648 100644
> > --- a/Documentation/git-branch.txt
> > +++ b/Documentation/git-branch.txt
> > @@ -168,8 +168,10 @@ This option is only applicable in non-verbose mode.
> > When in list mode,
> > show sha1 and commit subject line for each head, along with
> > relationship to upstream branch (if any). If given twice, print
> > - the name of the upstream branch, as well (see also `git remote
> > - show `).
> > + the path of the linked worktree, if applicable (not applicable
> > + for current worktree since user's path will already be in current
> > + worktree) and the name of the upstream branch, as well (see also
> > + `git remote show `).
>
> That parenthetical feels a bit awkward. Maybe:
>
> ...print the path of the linked worktree (if any) and the name of the
> upstream branch, as well (see also `git remote show `). Note
> that the current worktree's HEAD will not have its path printed (it
> will always be your current directory).
Sure I can make that change
>
> > diff --git a/builtin/branch.c b/builtin/branch.c
> > index c2a86362bb..0b8ba9e4c5 100644
> > --- a/builtin/branch.c
> > +++ b/builtin/branch.c
> > @@ -367,9 +367,13 @@ static char *build_format(struct ref_filter *filter,
> > int maxwidth, const char *r
> > strbuf_addf(&local, " %s ", obname.buf);
> >
> > if (filter->verbose > 1)
> > + {
> > + strbuf_addf(&local,
> > "%%(if:notequals=*)%%(HEAD)%%(then)%%(if)%%(worktreepath)%%(then)(%s%%(worktreepath)%s)
> > %%(end)%%(end)",
> > + branch_get_color(BRANCH_COLOR_WORKTREE),
> > branch_get_color(BRANCH_COLOR_RESET));
> > strbuf_addf(&local,
> > "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
> > "%%(then):
> > %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)",
> > branch_get_color(BRANCH_COLOR_UPSTREAM),
> > branch_get_color(BRANCH_COLOR_RESET));
> > + }
>
> Another unreadable long line (both the one you're adding, and the existing
> one!). I don't know if it's worth trying to clean these up, but if we
> do, it might be worth hitting the existing ones, too.
>
> I'm OK if that comes as a patch on top later on, though.
Agreed, but there's enough lines like this that it'll just look
inconsistent if only one were broken up.
>
>
> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> index 012ddde7f2..8065279be6 100755
> --- a/t/t3203-branch-output.sh
> +++ b/t/t3203-branch-output.sh
> @@ -284,22 +284,20 @@ test_expect_success '--color overrides auto-color' '
> test_cmp expect.color actual
> '
>
> -# This test case has some special code to strip the first 30 characters or so
> -# of the output so that we do not have to put commit hashes into the expect
> test_expect_success 'verbose output lists worktree path' '
> + one=$(git rev-parse --short HEAD) &&
> + two=$(git rev-parse --short master) &&
> cat >expect <<-EOF &&
> - one
> - one
> - two
> - one
> - two
> - ($(pwd)/worktree_dir) two
> - two
> - two
> + * (HEAD detached from fromtag) $one one
> + ambiguous$one one
> + branch-one $two two
> + branch-two $one one
> + master $two two
> + + master_worktree
Re: [PATCH v8 2/3] branch: update output to include worktree info
On Thu, Feb 21, 2019 at 4:44 AM Jeff King wrote: > > On Tue, Feb 19, 2019 at 05:31:22PM +0900, nbelakov...@gmail.com wrote: > > > From: Nickolai Belakovski > > > > The output of git branch is modified to mark branches checkout out in a > > s/checkout out/checked out/ ? > Yes, thanks > > > - strbuf_addf(&local, "%%(if)%%(HEAD)%%(then)* %s%%(else) %s%%(end)", > > - branch_get_color(BRANCH_COLOR_CURRENT), > > - branch_get_color(BRANCH_COLOR_LOCAL)); > > + strbuf_addf(&local, "%%(if)%%(HEAD)%%(then)* > > %s%%(else)%%(if)%%(worktreepath)%%(then)+ %s%%(else) %s%%(end)%%(end)", > > + branch_get_color(BRANCH_COLOR_CURRENT), > > + branch_get_color(BRANCH_COLOR_WORKTREE), > > + branch_get_color(BRANCH_COLOR_LOCAL)); > > Makes sense. The long line is ugly. Our format does not support breaking > long lines, though we could break the C string, I think, like: > > strbuf_add(&local, > "%%(if)%%(HEAD)" >"%%(then)* %s" > "%%(else)%(if)%%(worktreepath)" >"%%(then)+ %s" > "%%(else)" >"%%(then) %s" > "%%(end)%%(end)"); > > That's pretty ugly, too, but it at least shows the conditional > structure. True, but other lines within that file are about as long. I'd feel that I should make all of them reflect the conditional structure if I'm going to make one of them reflect it. Granted none of the others have nested if's, but personally I think it's OK as is. The nested if is short enough. > > > > > +test_expect_success '"add" a worktree' ' > > + mkdir worktree_dir && > > + git worktree add -b master_worktree worktree_dir master > > +' > > This mkdir gets deleted in the next patch. It should just not be added > here. Whoops, removed > > > +cat >expect <<'EOF' > > +* (HEAD detached from fromtag) > > + ambiguous > > + branch-one > > + branch-two > > + master > > ++ master_worktree > > + ref-to-branch -> branch-one > > + ref-to-remote -> origin/branch-one > > +EOF > > +test_expect_success TTY 'worktree colors correct' ' > > + test_terminal git branch >actual.raw && > > + test_decode_color actual && > > + test_cmp expect actual > > +' > > We are not testing the auto-color behavior here, so I think you could > just use "git branch --color >actual.raw" and drop the TTY prerequisite. > That's shorter and simpler, and will let your test run on more > platforms. > Done locally, will be part of v9 > -Peff
Re: [PATCH v7 1/3] ref-filter: add worktreepath atom
Well, it sounded like we didn't like the ":" extender from another conversation on this thread. Do you think this patch should move back in that direction? On Tue, Feb 5, 2019 at 3:14 AM Junio C Hamano wrote: > > Nickolai Belakovski writes: > > > There's been a little back and forth on it, but my understanding is > > that using the colon separator bypasses the caching mechanism in the > > atoms, so every instance of "worktree:path" in a format string would > > require a lookup. > > Would that be a problem, though? You now have a singleton hashmap > that is a file-scope global not tied to any particular atom, so...?
Re: [PATCH v7 0/3]
On Fri, Feb 1, 2019 at 2:54 PM Junio C Hamano wrote: > > > As you can see in "git shortlog --no-merges", later two patches > would look quite out of place by having overlong title and starting > the description(i.e. after ": ") in a capital letter. Hadn't looked at it that way. OK, will shorten/uncapitalize. > > It is still not clear why we would want 2/3, even though I think 3/3 > is a good idea. > It's interesting to me that you like 3/3 but not 2/3 :) My apologies for restating the commit message, but the point of 2/3 is to communicate to the user that highlighted/marked branches will behave differently from unhighlighted/unmarked branches for commands to check out or delete. I think this is useful since it gives the user actionable information ahead of time, as opposed to providing that information upon failure of checkout/delete. It also makes sense since 'git branch' is already highlighting the current branch, i.e. this is just an extension of that idea. As we've stated earlier in this thread, 1/3 allows for users to implement this on their own with a custom git branch format. Personally I think there's value in making it default, since it's adding information in a minimally intrusive way. I do believe that merely adding information isn't a good enough reason to change things, as information overload is a real thing, but in this case the output isn't changed for anyone not using a worktree (same goes for 3/3), and for someone using a worktree this provides useful and actionable information, IMO. Does that change your mind at all?
Re: [PATCH v7 2/3] branch: Mark and color a branch differently if it is checked out in a linked worktree
On Fri, Feb 1, 2019 at 2:54 PM Junio C Hamano wrote: > > > I had to apply and then use --color-words to see what is going on. > Please avoid unnecessary reflowing of the text that makes the patch > harder than necessary to read. I was trying to keep the line length consistent. How else can I accomplish that?
Re: [PATCH v7 3/3] branch: Add an extra verbose output displaying worktree path for refs checked out in a linked worktree
On Fri, Feb 1, 2019 at 2:54 PM Junio C Hamano wrote: > > If the rule were "a branch that is checked out in one of the > worktrees connected to the repository is shown with the path to that > worktree" (i.e. no exception), I would understand it. If the rule > were "a branch that is ... (the same sentence), unless it is the > branch that is checked out in the *current* worktree", then I would > understand it too. > It is the latter, as in, yes, I meant "current". I will update the docs as such. > > In any case, please add a test or two to protect this feature from > unintended future breakages. > > Thanks. > Will do.
Re: [PATCH v7 3/3] branch: Add an extra verbose output displaying worktree path for refs checked out in a linked worktree
On Fri, Feb 1, 2019 at 2:28 PM Eric Sunshine wrote: > > On Fri, Feb 1, 2019 at 5:04 PM wrote: > > Subject: branch: Add an extra verbose output displaying worktree path for > > refs checked out in a linked worktree > > Overlong subject. Perhaps shorten it to: > > branch: display worktree path in -v -v mode > > or something, and use the longer description as the rest of the body > of the commit message. OK > > > Signed-off-by: Nickolai Belakovski > > --- > > diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt > > @@ -167,8 +167,10 @@ This option is only applicable in non-verbose mode. > > When in list mode, > > show sha1 and commit subject line for each head, along with > > relationship to upstream branch (if any). If given twice, print > > - the name of the upstream branch, as well (see also `git remote > > - show `). > > + the path of the linked worktree, if applicable (not applicable > > + for main worktree since user's path will already be in main > > + worktree) and the name of the upstream branch, as well (see also > > + `git remote show `). > > I'm not sure I understand the "not applicable" explanation. When you > say "user's path", do you mean the current working directory? What > happens if the command is invoked from within one of the linked > worktrees (not from within the main worktree)? I should correct that to say "current worktree" as opposed to main, since that's what HEAD will give. And yes user's path means cwd. Does that make more sense?
Re: [PATCH v7 1/3] ref-filter: add worktreepath atom
On Fri, Feb 1, 2019 at 2:20 PM Eric Sunshine wrote:
>
> On Fri, Feb 1, 2019 at 5:04 PM wrote:
> > Add an atom providing the path of the linked worktree where this ref is
> > checked out, if it is checked out in any linked worktrees, and empty
> > string otherwise.
> >
> > Signed-off-by: Nickolai Belakovski
> > ---
> > diff --git a/Documentation/git-for-each-ref.txt
> > b/Documentation/git-for-each-ref.txt
> > @@ -214,6 +214,11 @@ symref::
> > +worktreepath::
> > + The absolute path to the worktree in which the ref is checked
> > + out, if it is checked out in any linked worktree. Empty string
> > + otherwise.
>
> This may have been asked previously, but is there a reason this name
> was chosen over the more extensible "worktree:" with "path" as a
> modifier (i.e. "worktree:path")? I scanned the thread a couple weeks
> ago and did see mention of "worktree:path" but did not find any
> followup. I ask because it's conceivable that someone in the future
> might want to retrieve other information about the worktree beyond its
> path (such as whether it's bare or detached, etc.). By using the form
> "worktree:", we leave that door open. (I'm not suggesting that
> this patch series needs to implement fetching of any of the other
> worktree properties, but just asking if "worktree:" should be
> considered.)
>
There's been a little back and forth on it, but my understanding is
that using the colon separator bypasses the caching mechanism in the
atoms, so every instance of "worktree:path" in a format string would
require a lookup. Future atoms should be along the lines of
"worktreeisdetached", "worktreeisbare", etc. This is consistent with
several of the other atoms, like objecttype/size/name,
comitter/name/email/date.
> > diff --git a/ref-filter.c b/ref-filter.c
> > @@ -1562,6 +1628,13 @@ static int populate_value(struct ref_array_item
> > *ref, struct strbuf *err)
> > if (starts_with(name, "refname"))
> > refname = get_refname(atom, ref);
> > + else if (starts_with(name, "worktreepath")) {
>
> I think this was brought up previously, but shouldn't this be strcmp()
> rather than starts_with()?
>
> (starts_with() would be appropriate, if you went with the suggested
> "worktree:".)
Not sure about it being brought up previously. starts_with seemed
consistent with other uses but now I see there's several other
instance of strcmp in populate value. Seems like a reasonable thing to
change. I had previously implemented "worktree:" and must've left
it alone after we went with worktreepath.
>
> > diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> > @@ -441,4 +441,19 @@ test_expect_success '--merged is incompatible with
> > --no-merged' '
> > +test_expect_success '"add" a worktree' '
> > + mkdir worktree_dir &&
> > + git worktree add -b master_worktree worktree_dir master
> > +'
>
> I don't think 'mkdir' is needed since "git worktree add" should create
> the directory itself.
>
> > +test_expect_success 'validate worktree atom' '
> > + cat >expect <<-EOF &&
> > + master: $(pwd)
> > + master_worktree: $(pwd)/worktree_dir
> > + side: not checked out
> > + EOF
> > + git for-each-ref --format="%(refname:short):
> > %(if)%(worktreepath)%(then)%(worktreepath)%(else)not checked out%(end)"
> > refs/heads/ >actual &&
> > + test_cmp expect actual
> > +'
>
> If this is the only test using that newly-created worktree, it might
> make sense to squash the two tests together.
Sure, can do, on both points.
Re: [PATCH v6 1/3] ref-filter: add worktreepath atom
So where does that leave us for this series? We could move hashmap back into used_atom, but if a user entered --format="%(worktreepath)%(worktreepath:)" we'd end up freeing worktrees twice. Not that that should stop us - that scenario is one where user input isn't sensible and personally I don't think it's necessary to protect against such things (unless the user was reasonably confused, but I don't see that as the case here). I agree with Jeff that a ref-filter "context" would help. And in more ways than one, it could help us decide ahead of time whether to check if a ref is a branch or a tag before doing a hashmap lookup or just skip the check (i.e. if there are no tags within the context, the check would only add cost). But I do believe that that would be outside the scope of this series. I think leaving it as globals is a tiny bit safer and also makes it easier to pack it into a context if/when we decide to do that work, but as always I'm open to other interpretations. On Thu, Jan 24, 2019 at 1:26 PM Jeff King wrote: > > On Thu, Jan 24, 2019 at 11:30:20AM -0800, Junio C Hamano wrote: > > > Jeff King writes: > > > > > What if you have other atoms that need worktrees? E.g., does > > > %(worktreepath:foo) use the same used_atom slot? What if we have another > > > worktree-related atom? > > > ... > > > And that one is a good example where we _do_ need the global, because we > > > already have multiple atoms pulling from it. > > > > I guess that we broke the original atom design by mistake when we > > added ":" support. There should have been one layer of > > indirection that binds the instances of the same atom with different > > modifiers together---I agree with you that we cannot avoid globals > > without fixing that mistake first. > > Yes, that's one way to fix it. > > I actually think the biggest mistake is having that used_atoms list in > the first place, as we iterate over it several times asking "can we fill > this in yet?". The way pretty.c does it is just to incrementally parse > the commit, saving intermediate results. And in cat-file.c, we figure > out what we need ahead of time in a single pass, and then just fill it > in for each object (which sort of works out the same, but doesn't > require that the parsing needed for item X is a strict superset of item > Y). > > So I'd much rather see us parse the format into a real tree of nodes, > and figure out (once) which properties of each object are required to > fulfill that. Then for each object, we grab those properties, and then > walk the tree to generate the output string. > > -Peff
Re: [PATCH v6 1/3] ref-filter: add worktreepath atom
On Wed, Jan 23, 2019 at 10:57 AM Junio C Hamano wrote:
>
> Missing sign-off?
>
My mistake, forgot -s on format-patch. Will remember to add it next go-around.
> > +static int ref_to_worktree_map_cmpfnc(const void *unused_lookupdata, const
> > void *existing_hashmap_entry_to_test,
> > +const void *key, const void
> > *keydata_aka_refname)
> > +{
> > + const struct ref_to_worktree_entry *e =
> > existing_hashmap_entry_to_test;
> > + const struct ref_to_worktree_entry *k = key;
> > + return strcmp(e->wt->head_ref, keydata_aka_refname ?
> > keydata_aka_refname : k->wt->head_ref);
>
> Overlong line.
>
> > +}
>
OK, should I shorten the function signature as well? Is it too long
because it's 101 characters and you're trying to stick to 100?
>
> > +
> > +static struct hashmap ref_to_worktree_map;
> > +static struct worktree **worktrees = NULL;
>
> No need to initialize static vars to 0/NULL.
OK
>
> We do not need a strbuf to do the above, do we?
>
> hashmap_entry_init(...);
> lookup_result = hashmap_get(...);
> if (lookup_result)
> return xstrdup(lookup_result->wt->path);
> else
> return xstrdup("");
>
> or something like that, perhaps?
>
Sure.
> > @@ -1562,6 +1624,13 @@ static int populate_value(struct ref_array_item
> > *ref, struct strbuf *err)
> >
> > if (starts_with(name, "refname"))
> > refname = get_refname(atom, ref);
> > + else if (starts_with(name, "worktreepath")) {
> > + if (ref->kind == FILTER_REFS_BRANCHES)
> > + v->s = get_worktree_path(atom, ref);
> > + else
> > + v->s = xstrdup("");
> > + continue;
> > + }
>
> I am wondering if get_worktree_path() being called should be the
> triggering event for lazy initialization worktree_atom_parser() is
> doing in this patch, instead of verify_ref_format() seeing the
> "%(worktreepath)" atom. Is there any codepath that wants to make
> sure the lazy initialization is done without/before populate_value()
> sees a use of the "%(worktreepath)" atom? If so, such a plan would
> not work, but otherwise, we can do the following:
>
> - rename worktree_atom_parser() to lazy_init_worktrees() or
>something, and remove all of its unused parameters.
>
> - remove parser callback for "worktreepath" from valid_atom[].
>
> - call lazy_inti_worktrees() at the beginning of
>get_worktree_path().
>
> Hmm?
Yes, the parser used the atom argument in an earlier version of this
patch, but we since moved the map out of the atom since it only needs
to exist once globally. Even though we have a caching mechanism for
atoms it still seemed like a logical move to explicitly keep one
instance of the map globally. Will make the change, thanks for taking
a look at it through fresh eyes.
Re: [PATCH v5 1/3] ref-filter: add worktreepath atom
On Fri, Jan 18, 2019 at 2:17 PM Nickolai Belakovski wrote: > > > I think avoiding this would be check, we can simply check ref->kind == > FILTER_REFS_BRANCHES ahead of calling into get_worktree_path and > provide an empty string otherwise. > *would be check -> would be cheap
Re: [PATCH v5 2/3] branch: Mark and color a branch differently if it is checked out in a linked worktree
I will start a separate thread containing these replies for the potential change to allow deleting branches checked out in worktrees. Getting back on track for this series, specifically this 2/3 patch, how do you feel about it? As I pointed out the goal is to communicate to the user that the branches marked/colored will behave differently from the other branches if the user tries to delete them or check them out.
Re: [PATCH v5 1/3] ref-filter: add worktreepath atom
On Mon, Jan 7, 2019 at 10:20 AM Junio C Hamano wrote:
> When seeing a tag or note ref, by definition that's not something we
> can have checked out in any worktree. I wonder if it is worth to
> optimize further by omitting this lookup when ref is not a local
> branch?
>
> IOW, with a typical number of worktrees and refs, how costly would ...
>
> > + hashmap_entry_init(&entry, strhash(ref->refname));
> > + lookup_result = hashmap_get(&ref_to_worktree_map, &entry,
> > ref->refname);
>
> ... this sequence of calls be.
>
It certainly wouldn't be free. Every check would compute the hash of
the refname and do a lookup into the hash table. If the bucket it
looked up was empty that'd be it, but if it were non-empty a few more
comparisons would happen.
I think avoiding this would be check, we can simply check ref->kind ==
FILTER_REFS_BRANCHES ahead of calling into get_worktree_path and
provide an empty string otherwise.
> > free_array_item(array->items[i]);
> > FREE_AND_NULL(array->items);
> > array->nr = array->alloc = 0;
> > + if (worktrees)
> > + {
>
>
> What's the point of ref_array_clear()? ... If that
> is the case, then the added code breaks that hope, as it leaves a
> dangling pointer in the worktrees variable.
>
Discussion of the point of ref_array_clear would be out of scope of
this change, but your point is well taken that setting worktrees to
NULL would be consistent with the rest of the function. Will
implement.
Re: [PATCH v5 2/3] branch: Mark and color a branch differently if it is checked out in a linked worktree
On Thu, Jan 10, 2019 at 1:43 PM Philip Oakley wrote: > > On 07/01/2019 19:04, Junio C Hamano wrote: > > I do not think it is warranted to paint the safety features as > > "limitations". > > Is this not just a case of needing to clarify that this is 'safety' > related to the _users_ mental model (or lack of) relative to the limited > information that was previously given by the branch command's list. > > You are right that there is no data safety issue, but users make > mistakes when they misunderstand the situation. Not trying to paint anything one way or another. I found that these features got in the way of my workflows and didn't see any immediate reason why they had to exist. Thinking about it a bit more, is it unreasonable to delete a branch even if it's checked out in a worktree as long as the user uses git branch --delete --force or -D? This would leave the worktree in a detached head state, but all the data would be untouched. The output of deletion could mention that the branch had been checked out in a worktree so that the user is fully informed. Checking out the same branch in two worktrees should be technically possible to implement safely, but I don't see a use case for having the same branch checked out in multiple worktrees anyway. Why use multiple worktrees at that point? If a user really wants the same contents in two directories they can work around this limitation/safety feature by just making another branch pointing at the same commit. But anyway I'm just explaining why I chose the word 'limitation'. > >If you do want to remove that branch, > > you need to go to that worktree that has a checkout of that branch, > > check out a different branch there, and then remove it. Again, > > knowing where that other worktree is is the fist thing you need to > > know. This just seems silly to me when git branch --delete has a --force option. But that's off topic. > > > >> The git worktree list command contains the relevant information, however > >> this is a much less frquently used command than git branch. > > It is not a good justification. If the "relevant information" given > > by the command is necessary one, the user can run that command. If > > the situation where that "relevant information" becomes necessary is > > rare, the command is run much less frequently is not a problem---it > > is expected. And overloading a more frequently used command with > > information that is less frequently wanted is actually not a great > > design. > But leaving the older command unaware of the newer developments and the > user unwise as to its missing info is equally a poor situation. > > > > A more relevant justification may be that even though the > > information can already be found in "worktree list" output, it would > > give us flexibility in presentation to allow the custom format in > > for-each-ref to show it. > > > > So, I am between moderately Meh to fairly negative on this step; Meh > > in the sense that "thanks to the previous step, we _could_ do this, > > it does not give incorrect information, and it makes the output more > > cheerful, but it does not add that much useful and actionable piece > > of information". Allow me to add some color to my original commit message. The point of this patch is so that the user is not surprised when they see the message that this branch is checked out in another worktree when trying to delete it or check it out, since they have presumably run git branch recently and seen the formatted output indicating that a branch they may want to delete/checkout is checked out in a worktree. This was my frustration that prompted me to dive into this in the first place - I'm cleaning up my branches and all of a sudden git decides it doesn't want to let me delete one because it's checked out somewhere else, even though I know I don't care about it because I know the branch has already been merged upstream, or is old, or whatever. I thought, if git branch output could at least let me know that it's going to treat some branches differently, I can be proactive about things and go to my worktree and delete the branch, or skip trying to clean it up or not check it out. I'll pursue the above-mentioned topic of allowing git branch -D to allow the user to delete branches checked out in a worktree separately, but even if that goes through, I think this patch would still be useful in that it tells me that I can't check out the branches that are colored in cyan. Does that make more sense?
Re: [PATCH v3 1/3] ref-filter: add worktreepath atom
On Tue, Dec 18, 2018 at 9:22 AM Jeff King wrote:
>
> On Sun, Dec 16, 2018 at 01:57:57PM -0800, nbelakov...@gmail.com wrote:
>
> > From: Nickolai Belakovski
> >
> > Add an atom proving the path of the linked worktree where this ref is
> > checked out, if it is checked out in any linked worktrees, and empty
> > string otherwise.
>
> I stumbled over the word "proving" here. Maybe "showing" would be more
> clear?
Oops, providing
> > +worktreepath::
> > + The absolute path to the worktree in which the ref is checked
> > + out, if it is checked out in any linked worktree. ' ' otherwise.
> > +
>
> Also, why are we replacing it with a single space? Wouldn't the empty
> string be more customary (and work with the other "if empty, then do
> this" formatting options)?
I was just following what was done for HEAD, but overall I agree that
empty is preferable to single space, will change.
> Minor style nit: we put the "*" in a pointer declaration next to the
> variable name, without intervening whitespace. Like:
>
> static struct worktree **worktrees;
Gotcha, will do, thanks for pointing it out.
To sum up the hashmap comments:
-I hadn't thought to re-use the head_ref of worktree as the key.
That's clever. I like the readability of having separate entries for
key and value, but I can see the benefit of not having to do an extra
allocation. I can make up for the readability hit with a comment.
-Actually, for any valid use case there will only be one instance of
the map since the entries of used_atom are cached, but regardless it
makes sense to keep per-atom info in used_atom and global context
somewhere else, so I'll make that change to make it a static variable
outside of used_atom.
-Will change the lookup logic to remove the extra allocation. Since
I'm letting the hashmap use its internal comparison function on the
hash, I don't need to provide a comparison function.
> What's this extra strncmp about? If we're _not_ a worktreepath atom,
> we'd still do the lookup only to put nothing in the string?
Leftover from an earlier iteration where I was going to support
getting more info out of the worktree struct. I decided to limit scope
to just the info I really needed for the branch change. I left it like
this because I thought it would make the code more readable for
someone who wanted to come in and add that extra info, but I think
you're right that it ends up just reading kind of awkwardly.
>
> > @@ -2013,7 +2076,14 @@ void ref_array_clear(struct ref_array *array)
> > int i;
> >
> > for (i = 0; i < used_atom_cnt; i++)
> > + {
> > + if (!strncmp(used_atom[i].name, "worktreepath",
> > strlen("worktreepath")))
> > + {
> > + hashmap_free(&(used_atom[i].u.reftoworktreeinfo_map),
> > 1);
> > + free_worktrees(worktrees);
> > + }
>
> And if we move the mapping out to a static global, then this only has to
> be done once, not once per atom. In fact, I think this could double-free
> "worktrees" with your current patch if you have two "%(worktree)"
> placeholders, since "worktrees" already is a global.
Only if someone put a colon on one of the %(worktree) atoms, otherwise
they're all cached, but as you say moot point anyway if the map is
moved outside the used_atom structure.
>
> It's probably worth testing that the path we get is actually sane, too.
> I.e., expect something more like:
>
>cat >expect <<-\EOF
>master: $PWD
>master: $PWD/worktree
>side: not checked out
>EOF
>git for-each-ref \
> --format="%(refname:short):
> %(if)%(worktreepath)%(then)%(worktreepath)%(else)not checked %out%(end)
>
> (I wish there was a way to avoid that really long line, but I don't
> think there is).
>
Yea good call, can do.
Thanks for all the feedback, will try to turn these around quickly.
Re: [PATCH v2 2/2] branch: Mark and colorize a branch differently if it is checked out in a linked worktree
OK, I see 3 votes for cyan and 4-5 people participating in the thread,
so I'll make it cyan in the next revision.
On Tue, Nov 13, 2018 at 3:49 PM Jeff King wrote:
>
> On Mon, Nov 12, 2018 at 06:07:18PM +, Rafael Ascensão wrote:
>
> > On Mon, Nov 12, 2018 at 07:14:23AM -0500, Jeff King wrote:
> > > just adding a bunch of color variants. It would be nice if we could just
> > > do this with a run-time parse_color("bold red") or whatever, but we use
> > > these as static initializers.
> >
> > I suggested those colors, but now, I think this needs to be
> > configurable.
>
> I think they are configurable in that patch, since it provides
> "worktree" as a n entry in color_branch_slots. But yeah, every color we
> add needs to be configurable, and this is really just about defaults.
>
> > I suggested using green and dim green as the obvious theoretical choice
> > but after using it for a while I found out that both shades are way too
> > similar, making it really hard to tell by glancing at the output,
> > especially when they're not side by side.
> >
> > If we continue with two dual green approach, current branch needs to be
> > at least bold. But I'm not sure if it's enough.
> >
> > I've been trying some other colors, and cyan feels neutral-ish.
>
> Yeah, cyan seems pretty reasonable to me.
>
> -Peff
Re: [PATCH v2 1/2] ref-filter: add worktree atom
I think if we move to making this atom just store worktree path, that
needs to be implemented as a hashmap of refname->wtpath, which would
also solve this string_list issue, correct? Just making sure I'm not
missing something before I submit another patch.
On Tue, Nov 13, 2018 at 2:38 AM Junio C Hamano wrote:
>
> Jeff King writes:
>
> >> I wonder if "The worktree at /local/src/wt1 has this branch checked
> >> out" is something the user of %(worktree) atom, or a variant thereof
> >> e.g. "%(worktree:detailed)", may want to learn, but because that
> >> information is lost when this function returns, such an enhancement
> >> cannot be done without fixing this funciton.
> >
> > Hmm. I think for the purposes of this series we could jump straight to
> > converting %(worktree) to mean "the path of the worktree for which this
> > branch is HEAD, or the empty string otherwise".
> >
> > Then the caller from git-branch (or anybody wanting to emulate it) could
> > still do:
> >
> > %(if)%(worktree)%(then)+ %(refname)%(end)
> >
> > As a bonus, the decision to use "+" becomes a lot easier. It is no
> > longer a part of the format language that we must promise forever, but
> > simply a porcelain decision by git-branch.
>
> Yeah, thanks for following through the thought process to the
> logical conclusion. If a branch is multply checked out, which is a
> condition "git worktree" and "git checkout" ought to prevent from
> happening, we could leave the result unspecified but a non-empty
> string, or something like that.
>
> > file-global data-structure storing the worktree info once (in an ideal
> > world, it would be part of a "struct ref_format" that uses no global
> > variables, but that is not how the code is structured today).
>
> Yes, I agree that would be the ideal longer-term direction to move
> this code in.
>
> >> > + } else if (!strcmp(name, "worktree")) {
> >> > + if (string_list_has_string(&atom->u.worktree_heads,
> >> > ref->refname))
> >>
> >> I thought we were moving towards killing the use of string_list as a
> >> look-up table, as we do not want to see thoughtless copy&paste such
> >> a code from parts of the code that are not performance critical to a
> >> part. Not very satisfying.
> >>
> >> I think we can let this pass, and later add a wrapper around
> >> hashmap that is meant to only be used to replace string-list
> >> used for this exact purpose, i.e. key is a string, and there
> >> is no need to iterate over the existing elements in any
> >> sorted order. Optionally, we can limit the look up to only
> >> checking for existence, if it makes the code for the wrapper
> >> simpler.
> >
> > This came up over in another thread yesterday, too. So yeah, perhaps we
> > should move on that (I am OK punting on it for this series and
> > converting it later, though).
>
> FWIW, I am OK punting and leaving, too.
Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible
On Sun, Oct 28, 2018 at 9:01 PM Eric Sunshine wrote:
>
> On Sun, Oct 28, 2018 at 9:11 PM Nickolai Belakovski
> wrote:
> > I would also suggest renaming is_worktree_locked to
> > worktree_lock_reason, the former makes me think the function is
> > returning a boolean, whereas the latter more clearly conveys that a
> > more detailed piece of information is being returned.
>
> I think the "boolean"-sounding name was intentional since most
> (current) callers only care about that; so, the following reads very
> naturally for such callers:
>
> if (is_worktree_locked(wt))
> die(_("worktree locked; aborting"));
>
> That said, I wouldn't necessarily oppose renaming the function, but I
> also don't think it's particularly important to do so.
Actually it's 3:2 in the current code for callers getting the reason
out of the function vs callers checking the value of the pointer for
null/not null. This leads to some rather unnatural looking code in the
current repo like
reason = is_worktree_locked(wt);
I think it would look a lot more natural if it were "reason =
worktree_lock_reason(wt)". The resulting if-statement wouldn't be too
bad, IMO
if (worktree_lock_reason(wt))
die(_("worktree locked; aborting"));
To me, I would just go lookup the signature of worktree_lock_reason
and see that it returns a pointer and I'd be satisfied with that. I
could also infer that from looking at the code if I'm just skimming
through. But if I see code like "reason = is_worktree_locked(wt)" I'm
like hold on, what's going on here?! :P
Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible
On Sun, Oct 28, 2018 at 8:52 PM Junio C Hamano wrote: > > > If the field "reason" should always be populated, there is *no* > reason why we need the "valid" boolean. They work as a pair to > realize lazy population of rarely used field. The lazy evaluation > technique is used as an optimization for common case, where majority > of operations do not care if worktrees are locked and if so why they > are locked, so that only rare operations that do want to find out > can ask "is this locked and why?" via is_worktree_locked() interface, > and at that point we lazily find it out by reading "locked" file. > > So it is by design that these fields are not always populated, but > are populated on demand as book-keeping info internal to the API's > implementation. It is not "an issue", and changing it is not a > "fix". Having fields in a struct that are not populated by a getter function with no documentation indicating that they are not populated and no documentation explaining how to populate them is the issue here. > > In addition, if we have already checked, then we do not even do the > same check again. If in an earlier call we found out that a worktree > is not locked, we flip the _valid bit to true while setting _reason > to NULL, so that the next call can say "oh, that's not locked and we > can tell that without looking at the filesystem again" [*1*]. I clearly misunderstood the use case of the _valid flag, thanks for pointing it out. > > You are forcing the callers of get_worktrees() to pay the cost to > check, open and read the "why is this worktree locked?" file for all > worktrees, whether they care if these worktrees are locked or why > they are locked. Such a change can be an improvement *ONLY* if you > can demonstrate that in the current code most codepaths that call > get_worktrees() end up calling is_worktree_locked() on all worktrees > anyways. If that were the case, not having to lazily evaluate the > "locked"-ness, but always check upfront, would have a simplification > value, as either approach would be spending the same cost to open > and read these "locked" files. > > But I do not think it is the case. Outside builtin/worktree.c (and > you need to admit "git worktree" is a rather rare command in the > first place, so you shouldn't be optimizing for that if it hurts > other codepaths), builtin/branch.c wants to go to all worktrees and > update their HEAD when a branch is renamed (if the old HEAD is > pointing at the original name, of course), but that code won't care > if the worktree is locked at all. I do not think of any caller of > get_worktrees() that want to know if it is locked and why for each > and every one of them, and I'd be surprised if that *is* the > majority, but as a proposer to burden get_worktrees() with this > extra cost, you certainly would have audited the callers and made > sure it is worth making them pay the extra cost? > > If we are going to change anything around this area, I'd not be > surprised that the right move is to go in the opposite direction. > Right now, you cannot just get "is it locked?" boolean answer (which > can be obtained by a simple stat(2) call) without getting "why is it > locked?" (which takes open(2) & read(2) & close(2)), and if you are > planning a new application that wants to ask "is it locked?" a lot > without having to know the reason, you may want to make the lazy > evaluation even lazier by splitting _valid field into two (i.e. a > "do we know if this is locked?" valid bit covers "is_locked" bit, > and another "do we know why this is locked?" valid bit accompanies > "locked_reason" string). And the callers would ask two separate > questions: is_worktree_locked() that says true or false, and then > why_worktree_locked() that yields NULL or string (i.e. essentially > that is what we have as is_worktree_locked() today). Of course, > such a change must also be justified with a code audit to > demonstrate that only minority case of the callers of is-locked? > wants to know why > > > [Footnote] > > *1* The codepaths that want to know if a worktree is locked or not > (and wants to learn the reason) are so rare and concentrated in > builtin/worktree.c, and more importantly, they do not need to ask > the same question twice, so we can stop caching and make > is_worktree_locked() always go to the filesystem, I think, and that > may be a valid change _if_ we allow worktrees to be randomly locked > and unlocked while we are looking at them, but if we want to worry > about such concurrent and competing uses, we need a big > repository-wide lock anyway, and it is the least of our problems > that the current caching may go stale without getting invalidated. > The code will be racing against such concurrent processes even if > you made it to go to the filesystem all the time. > Basically, I already implemented most of what you're saying. The v2 proposal does force all callers of get_worktrees to check the lock status, but by calling stat,
Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible
On Sun, Oct 28, 2018 at 4:03 PM Eric Sunshine wrote: > > On Sun, Oct 28, 2018 at 5:55 PM Nickolai Belakovski > > wrote: This was meant to be a reply to > > https://public-inbox.org/git/cac05386f1x7tspr6kgkulwewsmdiq4vktf5rxahvzpkwbmx...@mail.gmail.com/T/#m8898c8f7c68e1ea234aca21cb2d7776b375c6f51, > > please look there for some more context. I think it both did and > > didn't get listed as a reply? In my mailbox I see two separate > > threads but in public-inbox.org/git it looks like it correctly got > > labelled as 1 thread. This whole mailing list thing is new to me, > > thanks for bearing with me as I figure it out :). > > Gmail threads messages entirely by subject; it doesn't pay attention > to In-Reply-To: or other headers for threading, which is why you see > two separate threads. public-inbox.org, on the other hand, does pay > attention to the headers, thus understands that all the messages > belong to the same thread. Gmail's behavior may be considered > anomalous. > Got it, thanks! > > Next time I'll make sure to change the subject line on updated > > patches as PATCH v2 (that's the convention, right?). > > That's correct. > (thumbs up) > > This is an improvement because it fixes an issue in which the fields > > lock_reason and lock_reason_valid of the worktree struct were not > > being populated. This is related to work I'm doing to add a worktree > > atom to ref-filter.c. > > Those fields are considered private/internal. They are not intended to > be accessed by calling code. (Unfortunately, only 'lock_reason' is > thus marked; 'lock_reason_valid' should be marked "internal".) Clients > are expected to retrieve the lock reason only through the provided > API, is_worktree_locked(). > > > I see your concerns about extra hits to the filesystem when calling > > get_worktrees and about users interested in lock_reason having to > > make extra calls. As regards hits to the filesystem, I could remove > > is_locked from the worktree struct entirely. To address the second > > concern, I could refactor worktree_locked_reason to return null if > > the wt is not locked. I would still want to keep is_worktree_locked > > around to provide a facility to check whether or not the worktree is > > locked without having to go get the reason. > > > > There's also been some concerns raised about caching. As I pointed > > out in the other thread, the current use cases for this information > > die upon accessing it, so caching is a moot point. For the use case > > of a worktree atom, caching would be relevant, but it could be done > > within ref-filter.c. Another option is to add the lock_reason back > > to the worktree struct and have two functions for populating it: > > get_worktrees_wo_lock_reason and get_worktrees_with_lock_reason. A > > bit more verbose, but it makes it clear to the caller what they're > > getting and what they're not getting. I might suggest starting with > > doing the caching within ref-filter.c first, and if more use cases > > appear for caching lock_reason we can consider the second option. It > > could also be get_worktrees and get_worktrees_wo_lock_reason, though > > I think most callers would be calling the latter name. > > > > So, my proposal for driving this patch to completion would be to: > > -remove is_locked from the worktree struct > > -refactor worktree_locked_reason to return null if the wt is not locked > > -refactor calls to is_locked within builtin/worktree.c to call > > either the refactored worktree_locked_reason or is_worktree_locked > > My impression, thus far, is that this all seems to be complicating > rather than simplifying. These changes also seem entirely unnecessary. > In [1], I made the observation that it seemed that your new ref-filter > atom could be implemented with the existing is_worktree_locked() API. > As far as I can tell, it can indeed be implemented without having to > make any changes to the worktree API or implementation at all. > > The worktree API is both compact and orthogonal, and I haven't yet > seen a compelling reason to change it. That said, though, the API > documentation in worktree.h may be lacking, even if the implementation > is not. I'll say a bit more about that below. > > > In addition to making the worktree code clearer, this patch fixes a > > bug in which the current is_worktree_locked over-eagerly sets > > lock_reason_valid. There are currently no consumers of > > lock_reason_valid within master, but obviously we should fix this > > before they appear :) > > As noted
Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible
This was meant to be a reply to
https://public-inbox.org/git/cac05386f1x7tspr6kgkulwewsmdiq4vktf5rxahvzpkwbmx...@mail.gmail.com/T/#m8898c8f7c68e1ea234aca21cb2d7776b375c6f51,
please look there for some more context. I think it both did and
didn't get listed as a reply? In my mailbox I see two separate threads
but in public-inbox.org/git it looks like it correctly got labelled as
1 thread. This whole mailing list thing is new to me, thanks for
bearing with me as I figure it out :). Next time I'll make sure to
change the subject line on updated patches as PATCH v2 (that's the
convention, right?).
This is an improvement because it fixes an issue in which the fields
lock_reason and lock_reason_valid of the worktree struct were not
being populated. This is related to work I'm doing to add a worktree
atom to ref-filter.c.
I see your concerns about extra hits to the filesystem when calling
get_worktrees and about users interested in lock_reason having to make
extra calls. As regards hits to the filesystem, I could remove
is_locked from the worktree struct entirely. To address the second
concern, I could refactor worktree_locked_reason to return null if the
wt is not locked. I would still want to keep is_worktree_locked around
to provide a facility to check whether or not the worktree is locked
without having to go get the reason.
There's also been some concerns raised about caching. As I pointed out
in the other thread, the current use cases for this information die
upon accessing it, so caching is a moot point. For the use case of a
worktree atom, caching would be relevant, but it could be done within
ref-filter.c. Another option is to add the lock_reason back to the
worktree struct and have two functions for populating it:
get_worktrees_wo_lock_reason and get_worktrees_with_lock_reason. A bit
more verbose, but it makes it clear to the caller what they're getting
and what they're not getting. I might suggest starting with doing the
caching within ref-filter.c first, and if more use cases appear for
caching lock_reason we can consider the second option. It could also
be get_worktrees and get_worktrees_wo_lock_reason, though I think most
callers would be calling the latter name.
So, my proposal for driving this patch to completion would be to:
-remove is_locked from the worktree struct
-refactor worktree_locked_reason to return null if the wt is not locked
-refactor calls to is_locked within builtin/worktree.c to call either
the refactored worktree_locked_reason or is_worktree_locked
In addition to making the worktree code clearer, this patch fixes a
bug in which the current is_worktree_locked over-eagerly sets
lock_reason_valid. There are currently no consumers of
lock_reason_valid within master, but obviously we should fix this
before they appear :)
Thoughts?
On Wed, Oct 24, 2018 at 11:56 PM Junio C Hamano wrote:
>
> nbelakov...@gmail.com writes:
>
> > From: Nickolai Belakovski
> >
> > lock_reason_valid is renamed to is_locked and lock_reason is removed as
> > a field of the worktree struct. Lock reason can be obtained instead by a
> > standalone function.
> >
> > This is done in order to make the worktree struct more intuitive when it
> > is used elsewhere in the codebase.
>
> So a mere action of getting an in-core worktree instance now has to
> make an extra call to file_exists(), and in addition, the callers
> who want to learn why the worktree is locked, they need to open and
> read the contents of the file in addition?
>
> Why is that an improvement?
>
>
> >
> > Some unused variables are cleaned up as well.
> >
> > Signed-off-by: Nickolai Belakovski
> > ---
> > builtin/worktree.c | 16
> > worktree.c | 55
> > --
> > worktree.h | 8 +++-
> > 3 files changed, 40 insertions(+), 39 deletions(-)
> >
> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > index 41e771439..844789a21 100644
> > --- a/builtin/worktree.c
> > +++ b/builtin/worktree.c
> > @@ -634,8 +634,8 @@ static int lock_worktree(int ac, const char **av, const
> > char *prefix)
> > if (is_main_worktree(wt))
> > die(_("The main working tree cannot be locked or unlocked"));
> >
> > - old_reason = is_worktree_locked(wt);
> > - if (old_reason) {
> > + if (wt->is_locked) {
> > + old_reason = worktree_locked_reason(wt);
> > if (*old_reason)
> > die(_("'%s' is already locked, reason: %s"),
> > av[0], old_reason);
> > @@ -666,7 +666,7 @@ static int unlock_worktree(int ac, const char **av,
> > const char *prefix)
> &g
Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files
The motivation for the change is some work that I'm doing to add a worktree atom in ref-filter.c. I wanted that atom to be able to access all fields of the worktree struct and noticed that lock_reason wasn't getting populated so I figured I'd go and fix that. I figured that since get_worktrees is already hitting the filesystem for the directory it wouldn't matter much if it also went and got the lock reason. Reviewing this work in the context of your feedback and Junio's previous comments, I think it makes sense to only have a field in the struct indicating whether or not the worktree is locked, and have a separate function for getting the reason. Since the only cases in which the reason is retrieved in the current codebase are cases where the program immediately dies, caching seems a moot point. I'll send an updated patch just after this message. Thanks for the feedback, happy to receive more. On Wed, Oct 24, 2018 at 1:11 AM Eric Sunshine wrote: > > On Wed, Oct 24, 2018 at 2:39 AM wrote: > > lock_reason is now populated during the execution of get_worktrees > > > > is_worktree_locked has been simplified, renamed, and changed to internal > > linkage. It is simplified to only return the lock reason (or NULL in case > > there is no lock reason) and to not have any side effects on the inputs. > > As such it made sense to rename it since it only returns the reason. > > > > Since this function was now being used to populate the worktree struct's > > lock_reason field, it made sense to move the function to internal > > linkage and have callers refer to the lock_reason field. The > > lock_reason_valid field was removed since a NULL/non-NULL value of > > lock_reason accomplishes the same effect. > > > > Some unused variables within worktree source code were removed. > > Thanks for the submission. > > One thing which isn't clear from this commit message is _why_ this > change is desirable at this time, aside from the obvious > simplification of the code and client interaction (or perhaps those > are the _why_?). > > Although I had envisioned populating the "reason" field greedily in > the way this patch does, not everyone agrees that doing so is > desirable. In particular, Junio argued[1,2] for populating it lazily, > which accounts for the current implementation. That's why I ask about > the _why_ of this change since it will likely need to be justified in > a such a way to convince Junio to change his mind. > > Thanks. > > [1]: > https://public-inbox.org/git/xmqq8tyq5czn@gitster.mtv.corp.google.com/ > [2]: > https://public-inbox.org/git/xmqq4m9d0w6v@gitster.mtv.corp.google.com/
Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized
Not to hijack my own thread, but FWIW git branch -r shows remote branches in red, but old/new status of a remote branch is ambiguous (could have new stuff, could be out of date). Also, git branch -vv shows remote tracking branches in blue. One could argue it should be red since git branch -r is in red. But yea, probably best to take this topic to its own thread. On Thu, Sep 27, 2018 at 1:02 PM Ævar Arnfjörð Bjarmason wrote: > > > On Thu, Sep 27 2018, Rafael Ascensão wrote: > > > On Thu, Sep 27, 2018 at 02:17:08PM -0400, Jeff King wrote: > >> Do we want to limit this to git-branch, though? Ideally any output you > >> get from git-branch could be replicated with for-each-ref (or with > >> a custom "branch --format"). > >> > >> I.e., could we have a format in ref-filter that matches HEAD, but > >> returns a distinct symbol for a worktree HEAD? That would allow a few > >> things: > > > > I was going to suggest using dim green and green for elsewhere and here > > respectively, in a similar way how range-diff uses it to show different > > versions of the same diff. > > It would be really useful to (just via E-Mail to start) itemize the > colors we use in various places and what they mean. > > E.g. I thought green here made sense because in "diff" we show the > old/new as red/green, so the branch you're on is "new" in the same > sense, i.e. it's what your current state is. > > But maybe there's cases where that doesn't "rhyme" as it were.
Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized
Thanks for the feedback Peff. I actually agree with all your points.
I'd considered an approach like what you proposed, but rejected it for
the first iteration in an effort to keep scope limited and see what
kind of feedback I'd get overall (like would people even want this?).
This is a much better approach, and also gives a path for listing the
worktree path in the verbose output.
@Duy yea we can use a different color, maybe a darker shade of green.
I saw plenty to choose from in the color list so I'll play around with
it. It would definitely make it easier to distinguish at a glance
which branch is checked out in the current worktree vs others.
On Thu, Sep 27, 2018 at 11:17 AM Jeff King wrote:
>
> On Thu, Sep 27, 2018 at 08:13:13AM -0700, Nickolai Belakovski wrote:
>
> > In order to more clearly display which branches are active, the output
> > of git branch is modified to colorize branches checked out in any linked
> > worktrees with the same color as the current branch.
>
> I think the goal makes sense.
>
> Do we want to limit this to git-branch, though? Ideally any output you
> get from git-branch could be replicated with for-each-ref (or with
> a custom "branch --format").
>
> I.e., could we have a format in ref-filter that matches HEAD, but
> returns a distinct symbol for a worktree HEAD? That would allow a few
> things:
>
> - custom --formats for for-each-ref and branch could reuse the logic
>
> - we could show the symbol (in place of "*") even when color is not
> enabled
>
> - it should be much faster if there are a lot of worktrees; your patch
> does a linear if/else chain to look at each worktree, and it does it
> in the format-language, which is much slower than actual C. :)
>
> Something like the patch below. I just picked "+" arbitrarily, but any
> character would do (I avoided "*" just to make it visually distinct from
> the current-worktree HEAD). I've left plugging this into git-branch's
> default format as an exercise for the reader. ;)
>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index e1bcb4ca8a..b17eefed0d 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -20,6 +20,7 @@
> #include "commit-slab.h"
> #include "commit-graph.h"
> #include "commit-reach.h"
> +#include "worktree.h"
>
> static struct ref_msg {
> const char *gone;
> @@ -114,6 +115,7 @@ static struct used_atom {
> } objectname;
> struct refname_atom refname;
> char *head;
> + struct string_list worktree_heads;
> } u;
> } *used_atom;
> static int used_atom_cnt, need_tagged, need_symref;
> @@ -420,6 +422,29 @@ static int head_atom_parser(const struct ref_format
> *format, struct used_atom *a
> return 0;
> }
>
> +static int worktree_head_atom_parser(const struct ref_format *format,
> +struct used_atom *atom,
> +const char *arg,
> +struct strbuf *unused_err)
> +{
> + struct worktree **worktrees = get_worktrees(0);
> + int i;
> +
> + string_list_init(&atom->u.worktree_heads, 1);
> +
> + for (i = 0; worktrees[i]; i++) {
> + if (worktrees[i]->head_ref)
> + string_list_append(&atom->u.worktree_heads,
> + worktrees[i]->head_ref);
> + }
> +
> + string_list_sort(&atom->u.worktree_heads);
> +
> + free_worktrees(worktrees);
> + return 0;
> +
> +}
> +
> static struct {
> const char *name;
> info_source source;
> @@ -460,6 +485,7 @@ static struct {
> { "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },
> { "flag", SOURCE_NONE },
> { "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser },
> + { "worktree", SOURCE_NONE, FIELD_STR, worktree_head_atom_parser },
> { "color", SOURCE_NONE, FIELD_STR, color_atom_parser },
> { "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
> { "end", SOURCE_NONE },
> @@ -1588,6 +1614,13 @@ static int populate_value(struct ref_array_item *ref,
> struct strbuf *err)
> else
> v->s = " ";
> continue;
> + } else if (!strcmp(name, "worktree")) {
> + if (string_list_has_string(&atom->u.worktree_heads,
> + ref->refname))
> + v->s = "+";
> + else
> + v->s = " ";
> + continue;
> } else if (starts_with(name, "align")) {
> v->handler = align_atom_handler;
> v->s = "";
Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized
Will do re: screenshot when I get home, although it's pretty easy to imagine, the git branch output will have one other branch colored in green, bit without the asterisk (for one linked worktree) :) Also will do re: changing comments to /**/ (didn't know // was from C++, TIL) and I'll clean up the comments to remove some of the more obvious ones, but I'll try to keep a comment explaining the basic flow of creating a nest if statement to evaluate worktree refs for color. And yes, I copy/pasted into gmail. I was having trouble setting up send-email, but I think I may have it figured out now. Should I create a new thread with send-email? Or maybe reply to this one (I can do that by specifying the Message-ID to reply to right? This is my first time using this workflow, so I appreciate your patience :) )? Thanks for the feedback! On Thu, Sep 27, 2018 at 8:33 AM Ævar Arnfjörð Bjarmason wrote: > > > On Thu, Sep 27 2018, Nickolai Belakovski wrote: > > > In order to more clearly display which branches are active, the output > > of git branch is modified to colorize branches checked out in any linked > > worktrees with the same color as the current branch. > > > > This is meant to simplify workflows related to worktree, particularly > > due to the limitations of not being able to check out the same branch in > > two worktrees and the inability to delete a branch checked out in a > > worktree. When performing branch operations like checkout and delete, it > > would be useful to know more readily if the branches in which the user > > is interested are already checked out in a worktree. > > > > The git worktree list command contains the relevant information, however > > this is a much less frquently used command than git branch. > > > > Signed-off-by: Nickolai Belakovski > > Sounds cool, b.t.w. would be neat-o to have some screenshot uploaded to > imgur or whatever just to skim what it looks like before/after. > > > diff --git a/builtin/branch.c b/builtin/branch.c > > index 4fc55c350..65b58ff7c 100644 > > --- a/builtin/branch.c > > +++ b/builtin/branch.c > > @@ -334,11 +334,36 @@ static char *build_format(struct ref_filter > > *filter, int maxwidth, const char *r > > struct strbuf local = STRBUF_INIT; > > struct strbuf remote = STRBUF_INIT; > > > > - strbuf_addf(&local, "%%(if)%%(HEAD)%%(then)* %s%%(else) %s%%(end)", > > - branch_get_color(BRANCH_COLOR_CURRENT), > > - branch_get_color(BRANCH_COLOR_LOCAL)); > > - strbuf_addf(&remote, " %s", > > - branch_get_color(BRANCH_COLOR_REMOTE)); > > + // Prepend the current branch of this worktree with "* " and > > all other branches with " " > > > We use /* ... */ C comments, not C++-style // (well, it's in C now, but > not the ancient versions we need to support). > > It also seems all of this patch was copy/pasted into GMail or something, > it has wrapping and doesn't apply with "git am". > > Also most/all of these comments I'd say we could better do without, > i.e. the ones explaining basic code flow that's easy to see from the > code itself.
[PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized
In order to more clearly display which branches are active, the output
of git branch is modified to colorize branches checked out in any linked
worktrees with the same color as the current branch.
This is meant to simplify workflows related to worktree, particularly
due to the limitations of not being able to check out the same branch in
two worktrees and the inability to delete a branch checked out in a
worktree. When performing branch operations like checkout and delete, it
would be useful to know more readily if the branches in which the user
is interested are already checked out in a worktree.
The git worktree list command contains the relevant information, however
this is a much less frquently used command than git branch.
Signed-off-by: Nickolai Belakovski
---
Notes:
Travis CI results: https://travis-ci.org/nbelakovski/git/builds/432320949
builtin/branch.c | 35 ++-
t/t3203-branch-output.sh | 21 +
2 files changed, 51 insertions(+), 5 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 4fc55c350..65b58ff7c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -334,11 +334,36 @@ static char *build_format(struct ref_filter
*filter, int maxwidth, const char *r
struct strbuf local = STRBUF_INIT;
struct strbuf remote = STRBUF_INIT;
- strbuf_addf(&local, "%%(if)%%(HEAD)%%(then)* %s%%(else) %s%%(end)",
- branch_get_color(BRANCH_COLOR_CURRENT),
- branch_get_color(BRANCH_COLOR_LOCAL));
- strbuf_addf(&remote, " %s",
- branch_get_color(BRANCH_COLOR_REMOTE));
+ // Prepend the current branch of this worktree with "* " and
all other branches with " "
+ strbuf_addf(&local, "%%(if)%%(HEAD)%%(then)* %%(else) %%(end)");
+ // Prepend remote branches with two spaces
+ strbuf_addstr(&remote, " ");
+ if(want_color(branch_use_color)) {
+ // Create a nested if statement to evaluate if the
current ref is equal to a HEAD ref from either
+ // the main or any linked worktrees. If so, color it
CURRENT, otherwise color it LOCAL
+ struct strbuf color = STRBUF_INIT;
+ struct worktree **worktrees = get_worktrees(0);
+ int i;
+ for (i = 0; worktrees[i]; ++i) {
+ strbuf_addf(&color,
"%%(if:equals=%s)%%(refname)%%(then)%s%%(else)",
+ worktrees[i]->head_ref,
+ branch_get_color(BRANCH_COLOR_CURRENT));
+ }
+ // add one more check in the nested if-else to cover
the detached HEAD state
+ strbuf_addf(&color, "%%(if)%%(HEAD)%%(then)%s%%(else)%s%%(end)",
+ branch_get_color(BRANCH_COLOR_CURRENT),
+ branch_get_color(BRANCH_COLOR_LOCAL));
+ // close up the nested if-else
+ for (; i > 0; --i) {
+ strbuf_addf(&color, "%%(end)");
+ }
+ free_worktrees(worktrees);
+ strbuf_addbuf(&local, &color);
+ strbuf_release(&color);
+
+ strbuf_addf(&remote, "%s",
+ branch_get_color(BRANCH_COLOR_REMOTE));
+}
if (filter->verbose) {
struct strbuf obname = STRBUF_INIT;
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index ee6787614..369a156c0 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -240,6 +240,27 @@ test_expect_success 'git branch --format option' '
test_i18ncmp expect actual
'
+test_expect_success '"add" a worktree' '
+ mkdir worktree_dir &&
+ git worktree add -b master_worktree worktree_dir master
+'
+
+cat >expect <<'EOF'
+* (HEAD detached from fromtag)
+ ambiguous
+ branch-one
+ branch-two
+ master
+ master_worktree
+ ref-to-branch -> branch-one
+ ref-to-remote -> origin/branch-one
+EOF
+test_expect_success TTY 'worktree colors correct' '
+ test_terminal git branch >actual.raw &&
+ test_decode_color actual &&
+ test_cmp expect actual
+'
+
test_expect_success "set up color tests" '
echo "master" >expect.color &&
echo "master" >expect.bare &&
--
2.14.2
