Re: [PATCH] log: add %S option (like --source) to log --format

[Junio C Hamano <gitster@pobox.com>]

Derrick Stolee <stolee@gmail.com> writes:

>> +++ 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-<.

>> +		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.