git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 8/9] branch: use ref-filter printing APIs

From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date
Oct 3, 2015, 12:41 UTC
Message-ID
<vpqvbao86pj.fsf@grenoble-inp.fr>
In-Reply-To
<1443807546-5985-9-git-send-email-Karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> -	if (upstream_is_gone) {
> -		if (show_upstream_ref)
> -			strbuf_addf(stat, _("[%s: gone]"), fancy.buf);

The old string was translated, and you're replacing it with one which isn't.

I'm not a big fan of translation, so that change doesn't disturb me, but people used to having "git branch" talk their native language here may care.

Be careful: translation should happen only in porcelain. Typicall, "for-each-ref" should anyway continue saying "gone" in english whatever the configuration is.

See e.g. 7a76c28 (status: disable translation when --porcelain is used, 2014-03-20) for an example of translation only for porcelain (that was for status, but also for ahead/behind/gone).

> +static char *get_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)

I'd call that "build_format", since "get_..." usually implies that the value exists already and you're retrieving it.

Show 12 quoted lines
> +{
> +	char *remote = NULL;
> +	char *local = NULL;
> +	char *final = NULL;
> +
> +	if (filter->verbose) {
> +		if (filter->verbose > 1)
> +			local = xstrfmt("%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)%%(align:%d,left)%%(refname:short)%%(end)%s"
> +					" %%(objectname:short,7) %%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s] %%(end)"
> +					"%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)",
> +					branch_get_color(BRANCH_COLOR_CURRENT), maxwidth, branch_get_color(BRANCH_COLOR_RESET),
> +					branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
OK, that is a hell of a block of code ;-).

You should factor the common portions out of these if/else. C allows you to write several string litteral next to each other, so you can eg.

#define STAR_IF_HEAD "%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)" #define ALIGNED_REFNAME "%%(align:%d,left)%%(refname:short)%%(end)"

and then
STAR_IF_HEAD ALIGNED_REFNAME ...

Actually, this is not a performance-cricical piece of code at all, so I think it's even better to build an strbuf little by little using repeated strbuf_addf calls. This way you can do things like

strbuf_addf(fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)",
	    branch_get_color(BRANCH_COLOR_CURRENT));
strbuf_addf(fmt, "%%(align:%d,left)%%(refname:short)%%(end)", maxwidth);

which makes it much easier to pair the %s and the corresponding branch_get_color() or the %d and maxwidth.

But fundamentally, I think this function is doing the right thing. The function is a bit complex (I think it will be much nicer to read when better factored), but 1) it allows getting rid of a lot of specific code and 2) it's a proof that --format is expressive enough.

Show 14 quoted lines
> @@ -58,11 +58,11 @@ test_expect_success 'branch -v' '
>  '
>  
>  cat >expect <<\EOF
> -b1 [origin/master: ahead 1, behind 1] d
> -b2 [origin/master: ahead 1, behind 1] d
> -b3 [origin/master: behind 1] b
> -b4 [origin/master: ahead 2] f
> -b5 [brokenbase: gone] g
> +b1 [origin/master] [ahead 1, behind 1] d
> +b2 [origin/master] [ahead 1, behind 1] d
> +b3 [origin/master] [behind 1] b
> +b4 [origin/master] [ahead 2] f
> +b5 [brokenbase] [gone] g
This corresponds to this part of the commit message:
Show 9 quoted lines
> Since branch.c is being ported to use ref-filter APIs to print the
> required data, it is constricted to the constraints of printing as per
> ref-filter. Which means branch.c can only print as per the atoms
> available in ref-filter. Hence the "-vv" option of 'git branch' now
> prints the upstream and its tracking details separately as
> "[<upstream>] [<tracking info>]" instead of "[<upstream>: <tracking
> info>]".
> 
> Make changes in /t/t6040-tracking-info.sh to reflect the change.
nit: /t/t... -> t/t... (remove leading /)
That is a behavior change, and I actually prefered the previous one.

I actually don't understand why you can't get the old syntax: the [] around the refname come from the format string it get_format(), so you could decide to change "[%s%%(upstream:short)%s]" to "[%s%%(upstream:short)%s: ". The brackets around the status are a bit more tricky, since they were here before your series. But we could have a %(upstream:track,nobracket) to display just the status without the brackets around.

Then, the code must deal with up-to-date branches for which no " :ahead/behind" must be displayed. Probably one more if/then/else nested in the first one.

So, it seems feasible to me to keep the old behavior with the new system, even though it's going to be a bit tricky.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Previous: Karthik NayakNext: Karthik Nayak
Message 35 of 43 in “port branch.c to use ref-filter printing APIs”
  1. 0/9 port branch.c to use ref-filter printing APIsKarthik Nayak, Oct 2, 2015
  2. 1/9 ref-filter: implement %(if), %(then), and %(else) atomsKarthik Nayak, Oct 2, 2015
  3. Junio C HamanoOct 2, 2015
  4. Karthik NayakOct 3, 2015
  5. Matthieu MoyOct 3, 2015
  6. Junio C HamanoOct 3, 2015
  7. Matthieu MoyOct 4, 2015
  8. Junio C HamanoOct 4, 2015
  9. Karthik NayakOct 4, 2015
  10. 2/9 ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)Karthik Nayak, Oct 2, 2015
  11. Junio C HamanoOct 2, 2015
  12. Karthik NayakOct 3, 2015
  13. 3/9 ref-filter: add support for %(path) atomKarthik Nayak, Oct 2, 2015
  14. Matthieu MoyOct 3, 2015
  15. Karthik NayakOct 4, 2015
  16. Matthieu MoyOct 4, 2015
  17. Junio C HamanoOct 4, 2015
  18. Matthieu MoyOct 5, 2015
  19. Karthik NayakOct 4, 2015
  20. 4/9 ref-filter: modify "%(objectname:short)" to take lengthKarthik Nayak, Oct 2, 2015
  21. Junio C HamanoOct 2, 2015
  22. Karthik NayakOct 3, 2015
  23. 5/9 ref-filter: adopt get_head_description() from branch.cKarthik Nayak, Oct 2, 2015
  24. 6/9 ref-filter: introduce format_ref_array_item()Karthik Nayak, Oct 2, 2015
  25. Matthieu MoyOct 3, 2015
  26. Karthik NayakOct 5, 2015
  27. Matthieu MoyOct 5, 2015
  28. Karthik NayakOct 6, 2015
  29. Matthieu MoyOct 6, 2015
  30. Karthik NayakOct 6, 2015
  31. 7/9 ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreamsKarthik Nayak, Oct 2, 2015
  32. Matthieu MoyOct 3, 2015
  33. Karthik NayakOct 5, 2015
  34. 8/9 branch: use ref-filter printing APIsKarthik Nayak, Oct 2, 2015
  35. Matthieu MoyOct 3, 2015
  36. Karthik NayakOct 5, 2015
  37. Matthieu MoyOct 5, 2015
  38. Karthik NayakOct 6, 2015
  39. Matthieu MoyOct 6, 2015
  40. Karthik NayakOct 7, 2015
  41. Matthieu MoyOct 7, 2015
  42. Karthik NayakOct 7, 2015
  43. 9/9 branch: implement '--format' optionKarthik Nayak, Oct 2, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.