Re: [PATCH/WIP 0/9] for-each-ref format improvements

From: Duy Nguyen <pclouds@gmail.com>
Date: May 19, 2013, 11:54 UTC
Message-ID: <CACsJy8DMF3NzvMRUO56H+EwpXSjmY9qzdNdv9cGJ9XxUO=ekJA@mail.gmail.com>
In-Reply-To: <CALkWK0=7z91A54jSzc2yZU3g50u8H_f8su1Y+i=D+KxYtqor5g@mail.gmail.com>

On Sun, May 19, 2013 at 6:11 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 7 quoted lines

> Nguyễn Thái Ngọc Duy wrote:
>> The purpose of this series is to make "for-each-ref --format" powerful
>> enough to display what "branch -v" and "branch -vv" do so that we
>> could get rid of those display code and use for-each-ref code instead.
>
> Damn, you beat me to it.  I just introduced color, and was working on
> alignment.  See $gmane/224692.

Hmm.. I missed that mail (or I wouldn't have worked on this already). Do you want to take over?

Show 7 quoted lines

>>  - %(tracking[:upstream]) gives us the exact output that branch -v[v]
>>    does. %(upstream) does not include []. We can't change its
>>    semantics.
>
> There's already an atom called "upstream", and "upstream:short" works.
>  Why not introduce "upstream:diff" for "[ahead x, behind y]" and
> "upstream:shortdiff" for "<>" (like in the prompt)?

"branch -vv" shows [upstream: ahead x, behind y]. We need a syntax to cover that too.

Show 6 quoted lines

>>  - %(color:...) is pretty much the same as %C family in pretty code.
>>    I haven't added code for %(color:foo) == %C(foo) yet. There's a
>>    potential ambiguity here: %C(red) == %Cred or %C(red)??
>
> I'd vote for dropping %C<name> altogether and just go with %C(<name>).
>  Why do we need %(color:<name>) at all?

pretty and for-each-ref format seem to be on the opposite: one is terse, one verbose. Unless you are going to introduce a lot of new specifiers (and in the worst case, bring all pretty specifiers over, unify underlying code), I think we should stick with %(xx) convention.

Show 8 quoted lines

>>  - %(...:aligned) to do left aligning. I'm not entirely sure about
>>    this. We might be able to share code with %>, %< and %>< from
>>    pretty.c. But we need improvements there too because in
>>    for-each-ref case, we could calculate column width but %< would
>>    require the user to specify the width.
>
> Yeah, I think we should go with the %> and %< you introduced in
> pretty.c.  Yes, I want to be able to specify width.

I still think we should follow %(...), e.g. %(left:N), %(right:N) as equivalent of %< and %>...

Show 5 quoted lines

>>    Do people expect fancy layout with for-each-ref (and branch)? If so
>>    we might need to have %(align) or something instead of the simple
>>    left alignment case in %(...:aligned)
>
> Why should we deviate from the pretty case?  What is different here?

Laziness plays a big factor :) So again, you want to take over? ;)

Show 11 quoted lines

>>  - We may need an equivalent of the space following % in pretty
>>    format. If the specifier produces something, then prepend a space,
>>    otherwise produce nothing. Do it like %C( tracking) vs
>>    %C(tracking)??
>
> Yeah, sounds good.
>
>> You can try this after applying the series, which should give you the
>> about close to 'branch -v'. %(tracking) coloring does not work though.
>
> Why doesn't %(tracking) coloring work?

it uses builtin/branch.c:branch_use_color. Eventually fill_tracking_info() should be moved to for-each-ref.c and pass branch_use_color in as an argument. But for now, I just leave it broken. -- Duy