| Message ID | 11edd3e4dbaac7fada8a3bcd43f4bbd353087637.1651796862.git.chris@chrisdown.name (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bisect: status improvements when bisect is not fully fleshed out | expand |
On Fri, May 06, 2022 at 01:52:46AM +0100, Chris Down wrote: > In order to avoid situations like this, make it more clear what we're > waiting for: > > $ git bisect start d93ff48803f0 v6.3 > status: waiting for good commit(s), bad commit known Makes sense. It would be kind of nice to realize that (in your example) "v6.3" likely wasn't a pathspec that matched any files, either, and after trying to DWIM it into something sensible, printed an error and quit. But I think the behavior here is slightly more subtle, since we really care about whether or not a pathspec would match any revisions along the bisection, not just the tips or the currently checked-out revision. So I think the approach here makes sense. > +static void bisect_print_status(const struct bisect_terms *terms) > +{ > + const struct bisect_state bs = bisect_status(terms); > + > + /* If we had both, we'd already be started, and shouldn't get here. */ > + if (bs.nr_good && bs.nr_bad) > + return; > + > + if (!bs.nr_good && !bs.nr_bad) > + printf(_("status: waiting for both good and bad commits\n")); > + else if (bs.nr_good) > + printf(Q_("status: waiting for bad commit, %d good commit known\n", > + "status: waiting for bad commit, %d good commits known\n", > + bs.nr_good), bs.nr_good); > + else > + printf(_("status: waiting for good commit(s), bad commit known\n")); > +} Could or should these printf()'s be advise() calls instead? That would make it easier for users to turn them off if they don't want the extra output. At the very least, we should make sure that they are sent to stderr to discourage scripting around them. Thanks, Taylor
Taylor Blau writes: >Could or should these printf()'s be advise() calls instead? That would >make it easier for users to turn them off if they don't want the extra >output. At the very least, we should make sure that they are sent to >stderr to discourage scripting around them. Ah, I didn't know about advise(); looks like it could be reasonable to use here. If nobody objects, I'll change to advise() when the rest of the feedback has come in and v3 is sent. Thanks!
Taylor Blau <me@ttaylorr.com> writes: >> + if (!bs.nr_good && !bs.nr_bad) >> + printf(_("status: waiting for both good and bad commits\n")); >> + else if (bs.nr_good) >> + printf(Q_("status: waiting for bad commit, %d good commit known\n", >> + "status: waiting for bad commit, %d good commits known\n", >> + bs.nr_good), bs.nr_good); >> + else >> + printf(_("status: waiting for good commit(s), bad commit known\n")); >> +} > > Could or should these printf()'s be advise() calls instead? Given that existing bisect_next_all() mesasge to give estimates come to the standard output, I do not think so. /* * TRANSLATORS: the last %s will be replaced with "(roughly %d * steps)" translation. */ printf(Q_("Bisecting: %d revision left to test after this %s\n", "Bisecting: %d revisions left to test after this %s\n", nr), nr, steps_msg); I view these new messages as merely correcting the gap we used to have. We should have been giving feedback to the end-user when they did something, but instead we were giving feedback only when we did something, which resulted in the original "Huh?" that motivated this series. I actually wonder if we should do s/status:/Bisecting:/ to make the messages even more uniform, but if we were to go in that direction in the longer term, we'd probably be downcasing "Bisecting" to match our error/warning/info messages. Thanks.
Chris Down <chris@chrisdown.name> writes: > diff --git a/bisect.h b/bisect.h > index 1015aeb8ea..ccd9ad31f6 100644 > --- a/bisect.h > +++ b/bisect.h > @@ -62,6 +62,12 @@ enum bisect_error { > BISECT_INTERNAL_SUCCESS_MERGE_BASE = -11 > }; > > +struct bisect_state { > + int nr_good; /* How many good commits do we have? */ > + int nr_bad; /* How many bad commits do we have? (Well, you can only > + have 0 or 1, but...) */ > +}; ;-) Multi-line comment style (cf. Documentation/CodingGuidelines)? > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 8b2b259ff0..9d583f651c 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -329,12 +329,12 @@ static int check_and_set_terms(struct bisect_terms *terms, const char *cmd) > return 0; > } > > -static int mark_good(const char *refname, const struct object_id *oid, > - int flag, void *cb_data) > +static int inc_nr(const char *refname, const struct object_id *oid, > + int flag, void *cb_data) > { > - int *m_good = (int *)cb_data; > - *m_good = 0; > - return 1; > + int *nr = (int *)cb_data; > + (*nr)++; > + return 0; > } "mark_good" used to be a way to see if there is _any_ ref that match the glob "refs/bisect/good-*"---the missing_good variable in the caller is initialized to "true" (pessimism) and this callback is called for each such ref. Because the first such call is enough to say "yes, we do have such a ref" without finding the second or subsequent "good" ones, the function used to return 1 to stop for_each_glob_ref_in() early. All of that is now different here. The helper is to "count" the refs that match the glob. So the caller will instead initialize the counter to 0, and each time this callback is called, we increment it, and because we do not want to stop the iteration early, we return 0. All makes sense. > static const char need_bad_and_good_revision_warning[] = > @@ -384,23 +384,49 @@ static int decide_next(const struct bisect_terms *terms, > vocab_good, vocab_bad, vocab_good, vocab_bad); > } > > -static int bisect_next_check(const struct bisect_terms *terms, > - const char *current_term) > +static struct bisect_state bisect_status(const struct bisect_terms *terms) > { > - int missing_good = 1, missing_bad = 1; > char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad); > char *good_glob = xstrfmt("%s-*", terms->term_good); > + struct bisect_state bs; > + > + memset(&bs, 0, sizeof(bs)); We should just zero-initialize the struct by struct bisect_state bs = { 0 }; without memset(). But see below > if (ref_exists(bad_ref)) > - missing_bad = 0; > + bs.nr_bad = 1; > > - for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/", > - (void *) &missing_good); > + for_each_glob_ref_in(inc_nr, good_glob, "refs/bisect/", > + (void *) &bs.nr_good); > free(good_glob); > free(bad_ref); > > - return decide_next(terms, current_term, missing_good, missing_bad); > + return bs; > +} Passing struct by value? It is more usual in this codebase for the caller to prepare a struct and give a pointer to it to a helper function like this one, i.e. static void bisect_status(struct bisect_state *state, const struct bisect_terms *terms) { ... for_each_glob_ref_in(count_matches, good_glob, "refs/bisect", (void *) &state->nr_good); } static void bisect_print_status(const struct bisect_terms *terms) { struct bisect_state state = { 0 }; bisect_status(&state, terms); ... would be more common. > diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh > index 5382e5d216..a02587d1a7 100755 > --- a/t/t6030-bisect-porcelain.sh > +++ b/t/t6030-bisect-porcelain.sh > @@ -1025,4 +1025,22 @@ test_expect_success 'bisect visualize with a filename with dash and space' ' > git bisect visualize -p -- "-hello 2" > ' > > +test_expect_success 'bisect state output with multiple good commits' ' > + git bisect reset && > + git bisect start >output && > + grep "waiting for both good and bad commits" output && > + git bisect good "$HASH1" >output && > + grep "waiting for bad commit, 1 good commit known" output && > + git bisect good "$HASH2" >output && > + grep "waiting for bad commit, 2 good commits known" output > +' > + > +test_expect_success 'bisect state output with bad commit' ' > + git bisect reset && > + git bisect start >output && > + grep "waiting for both good and bad commits" output && > + git bisect bad "$HASH4" >output && > + grep -F "waiting for good commit(s), bad commit known" output > +' > + > test_done Looking good. Thanks.
diff --git a/bisect.h b/bisect.h index 1015aeb8ea..ccd9ad31f6 100644 --- a/bisect.h +++ b/bisect.h @@ -62,6 +62,12 @@ enum bisect_error { BISECT_INTERNAL_SUCCESS_MERGE_BASE = -11 }; +struct bisect_state { + int nr_good; /* How many good commits do we have? */ + int nr_bad; /* How many bad commits do we have? (Well, you can only + have 0 or 1, but...) */ +}; + enum bisect_error bisect_next_all(struct repository *r, const char *prefix); int estimate_bisect_steps(int all); diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 8b2b259ff0..9d583f651c 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -329,12 +329,12 @@ static int check_and_set_terms(struct bisect_terms *terms, const char *cmd) return 0; } -static int mark_good(const char *refname, const struct object_id *oid, - int flag, void *cb_data) +static int inc_nr(const char *refname, const struct object_id *oid, + int flag, void *cb_data) { - int *m_good = (int *)cb_data; - *m_good = 0; - return 1; + int *nr = (int *)cb_data; + (*nr)++; + return 0; } static const char need_bad_and_good_revision_warning[] = @@ -384,23 +384,49 @@ static int decide_next(const struct bisect_terms *terms, vocab_good, vocab_bad, vocab_good, vocab_bad); } -static int bisect_next_check(const struct bisect_terms *terms, - const char *current_term) +static struct bisect_state bisect_status(const struct bisect_terms *terms) { - int missing_good = 1, missing_bad = 1; char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad); char *good_glob = xstrfmt("%s-*", terms->term_good); + struct bisect_state bs; + + memset(&bs, 0, sizeof(bs)); if (ref_exists(bad_ref)) - missing_bad = 0; + bs.nr_bad = 1; - for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/", - (void *) &missing_good); + for_each_glob_ref_in(inc_nr, good_glob, "refs/bisect/", + (void *) &bs.nr_good); free(good_glob); free(bad_ref); - return decide_next(terms, current_term, missing_good, missing_bad); + return bs; +} + +static void bisect_print_status(const struct bisect_terms *terms) +{ + const struct bisect_state bs = bisect_status(terms); + + /* If we had both, we'd already be started, and shouldn't get here. */ + if (bs.nr_good && bs.nr_bad) + return; + + if (!bs.nr_good && !bs.nr_bad) + printf(_("status: waiting for both good and bad commits\n")); + else if (bs.nr_good) + printf(Q_("status: waiting for bad commit, %d good commit known\n", + "status: waiting for bad commit, %d good commits known\n", + bs.nr_good), bs.nr_good); + else + printf(_("status: waiting for good commit(s), bad commit known\n")); +} + +static int bisect_next_check(const struct bisect_terms *terms, + const char *current_term) +{ + const struct bisect_state bs = bisect_status(terms); + return decide_next(terms, current_term, !bs.nr_good, !bs.nr_bad); } static int get_terms(struct bisect_terms *terms) @@ -606,8 +632,10 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix) { - if (bisect_next_check(terms, NULL)) + if (bisect_next_check(terms, NULL)) { + bisect_print_status(terms); return BISECT_OK; + } return bisect_next(terms, prefix); } diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 5382e5d216..a02587d1a7 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -1025,4 +1025,22 @@ test_expect_success 'bisect visualize with a filename with dash and space' ' git bisect visualize -p -- "-hello 2" ' +test_expect_success 'bisect state output with multiple good commits' ' + git bisect reset && + git bisect start >output && + grep "waiting for both good and bad commits" output && + git bisect good "$HASH1" >output && + grep "waiting for bad commit, 1 good commit known" output && + git bisect good "$HASH2" >output && + grep "waiting for bad commit, 2 good commits known" output +' + +test_expect_success 'bisect state output with bad commit' ' + git bisect reset && + git bisect start >output && + grep "waiting for both good and bad commits" output && + git bisect bad "$HASH4" >output && + grep -F "waiting for good commit(s), bad commit known" output +' + test_done
Commit 73c6de06aff8 ("bisect: don't use invalid oid as rev when starting") changes the behaviour of `git bisect` to consider invalid oids as pathspecs again, as in the old shell implementation. While that behaviour may be desirable, it can also cause confusion. For example, while bisecting in a particular repo I encountered this: $ git bisect start d93ff48803f0 v6.3 $ ...which led to me sitting for a few moments, wondering why there's no printout stating the first rev to check. It turns out that the tag was actually "6.3", not "v6.3", and thus the bisect was still silently started with only a bad rev, because d93ff48803f0 was a valid oid and "v6.3" was silently considered to be a pathspec. While this behaviour may be desirable, it can be confusing, especially with different repo conventions either using or not using "v" before release names, or when a branch name or tag is simply misspelled on the command line. In order to avoid situations like this, make it more clear what we're waiting for: $ git bisect start d93ff48803f0 v6.3 status: waiting for good commit(s), bad commit known We already have good output once the bisect process has begun in earnest, so we don't need to do anything more there. Signed-off-by: Chris Down <chris@chrisdown.name> --- bisect.h | 6 +++++ builtin/bisect--helper.c | 54 ++++++++++++++++++++++++++++--------- t/t6030-bisect-porcelain.sh | 18 +++++++++++++ 3 files changed, 65 insertions(+), 13 deletions(-)