Re: [PATCH v5 5/6] last-modified: support --extended format

[Toon Claes <toon@iotcl.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Toon Claes <toon@iotcl.com> writes:
>
>> +OUTPUT
>> +------
>> +
>> +The default format prints for each path:
>> +
>> +	<oid> TAB <path> LF
>> +
>> +When the commit is at boundary, it's prefixed with a caret `^`.
>> +
>> +Or when option `-z` is given:
>> +
>> +	<oid> TAB <path> NUL
>> +
>> +When `--extended` is provided, the output will be in the format:
>> +
>> +	path SP <path> LF
>> +	commit SP <oid> LF
>> +	tree SP <tree> LF
>> +	parent SP <parent> LF
>> +	author SP <author> LF
>> +	    <message>
>> +
>> +Each line of the commit message is indented with four spaces.
>> +
>> +Unless together with `--extended` option `-z` is given, then the output is:
>
> "If" would probably have been more readable.
>
> I can see why you wrote "Unless" here, i.e.
>
>     We indent by four spaces.
>     Unless you use "-z" and "--extended" together, that is.
>
> but I do not think it is a good idea to use such a construct here.
> The reason why I do not think you want to phrase it that way is
> because the next block that illustrates what happens when "-z" and
> "--extended" are used together has more differences than just a mere
> "is the message indented?" single bit.  Unlike "--extended" without
> "-z" that uniformly use LF as inter-item separator, some items are
> NUL terminated while others are LF terminated.
>
>> +	path SP <path> NUL
>> +	commit SP <oid> LF
>> +	tree SP <tree> LF
>> +	parent SP <parent> LF
>> +	author SP <author> LF
>> +	<message>
>> +
>> +In this situation the commit message is not indented.
>> +
>> +A path containing SP or special characters is enclosed in double-quotes in the C
>> +style as needed, unless option `-z` is provided.
>
> Another thing I find the above output description somewhat lacking
> is that, while it is clear how each output entry ends when
> "--extended" is not given (i.e. it shows what terminates each output
> entry.  The output is one entry per path and either LF or NUL
> terminates an entry), the description of "--extended", with or
> without "-z" is silent about how the reader program is expected to
> notice when the message ends.
>
> Without "-z" and indented, the end of the <message> part if either
> EOF or any unindented line, whichever comes earlier, I presume?

That's the idea.

> I
> am planning to teach pretty_print_commit() to stop indenting an
> empty line by 4 spaces, by the way---non-"-z" format needs to be
> designed to withstand such a change.

It kind of is. A new entry should start with "path " (no leading space).

> How would this extended format gain more fields in the future?  A
> free-text <message> has to be at the end?  What if we later need to
> add another free-text thing (e.g., notes ttached to the commit that
> is responsible for that latest state of the path)?

Ahha, you mean a future field that's multi-line? I assume we'd indent
those lines, but I agree it would make parsing harder.

> I suspect that
> you'd want an explicit tag (perhaps "message SP <message>") so that
> the log message does not have to be anything special among others.

The idea was to have the output compatible with the git-cat-file(1)
output. That would then no longe be the case. That's also why you see
mixed use of NUL and LF delimited fields in "-z" mode.

> In any case, the above considerations need to be documented.

Yeah, that's worth elaborating on.

> With "-z", a message body can begin with "path ", so you'd need to
> arrange some terminator (like NUL) after the message body anyway.

That should be the case, but it seems I missed that in the docs.

> Unless your format is "we tell about one path and then always exit",
> that is, but that is probably not what we want.
>
> Thanks.

Thanks for this feedback. I've submitted this patch on top mainly to
gather some feedback, and this is really valuable. I think in the next
version of this series I'm gonna leave out this patch because there are
too many loose ends, and I think in our product we can easily integrate
git-last-modified(1) if it only supports the single-line output format.

-- 
Cheers,
Toon