Re: [PATCH 08/15] for-each-ref: get --pretty using format_commit_message

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 6, 2013 at 12:09 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Duy Nguyen <pclouds@xxxxxxxxx> writes:
>
>> On Wed, Jun 5, 2013 at 4:12 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>>>> +Caveats:
>>>> +
>>>> +1. Many of the placeholders in "PRETTY FORMATS" are designed to work
>>>> +   specifically on commit objects: when non-commit objects are
>>>> +   supplied, those placeholders won't work.
>>>
>>> Should "won't work" be expanded upon? It's not clear if this means
>>> that git will outright crash, or if it will abort with an appropriate
>>> error message, or if the directive will be displayed as-is or removed
>>> from the output.
>>
>> It will be displayed as-is but that's a bit inconsistent: %(unknown)
>> prints error and aborts while %unknown simply produces %unknown. The
>> latter is how "git log --format" does it. But I think we could make
>> for-each-ref --pretty to do the former for %unknown. It'll be
>> consistent with %(unknown) and we do not need to elaborate much (it's
>> pretty obvious when it does not work).
>
> The Caveat Eric is asking about talks about "what happens to a
> %(field) that only makes sense for a commit when showing a ref
> pointing at a non-commit?", but you are answering "what happend to a
> %(invalidfield) that is not defined", aren't you?

Because %(field) is treated like %(invalidfield) in this case. I'm not
saying this is the best thing to do though.

> IIRC, the reason we show literal from "log --format" is to make it
> easier for the person who misspelt %placeholder to spot it in the
> output, and also make it easier for the person who use %placeholder
> meant for newer versions of Git with an older one.  It would be a
> bit unnice to die for the latter, especially if the format string is
> in a script or something.

That makes more sense for for-each-ref than log because for-each-ref
is a plumbing and should support scripting. But old for-each-ref will
happily reject %(newplacholder) right away. Should we take this
opportunity to change this behavior in for-each-ref too?

> To "log --format", all input objects are expected to be commits, so
> it does not have the "what does %(authordate) give when given a blob"
> issue.
>
> But for "for-each-ref --format", it is perfectly normal that you may
> feed a non-commit; it makes the mechanism unusable if you errored
> out %(authordate) when showing a ref that points at a tag, doesn't
> it?  Substituting an inapplicable placeholder with an empty string
> would be an easies way out, unless it learns a flexible/elaborate
> conditional formatting mechanism, I would think.

There is a blurred line here. %H (or %h) should produce an object name
even for tags or blobs, but right now it produces "%H" instead. The
same applies for %ad and friends, but these are clearly for commits
and should probably behave like %(authordate), i.e. producing empty
string.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]