[BUG] b9c8e7f2fb6e breaks git bisect visualize

[BUG] b9c8e7f2fb6e breaks git bisect visualize

[Øyvind A. Holm]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

Commit b9c8e7f2fb6e ("prefix_ref_iterator: don't trim too much") breaks 
git bisect visualize, this reproduces the bug:

  $ git bisect start
  $ git bisect bad
  $ git bisect good HEAD^^
  $ git bisect visualize
  fatal: BUG: attempt to trim too many characters
  $

Reverting b9c8e7f2fb6e makes git bisect visualize work again.

Tested on Debian GNU/Linux 8.8 (jessie).

Øyvind

N 60.37604° E 5.33339°
OpenPGP fingerprint: A006 05D6 E676 B319 55E2  E77E FB0C BEE8 94A5 06E5
dcacbb24-5094-11e7-b7e4-db5caa6d21d3

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [BUG] b9c8e7f2fb6e breaks git bisect visualize

[Michael Haggerty]

On 06/14/2017 02:06 AM, Øyvind A. Holm wrote:
> Commit b9c8e7f2fb6e ("prefix_ref_iterator: don't trim too much") breaks 
> git bisect visualize, this reproduces the bug:
> 
>   $ git bisect start
>   $ git bisect bad
>   $ git bisect good HEAD^^
>   $ git bisect visualize
>   fatal: BUG: attempt to trim too many characters
>   $
> 
> Reverting b9c8e7f2fb6e makes git bisect visualize work again.

Thanks for the bug report.

The same error occurs if the last step is simplified to

    git log --bisect

The corresponding stack trace is

#0  prefix_ref_iterator_advance (ref_iterator=0x91c5a0) at
refs/iterator.c:305
#1  0x000000000054edd7 in ref_iterator_advance (ref_iterator=0x91c5a0)
    at refs/iterator.c:13
#2  0x000000000054f62f in do_for_each_ref_iterator (iter=0x91c5a0,
    fn=0x56337a <handle_one_ref>, cb_data=0x7fffffffcdb0)
    at refs/iterator.c:382
#3  0x0000000000546a40 in do_for_each_ref (refs=0x8ce3c0,
    prefix=0x8c1af0 "refs/bisect/bad", fn=0x56337a <handle_one_ref>,
trim=15,
    flags=0, cb_data=0x7fffffffcdb0) at refs.c:1298
#4  0x0000000000546b2d in refs_for_each_ref_in (refs=0x8ce3c0,
    prefix=0x8c1af0 "refs/bisect/bad", fn=0x56337a <handle_one_ref>,
    cb_data=0x7fffffffcdb0) at refs.c:1319
#5  0x0000000000546bf9 in for_each_ref_in_submodule (submodule=0x0,
    prefix=0x8c1af0 "refs/bisect/bad", fn=0x56337a <handle_one_ref>,
    cb_data=0x7fffffffcdb0) at refs.c:1340
#6  0x0000000000566842 in for_each_bisect_ref (submodule=0x0,
    fn=0x56337a <handle_one_ref>, cb_data=0x7fffffffcdb0, term=0x8ce600
"bad")
    at revision.c:2083
#7  0x0000000000566885 in for_each_bad_bisect_ref (submodule=0x0,
    fn=0x56337a <handle_one_ref>, cb_data=0x7fffffffcdb0) at revision.c:2090
#8  0x0000000000563541 in handle_refs (submodule=0x0, revs=0x7fffffffd210,
    flags=0, for_each=0x566856 <for_each_bad_bisect_ref>) at revision.c:1196
#9  0x0000000000566a09 in handle_revision_pseudo_opt (submodule=0x0,
    revs=0x7fffffffd210, argc=1, argv=0x7fffffffdd28, flags=0x7fffffffcf44)
    at revision.c:2125
#10 0x000000000056711e in setup_revisions (argc=2, argv=0x7fffffffdd20,
    revs=0x7fffffffd210, opt=0x7fffffffd1f0) at revision.c:2247
#11 0x0000000000448ce4 in cmd_log_init_finish (argc=2, argv=0x7fffffffdd20,
    prefix=0x0, rev=0x7fffffffd210, opt=0x7fffffffd1f0) at builtin/log.c:168
#12 0x0000000000448f53 in cmd_log_init (argc=2, argv=0x7fffffffdd20,
    prefix=0x0, rev=0x7fffffffd210, opt=0x7fffffffd1f0) at builtin/log.c:220
#13 0x000000000044a37f in cmd_log (argc=2, argv=0x7fffffffdd20, prefix=0x0)
    at builtin/log.c:692
