Re: [PATCH 01/15] handle_revision_arg: reset "dotdot" consistently

[Philip Oakley]

From: "Jeff King" 

On Sat, May 20, 2017 at 03:56:32PM +0100, Philip Oakley wrote:


> That means we do report the correct name for "a" in the
> pending array. But some code paths try to show the whole
> "a..b" name in error messages, and these erroneously show
> only "a" instead of "a..b". E.g.:
>
>  $ git cherry-pick HEAD:foo..HEAD:foo

shouldn't this be three dots? Also the para above uses two dot examples 
in
its description but the paras before that start by describing the three 
dot

case.


Yeah, it should be. The problem happens with the two-dot case, too (it's
the same code) but to provoke cherry-pick to actually show the error in
question, you need to use three-dots. There's probably a way to provoke
a broken error message with the two-dot case, but I didn't dig further
after finding this one.

Thanks for being a careful reader. Here's the patch with a modified
commit message. I don't think this series otherwise needs a re-roll so
far.


Agreed, it's only a small point. It is difficult to cover both the two and 
three dot cases mid flow. Perhaps show the two options very early


-- >8 --
Subject: [PATCH] handle_revision_arg: reset "dotdot" consistently

When we are parsing a range like "a..b", we write a


Perhaps this early?   s/"a..b"/"a..b" or "a...b"/


temporary NUL over the first ".", so that we can access the
names "a" and "b" as C strings. But our restoration of the
original "." is done at inconsistent times, which can lead
to confusing results.

For most calls, we restore the "." after we resolve the
names, but before we call verify_non_filename().  This means
that when we later call add_pending_object(), the name for
the left-hand "a" has been re-expanded to "a..b". You can
see this with:


This now looks 'correct' relative to the initial multi-dot options.


 git log --source a...b

where "b" will be correctly marked with "b", but "a" will be
marked with "a...b". Likewise with "a..b" (though you need
to use --boundary to even see "a" at all in that case).

To top off the confusion, when the REVARG_CANNOT_BE_FILENAME
flag is set, we skip the non-filename check, and leave the
NUL in place.

That means we do report the correct name for "a" in the
pending array. But some code paths try to show the whole
"a...b" name in error messages, and these erroneously show
only "a" instead of "a...b". E.g.:

 $ git cherry-pick HEAD:foo...HEAD:foo
 error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a 
commit
 error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a 
commit

 fatal: Invalid symmetric difference expression HEAD:foo

(That last message should be "HEAD:foo...HEAD:foo"; I used
cherry-pick because it passes the CANNOT_BE_FILENAME flag).

As an interesting side note, cherry-pick actually looks at
and re-resolves the arguments from the pending->name fields.
So it would have been visibly broken by the first bug, but
the effect was canceled out by the second one.

This patch makes the whole function consistent by re-writing
the NUL immediately after calling verify_non_filename(), and
then restoring the "." as appropriate in some error-printing
and early-return code paths.

Signed-off-by: Jeff King 

--
Philip



---
revision.c | 3 +++
t/t4202-log.sh | 9 +
2 files changed, 12 insertions(+)

