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

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