Re: [PATCH 0/4] rev-list: introduce NUL-delimited output mode

From: Justin Tobler <jltobler@gmail.com>
Date: Mar 12, 2025, 22:09 UTC
Message-ID: <sm5xvh7bipyrlpw45bptlctbuhey7v3qktpskal5dzexomt34f@wgfziyhcf3t6>
In-Reply-To: <20250311235720.GA73755@coredump.intra.peff.net>

On 25/03/11 07:57PM, Jeff King wrote:

Show 22 quoted lines

> On Mon, Mar 10, 2025 at 02:28:25PM -0500, Justin Tobler wrote:
> 
> > To make machine parsing easier, this series introduces a NUL-delimited
> > output mode for git-rev-list(1) via a `-z` option following a suggestion
> > from Junio in a previous thread[1]. In this mode, instead of LF, each
> > object is delimited with two NUL bytes and any object metadata is
> > separated with a single NUL byte. Examples:
> > 
> >         <oid> NUL NUL
> >         <oid> [NUL <path>] NUL NUL
> >         ?<oid> [NUL <token>=<value>]... NUL NUL
> > 
> > In this mode, path and value info are printed as-is without any special
> > encoding or truncation.
> 
> I think this is a good direction, but I have two compatibility
> questions:
> 
>   1. What should "git rev-list -z --stdin" do? In most other programs
>      with a "-z" option it affects both input and output. I don't
>      particularly care about this case myself, but it will be hard to
>      change later. So we probably want to decide now.

As others suggested in this thread, in the next version I'll make revision and pathspec parsing on stdin also be NUL-delimited when -z is used.

Show 5 quoted lines

>   2. I was a little surprised that rev-list already takes a "-z" option,
>      but it doesn't seem to do anything. I guess it's probably picked up
>      via diff_opt_parse(), though (I think) you can't actually trigger a
>      diff via rev-list itself. So even though this is a change in
>      behavior, probably nobody would have been using it until now?

This is correct. Technically git-rev-list(1) accepts all the common diff options because it invokes `setup_revisions()`. From my understanding it isn't possible to trigger diffs so I think parsing diff options is unnecessary to begin with. As it also isn't a documented option, I figured -z being an accepted option for git-rev-list(1) is largely an unintended consequence of it using `setup_revisions()` and should be fine to use.

Show 5 quoted lines

>      If it is possible to see some effect from "-z" now (I didn't dig
>      very far), then it may be better to continue to let the diff
>      options parser handle it, and simply pick the result out of
>      revs.diffopt.line_termination. As your patch 3 is written, I think
>      the diff code probably doesn't see it anymore at all.

As currently implemented, the early parsing of -z doesn't effect the diff options parsing in `setup_revisions()`. The early parsing doesn't remove the option and thus it continues to be set in the diff options.

Furthermore, revision and pathspec argument parsing is all handled in `setup_revisions()` so if we want to NUL-delimit arguments parsed on stdin with -z, we would still need to parse the option early anyway. I think it should be fine to leave the early -z option parsing as-is.

Thanks, -Justin