#14 0x0000000000405983 in run_builtin (p=0x870158 <commands+1176>, argc=2,
    argv=0x7fffffffdd20) at git.c:371
#15 0x0000000000405bed in handle_builtin (argc=2, argv=0x7fffffffdd20)
    at git.c:572
#16 0x0000000000405d62 in run_argv (argcp=0x7fffffffdbdc,
argv=0x7fffffffdbd0)
    at git.c:624
#17 0x0000000000405f04 in cmd_main (argc=2, argv=0x7fffffffdd20) at
git.c:701
#18 0x0000000000498ba6 in main (argc=3, argv=0x7fffffffdd18)
    at common-main.c:43

The code for `git log --bisect` is questionable. It calls
`for_each_ref_in_submodule()` with prefix "refs/bisect/bad", which is
the actual name (not a prefix) of the reference that it is interested
in. So the callback is called with the empty string as path, and that in
turn is passed to a variety of functions, like `ref_excluded()`,
`get_reference()`, `add_rev_cmdline()`, and `add_pending_oid()`. I'm not
sure whom to ping; the code in question was introduced eons ago:

    ad3f9a71a8 Add '--bisect' revision machinery argument, 2009-10-27

It seems to me that we should add a `for_each_fullref_in_submodule()`
and call that instead. I'll submit a patch doing that, though I'm not
certain that no new problems will arise from the callbacks getting full
rather than trimmed reference names (also for "refs/bisect/good").

Another possible orthogonal "fix" is to make the refs side tolerate
being asked to trim a refname down to the empty string, while still
refusing to trim even more than that. I'll also submit a patch to that
effect.

Either of the patches fix the issue that was reported and pass the whole
test suite (except for t1308, which seems to be broken in master for
unrelated reasons).

Michael

[PATCH 0/2] Fix a refname trimming problem in `log --bisect`

[Michael Haggerty]

The code for `git log --bisect` was calling
`for_each_ref_in_submodule()` with prefix set to "refs/bisect/bad",
which is the actual name of the reference that it wants. This resulted
in the refname being trimmed completely away and the empty string
being passed to the callback. That became impermissible after

    b9c8e7f2fb prefix_ref_iterator: don't trim too much, 2017-05-22

, so the command was failing.

Fix the problem in two orthogonal ways:

1. Add a new function, `for_each_fullref_in_submodule()`, that doesn't
   trim the refnames that it passes to callbacks, and us that instead.
   I *think* that this is a strict improvement, though I don't know
   the `git log` code well enough to be sure that it won't have bad
   side-effects.

2. Relax the "trimming too many characters" check to allow the full
   length of the refname to be trimmed away (though not more than
   that).

In an ideal world the second patch shouldn't be necessary, because
this calling pattern is questionable and it might be better that we
learn about any other offenders. But if we'd rather be conservative
and not break any other code that might rely on the old behavior,
patch 2 is my suggestion for how to do it.

This patch series can be applied on top of branch
`mh/packed-ref-store-prep`, but it also applies cleanly to master. It
is also available as branch `fix-bisect-trim-check` from my GitHub
fork [1].

Michael

[1] https://github.com/mhagger/git

Michael Haggerty (2):
  for_each_bisect_ref(): don't trim refnames
  prefix_ref_iterator_advance(): relax the check of trim length

 refs.c          | 12 ++++++++++++
 refs.h          |  5 ++++-
 refs/iterator.c |  8 ++++----
 revision.c      |  2 +-
 4 files changed, 21 insertions(+), 6 deletions(-)

-- 
2.11.0

[PATCH 1/2] for_each_bisect_ref(): don't trim refnames

[Michael Haggerty]

`for_each_bisect_ref()` is called by `for_each_bad_bisect_ref()` with
a term "bad". This used to make it call `for_each_ref_in_submodule()`
with a prefix "refs/bisect/bad". But the latter is the name of the
reference that is being sought, so the empty string was being passed
to the callback as the trimmed refname. Moreover, this questionable
practice was turned into an error by

    b9c8e7f2fb prefix_ref_iterator: don't trim too much, 2017-05-22

It makes more sense (and agrees better with the documentation of
`--bisect`) for the callers to receive the full reference names. So

* Add a new function, `for_each_fullref_in_submodule()`, to the refs
  API.

* Change `for_each_bad_bisect_ref()` to call the new function rather
  than `for_each_ref_in_submodule()`.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c     | 12 ++++++++++++
 refs.h     |  5 ++++-
 revision.c |  2 +-
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index f0685c9251..32177969f0 100644
--- a/refs.c
+++ b/refs.c
@@ -1341,6 +1341,18 @@ int for_each_ref_in_submodule(const char *submodule, const char *prefix,
 				    prefix, fn, cb_data);
 }
 
