Re: [PATCH v2 2/3] builtin: introduce diff-pairs command

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

Justin Tobler <jltobler@gmail.com> writes:

>> I think only accepting NUL terminated input is fine, but if we want to
>> accept other formats we should  have a plan for how to do that in a
>> backwards compatible way as we cannot use `-z` to distinguish between input
>> formats.
>
> If in the future we want to support the normal format, we could introduce
> an `--input-format=normal` option or something along those lines. 

Please don't.  Have an explicit '-z' option from the beginning, and
if the initial version is incapable of reading from text input, then
it is perfectly fine to have

	if (!nul_termination)
		die(_("working without -z not supported (yet)");

Otherwise people have to remember that unlike everybody else that
uses "-z" to signal NUL termination, this one alone wants to use a
"--input-format" option that nobody else uses.

>> > +	/* Don't allow pathspecs at all. */
>> > +	if (revs.prune_data.nr)
>> > +		usage_with_options(usage, options);

Hmph, this is very unfortuate.

The "--raw" format was originally designed as an interchange format
between the frontend and backend.  

The frontend programs take two sets of contents stored in various
places (like tree vs index, tree vs another tree) and express
comparison of corresponding paths in (<from mode+contents> <to
mode+contents> <path>) tuples" (a rough equivalent to what we
internally have on the diff_queued_diff queue in core).

The "--raw" format was designed to "dump" what is in the
diff_queued_diff list.

And then it would be passed to the single backend, that takes
"--raw" format, pass them through the diffcore transform machinery
(like matching removal and addition to detect renames), and produce
various forms of output (like patch, diffstat, etc.).

To me, what you are writing is the output phase of that pipeline,
i.e. the backend.  We do want to (evantually) be able to filter with
pathspec, and all other things the current diff machinery does after
the existing "all-in-one" "git diff" and "git diff-{files,index,tree}"
commands do from their call to diffcore_std() and diffcore_flush().

The revisions option parsing machinery does accept options that
would *not* make sense to expect for them to make any difference to
the result of running "diff".  Rejecting them is a nice thing to
have, e.g. "git diff --no-merges HEAD^ HEAD" does not error out, but
some people may want it to barf (I don't care---I am not sick enough
to give apparently nonsense options to random commands), but it is
perfectly fine to start your implementation with "nonsense options
may be ignored".

But in a "git diff-* -z | git diff-pairs -z" pipeline, I do not see
a particular reason why you would want to forbid the downstream
command to further limit the paths it processes with its own
pathspec, e.g.

    git diff-tree -z --raw A B -- t/ | git diff-pairs -z t/helper/

sounds like a perfectly sensible request to grant.

My recommendation is to avoid deciding to reject things your initial
implementation happens not to support (yet) too early.  In the end,
we want this backend half just as powerful as, if not more than, the
real "git diff" machinery that has both front- and backend in the
same binary.

Thanks.