Re: [PATCH v14 13/27] bisect--helper: `bisect_start` shell function partially in C
Pranit Bauva writes: >> I wonder the whole thing above is better restructured to avoid >> repeated checks of the same thing. >> >> if (is it 40-hex, i.e. detached?) { >> stuff it to start_head; >> } else if (skip_prefix(head, "refs/heads/", &branchname)) { >> do the "cogito" check; >> stuff it to start_head; >> } else { >> that's a strange symbolic ref HEAD you have there; >> } > > I guess it changes the behaviour. Its a strange symbolic ref if it > does not start with "refs/heads". I did not think my suggestion would change the behaviour at all. It would change the code structure a bit to make it more readable, though. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v14 13/27] bisect--helper: `bisect_start` shell function partially in C
Hey Junio, On Fri, Aug 26, 2016 at 12:32 AM, Junio C Hamano wrote: > Pranit Bauva writes: > >> +static int bisect_start(struct bisect_terms *terms, int no_checkout, >> + const char **argv, int argc) >> +{ >> + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0; >> + int flags, pathspec_pos; >> + struct string_list revs = STRING_LIST_INIT_DUP; >> + struct string_list states = STRING_LIST_INIT_DUP; > > The original has a single state, not states. Let's see if there is > a reason behind the name change > >> + unsigned char sha1[20]; >> + struct object_id oid; > > More on these below... > >> + ... >> + for (i = 0; i < argc; i++) { >> + const char *commit_id = xstrfmt("%s^{commit}", argv[i]); >> + if (!strcmp(argv[i], "--")) { >> + has_double_dash = 1; >> + break; >> + } else if (!strcmp(argv[i], "--no-checkout")) >> + no_checkout = 1; >> + else if (!strcmp(argv[i], "--term-good") || >> + ... >> + } else if (starts_with(argv[i], "--") && >> + !one_of(argv[i], "--term-good", "--term-bad", NULL)) { >> + string_list_clear(&revs, 0); >> + string_list_clear(&states, 0); >> + die(_("unrecognised option: '%s'"), argv[i]); >> + } else if (get_oid(commit_id, &oid) && has_double_dash) { >> + string_list_clear(&revs, 0); >> + string_list_clear(&states, 0); >> + die(_("'%s' does not appear to be a valid revision"), >> argv[i]); >> + } else { >> + string_list_append(&revs, oid_to_hex(&oid)); >> + } >> + } > > What I do not understand is clearing the string list "states" inside > this loop. It may have been INIT_DUPed, but nothing in this loop > adds anything to it. Because "revs" does get extended in the loop, > it is understandable if you wanted to clear it before dying, but "if > you are dying anyway why bother clearing?" is also a valid stance to > take. I think I should probably use return here instead of die(). > The same "perhaps want to use the 'retval' with goto 'finish:' pattern?" > comment applies here, too. Okay sure could do that. >> + pathspec_pos = i; >> + >> + /* >> + * The user ran "git bisect start ", hence did not >> + * explicitly specify the terms, but we are already starting to >> + * set references named with the default terms, and won't be able >> + * to change afterwards. >> + */ >> + must_write_terms |= !!revs.nr; >> + for (i = 0; i < revs.nr; i++) { >> + if (bad_seen) >> + string_list_append(&states, terms->term_good.buf); >> + else { >> + bad_seen = 1; >> + string_list_append(&states, terms->term_bad.buf); >> + } >> + } > > This is certainly different from the original. We used to do one > "bisect_write" per element in revs in the loop; we no longer do that > and instead collect them in states list. Each element in these two > lists, i.e. revs.item[i] and states.item[i], corresponds to each > other. > > That explains why the variable is renamed to states. We haven't > seen enough to say if this behaviour change is a good idea or not. > >> + /* >> + * Verify HEAD >> + */ >> + head = resolve_ref_unsafe("HEAD", 0, sha1, &flags); >> + if (!head) { >> + if (get_sha1("HEAD", sha1)) { >> + string_list_clear(&revs, 0); >> + string_list_clear(&states, 0); >> + die(_("Bad HEAD - I need a HEAD")); >> + } >> + } >> + if (!is_empty_or_missing_file(git_path_bisect_start())) { >> + /* Reset to the rev from where we started */ >> + strbuf_read_file(&start_head, git_path_bisect_start(), 0); >> + strbuf_trim(&start_head); >> + if (!no_checkout) { >> + struct argv_array argv = ARGV_ARRAY_INIT; >> + argv_array_pushl(&argv, "checkout", start_head.buf, >> + "--", NULL); >> + if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) { >> + error(_("checking out '%s' failed. Try 'git " >> + "bisect start '."), >> + start_head.buf); >> + strbuf_release(&start_head); >> + string_list_clear(&revs, 0); >> + string_list_clear(&states, 0); >> + return -1; > > The original died here, but you expect the caller to respond to a > negative return. I haven't read enough to judge if that is a good > idea, but doesn't it make sense to do the same through
Re: [PATCH v14 13/27] bisect--helper: `bisect_start` shell function partially in C
Junio C Hamano writes: >> +for (i = 0; i < revs.nr; i++) { >> +if (bad_seen) >> +string_list_append(&states, terms->term_good.buf); >> +else { >> +bad_seen = 1; >> +string_list_append(&states, terms->term_bad.buf); >> +} >> +} > > This is certainly different from the original. We used to do one > "bisect_write" per element in revs in the loop; we no longer do that > and instead collect them in states list. Each element in these two > lists, i.e. revs.item[i] and states.item[i], corresponds to each > other. > > That explains why the variable is renamed to states. We haven't > seen enough to say if this behaviour change is a good idea or not. Ahh, I misread the original. It accumulates the writes to be executed in $eval and does so at the end. So there is no change in behaviour. So please ignore that point in the previous message. That leaves only the following points: * Perhaps 'retval' with 'goto finish' pattern? * ref_exists()? Perhaps use skip_prefix(head, "refs/heads/, &branch)? * if (clean-state) { return -1 }? * Is comment on trap relevant here? Sorry, and thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v14 13/27] bisect--helper: `bisect_start` shell function partially in C
Pranit Bauva writes: > +static int bisect_start(struct bisect_terms *terms, int no_checkout, > + const char **argv, int argc) > +{ > + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0; > + int flags, pathspec_pos; > + struct string_list revs = STRING_LIST_INIT_DUP; > + struct string_list states = STRING_LIST_INIT_DUP; The original has a single state, not states. Let's see if there is a reason behind the name change > + unsigned char sha1[20]; > + struct object_id oid; More on these below... > + ... > + for (i = 0; i < argc; i++) { > + const char *commit_id = xstrfmt("%s^{commit}", argv[i]); > + if (!strcmp(argv[i], "--")) { > + has_double_dash = 1; > + break; > + } else if (!strcmp(argv[i], "--no-checkout")) > + no_checkout = 1; > + else if (!strcmp(argv[i], "--term-good") || > + ... > + } else if (starts_with(argv[i], "--") && > + !one_of(argv[i], "--term-good", "--term-bad", NULL)) { > + string_list_clear(&revs, 0); > + string_list_clear(&states, 0); > + die(_("unrecognised option: '%s'"), argv[i]); > + } else if (get_oid(commit_id, &oid) && has_double_dash) { > + string_list_clear(&revs, 0); > + string_list_clear(&states, 0); > + die(_("'%s' does not appear to be a valid revision"), > argv[i]); > + } else { > + string_list_append(&revs, oid_to_hex(&oid)); > + } > + } What I do not understand is clearing the string list "states" inside this loop. It may have been INIT_DUPed, but nothing in this loop adds anything to it. Because "revs" does get extended in the loop, it is understandable if you wanted to clear it before dying, but "if you are dying anyway why bother clearing?" is also a valid stance to take. The same "perhaps want to use the 'retval' with goto 'finish:' pattern?" comment applies here, too. > + pathspec_pos = i; > + > + /* > + * The user ran "git bisect start ", hence did not > + * explicitly specify the terms, but we are already starting to > + * set references named with the default terms, and won't be able > + * to change afterwards. > + */ > + must_write_terms |= !!revs.nr; > + for (i = 0; i < revs.nr; i++) { > + if (bad_seen) > + string_list_append(&states, terms->term_good.buf); > + else { > + bad_seen = 1; > + string_list_append(&states, terms->term_bad.buf); > + } > + } This is certainly different from the original. We used to do one "bisect_write" per element in revs in the loop; we no longer do that and instead collect them in states list. Each element in these two lists, i.e. revs.item[i] and states.item[i], corresponds to each other. That explains why the variable is renamed to states. We haven't seen enough to say if this behaviour change is a good idea or not. > + /* > + * Verify HEAD > + */ > + head = resolve_ref_unsafe("HEAD", 0, sha1, &flags); > + if (!head) { > + if (get_sha1("HEAD", sha1)) { > + string_list_clear(&revs, 0); > + string_list_clear(&states, 0); > + die(_("Bad HEAD - I need a HEAD")); > + } > + } > + if (!is_empty_or_missing_file(git_path_bisect_start())) { > + /* Reset to the rev from where we started */ > + strbuf_read_file(&start_head, git_path_bisect_start(), 0); > + strbuf_trim(&start_head); > + if (!no_checkout) { > + struct argv_array argv = ARGV_ARRAY_INIT; > + argv_array_pushl(&argv, "checkout", start_head.buf, > + "--", NULL); > + if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) { > + error(_("checking out '%s' failed. Try 'git " > + "bisect start '."), > + start_head.buf); > + strbuf_release(&start_head); > + string_list_clear(&revs, 0); > + string_list_clear(&states, 0); > + return -1; The original died here, but you expect the caller to respond to a negative return. I haven't read enough to judge if that is a good idea, but doesn't it make sense to do the same throughout the function consistently? I saw a few die()'s already in the command line parsing loop--shouldn't they also return -1? The original has called bisect_write already when we attempt this checkout and the user will see the states in the filesystem. This rewrite does not. What effe
[PATCH v14 13/27] bisect--helper: `bisect_start` shell function partially in C
Reimplement the `bisect_start` shell function partially in C and add `bisect-start` subcommand to `git bisect--helper` to call it from git-bisect.sh . The last part is not converted because it calls another shell function `bisect_start` shell function will be completed after the `bisect_next` shell function is ported in C. Using `--bisect-start` subcommand is a temporary measure to port shell function in C so as to use the existing test suite. As more functions are ported, this subcommand will be retired and will be called by some other methods. Mentored-by: Lars Schneider Mentored-by: Christian Couder Signed-off-by: Pranit Bauva --- builtin/bisect--helper.c | 238 ++- git-bisect.sh| 133 +- 2 files changed, 238 insertions(+), 133 deletions(-) diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 44adf6b..c64996a 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -7,6 +7,7 @@ #include "argv-array.h" #include "run-command.h" #include "prompt.h" +#include "quote.h" static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS") static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV") @@ -14,6 +15,8 @@ static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK") static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START") static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD") +static GIT_PATH_FUNC(git_path_head_name, "head-name") +static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES") static const char * const git_bisect_helper_usage[] = { N_("git bisect--helper --next-all [--no-checkout]"), @@ -24,6 +27,8 @@ static const char * const git_bisect_helper_usage[] = { N_("git bisect--helper --bisect-check-and-set-terms "), N_("git bisect--helper --bisect-next-check [] --term-{new,bad}=]" + "[--no-checkout] [ [...]] [--] [...]"), NULL }; @@ -391,6 +396,226 @@ static int bisect_terms(struct bisect_terms *terms, const char **argv, int argc) return 0; } +static int bisect_start(struct bisect_terms *terms, int no_checkout, + const char **argv, int argc) +{ + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0; + int flags, pathspec_pos; + struct string_list revs = STRING_LIST_INIT_DUP; + struct string_list states = STRING_LIST_INIT_DUP; + struct strbuf start_head = STRBUF_INIT; + struct strbuf bisect_names = STRBUF_INIT; + struct strbuf orig_args = STRBUF_INIT; + const char *head; + unsigned char sha1[20]; + FILE *fp; + struct object_id oid; + + if (is_bare_repository()) + no_checkout = 1; + + for (i = 0; i < argc; i++) { + if (!strcmp(argv[i], "--")) { + has_double_dash = 1; + break; + } + } + + for (i = 0; i < argc; i++) { + const char *commit_id = xstrfmt("%s^{commit}", argv[i]); + if (!strcmp(argv[i], "--")) { + has_double_dash = 1; + break; + } else if (!strcmp(argv[i], "--no-checkout")) + no_checkout = 1; + else if (!strcmp(argv[i], "--term-good") || +!strcmp(argv[i], "--term-old")) { + must_write_terms = 1; + strbuf_reset(&terms->term_good); + strbuf_addstr(&terms->term_good, argv[++i]); + } else if (skip_prefix(argv[i], "--term-good=", &argv[i])) { + must_write_terms = 1; + strbuf_reset(&terms->term_good); + strbuf_addstr(&terms->term_good, argv[i]); + } else if (skip_prefix(argv[i], "--term-old=", &argv[i])) { + must_write_terms = 1; + strbuf_reset(&terms->term_good); + strbuf_addstr(&terms->term_good, argv[i]); + } else if (!strcmp(argv[i], "--term-bad") || +!strcmp(argv[i], "--term-new")) { + must_write_terms = 1; + strbuf_reset(&terms->term_bad); + strbuf_addstr(&terms->term_bad, argv[++i]); + } else if (skip_prefix(argv[i], "--term-bad=", &argv[i])) { + must_write_terms = 1; + strbuf_reset(&terms->term_bad); + strbuf_addstr(&terms->term_bad, argv[i]); + } else if (skip_prefix(argv[i], "--term-new=", &argv[i])) { + must_write_terms = 1; + strbuf_reset(&terms->term_good); + strbuf_addstr(&terms->term_good, argv[i]); + } else if (starts_with(ar