Hi Thomas,
On Sun, 12 Aug 2018, Thomas Gummerer wrote:
> On 08/10, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@xxxxxx>
> >
> > [..]
> >
> > @@ -13,15 +14,38 @@ NULL
> > int cmd_range_diff(int argc, const char **argv, const char *prefix)
> > {
> > int creation_factor = 60;
> > + struct diff_options diffopt = { NULL };
> > struct option options[] = {
> > OPT_INTEGER(0, "creation-factor", &creation_factor,
> > N_("Percentage by which creation is weighted")),
> > OPT_END()
> > };
> > - int res = 0;
> > + int i, j, res = 0;
> > struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
> >
> > + git_config(git_diff_ui_config, NULL);
> > +
> > + diff_setup(&diffopt);
> > + diffopt.output_format = DIFF_FORMAT_PATCH;
> > +
> > argc = parse_options(argc, argv, NULL, options,
> > + builtin_range_diff_usage, PARSE_OPT_KEEP_UNKNOWN |
> > + PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);
> > +
> > + for (i = j = 1; i < argc && strcmp("--", argv[i]); ) {
> > + int c = diff_opt_parse(&diffopt, argv + i, argc - i, prefix);
> > +
> > + if (!c)
> > + argv[j++] = argv[i++];
> > + else
> > + i += c;
> > + }
>
> I don't think this handles "--" quite as would be expected. Trying to
> use "git range-diff -- js/range-diff-v4...HEAD" I get:
>
> $ ./git range-diff -- js/range-diff-v4...HEAD
> error: need two commit ranges
> usage: git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>
> or: git range-diff [<options>] <old-tip>...<new-tip>
> or: git range-diff [<options>] <base> <old-tip> <new-tip>
>
> --creation-factor <n>
> Percentage by which creation is weighted
> --no-dual-color color both diff and diff-between-diffs
>
> while what I would have expected is to actually get a range diff.
> This happens because after we break out of the loop we don't add the
> actual ranges to argv, but just skip them instead.
Ouch, good point.
> I think something like the following should be squashed in to this
> patch.
>
> --->8---
> diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> index ef3ba22e29..132574c57a 100644
> --- a/builtin/range-diff.c
> +++ b/builtin/range-diff.c
> @@ -53,6 +53,11 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
> else
> i += c;
> }
> + if (i < argc && !strcmp("--", argv[i])) {
> + i++; j++;
> + while (i < argc)
> + argv[j++] = argv[i++];
> + }
> argc = j;
> diff_setup_done(&diffopt);
I do not think that is correct. The original idea was for the first
`parse_options()` call to keep the dashdash, for the second one to keep
the dashdash, too, and for the final one to swallow it.
Also, if `i < argc` at this point, we already know that `argv[i]` refers
to the dashdash, otherwise the previous loop would not have exited early.
I went with this simple version instead:
while (i < argc)
argv[j++] = argv[i++];
Thanks!
Dscho
> --->8---
>
> > + argc = j;
> > + diff_setup_done(&diffopt);
> > +
> > + /* Make sure that there are no unparsed options */
> > + argc = parse_options(argc, argv, NULL,
> > + options + ARRAY_SIZE(options) - 1, /* OPT_END */
> > builtin_range_diff_usage, 0);
> >
> > if (argc == 2) {
> > @@ -59,7 +83,8 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
> > usage_with_options(builtin_range_diff_usage, options);
> > }
> >
> > - res = show_range_diff(range1.buf, range2.buf, creation_factor);
> > + res = show_range_diff(range1.buf, range2.buf, creation_factor,
> > + &diffopt);
> >
> > strbuf_release(&range1);
> > strbuf_release(&range2);
> > diff --git a/range-diff.c b/range-diff.c
> > index 2d94200d3..71883a4b7 100644
> > --- a/range-diff.c
> > +++ b/range-diff.c
> > @@ -6,6 +6,7 @@
> > #include "hashmap.h"
> > #include "xdiff-interface.h"
> > #include "linear-assignment.h"
> > +#include "diffcore.h"
> >
> > struct patch_util {
> > /* For the search for an exact match */
> > @@ -258,7 +259,31 @@ static const char *short_oid(struct patch_util *util)
> > return find_unique_abbrev(&util->oid, DEFAULT_ABBREV);
> > }
> >
> > -static void output(struct string_list *a, struct string_list *b)
> > +static struct diff_filespec *get_filespec(const char *name, const char *p)
> > +{
> > + struct diff_filespec *spec = alloc_filespec(name);
> > +
> > + fill_filespec(spec, &null_oid, 0, 0644);
> > + spec->data = (char *)p;
> > + spec->size = strlen(p);
> > + spec->should_munmap = 0;
> > + spec->is_stdin = 1;
> > +
> > + return spec;
> > +}
> > +
> > +static void patch_diff(const char *a, const char *b,
> > + struct diff_options *diffopt)
> > +{
> > + diff_queue(&diff_queued_diff,
> > + get_filespec("a", a), get_filespec("b", b));
> > +
> > + diffcore_std(diffopt);
> > + diff_flush(diffopt);
> > +}
> > +
> > +static void output(struct string_list *a, struct string_list *b,
> > + struct diff_options *diffopt)
> > {
> > int i = 0, j = 0;
> >
> > @@ -300,6 +325,9 @@ static void output(struct string_list *a, struct string_list *b)
> > printf("%d: %s ! %d: %s\n",
> > b_util->matching + 1, short_oid(a_util),
> > j + 1, short_oid(b_util));
> > + if (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT))
> > + patch_diff(a->items[b_util->matching].string,
> > + b->items[j].string, diffopt);
> > a_util->shown = 1;
> > j++;
> > }
> > @@ -307,7 +335,7 @@ static void output(struct string_list *a, struct string_list *b)
> > }
> >
> > int show_range_diff(const char *range1, const char *range2,
> > - int creation_factor)
> > + int creation_factor, struct diff_options *diffopt)
> > {
> > int res = 0;
> >
> > @@ -322,7 +350,7 @@ int show_range_diff(const char *range1, const char *range2,
> > if (!res) {
> > find_exact_matches(&branch1, &branch2);
> > get_correspondences(&branch1, &branch2, creation_factor);
> > - output(&branch1, &branch2);
> > + output(&branch1, &branch2, diffopt);
> > }
> >
> > string_list_clear(&branch1, 1);
> > diff --git a/range-diff.h b/range-diff.h
> > index 7b6eef303..2407d46a3 100644
> > --- a/range-diff.h
> > +++ b/range-diff.h
> > @@ -1,7 +1,9 @@
> > #ifndef RANGE_DIFF_H
> > #define RANGE_DIFF_H
> >
> > +#include "diff.h"
> > +
> > int show_range_diff(const char *range1, const char *range2,
> > - int creation_factor);
> > + int creation_factor, struct diff_options *diffopt);
> >
> > #endif
> > --
> > gitgitgadget
> >
>
