* [PATCH 1/2] for_each_bisect_ref(): don't trim refnames
2017-06-14 9:07 ` [PATCH 0/2] Fix a refname trimming problem in `log --bisect` Michael Haggerty
@ 2017-06-14 9:07 ` Michael Haggerty
2017-06-14 9:22 ` Jeff King
2017-06-15 22:26 ` Junio C Hamano
2017-06-14 9:07 ` [PATCH 2/2] prefix_ref_iterator_advance(): relax the check of trim length Michael Haggerty
` (2 subsequent siblings)
3 siblings, 2 replies; 12+ messages in thread
From: Michael Haggerty @ 2017-06-14 9:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller, Jeff King,
Ævar Arnfjörð Bjarmason, David Turner,
Brandon Williams, Johannes Sixt, Øyvind Holm, Linus Torvalds,
git, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] for_each_bisect_ref(): don't trim refnames
2017-06-14 9:07 ` [PATCH 1/2] for_each_bisect_ref(): don't trim refnames Michael Haggerty
@ 2017-06-14 9:22 ` Jeff King
2017-06-14 9:32 ` Jeff King
2017-06-15 22:26 ` Junio C Hamano
1 sibling, 1 reply; 12+ messages in thread
From: Jeff King @ 2017-06-14 9:22 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Stefan Beller, Ævar Arnfjörð Bjarmason,
David Turner, Brandon Williams, Johannes Sixt, Øyvind Holm,
Linus Torvalds, git
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] for_each_bisect_ref(): don't trim refnames
2017-06-14 9:22 ` Jeff King
@ 2017-06-14 9:32 ` Jeff King
2017-06-14 10:05 ` Junio C Hamano
0 siblings, 1 reply; 12+ messages in thread
From: Jeff King @ 2017-06-14 9:32 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Stefan Beller, Ævar Arnfjörð Bjarmason,
David Turner, Brandon Williams, Johannes Sixt, Øyvind Holm,
Linus Torvalds, git
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] for_each_bisect_ref(): don't trim refnames
2017-06-14 9:32 ` Jeff King
@ 2017-06-14 10:05 ` Junio C Hamano
0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2017-06-14 10:05 UTC (permalink / raw)
To: Jeff King
Cc: Michael Haggerty, Nguyễn Thái Ngọc Duy,
Stefan Beller, Ævar Arnfjörð Bjarmason,
David Turner, Brandon Williams, Johannes Sixt, Øyvind Holm,
Linus Torvalds, git
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] for_each_bisect_ref(): don't trim refnames
2017-06-14 9:07 ` [PATCH 1/2] for_each_bisect_ref(): don't trim refnames Michael Haggerty
2017-06-14 9:22 ` Jeff King
@ 2017-06-15 22:26 ` Junio C Hamano
1 sibling, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2017-06-15 22:26 UTC (permalink / raw)
To: Michael Haggerty
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller, Jeff King,
Ævar Arnfjörð Bjarmason, David Turner,
Brandon Williams, Johannes Sixt, Øyvind Holm, Linus Torvalds,
git
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] prefix_ref_iterator_advance(): relax the check of trim length
2017-06-14 9:07 ` [PATCH 0/2] Fix a refname trimming problem in `log --bisect` Michael Haggerty
2017-06-14 9:07 ` [PATCH 1/2] for_each_bisect_ref(): don't trim refnames Michael Haggerty
@ 2017-06-14 9:07 ` Michael Haggerty
2017-06-14 9:24 ` [PATCH 0/2] Fix a refname trimming problem in `log --bisect` Jeff King
2017-06-14 10:09 ` Junio C Hamano
3 siblings, 0 replies; 12+ messages in thread
From: Michael Haggerty @ 2017-06-14 9:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller, Jeff King,
Ævar Arnfjörð Bjarmason, David Turner,
Brandon Williams, Johannes Sixt, Øyvind Holm, Linus Torvalds,
git, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] Fix a refname trimming problem in `log --bisect`
2017-06-14 9:07 ` [PATCH 0/2] Fix a refname trimming problem in `log --bisect` Michael Haggerty
2017-06-14 9:07 ` [PATCH 1/2] for_each_bisect_ref(): don't trim refnames Michael Haggerty
2017-06-14 9:07 ` [PATCH 2/2] prefix_ref_iterator_advance(): relax the check of trim length Michael Haggerty
@ 2017-06-14 9:24 ` Jeff King
2017-06-14 10:09 ` Junio C Hamano
3 siblings, 0 replies; 12+ messages in thread
From: Jeff King @ 2017-06-14 9:24 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Stefan Beller, Ævar Arnfjörð Bjarmason,
David Turner, Brandon Williams, Johannes Sixt, Øyvind Holm,
Linus Torvalds, git
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] Fix a refname trimming problem in `log --bisect`
2017-06-14 9:07 ` [PATCH 0/2] Fix a refname trimming problem in `log --bisect` Michael Haggerty
` (2 preceding siblings ...)
2017-06-14 9:24 ` [PATCH 0/2] Fix a refname trimming problem in `log --bisect` Jeff King
@ 2017-06-14 10:09 ` Junio C Hamano
3 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2017-06-14 10:09 UTC (permalink / raw)
To: Michael Haggerty
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller, Jeff King,
Ævar Arnfjörð Bjarmason, David Turner,
Brandon Williams, Johannes Sixt, Øyvind Holm, Linus Torvalds,
git
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.
^ permalink raw reply [flat|nested] 12+ messages in thread