Re: [RFC/PATCH v11 13/13] bisect--helper: `bisect_start` shell function partially in C

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

Hey Junio,

On Wed, Aug 3, 2016 at 1:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> +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++) {
>
> SP after for.

Sure!

>> +             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.


I wanted to discuss this precisely by this RFC. I was initially
thinking of using OPT_ARGUMENT() for bisect_terms() which would in
turn cover up for bisect_start() too. Currently the code does not
support --term-good=boa because it treats --term-good as a boolean Do
you have any other thing in mind?

>> +             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.

Yes sure!

>> +                     string_list_clear(&revs, 0);
>> +                     string_list_clear(&revs, 0);
>
> You seem to want the revs list really really clean ;-)

Haha! ;) My bad. Will remove the extra line!

>> +                     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.

I am using i afterwards for writing the arguments to BISECT_NAMES. But
I think it would be better to use pathspec_pos and discard j
altogether. Thanks!
>
>> +             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);
>         }

Seems better. Thanks!

>> +
>> +     /*
>> +      * Verify HEAD
>> +      */
>> +     head = resolve_ref_unsafe("HEAD", 0, sha1, &flag);
>
> The last parameter is a set of flag bits, so call it flags.

Sure!

>> +     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.

Its becoming really bad. I tried it once. Different things are being
cleaned up at different times which makes it all the more tedious to
maintain.

>> +             }
>> +     }
>> +     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.

Will include it.

>> +                             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.

Yes definitely. Might have been missed.

>> +                     /*
>> +                      * 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?

Sure!

>> +     /*
>> +      * 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"?

I should have been more careful about fprintf's return value.

>> +             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?

Yes, that seems better. Thanks!

>> +     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?

Yes, will change.

>> +                     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.

Yes its a good idea to retain its --sq-quote nature.

>> +                     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?

Yes! Thanks! Looks pretty good to me.

>> +     for (j = 0; j < states.nr; j ++) {
>
> Again, is "i" still precious here?  Style: drop SP between j and ++.

After BISECT_NAMES, "i" ceases to be precious.

>> +     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.

Yes. Will take care of it.

>> +     fclose(fp);
>> +
>> +     return 0;
>> +}
>> +

Regards,
Pranit Bauva