Re: [PATCH v2 2/3] pretty: make %H/%h dependent on --abbrev[-commit]

[Will Palmer <wmpalmer@gmail.com>]

> I agree that this is the right to do, since this is how the built-in
> formats work (the ‘commit ...’ line follows the semantics of your %H,
> and ‘Merge: ...’ line your %p, for example).
>
> Documentation and tests?
Tests, noted. I'll include some in the next version of the patch.
As for documentation, I'm not entirely sure what to add, as it seemed
like the change
merely implements what I would expect when reading the docs for
git-log. Still, noted.
I'll stick a short note in each of %H, %h, %P, %p, and %t


> Shortlog doesn’t print commit hashes, does it?

Shortlog accepts --format, though this doesn't seem to be documented
(if I type "man" and search
for "format"), so perhaps it should be.

>
>> diff --git a/commit.h b/commit.h
>> index b6caf91..7a476a0 100644
>> --- a/commit.h
>> +++ b/commit.h
>> @@ -72,6 +72,7 @@ struct pretty_print_context
>>       int need_8bit_cte;
>>       int show_notes;
>>       int use_color;
>> +     int abbrev_commit;
>>       struct reflog_walk_info *reflog_info;
>>  };
>>
>
> nitpick: I’d stick this up with abbrev and maybe add a comment to
> explain their distinct uses.
Noted. Thanks

>
>> diff --git a/log-tree.c b/log-tree.c
>> index 6bb4748..0a2309c 100644
>> --- a/log-tree.c
>> +++ b/log-tree.c
>> @@ -282,6 +282,8 @@ void show_log(struct rev_info *opt)
>>       int abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40;
>>       const char *extra_headers = opt->extra_headers;
>>       struct pretty_print_context ctx = {0};
>> +     ctx.abbrev = opt->abbrev;
>> +     ctx.abbrev_commit = opt->abbrev_commit;
>>       ctx.use_color = DIFF_OPT_TST(&opt->diffopt, COLOR_DIFF);
>>
>>       opt->loginfo = NULL;
>
> There is a
>
> ctx.abbrev = opt->diffopt.abbrev;
>
> later in the same function; how do these interact?

I hadn't caught that. My guess: stupidly doing the same thing twice.
I'll double-check,
and take out one of them if that's the case.

What all this
has shown me is that there are really too many ways to specify "context when
printing information about a commit". I don't like it, and the lot of
them can probably
be re-factored, perhaps by getting rid of pretty_context and passing
rev_info around
everywhere. I don't know the full implications of that and it seems
outside the scope
of this change.


>
>> @@ -741,24 +744,29 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
> [...]
>>                       strbuf_addstr(sb, find_unique_abbrev(
>> -                                     p->item->object.sha1, DEFAULT_ABBREV));
>> +                                   p->item->object.sha1,
>> +                                   c->pretty_ctx->abbrev));
>
> nitpick: the new indentation makes these look like parameters to
> strbuf_addstr.

Noted. I'll restore the extra indent, which I had assumed was a typo.

>
> Here’s an alternative implementation of the more controversial half of
> your patch, for your amusement.  The big downside is that it requires
> one to specify --abbrev-commit before the --format option.
>
> Thanks for the pleasant read.
>
> Jonathan
>
> diff --git a/pretty.c b/pretty.c
> index 7cb3a2a..1008a41 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -12,10 +12,31 @@
>
>  static char *user_format;
>
> +static void abbreviate_commit_hashes(char *fmt)
> +{
> +       char *p;
> +       for (p = fmt; p != NULL; p = strchr(p + 1, '%')) {
> +               p++;
> +               switch (*p) {
> +               case 'H':
> +                       *p = 'h';
> +                       break;
> +               case 'P':
> +                       *p = 'p';
> +                       break;
> +               case 'T':
> +               default:
> +                       break;
> +               }
> +       }
> +}
> +
>  static void save_user_format(struct rev_info *rev, const char *cp, int is_tformat)
>  {
>        free(user_format);
>        user_format = xstrdup(cp);
> +       if (rev->abbrev_commit)
> +               abbreviate_commit_hashes(user_format);
>        if (is_tformat)
>                rev->use_terminator = 1;
>        rev->commit_format = CMIT_FMT_USERFORMAT;
>

I had been thinking that this wouldn't be safe, but then that was my
being overly-cautious:
it's just been xstrdup()ed, so what we're parsing is ours, no real
reason not to do it.
I think the "must be specified after --abbrev-commit" is a rather
large nail, though. If you work
out the bugs mentioned by Jeff King, and it works, I'll stick it in
there, as I don't like
falling through case statements any more than the next guy. (well,
maybe a little more than
the next guy).

Thanks for the review!
-- Will Palmer