On 25/02/27 01:56PM, Patrick Steinhardt wrote:
> > +static void flush_diff_queue(struct diff_options *options)
> > +{
> > + /*
> > + * If rename detection is not requested, use rename information from the
> > + * raw diff formatted input. Setting found_follow ensures diffcore_std()
> > + * does not mess with rename information already present in queued
> > + * filepairs.
> > + */
> > + if (!options->detect_rename)
> > + options->found_follow = 1;
>
> It's a bit weird that we set this over here. Shouldn't we have set it up
> in the main function already?
Everytime diffcore_std() is invoked found_follow gets reset. This was
included here to ensure the correct value is always set.
In the next version I am going to move away from using found_follow in
favor of a new diff_options field to avoid some of this awkwardness
altogether.
> > + diffcore_std(options);
> > + diff_flush(options);
> > +}
> > +
> > +int cmd_diff_pairs(int argc, const char **argv, const char *prefix,
> > + struct repository *repo)
> > +{
> > + struct strbuf path_dst = STRBUF_INIT;
> > + struct strbuf path = STRBUF_INIT;
> > + struct strbuf meta = STRBUF_INIT;
> > + struct rev_info revs;
> > + int ret;
> > +
> > + const char * const usage[] = {
> > + N_("git diff-pairs -z [<diff-options>]"),
> > + NULL
> > + };
> > + struct option options[] = {
> > + OPT_END()
> > + };
> > + struct option *parseopts = add_diff_options(options, &revs.diffopt);
> > +
> > + show_usage_with_options_if_asked(argc, argv, usage, parseopts);
>
> Don't we also have to call `parse_options()` even though we don't have
> our own options yet? Or is this all handled by `setup_revisions()`?
In the current implementation, the diff options that get appended are
only really used so that the usage message prints with the diff option
info. It still relies on setup_revisions() to actually parse the
options. Since there are not any real options that need parsing,
parse_options() was not invoked.
This is fairly confusing though. I plan to instead parse the diff
options upfront with parse_options(). The diff options parsing through
setup_revisions() becomes effectively a no-op. I think this makes more
sense to read and still lets us print the common diff options is the
usage message.
> > + repo_init_revisions(repo, &revs, prefix);
> > + repo_config(repo, git_diff_basic_config, NULL);
> > + revs.disable_stdin = 1;
> > + revs.abbrev = 0;
> > + revs.diff = 1;
> > +
> > + if (setup_revisions(argc, argv, &revs, NULL) > 1)
> > + usage_with_options(usage, parseopts);
>
> I think it's discouraged nowadays to use `usage_with_options()` as it
> generates a ton of noise while hiding the actual error message. It is
> instead recommended to directly call `usage()` with an error message.
>
> In this case here we would say e.g. `usage(_("unrecognized argument:
> %s"), argv[0])`, in the cases below we'd use the error messages you
> already have.
Good to know. I'll avoid printing the usage options message in all these
failure scenarios in favor of what you suggested.
> > + if (!revs.diffopt.output_format)
> > + revs.diffopt.output_format = DIFF_FORMAT_PATCH;
>
> Instead of setting this conditionally, can we already set it up as a
> default before calling `setup_revisions()`?
The diff output format is set via OPT_BITOP() and thus can have multiple
values at the same time. For example:
$ git diff-tree --raw --patch HEAD
will render both patch and raw output. If we unconditionally set
DIFF_FORMAT_PATCH, it will always be included in the output which is not
what we want. We only want to set DIFF_FORMAT_PATCH if there is still no
value after all options parsing has occurred.
> > + while (1) {
> > + struct object_id oid_a, oid_b;
> > + struct diff_filepair *pair;
> > + unsigned mode_a, mode_b;
> > + const char *p;
> > + char status;
> > +
> > + if (strbuf_getline_nul(&meta, stdin) == EOF)
> > + break;
> > +
> > + p = meta.buf;
> > + if (*p != ':')
> > + die(_("invalid raw diff input"));
> > + p++;
> > +
> > + mode_a = parse_mode_or_die(p, &p);
> > + mode_b = parse_mode_or_die(p, &p);
> > +
> > + if (S_ISDIR(mode_a) || S_ISDIR(mode_b))
> > + die(_("tree objects not supported"));
>
> I assume submodules aren't supported either, are they? If so, do we also
> have to check for `S_ISGITLINK()`? It would be nice to have a test for
> them.
Submodules should actually be supported as I believe all the info
present in the raw formatted input should be enough to properly display
patch output. I'll add a submodule to the existing test setup to
validate.
Thanks
-Justin
