[v4,2/3] range-diff/format-patch: handle commit ranges other than A..B

Commit Message

Johannes Schindelin Feb. 4, 2021, 9:31 a.m. UTC

From: Johannes Schindelin <[e-mail]>

In the `SPECIFYING RANGES` section of gitrevisions[7], two ways are
described to specify commit ranges that `range-diff` does not yet
accept: "<commit>^!" and "<commit>^-<n>".

Let's accept them, by parsing them via the revision machinery and
looking for at least one interesting and one uninteresting revision in
the resulting `pending` array.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 range-diff.c          | 19 ++++++++++++++++++-
 t/t3206-range-diff.sh |  8 ++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

Comments

Junio C Hamano Feb. 4, 2021, 6:51 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

>  int is_range_diff_range(const char *arg)
>  {
> -	return !!strstr(arg, "..");
> +	char *copy = xstrdup(arg); /* setup_revisions() modifies it */
> +	const char *argv[] = { "", copy, "--", NULL };
> +	int i, positive = 0, negative = 0;
> +	struct rev_info revs;
> +
> +	init_revisions(&revs, NULL);
> +	if (setup_revisions(3, argv, &revs, 0) == 1) {
> +		for (i = 0; i < revs.pending.nr; i++)
> +			if (revs.pending.objects[i].item->flags & UNINTERESTING)
> +				negative++;
> +			else
> +				positive++;
> +	}
> +
> +	free(copy);
> +	object_array_clear(&revs.pending);
> +	return negative > 0 && positive > 0;
>  }

One thing that worries me with this code is that I do not see
anybody that clears UNINTERESTING bit in the flags.  In-core objects
are singletons, so if a user fed the command two ranges,

	git range-diff A..B C..A

and this code first handled "A..B", smudging the in-core instance of
the commit object A with UNINTERESTING bit, that in-core instance
will be reused when the second range argument "C..A" is given to
this function again.

At that point, has anybody cleared the UNINTERESTING bit in the
flags word for the in-core commit A?  I do not see it done in this
function, but perhaps I am missing it done in the init/setup
functions (I somehow doubt it, though)?

Shoudn't we be calling clear_commit_marks(ALL_REF_FLAGS) on the
commits in revs.pending[] array before we clear it?  Depending on
the shape of "arg" that is end-user supplied, we may have walked the
history in handle_dotdot_1() to parse it (e.g. "A...B").

Also we'd want to see what needs to be cleared in revs.cmdline
that would have been populated by calls to add_rev_cmdline().

Other than that, I quite like the way the actual code turned out to
be.

> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 6eb344be0312..e217cecac9ed 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -150,6 +150,14 @@ test_expect_success 'simple A B C (unmodified)' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'A^! and A^-<n> (unmodified)' '
> +	git range-diff --no-color topic^! unmodified^-1 >actual &&
> +	cat >expect <<-EOF &&
> +	1:  $(test_oid t4) = 1:  $(test_oid u4) s/12/B/
> +	EOF
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'trivial reordering' '
>  	git range-diff --no-color master topic reordered >actual &&
>  	cat >expect <<-EOF &&

Junio C Hamano Feb. 4, 2021, 6:58 p.m. UTC | #2

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> +test_expect_success 'A^! and A^-<n> (unmodified)' '
> +	git range-diff --no-color topic^! unmodified^-1 >actual &&
> +	cat >expect <<-EOF &&
> +	1:  $(test_oid t4) = 1:  $(test_oid u4) s/12/B/
> +	EOF
> +	test_cmp expect actual
> +'

Now we actually parse the single-token range, instead of relying on
"does it have dot-dot" heuristics, we can make sure that we reject

    "HEAD^{/^string with .. in it}"

as "not a range with negative and positive ends".

Johannes Schindelin Feb. 4, 2021, 9:42 p.m. UTC | #3

Hi Junio,

On Thu, 4 Feb 2021, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
> >  int is_range_diff_range(const char *arg)
> >  {
> > -	return !!strstr(arg, "..");
> > +	char *copy = xstrdup(arg); /* setup_revisions() modifies it */
> > +	const char *argv[] = { "", copy, "--", NULL };
> > +	int i, positive = 0, negative = 0;
> > +	struct rev_info revs;
> > +
> > +	init_revisions(&revs, NULL);
> > +	if (setup_revisions(3, argv, &revs, 0) == 1) {
> > +		for (i = 0; i < revs.pending.nr; i++)
> > +			if (revs.pending.objects[i].item->flags & UNINTERESTING)
> > +				negative++;
> > +			else
> > +				positive++;
> > +	}
> > +
> > +	free(copy);
> > +	object_array_clear(&revs.pending);
> > +	return negative > 0 && positive > 0;
> >  }
>
> One thing that worries me with this code is that I do not see
> anybody that clears UNINTERESTING bit in the flags.  In-core objects
> are singletons, so if a user fed the command two ranges,
>
> 	git range-diff A..B C..A
>
> and this code first handled "A..B", smudging the in-core instance of
> the commit object A with UNINTERESTING bit, that in-core instance
> will be reused when the second range argument "C..A" is given to
> this function again.
>
> At that point, has anybody cleared the UNINTERESTING bit in the
> flags word for the in-core commit A?  I do not see it done in this
> function, but perhaps I am missing it done in the init/setup
> functions (I somehow doubt it, though)?
>
> Shoudn't we be calling clear_commit_marks(ALL_REF_FLAGS) on the
> commits in revs.pending[] array before we clear it?

