Re: [RFC/PATCH v11 13/13] bisect--helper: `bisect_start` shell function partially in C
Hey Junio,
On Wed, Aug 3, 2016 at 1:49 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, 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 ", 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->
Re: [RFC/PATCH v11 13/13] 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, 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.
> + 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.
> + 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 ;-)
> + 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.
> + struct strbuf state = STRBUF_INIT;
> + /*
> + * 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 = 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);
}
> +
> + /*
> + * Verify HEAD
> + */
> + head = resolve_ref_unsafe("HEAD", 0, sha1, &flag);
The last parameter is a set of flag bits, so call it 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"));
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 stan
