| Message ID | 20210123154056.48234-2-mirucam@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Finish converting git bisect to C part 3 | expand |
Nice work on this series. I have one comment on this series regarding a behavior diff between the C and shell version, and small comment about style, see below. Miriam Rubio writes: > From: Pranit Bauva <pranit.bauva@gmail.com> > > Reimplement the `bisect_log()` shell function in C and also add > `--bisect-log` subcommand to `git bisect--helper` to call it from > git-bisect.sh . > > Using `--bisect-log` subcommand is a temporary measure to port shell > function to C so as to use the existing test suite. > > Mentored-by: Lars Schneider <larsxschneider@gmail.com> > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com> > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> > Signed-off-by: Miriam Rubio <mirucam@gmail.com> > --- > builtin/bisect--helper.c | 22 +++++++++++++++++++++- > git-bisect.sh | 7 +------ > 2 files changed, 22 insertions(+), 7 deletions(-) > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 709eb713a3..a65244a0f5 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -904,6 +904,18 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a > return bisect_auto_next(terms, NULL); > } > > +static enum bisect_error bisect_log(void) > +{ > + int fd, status; > + fd = open(git_path_bisect_log(), O_RDONLY); > + if (fd < 0) > + return BISECT_FAILED; > + > + status = copy_fd(fd, STDOUT_FILENO); > + close(fd); > + return status ? BISECT_FAILED : BISECT_OK; > +} > + In the shell version, when we are not bisecting it the `git bisect log` command will `die` with the text "We are not bisecting." which state to the user that a bisect is not yet started. The shell version does that by checking if the `$GIT_DIR/BISECT_LOG` file doesn't exists or it's an empty file as the following code snippet copied from the shell version that is remove by this patch: test -s "$GIT_DIR/BISECT_LOG" || die "$(gettext "We are not bisecting.")" This seems to be "missing" from the new C version implemented by this patch and perhaps we should add it. I'm not sure whether this change was intentional and I'm missing some context here of why we are dropping the message, if that's the case I apologize in advance. But, IMHO outputting the error message provides a better user experience as it would be obvious that the user forgot to `git bisect start` instead of silently failing. With that said, perhaps an obvious way of implementing is to use `is_empty_or_missing_file()`, much like `bisect_replay()` does it in the [2/7] patch on this series, and return the same error message from the shell version: -- >8 -- diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index a65244a0f5..ce11383125 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -907,7 +907,12 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a static enum bisect_error bisect_log(void) { int fd, status; - fd = open(git_path_bisect_log(), O_RDONLY); + const char* filename = git_path_bisect_log(); + + if (is_empty_or_missing_file(filename)) + return error(_("We are not bisecting.")); + + fd = open(filename, O_RDONLY); if (fd < 0) return BISECT_FAILED; -- >8 -- Although I compiled and did small test on the above code snippet, don't trust it blindly and perform your own test and judge whether this is the best way to implement this shortcoming. > > @@ -210,7 +205,7 @@ case "$#" in > replay) > bisect_replay "$@" ;; > log) > - bisect_log ;; > + git bisect--helper --bisect-log || exit;; Style: just a minor change to add a space between `exit` and `;;`.
Hi Rafael, El dom, 24 ene 2021 a las 14:56, Rafael Silva (<rafaeloliveira.cs@gmail.com>) escribió: > > > Nice work on this series. > > I have one comment on this series regarding a behavior diff between the > C and shell version, and small comment about style, see below. > > Miriam Rubio writes: > > > From: Pranit Bauva <pranit.bauva@gmail.com> > > > > Reimplement the `bisect_log()` shell function in C and also add > > `--bisect-log` subcommand to `git bisect--helper` to call it from > > git-bisect.sh . > > > > Using `--bisect-log` subcommand is a temporary measure to port shell > > function to C so as to use the existing test suite. > > > > Mentored-by: Lars Schneider <larsxschneider@gmail.com> > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > > Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > > Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com> > > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> > > Signed-off-by: Miriam Rubio <mirucam@gmail.com> > > --- > > builtin/bisect--helper.c | 22 +++++++++++++++++++++- > > git-bisect.sh | 7 +------ > > 2 files changed, 22 insertions(+), 7 deletions(-) > > > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > > index 709eb713a3..a65244a0f5 100644 > > --- a/builtin/bisect--helper.c > > +++ b/builtin/bisect--helper.c > > @@ -904,6 +904,18 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a > > return bisect_auto_next(terms, NULL); > > } > > > > +static enum bisect_error bisect_log(void) > > +{ > > + int fd, status; > > + fd = open(git_path_bisect_log(), O_RDONLY); > > + if (fd < 0) > > + return BISECT_FAILED; > > + > > + status = copy_fd(fd, STDOUT_FILENO); > > + close(fd); > > + return status ? BISECT_FAILED : BISECT_OK; > > +} > > + > > In the shell version, when we are not bisecting it the `git bisect log` > command will `die` with the text "We are not bisecting." which state to > the user that a bisect is not yet started. The shell version does that > by checking if the `$GIT_DIR/BISECT_LOG` file doesn't exists or it's > an empty file as the following code snippet copied from the shell > version that is remove by this patch: > > test -s "$GIT_DIR/BISECT_LOG" || die "$(gettext "We are not bisecting.")" > > This seems to be "missing" from the new C version implemented by this > patch and perhaps we should add it. I'm not sure whether this change was > intentional and I'm missing some context here of why we are dropping > the message, if that's the case I apologize in advance. But, IMHO > outputting the error message provides a better user experience as it > would be obvious that the user forgot to `git bisect start` instead of > silently failing. > > With that said, perhaps an obvious way of implementing is to use > `is_empty_or_missing_file()`, much like `bisect_replay()` does it in the > [2/7] patch on this series, and return the same error message from > the shell version: > > -- >8 -- > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index a65244a0f5..ce11383125 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -907,7 +907,12 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a > static enum bisect_error bisect_log(void) > { > int fd, status; > - fd = open(git_path_bisect_log(), O_RDONLY); > + const char* filename = git_path_bisect_log(); > + > + if (is_empty_or_missing_file(filename)) > + return error(_("We are not bisecting.")); > + > + fd = open(filename, O_RDONLY); > if (fd < 0) > return BISECT_FAILED; > -- >8 -- > > Although I compiled and did small test on the above code snippet, don't > trust it blindly and perform your own test and judge whether this is the > best way to implement this shortcoming. Ok, thank you. I am not the original author of this subcommand reimplementation and I don't know if there is a reason for the difference with the error message. Maybe we can wait for some other reviewers opinion. > > > > > @@ -210,7 +205,7 @@ case "$#" in > > replay) > > bisect_replay "$@" ;; > > log) > > - bisect_log ;; > > + git bisect--helper --bisect-log || exit;; > > Style: just a minor change to add a space between `exit` and `;;`. Noted. > > -- > Thanks > Rafael Thank you for your suggestions. Best, Miriam
"Miriam R." <mirucam@gmail.com> writes: >> Although I compiled and did small test on the above code snippet, don't >> trust it blindly and perform your own test and judge whether this is the >> best way to implement this shortcoming. > Ok, thank you. > I am not the original author of this subcommand reimplementation > and I don't know if there is a reason for the difference with the > error message. Maybe we can wait for some other reviewers opinion. Sorry I missed this thread. My understanding is that this topic is an attempt to "reimplement" what is there in the scripted version, so any deviation of behaviour obserbable from outside, which is *not* justified, should by definition be treated as a bug. If the original author did not explain why the behaviour difference exists and defend why the new behaviour in the reimplementation is better, and if you do not think of a good reason why the behaviour should be different and the new behaviour is better, then let's treat it in a bug and fix it. Thanks.
Hi, El mar, 26 ene 2021 a las 19:32, Junio C Hamano (<gitster@pobox.com>) escribió: > > "Miriam R." <mirucam@gmail.com> writes: > > >> Although I compiled and did small test on the above code snippet, don't > >> trust it blindly and perform your own test and judge whether this is the > >> best way to implement this shortcoming. > > Ok, thank you. > > I am not the original author of this subcommand reimplementation > > and I don't know if there is a reason for the difference with the > > error message. Maybe we can wait for some other reviewers opinion. > > Sorry I missed this thread. > > My understanding is that this topic is an attempt to "reimplement" > what is there in the scripted version, so any deviation of behaviour > obserbable from outside, which is *not* justified, should by > definition be treated as a bug. > > If the original author did not explain why the behaviour difference > exists and defend why the new behaviour in the reimplementation is > better, and if you do not think of a good reason why the behaviour > should be different and the new behaviour is better, then let's > treat it in a bug and fix it. > Ok, I will send another patch series adding this. Thank you. Miriam. > Thanks.
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 709eb713a3..a65244a0f5 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -904,6 +904,18 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a return bisect_auto_next(terms, NULL); } +static enum bisect_error bisect_log(void) +{ + int fd, status; + fd = open(git_path_bisect_log(), O_RDONLY); + if (fd < 0) + return BISECT_FAILED; + + status = copy_fd(fd, STDOUT_FILENO); + close(fd); + return status ? BISECT_FAILED : BISECT_OK; +} + int cmd_bisect__helper(int argc, const char **argv, const char *prefix) { enum { @@ -916,7 +928,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) BISECT_AUTOSTART, BISECT_NEXT, BISECT_AUTO_NEXT, - BISECT_STATE + BISECT_STATE, + BISECT_LOG } cmdmode = 0; int res = 0, nolog = 0; struct option options[] = { @@ -938,6 +951,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT), OPT_CMDMODE(0, "bisect-state", &cmdmode, N_("mark the state of ref (or refs)"), BISECT_STATE), + OPT_CMDMODE(0, "bisect-log", &cmdmode, + N_("list the bisection steps so far"), BISECT_LOG), OPT_BOOL(0, "no-log", &nolog, N_("no log for BISECT_WRITE")), OPT_END() @@ -1000,6 +1015,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) get_terms(&terms); res = bisect_state(&terms, argv, argc); break; + case BISECT_LOG: + if (argc) + return error(_("--bisect-log requires 0 arguments")); + res = bisect_log(); + break; default: BUG("unknown subcommand %d", cmdmode); } diff --git a/git-bisect.sh b/git-bisect.sh index 1f3f6e9fc5..c6149846ff 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -169,11 +169,6 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2 done } -bisect_log () { - test -s "$GIT_DIR/BISECT_LOG" || die "$(gettext "We are not bisecting.")" - cat "$GIT_DIR/BISECT_LOG" -} - get_terms () { if test -s "$GIT_DIR/BISECT_TERMS" then @@ -210,7 +205,7 @@ case "$#" in replay) bisect_replay "$@" ;; log) - bisect_log ;; + git bisect--helper --bisect-log || exit;; run) bisect_run "$@" ;; terms)