+int for_each_fullref_in_submodule(const char *submodule, const char *prefix,
+				  each_ref_fn fn, void *cb_data,
+				  unsigned int broken)
+{
+	unsigned int flag = 0;
+
+	if (broken)
+		flag = DO_FOR_EACH_INCLUDE_BROKEN;
+	return do_for_each_ref(get_submodule_ref_store(submodule),
+			       prefix, fn, 0, flag, cb_data);
+}
+
 int for_each_replace_ref(each_ref_fn fn, void *cb_data)
 {
 	return do_for_each_ref(get_main_ref_store(),
diff --git a/refs.h b/refs.h
index 4be14c4b3c..aa4ecc83d0 100644
--- a/refs.h
+++ b/refs.h
@@ -303,7 +303,10 @@ int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
 int for_each_ref_submodule(const char *submodule,
 			   each_ref_fn fn, void *cb_data);
 int for_each_ref_in_submodule(const char *submodule, const char *prefix,
-		each_ref_fn fn, void *cb_data);
+			      each_ref_fn fn, void *cb_data);
+int for_each_fullref_in_submodule(const char *submodule, const char *prefix,
+				  each_ref_fn fn, void *cb_data,
+				  unsigned int broken);
 int for_each_tag_ref_submodule(const char *submodule,
 			       each_ref_fn fn, void *cb_data);
 int for_each_branch_ref_submodule(const char *submodule,
diff --git a/revision.c b/revision.c
index 9c67cb6026..50039c92d6 100644
--- a/revision.c
+++ b/revision.c
@@ -2044,7 +2044,7 @@ static int for_each_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_d
 	struct strbuf bisect_refs = STRBUF_INIT;
 	int status;
 	strbuf_addf(&bisect_refs, "refs/bisect/%s", term);
-	status = for_each_ref_in_submodule(submodule, bisect_refs.buf, fn, cb_data);
+	status = for_each_fullref_in_submodule(submodule, bisect_refs.buf, fn, cb_data, 0);
 	strbuf_release(&bisect_refs);
 	return status;
 }
-- 
2.11.0

Re: [PATCH 1/2] for_each_bisect_ref(): don't trim refnames

[Jeff King]

On Wed, Jun 14, 2017 at 11:07:26AM +0200, Michael Haggerty wrote:

> `for_each_bisect_ref()` is called by `for_each_bad_bisect_ref()` with
> a term "bad". This used to make it call `for_each_ref_in_submodule()`
> with a prefix "refs/bisect/bad". But the latter is the name of the
> reference that is being sought, so the empty string was being passed
> to the callback as the trimmed refname. Moreover, this questionable
> practice was turned into an error by
> 
>     b9c8e7f2fb prefix_ref_iterator: don't trim too much, 2017-05-22
> 
> It makes more sense (and agrees better with the documentation of
> `--bisect`) for the callers to receive the full reference names. So
> 
> * Add a new function, `for_each_fullref_in_submodule()`, to the refs
>   API.

You might want to mention that this is really just a hole in the
existing API. We have for_each_ref_in_submodule() and
for_each_fullref_in(), but not the missing link.

I don't think that makes it any more or less correct, but I thought at
first you had to invent a new function totally.

> * Change `for_each_bad_bisect_ref()` to call the new function rather
>   than `for_each_ref_in_submodule()`.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c     | 12 ++++++++++++
>  refs.h     |  5 ++++-
>  revision.c |  2 +-
>  3 files changed, 17 insertions(+), 2 deletions(-)

The change itself looks fine to me.

Since we obviously don't have even a single test for "--bisect", that
might be worth adding.

-Peff

Re: [PATCH 1/2] for_each_bisect_ref(): don't trim refnames

[Jeff King]

On Wed, Jun 14, 2017 at 05:22:56AM -0400, Jeff King wrote:

> >  refs.c     | 12 ++++++++++++
> >  refs.h     |  5 ++++-
> >  revision.c |  2 +-
> >  3 files changed, 17 insertions(+), 2 deletions(-)
> 
> The change itself looks fine to me.
> 
> Since we obviously don't have even a single test for "--bisect", that
> might be worth adding.

It turns out we do, but none that actually check that we use the default
refnames. So maybe squash this in?

diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
index 3bf2759ea..534903bbd 100755
--- a/t/t6002-rev-list-bisect.sh
+++ b/t/t6002-rev-list-bisect.sh
@@ -235,4 +235,18 @@ test_sequence "--bisect"
 
 #
 #
+
+test_expect_success '--bisect can default to good/bad refs' '
+	git update-ref refs/bisect/bad c3 &&
+	good=$(git rev-parse b1) &&
+	git update-ref refs/bisect/good-$good $good &&
+	good=$(git rev-parse c1) &&
+	git update-ref refs/bisect/good-$good $good &&
+
+	# the only thing between c3 and c1 is c2
+	git rev-parse c2 >expect &&
+	git rev-list --bisect >actual &&
+	test_cmp expect actual
+'
+
 test_done

Re: [PATCH 1/2] for_each_bisect_ref(): don't trim refnames

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Since we obviously don't have even a single test for "--bisect", that
>> might be worth adding.
>
> It turns out we do, but none that actually check that we use the default
> refnames. So maybe squash this in?

Sounds sensible.  Thanks.

>
> diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
> index 3bf2759ea..534903bbd 100755
> --- a/t/t6002-rev-list-bisect.sh
> +++ b/t/t6002-rev-list-bisect.sh
> @@ -235,4 +235,18 @@ test_sequence "--bisect"
>  
>  #
>  #
> +
> +test_expect_success '--bisect can default to good/bad refs' '
> +	git update-ref refs/bisect/bad c3 &&
> +	good=$(git rev-parse b1) &&
> +	git update-ref refs/bisect/good-$good $good &&
> +	good=$(git rev-parse c1) &&
> +	git update-ref refs/bisect/good-$good $good &&
> +
> +	# the only thing between c3 and c1 is c2
> +	git rev-parse c2 >expect &&
> +	git rev-list --bisect >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done

Re: [PATCH 1/2] for_each_bisect_ref(): don't trim refnames

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> `for_each_bisect_ref()` is called by `for_each_bad_bisect_ref()` with
> a term "bad". This used to make it call `for_each_ref_in_submodule()`
> with a prefix "refs/bisect/bad". But the latter is the name of the
> reference that is being sought, so the empty string was being passed
> to the callback as the trimmed refname. Moreover, this questionable
> practice was turned into an error by
>
>     b9c8e7f2fb prefix_ref_iterator: don't trim too much, 2017-05-22
>
> It makes more sense (and agrees better with the documentation of
> `--bisect`) for the callers to receive the full reference names. So
>
> * Add a new function, `for_each_fullref_in_submodule()`, to the refs
>   API.
>
> * Change `for_each_bad_bisect_ref()` to call the new function rather
>   than `for_each_ref_in_submodule()`.

This unfortunately makes nd/prune-in-worktree topic rather
obsolete.  Can somebody volunteer to update it to newer codebase
including this fix?

Thanks.

[PATCH 2/2] prefix_ref_iterator_advance(): relax the check of trim length

[Michael Haggerty]

Before the previous commit, `for_each_bad_bisect_ref()` called
`for_each_fullref_in_submodule()` in such a way as to trim the whole
refname away. This is a questionable use of the API, but is not ipso
facto dangerous, so tolerate it in case there are other callers
relying on this behavior. But continue to refuse to trim *more*
characters than the refname contains, as that really makes no sense.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/iterator.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/refs/iterator.c b/refs/iterator.c
index 4cf449ef66..de52d5fe93 100644
--- a/refs/iterator.c
+++ b/refs/iterator.c
@@ -298,11 +298,11 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)
 			 * you haven't already checked for via a
 			 * prefix check, whether via this
 			 * `prefix_ref_iterator` or upstream in
