Re: [PATCH v4 4/6] bisect--helper: reimplement `bisect_visualize()`shell function in C

Hi Johannes,

El mar, 17 ago 2021 a las 13:30, Johannes Schindelin
([off-list ref]) escribió:

Hi Miriam,

this looks good!

Just one suggestion (but I won't insist on it):

On Tue, 17 Aug 2021, Miriam Rubio wrote:

quoted

@@ -1036,6 +1037,44 @@ static enum bisect_error bisect_skip(struct bisect_terms *terms, const char **ar
      return res;
 }

+static int bisect_visualize(struct bisect_terms *terms, const char **argv, int argc)
+{
+     struct strvec args = STRVEC_INIT;
+     int flags = RUN_COMMAND_NO_STDIN, res = 0;
+     struct strbuf sb = STRBUF_INIT;
+
+     if (bisect_next_check(terms, NULL) != 0)
+             return BISECT_FAILED;
+
+     if (!argc) {
+             if ((getenv("DISPLAY") || getenv("SESSIONNAME") || getenv("MSYSTEM") ||
+                  getenv("SECURITYSESSIONID")) && exists_in_PATH("gitk"))
+                     strvec_push(&args, "gitk");
+             else {
+                     strvec_pushl(&args, "log", NULL);

This could be written more concisely as `strvec_push(&args, "log")`.

quoted

+                     flags |= RUN_GIT_CMD;
+             }
+     } else {
+             if (argv[0][0] == '-') {
+                     strvec_pushl(&args, "log", NULL);

Same here.

Sure, I will change it in both cases.
Thank you for reviewing,
Miriam.

Otherwise, it looks good to me!

Thank you,
Dscho

quoted

+                     flags |= RUN_GIT_CMD;
+             } else if (strcmp(argv[0], "tig") && !starts_with(argv[0], "git"))
+                     flags |= RUN_GIT_CMD;
+
+             strvec_pushv(&args, argv);
+     }
+
+     strvec_pushl(&args, "--bisect", "--", NULL);
+
+     strbuf_read_file(&sb, git_path_bisect_names(), 0);
+     sq_dequote_to_strvec(sb.buf, &args);
+     strbuf_release(&sb);
+
+     res = run_command_v_opt(args.v, flags);
+     strvec_clear(&args);
+     return res;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
      enum {

@@ -1048,7 +1087,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
              BISECT_STATE,
              BISECT_LOG,
              BISECT_REPLAY,
-             BISECT_SKIP
+             BISECT_SKIP,
+             BISECT_VISUALIZE,
      } cmdmode = 0;
      int res = 0, nolog = 0;
      struct option options[] = {

@@ -1070,6 +1110,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
                       N_("replay the bisection process from the given file"), BISECT_REPLAY),
              OPT_CMDMODE(0, "bisect-skip", &cmdmode,
                       N_("skip some commits for checkout"), BISECT_SKIP),
+             OPT_CMDMODE(0, "bisect-visualize", &cmdmode,
+                      N_("visualize the bisection"), BISECT_VISUALIZE),
              OPT_BOOL(0, "no-log", &nolog,
                       N_("no log for BISECT_WRITE")),
              OPT_END()

@@ -1131,6 +1173,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
              get_terms(&terms);
              res = bisect_skip(&terms, argv, argc);
              break;
+     case BISECT_VISUALIZE:
+             get_terms(&terms);
+             res = bisect_visualize(&terms, argv, argc);
+             break;
      default:
              BUG("unknown subcommand %d", cmdmode);
      }

diff --git a/git-bisect.sh b/git-bisect.sh
index 6a7afaea8d..95f7f3fb8c 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh

@@ -39,29 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
 TERM_BAD=bad
 TERM_GOOD=good

-bisect_visualize() {
-     git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
-
-     if test $# = 0
-     then
-             if test -n "${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}" &&
-                     type gitk >/dev/null 2>&1
-             then
-                     set gitk
-             else
-                     set git log
-             fi
-     else
-             case "$1" in
-             git*|tig) ;;
-             -*)     set git log "$@" ;;
-             *)      set git "$@" ;;
-             esac
-     fi
-
-     eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")
-}
-
 bisect_run () {
      git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit

@@ -152,7 +129,7 @@ case "$#" in
              # Not sure we want "next" at the UI level anymore.
              git bisect--helper --bisect-next "$@" || exit ;;
      visualize|view)
-             bisect_visualize "$@" ;;
+             git bisect--helper --bisect-visualize "$@" || exit;;
      reset)
              git bisect--helper --bisect-reset "$@" ;;
      replay)
--

2.29.2