| Message ID | acab8859d02c95750fdbc691ac672c17d5be0291.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:54AM +0100, Chris Down wrote: > This allows seeing the current intermediate status without adding a new > good or bad commit: > > $ git bisect log | tail -1 > # status: waiting for bad commit, 1 good commit known Hmm. I was worried that this would make it harder to turn the output of "git bisect log" into something you can inject into "git bisect replay <log>". But it doesn't, because you prefix the status lines with a '#' character. That's good, and I think it's an improvement over what I'd currently recommend, which would be something like: git bisect log | grep '^# bad:' git bisect log | grep '^# good:' to see the state of our good and bad endpoints. I'm not totally convinced it _needs_ to live in "git bisect log", though, since it feels like additional information that is just added for convenience. That's not the worst thing in the world, but I think it would be fine to just take the first patch (with my suggestions applied) as well. > Signed-off-by: Chris Down <chris@chrisdown.name> > --- > builtin/bisect--helper.c | 25 ++++++++++++++++++++----- > t/t6030-bisect-porcelain.sh | 9 +++++++-- > 2 files changed, 27 insertions(+), 7 deletions(-) > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 9d583f651c..ef75f0a0ce 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -404,6 +404,21 @@ static struct bisect_state bisect_status(const struct bisect_terms *terms) > return bs; > } > > +__attribute__((format (printf, 1, 2))) > +static void bisect_log_printf(const char *fmt, ...) > +{ > + va_list ap; > + char buf[1024]; > + > + va_start(ap, fmt); > + if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0) > + *buf = '\0'; > + va_end(ap); > + > + printf("%s", buf); > + append_to_file(git_path_bisect_log(), "# %s", buf); > +} This direct use of vsnprintf might be avoided by preparing the output in bisect_print_status() via a strbuf and then calling: append_to_file(git_path_bisect_log(), "# %s", buf.buf). > static void bisect_print_status(const struct bisect_terms *terms) > { > const struct bisect_state bs = bisect_status(terms); > @@ -413,13 +428,13 @@ static void bisect_print_status(const struct bisect_terms *terms) > return; > > if (!bs.nr_good && !bs.nr_bad) > - printf(_("status: waiting for both good and bad commits\n")); > + bisect_log_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); > + bisect_log_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")); > + bisect_log_printf(_("status: waiting for good commit(s), bad commit known\n")); > } Interesting; this patch removes the output that we were giving to users in the last patch. Should it go to both places? Thanks, Taylor
Taylor Blau writes: >On Fri, May 06, 2022 at 01:52:54AM +0100, Chris Down wrote: >> This allows seeing the current intermediate status without adding a new >> good or bad commit: >> >> $ git bisect log | tail -1 >> # status: waiting for bad commit, 1 good commit known > >Hmm. I was worried that this would make it harder to turn the output of >"git bisect log" into something you can inject into "git bisect replay ><log>". But it doesn't, because you prefix the status lines with a '#' >character. > >That's good, and I think it's an improvement over what I'd currently >recommend, which would be something like: > > git bisect log | grep '^# bad:' > git bisect log | grep '^# good:' > >to see the state of our good and bad endpoints. > >I'm not totally convinced it _needs_ to live in "git bisect log", >though, since it feels like additional information that is just added >for convenience. That's not the worst thing in the world, but I think >it would be fine to just take the first patch (with my suggestions >applied) as well. There was some discussion in the v1 thread (Message-ID: <xmqqv8uo1mk6.fsf@gitster.g>) about adding an additional `git bisect status' command, but while writing it my immediate thoughts were that it doesn't make much sense to separate from the rest of the log. I'm curious what Junio's thoughts are on that, happy to do it whichever way is preferred. :-) >> Signed-off-by: Chris Down <chris@chrisdown.name> >> --- >> builtin/bisect--helper.c | 25 ++++++++++++++++++++----- >> t/t6030-bisect-porcelain.sh | 9 +++++++-- >> 2 files changed, 27 insertions(+), 7 deletions(-) >> >> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c >> index 9d583f651c..ef75f0a0ce 100644 >> --- a/builtin/bisect--helper.c >> +++ b/builtin/bisect--helper.c >> @@ -404,6 +404,21 @@ static struct bisect_state bisect_status(const struct bisect_terms *terms) >> return bs; >> } >> >> +__attribute__((format (printf, 1, 2))) >> +static void bisect_log_printf(const char *fmt, ...) >> +{ >> + va_list ap; >> + char buf[1024]; >> + >> + va_start(ap, fmt); >> + if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0) >> + *buf = '\0'; >> + va_end(ap); >> + >> + printf("%s", buf); >> + append_to_file(git_path_bisect_log(), "# %s", buf); >> +} > >This direct use of vsnprintf might be avoided by preparing the output in >bisect_print_status() via a strbuf and then calling: > > append_to_file(git_path_bisect_log(), "# %s", buf.buf). I'm not intimately familiar with the codebase, so maybe I'm missing something, but isn't it overkill to use a string buffer for something which is isn't going to then be used as a mutable buffer? Happy to do it whichever way works for you folks, but would be good to understand the rationale so that I can write better patches next time :-) >> static void bisect_print_status(const struct bisect_terms *terms) >> { >> const struct bisect_state bs = bisect_status(terms); >> @@ -413,13 +428,13 @@ static void bisect_print_status(const struct bisect_terms *terms) >> return; >> >> if (!bs.nr_good && !bs.nr_bad) >> - printf(_("status: waiting for both good and bad commits\n")); >> + bisect_log_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); >> + bisect_log_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")); >> + bisect_log_printf(_("status: waiting for good commit(s), bad commit known\n")); >> } > >Interesting; this patch removes the output that we were giving to users >in the last patch. Should it go to both places? Not sure I understand, we printf() in bisect_log_printf, no?
Chris Down <chris@chrisdown.name> writes: > This allows seeing the current intermediate status without adding a new > good or bad commit: > > $ git bisect log | tail -1 > # status: waiting for bad commit, 1 good commit known Clever. As we are keeping the "log" of what we have done so far, it is a good place to record this new piece of info, too. I find it much better than my earlier suggestion to add "bisect status" subcommand. > +__attribute__((format (printf, 1, 2))) > +static void bisect_log_printf(const char *fmt, ...) > +{ > + va_list ap; > + char buf[1024]; > + > + va_start(ap, fmt); > + if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0) > + *buf = '\0'; > + va_end(ap); We probably should use strbuf_addf() or something so that we do not even have to wonder if 1024 is large enough for everybody. That would be in line with what is done in other parts of the codebase, e.g. bisect_next_all() uses xstrfmt() that is a thin wrapper around strbuf to format the estimated steps. Thanks.
Taylor Blau <me@ttaylorr.com> writes: > On Fri, May 06, 2022 at 01:52:54AM +0100, Chris Down wrote: >> This allows seeing the current intermediate status without adding a new >> good or bad commit: >> >> $ git bisect log | tail -1 >> # status: waiting for bad commit, 1 good commit known > > Hmm. I was worried that this would make it harder to turn the output of > "git bisect log" into something you can inject into "git bisect replay > <log>". But it doesn't, because you prefix the status lines with a '#' > character. ;-) It has always been the basic design of "git bisect replay" since the old days back when the command was scripted. The log is designed to be an executable shell script and we do not need to use "replay" at all. > I'm not totally convinced it _needs_ to live in "git bisect log", It is a convenient place and other "helpful" comments are also stored there, so the existing users may already know to look into it to reorient where they were. The original suggestion during the previous round of this series was to come up with a way to help those who get lost during a long session of spelunking the history to start bisection, like so: * start a bisection session * find one end (perhaps bad end) * say "git bisect bad $this" and be told "now go find a good one" * ran around various versions with "git reset --hard $that && make" to see the other end (perhaps good end). * forget where they were---was it their turn to feed a good revision or bad revision? * "git bisect status" can be used to remind them that they were told "now go find a good one". And I actually find that "go to log and see what progress you made so far" as implemented in this series a far easier and scalable solution than "git bisect status". Next time we add more "here is an extra piece of information to help reorient yourself", we do not have to worry about how to present it in "git bisect status" output (and we do not have to worry about teaching "bisect status" subcommand about it in the first place). As long as we make sure that everybody uses the new "bisect_log_printf()" wrapper for informational messages, we get it for free. Thanks.
Chris Down <chris@chrisdown.name> writes: > +__attribute__((format (printf, 1, 2))) > +static void bisect_log_printf(const char *fmt, ...) > +{ > + va_list ap; > + char buf[1024]; > + > + va_start(ap, fmt); > + if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0) > + *buf = '\0'; > + va_end(ap); We should just do struct strbuf buf = STRBUF_INIT; va_start(ap, fmt); strbuf_vaddf(&buf, fmt, ap); va_end(ap); > + printf("%s", buf); > + append_to_file(git_path_bisect_log(), "# %s", buf); and free the resource with strbuf_release(&buf); I think. > +} > diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh > index a02587d1a7..d16730a2e2 100755 > --- a/t/t6030-bisect-porcelain.sh > +++ b/t/t6030-bisect-porcelain.sh > @@ -1029,18 +1029,23 @@ 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 log | grep "waiting for both good and bad commits" && Having "git" command on the left hand side of pipe would hide a failure signalled by its exit status from the command. The "but if the command fails, how likely would we see the expected output to its standard ouput?" argument aside, it is more common to write git bisect log >output && grep "..." && instead. Thanks.
On Fri, May 06, 2022 at 11:09:46AM +0100, Chris Down wrote: > > I'm not totally convinced it _needs_ to live in "git bisect log", > > though, since it feels like additional information that is just added > > for convenience. That's not the worst thing in the world, but I think > > it would be fine to just take the first patch (with my suggestions > > applied) as well. > > There was some discussion in the v1 thread (Message-ID: > <xmqqv8uo1mk6.fsf@gitster.g>) about adding an additional `git bisect status' > command, but while writing it my immediate thoughts were that it doesn't > make much sense to separate from the rest of the log. I'm curious what > Junio's thoughts are on that, happy to do it whichever way is preferred. :-) I don't have a strong feeling either way. The way that you have incorporated it into the output of "git bisect log" feels simple and sane to me. The most important thing is that you didn't break the ability to pipe the unmodified output of "git bisect log" into "git bisect replay", so I beyond that I don't have any strong feelings about it. > > > Signed-off-by: Chris Down <chris@chrisdown.name> > > > --- > > > builtin/bisect--helper.c | 25 ++++++++++++++++++++----- > > > t/t6030-bisect-porcelain.sh | 9 +++++++-- > > > 2 files changed, 27 insertions(+), 7 deletions(-) > > > > > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > > > index 9d583f651c..ef75f0a0ce 100644 > > > --- a/builtin/bisect--helper.c > > > +++ b/builtin/bisect--helper.c > > > @@ -404,6 +404,21 @@ static struct bisect_state bisect_status(const struct bisect_terms *terms) > > > return bs; > > > } > > > > > > +__attribute__((format (printf, 1, 2))) > > > +static void bisect_log_printf(const char *fmt, ...) > > > +{ > > > + va_list ap; > > > + char buf[1024]; > > > + > > > + va_start(ap, fmt); > > > + if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0) > > > + *buf = '\0'; > > > + va_end(ap); > > > + > > > + printf("%s", buf); > > > + append_to_file(git_path_bisect_log(), "# %s", buf); > > > +} > > > > This direct use of vsnprintf might be avoided by preparing the output in > > bisect_print_status() via a strbuf and then calling: > > > > append_to_file(git_path_bisect_log(), "# %s", buf.buf). > > I'm not intimately familiar with the codebase, so maybe I'm missing > something, but isn't it overkill to use a string buffer for something which > is isn't going to then be used as a mutable buffer? > > Happy to do it whichever way works for you folks, but would be good to > understand the rationale so that I can write better patches next time :-) I'm trying to avoid having a vsnprintf() when one isn't strictly needed. This part of bisect isn't performance critical code, so having a strbuf around doesn't bother me in the slightest. If you really wanted to, you could treat the strbuf like a static buffer by declaring it as such and calling strbuf_reset() before or after using it. But allocating little bits of memory in bisect_print_status() doesn't bother me much, either. At the very least, it would dramatically simplify the implementation of this patch with a negligible cost, so I'd recommend doing it. > > > static void bisect_print_status(const struct bisect_terms *terms) > > > { > > > const struct bisect_state bs = bisect_status(terms); > > > @@ -413,13 +428,13 @@ static void bisect_print_status(const struct bisect_terms *terms) > > > return; > > > > > > if (!bs.nr_good && !bs.nr_bad) > > > - printf(_("status: waiting for both good and bad commits\n")); > > > + bisect_log_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); > > > + bisect_log_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")); > > > + bisect_log_printf(_("status: waiting for good commit(s), bad commit known\n")); > > > } > > > > Interesting; this patch removes the output that we were giving to users > > in the last patch. Should it go to both places? > > Not sure I understand, we printf() in bisect_log_printf, no? I missed that, thanks for pointing it out. Thanks, Taylor
On Fri, May 06, 2022 at 11:18:25AM -0700, Junio C Hamano wrote: > Chris Down <chris@chrisdown.name> writes: > > > +__attribute__((format (printf, 1, 2))) > > +static void bisect_log_printf(const char *fmt, ...) > > +{ > > + va_list ap; > > + char buf[1024]; > > + > > + va_start(ap, fmt); > > + if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0) > > + *buf = '\0'; > > + va_end(ap); > > We should just do > > struct strbuf buf = STRBUF_INIT; > > va_start(ap, fmt); > strbuf_vaddf(&buf, fmt, ap); > va_end(ap); > > > + printf("%s", buf); > > + append_to_file(git_path_bisect_log(), "# %s", buf); > > and free the resource with > > strbuf_release(&buf); > > I think. I don't think so. bisect_log_printf() has only one caller, which is bisect_print_status(). Couldn't the latter manage its own strbuf without the need to introduce a new varargs function? Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > On Fri, May 06, 2022 at 11:18:25AM -0700, Junio C Hamano wrote: >> Chris Down <chris@chrisdown.name> writes: >> >> > +__attribute__((format (printf, 1, 2))) >> > +static void bisect_log_printf(const char *fmt, ...) >> > +{ >> > + va_list ap; >> > + char buf[1024]; >> > + >> > + va_start(ap, fmt); >> > + if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0) >> > + *buf = '\0'; >> > + va_end(ap); >> >> We should just do >> >> struct strbuf buf = STRBUF_INIT; >> >> va_start(ap, fmt); >> strbuf_vaddf(&buf, fmt, ap); >> va_end(ap); >> >> > + printf("%s", buf); >> > + append_to_file(git_path_bisect_log(), "# %s", buf); >> >> and free the resource with >> >> strbuf_release(&buf); >> >> I think. > > I don't think so. bisect_log_printf() has only one caller, which is > bisect_print_status(). Couldn't the latter manage its own strbuf without > the need to introduce a new varargs function? I actually was hoping that other existing informational messages prefixed with "Bisecting:" (i.e. those that tells you how many steps are remaining) can go as similar comments to the log file; they are currently written with plain-vanilla printf(3), and could use this one instead.
On Mon, May 09, 2022 at 09:08:46AM -0700, Junio C Hamano wrote: > > I don't think so. bisect_log_printf() has only one caller, which is > > bisect_print_status(). Couldn't the latter manage its own strbuf without > > the need to introduce a new varargs function? > > I actually was hoping that other existing informational messages > prefixed with "Bisecting:" (i.e. those that tells you how many steps > are remaining) can go as similar comments to the log file; they are > currently written with plain-vanilla printf(3), and could use this > one instead. I see. In that case I agree that we this function should take varargs, and it should pass that va_list to strbuf_vaddf(). But this patch shouldn't reimplement the wheel here with a bare vsnprintf() call that could be replaced with a strbuf. Thanks, Taylor
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 9d583f651c..ef75f0a0ce 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -404,6 +404,21 @@ static struct bisect_state bisect_status(const struct bisect_terms *terms) return bs; } +__attribute__((format (printf, 1, 2))) +static void bisect_log_printf(const char *fmt, ...) +{ + va_list ap; + char buf[1024]; + + va_start(ap, fmt); + if (vsnprintf(buf, sizeof(buf), fmt, ap) < 0) + *buf = '\0'; + va_end(ap); + + printf("%s", buf); + append_to_file(git_path_bisect_log(), "# %s", buf); +} + static void bisect_print_status(const struct bisect_terms *terms) { const struct bisect_state bs = bisect_status(terms); @@ -413,13 +428,13 @@ static void bisect_print_status(const struct bisect_terms *terms) return; if (!bs.nr_good && !bs.nr_bad) - printf(_("status: waiting for both good and bad commits\n")); + bisect_log_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); + bisect_log_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")); + bisect_log_printf(_("status: waiting for good commit(s), bad commit known\n")); } static int bisect_next_check(const struct bisect_terms *terms, diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index a02587d1a7..d16730a2e2 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -1029,18 +1029,23 @@ 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 log | grep "waiting for both good and bad commits" && git bisect good "$HASH1" >output && grep "waiting for bad commit, 1 good commit known" output && + git bisect log | grep "waiting for bad commit, 1 good commit known" && git bisect good "$HASH2" >output && - grep "waiting for bad commit, 2 good commits known" output + grep "waiting for bad commit, 2 good commits known" output && + git bisect log | grep "waiting for bad commit, 2 good commits known" ' 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 log | grep "waiting for both good and bad commits" && git bisect bad "$HASH4" >output && - grep -F "waiting for good commit(s), bad commit known" output + grep -F "waiting for good commit(s), bad commit known" output && + git bisect log | grep -F "waiting for good commit(s), bad commit known" ' test_done
This allows seeing the current intermediate status without adding a new good or bad commit: $ git bisect log | tail -1 # status: waiting for bad commit, 1 good commit known Signed-off-by: Chris Down <chris@chrisdown.name> --- builtin/bisect--helper.c | 25 ++++++++++++++++++++----- t/t6030-bisect-porcelain.sh | 9 +++++++-- 2 files changed, 27 insertions(+), 7 deletions(-)