Re: [RFC/PATCH v11 13/13] bisect--helper: `bisect_start` shell function partially in C
Pranit Bauva <pranit.bauva@gmail.com> writes:
Show 17 quoted lines
> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
> + const char **argv, int argc)
> +{
> + int i, j, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
> + int flag;
> + struct string_list revs = STRING_LIST_INIT_DUP;
> + struct string_list states = STRING_LIST_INIT_DUP;
> + struct strbuf start_head = 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++) {Show 16 quoted lines
> + if (!strcmp(argv[i], "--")) {
> + has_double_dash = 1;
> + break;
> + }
> + if (!strcmp(argv[i], "--term-good")) {
> + must_write_terms = 1;
> + strbuf_reset(&terms->term_good);
> + strbuf_addstr(&terms->term_good, argv[++i]);
> + break;
> + }
> + if (!strcmp(argv[i], "--term-bad")) {
> + must_write_terms = 1;
> + strbuf_reset(&terms->term_bad);
> + strbuf_addstr(&terms->term_bad, argv[++i]);
> + break;
> + }The original was not careful, either, but what if the user ends the command line with "--term-good", without anything after it?
Also the original is prepared to handle --term-good=boa; because this function can be be called directly from the UI (i.e. "git bisect start --term-good=boa"), not supporting that form would be seen as a regression.
Show 7 quoted lines
> + 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]);
> + }
> + if (get_oid(argv[i], &oid) || has_double_dash) {Calling get_oid() alone is insufficient to make sure argv[i] refers to an existing object that is a committish. The "^{commit}" suffix in the original is there for a reason.
> + string_list_clear(&revs, 0);
> + string_list_clear(&revs, 0);
You seem to want the revs list really really clean ;-)
Show 7 quoted lines
> + die(_("'%s' does not appear to be a valid revision"), argv[i]);
> + }
> + else
> + string_list_append(&revs, oid_to_hex(&oid));
> + }
> +
> + for (j = 0; j < revs.nr; j++) {Why "j", not "i", as clearly the previous loop has finished at this point? The only reason why replacing "j" with "i" would make this function buggy would be if a later part of this function depended on the value of "i" when the control left the above loop, but if that were the case (I didn't check carefully), such a precious value that has long term effect throughout the remainder of the function must not be kept in an otherwise throw-away loop counter variable "i".
Introduce a new "int pathspec_pos" and set it to "i" immediately after the "for (i = 0; i < argc; i++) { ... }" loop above, perhaps.
Show 18 quoted lines
> + struct strbuf state = STRBUF_INIT;
> + /*
> + * The user ran "git bisect start <sha1> <sha1>", 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 = 1;
> +
> + if (bad_seen)
> + strbuf_addstr(&state, terms->term_good.buf);
> + else {
> + bad_seen = 1;
> + strbuf_addstr(&state, terms->term_bad.buf);
> + }
> + string_list_append(&states, state.buf);
> + strbuf_release(&state);
> + }How about this instead?
/*
* that comment block goes here
*/
must_write_terms = !!revs.nr;
for (i = 0; i < revs.nr; i++) {
if (bad_seen)
string_list_append(&states, terms->term_good.buf);
else
string_list_append(&states, terms->term_bad.buf);
}Show 5 quoted lines
> +
> + /*
> + * Verify HEAD
> + */
> + head = resolve_ref_unsafe("HEAD", 0, sha1, &flag);The last parameter is a set of flag bits, so call it flags.
Show 5 quoted lines
> + if (!head) {
> + if (get_sha1("HEAD", sha1)) {
> + string_list_clear(&revs, 0);
> + string_list_clear(&states, 0);
> + die(_("Bad HEAD - I need a HEAD"));We see many repeated calls to clear these two string lists before exiting with failure, either by dying or return -1.
I wonder how bad the resulting code would look like if we employed the standard pattern of having a "fail_return:" label at the end of the function (after the "return" for the usual control flow) to clear them. If the result becomes less readable (and I suspect that you would end up making it less readable), leaving the current code structure is OK.
Show 13 quoted lines
> + }
> + }
> + 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 again."),
> + start_head.buf);The original suggests to try "git bisect reset" here to recover.
Show 8 quoted lines
> + strbuf_release(&start_head);
> + string_list_clear(&revs, 0);
> + string_list_clear(&states, 0);
> + return -1;
> + }
> + }
> + } else {
> + if (starts_with(head, "refs/head/") || !get_oid(head, &oid)) {get_oid() is insufficient to ensure what you have in $head is 40-hex. I think you meant get_oid_hex() here.
Show 12 quoted lines
> + /*
> + * This error message should only be triggered by
> + * cogito usage, and cogito users should understand
> + * it relates to cg-seek.
> + */
> + if (!is_empty_or_missing_file(git_path_head_name())) {
> + strbuf_release(&start_head);
> + string_list_clear(&revs, 0);
> + string_list_clear(&states, 0);
> + die(_("won't bisect on cg-seek'ed tree"));
> + }
> + if (starts_with(head, "refs/heads/")) {skip_prefix(), perhaps, if "head" is no longer used from here on?
Show 12 quoted lines
> + /*
> + * Write new start state
> + */
> + fp = fopen(git_path_bisect_start(), "w");
> + if (!fp) {
> + bisect_clean_state();
> + strbuf_release(&start_head);
> + string_list_clear(&revs, 0);
> + string_list_clear(&states, 0);
> + return -1;
> + }
> + if (!fprintf(fp, "%s\n", start_head.buf)) {man 3 fprintf and look for "Return Value"?
Show 8 quoted lines
> + fclose(fp);
> + bisect_clean_state();
> + strbuf_release(&start_head);
> + string_list_clear(&revs, 0);
> + string_list_clear(&states, 0);
> + return -1;
> + }
> + fclose(fp);
Perhaps use write_file() instead of the above block of text?
> + if (no_checkout) {
> + get_oid(start_head.buf, &oid);
> + if (update_ref(NULL, "BISECT_HEAD", oid.hash, NULL, 0,
> + UPDATE_REFS_MSG_ON_ERR)) {Doesn't the original use --no-deref for this update-ref call?
Show 12 quoted lines
> + bisect_clean_state();
> + strbuf_release(&start_head);
> + string_list_clear(&revs, 0);
> + string_list_clear(&states, 0);
> + return -1;
> + }
> + }
> + strbuf_release(&start_head);
> + fp = fopen(git_path_bisect_names(), "w");
> +
> + for (; i < argc; i++) {
> + if (!fprintf(fp, "%s ", argv[i])) {man 3 fprintf and look for "Return Value"?
More importantly, the original does --sq-quote so that BISECT_NAMES file can be read back by a shell. This is important as argv[i] can have whitespace in it, and you are concatenating them with SP in between here. Also you are not terminating that line.
Show 8 quoted lines
> + fclose(fp);
> + bisect_clean_state();
> + string_list_clear(&revs, 0);
> + string_list_clear(&states, 0);
> + return -1;
> + }
> + }
> + fclose(fp);
Perhaps
strbuf_reset(&bisect_names);
if (pathspec_pos < argc)
sq_quote_argv(&bisect_names, argv + pathspec_pos, 0);
write_file(git_path_bisect_names(), "%s\n", bisect_names.buf);
or something like that?
> + for (j = 0; j < states.nr; j ++) {Again, is "i" still precious here? Style: drop SP between j and ++.
Show 21 quoted lines
> + fp = fopen(git_path_bisect_log(), "a");
> + if (!fp) {
> + bisect_clean_state();
> + return -1;
> + }
> + if (!fprintf(fp, "git bisect start")) {
> + bisect_clean_state();
> + return -1;
> + }
> + for (i = 0; i < argc; i++) {
> + if (!fprintf(fp, " '%s'", argv[i])) {
> + fclose(fp);
> + bisect_clean_state();
> + return -1;
> + }
> + }
> + if (!fprintf(fp, "\n")) {
> + fclose(fp);
> + bisect_clean_state();
> + return -1;
> + }Again, the original writes orig_args which was protected with --sq-quote.
Show 5 quoted lines
> + fclose(fp);
> +
> + return 0;
> +}
> +
Pranit Bauva