Re: [PATCH v2 2/3] pretty: make %H/%h dependent on --abbrev[-commit]
- From
Will Palmer <wmpalmer@gmail.com>
- Date
- Apr 26, 2010, 07:47 UTC
- Message-ID
- <u2i5b9751661004260047n168ad87bx6083fc201bfb21d9@mail.gmail.com>
- In-Reply-To
- <20100426031012.GA29953@progeny.tock>
Show 5 quoted lines
> 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.
Show 16 quoted lines
> >> 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
Show 20 quoted lines
>
>> 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.
Show 10 quoted lines
> >> @@ -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.
Show 46 quoted lines
>
> 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