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

[Pranit Bauva]

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(>term_good);
>> + strbuf_addstr(>term_good, argv[++i]);
>> + break;
>> + }
>> + if (!strcmp(argv[i], "--term-bad")) {
>> + must_write_terms = 1;
>> + strbuf_reset(>term_bad);
>> + strbuf_addstr(>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(, 0);
>> + string_list_clear(, 0);
>> + die(_("unrecognised option: '%s'"), argv[i]);
>> + }
>> + if (get_oid(argv[i], ) || 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(, 0);
>> + string_list_clear(, 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(, oid_to_hex());
>> + }
>> +
>> + 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(, terms->term_good.buf);
>> + else {
>> + bad_seen = 1;
>> + strbuf_addstr(, terms->term_bad.buf);
>> + }
>> + string_list_append(, state.buf);
>> + strbuf_release();
>> + }
>
> 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(, terms->term_good.buf);
> else
>

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

[Junio C Hamano]

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(>term_good);
> + strbuf_addstr(>term_good, argv[++i]);
> + break;
> + }
> + if (!strcmp(argv[i], "--term-bad")) {
> + must_write_terms = 1;
> + strbuf_reset(>term_bad);
> + strbuf_addstr(>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(, 0);
> + string_list_clear(, 0);
> + die(_("unrecognised option: '%s'"), argv[i]);
> + }
> + if (get_oid(argv[i], ) || 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(, 0);
> + string_list_clear(, 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(, oid_to_hex());
> + }
> +
> + 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(, terms->term_good.buf);
> + else {
> + bad_seen = 1;
> + strbuf_addstr(, terms->term_bad.buf);
> + }
> + string_list_append(, state.buf);
> + strbuf_release();
> + }

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(, terms->term_good.buf);
else
string_list_append(, terms->term_bad.buf);
}

> +
> + /*
> +  * Verify HEAD
> +  */
> + head = resolve_ref_unsafe("HEAD", 0, sha1, );

The last parameter is a set of flag bits, so call it flags.

> + if (!head) {
> + if (get_sha1("HEAD", sha1)) {
> + string_list_clear(, 0);
> + string_list_clear(, 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" 

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

[Pranit Bauva]

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 

---
This patch contains a small bug. The option handling for `--term-good`
and `--term-bad` needs to be decided as it is now shared between
`--bisect-terms` and `--bisect-start` and the later one also requires
string support. Can comments on which approach would seem the most
feasible? Here[1] is a normal output of t6030 and here[2] is a verbose
output of [2].

[1]: http://paste.ubuntu.com/21252069/
[2]: http://paste.ubuntu.com/21252140/
---
 builtin/bisect--helper.c | 232 ++-
 git-bisect.sh| 132 +--
 2 files changed, 232 insertions(+), 132 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 81a16a5..ab18786 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -404,6 +404,228 @@ static int bisect_terms(struct bisect_terms *terms, int 
term_defined)
return 0;
 }
 
+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++) {
+   if (!strcmp(argv[i], "--")) {
+   has_double_dash = 1;
+   break;
+   }
+   if (!strcmp(argv[i], "--term-good")) {
+   must_write_terms = 1;
+   strbuf_reset(>term_good);
+   strbuf_addstr(>term_good, argv[++i]);
+   break;
+   }
+   if (!strcmp(argv[i], "--term-bad")) {
+   must_write_terms = 1;
+   strbuf_reset(>term_bad);
+   strbuf_addstr(>term_bad, argv[++i]);
+   break;
+   }
+   if (starts_with(argv[i], "--") &&
+   !one_of(argv[i], "--term-good", "--term-bad", NULL)) {
+   string_list_clear(, 0);
+   string_list_clear(, 0);
+   die(_("unrecognised option: '%s'"), argv[i]);
+   }
+   if (get_oid(argv[i], ) || has_double_dash) {
+   string_list_clear(, 0);
+   string_list_clear(, 0);
+   die(_("'%s' does not appear to be a valid revision"), 
argv[i]);
+   }
+   else
+   string_list_append(, oid_to_hex());
+   }
+
+   for (j = 0; j < revs.nr; j++) {
+   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(, terms->term_good.buf);
+   else {
+   bad_seen = 1;
+   strbuf_addstr(, terms->term_bad.buf);
+   }
+   string_list_append(, state.buf);
+   strbuf_release();
+   }
+
+   /*
+* Verify HEAD
+*/
+   head = resolve_ref_unsafe("HEAD", 0, sha1, );
+   if (!head) {
+   if (get_sha1("HEAD", sha1)) {
+   string_list_clear(, 0);
+   string_list_clear(, 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(_head, git_path_bisect_start(), 0);
+   strbuf_trim(_head);
+   if (!no_checkout) {
+   struct argv_array argv =