From: Nickolai Belakovski <nbelakovski@gmail.com>
Date: Thu, 14 Mar 2019 05:45:33 GMT
Subject: Re: [PATCH v8 2/3] branch: update output to include worktree info
Message-ID: <CAC05385ECz0=zfBHungpFHqPT5Wr0PviduEBraUWbBVrwHMzaA@mail.gmail.com>
In-Reply-To: <20190221124408.GA13403@sigill.intra.peff.net>

On Thu, Feb 21, 2019 at 4:44 AM Jeff King <peff@peff.net> wrote:
>
> On Tue, Feb 19, 2019 at 05:31:22PM +0900, nbelakovski@gmail.com wrote:
>
> > From: Nickolai Belakovski <nbelakovski@gmail.com>
> >
> > 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'
> > +* <GREEN>(HEAD detached from fromtag)<RESET>
> > +  ambiguous<RESET>
> > +  branch-one<RESET>
> > +  branch-two<RESET>
> > +  master<RESET>
> > ++ <CYAN>master_worktree<RESET>
> > +  ref-to-branch<RESET> -> branch-one
> > +  ref-to-remote<RESET> -> origin/branch-one
> > +EOF
> > +test_expect_success TTY 'worktree colors correct' '
> > +     test_terminal git branch >actual.raw &&
> > +     test_decode_color <actual.raw >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