diff --git a/revision.c b/revision.c
index 8a8c1789c..014bf52e3 100644
--- a/revision.c
+++ b/revision.c
@@ -1477,12 +1477,14 @@ int handle_revision_arg(const char *arg_, struct 
rev_info *revs, int flags, unsi

 if (!cant_be_filename) {
 *dotdot = '.';
 verify_non_filename(revs->prefix, arg);
+ *dotdot = '\0';
 }

 a_obj = parse_object(from_sha1);
 b_obj = parse_object(sha1);
 if (!a_obj || !b_obj) {
 missing:
+ *dotdot = '.';
 if (revs->ignore_missing)
 return 0;
 die(symmetric
@@ -1525,6 +1527,7 @@ int handle_revision_arg(const char *arg_, struct 
rev_info *revs, int flags, unsi

 REV_CMD_RIGHT, flags);
 add_pending_object(revs, a_obj, this);
 add_pending_object(revs, b_obj, next);
+ *dotdot = '.';
 return 0;
 }
 *dotdot = '.';
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 1c7d6729c..76c511973 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1392,4 +1392,13 @@ test_expect_success 'log --source paints tag names' 
'

 test_cmp expect actual
'

+test_expect_success 'log --source paints symmetric ranges' '
+ cat >expect <<-\EOF &&
+ 09e12a9 source-b three
+ 8e393e1 source-a two
+ EOF
+ git log --oneline --source source-a...source-b >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.13.0.387.gec0afcebb.dirty

Re: [PATCH 01/15] handle_revision_arg: reset "dotdot" consistently

[Jeff King]

On Sat, May 20, 2017 at 03:56:32PM +0100, Philip Oakley wrote:

> > That means we do report the correct name for "a" in the
> > pending array. But some code paths try to show the whole
> > "a..b" name in error messages, and these erroneously show
> > only "a" instead of "a..b". E.g.:
> > 
> >  $ git cherry-pick HEAD:foo..HEAD:foo
> 
> shouldn't this be three dots? Also the para above uses two dot examples in
> its description but the paras before that start by describing the three dot
> case.

Yeah, it should be. The problem happens with the two-dot case, too (it's
the same code) but to provoke cherry-pick to actually show the error in
question, you need to use three-dots. There's probably a way to provoke
a broken error message with the two-dot case, but I didn't dig further
after finding this one.

Thanks for being a careful reader. Here's the patch with a modified
commit message. I don't think this series otherwise needs a re-roll so
far.

-- >8 --
Subject: [PATCH] handle_revision_arg: reset "dotdot" consistently

When we are parsing a range like "a..b", we write a
temporary NUL over the first ".", so that we can access the
names "a" and "b" as C strings. But our restoration of the
original "." is done at inconsistent times, which can lead
to confusing results.

For most calls, we restore the "." after we resolve the
names, but before we call verify_non_filename().  This means
that when we later call add_pending_object(), the name for
the left-hand "a" has been re-expanded to "a..b". You can
see this with:

  git log --source a...b

where "b" will be correctly marked with "b", but "a" will be
marked with "a...b". Likewise with "a..b" (though you need
to use --boundary to even see "a" at all in that case).

To top off the confusion, when the REVARG_CANNOT_BE_FILENAME
flag is set, we skip the non-filename check, and leave the
NUL in place.

That means we do report the correct name for "a" in the
pending array. But some code paths try to show the whole
"a...b" name in error messages, and these erroneously show
only "a" instead of "a...b". E.g.:

  $ git cherry-pick HEAD:foo...HEAD:foo
  error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a commit
  error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a commit
  fatal: Invalid symmetric difference expression HEAD:foo

(That last message should be "HEAD:foo...HEAD:foo"; I used
cherry-pick because it passes the CANNOT_BE_FILENAME flag).

As an interesting side note, cherry-pick actually looks at
and re-resolves the arguments from the pending->name fields.
So it would have been visibly broken by the first bug, but
the effect was canceled out by the second one.

This patch makes the whole function consistent by re-writing
the NUL immediately after calling verify_non_filename(), and
then restoring the "." as appropriate in some error-printing
and early-return code paths.

Signed-off-by: Jeff King 
---
 revision.c | 3 +++
 t/t4202-log.sh | 9 +
 2 files changed, 12 insertions(+)

diff --git a/revision.c b/revision.c
index 8a8c1789c..014bf52e3 100644
--- a/revision.c
+++ b/revision.c
@@ -1477,12 +1477,14 @@ int handle_revision_arg(const char *arg_, struct 
rev_info *revs, int flags, unsi
if (!cant_be_filename) {
*dotdot = '.';
verify_non_filename(revs->prefix, arg);
+   *dotdot = '\0';
}
 
a_obj = parse_object(from_sha1);
b_obj = parse_object(sha1);
if (!a_obj || !b_obj) {
missing:
+   *dotdot = '.';
if (revs->ignore_missing)
return 0;
die(symmetric
@@ -1525,6 +1527,7 @@ int handle_revision_arg(const char *arg_, struct rev_info 
*revs, int flags, unsi
REV_CMD_RIGHT, flags);
add_pending_object(revs, a_obj, this);
add_pending_object(revs, b_obj, next);
+   *dotdot = '.';
return 0;
}
*dotdot = '.';
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 1c7d6729c..76c511973 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1392,4 +1392,13 @@ test_expect_success 'log --source paints tag names' '
test_cmp expect actual
 '
 
+test_expect_success 'log --source paints symmetric ranges' '
+   cat >expect <<-\EOF &&
+   09e12a9 source-b three
+   8e393e1 source-a two
+   EOF
+   git log --oneline --source source-a...source-b >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.13.0.387.gec0afcebb.dirty

Re: [PATCH 01/15] handle_revision_arg: reset "dotdot" consistently

[Philip Oakley]

From: "Jeff King" 

When we are parsing a range like "a..b", we write a
temporary NUL over the first ".", so that we can access the
names "a" and "b" as C strings. But our restoration of the
original "." is done at inconsistent times, which can lead
to confusing results.

For most calls, we restore the "." after we resolve the
names, but before we call verify_non_filename().  This means
that when we later call add_pending_object(), the name for
the left-hand "a" has been re-expanded to "a..b". You can
see this with:

 git log --source a...b

where "b" will be correctly marked with "b", but "a" will be
marked with "a...b". Likewise with "a..b" (though you need
to use --boundary to even see "a" at all in that case).

To top off the confusion, when the REVARG_CANNOT_BE_FILENAME
flag is set, we skip the non-filename check, and leave the
NUL in place.

That means we do report the correct name for "a" in the
pending array. But some code paths try to show the whole
"a..b" name in error messages, and these erroneously show
only "a" instead of "a..b". E.g.:

 $ git cherry-pick HEAD:foo..HEAD:foo


shouldn't this be three dots? Also the para above uses two dot examples in 
its description but the paras before that start by describing the three dot 
case.

--
Philip

 error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a 
commit
 error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a 
commit

 fatal: Invalid symmetric difference expression HEAD:foo

(That last message should be "HEAD:foo...HEAD:foo"; I used
cherry-pick because it passes the CANNOT_BE_FILENAME flag).

As an interesting side note, cherry-pick actually looks at
and re-resolves the arguments from the pending->name fields.
So it would have been visibly broken by the first bug, but
the effect was canceled out by the second one.

This patch makes the whole function consistent by re-writing
the NUL immediately after calling verify_non_filename(), and
then restoring the "." as appropriate in some error-printing
and early-return code paths.

Signed-off-by: Jeff King 
---
I also considered just making a copy of the string rather than this
in-place munging (technically we get it as a pointer-to-const; it's only
the use of strstr() that lets us quietly drop the const). But it doesn't
really make the code any cleaner; now instead of restoring the dot you
have to remember to free() the string in each code path.

revision.c | 3 +++
t/t4202-log.sh | 9 +
2 files changed, 12 insertions(+)

diff --git a/revision.c b/revision.c
index 8a8c1789c..014bf52e3 100644
--- a/revision.c
+++ b/revision.c
@@ -1477,12 +1477,14 @@ int handle_revision_arg(const char *arg_, struct 
rev_info *revs, int flags, unsi

 if (!cant_be_filename) {
 *dotdot = '.';
 verify_non_filename(revs->prefix, arg);
+ *dotdot = '\0';
 }

 a_obj = parse_object(from_sha1);
 b_obj = parse_object(sha1);
 if (!a_obj || !b_obj) {
 missing:
+ *dotdot = '.';
 if (revs->ignore_missing)
 return 0;
 die(symmetric
@@ -1525,6 +1527,7 @@ int handle_revision_arg(const char *arg_, struct 
rev_info *revs, int flags, unsi

 REV_CMD_RIGHT, flags);
 add_pending_object(revs, a_obj, this);
 add_pending_object(revs, b_obj, next);
+ *dotdot = '.';
 return 0;
 }
 *dotdot = '.';
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index f57799071..6da1bbe91 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1380,4 +1380,13 @@ test_expect_success 'log --source paints tag names' 
'

 test_cmp expect actual
'

+test_expect_success 'log --source paints symmetric ranges' '
+ cat >expect <<-\EOF &&
+ 09e12a9 source-b three
+ 8e393e1 source-a two
+ EOF
+ git log --oneline --source source-a...source-b >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.13.0.219.g63f6bc368

[PATCH 01/15] handle_revision_arg: reset "dotdot" consistently

[Jeff King]

When we are parsing a range like "a..b", we write a
temporary NUL over the first ".", so that we can access the
names "a" and "b" as C strings. But our restoration of the
original "." is done at inconsistent times, which can lead
to confusing results.

For most calls, we restore the "." after we resolve the
names, but before we call verify_non_filename().  This means
that when we later call add_pending_object(), the name for
the left-hand "a" has been re-expanded to "a..b". You can
see this with:

  git log --source a...b

where "b" will be correctly marked with "b", but "a" will be
marked with "a...b". Likewise with "a..b" (though you need
to use --boundary to even see "a" at all in that case).

To top off the confusion, when the REVARG_CANNOT_BE_FILENAME
flag is set, we skip the non-filename check, and leave the
NUL in place.

That means we do report the correct name for "a" in the
pending array. But some code paths try to show the whole
"a..b" name in error messages, and these erroneously show
only "a" instead of "a..b". E.g.:

  $ git cherry-pick HEAD:foo..HEAD:foo
  error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a commit
  error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a commit
  fatal: Invalid symmetric difference expression HEAD:foo

(That last message should be "HEAD:foo...HEAD:foo"; I used
cherry-pick because it passes the CANNOT_BE_FILENAME flag).

As an interesting side note, cherry-pick actually looks at
and re-resolves the arguments from the pending->name fields.
So it would have been visibly broken by the first bug, but
the effect was canceled out by the second one.

This patch makes the whole function consistent by re-writing
the NUL immediately after calling verify_non_filename(), and
then restoring the "." as appropriate in some error-printing
and early-return code paths.

Signed-off-by: Jeff King 
---
I also considered just making a copy of the string rather than this
in-place munging (technically we get it as a pointer-to-const; it's only
the use of strstr() that lets us quietly drop the const). But it doesn't
really make the code any cleaner; now instead of restoring the dot you
have to remember to free() the string in each code path.

 revision.c | 3 +++
 t/t4202-log.sh | 9 +
 2 files changed, 12 insertions(+)

diff --git a/revision.c b/revision.c
index 8a8c1789c..014bf52e3 100644
--- a/revision.c
+++ b/revision.c
@@ -1477,12 +1477,14 @@ int handle_revision_arg(const char *arg_, struct 
rev_info *revs, int flags, unsi
if (!cant_be_filename) {
*dotdot = '.';
verify_non_filename(revs->prefix, arg);
+   *dotdot = '\0';
}
 
a_obj = parse_object(from_sha1);
b_obj = parse_object(sha1);
if (!a_obj || !b_obj) {
missing:
+   *dotdot = '.';
if (revs->ignore_missing)
return 0;
die(symmetric
@@ -1525,6 +1527,7 @@ int handle_revision_arg(const char *arg_, struct rev_info 
*revs, int flags, unsi
REV_CMD_RIGHT, flags);
add_pending_object(revs, a_obj, this);
add_pending_object(revs, b_obj, next);
+   *dotdot = '.';
return 0;
}
*dotdot = '.';
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index f57799071..6da1bbe91 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1380,4 +1380,13 @@ test_expect_success 'log --source paints tag names' '
test_cmp expect actual
 '
 
+test_expect_success 'log --source paints symmetric ranges' '
+   cat >expect <<-\EOF &&
+   09e12a9 source-b three
+   8e393e1 source-a two
+   EOF
+   git log --oneline --source source-a...source-b >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.13.0.219.g63f6bc368