Re: [PATCH] log: add %S option (like --source) to log --format
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 28, 2018, 18:14 UTC
- Message-ID
- <xmqqd0plplhe.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <5dfd92d1-2e87-3006-1630-a33794b6066b@gmail.com>
Derrick Stolee <stolee@gmail.com> writes:
Show 11 quoted lines
>> +++ b/Documentation/pretty-formats.txt
>> @@ -134,6 +134,8 @@ The placeholders are:
>> - '%cI': committer date, strict ISO 8601 format
>> - '%d': ref names, like the --decorate option of linkgit:git-log[1]
>> - '%D': ref names without the " (", ")" wrapping.
>> +- '%S': ref name given on the command line by which the commit was reached
>> + (like `git log --source`), only works with `git log`
>
> This "only works with `git log`" made me think about what would happen
> with `git rev-list --pretty=format:"%h %S"` and the answer (on my
> machine) was a segfault.That's a bad one X-<.
Show 17 quoted lines
>> + slot = revision_sources_at(c->pretty_ctx->rev->sources, commit);
>> + if (slot && *slot) {
> I'm not sure this check for 'slot' being non-null is necessary, as we
> would already get a failure in the commit-slab code (for
> revision_sources_at()) if the slab is not initialized.
>> + strbuf_addstr(sb, *slot);
>> + return 1;
>> + } else {
>> + die(_("failed to get info for %%S"));
>
> Here, you die() when you fail to get a slot but above you return 0
> when the sources are not initialized.
>
> I don't see another use of die() in this method. Is that the right way
> to handle failure here? (I'm legitimately asking because I have
> over-used 'die()' in the past and am still unclear on when it is
> appropriate.)This is definitely a bad one, too. If '%d' cannot find decoration, it would not "die".
Thanks.