Re: [PATCH v8 3/3] branch: add worktree info on verbose output

From: Nickolai Belakovski <nbelakovski@gmail.com>
To: Jeff King <peff@peff.net>
Cc: "Git List" <git@vger.kernel.org>,
	"Rafael Ascensão" <rafa.almas@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v8 3/3] branch: add worktree info on verbose output
Date: Wed, 13 Mar 2019 22:58:06 -0700	[thread overview]
Message-ID: <CAC05385Q0EQWX8B5PfR2m6N7o573NTF+a0HyXS+zqYAAdgTVOw@mail.gmail.com> (raw)
In-Reply-To: <20190221125952.GB13403@sigill.intra.peff.net>

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