Yes, we should ;-)

> Depending on the shape of "arg" that is end-user supplied, we may have
> walked the history in handle_dotdot_1() to parse it (e.g. "A...B").

The code in `handle_dotdot_1()` that handles `...` calls
`get_merge_bases()`, which eventually calls `get_merge_bases_many_0()`
with the `cleanup` parameter set to `1`, i.e. it _does_ clean up the
flags.

Otherwise, `git log A...B` couldn't work, could it? ;-)

> Also we'd want to see what needs to be cleared in revs.cmdline
> that would have been populated by calls to add_rev_cmdline().

Is this cleared, like, ever? I don't see any code that handles `cmdline`
that way. Seems as we're willfully leaking this.

Ciao,
Dscho

> Other than that, I quite like the way the actual code turned out to
> be.
>
> > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> > index 6eb344be0312..e217cecac9ed 100755
> > --- a/t/t3206-range-diff.sh
> > +++ b/t/t3206-range-diff.sh
> > @@ -150,6 +150,14 @@ test_expect_success 'simple A B C (unmodified)' '
> >  	test_cmp expect actual
> >  '
> >
> > +test_expect_success 'A^! and A^-<n> (unmodified)' '
> > +	git range-diff --no-color topic^! unmodified^-1 >actual &&
> > +	cat >expect <<-EOF &&
> > +	1:  $(test_oid t4) = 1:  $(test_oid u4) s/12/B/
> > +	EOF
> > +	test_cmp expect actual
> > +'
> > +
> >  test_expect_success 'trivial reordering' '
> >  	git range-diff --no-color master topic reordered >actual &&
> >  	cat >expect <<-EOF &&
>

Johannes Schindelin Feb. 4, 2021, 9:57 p.m. UTC | #4

Hi Junio,

On Thu, 4 Feb 2021, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
> > +test_expect_success 'A^! and A^-<n> (unmodified)' '
> > +	git range-diff --no-color topic^! unmodified^-1 >actual &&
> > +	cat >expect <<-EOF &&
> > +	1:  $(test_oid t4) = 1:  $(test_oid u4) s/12/B/
> > +	EOF
> > +	test_cmp expect actual
> > +'
>
> Now we actually parse the single-token range, instead of relying on
> "does it have dot-dot" heuristics, we can make sure that we reject
>
>     "HEAD^{/^string with .. in it}"
>
> as "not a range with negative and positive ends".

Sure, why not.

Ciao,
Dscho

Patch

diff --git a/range-diff.c b/range-diff.c
index 9b93e08e8407..07e212d5bb8c 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -11,6 +11,7 @@ 
 #include "pretty.h"
 #include "userdiff.h"
 #include "apply.h"
+#include "revision.h"
 
 struct patch_util {
 	/* For the search for an exact match */
@@ -567,5 +568,21 @@  int show_range_diff(const char *range1, const char *range2,
 
 int is_range_diff_range(const char *arg)
 {
-	return !!strstr(arg, "..");
+	char *copy = xstrdup(arg); /* setup_revisions() modifies it */
+	const char *argv[] = { "", copy, "--", NULL };
+	int i, positive = 0, negative = 0;
+	struct rev_info revs;
+
+	init_revisions(&revs, NULL);
+	if (setup_revisions(3, argv, &revs, 0) == 1) {
+		for (i = 0; i < revs.pending.nr; i++)
+			if (revs.pending.objects[i].item->flags & UNINTERESTING)
+				negative++;
+			else
+				positive++;
+	}
+
+	free(copy);
+	object_array_clear(&revs.pending);
+	return negative > 0 && positive > 0;
 }
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 6eb344be0312..e217cecac9ed 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -150,6 +150,14 @@  test_expect_success 'simple A B C (unmodified)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'A^! and A^-<n> (unmodified)' '
+	git range-diff --no-color topic^! unmodified^-1 >actual &&
+	cat >expect <<-EOF &&
+	1:  $(test_oid t4) = 1:  $(test_oid u4) s/12/B/
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'trivial reordering' '
 	git range-diff --no-color master topic reordered >actual &&
 	cat >expect <<-EOF &&