Hey Junio,
On Wed, Aug 3, 2016 at 1:49 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Pranit Bauva <pranit.bauva@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html