Hey Ramsay,
On Sat, Sep 30, 2017 at 6:29 PM, Ramsay Jones
<ram...@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
