Karthik Nayak <karthik....@gmail.com> writes:
> +An example to show the usage of %(if)...%(then)...%(else)...%(end).
> +This prefixes the current branch with a star.
> +
> +------------
> +#!/bin/sh
> +
> +git for-each-ref --format="%(if)%(HEAD)%(then)* %(else)
> %(end)%(refname:short)" refs/heads/
I don't think the #!/bin/sh adds any value here. Just the 'git
for-each-ref' line is sufficient IMHO.
> +An example to show the usage of %(if)...%(then)...%(end).
> +This adds a red color to authorname, if present
I don't think this is such a good example.
%(color:red)%(authorname)%(color:reset) just works even if %(authorname)
is empty.
A better example would be
git for-each-ref --format='%(if)%(authorname)%(then)Authored by
%(authorname)%(end)'
which avoids writting "Authored by " with no author.
> -static int is_empty(const char * s){
> +static int is_empty(const char *s){
You still have the { on the declaration line, it should be on the next
line.
> @@ -309,10 +311,14 @@ static int is_empty(const char * s){
> static void then_atom_handler(struct atom_value *atomv, struct
> ref_formatting_state *state)
> {
> struct ref_formatting_stack *cur = state->stack;
> - struct if_then_else *if_then_else = (struct if_then_else
> *)cur->at_end_data;
> + struct if_then_else *if_then_else = NULL;
>
> + if (cur->at_end == if_then_else_handler)
> + if_then_else = (struct if_then_else *)cur->at_end_data;
OK, now the cast is safe since at_end_data has to be of type struct
if_then_else * if at_end is if_then_else_handler.
> + unsigned int nobracket = 0;
> +
> + if (!strcmp(valp, ",nobracket"))
> + nobracket = 1;
The code to parse comma-separated values is different here and
elsewhere. I'd rather have the same code (possibly factored into a
helper function), both to get consistent behavior (you're not allowing
%(upstream:nobracket,track) for example, right?) and consistent code.
> if (!num_ours && !num_theirs)
> v->s = "";
> else if (!num_ours) {
> - sprintf(buf, "[behind %d]", num_theirs);
> + if (nobracket)
> + sprintf(buf, "behind %d",
> num_theirs);
> + else
> + sprintf(buf, "[behind %d]",
> num_theirs);
Perhaps use sprintf(buf, "%sbehind %d%s", obracket, num_their, cbracket)
unconditionnally, and set obracket = "" or obracket = "[" once and for
all when you test for "nobracket" above. This avoids these "if
(nobracket)" spread accross the code, but at the price of extra %s in
the format strings.
> @@ -1170,6 +1173,29 @@ static void populate_value(struct ref_array_item *ref)
> else
> v->s = "<>";
> continue;
> + } else if (!strcmp(formatp, "dir") &&
> + (starts_with(name, "refname"))) {
> + const char *sp, *ep, *tmp;
> +
> + sp = tmp = ref->refname;
> + /* Obtain refs/foo/bar/ from refname
> refs/foo/bar/abc */
> + do {
> + ep = tmp;
> + tmp = strchr(ep + 1, '/');
> + } while (tmp);
To look for the last occurence of '/' you can also use strrchr().
Doesn't it do what you want here?
> --- a/t/t6040-tracking-info.sh
> +++ b/t/t6040-tracking-info.sh
> @@ -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
> b6 [origin/master] c
> EOF
Cool!
I didn't go through the patches themselves, but modulo my remarks above
the interdiff looks good. Thanks.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
