| Message ID | 20190219083123.27686-4-nbelakovski@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v8,1/3] ref-filter: add worktreepath atom | expand |
On Tue, Feb 19, 2019 at 05:31:23PM +0900, nbelakovski@gmail.com wrote: > From: Nickolai Belakovski <nbelakovski@gmail.com> > > 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. :) > 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 <remote>`). > + 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 <remote>`). 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 <remote>`). Note that the current worktree's HEAD will not have its path printed (it will always be your current directory). > 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. > diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh > index 94ab05ad59..012ddde7f2 100755 > --- a/t/t3203-branch-output.sh > +++ b/t/t3203-branch-output.sh > @@ -241,7 +241,6 @@ test_expect_success 'git branch --format option' ' > ' > > test_expect_success '"add" a worktree' ' > - mkdir worktree_dir && > git worktree add -b master_worktree worktree_dir master > ' Here's that mysterious mkdir going away. :) > @@ -285,4 +284,24 @@ 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' ' > + cat >expect <<-EOF && > + one > + one > + two > + one > + two > + ($(pwd)/worktree_dir) two > + two > + two > + EOF > + git branch -vv >tmp && > + SUBSTRLENGTH=$(head -1 tmp | awk "{print index(\$0, \"one\")}") && > + awk -v substrlength="$SUBSTRLENGTH" "{print substr(\$0,substrlength,length(\$0))}" <tmp >actual && > + test_cmp expect actual > +' It's hard to tell if this awk is doing the right thing. I guess it works because git-branch tries to line up all of the hashes. I think the result might be easier to verify if we simply blanked the hashes. Unfortunately the output is actually pretty hard to parse. Since there are only two tip commits, perhaps it would not be so bad to just do something like this: 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 $two ($(pwd)/worktree_dir) two + ref-to-branch $two two + ref-to-remote $two two EOF - git branch -vv >tmp && - SUBSTRLENGTH=$(head -1 tmp | awk "{print index(\$0, \"one\")}") && - awk -v substrlength="$SUBSTRLENGTH" "{print substr(\$0,substrlength,length(\$0))}" <tmp >actual && + git branch -vv >actual && test_cmp expect actual ' I don't like how it depends on the space alignment of the branches, but I do like that you can clearly see which branch is being annotated. -Peff
On Thu, Feb 21, 2019 at 4:59 AM Jeff King <peff@peff.net> wrote: > > On Tue, Feb 19, 2019 at 05:31:23PM +0900, nbelakovski@gmail.com wrote: > > > From: Nickolai Belakovski <nbelakovski@gmail.com> > > > > 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 <remote>`). > > + 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 <remote>`). > > 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 <remote>`). 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 $two ($(pwd)/worktree_dir) two > + ref-to-branch $two two > + ref-to-remote $two two > EOF > - git branch -vv >tmp && > - SUBSTRLENGTH=$(head -1 tmp | awk "{print index(\$0, \"one\")}") && > - awk -v substrlength="$SUBSTRLENGTH" "{print substr(\$0,substrlength,length(\$0))}" <tmp >actual && > + git branch -vv >actual && > test_cmp expect actual > ' > > > I don't like how it depends on the space alignment of the branches, but > I do like that you can clearly see which branch is being annotated. Thanks for the suggestion. While I'm kinda proud of my awk thing, I think yours is a lot easier to read. Will add. > > -Peff
Nickolai Belakovski <nbelakovski@gmail.com> writes: > On Thu, Feb 21, 2019 at 4:59 AM Jeff King <peff@peff.net> wrote: >> >> On Tue, Feb 19, 2019 at 05:31:23PM +0900, nbelakovski@gmail.com wrote: >> >> > From: Nickolai Belakovski <nbelakovski@gmail.com> >> > >> > 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. I am moderately skeptical on 2/3, but together with 3/3 I think it makes sense. The fact that two branch names painted in different colors from the other ones, without knowing which one is checked out in what worktree, will not give enough information to allow the user to actually start working on one of them. All the user gets with 2/3 alone is "don't touch it---it is used elsewhere and I am not telling where", isn't it?
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 <remote>`). + 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 <remote>`). -q:: --quiet:: 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)); + } else strbuf_addf(&local, "%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)"); diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh index 94ab05ad59..012ddde7f2 100755 --- a/t/t3203-branch-output.sh +++ b/t/t3203-branch-output.sh @@ -241,7 +241,6 @@ test_expect_success 'git branch --format option' ' ' test_expect_success '"add" a worktree' ' - mkdir worktree_dir && git worktree add -b master_worktree worktree_dir master ' @@ -285,4 +284,24 @@ 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' ' + cat >expect <<-EOF && + one + one + two + one + two + ($(pwd)/worktree_dir) two + two + two + EOF + git branch -vv >tmp && + SUBSTRLENGTH=$(head -1 tmp | awk "{print index(\$0, \"one\")}") && + awk -v substrlength="$SUBSTRLENGTH" "{print substr(\$0,substrlength,length(\$0))}" <tmp >actual && + test_cmp expect actual +' + + test_done