On 25/02/19 01:57PM, Junio C Hamano wrote:
> Justin Tobler <jltobler@xxxxxxxxx> 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.
Thanks, I think this is a much better approach! :)
>
> >> > + /* 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.
Ok, that makes sense. I was originally thinking pathspec limiting could
just be handled upstream, but it probably doesn't make much to
arbitrarily limit this functionality and remain more flexible. I'll do
this in the next version.
Thanks
-Justin
