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.pbeftfyz2md4o...@sigill.intra.peff.net/

-Peff

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  0x0054edd7 in ref_iterator_advance (ref_iterator=0x91c5a0)
at refs/iterator.c:13
#2  0x0054f62f in do_for_each_ref_iterator (iter=0x91c5a0,
fn=0x56337a , cb_data=0x7fffcdb0)
at refs/iterator.c:382
#3  0x00546a40 in do_for_each_ref (refs=0x8ce3c0,
prefix=0x8c1af0 "refs/bisect/bad", fn=0x56337a ,
trim=15,
flags=0, cb_data=0x7fffcdb0) at refs.c:1298
#4  0x00546b2d in refs_for_each_ref_in (refs=0x8ce3c0,
prefix=0x8c1af0 "refs/bisect/bad", fn=0x56337a ,
cb_data=0x7fffcdb0) at refs.c:1319
#5  0x00546bf9 in for_each_ref_in_submodule (submodule=0x0,
prefix=0x8c1af0 "refs/bisect/bad", fn=0x56337a ,
cb_data=0x7fffcdb0) at refs.c:1340
#6  0x00566842 in for_each_bisect_ref (submodule=0x0,
fn=0x56337a , cb_data=0x7fffcdb0, term=0x8ce600
"bad")
at revision.c:2083
#7  0x00566885 in for_each_bad_bisect_ref (submodule=0x0,
fn=0x56337a , cb_data=0x7fffcdb0) at revision.c:2090
#8  0x00563541 in handle_refs (submodule=0x0, revs=0x7fffd210,
flags=0, for_each=0x566856 ) at revision.c:1196
#9  0x00566a09 in handle_revision_pseudo_opt (submodule=0x0,
revs=0x7fffd210, argc=1, argv=0x7fffdd28, flags=0x7fffcf44)
at revision.c:2125
#10 0x0056711e in setup_revisions (argc=2, argv=0x7fffdd20,
revs=0x7fffd210, opt=0x7fffd1f0) at revision.c:2247
#11 0x00448ce4 in cmd_log_init_finish (argc=2, argv=0x7fffdd20,
prefix=0x0, rev=0x7fffd210, opt=0x7fffd1f0) at builtin/log.c:168
#12 0x00448f53 in cmd_log_init (argc=2, argv=0x7fffdd20,
prefix=0x0, rev=0x7fffd210, opt=0x7fffd1f0) at builtin/log.c:220
#13 0x0044a37f in cmd_log (argc=2, argv=0x7fffdd20, prefix=0x0)
at builtin/log.c:692
#14 0x00405983 in run_builtin (p=0x870158 , argc=2,
argv=0x7fffdd20) at git.c:371
#15 0x00405bed in handle_builtin (argc=2, argv=0x7fffdd20)
at git.c:572
#16 0x00405d62 in run_argv (argcp=0x7fffdbdc,
argv=0x7fffdbd0)
at git.c:624
#17 0x00405f04 in cmd_main (argc=2, argv=0x7fffdd20) at
git.c:701
#18 0x00498ba6 in main (argc=3, argv=0x7fffdd18)
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

[BUG] b9c8e7f2fb6e breaks git bisect visualize

[Øyvind A . Holm]

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.9°
OpenPGP fingerprint: A006 05D6 E676 B319 55E2  E77E FB0C BEE8 94A5 06E5
dcacbb24-5094-11e7-b7e4-db5caa6d21d3


signature.asc
Description: PGP signature