-			 * `iter0`). So if there wouldn't be at least
-			 * one character left in the refname after
-			 * trimming, report it as a bug:
+			 * `iter0`. So consider it a bug if we are
+			 * asked to trim off more characters than the
+			 * refname contains:
 			 */
-			if (strlen(iter->iter0->refname) <= iter->trim)
+			if (strlen(iter->iter0->refname) < iter->trim)
 				die("BUG: attempt to trim too many characters");
 			iter->base.refname = iter->iter0->refname + iter->trim;
 		} else {
-- 
2.11.0

Re: [PATCH 0/2] Fix a refname trimming problem in `log --bisect`

[Jeff King]

On Wed, Jun 14, 2017 at 11:07:25AM +0200, Michael Haggerty wrote:

> Fix the problem in two orthogonal ways:
> 
> 1. Add a new function, `for_each_fullref_in_submodule()`, that doesn't
>    trim the refnames that it passes to callbacks, and us that instead.
>    I *think* that this is a strict improvement, though I don't know
>    the `git log` code well enough to be sure that it won't have bad
>    side-effects.

I think this is fine, for the reasons I gave elsewhere in the thread.

> 2. Relax the "trimming too many characters" check to allow the full
>    length of the refname to be trimmed away (though not more than
>    that).
> 
> In an ideal world the second patch shouldn't be necessary, because
> this calling pattern is questionable and it might be better that we
> learn about any other offenders. But if we'd rather be conservative
> and not break any other code that might rely on the old behavior,
> patch 2 is my suggestion for how to do it.

My preference would be to hold off on (2) if we can avoid it. It's
cleaner, and I think flushing out these kinds of bugs is useful.

-Peff

Re: [PATCH 0/2] Fix a refname trimming problem in `log --bisect`

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> The code for `git log --bisect` was calling
> `for_each_ref_in_submodule()` with prefix set to "refs/bisect/bad",
> which is the actual name of the reference that it wants. This resulted
> in the refname being trimmed completely away and the empty string
> being passed to the callback. That became impermissible after
>
>     b9c8e7f2fb prefix_ref_iterator: don't trim too much, 2017-05-22
>
> , so the command was failing.
>
> Fix the problem in two orthogonal ways:
>
> 1. Add a new function, `for_each_fullref_in_submodule()`, that doesn't
>    trim the refnames that it passes to callbacks, and us that instead.
>    I *think* that this is a strict improvement, though I don't know
>    the `git log` code well enough to be sure that it won't have bad
>    side-effects.
>
> 2. Relax the "trimming too many characters" check to allow the full
>    length of the refname to be trimmed away (though not more than
>    that).
>
> In an ideal world the second patch shouldn't be necessary, because
> this calling pattern is questionable and it might be better that we
> learn about any other offenders. But if we'd rather be conservative
> and not break any other code that might rely on the old behavior,
> patch 2 is my suggestion for how to do it.

Thanks for a nice summary.  

I agree that 2. is a nice safety to have, especially if the code
before b9c8e7f2 ("prefix_ref_iterator: don't trim too much",
2017-05-22) has been seeing the completely trimmed result (i.e. an
empty string) in the callback function.

And I agree that 1. is also a good interface to have.

Re: [BUG] b9c8e7f2fb6e breaks git bisect visualize

[Jeff King]

On Wed, Jun 14, 2017 at 10:36:43AM +0200, Michael Haggerty wrote:

> The code for `git log --bisect` is questionable. It calls
> `for_each_ref_in_submodule()` with prefix "refs/bisect/bad", which is
> the actual name (not a prefix) of the reference that it is interested
> in. So the callback is called with the empty string as path, and that in
> turn is passed to a variety of functions, like `ref_excluded()`,
> `get_reference()`, `add_rev_cmdline()`, and `add_pending_oid()`. I'm not
> sure whom to ping; the code in question was introduced eons ago:
> 
>     ad3f9a71a8 Add '--bisect' revision machinery argument, 2009-10-27
> 
> It seems to me that we should add a `for_each_fullref_in_submodule()`
> and call that instead. I'll submit a patch doing that, though I'm not
> certain that no new problems will arise from the callbacks getting full
> rather than trimmed reference names (also for "refs/bisect/good").

I doubt that would be a problem. The current values are nonsensical (an
empty string for bad, and "-$sha1" for good. They're mostly used in the
cmdline and pending lists. It would affect things like --exclude or
--source. I doubt anybody cares, but if they do IMHO the full names
would be a vast improvement.

Another option would be for this code to ask for:

  for_each_ref_in("refs/bisect", ...);

and then match the various names in the callback. That gives sane short
names ("bad" and "good-$sha1"). But I think the full names are a much
better outcome. Some code (though note code I'd expect to use with
--bisect) assumes that the contents of the "name" field for pending
objects can be used to look up the object again. That's definitely not
true of the nonsense we produce now, but it would also not be true of
"bad" (because we don't DWIM refs/bisect).

> Another possible orthogonal "fix" is to make the refs side tolerate
> being asked to trim a refname down to the empty string, while still
> refusing to trim even more than that. I'll also submit a patch to that
> effect.

Even though the resulting "name" is silly, it does seem possible that
some caller would want to ask for all of refs/foo, even if it didn't
know if that was a single ref or a hierarchy. I do agree that any such
caller should probably be using for_each_fullref_in, though.

> Either of the patches fix the issue that was reported and pass the whole
> test suite (except for t1308, which seems to be broken in master for
> unrelated reasons).

It's broken if you use autoconf. See the patch I posted a few hours ago:

  http://public-inbox.org/git/20170614053018.pbeftfyz2md4o73h@sigill.intra.peff.net/

-Peff