Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva <pranit.bauva@gmail.com>]

Hey Ramsay,

On Sat, Sep 30, 2017 at 6:29 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:

> Hi Pranit,
>
> Just before Junio dropped your 'pb/bisect' branch from his
> repository (and What's cooking), I fetched it locally with
> the intention of finishing it off. (It would have been silly
> to waste all your good work).

Thanks!

> Although I have rebased your branch a few times, and added
> a few commits while 'reading' the code, I haven't actually
> added much to your branch (only 12 commits and I had meant
> to squash some of those together)!
>
> However, there were some bug fixes in there, so you may want
> to take a look at:
>
>     git://repo.or.cz/git/raj.git branch 'bisect'
>
> [the 'pb-bisect' branch was the original branch from Junio,
> including the 'SQUASH' commit that I squashed!]

Yes, I have checked it out. I had worked on Stephan's review and
updated a few parts. I think you have included that as well as some of
your modifications. I will squash it in together and send the series
out in parts.

> These patches seem to relate to patches 1-5 & 8 of the original
> series. The diff between these patches and the first 6 patches
> of my bisect branch is given below. Note that most of the diff
> seems to be caused by swapping patch #6 for #8, but not all of
> the hunks are caused by this.
>
> Note that I moved some code between patches (e.g. some of the
> GIT_PATH_FUNC()s moved out of patch #4, because they were not
> used in that patch. Ah, is that why you moved patch #8 up?).
> [Also I added the 'bisect clean' message to delet_refs() to
> patch #4 as well.]

Even I thought keeping GIT_PATH_FUNC()s should be declared whenever
required. Did that change in this patch series.

> Look for []-ed comments in the commit messages for a note of
> the changes I made to your original patches, in patches #2,
> #4, #7-9, #11-12 and #14.
>
> The commits I added, which are just WIP, are as follows:
>
>   $ git log --oneline bisect~12..bisect
>   7d7117040 (raj/bisect, bisect) bisect--helper: convert to struct object_id
>   188ea5855 bisect--helper: add the get_bad_commit() function
>   b75f46fb4 bisect--helper: add a log_commit() helper function
>   4afc34403 bisect--helper: reduce the scope of a variable
>   62495f6ae bisect--helper: remove useless sub-expression in condition
>   964f4e2b0 bisect--helper: set correct term from --term-new= option
>   62efc099f bisect--helper: remove redundant assignment to has_double_dash
>   d35950b92 bisect--helper: remove redundant goto's
>   b33f313ac bisect--helper: remove space just before \n in string
>   3eb407156 bisect--helper: remove some unnecessary braces
>   c2b89c9b8 bisect--helper: add some vertical whitespace
>   8c883701c bisect--helper: fix up some coding style issues
>   $
>
> Again IIRC, there are a couple of bug fixes in these commits ...

There is actually a major bug in the later part of previous series
mostly in the bisect-next which actually caused delays. I think you
have fixed it in your commit 682d0bff0. Although I would need to have
a closer look at it. In original series, I did get a sigserv, and as
you mention it in the commit that you have fixed it.

> I have to go now, so I will leave it with you. ;-)
>
> Hope that helps.
>
> ATB,
> Ramsay Jones

Again Thanks!

>
> -- >8 --
> diff --git a/bisect.c b/bisect.c
> index 2838d672d..b19311ca7 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -1066,7 +1066,7 @@ int bisect_clean_state(void)
>         struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
>         for_each_ref_in("refs/bisect", mark_for_removal, (void *) &refs_for_removal);
>         string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
> -       result = delete_refs("bisect: remove", &refs_for_removal, REF_NODEREF);
> +       result = delete_refs("bisect: clean", &refs_for_removal, REF_NODEREF);
>         refs_for_removal.strdup_strings = 1;
>         string_list_clear(&refs_for_removal, 0);
>         unlink_or_warn(git_path_bisect_expected_rev());
> diff --git a/builtin/am.c b/builtin/am.c
> index c973bd96d..aa66f9915 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -32,22 +32,6 @@
>  #include "apply.h"
>  #include "string-list.h"
>
> -/**
> - * Returns 1 if the file is empty or does not exist, 0 otherwise.
> - */
> -static int is_empty_file(const char *filename)
> -{
> -       struct stat st;
> -
> -       if (stat(filename, &st) < 0) {
> -               if (errno == ENOENT)
> -                       return 1;
> -               die_errno(_("could not stat %s"), filename);
> -       }
> -
> -       return !st.st_size;
> -}
> -
>  /**
>   * Returns the length of the first line of msg.
>   */
> @@ -1300,7 +1284,7 @@ static int parse_mail(struct am_state *state, const char *mail)
>                 goto finish;
>         }
>
> -       if (is_empty_file(am_path(state, "patch"))) {
> +       if (is_empty_or_missing_file(am_path(state, "patch"))) {
>                 printf_ln(_("Patch is empty."));
>                 die_user_resolve(state);
>         }
> @@ -1883,7 +1867,7 @@ static void am_run(struct am_state *state, int resume)
>                 resume = 0;
>         }
>
> -       if (!is_empty_file(am_path(state, "rewritten"))) {
> +       if (!is_empty_or_missing_file(am_path(state, "rewritten"))) {
>                 assert(state->rebasing);
>                 copy_notes_for_rebase(state);
>                 run_post_rewrite_hook(state);
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 35d2105f9..2af024f60 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -5,8 +5,6 @@
>  #include "refs.h"
>
>  static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
> -static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
> -static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
>
>  static const char * const git_bisect_helper_usage[] = {
>         N_("git bisect--helper --next-all [--no-checkout]"),
> @@ -45,8 +43,8 @@ static int check_term_format(const char *term, const char *orig_term)
>         if (res)
>                 return error(_("'%s' is not a valid term"), term);
>
> -       if (one_of(term, "help", "start", "skip", "next", "reset",
> -                       "visualize", "replay", "log", "run", "terms", NULL))
> +       if (one_of(term, "help", "start", "terms", "skip", "next", "reset",
> +                       "visualize", "replay", "log", "run", NULL))
>                 return error(_("can't use the builtin command '%s' as a term"), term);
>
>         /*
> @@ -82,37 +80,12 @@ static int write_terms(const char *bad, const char *good)
>         return (res < 0) ? -1 : 0;
>  }
>
> -static int is_expected_rev(const char *expected_hex)
> -{
> -       struct strbuf actual_hex = STRBUF_INIT;
> -       int res = 0;
> -       if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 40) {
> -               strbuf_trim(&actual_hex);
> -               res = !strcmp(actual_hex.buf, expected_hex);
> -       }
> -       strbuf_release(&actual_hex);
> -       return res;
> -}
> -
> -static void check_expected_revs(const char **revs, int rev_nr)
> -{
> -       int i;
> -
> -       for (i = 0; i < rev_nr; i++) {
> -               if (!is_expected_rev(revs[i])) {
> -                       unlink_or_warn(git_path_bisect_ancestors_ok());
> -                       unlink_or_warn(git_path_bisect_expected_rev());
> -               }
> -       }
> -}
> -
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>         enum {
>                 NEXT_ALL = 1,
>                 WRITE_TERMS,
> -               BISECT_CLEAN_STATE,
> -               CHECK_EXPECTED_REVS
> +               BISECT_CLEAN_STATE
>         } cmdmode = 0;
>         int no_checkout = 0;
>         struct option options[] = {
> @@ -122,8 +95,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>                          N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
>                 OPT_CMDMODE(0, "bisect-clean-state", &cmdmode,
>                          N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
> -               OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
> -                        N_("check for expected revs"), CHECK_EXPECTED_REVS),
>                 OPT_BOOL(0, "no-checkout", &no_checkout,
>                          N_("update BISECT_HEAD instead of checking out the current commit")),
>                 OPT_END()
> @@ -140,17 +111,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>                 return bisect_next_all(prefix, no_checkout);
>         case WRITE_TERMS:
>                 if (argc != 2)
> -                       return error(_("--write-terms requires two arguments"));
> +                       die(_("--write-terms requires two arguments"));
>                 return write_terms(argv[0], argv[1]);
>         case BISECT_CLEAN_STATE:
>                 if (argc != 0)
> -                       return error(_("--bisect-clean-state requires no arguments"));
> +                       die(_("--bisect-clean-state requires no arguments"));
>                 return bisect_clean_state();
> -       case CHECK_EXPECTED_REVS:
> -               check_expected_revs(argv, argc);
> -               return 0;
>         default:
> -               return error("BUG: unknown subcommand '%d'", cmdmode);
> +               die("BUG: unknown subcommand '%d'", cmdmode);

I will keep the return rather than die since Christian and I had a few
conversations long back.

Regards,
Pranit Bauva
www.bauva.com