Re: [PATCH v7 16/17] branch: use ref-filter printing APIs

On Mon, Nov 14, 2016 at 11:23 AM, Karthik Nayak [off-list ref] wrote:

Hello

On Wed, Nov 9, 2016 at 5:44 AM, Jacob Keller [off-list ref] wrote:

quoted

On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak [off-list ref] wrote:

quoted

From: Karthik Nayak <redacted>

Port branch.c to use ref-filter APIs for printing. This clears out
most of the code used in branch.c for printing and replaces them with
calls made to the ref-filter library.

Nice. This looks correct based on checking against the current
branch.c implementation by hand. There was one minor change I
suggested but I'm not really sure it buys is that much.

Thanks for this review. More down.

quoted

quoted

+               if (filter->verbose > 1)
+                       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));

When we have extra verbose, we check whether we have an upstream, and
if so, we print the short name of that upstream inside brackets. If we
have tracking information, we print that without brackets, and then we
end this section. Finally we print the subject.

We could almost re-use the code for the subject bits, but I'm not sure
it's worth it. Maybe drop the %contents:subject part and add it
afterwards since we always want it? It would remove some duplication
but overall not sure it's actually worth it.

If you see that's the last part we add to the 'local' strbuf in the
verbose case.
If we want to remove the duplication we'll end up adding one more
strbuf_addf(...).
So I guess its better this way.

Agreed, I think that it makes more sense to keep this as is. It is
relatively complicated and the strings do have some duplicate code,
but I think it's still ok.

Thanks,
Jake

--
Regards,
Karthik Nayak