[PATCH] bisect--helper: convert a function in shell to C

[PATCH] bisect--helper: convert a function in shell to C

[Pranit Bauva]

Convert the code literally without changing its design even though it
seems that its obscure as to the use of comparing revision to different bisect
arguments which seems like a problem in shell because of the way
function arguments are handled.

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229..61abe68 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -2,12 +2,35 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "bisect.h"
+#include "refs.h"
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
 	NULL
 };
 
+static int check_term_format(const char *term, const char *revision, int flag) {
+	if (check_refname_format(term, flag))
+		die("'%s' is not a valid term", term);
+
+	if (!strcmp(term, "help") || !strcmp(term, "start") ||
+		!strcmp(term, "skip") || !strcmp(term, "next") ||
+		!strcmp(term, "reset") || !strcmp(term, "visualize") ||
+		!strcmp(term, "replay") || !strcmp(term, "log") ||
+		!strcmp(term, "run"))
+		die("can't use the builtin command '%s' as a term", term);
+
+	if (!strcmp(term, "bad") || !strcmp(term, "new"))
+		if(strcmp(revision, "bad"))
+			die("can't change the meaning of term '%s'", term);
+
+	if (!strcmp(term, "good") || !strcmp(term, "old"))
+		if (strcmp(revision, "good"))
+			die("can't change the meaning of term '%s'", term);
+
+	return 1;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	int next_all = 0;

--
https://github.com/git/git/pull/216

Re: [PATCH] bisect--helper: convert a function in shell to C

[Stefan Beller]

On Mon, Mar 21, 2016 at 12:00 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Convert the code literally without changing its design even though it
> seems that its obscure as to the use of comparing revision to different bisect
> arguments which seems like a problem in shell because of the way
> function arguments are handled.

How would I use the C version instead of the shell version now?
I'd imagine you'd want to change calls in git-bisect.sh from
    check_term_format <term> <bad/new>
to be:
    git bisect--helper check_term_format <term> <bad/new>
and "git bisect--helper" would then call the new static method?
Once you have the C version working (do we need additional tests
or can we rely on the test suite being enough for now?),
you can also delete the shell version.

Thanks,
Stefan

>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
>  builtin/bisect--helper.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3324229..61abe68 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -2,12 +2,35 @@
>  #include "cache.h"
>  #include "parse-options.h"
>  #include "bisect.h"
> +#include "refs.h"
>
>  static const char * const git_bisect_helper_usage[] = {
>         N_("git bisect--helper --next-all [--no-checkout]"),
>         NULL
>  };
>
> +static int check_term_format(const char *term, const char *revision, int flag) {
> +       if (check_refname_format(term, flag))
> +               die("'%s' is not a valid term", term);
> +
> +       if (!strcmp(term, "help") || !strcmp(term, "start") ||
> +               !strcmp(term, "skip") || !strcmp(term, "next") ||
> +               !strcmp(term, "reset") || !strcmp(term, "visualize") ||
> +               !strcmp(term, "replay") || !strcmp(term, "log") ||
> +               !strcmp(term, "run"))
> +               die("can't use the builtin command '%s' as a term", term);

"terms" is missing?

eval_gettext would translate into C as
    die (_("translatable message"));


> +
> +       if (!strcmp(term, "bad") || !strcmp(term, "new"))
> +               if(strcmp(revision, "bad"))
> +                       die("can't change the meaning of term '%s'", term);
> +
> +       if (!strcmp(term, "good") || !strcmp(term, "old"))
> +               if (strcmp(revision, "good"))
> +                       die("can't change the meaning of term '%s'", term);
> +
> +       return 1;

Why 1? Usually we use 0 for success in C. die(...) returns with non zero,
so having 0 here would help us in the shell code to see if the C version
died (or not).

> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>         int next_all = 0;
>
> --
> https://github.com/git/git/pull/216
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] bisect--helper: convert a function in shell to C

[Christian Couder]

On Tue, Mar 22, 2016 at 1:28 AM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Mar 21, 2016 at 12:00 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Convert the code literally without changing its design even though it
>> seems that its obscure as to the use of comparing revision to different bisect
>> arguments which seems like a problem in shell because of the way
>> function arguments are handled.
>
> How would I use the C version instead of the shell version now?

Hint: one can look at how other functions in builtin/bisect--helper.c are used.

> I'd imagine you'd want to change calls in git-bisect.sh from
>     check_term_format <term> <bad/new>
> to be:
>     git bisect--helper check_term_format <term> <bad/new>

Hint: to get a good idea of how the call should be, one can look at
the way "git-bisect.sh" already calls "git bisect--helper".

> and "git bisect--helper" would then call the new static method?
> Once you have the C version working (do we need additional tests
> or can we rely on the test suite being enough for now?),
> you can also delete the shell version.

Re: [PATCH] bisect--helper: convert a function in shell to C

[Pranit Bauva]

On Tue, Mar 22, 2016 at 11:40 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Tue, Mar 22, 2016 at 1:28 AM, Stefan Beller <sbeller@google.com> wrote:
>> On Mon, Mar 21, 2016 at 12:00 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> Convert the code literally without changing its design even though it
>>> seems that its obscure as to the use of comparing revision to different bisect
>>> arguments which seems like a problem in shell because of the way
>>> function arguments are handled.
>>
>> How would I use the C version instead of the shell version now?
>
> Hint: one can look at how other functions in builtin/bisect--helper.c are used.
>
>> I'd imagine you'd want to change calls in git-bisect.sh from
>>     check_term_format <term> <bad/new>
>> to be:
>>     git bisect--helper check_term_format <term> <bad/new>
>
> Hint: to get a good idea of how the call should be, one can look at
> the way "git-bisect.sh" already calls "git bisect--helper".
>
>> and "git bisect--helper" would then call the new static method?
>> Once you have the C version working (do we need additional tests
>> or can we rely on the test suite being enough for now?),
>> you can also delete the shell version.

Thanks a lot! I will dig more into this.

Re: [PATCH] bisect--helper: convert a function in shell to C

[Pranit Bauva]

On Tue, Mar 22, 2016 at 5:58 AM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Mar 21, 2016 at 12:00 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Convert the code literally without changing its design even though it
>> seems that its obscure as to the use of comparing revision to different bisect
>> arguments which seems like a problem in shell because of the way
>> function arguments are handled.
>
> How would I use the C version instead of the shell version now?
> I'd imagine you'd want to change calls in git-bisect.sh from
>     check_term_format <term> <bad/new>
> to be:
>     git bisect--helper check_term_format <term> <bad/new>
> and "git bisect--helper" would then call the new static method?
> Once you have the C version working (do we need additional tests
> or can we rely on the test suite being enough for now?),
> you can also delete the shell version.

I have to yet think about this. Currently, I just called this function
from cmd_bisect__helper().

> Thanks,
> Stefan
>
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>>  builtin/bisect--helper.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 3324229..61abe68 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -2,12 +2,35 @@
>>  #include "cache.h"
>>  #include "parse-options.h"
>>  #include "bisect.h"
>> +#include "refs.h"
>>
>>  static const char * const git_bisect_helper_usage[] = {
>>         N_("git bisect--helper --next-all [--no-checkout]"),
>>         NULL
>>  };
>>
>> +static int check_term_format(const char *term, const char *revision, int flag) {
>> +       if (check_refname_format(term, flag))
>> +               die("'%s' is not a valid term", term);
>> +
>> +       if (!strcmp(term, "help") || !strcmp(term, "start") ||
>> +               !strcmp(term, "skip") || !strcmp(term, "next") ||
>> +               !strcmp(term, "reset") || !strcmp(term, "visualize") ||
>> +               !strcmp(term, "replay") || !strcmp(term, "log") ||
>> +               !strcmp(term, "run"))
>> +               die("can't use the builtin command '%s' as a term", term);
>
> "terms" is missing?
>
> eval_gettext would translate into C as
>     die (_("translatable message"));
>
>
>> +
>> +       if (!strcmp(term, "bad") || !strcmp(term, "new"))
>> +               if(strcmp(revision, "bad"))
>> +                       die("can't change the meaning of term '%s'", term);
>> +
>> +       if (!strcmp(term, "good") || !strcmp(term, "old"))
>> +               if (strcmp(revision, "good"))
>> +                       die("can't change the meaning of term '%s'", term);
>> +
>> +       return 1;
>
> Why 1? Usually we use 0 for success in C. die(...) returns with non zero,
> so having 0 here would help us in the shell code to see if the C version
> died (or not).

Sorry I forgot to change the value. I had initially kept it to 0 but
then to do some tweaks, I changed it to 1. I had to do manual tweaks
as I am still scared to use gdb for big projects. I am trying to get
more comfortable with it. Also reading the patch again, I seem to have
forgotten the function declaration statement also.

[1] : http://thread.gmane.org/gmane.comp.version-control.git/289299/focus=289364

[PATCH v2] bisect--helper: convert a function in shell to C

[Pranit Bauva]

Convert the code literally without changing its design even though it
seems that its obscure as to the use of comparing revision to different
bisect arguments which seems like a problem in shell because of the way
function arguments are handled.

The argument handling is kind of hard coded right now because it is not
really be meant to be used like this and this is just for testing
purposes whether this new method is as functional as its counter part.
The shell counter part of the method has been retained for historical
purposes.

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 37 +++++++++++++++++++++++++++++++++++++
 git-bisect.sh            |  4 ++--
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229..6cdae82 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -2,27 +2,64 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "bisect.h"
+#include "refs.h"
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
+	N_("git bisect--helper --check-term-format <term> <revision>"),
 	NULL
 };
 
+static int check_term_format(const char *term, const char *revision, int flags);
+
+static int check_term_format(const char *term, const char *revision, int flag) {
+	if (check_refname_format(term, flag))
+		die("'%s' is not a valid term", term);
+
+	if (!strcmp(term, "help") || !strcmp(term, "start") ||
+		!strcmp(term, "skip") || !strcmp(term, "next") ||
+		!strcmp(term, "reset") || !strcmp(term, "visualize") ||
+		!strcmp(term, "replay") || !strcmp(term, "log") ||
+		!strcmp(term, "run"))
+		die("can't use the builtin command '%s' as a term", term);
+
+	if (!strcmp(term, "bad") || !strcmp(term, "new"))
+		if(strcmp(revision, "bad"))
+			die("can't change the meaning of term '%s'", term);
+
+	if (!strcmp(term, "good") || !strcmp(term, "old"))
+		if (strcmp(revision, "good"))
+			die("can't change the meaning of term '%s'", term);
+
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	int next_all = 0;
 	int no_checkout = 0;
+	const char *term;
 	struct option options[] = {
 		OPT_BOOL(0, "next-all", &next_all,
 			 N_("perform 'git bisect next'")),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
+		OPT_STRING(0, "check-term-format", &term, N_("term"),
+			 N_("check the format of the ref")),
 		OPT_END()
 	};
 
 	argc = parse_options(argc, argv, prefix, options,
 			     git_bisect_helper_usage, 0);
 
+
+	if (term != NULL) {
+		if (argc > 0)
+			return check_term_format(term, argv[0], 0);
+		else
+			die("no revision provided with check_for_term");
+	}
+
 	if (!next_all)
 		usage_with_options(git_bisect_helper_usage, options);
 
diff --git a/git-bisect.sh b/git-bisect.sh
index 5d1cb00..ea237be 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -564,8 +564,8 @@ write_terms () {
 	then
 		die "$(gettext "please use two different terms")"
 	fi
-	check_term_format "$TERM_BAD" bad
-	check_term_format "$TERM_GOOD" good
+	git bisect--helper --check-term-format="$TERM_BAD" bad
+	git bisect--helper --check-term-format="$TERM_GOOD" good
 	printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
 }
 

--
https://github.com/git/git/pull/216

Re: [PATCH v2] bisect--helper: convert a function in shell to C

[Johannes Schindelin]

Hi,

On Tue, 22 Mar 2016, Pranit Bauva wrote:

> Convert the code literally without changing its design even though it
> seems that its obscure as to the use of comparing revision to different

s/its/it is/

> bisect arguments which seems like a problem in shell because of the way
> function arguments are handled.

I agree that it is obscure. That is why I would suggest to fix it during
the conversion. Using 'new_term' and 'orig_term' (or something similar)
would make much more sense.

Another good idea would be to include the shell code, or at least to
provide a link such as:

	https://github.com/git/git/blob/v2.8.0-rc4/git-bisect.sh#L572-L597

> The argument handling is kind of hard coded right now because it is not
> really be meant to be used like this and this is just for testing
> purposes whether this new method is as functional as its counter part.
> The shell counter part of the method has been retained for historical
> purposes.

Still, it would make more sense (both in terms of readability and in terms
of code safety) to introduce and use a function like

static int one_of(const char *term, ...)
{
	va_list matches;
	const char *match;

	va_start(matches, term);
	while ((match = va_arg(matches, const char *)))
		if (!strcmp(term, match))
			return 1;
	va_end(matches);

	return 0;
}

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3324229..6cdae82 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -2,27 +2,64 @@
>  #include "cache.h"
>  #include "parse-options.h"
>  #include "bisect.h"
> +#include "refs.h"
>  
>  static const char * const git_bisect_helper_usage[] = {
>  	N_("git bisect--helper --next-all [--no-checkout]"),
> +	N_("git bisect--helper --check-term-format <term> <revision>"),

Good. This imitates the existing code.

>  	NULL
>  };
>  
> +static int check_term_format(const char *term, const char *revision, int flags);
> +
> +static int check_term_format(const char *term, const char *revision, int flag) {

Since you define the check_term_format() function here, the declaration
above is unnecessary. Let's just delete it.

> +	if (check_refname_format(term, flag))
> +		die("'%s' is not a valid term", term);
> +
> +	if (!strcmp(term, "help") || !strcmp(term, "start") ||
> +		!strcmp(term, "skip") || !strcmp(term, "next") ||
> +		!strcmp(term, "reset") || !strcmp(term, "visualize") ||
> +		!strcmp(term, "replay") || !strcmp(term, "log") ||
> +		!strcmp(term, "run"))
> +		die("can't use the builtin command '%s' as a term", term);

This would look so much nicer using the one_of() function above.

Please also note that our coding convention (as can be seen from the
existing code in builtin/*.c) is to indent the condition differently than
the block, either using an extra tab, or by using 4 spaces instead of the
tab.

> +	if (!strcmp(term, "bad") || !strcmp(term, "new"))
> +		if(strcmp(revision, "bad"))
> +			die("can't change the meaning of term '%s'", term);
> +
> +	if (!strcmp(term, "good") || !strcmp(term, "old"))
> +		if (strcmp(revision, "good"))
> +			die("can't change the meaning of term '%s'", term);

These two can be combined. Actually, these *four* can easily be combined:

	if ((one_of(term, "bad", "new", NULL) && strcmp(orig, "bad")) ||
	    (one_of(term, "good", "old", NULL) && strcmp(orig, "good")))
		die("can't change the meaning of term '%s'", term);

>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>  	int next_all = 0;
>  	int no_checkout = 0;
> +	const char *term;

Better use the existing convention:

	int check_term_format = 0;

>  	struct option options[] = {
>  		OPT_BOOL(0, "next-all", &next_all,
>  			 N_("perform 'git bisect next'")),
>  		OPT_BOOL(0, "no-checkout", &no_checkout,
>  			 N_("update BISECT_HEAD instead of checking out the current commit")),
> +		OPT_STRING(0, "check-term-format", &term, N_("term"),
> +			 N_("check the format of the ref")),

Hmm. The existing code suggests to use OPT_BOOL instead.

>  		OPT_END()
>  	};
>  
>  	argc = parse_options(argc, argv, prefix, options,
>  			     git_bisect_helper_usage, 0);
>  
> +
> +	if (term != NULL) {
> +		if (argc > 0)

Here you need to test for a precise argc, not for a range.

> +			return check_term_format(term, argv[0], 0);
> +		else
> +			die("no revision provided with check_for_term");
> +	}
> +
>  	if (!next_all)
>  		usage_with_options(git_bisect_helper_usage, options);
>  
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 5d1cb00..ea237be 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -564,8 +564,8 @@ write_terms () {
>  	then
>  		die "$(gettext "please use two different terms")"
>  	fi
> -	check_term_format "$TERM_BAD" bad
> -	check_term_format "$TERM_GOOD" good
> +	git bisect--helper --check-term-format="$TERM_BAD" bad
> +	git bisect--helper --check-term-format="$TERM_GOOD" good

The existing convention is to make the first argument *not* a value of the
"option", i.e. `--check-term-format "$TERM_BAD"` without an equal sign.

Did you also run the test suite after compiling, to verify that the
documented expectations are still met after the conversion?

Ciao,
Johannes

Re: [PATCH v2] bisect--helper: convert a function in shell to C

[Johannes Schindelin]

Hi,

On Tue, 22 Mar 2016, Johannes Schindelin wrote:

> On Tue, 22 Mar 2016, Pranit Bauva wrote:
> 
> > +	if (!strcmp(term, "bad") || !strcmp(term, "new"))
> > +		if(strcmp(revision, "bad"))
> > +			die("can't change the meaning of term '%s'", term);
> > +
> > +	if (!strcmp(term, "good") || !strcmp(term, "old"))
> > +		if (strcmp(revision, "good"))
> > +			die("can't change the meaning of term '%s'", term);
> 
> These two can be combined. Actually, these *four* can easily be combined:
> 
> 	if ((one_of(term, "bad", "new", NULL) && strcmp(orig, "bad")) ||
> 	    (one_of(term, "good", "old", NULL) && strcmp(orig, "good")))
> 		die("can't change the meaning of term '%s'", term);

Completely forgot to mention: This conversion skipped the comment

	# In theory, nothing prevents swapping
	# completely good and bad, but this situation
	# could be confusing and hasn't been tested
	# enough. Forbid it for now.

Let's port that comment over, too?

Ciao,
Johannes

Re: [PATCH v2] bisect--helper: convert a function in shell to C

[Pranit Bauva]

On Tue, Mar 22, 2016 at 8:41 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Tue, 22 Mar 2016, Johannes Schindelin wrote:
>
>> On Tue, 22 Mar 2016, Pranit Bauva wrote:
>>
>> > +   if (!strcmp(term, "bad") || !strcmp(term, "new"))
>> > +           if(strcmp(revision, "bad"))
>> > +                   die("can't change the meaning of term '%s'", term);
>> > +
>> > +   if (!strcmp(term, "good") || !strcmp(term, "old"))
>> > +           if (strcmp(revision, "good"))
>> > +                   die("can't change the meaning of term '%s'", term);
>>
>> These two can be combined. Actually, these *four* can easily be combined:
>>
>>       if ((one_of(term, "bad", "new", NULL) && strcmp(orig, "bad")) ||
>>           (one_of(term, "good", "old", NULL) && strcmp(orig, "good")))
>>               die("can't change the meaning of term '%s'", term);
>
> Completely forgot to mention: This conversion skipped the comment
>
>         # In theory, nothing prevents swapping
>         # completely good and bad, but this situation
>         # could be confusing and hasn't been tested
>         # enough. Forbid it for now.
>
> Let's port that comment over, too?

Sure! Adding a comment won't harm anyone. We can remove it when its
thoroughly tested.
>
> Ciao,
> Johannes

Re: [PATCH v2] bisect--helper: convert a function in shell to C

[Johannes Schindelin]

Hi Pranit,

On Tue, 22 Mar 2016, Pranit Bauva wrote:

> On Tue, Mar 22, 2016 at 8:41 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Tue, 22 Mar 2016, Johannes Schindelin wrote:
> >
> >> On Tue, 22 Mar 2016, Pranit Bauva wrote:
> >>
> >> > +   if (!strcmp(term, "bad") || !strcmp(term, "new"))
> >> > +           if(strcmp(revision, "bad"))
> >> > +                   die("can't change the meaning of term '%s'", term);
> >> > +
> >> > +   if (!strcmp(term, "good") || !strcmp(term, "old"))
> >> > +           if (strcmp(revision, "good"))
> >> > +                   die("can't change the meaning of term '%s'", term);
> >>
> >> These two can be combined. Actually, these *four* can easily be combined:
> >>
> >>       if ((one_of(term, "bad", "new", NULL) && strcmp(orig, "bad")) ||
> >>           (one_of(term, "good", "old", NULL) && strcmp(orig, "good")))
> >>               die("can't change the meaning of term '%s'", term);
> >
> > Completely forgot to mention: This conversion skipped the comment
> >
> >         # In theory, nothing prevents swapping
> >         # completely good and bad, but this situation
> >         # could be confusing and hasn't been tested
> >         # enough. Forbid it for now.
> >
> > Let's port that comment over, too?
> 
> Sure! Adding a comment won't harm anyone. We can remove it when its
> thoroughly tested.

I am actually not so eager to remove the comment...

Ciao,
Johannes

Re: [PATCH v2] bisect--helper: convert a function in shell to C

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> static int one_of(const char *term, ...)
> {
> 	va_list matches;
> 	const char *match;
>
> 	va_start(matches, term);
> 	while ((match = va_arg(matches, const char *)))
> 		if (!strcmp(term, match))
> 			return 1;
> 	va_end(matches);
>
> 	return 0;
> }

This is a very good suggestion.  We tend to avoid explicit
comparison to NULL and zero, but we avoid assignment in condition
part of if/while statements even more, so

 	while ((match = va_arg(matches, const char *)) != NULL)

would be the best compromise to make it clear and readable.

>> +	if (!strcmp(term, "help") || !strcmp(term, "start") ||
>> +		!strcmp(term, "skip") || !strcmp(term, "next") ||
>> +		!strcmp(term, "reset") || !strcmp(term, "visualize") ||
>> +		!strcmp(term, "replay") || !strcmp(term, "log") ||
>> +		!strcmp(term, "run"))
>> +		die("can't use the builtin command '%s' as a term", term);
>
> This would look so much nicer using the one_of() function above.
>
> Please also note that our coding convention (as can be seen from the
> existing code in builtin/*.c) is to indent the condition differently than
> the block, either using an extra tab, or by using 4 spaces instead of the
> tab.

In general, "or by using 4 spaces" is better spelled as "or by
indenting so that the line aligns with the beginning of the inside
of the opening parenthesis on the above line"; "if (" happens to
take 4 display spaces and that is where "4" comes from and it would
be different for "while (" condition ;-).

But with one_of(...) this part would change a lot, I imagine.
>>  	struct option options[] = {
>>  		OPT_BOOL(0, "next-all", &next_all,
>>  			 N_("perform 'git bisect next'")),
>>  		OPT_BOOL(0, "no-checkout", &no_checkout,
>>  			 N_("update BISECT_HEAD instead of checking out the current commit")),
>> +		OPT_STRING(0, "check-term-format", &term, N_("term"),
>> +			 N_("check the format of the ref")),
>
> Hmm. The existing code suggests to use OPT_BOOL instead.
> ...
> The existing convention is to make the first argument *not* a value of the
> "option", i.e. `--check-term-format "$TERM_BAD"` without an equal sign.

I think it is preferrable to keep using OPT_BOOL() for this new one
if we are incrementally building on top of existing code.

But if the convention is that the option is to specify what opration
is invoked, using OPT_CMDMODE() to implement all of them would be a
worthwhile cleanup to consider at some point.

I agree with all the points you raised in your review.  Just wanted
to add some clarification myself.

Thanks.

Re: [PATCH v2] bisect--helper: convert a function in shell to C

[Johannes Schindelin]

Hi Junio,

On Tue, 22 Mar 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >>  	struct option options[] = {
> >>  		OPT_BOOL(0, "next-all", &next_all,
> >>  			 N_("perform 'git bisect next'")),
> >>  		OPT_BOOL(0, "no-checkout", &no_checkout,
> >>  			 N_("update BISECT_HEAD instead of checking out the current commit")),
> >> +		OPT_STRING(0, "check-term-format", &term, N_("term"),
> >> +			 N_("check the format of the ref")),
> >
> > Hmm. The existing code suggests to use OPT_BOOL instead.
> > ...
> > The existing convention is to make the first argument *not* a value of the
> > "option", i.e. `--check-term-format "$TERM_BAD"` without an equal sign.
> 
> I think it is preferrable to keep using OPT_BOOL() for this new one
> if we are incrementally building on top of existing code.
> 
> But if the convention is that the option is to specify what opration
> is invoked, using OPT_CMDMODE() to implement all of them would be a
> worthwhile cleanup to consider at some point.

Good point, I keep forgetting that OPT_CMDMODE() was introduced
specifically for subcommands.

Ciao,
Dscho

Re: [PATCH v2] bisect--helper: convert a function in shell to C

[Pranit Bauva]

On Tue, Mar 22, 2016 at 9:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> static int one_of(const char *term, ...)
>> {
>>       va_list matches;
>>       const char *match;
>>
>>       va_start(matches, term);
>>       while ((match = va_arg(matches, const char *)))
>>               if (!strcmp(term, match))
>>                       return 1;
>>       va_end(matches);
>>
>>       return 0;
>> }
>
> This is a very good suggestion.  We tend to avoid explicit
> comparison to NULL and zero, but we avoid assignment in condition
> part of if/while statements even more, so
>
>         while ((match = va_arg(matches, const char *)) != NULL)
>
> would be the best compromise to make it clear and readable.
>
>>> +    if (!strcmp(term, "help") || !strcmp(term, "start") ||
>>> +            !strcmp(term, "skip") || !strcmp(term, "next") ||
>>> +            !strcmp(term, "reset") || !strcmp(term, "visualize") ||
>>> +            !strcmp(term, "replay") || !strcmp(term, "log") ||
>>> +            !strcmp(term, "run"))
>>> +            die("can't use the builtin command '%s' as a term", term);
>>
>> This would look so much nicer using the one_of() function above.
>>
>> Please also note that our coding convention (as can be seen from the
>> existing code in builtin/*.c) is to indent the condition differently than
>> the block, either using an extra tab, or by using 4 spaces instead of the
>> tab.
>
> In general, "or by using 4 spaces" is better spelled as "or by
> indenting so that the line aligns with the beginning of the inside
> of the opening parenthesis on the above line"; "if (" happens to
> take 4 display spaces and that is where "4" comes from and it would
> be different for "while (" condition ;-).

I will definitely keep this in mind henceforth.
>
> But with one_of(...) this part would change a lot, I imagine.
>>>      struct option options[] = {
>>>              OPT_BOOL(0, "next-all", &next_all,
>>>                       N_("perform 'git bisect next'")),
>>>              OPT_BOOL(0, "no-checkout", &no_checkout,
>>>                       N_("update BISECT_HEAD instead of checking out the current commit")),
>>> +            OPT_STRING(0, "check-term-format", &term, N_("term"),
>>> +                     N_("check the format of the ref")),
>>
>> Hmm. The existing code suggests to use OPT_BOOL instead.
>> ...
>> The existing convention is to make the first argument *not* a value of the
>> "option", i.e. `--check-term-format "$TERM_BAD"` without an equal sign.
>
> I think it is preferrable to keep using OPT_BOOL() for this new one
> if we are incrementally building on top of existing code.
>
> But if the convention is that the option is to specify what opration
> is invoked, using OPT_CMDMODE() to implement all of them would be a
> worthwhile cleanup to consider at some point.

OPT_CMDMODE() is actually a better option. I also noticed that it
isn't mentioned in Documentation/technical/api-parse-options.txt .
Should I send a patch to include it there just to make it easier for
someone who is new and isn't aware of the changes ?

> I agree with all the points you raised in your review.  Just wanted
> to add some clarification myself.
>
> Thanks.

Re: [PATCH v2] bisect--helper: convert a function in shell to C

[Stefan Beller]

On Tue, Mar 22, 2016 at 10:52 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> OPT_CMDMODE() is actually a better option. I also noticed that it
> isn't mentioned in Documentation/technical/api-parse-options.txt .
> Should I send a patch to include it there just to make it easier for
> someone who is new and isn't aware of the changes ?

Patches to outdated documentation are most awesome. ;)

Re: [PATCH v2] bisect--helper: convert a function in shell to C

[Johannes Schindelin]

Hi,

On Tue, 22 Mar 2016, Stefan Beller wrote:

> On Tue, Mar 22, 2016 at 10:52 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> > OPT_CMDMODE() is actually a better option. I also noticed that it
> > isn't mentioned in Documentation/technical/api-parse-options.txt .
> > Should I send a patch to include it there just to make it easier for
> > someone who is new and isn't aware of the changes ?
> 
> Patches to outdated documentation are most awesome. ;)

Yes!

Thanks,
Johannes

Re: [PATCH v2] bisect--helper: convert a function in shell to C

[Pranit Bauva]

On Tue, Mar 22, 2016 at 8:39 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Tue, 22 Mar 2016, Pranit Bauva wrote:
>
>> Convert the code literally without changing its design even though it
>> seems that its obscure as to the use of comparing revision to different
>
> s/its/it is/

Sure! A typo.

>> bisect arguments which seems like a problem in shell because of the way
>> function arguments are handled.
>
> I agree that it is obscure. That is why I would suggest to fix it during
> the conversion. Using 'new_term' and 'orig_term' (or something similar)
> would make much more sense.
>
> Another good idea would be to include the shell code, or at least to
> provide a link such as:
>
>         https://github.com/git/git/blob/v2.8.0-rc4/git-bisect.sh#L572-L597
>

I will take care about this henceforth.

>> The argument handling is kind of hard coded right now because it is not
>> really be meant to be used like this and this is just for testing
>> purposes whether this new method is as functional as its counter part.
>> The shell counter part of the method has been retained for historical
>> purposes.
>
> Still, it would make more sense (both in terms of readability and in terms
> of code safety) to introduce and use a function like
>
> static int one_of(const char *term, ...)
> {
>         va_list matches;
>         const char *match;
>
>         va_start(matches, term);
>         while ((match = va_arg(matches, const char *)))
>                 if (!strcmp(term, match))
>                         return 1;
>         va_end(matches);
>
>         return 0;
> }
>

>> +static int check_term_format(const char *term, const char *revision, int flags);
>> +
>> +static int check_term_format(const char *term, const char *revision, int flag) {
>
> Since you define the check_term_format() function here, the declaration
> above is unnecessary. Let's just delete it.

Yes. We could just add functions below this so that it would not
create a problem.

>
>> +     if (check_refname_format(term, flag))
>> +             die("'%s' is not a valid term", term);
>> +
>> +     if (!strcmp(term, "help") || !strcmp(term, "start") ||
>> +             !strcmp(term, "skip") || !strcmp(term, "next") ||
>> +             !strcmp(term, "reset") || !strcmp(term, "visualize") ||
>> +             !strcmp(term, "replay") || !strcmp(term, "log") ||
>> +             !strcmp(term, "run"))
>> +             die("can't use the builtin command '%s' as a term", term);
>
> This would look so much nicer using the one_of() function above.

one_of() will definitely make it clean.

> Please also note that our coding convention (as can be seen from the
> existing code in builtin/*.c) is to indent the condition differently than
> the block, either using an extra tab, or by using 4 spaces instead of the
> tab.
>

I
>> +     if (!strcmp(term, "bad") || !strcmp(term, "new"))
>> +             if(strcmp(revision, "bad"))
>> +                     die("can't change the meaning of term '%s'", term);
>> +
>> +     if (!strcmp(term, "good") || !strcmp(term, "old"))
>> +             if (strcmp(revision, "good"))
>> +                     die("can't change the meaning of term '%s'", term);
>
> These two can be combined. Actually, these *four* can easily be combined:
>
>         if ((one_of(term, "bad", "new", NULL) && strcmp(orig, "bad")) ||
>             (one_of(term, "good", "old", NULL) && strcmp(orig, "good")))
>                 die("can't change the meaning of term '%s'", term);
>
>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>  {
>>       int next_all = 0;
>>       int no_checkout = 0;
>> +     const char *term;
>
> Better use the existing convention:
>
>         int check_term_format = 0;
>
>>       struct option options[] = {
>>               OPT_BOOL(0, "next-all", &next_all,
>>                        N_("perform 'git bisect next'")),
>>               OPT_BOOL(0, "no-checkout", &no_checkout,
>>                        N_("update BISECT_HEAD instead of checking out the current commit")),
>> +             OPT_STRING(0, "check-term-format", &term, N_("term"),
>> +                      N_("check the format of the ref")),
>
> Hmm. The existing code suggests to use OPT_BOOL instead.
>
>>               OPT_END()
>>       };
>>
>>       argc = parse_options(argc, argv, prefix, options,
>>                            git_bisect_helper_usage, 0);
>>
>> +
>> +     if (term != NULL) {
>> +             if (argc > 0)
>
> Here you need to test for a precise argc, not for a range.

True.

>> +                     return check_term_format(term, argv[0], 0);
>> +             else
>> +                     die("no revision provided with check_for_term");
>> +     }
>> +
>>       if (!next_all)
>>               usage_with_options(git_bisect_helper_usage, options);
>>
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index 5d1cb00..ea237be 100755
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -564,8 +564,8 @@ write_terms () {
>>       then
>>               die "$(gettext "please use two different terms")"
>>       fi
>> -     check_term_format "$TERM_BAD" bad
>> -     check_term_format "$TERM_GOOD" good
>> +     git bisect--helper --check-term-format="$TERM_BAD" bad
>> +     git bisect--helper --check-term-format="$TERM_GOOD" good
>
> The existing convention is to make the first argument *not* a value of the
> "option", i.e. `--check-term-format "$TERM_BAD"` without an equal sign.

Will change this.

> Did you also run the test suite after compiling, to verify that the
> documented expectations are still met after the conversion?

Yes I did run the tests. They produce the same results as they did before.
To ease review I will next time include these the output of the tests
in the commented section.

t6002-rev-list-bisect.sh : http://paste.ubuntu.com/15473728/
t6030-bisect-porcelain.sh : http://paste.ubuntu.com/15473734/
t6041-bisect-submodule.sh : http://paste.ubuntu.com/15473743/

Is there any other test I would need to run?

Regards,
Pranit Bauva

Re: [PATCH v2] bisect--helper: convert a function in shell to C

[Johannes Schindelin]

Hi Pranit,

On Tue, 22 Mar 2016, Pranit Bauva wrote:

> I did run the tests. They produce the same results as they did before.
> To ease review I will next time include these the output of the tests
> in the commented section.
> 
> t6002-rev-list-bisect.sh : http://paste.ubuntu.com/15473728/
> t6030-bisect-porcelain.sh : http://paste.ubuntu.com/15473734/
> t6041-bisect-submodule.sh : http://paste.ubuntu.com/15473743/
> 
> Is there any other test I would need to run?

Oh, I just meant to point out that you need to make sure that the entire
test suite passes after your patch series (and ideally, after every patch,
that is at least what I frequently test before sending out patch series).

There is not really a need to mention that you ran the test suite. There
is only a need to run it ;-)

Ciao,
Johannes

Re: [PATCH v2] bisect--helper: convert a function in shell to C

[Pranit Bauva]

On Wed, Mar 23, 2016 at 4:52 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Pranit,
>
> On Tue, 22 Mar 2016, Pranit Bauva wrote:
>
>> I did run the tests. They produce the same results as they did before.
>> To ease review I will next time include these the output of the tests
>> in the commented section.
>>
>> t6002-rev-list-bisect.sh : http://paste.ubuntu.com/15473728/
>> t6030-bisect-porcelain.sh : http://paste.ubuntu.com/15473734/
>> t6041-bisect-submodule.sh : http://paste.ubuntu.com/15473743/
>>
>> Is there any other test I would need to run?
>
> Oh, I just meant to point out that you need to make sure that the entire
> test suite passes after your patch series (and ideally, after every patch,
> that is at least what I frequently test before sending out patch series).

Up to now, I used to only run the tests of the concerned area of the
patch. Though from next time, I will take care to run the complete
test suite.

[PATCH v3] bisect--helper: convert a function in shell to C

[Pranit Bauva]

Convert the code literally without changing its design even though it
seems that it is obscure as to the use of comparing revision to different
bisect arguments which seems like a problem in shell because of the way
function arguments are handled.

The argument handling is kind of hard coded right now because it is not
really be meant to be used like this and this is just for testing
purposes whether this new method is as functional as its counter part.
The shell counter part of the method has been retained for historical
purposes.

Also using OPT_CMDMODE() to handle check-term-format and next-all
because these sub commands are independent and error should be shown if used
together and should be handled independently.

This commit reduces the number of failed tests in
t6030-bisect-porcelain.sh and t6041-bisect-submodule.sh

The corresponding shell function is :
https://github.com/git/git/blob/v2.8.0-rc4/git-bisect.sh#L572-L597

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

---
Changes wrt v1:
 - Remove the function declaration
 - Introduce another method one_of() to reduce the clutter in if
 - Add the documentation as to which part should remain untouched
 - Use OPT_CMDMODE() for --check-term-format and --next-all
 - Remove the '=' in git-bisect.sh
 - Respect the coding convention to indent when a line is spread across
   many lines
 - s/its/it is/g
 - Output of tests:
   - t6002 : http://paste.ubuntu.com/15477883/
   - t6030 : http://paste.ubuntu.com/15477887/
   - t6041 : http://paste.ubuntu.com/15477897/
 - Add the comment that a part shouldn't be touched
---
 builtin/bisect--helper.c | 73 ++++++++++++++++++++++++++++++++++++++++++++----
 git-bisect.sh            |  4 +--
 2 files changed, 70 insertions(+), 7 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229..ab3891c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -2,30 +2,93 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "bisect.h"
+#include "refs.h"
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
+	N_("git bisect--helper --check-term-format <term> <revision>"),
 	NULL
 };
 
+static int one_of(const char *term, ...) {
+	va_list matches;
+	const char *match;
+
+	va_start(matches, term);
+	while ((match = va_arg(matches, const char *)) != NULL)
+		if (!strcmp(term, match))
+			return 1;
+
+	va_end(matches);
+
+	return 0;
+}
+
+static int check_term_format(const char *term, const char *revision, int flag) {
+	if (check_refname_format(term, flag))
+		die("'%s' is not a valid term", term);
+
+	if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
+	    "replay", "log", "run", NULL))
+		die("can't use the builtin command '%s' as a term", term);
+
+	/* In theory, nothing prevents swapping
+	 * completely good and bad, but this situation
+	 * could be confusing and hasn't been tested
+	 * enough. Forbid it for now.
+	 */
+
+	if (!strcmp(term, "bad") || !strcmp(term, "new"))
+		if (strcmp(revision, "bad"))
+			die("can't change the meaning of term '%s'", term);
+
+	if(!strcmp(term, "good") || !strcmp(term, "old"))
+		if (strcmp(revision, "good"))
+			die("can't change the meaning of term '%s'", term);
+
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-	int next_all = 0;
+	int sub_command = 0;
 	int no_checkout = 0;
+
+	enum sub_commands {
+		NEXT_ALL,
+		CHECK_TERM_FMT
+	};
+
 	struct option options[] = {
-		OPT_BOOL(0, "next-all", &next_all,
-			 N_("perform 'git bisect next'")),
+		OPT_CMDMODE(0, "next-all", &sub_command,
+			 N_("perform 'git bisect next'"), NEXT_ALL),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
+		OPT_CMDMODE(0, "check-term-format", &sub_command,
+			 N_("check the format of the ref"), CHECK_TERM_FMT),
 		OPT_END()
 	};
 
 	argc = parse_options(argc, argv, prefix, options,
 			     git_bisect_helper_usage, 0);
 
-	if (!next_all)
+	if (sub_command == CHECK_TERM_FMT) {
+		if (argc == 2) {
+			if (argv[0] != NULL && argv[1] != NULL)
+				return check_term_format(argv[0], argv[1], 0);
+			else
+				die("no revision or term provided with check_for_term");
+		}
+		else
+			die("--check-term-format expects 2 arguments");
+	}
+
+	if (sub_command != NEXT_ALL && sub_command != CHECK_TERM_FMT)
 		usage_with_options(git_bisect_helper_usage, options);
 
 	/* next-all */
-	return bisect_next_all(prefix, no_checkout);
+	if (sub_command == NEXT_ALL)
+		return bisect_next_all(prefix, no_checkout);
+
+	return 1;
 }
diff --git a/git-bisect.sh b/git-bisect.sh
index 5d1cb00..f63b83e 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -564,8 +564,8 @@ write_terms () {
 	then
 		die "$(gettext "please use two different terms")"
 	fi
-	check_term_format "$TERM_BAD" bad
-	check_term_format "$TERM_GOOD" good
+	git bisect--helper --check-term-format "$TERM_BAD" bad
+	git bisect--helper --check-term-format "$TERM_GOOD" good
 	printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
 }
 

--
https://github.com/git/git/pull/216

Re: [PATCH v3] bisect--helper: convert a function in shell to C

[Johannes Schindelin]

Hi Pranit,

On Wed, 23 Mar 2016, Pranit Bauva wrote:

> Convert the code literally without changing its design even though it
> seems that it is obscure as to the use of comparing revision to different
> bisect arguments which seems like a problem in shell because of the way
> function arguments are handled.

I still believe that it would make for an improvement to replace
"revision" by "orig_term".

> The argument handling is kind of hard coded right now because it is not
> really be meant to be used like this and this is just for testing
> purposes whether this new method is as functional as its counter part.
> The shell counter part of the method has been retained for historical
> purposes.

Well, maybe the argument handling is really meant to be used like this in
the end? Just skip that paragraph.

> Also using OPT_CMDMODE() to handle check-term-format and next-all
> because these sub commands are independent and error should be shown if
> used together and should be handled independently.

As is often the case, after some discussion a single patch becomes more
than just one change. This is what we see here, too: OPT_CMDMODE() is a
change that needs preparation of the existing code in
builtin/bisect--helper.c, and I would highly suggest to split that change
out into its own patch. It makes for a nicer story, and it is *much*
easier to review.

> This commit reduces the number of failed tests in
> t6030-bisect-porcelain.sh and t6041-bisect-submodule.sh

Oh? Which tests are supposed to fail? I do not see any failing tests
here...

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3324229..ab3891c 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> [...]
> +static int check_term_format(const char *term, const char *revision, int flag) {
> +	if (check_refname_format(term, flag))
> +		die("'%s' is not a valid term", term);
> +
> +	if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
> +	    "replay", "log", "run", NULL))

If I understood Junio correctly, he meant to line up the second line with
the corresponding level. In this case, as "replay" is a parameter of the
one_of() function, the indentation would look like this instead:

	if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
		   "replay", "log", "run", NULL))

> +		die("can't use the builtin command '%s' as a term", term);
> +
> +	/* In theory, nothing prevents swapping
> +	 * completely good and bad, but this situation
> +	 * could be confusing and hasn't been tested
> +	 * enough. Forbid it for now.
> +	 */

I see quite a few comments that put the closing "*/" on its own line, but
do not award the same pleasure to the opening "/*". Personally, I much
prefer the opening "/*" to have an entire line to itself, too, but I guess
that there is enough precendence in Git's source code to accept both
forms.

> +	if (!strcmp(term, "bad") || !strcmp(term, "new"))
> +		if (strcmp(revision, "bad"))
> +			die("can't change the meaning of term '%s'", term);
> +
> +	if(!strcmp(term, "good") || !strcmp(term, "old"))
> +		if (strcmp(revision, "good"))
> +			die("can't change the meaning of term '%s'", term);

I am still convinced that

	if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
	    (one_of(term, "good", "old", NULL) && strcmp(orig_term, "good")))
		die("can't change the meaning of term '%s'", term);

looks so much nicer.

>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
> -	int next_all = 0;
> +	int sub_command = 0;
>  	int no_checkout = 0;
> +
> +	enum sub_commands {
> +		NEXT_ALL,
> +		CHECK_TERM_FMT
> +	};

Interesting. I did not think that Git's source code declares enums inside
functions, but builtin/remote.c's config_read_branches() does, so this
code is fine.

Still, the patch would be easier to review (corollary: bugs would have a
harder time to hide) if the change from OPT_BOOL to OPT_CMDMODE was done
in a separate, preparatory patch.

>  	argc = parse_options(argc, argv, prefix, options,
>  			     git_bisect_helper_usage, 0);
>  
> -	if (!next_all)
> +	if (sub_command == CHECK_TERM_FMT) {
> +		if (argc == 2) {
> +			if (argv[0] != NULL && argv[1] != NULL)
> +				return check_term_format(argv[0], argv[1], 0);
> +			else
> +				die("no revision or term provided with check_for_term");
> +		}
> +		else
> +			die("--check-term-format expects 2 arguments");
> +	}
> +
> +	if (sub_command != NEXT_ALL && sub_command != CHECK_TERM_FMT)
>  		usage_with_options(git_bisect_helper_usage, options);

Personally, I would prefer

- the usage_with_options() code to come before any sub_command processing,

- the sub_command enum to be initialized with -1, or alternatively
  hardcoding NEXT_ALL to 1,

- to avoid else clauses after an if () clause whose block returns or
  die()s,

- to order the clauses of an if/else in ascending size, i.e.

	if (argc != 2)
		die(...);
	if ...

- to avoid checking argv[i] for NULL when i < argc (it is the contract
  that argv[0..argc-1] are all non-NULL, so checking for it is unnecessary
  churn),

- use a switch() on sub_command instead of unrolled if () statements, and

- wrap the code at 80 columns/row (interpreting tabs as "up to 8 spaces").

The rest of the patch looks good.

Ciao,
Johannes

Re: [PATCH v3] bisect--helper: convert a function in shell to C

[Pranit Bauva]

On Wed, Mar 23, 2016 at 5:27 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Pranit,
>
> On Wed, 23 Mar 2016, Pranit Bauva wrote:
>
>> Convert the code literally without changing its design even though it
>> seems that it is obscure as to the use of comparing revision to different
>> bisect arguments which seems like a problem in shell because of the way
>> function arguments are handled.
>
> I still believe that it would make for an improvement to replace
> "revision" by "orig_term".

I missed this. Will do it.

>> The argument handling is kind of hard coded right now because it is not
>> really be meant to be used like this and this is just for testing
>> purposes whether this new method is as functional as its counter part.
>> The shell counter part of the method has been retained for historical
>> purposes.
>
> Well, maybe the argument handling is really meant to be used like this in
> the end? Just skip that paragraph.

Sure.

>> Also using OPT_CMDMODE() to handle check-term-format and next-all
>> because these sub commands are independent and error should be shown if
>> used together and should be handled independently.
>
> As is often the case, after some discussion a single patch becomes more
> than just one change. This is what we see here, too: OPT_CMDMODE() is a
> change that needs preparation of the existing code in
> builtin/bisect--helper.c, and I would highly suggest to split that change
> out into its own patch. It makes for a nicer story, and it is *much*
> easier to review.
>
>> This commit reduces the number of failed tests in
>> t6030-bisect-porcelain.sh and t6041-bisect-submodule.sh
>
> Oh? Which tests are supposed to fail? I do not see any failing tests
> here...

What I meant by this is that before there were 55 out of 70 tests
which failed. After this patch, there are 3 tests out of 70 which
failed.

>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 3324229..ab3891c 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> [...]
>> +static int check_term_format(const char *term, const char *revision, int flag) {
>> +     if (check_refname_format(term, flag))
>> +             die("'%s' is not a valid term", term);
>> +
>> +     if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
>> +         "replay", "log", "run", NULL))
>
> If I understood Junio correctly, he meant to line up the second line with
> the corresponding level. In this case, as "replay" is a parameter of the
> one_of() function, the indentation would look like this instead:

I misunderstood his point.

>         if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
>                    "replay", "log", "run", NULL))
>
>> +             die("can't use the builtin command '%s' as a term", term);
>> +
>> +     /* In theory, nothing prevents swapping
>> +      * completely good and bad, but this situation
>> +      * could be confusing and hasn't been tested
>> +      * enough. Forbid it for now.
>> +      */
>
> I see quite a few comments that put the closing "*/" on its own line, but
> do not award the same pleasure to the opening "/*". Personally, I much
> prefer the opening "/*" to have an entire line to itself, too, but I guess
> that there is enough precendence in Git's source code to accept both
> forms.
>
>> +     if (!strcmp(term, "bad") || !strcmp(term, "new"))
>> +             if (strcmp(revision, "bad"))
>> +                     die("can't change the meaning of term '%s'", term);
>> +
>> +     if(!strcmp(term, "good") || !strcmp(term, "old"))
>> +             if (strcmp(revision, "good"))
>> +                     die("can't change the meaning of term '%s'", term);
>
> I am still convinced that
>
>         if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
>             (one_of(term, "good", "old", NULL) && strcmp(orig_term, "good")))
>                 die("can't change the meaning of term '%s'", term);
>
> looks so much nicer.

True. I missed this point.

>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>  {
>> -     int next_all = 0;
>> +     int sub_command = 0;
>>       int no_checkout = 0;
>> +
>> +     enum sub_commands {
>> +             NEXT_ALL,
>> +             CHECK_TERM_FMT
>> +     };
>
> Interesting. I did not think that Git's source code declares enums inside
> functions, but builtin/remote.c's config_read_branches() does, so this
> code is fine.

I didn't notice this before. Since git has the convention of declaring
enums outside function, it will be better to follow the trend rather
than allowing another trend to spread.

> Still, the patch would be easier to review (corollary: bugs would have a
> harder time to hide) if the change from OPT_BOOL to OPT_CMDMODE was done
> in a separate, preparatory patch.

I was confused about this. Now that u mention it, I will make the change.

>>       argc = parse_options(argc, argv, prefix, options,
>>                            git_bisect_helper_usage, 0);
>>
>> -     if (!next_all)
>> +     if (sub_command == CHECK_TERM_FMT) {
>> +             if (argc == 2) {
>> +                     if (argv[0] != NULL && argv[1] != NULL)
>> +                             return check_term_format(argv[0], argv[1], 0);
>> +                     else
>> +                             die("no revision or term provided with check_for_term");
>> +             }
>> +             else
>> +                     die("--check-term-format expects 2 arguments");
>> +     }
>> +
>> +     if (sub_command != NEXT_ALL && sub_command != CHECK_TERM_FMT)
>>               usage_with_options(git_bisect_helper_usage, options);
>
> Personally, I would prefer
>
> - the usage_with_options() code to come before any sub_command processing,
>
> - the sub_command enum to be initialized with -1, or alternatively
>   hardcoding NEXT_ALL to 1,

Oh! I now understand that initializing with 0 can be problematic.

> - to avoid else clauses after an if () clause whose block returns or
>   die()s,

Sure

> - to order the clauses of an if/else in ascending size, i.e.
>
>         if (argc != 2)
>                 die(...);
>         if ...
>
> - to avoid checking argv[i] for NULL when i < argc (it is the contract
>   that argv[0..argc-1] are all non-NULL, so checking for it is unnecessary
>   churn),

I wasn't aware of this.

> - use a switch() on sub_command instead of unrolled if () statements, and
I just browsed through some other parts and found that subcommands are
put in switch case

> - wrap the code at 80 columns/row (interpreting tabs as "up to 8 spaces").


> The rest of the patch looks good.
>
> Ciao,
> Johannes

Thanks!

Re: [PATCH v3] bisect--helper: convert a function in shell to C

[Junio C Hamano]

Pranit Bauva <pranit.bauva@gmail.com> writes:

> On Wed, Mar 23, 2016 at 5:27 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> Hi Pranit,
>>
>> On Wed, 23 Mar 2016, Pranit Bauva wrote:
>>
>>> Convert the code literally without changing its design even though it
>>> seems that it is obscure as to the use of comparing revision to different
>>> bisect arguments which seems like a problem in shell because of the way
>>> function arguments are handled.

Are you talking about the need to do one_of("help", "start", ...)?

I do not see how that is "problem in shell" or "they way function
arguments are handled".

    git bisect bad
    git bisect good

are the ways how you mark the current commit as bad or good, and
recent change that introduced the "term" thingy allows you to
replace these "bad" and "good" with your own words, but

    git bisect start
    git bisect help

etc. have their own meaning, so you cannot say "I call bad state
'start' and good state 'help'" without confusing yourself.  You'd
never be able to start or get help if you did so, and that does not
have anything to do with "shell" or "function argument" which are
implementation detail.

You cannot fundamentally allow replacing bad/good with these
blacklisted words unless you are going to adopt different command
line syntax (e.g. instead of accepting "git bisect $bad" with a word
chosen by the end user, use "git bisect mark $bad", and $bad can be
any word including "start", "visualize", etc.).

Re: [PATCH v3] bisect--helper: convert a function in shell to C

[Pranit Bauva]

On Wed, Mar 23, 2016 at 9:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> On Wed, Mar 23, 2016 at 5:27 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>> Hi Pranit,
>>>
>>> On Wed, 23 Mar 2016, Pranit Bauva wrote:
>>>
>>>> Convert the code literally without changing its design even though it
>>>> seems that it is obscure as to the use of comparing revision to different
>>>> bisect arguments which seems like a problem in shell because of the way
>>>> function arguments are handled.
>
> Are you talking about the need to do one_of("help", "start", ...)?
>
> I do not see how that is "problem in shell" or "they way function
> arguments are handled".
>
>     git bisect bad
>     git bisect good
>
> are the ways how you mark the current commit as bad or good, and
> recent change that introduced the "term" thingy allows you to
> replace these "bad" and "good" with your own words, but
>
>     git bisect start
>     git bisect help
>
> etc. have their own meaning, so you cannot say "I call bad state
> 'start' and good state 'help'" without confusing yourself.  You'd
> never be able to start or get help if you did so, and that does not
> have anything to do with "shell" or "function argument" which are
> implementation detail.
>
> You cannot fundamentally allow replacing bad/good with these
> blacklisted words unless you are going to adopt different command
> line syntax (e.g. instead of accepting "git bisect $bad" with a word
> chosen by the end user, use "git bisect mark $bad", and $bad can be
> any word including "start", "visualize", etc.).

Seems like I got confused. Thanks for the clarification. :)

Re: [PATCH v3] bisect--helper: convert a function in shell to C

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> +	if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
>> +	    "replay", "log", "run", NULL))
>
> If I understood Junio correctly, he meant to line up the second line with
> the corresponding level. In this case, as "replay" is a parameter of the
> one_of() function, the indentation would look like this instead:
>
> 	if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
> 		   "replay", "log", "run", NULL))

Thanks for clarification.  It may also make sense to wrap the first
line one word earlier.

>> +		die("can't use the builtin command '%s' as a term", term);
>> +
>> +	/* In theory, nothing prevents swapping
>> +	 * completely good and bad, but this situation
>> +	 * could be confusing and hasn't been tested
>> +	 * enough. Forbid it for now.
>> +	 */
>
> I see quite a few comments that put the closing "*/" on its own line, but
> do not award the same pleasure to the opening "/*". Personally, I much
> prefer the opening "/*" to have an entire line to itself, too, but I guess
> that there is enough precendence in Git's source code to accept both
> forms.

We do want to see "/*" and "*/" on their own lines, and new code
should definitely do so.

>> +	if (!strcmp(term, "bad") || !strcmp(term, "new"))
>> +		if (strcmp(revision, "bad"))
>> +			die("can't change the meaning of term '%s'", term);
>> +
>> +	if(!strcmp(term, "good") || !strcmp(term, "old"))
>> +		if (strcmp(revision, "good"))
>> +			die("can't change the meaning of term '%s'", term);
>
> I am still convinced that
>
> 	if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
> 	    (one_of(term, "good", "old", NULL) && strcmp(orig_term, "good")))
> 		die("can't change the meaning of term '%s'", term);
>
> looks so much nicer.

... and more importantly, easier to understand what is going on.

Thanks.

Re: [PATCH v3] bisect--helper: convert a function in shell to C

[Pranit Bauva]

On Wed, Mar 23, 2016 at 9:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> +    if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
>>> +        "replay", "log", "run", NULL))
>>
>> If I understood Junio correctly, he meant to line up the second line with
>> the corresponding level. In this case, as "replay" is a parameter of the
>> one_of() function, the indentation would look like this instead:
>>
>>       if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
>>                  "replay", "log", "run", NULL))
>
> Thanks for clarification.  It may also make sense to wrap the first
> line one word earlier.
>
>>> +            die("can't use the builtin command '%s' as a term", term);
>>> +
>>> +    /* In theory, nothing prevents swapping
>>> +     * completely good and bad, but this situation
>>> +     * could be confusing and hasn't been tested
>>> +     * enough. Forbid it for now.
>>> +     */
>>
>> I see quite a few comments that put the closing "*/" on its own line, but
>> do not award the same pleasure to the opening "/*". Personally, I much
>> prefer the opening "/*" to have an entire line to itself, too, but I guess
>> that there is enough precendence in Git's source code to accept both
>> forms.
>
> We do want to see "/*" and "*/" on their own lines, and new code
> should definitely do so.

I also think it is better to promote one format and try and reduce the
other one.

>>> +    if (!strcmp(term, "bad") || !strcmp(term, "new"))
>>> +            if (strcmp(revision, "bad"))
>>> +                    die("can't change the meaning of term '%s'", term);
>>> +
>>> +    if(!strcmp(term, "good") || !strcmp(term, "old"))
>>> +            if (strcmp(revision, "good"))
>>> +                    die("can't change the meaning of term '%s'", term);
>>
>> I am still convinced that
>>
>>       if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
>>           (one_of(term, "good", "old", NULL) && strcmp(orig_term, "good")))
>>               die("can't change the meaning of term '%s'", term);
>>
>> looks so much nicer.
>
> ... and more importantly, easier to understand what is going on.

I will take care about this next time.

[PATCH 0/2] bisect--helper: rewrite of check_term_format()

[Pranit Bauva]

Important Changes:

 * The previous patch is split into two patches for a better and logical
   commit history.

 * As previously absent, strbuf is used for string manipulation.

 * This passes the complete test suite unlike the previous patches.

 * Proper use of error handling (die() and error() are used at appropriate
   places).

 * Remove the shell code which is then redundant.

 * Improve according to the minor nits given by Johannes Schindelin.

Pranit Bauva (2):
  bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
  bisect: rewrite `check_term_format` shell function in C

 builtin/bisect--helper.c | 77 ++++++++++++++++++++++++++++++++++++++++++++----
 git-bisect.sh            | 31 ++-----------------
 2 files changed, 73 insertions(+), 35 deletions(-)

-- 
2.8.1

[PATCH 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

`--next-all` is meant to be used as a sub command to support multiple
"operation mode" though the current implementation does not contain any
other sub command along side with `--next-all` but further commits will
include some more subcommands.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christain Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229..5f6ef8c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -8,13 +8,17 @@ static const char * const git_bisect_helper_usage[] = {
 	NULL
 };
 
+enum sub_commands {
+	NEXT_ALL = 1
+};
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-	int next_all = 0;
+	int sub_command = 0;
 	int no_checkout = 0;
 	struct option options[] = {
-		OPT_BOOL(0, "next-all", &next_all,
-			 N_("perform 'git bisect next'")),
+		OPT_CMDMODE(0, "next-all", &sub_command,
+			 N_("perform 'git bisect next'"), NEXT_ALL),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -23,9 +27,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_bisect_helper_usage, 0);
 
-	if (!next_all)
+	if (!sub_command)
 		usage_with_options(git_bisect_helper_usage, options);
 
-	/* next-all */
-	return bisect_next_all(prefix, no_checkout);
+	switch (sub_command) {
+	case NEXT_ALL:
+		return bisect_next_all(prefix, no_checkout);
+	default:
+		die(_("bug: unknown subcommand '%d'"), sub_command);
+	}
+	return 0;
 }
-- 
2.8.1

Re: [PATCH 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

On Wed, May 4, 2016 at 10:37 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> `--next-all` is meant to be used as a sub command to support multiple
> "operation mode" though the current implementation does not contain any
> other sub command along side with `--next-all` but further commits will
> include some more subcommands.
>
> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> Mentored-by: Christain Couder <chriscool@tuxfamily.org>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---

This is also a typo. s/Christain/Christian/g

>  builtin/bisect--helper.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3324229..5f6ef8c 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -8,13 +8,17 @@ static const char * const git_bisect_helper_usage[] = {
>         NULL
>  };
>
> +enum sub_commands {
> +       NEXT_ALL = 1
> +};
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
> -       int next_all = 0;
> +       int sub_command = 0;
>         int no_checkout = 0;
>         struct option options[] = {
> -               OPT_BOOL(0, "next-all", &next_all,
> -                        N_("perform 'git bisect next'")),
> +               OPT_CMDMODE(0, "next-all", &sub_command,
> +                        N_("perform 'git bisect next'"), NEXT_ALL),
>                 OPT_BOOL(0, "no-checkout", &no_checkout,
>                          N_("update BISECT_HEAD instead of checking out the current commit")),
>                 OPT_END()
> @@ -23,9 +27,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>         argc = parse_options(argc, argv, prefix, options,
>                              git_bisect_helper_usage, 0);
>
> -       if (!next_all)
> +       if (!sub_command)
>                 usage_with_options(git_bisect_helper_usage, options);
>
> -       /* next-all */
> -       return bisect_next_all(prefix, no_checkout);
> +       switch (sub_command) {
> +       case NEXT_ALL:
> +               return bisect_next_all(prefix, no_checkout);
> +       default:
> +               die(_("bug: unknown subcommand '%d'"), sub_command);
> +       }
> +       return 0;
>  }
> --
> 2.8.1
>

Re: [PATCH 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Eric Sunshine]

On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

This subject is too low-level, talking about implementation details,
whereas it should be giving a high-level summary of the change.

> `--next-all` is meant to be used as a sub command to support multiple
> "operation mode" though the current implementation does not contain any
> other sub command along side with `--next-all` but further commits will
> include some more subcommands.

You've spelled this as both "sub command" and "subcommand". Choose one
and stick with it. ("subcommand" is probably more common.)

> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -8,13 +8,17 @@ static const char * const git_bisect_helper_usage[] = {
>         NULL
>  };
>
> +enum sub_commands {

How about calling this 'enum subcommand' (no underscore, non-plural)?

> +       NEXT_ALL = 1
> +};
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
> -       int next_all = 0;
> +       int sub_command = 0;

s/sub_command/subcommand/

>         struct option options[] = {
> -               OPT_BOOL(0, "next-all", &next_all,
> -                        N_("perform 'git bisect next'")),
> +               OPT_CMDMODE(0, "next-all", &sub_command,
> +                        N_("perform 'git bisect next'"), NEXT_ALL),
> @@ -23,9 +27,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>         argc = parse_options(argc, argv, prefix, options,
>                              git_bisect_helper_usage, 0);
>
> -       if (!next_all)
> +       if (!sub_command)
>                 usage_with_options(git_bisect_helper_usage, options);
>
> -       /* next-all */
> -       return bisect_next_all(prefix, no_checkout);
> +       switch (sub_command) {
> +       case NEXT_ALL:
> +               return bisect_next_all(prefix, no_checkout);
> +       default:
> +               die(_("bug: unknown subcommand '%d'"), sub_command);

s/bug/BUG/

Also, as this is a programmer error rather than an end-user error, it
does not need to be localized, thus drop the _(...).

> +       }
> +       return 0;
>  }

Re: [PATCH 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Christian Couder]

On Wed, May 4, 2016 at 8:07 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
>
> This subject is too low-level, talking about implementation details,
> whereas it should be giving a high-level summary of the change.

When a patch is all about a low level detail, I think it is good to
talk about the detail in the subject.

Here I think something like "bisect--helper: parse options using
OPT_CMDMODE instead of OPT_BOOL" would be ok for example.

>> `--next-all` is meant to be used as a sub command to support multiple
>> "operation mode" though the current implementation does not contain any
>> other sub command along side with `--next-all` but further commits will
>> include some more subcommands.
>
> You've spelled this as both "sub command" and "subcommand". Choose one
> and stick with it. ("subcommand" is probably more common.)

I agree with that and the other suggestions, thanks.

Re: [PATCH 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Johannes Schindelin]

Hi Christian,

On Wed, 4 May 2016, Christian Couder wrote:

> On Wed, May 4, 2016 at 8:07 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> >> bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
> >
> > This subject is too low-level, talking about implementation details,
> > whereas it should be giving a high-level summary of the change.
> 
> When a patch is all about a low level detail, I think it is good to
> talk about the detail in the subject.

Well, this is not the case here. The intent of this commit is to prepare
for other command modes.

So... why not just say "bisect--helper: prepare for modes other than
'next-all'"?

Ciao,
Dscho

Re: [PATCH 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

On Wed, May 4, 2016 at 4:35 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Christian,
>
> On Wed, 4 May 2016, Christian Couder wrote:
>
>> On Wed, May 4, 2016 at 8:07 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> > On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> >> bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
>> >
>> > This subject is too low-level, talking about implementation details,
>> > whereas it should be giving a high-level summary of the change.
>>
>> When a patch is all about a low level detail, I think it is good to
>> talk about the detail in the subject.
>
> Well, this is not the case here. The intent of this commit is to prepare
> for other command modes.
>
> So... why not just say "bisect--helper: prepare for modes other than
> 'next-all'"?

Sounds nice!

> Ciao,
> Dscho

Thanks,
Pranit Bauva

Re: [PATCH 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Christian Couder]

On Wed, May 4, 2016 at 1:05 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Christian,
>
> On Wed, 4 May 2016, Christian Couder wrote:
>
>> On Wed, May 4, 2016 at 8:07 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> > On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> >> bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
>> >
>> > This subject is too low-level, talking about implementation details,
>> > whereas it should be giving a high-level summary of the change.
>>
>> When a patch is all about a low level detail, I think it is good to
>> talk about the detail in the subject.
>
> Well, this is not the case here. The intent of this commit is to prepare
> for other command modes.

It depends if you think that describing the long term intent of a
commit is more important than just describing what the commit actually
does in the subject.
In my opinion the long term intent of the commit is better explained
in the commit message. (And the patch indeed talks about what the long
term intent is, even if it could probably be improved.)

> So... why not just say "bisect--helper: prepare for modes other than
> 'next-all'"?

For (an extreme) example, in my patch series about libifying "git
apply" functionality, should many of the patches have subjects like
"builtin/apply: prepare for an apply.{c,h} lib"?

Re: [PATCH 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Johannes Schindelin]

Hi Chris,

On Wed, 4 May 2016, Christian Couder wrote:

> On Wed, May 4, 2016 at 1:05 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> 
> > So... why not just say "bisect--helper: prepare for modes other than
> > 'next-all'"?
> 
> For (an extreme) example, in my patch series about libifying "git apply"
> functionality, should many of the patches have subjects like
> "builtin/apply: prepare for an apply.{c,h} lib"?

You are deliberately misunderstanding me.

If your patch series contained *one* patch whose intent was to prepare for
a libified 'apply', yes, indeed, I would think that it would make for a
fine commit subject. Especially if the other patches tried to do
completely unrelated things. Then your "low-level" change would really
benefit from a "high-level" commit message: the "low-level" aspect is seen
easily enough in the patch itself, thankyouverymuch.

It really misrepresents my comment to pretend that I had tried to
suggest something as utterly confusing as identically repeated commit
subjects.

Ciao,
Johannes

Re: [PATCH 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Christian Couder]

Hi Dscho,

On Wed, May 4, 2016 at 2:21 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> If your patch series contained *one* patch whose intent was to prepare for
> a libified 'apply', yes, indeed, I would think that it would make for a
> fine commit subject. Especially if the other patches tried to do
> completely unrelated things. Then your "low-level" change would really
> benefit from a "high-level" commit message: the "low-level" aspect is seen
> easily enough in the patch itself, thankyouverymuch.

One can also say that if the long term intent is described in the
commit message and if the commit message is already "high level", then
a "high level" subject on top of it doesn't provide any more benefit.
A "high level" subject on top can just make it harder to relate the
patch, when you only see its subject, to what it contains.

> It really misrepresents my comment to pretend that I had tried to
> suggest something as utterly confusing as identically repeated commit
> subjects.

Sorry but that was not my intent to suggest that. My intent was to try
to show that there is some important value to make the subject close
to the "low level" thing the patch actually does.

Thanks,
Christian.

Re: [PATCH 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Johannes Schindelin]

Hi Christian,

On Wed, 4 May 2016, Christian Couder wrote:

> My intent was to try to show that there is some important value to make
> the subject close to the "low level" thing the patch actually does.

I disagree. The place to describe low-level details that are not
immediately obvious from the patch is the commit message. Way, way down on
the bottom.

The commit message should start with a subject that gives me a good clue
why this is a good change. A low-level detail won't do that very often.

Ciao,
Johannes

Re: [PATCH 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Christian Couder]

Hi Dscho,

On Wed, May 4, 2016 at 4:56 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Christian,
>
> On Wed, 4 May 2016, Christian Couder wrote:
>
>> My intent was to try to show that there is some important value to make
>> the subject close to the "low level" thing the patch actually does.
>
> I disagree. The place to describe low-level details that are not
> immediately obvious from the patch is the commit message. Way, way down on
> the bottom.
>
> The commit message should start with a subject that gives me a good clue
> why this is a good change. A low-level detail won't do that very often.

Let me give you another example.

You want to do X and you discover that you need to tweek knob foo to
do X, and also that tweeking knob foo is a good thing to do in general
(for example it could be a refactoring that save some lines of code).
So you create a patch that tweeks knob foo and call it "prepare to do
X", and then send it so that it can be merged. Then you want to do X
but you are interupted by an event, for example you boss tells you to
do Y instead of X right away because of an urgent business need. So
you do Y instead of X. And then sometimes later (it could be weeks,
months or years later), when you are finished with Y, you now want to
go back to your previous work on X. At that time you discover that you
need to tweek knob bar (it could be another refactoring of another
part of the code), but you forgot about your previous patch that
tweeked knob foo, so you call your new patch that tweeks knob bar
"prepare to do X". As your previous patch that was also called
"prepare to do X" has already been integrated, you now have too
patches that do different things that are called the same, and you can
easily mistake the first one for the second one.

If you had just called your first patch "tweek knob foo" and the
second patch "tweek knob bar", it would be much clearer which patch
does what.

Re: [PATCH 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Johannes Schindelin]

Hi Chris,

On Wed, 4 May 2016, Christian Couder wrote:

> On Wed, May 4, 2016 at 4:56 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Wed, 4 May 2016, Christian Couder wrote:
> >
> >> My intent was to try to show that there is some important value to make
> >> the subject close to the "low level" thing the patch actually does.
> >
> > I disagree. The place to describe low-level details that are not
> > immediately obvious from the patch is the commit message. Way, way down on
> > the bottom.
> >
> > The commit message should start with a subject that gives me a good clue
> > why this is a good change. A low-level detail won't do that very often.
> 
> Let me give you another example.
> 
> You want to do X and you discover that you need to tweek knob foo to
> do X, and also that tweeking knob foo is a good thing to do in general
> (for example it could be a refactoring that save some lines of code).
> So you create a patch that tweeks knob foo and call it "prepare to do
> X", and then send it so that it can be merged. Then you want to do X
> but you are interupted by an event, for example you boss tells you to
> do Y instead of X right away because of an urgent business need. So
> you do Y instead of X. And then sometimes later (it could be weeks,
> months or years later), when you are finished with Y, you now want to
> go back to your previous work on X. At that time you discover that you
> need to tweek knob bar (it could be another refactoring of another
> part of the code), but you forgot about your previous patch that
> tweeked knob foo, so you call your new patch that tweeks knob bar
> "prepare to do X". As your previous patch that was also called
> "prepare to do X" has already been integrated, you now have too
> patches that do different things that are called the same, and you can
> easily mistake the first one for the second one.
> 
> If you had just called your first patch "tweek knob foo" and the
> second patch "tweek knob bar", it would be much clearer which patch
> does what.

And then, six months later, when *you* are stuck in a plane and bisect a
problem down to my "tweak knob foo", you curse at me, asking fellow
passengers "what was Dscho thinking? 'foo'? What the hell is 'foo'
supposed to mean? And what was the *intention* of this patch? There is a
bug! And now I cannot fix it properly because I do not understand what the
patch was intended to do!".

Do not believe even one second this scenario s foreign to *me*. Even with
some of git.git's commits whose messages were so in love with low-level
details, essentially repeating what the diff already told me, that they
forgot to include the context, the big picture, leaving me puzzled and
frustrated.

And my fellow passengers, too.

This puzzlement and frustration is unnecessary. I typically follow Peff's
examples. They typically do a good job of telling me right away what the
big picture is, even if they are in the middle of a patch series, not
worrying too much whether adjacent commits say related things, without
forgetting low-level details. The crucial point is that they start out by
assuming little familiarity of the reader with the context.

In short: you can have both. The commit message *can* be informative, and
*still* contain low-level details. You simply start with the former and
ease the reader into the latter, instead of skipping the first part, is
all.

Ciao,
Johannes

Re: [PATCH 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +enum sub_commands {
>
> How about calling this 'enum subcommand' (no underscore, non-plural)?
>
>> +       NEXT_ALL = 1
>> +};
>> +
>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>  {
>> -       int next_all = 0;
>> +       int sub_command = 0;
>
> s/sub_command/subcommand/

Non-plural is a good suggestion, but remember that you are using
OPT_CMDMODE to parse out that thing.  There is already a name for
that concept: "command name".  Why not name these after that phrase,
just like merge-base, replace, and tag already do?

Re: [PATCH 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Johannes Schindelin]

Hi Pranit,

On Wed, 4 May 2016, Pranit Bauva wrote:

> `--next-all` is meant to be used as a sub command to support multiple
> "operation mode" though the current implementation does not contain any
> other sub command along side with `--next-all` but further commits will
> include some more subcommands.

That is a good explanation.

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3324229..5f6ef8c 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -8,13 +8,17 @@ static const char * const git_bisect_helper_usage[] = {
>  	NULL
>  };
>  
> +enum sub_commands {

Instead of "sub_commands" and ...

> +	NEXT_ALL = 1
> +};
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
> -	int next_all = 0;
> +	int sub_command = 0;

... sub_command, seeing as you convert things into ...

>  	int no_checkout = 0;
>  	struct option options[] = {
> -		OPT_BOOL(0, "next-all", &next_all,
> -			 N_("perform 'git bisect next'")),
> +		OPT_CMDMODE(0, "next-all", &sub_command,
> +			 N_("perform 'git bisect next'"), NEXT_ALL),

... using CMDMODE, how about using `enum mode` and `int mode`? And
actually `enum mode mode` instead of `int mode`. Or...

Actually, it could be even more concise by writing

	enum { NEXT_ALL = 1 } mode = 0;

There is not really a need for that enum to be in a different scope than
cmd_bisect__helper(), and neither does it need to be named...

Ciao,
Dscho

[PATCH 2/2] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

This reimplements the `check_term_format` shell function in C and adds
a `--check-term-format` subcommand to `git bisect--helper` to call it
from git-bisect.sh

Helped-by: Johannes Schindelein <Johannes.Schindelein@gmx.de>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 31 ++------------------------
 2 files changed, 59 insertions(+), 30 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 5f6ef8c..228920f 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -2,16 +2,66 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "bisect.h"
+#include "refs.h"
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
+	N_("git bisect--helper --check-term-format <term> <orig_term>"),
 	NULL
 };
 
 enum sub_commands {
-	NEXT_ALL = 1
+	NEXT_ALL = 1,
+	CHECK_TERM_FMT
 };
 
+/*
+ * To check whether the string `term` belongs to the set of strings
+ * included in the variable arguments so as to make the code look
+ * clean and avoid having long lines in the `if` statement.
+ */
+static int one_of(const char *term, ...)
+{
+	va_list matches;
+	const char *match;
+
+	va_start(matches, term);
+	while ((match = va_arg(matches, const char *)) != NULL)
+		if (!strcmp(term, match))
+			return 1;
+
+	va_end(matches);
+
+	return 0;
+}
+
+static int check_term_format(const char *term, const char *orig_term,
+			     int flag)
+{
+	struct strbuf new_term = STRBUF_INIT;
+	strbuf_addf(&new_term, "refs/bisect/%s", term);
+
+	if (check_refname_format(new_term.buf, flag))
+		die(_("'%s' is not a valid term\n"), term);
+
+	else if (one_of(term, "help", "start", "skip", "next", "reset",
+			"visualize", "replay", "log", "run", NULL))
+		return error("can't use the builtin command '%s' as a term\n", term);
+
+	/*
+	 * In theory, nothing prevents swapping
+	 * completely good and bad, but this situation
+	 * could be confusing and hasn't been tested
+	 * enough. Forbid it for now.
+	 */
+
+	else if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
+		 (one_of(term, "good", "old", NULL) && strcmp(orig_term, "good")))
+		return error("can't change the meaning of the term '%s'\n", term);
+
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	int sub_command = 0;
@@ -19,6 +69,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &sub_command,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
+		OPT_CMDMODE(0, "check-term-format", &sub_command,
+			 N_("check format of the ref"), CHECK_TERM_FMT),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -33,6 +85,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	switch (sub_command) {
 	case NEXT_ALL:
 		return bisect_next_all(prefix, no_checkout);
+	case CHECK_TERM_FMT:
+		if (argc != 2)
+			die(_("--check-term-format should be followed by exactly 2 arguments."));
+		return check_term_format(argv[0], argv[1], 0);
 	default:
 		die(_("bug: unknown subcommand '%d'"), sub_command);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 5d1cb00..7d7965d 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -564,38 +564,11 @@ write_terms () {
 	then
 		die "$(gettext "please use two different terms")"
 	fi
-	check_term_format "$TERM_BAD" bad
-	check_term_format "$TERM_GOOD" good
+	git bisect--helper --check-term-format "$TERM_BAD" bad || exit
+	git bisect--helper --check-term-format "$TERM_GOOD" good || exit
 	printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
 }
 
-check_term_format () {
-	term=$1
-	git check-ref-format refs/bisect/"$term" ||
-	die "$(eval_gettext "'\$term' is not a valid term")"
-	case "$term" in
-	help|start|terms|skip|next|reset|visualize|replay|log|run)
-		die "$(eval_gettext "can't use the builtin command '\$term' as a term")"
-		;;
-	bad|new)
-		if test "$2" != bad
-		then
-			# In theory, nothing prevents swapping
-			# completely good and bad, but this situation
-			# could be confusing and hasn't been tested
-			# enough. Forbid it for now.
-			die "$(eval_gettext "can't change the meaning of term '\$term'")"
-		fi
-		;;
-	good|old)
-		if test "$2" != good
-		then
-			die "$(eval_gettext "can't change the meaning of term '\$term'")"
-		fi
-		;;
-	esac
-}
-
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
-- 
2.8.1

Re: [PATCH 2/2] bisect: rewrite `check_term_format` shell function in C

[Eric Sunshine]

On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> This reimplements the `check_term_format` shell function in C and adds

s/This reimplements/Reimplement/
s/adds/add/

> a `--check-term-format` subcommand to `git bisect--helper` to call it
> from git-bisect.sh

s/$/./

Okay, I'll bite: Why is this a good idea? What does it buy you?

It's not as if the rewrite is especially faster or more easily
expressed in C; quite the contrary, the shell code is more concise and
probably about equally as fast (not that execution speed matters in
this case).

I could understand this functionality being ported to C in the form of
a static function as a minor part of porting "git bisect terms" in its
entirety to C, but I'm not imaginative enough to see why this
functionality is useful as a standalone git-bisect--helper subcommand,
and the commit message doesn't enlighten. Consequently, it seems like
unnecessary complexity.

> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -2,16 +2,66 @@
>  static const char * const git_bisect_helper_usage[] = {
>         N_("git bisect--helper --next-all [--no-checkout]"),
> +       N_("git bisect--helper --check-term-format <term> <orig_term>"),

Could this be shortened to --check-term or would that be undesirable?

>         NULL
>  };
>
>  enum sub_commands {
> -       NEXT_ALL = 1
> +       NEXT_ALL = 1,
> +       CHECK_TERM_FMT
>  };
>
> +/*
> + * To check whether the string `term` belongs to the set of strings
> + * included in the variable arguments so as to make the code look
> + * clean and avoid having long lines in the `if` statement.
> + */

Is this a (long) sentence fragment? Code cleanliness is an obviously
desirable trait, thus talking about it in the comment adds no value;
it's just noise.

> +static int one_of(const char *term, ...)
> +{
> +       va_list matches;
> +       const char *match;
> +
> +       va_start(matches, term);
> +       while ((match = va_arg(matches, const char *)) != NULL)
> +               if (!strcmp(term, match))
> +                       return 1;

Is it wise to return here without invoking va_end()?

> +       va_end(matches);
> +
> +       return 0;
> +}
> +
> +static int check_term_format(const char *term, const char *orig_term,
> +                            int flag)

What is 'flag' for? The single caller only ever passes 0, so why is this needed?

> +{
> +       struct strbuf new_term = STRBUF_INIT;

'new_term' is being leaked at every 'return' statement in this function.

> +       strbuf_addf(&new_term, "refs/bisect/%s", term);
> +
> +       if (check_refname_format(new_term.buf, flag))
> +               die(_("'%s' is not a valid term\n"), term);

Why does this die() while the other "invalid" cases merely return
error()? What makes this special?

Also, drop "\n" from the error string.

> +       else if (one_of(term, "help", "start", "skip", "next", "reset",

s/else //

> +                       "visualize", "replay", "log", "run", NULL))
> +               return error("can't use the builtin command '%s' as a term\n", term);

This should be wrapped in _(...). Also, drop the "\n".

> +       /*
> +        * In theory, nothing prevents swapping
> +        * completely good and bad, but this situation
> +        * could be confusing and hasn't been tested
> +        * enough. Forbid it for now.
> +        */

This would be a bit easier to read if re-wrapped to fit within 80
columns rather than 53 or so.

> +       else if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||

s/else //

> +                (one_of(term, "good", "old", NULL) && strcmp(orig_term, "good")))

This can be more efficient by doing the strcmp() before the expensive one_of():

    if ((strcmp(...) && one_of(...)) ||
        strcmp(...) && one_of(...)))

> +               return error("can't change the meaning of the term '%s'\n", term);

This should be wrapped in _(...). Also, drop the "\n".

> +       return 0;
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>         int sub_command = 0;
> @@ -19,6 +69,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>         struct option options[] = {
>                 OPT_CMDMODE(0, "next-all", &sub_command,
>                          N_("perform 'git bisect next'"), NEXT_ALL),
> +               OPT_CMDMODE(0, "check-term-format", &sub_command,
> +                        N_("check format of the ref"), CHECK_TERM_FMT),

What "ref"?

>                 OPT_BOOL(0, "no-checkout", &no_checkout,
>                          N_("update BISECT_HEAD instead of checking out the current commit")),
>                 OPT_END()
> @@ -33,6 +85,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>         switch (sub_command) {
>         case NEXT_ALL:
>                 return bisect_next_all(prefix, no_checkout);
> +       case CHECK_TERM_FMT:
> +               if (argc != 2)
> +                       die(_("--check-term-format should be followed by exactly 2 arguments."));

Drop the period. Possible reword:

    --check-term-format requires two arguments

> +               return check_term_format(argv[0], argv[1], 0);
>         default:
>                 die(_("bug: unknown subcommand '%d'"), sub_command);
>         }

Re: [PATCH 2/2] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

On Wed, May 4, 2016 at 12:22 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> This reimplements the `check_term_format` shell function in C and adds
>
> s/This reimplements/Reimplement/
> s/adds/add/
>
>> a `--check-term-format` subcommand to `git bisect--helper` to call it
>> from git-bisect.sh
>
> s/$/./

Sure.

> Okay, I'll bite: Why is this a good idea? What does it buy you?
>
> It's not as if the rewrite is especially faster or more easily
> expressed in C; quite the contrary, the shell code is more concise and
> probably about equally as fast (not that execution speed matters in
> this case).
>
> I could understand this functionality being ported to C in the form of
> a static function as a minor part of porting "git bisect terms" in its
> entirety to C, but I'm not imaginative enough to see why this
> functionality is useful as a standalone git-bisect--helper subcommand,
> and the commit message doesn't enlighten. Consequently, it seems like
> unnecessary complexity.

It is important to understand that the subcommand is just a
**temporary** measure.
Yes, I agree that making it a subcommand increases unnecessary
complexity. As a part of complete rewrite of git-bisect.sh, I am
converting one function individually to C. The functionality of
subcommand is useful so that I can use the existing test suite to
verify whether I have done the conversion properly. As more functions
get ported (which I intend to finish this summers), previously
existing subcommands will be removed. For eg. After this patch, I will
now convert the function write_terms(). So in that patch, I will
remove the subcommand for check-term-format and instead use the
check_term_format() method and then introduce a new subcommand for
write_terms(). Verifying the function conversion was suggested by
Stefan Beller[1] and Christian Couder[2] gave a hint of how to go
about with using the existing test suite. As for the current
situation, git-bisect.sh calls `--next-all` in a similar way which was
the hint for me of how to go about with this project.

>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -2,16 +2,66 @@
>>  static const char * const git_bisect_helper_usage[] = {
>>         N_("git bisect--helper --next-all [--no-checkout]"),
>> +       N_("git bisect--helper --check-term-format <term> <orig_term>"),
>
> Could this be shortened to --check-term or would that be undesirable?

I guess --check-term-format would be more appropriate and as this is
temporary, it wouldn't matter afterwards.

>>         NULL
>>  };
>>
>>  enum sub_commands {
>> -       NEXT_ALL = 1
>> +       NEXT_ALL = 1,
>> +       CHECK_TERM_FMT
>>  };
>>
>> +/*
>> + * To check whether the string `term` belongs to the set of strings
>> + * included in the variable arguments so as to make the code look
>> + * clean and avoid having long lines in the `if` statement.
>> + */
>
> Is this a (long) sentence fragment? Code cleanliness is an obviously
> desirable trait, thus talking about it in the comment adds no value;
> it's just noise.

I will keep the initial part of the comment.

>> +static int one_of(const char *term, ...)
>> +{
>> +       va_list matches;
>> +       const char *match;
>> +
>> +       va_start(matches, term);
>> +       while ((match = va_arg(matches, const char *)) != NULL)
>> +               if (!strcmp(term, match))
>> +                       return 1;
>
> Is it wise to return here without invoking va_end()?

I guess since it already checks for NULL, invoking va_end() will make
it redundant[3].

>> +       va_end(matches);
>> +
>> +       return 0;
>> +}
>> +
>> +static int check_term_format(const char *term, const char *orig_term,
>> +                            int flag)
>
> What is 'flag' for? The single caller only ever passes 0, so why is this needed?

Well, currently the subcommand does not use this flag but this flag is
present in the method check_refname_format() so it would be better to
use it. This flag might be useful in further parts of conversion since
as I previously mentioned check-term-format isn't a permanent
solution.

>> +{
>> +       struct strbuf new_term = STRBUF_INIT;
>
> 'new_term' is being leaked at every 'return' statement in this function.

I will have to free this memory.

>
>> +       strbuf_addf(&new_term, "refs/bisect/%s", term);
>> +
>> +       if (check_refname_format(new_term.buf, flag))
>> +               die(_("'%s' is not a valid term\n"), term);
>
> Why does this die() while the other "invalid" cases merely return
> error()? What makes this special?

This is because I felt that if check_refname_format() fails then its a
fatal error while in other cases, it is not as fatal.

> Also, drop "\n" from the error string.

Sure!

>> +       else if (one_of(term, "help", "start", "skip", "next", "reset",
>
> s/else //

Agree since it would be a part of the switch which is not included
with the check_refname_format().

>> +                       "visualize", "replay", "log", "run", NULL))
>> +               return error("can't use the builtin command '%s' as a term\n", term);
>
> This should be wrapped in _(...). Also, drop the "\n".
>
>> +       /*
>> +        * In theory, nothing prevents swapping
>> +        * completely good and bad, but this situation
>> +        * could be confusing and hasn't been tested
>> +        * enough. Forbid it for now.
>> +        */
>
> This would be a bit easier to read if re-wrapped to fit within 80
> columns rather than 53 or so.
>
>> +       else if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
>
> s/else //

In the shell script a switch was used, thus `else if` would be a more
appropriate choice over `if`. Also if the first if statement fails
then it is unnecessary to go further.

>
>> +                (one_of(term, "good", "old", NULL) && strcmp(orig_term, "good")))
>
> This can be more efficient by doing the strcmp() before the expensive one_of():
>
>     if ((strcmp(...) && one_of(...)) ||
>         strcmp(...) && one_of(...)))

Nice. Will include this.

>> +               return error("can't change the meaning of the term '%s'\n", term);
>
> This should be wrapped in _(...). Also, drop the "\n".

Sure

>
>> +       return 0;
>> +}
>> +
>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>  {
>>         int sub_command = 0;
>> @@ -19,6 +69,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>         struct option options[] = {
>>                 OPT_CMDMODE(0, "next-all", &sub_command,
>>                          N_("perform 'git bisect next'"), NEXT_ALL),
>> +               OPT_CMDMODE(0, "check-term-format", &sub_command,
>> +                        N_("check format of the ref"), CHECK_TERM_FMT),
>
> What "ref"?

The ref here means that ref (like HEAD).


>>                 OPT_BOOL(0, "no-checkout", &no_checkout,
>>                          N_("update BISECT_HEAD instead of checking out the current commit")),
>>                 OPT_END()
>> @@ -33,6 +85,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>         switch (sub_command) {
>>         case NEXT_ALL:
>>                 return bisect_next_all(prefix, no_checkout);
>> +       case CHECK_TERM_FMT:
>> +               if (argc != 2)
>> +                       die(_("--check-term-format should be followed by exactly 2 arguments."));
>
> Drop the period. Possible reword:
>
>     --check-term-format requires two arguments

Seems better

>> +               return check_term_format(argv[0], argv[1], 0);
>>         default:
>>                 die(_("bug: unknown subcommand '%d'"), sub_command);
>>         }

[1]: http://article.gmane.org/gmane.comp.version-control.git/293489

[2]: http://article.gmane.org/gmane.comp.version-control.git/293489

[3]: http://article.gmane.org/gmane.comp.version-control.git/293489

Re: [PATCH 2/2] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

On Wed, May 4, 2016 at 1:06 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:

>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/293489
>
> [2]: http://article.gmane.org/gmane.comp.version-control.git/293489
>
> [3]: http://article.gmane.org/gmane.comp.version-control.git/293489

These are incorrect links.

The correct ones are:
[1]: http://article.gmane.org/gmane.comp.version-control.git/289477

[2]: http://article.gmane.org/gmane.comp.version-control.git/289487

[3]: http://article.gmane.org/gmane.comp.version-control.git/289511

Re: [PATCH 2/2] bisect: rewrite `check_term_format` shell function in C

[Eric Sunshine]

On Wed, May 4, 2016 at 3:36 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Wed, May 4, 2016 at 12:22 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Okay, I'll bite: Why is this a good idea? What does it buy you?
>>
>> It's not as if the rewrite is especially faster or more easily
>> expressed in C; quite the contrary, the shell code is more concise and
>> probably about equally as fast (not that execution speed matters in
>> this case).
>>
>> I could understand this functionality being ported to C in the form of
>> a static function as a minor part of porting "git bisect terms" in its
>> entirety to C, but I'm not imaginative enough to see why this
>> functionality is useful as a standalone git-bisect--helper subcommand,
>> and the commit message doesn't enlighten. Consequently, it seems like
>> unnecessary complexity.
>
> It is important to understand that the subcommand is just a
> **temporary** measure.

The commit message seems to be lacking this information and any other rationale.

> Yes, I agree that making it a subcommand increases unnecessary
> complexity. As a part of complete rewrite of git-bisect.sh, I am
> converting one function individually to C. The functionality of
> subcommand is useful so that I can use the existing test suite to
> verify whether I have done the conversion properly. As more functions
> get ported (which I intend to finish this summers), previously
> existing subcommands will be removed. For eg. After this patch, I will
> now convert the function write_terms(). So in that patch, I will
> remove the subcommand for check-term-format and instead use the
> check_term_format() method and then introduce a new subcommand for
> write_terms(). Verifying the function conversion was suggested by
> Stefan Beller[1] and Christian Couder[2] gave a hint of how to go
> about with using the existing test suite. As for the current
> situation, git-bisect.sh calls `--next-all` in a similar way which was
> the hint for me of how to go about with this project.

You're taking an inverted bottom-up approach which repeatedly adds and
removes unnecessary complexity rather than a more straight-forward
top-down approach. For instance, with a top-down approach, as a first
step, you could instead add a skeleton, do-nothing "git-bisect--helper
set-terms" and flesh it out in subsequent patches until fully
implemented, at which point drop all the "terms" code from
git-bisect.sh and have it invoke the helper instead. You get the same
benefit of being able to use the existing test suite without the
unnecessary complexity.

In fact, git-bisect.sh could start invoking "git-bisect--helper"
before it's fully fleshed out. For instance, a partially fleshed out C
write_terms(), might just verify that the two terms are not the same
and then write them out to BISECT_TERMS, and the shell script would
invoke its own check_term_format() before calling the helper.

>>> +static int one_of(const char *term, ...)
>>> +{
>>> +       va_list matches;
>>> +       const char *match;
>>> +
>>> +       va_start(matches, term);
>>> +       while ((match = va_arg(matches, const char *)) != NULL)
>>> +               if (!strcmp(term, match))
>>> +                       return 1;
>>
>> Is it wise to return here without invoking va_end()?
>
> I guess since it already checks for NULL, invoking va_end() will make
> it redundant[3].

Sorry, your response does not compute. Each va_start() *must* be
balanced with a va_end(). (While it's true that you may encounters
platforms/compilers for which a missing va_end() does no harm, such
code is not portable.)

>>> +       va_end(matches);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int check_term_format(const char *term, const char *orig_term,
>>> +                            int flag)
>>
>> What is 'flag' for? The single caller only ever passes 0, so why is this needed?
>
> Well, currently the subcommand does not use this flag but this flag is
> present in the method check_refname_format() so it would be better to
> use it. This flag might be useful in further parts of conversion since
> as I previously mentioned check-term-format isn't a permanent
> solution.

Sorry, again this does not compute. Certainly, you must pass *some*
flags argument to check_refname_format() as 'flags' is part of its
signature, but that doesn't explain why check_term_format() accepts a
'flag' argument. Moreover, check_term_format() is not a general
purpose function like check_refname_format(), so this sort of
*apparent* flexibility adds complexity with no obvious benefit.

>>> +       strbuf_addf(&new_term, "refs/bisect/%s", term);
>>> +
>>> +       if (check_refname_format(new_term.buf, flag))
>>> +               die(_("'%s' is not a valid term\n"), term);
>>
>> Why does this die() while the other "invalid" cases merely return
>> error()? What makes this special?
>
> This is because I felt that if check_refname_format() fails then its a
> fatal error while in other cases, it is not as fatal.

The name of the command is "check-term-format" and that is precisely
its purpose so, from the perspective of the caller, *all* problems
with the term are fatal. It's black-and-white, there is no grey:
either a term is acceptable, or it isn't; that's all the caller wants
to know. Consequently, all problems detected by this function should
be reported the same way (preferably via 'return error()').

>>> +       else if (one_of(term, "help", "start", "skip", "next", "reset",
>>
>> s/else //
>
> Agree since it would be a part of the switch which is not included
> with the check_refname_format().
>
>>> +       else if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
>>
>> s/else //
>
> In the shell script a switch was used, thus `else if` would be a more
> appropriate choice over `if`. Also if the first if statement fails
> then it is unnecessary to go further.

Whether this was a 'switch' statement in the shell script is
immaterial. The body of each of these 'if' statements exits the
function, so no following code will be executed anyhow when the
condition is true. This makes the 'else' pure noise which is why
's/else //' is suggested and good style. The less the reader's brain
has to process, the easier the code is to comprehend.

>>> +               OPT_CMDMODE(0, "check-term-format", &sub_command,
>>> +                        N_("check format of the ref"), CHECK_TERM_FMT),
>>
>> What "ref"?
>
> The ref here means that ref (like HEAD).

Sorry, does not compute. To what HEAD or other ref are you referring?
This command is about checking the name of a bisection term. Where
does 'ref' come into it (other than as an implementation detail)?

Re: [PATCH 2/2] bisect: rewrite `check_term_format` shell function in C

[Christian Couder]

On Wed, May 4, 2016 at 10:28 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, May 4, 2016 at 3:36 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Wed, May 4, 2016 at 12:22 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> Okay, I'll bite: Why is this a good idea? What does it buy you?
>>>
>>> It's not as if the rewrite is especially faster or more easily
>>> expressed in C; quite the contrary, the shell code is more concise and
>>> probably about equally as fast (not that execution speed matters in
>>> this case).
>>>
>>> I could understand this functionality being ported to C in the form of
>>> a static function as a minor part of porting "git bisect terms" in its
>>> entirety to C, but I'm not imaginative enough to see why this
>>> functionality is useful as a standalone git-bisect--helper subcommand,
>>> and the commit message doesn't enlighten. Consequently, it seems like
>>> unnecessary complexity.
>>
>> It is important to understand that the subcommand is just a
>> **temporary** measure.
>
> The commit message seems to be lacking this information and any other rationale.

Yeah, I agree it could be improved in this regard.

>> Yes, I agree that making it a subcommand increases unnecessary
>> complexity. As a part of complete rewrite of git-bisect.sh, I am
>> converting one function individually to C. The functionality of
>> subcommand is useful so that I can use the existing test suite to
>> verify whether I have done the conversion properly. As more functions
>> get ported (which I intend to finish this summers), previously
>> existing subcommands will be removed. For eg. After this patch, I will
>> now convert the function write_terms(). So in that patch, I will
>> remove the subcommand for check-term-format and instead use the
>> check_term_format() method and then introduce a new subcommand for
>> write_terms(). Verifying the function conversion was suggested by
>> Stefan Beller[1] and Christian Couder[2] gave a hint of how to go
>> about with using the existing test suite. As for the current
>> situation, git-bisect.sh calls `--next-all` in a similar way which was
>> the hint for me of how to go about with this project.
>
> You're taking an inverted bottom-up approach which repeatedly adds and
> removes unnecessary complexity rather than a more straight-forward
> top-down approach. For instance, with a top-down approach, as a first
> step, you could instead add a skeleton, do-nothing "git-bisect--helper
> set-terms" and flesh it out in subsequent patches until fully
> implemented,

I am not sure this is the best approach. If for some reason the GSoC
project is not finished, we will end up with a badly named
"git-bisect--helper --set-terms" command and a badly named
write_term() shell functions. They will be badly named because they
will not do all what the name suggests.

> at which point drop all the "terms" code from
> git-bisect.sh and have it invoke the helper instead. You get the same
> benefit of being able to use the existing test suite without the
> unnecessary complexity.
>
> In fact, git-bisect.sh could start invoking "git-bisect--helper"
> before it's fully fleshed out. For instance, a partially fleshed out C
> write_terms(), might just verify that the two terms are not the same
> and then write them out to BISECT_TERMS, and the shell script would
> invoke its own check_term_format() before calling the helper.
>
>>>> +static int one_of(const char *term, ...)
>>>> +{
>>>> +       va_list matches;
>>>> +       const char *match;
>>>> +
>>>> +       va_start(matches, term);
>>>> +       while ((match = va_arg(matches, const char *)) != NULL)
>>>> +               if (!strcmp(term, match))
>>>> +                       return 1;
>>>
>>> Is it wise to return here without invoking va_end()?
>>
>> I guess since it already checks for NULL, invoking va_end() will make
>> it redundant[3].
>
> Sorry, your response does not compute. Each va_start() *must* be
> balanced with a va_end(). (While it's true that you may encounters
> platforms/compilers for which a missing va_end() does no harm, such
> code is not portable.)

Yeah, I agree with that and the rest of your comments. Thanks.

Re: [PATCH 2/2] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

On Wed, May 4, 2016 at 1:58 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, May 4, 2016 at 3:36 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Wed, May 4, 2016 at 12:22 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> Okay, I'll bite: Why is this a good idea? What does it buy you?
>>>
>>> It's not as if the rewrite is especially faster or more easily
>>> expressed in C; quite the contrary, the shell code is more concise and
>>> probably about equally as fast (not that execution speed matters in
>>> this case).
>>>
>>> I could understand this functionality being ported to C in the form of
>>> a static function as a minor part of porting "git bisect terms" in its
>>> entirety to C, but I'm not imaginative enough to see why this
>>> functionality is useful as a standalone git-bisect--helper subcommand,
>>> and the commit message doesn't enlighten. Consequently, it seems like
>>> unnecessary complexity.
>>
>> It is important to understand that the subcommand is just a
>> **temporary** measure.
>
> The commit message seems to be lacking this information and any other rationale.

I will modify the commit message in order to reflect this.

>>>> +static int one_of(const char *term, ...)
>>>> +{
>>>> +       va_list matches;
>>>> +       const char *match;
>>>> +
>>>> +       va_start(matches, term);
>>>> +       while ((match = va_arg(matches, const char *)) != NULL)
>>>> +               if (!strcmp(term, match))
>>>> +                       return 1;
>>>
>>> Is it wise to return here without invoking va_end()?
>>
>> I guess since it already checks for NULL, invoking va_end() will make
>> it redundant[3].
>
> Sorry, your response does not compute. Each va_start() *must* be
> balanced with a va_end(). (While it's true that you may encounters
> platforms/compilers for which a missing va_end() does no harm, such
> code is not portable.)

I am sorry for my misunderstanding. I had very little idea about
variable arguments. I have searched on this now. I will update by
according to Johannes which seems nice to me.

>>>> +       va_end(matches);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int check_term_format(const char *term, const char *orig_term,
>>>> +                            int flag)
>>>
>>> What is 'flag' for? The single caller only ever passes 0, so why is this needed?
>>
>> Well, currently the subcommand does not use this flag but this flag is
>> present in the method check_refname_format() so it would be better to
>> use it. This flag might be useful in further parts of conversion since
>> as I previously mentioned check-term-format isn't a permanent
>> solution.
>
> Sorry, again this does not compute. Certainly, you must pass *some*
> flags argument to check_refname_format() as 'flags' is part of its
> signature, but that doesn't explain why check_term_format() accepts a
> 'flag' argument. Moreover, check_term_format() is not a general
> purpose function like check_refname_format(), so this sort of
> *apparent* flexibility adds complexity with no obvious benefit.

I check the future functions and it does not require the flag argument
so I will remove it.

>>>> +       strbuf_addf(&new_term, "refs/bisect/%s", term);
>>>> +
>>>> +       if (check_refname_format(new_term.buf, flag))
>>>> +               die(_("'%s' is not a valid term\n"), term);
>>>
>>> Why does this die() while the other "invalid" cases merely return
>>> error()? What makes this special?
>>
>> This is because I felt that if check_refname_format() fails then its a
>> fatal error while in other cases, it is not as fatal.
>
> The name of the command is "check-term-format" and that is precisely
> its purpose so, from the perspective of the caller, *all* problems
> with the term are fatal. It's black-and-white, there is no grey:
> either a term is acceptable, or it isn't; that's all the caller wants
> to know. Consequently, all problems detected by this function should
> be reported the same way (preferably via 'return error()').

Sure. I will use 'return error()'. Any particular reason why this
instead of die() ?

>>>> +       else if (one_of(term, "help", "start", "skip", "next", "reset",
>>>
>>> s/else //
>>
>> Agree since it would be a part of the switch which is not included
>> with the check_refname_format().
>>
>>>> +       else if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
>>>
>>> s/else //
>>
>> In the shell script a switch was used, thus `else if` would be a more
>> appropriate choice over `if`. Also if the first if statement fails
>> then it is unnecessary to go further.
>
> Whether this was a 'switch' statement in the shell script is
> immaterial. The body of each of these 'if' statements exits the
> function, so no following code will be executed anyhow when the
> condition is true. This makes the 'else' pure noise which is why
> 's/else //' is suggested and good style. The less the reader's brain
> has to process, the easier the code is to comprehend.

Okay. I get it. Will drop off the else.

>>>> +               OPT_CMDMODE(0, "check-term-format", &sub_command,
>>>> +                        N_("check format of the ref"), CHECK_TERM_FMT),
>>>
>>> What "ref"?
>>
>> The ref here means that ref (like HEAD).
>
> Sorry, does not compute. To what HEAD or other ref are you referring?
> This command is about checking the name of a bisection term. Where
> does 'ref' come into it (other than as an implementation detail)?

I guess it would be more appropriate to use term.

Thanks,
Pranit Bauva

Re: [PATCH 2/2] bisect: rewrite `check_term_format` shell function in C

[Eric Sunshine]

On Wed, May 4, 2016 at 7:58 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Wed, May 4, 2016 at 1:58 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, May 4, 2016 at 3:36 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> On Wed, May 4, 2016 at 12:22 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>>> +       if (check_refname_format(new_term.buf, flag))
>>>>> +               die(_("'%s' is not a valid term\n"), term);
>>>>
>>>> Why does this die() while the other "invalid" cases merely return
>>>> error()? What makes this special?
>>>
>>> This is because I felt that if check_refname_format() fails then its a
>>> fatal error while in other cases, it is not as fatal.
>>
>> The name of the command is "check-term-format" and that is precisely
>> its purpose so, from the perspective of the caller, *all* problems
>> with the term are fatal. It's black-and-white, there is no grey:
>> either a term is acceptable, or it isn't; that's all the caller wants
>> to know. Consequently, all problems detected by this function should
>> be reported the same way (preferably via 'return error()').
>
> Sure. I will use 'return error()'. Any particular reason why this
> instead of die() ?

This function's specific purpose is *check* some input for validity
and let the caller know the result of that check. The most natural way
to do so is by returning a success/failure value to the caller.

die() is for signalling an exceptional condition which should abort
the program. There is nothing exceptional about a "false" result from
check-term-format, thus die() not an appropriate way to pass the
result to the caller.

Re: [PATCH 2/2] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

On Wed, May 4, 2016 at 11:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, May 4, 2016 at 7:58 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Wed, May 4, 2016 at 1:58 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Wed, May 4, 2016 at 3:36 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>> On Wed, May 4, 2016 at 12:22 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>>> On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>>>> +       if (check_refname_format(new_term.buf, flag))
>>>>>> +               die(_("'%s' is not a valid term\n"), term);
>>>>>
>>>>> Why does this die() while the other "invalid" cases merely return
>>>>> error()? What makes this special?
>>>>
>>>> This is because I felt that if check_refname_format() fails then its a
>>>> fatal error while in other cases, it is not as fatal.
>>>
>>> The name of the command is "check-term-format" and that is precisely
>>> its purpose so, from the perspective of the caller, *all* problems
>>> with the term are fatal. It's black-and-white, there is no grey:
>>> either a term is acceptable, or it isn't; that's all the caller wants
>>> to know. Consequently, all problems detected by this function should
>>> be reported the same way (preferably via 'return error()').
>>
>> Sure. I will use 'return error()'. Any particular reason why this
>> instead of die() ?
>
> This function's specific purpose is *check* some input for validity
> and let the caller know the result of that check. The most natural way
> to do so is by returning a success/failure value to the caller.
>
> die() is for signalling an exceptional condition which should abort
> the program. There is nothing exceptional about a "false" result from
> check-term-format, thus die() not an appropriate way to pass the
> result to the caller.

Thanks for the information! :)

Re: [PATCH 2/2] bisect: rewrite `check_term_format` shell function in C

[Johannes Schindelin]

Hi Pranit,

On Wed, 4 May 2016, Pranit Bauva wrote:

> On Wed, May 4, 2016 at 12:22 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> >
> >> +static int one_of(const char *term, ...)
> >> +{
> >> +       va_list matches;
> >> +       const char *match;
> >> +
> >> +       va_start(matches, term);
> >> +       while ((match = va_arg(matches, const char *)) != NULL)
> >> +               if (!strcmp(term, match))
> >> +                       return 1;
> >
> > Is it wise to return here without invoking va_end()?
> 
> I guess since it already checks for NULL, invoking va_end() will make
> it redundant[3].

Actually, this is my fault (I suggested that form of the one_of()
function). The man page for va_end() clearly states that every call to
va_start() needs to be paired with a corresponding va_end(), so it is
incorrect to return without a call to va_end().

Maybe something like instead?

	static int one_of(const char *term, ...)
	{
		int res = 0;
		va_list matches;
		const char *match;

		va_start(matches, term);
		while (!res && (match = va_arg(matches, const char *)))
			res = !strcmp(term, match);
		va_end(matches);

		return res;
	}
Ciao,
Dscho

Re: [PATCH 2/2] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

On Wed, May 4, 2016 at 4:43 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Pranit,
>
> On Wed, 4 May 2016, Pranit Bauva wrote:
>
>> On Wed, May 4, 2016 at 12:22 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> > On Wed, May 4, 2016 at 1:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> >
>> >> +static int one_of(const char *term, ...)
>> >> +{
>> >> +       va_list matches;
>> >> +       const char *match;
>> >> +
>> >> +       va_start(matches, term);
>> >> +       while ((match = va_arg(matches, const char *)) != NULL)
>> >> +               if (!strcmp(term, match))
>> >> +                       return 1;
>> >
>> > Is it wise to return here without invoking va_end()?
>>
>> I guess since it already checks for NULL, invoking va_end() will make
>> it redundant[3].
>
> Actually, this is my fault (I suggested that form of the one_of()
> function). The man page for va_end() clearly states that every call to
> va_start() needs to be paired with a corresponding va_end(), so it is
> incorrect to return without a call to va_end().
>
> Maybe something like instead?
>
>         static int one_of(const char *term, ...)
>         {
>                 int res = 0;
>                 va_list matches;
>                 const char *match;
>
>                 va_start(matches, term);
>                 while (!res && (match = va_arg(matches, const char *)))
>                         res = !strcmp(term, match);
>                 va_end(matches);
>
>                 return res;
>         }
> Ciao,
> Dscho

Thanks for this. I had little idea about variable arguments before. I
have searched it now. Will use your bits.

Re: [PATCH 2/2] bisect: rewrite `check_term_format` shell function in C

[Christian Couder]

On Wed, May 4, 2016 at 1:13 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Maybe something like instead?
>
>         static int one_of(const char *term, ...)
>         {
>                 int res = 0;
>                 va_list matches;
>                 const char *match;
>
>                 va_start(matches, term);
>                 while (!res && (match = va_arg(matches, const char *)))
>                         res = !strcmp(term, match);

Yeah or maybe:

                    while ((match = va_arg(matches, const char *)) != NULL)
                            if (!strcmp(term, match)) {
                                    res = 1
                                    break;
                            }

>                 va_end(matches);
>
>                 return res;
>         }

Re: [PATCH 2/2] bisect: rewrite `check_term_format` shell function in C

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Maybe something like instead?
>
> 	static int one_of(const char *term, ...)
> 	{
> 		int res = 0;
> 		va_list matches;
> 		const char *match;
>
> 		va_start(matches, term);
> 		while (!res && (match = va_arg(matches, const char *)))
> 			res = !strcmp(term, match);
> 		va_end(matches);
>
> 		return res;
> 	}
> Ciao,
> Dscho

This looks good (even though we try to avoid assignment in
conditionals as much as possible).

Re: [PATCH 0/2] bisect--helper: rewrite of check_term_format()

[Christian Couder]

Pranit,

My gmail address is "christian.couder@gmail.com", not
"christain.couder@gmail.com". ('i' and 'a' are in the wrong oder in my
first name).

Thanks,
Christian.

Re: [PATCH 0/2] bisect--helper: rewrite of check_term_format()

[Pranit Bauva]

On Wed, May 4, 2016 at 10:52 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> Pranit,
>
> My gmail address is "christian.couder@gmail.com", not
> "christain.couder@gmail.com". ('i' and 'a' are in the wrong oder in my
> first name).
>
> Thanks,
> Christian.

Sorry. It was a typo.

[PATCH v5 0/2] bisect--helper: rewrite of check-term-format()

[Pranit Bauva]

A very interesting fact to note:
I made a mistake by dropping of the first argument 'term' in the function
one_of() and compiled and the test suite passed fully (which was pointed
out by Christian). This shows that the test coverage is incomplete. I am
currently writing tests to improve on that coverage. It will be included
separately.

Link to v4: http://thread.gmane.org/gmane.comp.version-control.git/293488

Changes wrt v4:
 * Stick with 'subcommand' in the commit message.
 * Rename enum as 'subcommand' to make it singular.
 * s/bug/BUG/g
 * Drop _() in the default of switch case
 * Use some suggestions for commit message no. 2 and also include why I am
   using subcommand.
 * Drop of the latter part in the comment of one_of()
 * Remove flag from check_term_format()
 * Use return error() instead of die()
 * Drop '\n' from the localization strings
 * Include localization for a missed string
 * A comment in function check_term_format() is re-wrapped to fit in 80 columns
 * Use 'term' instead of 'ref'
 * Reword the string in the default of switch case
 * I have kept the commit message same as previous for commit no. 1 except for
   addressing the minor nits.

Pranit Bauva (2):
  bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
  bisect: rewrite `check_term_format` shell function in C

 builtin/bisect--helper.c | 76 ++++++++++++++++++++++++++++++++++++++++++++----
 git-bisect.sh            | 31 ++------------------
 2 files changed, 72 insertions(+), 35 deletions(-)

-- 
2.8.1

[PATCH v5 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

`--next-all` is meant to be used as a subcommand to support multiple
"operation mode" though the current implementation does not contain any
other subcommand along side with `--next-all` but further commits will
include some more subcommands.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229..d8de651 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -8,13 +8,17 @@ static const char * const git_bisect_helper_usage[] = {
 	NULL
 };
 
+enum subcommand {
+	NEXT_ALL = 1
+};
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-	int next_all = 0;
+	int subcommand = 0;
 	int no_checkout = 0;
 	struct option options[] = {
-		OPT_BOOL(0, "next-all", &next_all,
-			 N_("perform 'git bisect next'")),
+		OPT_CMDMODE(0, "next-all", &subcommand,
+			 N_("perform 'git bisect next'"), NEXT_ALL),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -23,9 +27,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_bisect_helper_usage, 0);
 
-	if (!next_all)
+	if (!subcommand)
 		usage_with_options(git_bisect_helper_usage, options);
 
-	/* next-all */
-	return bisect_next_all(prefix, no_checkout);
+	switch (subcommand) {
+	case NEXT_ALL:
+		return bisect_next_all(prefix, no_checkout);
+	default:
+		die("BUG: unknown subcommand '%d'", subcommand);
+	}
+	return 0;
 }
-- 
2.8.1

Re: [PATCH v5 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Johannes Schindelin]

Hi Pranit,

On Fri, 6 May 2016, Pranit Bauva wrote:

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3324229..d8de651 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -8,13 +8,17 @@ static const char * const git_bisect_helper_usage[] = {
>  	NULL
>  };
>  
> +enum subcommand {
> +	NEXT_ALL = 1
> +};

I still do not think that this enum needs to have file scope. Function
scope is enough.

>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
> -	int next_all = 0;
> +	int subcommand = 0;

Since subcommand is not simply an integer, but wants to take on the values
defined in the enum above, the type should be changed accordingly. You
could do it this way (short and sweet, with the appropriate scope):

	enum { NEXT_ALL = 1 } subcommand = 0;

See https://github.com/git/git/blob/v2.8.2/builtin/replace.c#L423-L430 for
an example (which uses "cmdmode" instead of "subcommand", too).

Ciao,
Dscho

Re: [PATCH v5 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

On Sun, May 8, 2016 at 12:34 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Pranit,
>
> On Fri, 6 May 2016, Pranit Bauva wrote:
>
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 3324229..d8de651 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -8,13 +8,17 @@ static const char * const git_bisect_helper_usage[] = {
>>       NULL
>>  };
>>
>> +enum subcommand {
>> +     NEXT_ALL = 1
>> +};
>
> I still do not think that this enum needs to have file scope. Function
> scope is enough.

In the very initial patch I made it in function scope. To which you
pointed out[1] that in all other examples but for one have file scope
so then I thought maybe that exception was a wrong example and I
should stick to the convention of putting it in file scope. But now I
also realize that builtin/replace.c uses "cmdmode" instead of
"subcommand" so I am still wondering what would be the most
appropriate?

>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>  {
>> -     int next_all = 0;
>> +     int subcommand = 0;
>
> Since subcommand is not simply an integer, but wants to take on the values
> defined in the enum above, the type should be changed accordingly. You
> could do it this way (short and sweet, with the appropriate scope):
>
>         enum { NEXT_ALL = 1 } subcommand = 0;
>
> See https://github.com/git/git/blob/v2.8.2/builtin/replace.c#L423-L430 for
> an example (which uses "cmdmode" instead of "subcommand", too).
>
> Ciao,
> Dscho
[1]: http://article.gmane.org/gmane.comp.version-control.git/289653

Regards,
Pranit Bauva

Re: [PATCH v5 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Christian Couder]

On Sun, May 8, 2016 at 9:17 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Sun, May 8, 2016 at 12:34 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> Hi Pranit,
>>
>> On Fri, 6 May 2016, Pranit Bauva wrote:
>>
>>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>>> index 3324229..d8de651 100644
>>> --- a/builtin/bisect--helper.c
>>> +++ b/builtin/bisect--helper.c
>>> @@ -8,13 +8,17 @@ static const char * const git_bisect_helper_usage[] = {
>>>       NULL
>>>  };
>>>
>>> +enum subcommand {
>>> +     NEXT_ALL = 1
>>> +};
>>
>> I still do not think that this enum needs to have file scope. Function
>> scope is enough.
>
> In the very initial patch I made it in function scope. To which you
> pointed out[1] that in all other examples but for one have file scope
> so then I thought maybe that exception was a wrong example and I
> should stick to the convention of putting it in file scope.

In the message Dscho wrote:

"Interesting. I did not think that Git's source code declares enums inside
functions, but builtin/remote.c's config_read_branches() does, so this
code is fine." So you didn't need to put it back in file scope.

Please don't change things when you are told they are fine unless
there is a good reason like a bug and in this case explain the reason.

> But now I
> also realize that builtin/replace.c uses "cmdmode" instead of
> "subcommand" so I am still wondering what would be the most
> appropriate?
>
>>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>>  {
>>> -     int next_all = 0;
>>> +     int subcommand = 0;
>>
>> Since subcommand is not simply an integer, but wants to take on the values
>> defined in the enum above, the type should be changed accordingly. You
>> could do it this way (short and sweet, with the appropriate scope):
>>
>>         enum { NEXT_ALL = 1 } subcommand = 0;
>>
>> See https://github.com/git/git/blob/v2.8.2/builtin/replace.c#L423-L430 for
>> an example (which uses "cmdmode" instead of "subcommand", too).

Yeah, please use "cmdmode" as Junio already suggested and do it like
in the example that Dscho points to.

Thanks,
Christian.

Re: [PATCH v5 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

On Sun, May 8, 2016 at 9:00 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Sun, May 8, 2016 at 9:17 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Sun, May 8, 2016 at 12:34 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>> Hi Pranit,
>>>
>>> On Fri, 6 May 2016, Pranit Bauva wrote:
>>>
>>>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>>>> index 3324229..d8de651 100644
>>>> --- a/builtin/bisect--helper.c
>>>> +++ b/builtin/bisect--helper.c
>>>> @@ -8,13 +8,17 @@ static const char * const git_bisect_helper_usage[] = {
>>>>       NULL
>>>>  };
>>>>
>>>> +enum subcommand {
>>>> +     NEXT_ALL = 1
>>>> +};
>>>
>>> I still do not think that this enum needs to have file scope. Function
>>> scope is enough.
>>
>> In the very initial patch I made it in function scope. To which you
>> pointed out[1] that in all other examples but for one have file scope
>> so then I thought maybe that exception was a wrong example and I
>> should stick to the convention of putting it in file scope.
>
> In the message Dscho wrote:
>
> "Interesting. I did not think that Git's source code declares enums inside
> functions, but builtin/remote.c's config_read_branches() does, so this
> code is fine." So you didn't need to put it back in file scope.
>
> Please don't change things when you are told they are fine unless
> there is a good reason like a bug and in this case explain the reason.

I will keep this in mind.

>> But now I
>> also realize that builtin/replace.c uses "cmdmode" instead of
>> "subcommand" so I am still wondering what would be the most
>> appropriate?
>>
>>>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>>>  {
>>>> -     int next_all = 0;
>>>> +     int subcommand = 0;
>>>
>>> Since subcommand is not simply an integer, but wants to take on the values
>>> defined in the enum above, the type should be changed accordingly. You
>>> could do it this way (short and sweet, with the appropriate scope):
>>>
>>>         enum { NEXT_ALL = 1 } subcommand = 0;
>>>
>>> See https://github.com/git/git/blob/v2.8.2/builtin/replace.c#L423-L430 for
>>> an example (which uses "cmdmode" instead of "subcommand", too).
>
> Yeah, please use "cmdmode" as Junio already suggested and do it like
> in the example that Dscho points to.

Yes sure!

> Thanks,
> Christian.

Re: [PATCH v5 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Johannes Schindelin]

Hi Pranit,

On Sun, 8 May 2016, Pranit Bauva wrote:

> On Sun, May 8, 2016 at 12:34 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Fri, 6 May 2016, Pranit Bauva wrote:
> >
> >> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> >> index 3324229..d8de651 100644
> >> --- a/builtin/bisect--helper.c
> >> +++ b/builtin/bisect--helper.c
> >> @@ -8,13 +8,17 @@ static const char * const git_bisect_helper_usage[] = {
> >>       NULL
> >>  };
> >>
> >> +enum subcommand {
> >> +     NEXT_ALL = 1
> >> +};
> >
> > I still do not think that this enum needs to have file scope. Function
> > scope is enough.
> 
> In the very initial patch I made it in function scope. To which you
> pointed out[1] that in all other examples but for one have file scope
> so then I thought maybe that exception was a wrong example and I
> should stick to the convention of putting it in file scope.

Oh, sorry, I meant to imply that it is good as it is by saying "so this
code is fine"...

I was just surprised because I thought I remembered that some old C
standard does not allow enums to be function scoped. But I was wrong.

> But now I also realize that builtin/replace.c uses "cmdmode" instead of
> "subcommand" so I am still wondering what would be the most appropriate?

I think the replace.c code is really a good example. Function-scoped,
using the "cmdmode" name that obviously corresponds to the OPT_CMDMODE
name.

Ciao,
Dscho

Re: [PATCH v5 1/2] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

Hey Johannes,

On Mon, May 9, 2016 at 8:29 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Pranit,
>
> On Sun, 8 May 2016, Pranit Bauva wrote:
>
>> On Sun, May 8, 2016 at 12:34 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> >
>> > On Fri, 6 May 2016, Pranit Bauva wrote:
>> >
>> >> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> >> index 3324229..d8de651 100644
>> >> --- a/builtin/bisect--helper.c
>> >> +++ b/builtin/bisect--helper.c
>> >> @@ -8,13 +8,17 @@ static const char * const git_bisect_helper_usage[] = {
>> >>       NULL
>> >>  };
>> >>
>> >> +enum subcommand {
>> >> +     NEXT_ALL = 1
>> >> +};
>> >
>> > I still do not think that this enum needs to have file scope. Function
>> > scope is enough.
>>
>> In the very initial patch I made it in function scope. To which you
>> pointed out[1] that in all other examples but for one have file scope
>> so then I thought maybe that exception was a wrong example and I
>> should stick to the convention of putting it in file scope.
>
> Oh, sorry, I meant to imply that it is good as it is by saying "so this
> code is fine"...
>
> I was just surprised because I thought I remembered that some old C
> standard does not allow enums to be function scoped. But I was wrong.
>
>> But now I also realize that builtin/replace.c uses "cmdmode" instead of
>> "subcommand" so I am still wondering what would be the most appropriate?
>
> I think the replace.c code is really a good example. Function-scoped,
> using the "cmdmode" name that obviously corresponds to the OPT_CMDMODE
> name.

Sure. WIll do!

Regards,
Pranit Bauva

[PATCH v5 2/2] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

Reimplement the `check_term_format` shell function in C and add
a `--check-term-format` subcommand to `git bisect--helper` to call it
from git-bisect.sh

Using `--check-term-format` subcommand is a temporary measure to port
shell function to C so as to use the existing test suite. As more
functions are ported, this subcommand will loose its existence and will
be called by some other method/subcommand. For eg. In conversion of
write_terms() of git-bisect.sh, the subcommand will be removed and
instead check_term_format() will be called in its C implementation while
a new subcommand will be introduced for write_terms().

Helped-by: Johannes Schindelein <Johannes.Schindelein@gmx.de>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 31 ++------------------------
 2 files changed, 58 insertions(+), 30 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index d8de651..cc50438 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -2,16 +2,65 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "bisect.h"
+#include "refs.h"
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
+	N_("git bisect--helper --check-term-format <term> <orig_term>"),
 	NULL
 };
 
 enum subcommand {
-	NEXT_ALL = 1
+	NEXT_ALL = 1,
+	CHECK_TERM_FMT
 };
 
+/*
+ * To check whether the string `term` belongs to the set of strings
+ * included in the variable arguments.
+ */
+static int one_of(const char *term, ...)
+{
+	int res = 0;
+	va_list matches;
+	const char *match;
+
+	va_start(matches, term);
+	while (!res && (match=va_arg(matches, const char *)))
+		res = !strcmp(term, match);
+	va_end(matches);
+
+	return res;
+}
+
+static int check_term_format(const char *term, const char *orig_term)
+{
+	struct strbuf new_term = STRBUF_INIT;
+	strbuf_addf(&new_term, "refs/bisect/%s", term);
+
+	if (check_refname_format(new_term.buf, 0)) {
+		strbuf_release(&new_term);
+		return error(_("'%s' is not a valid term"), term);
+	}
+	strbuf_release(&new_term);
+
+	if (one_of(term, "help", "start", "skip", "next", "reset",
+			"visualize", "replay", "log", "run", NULL))
+		return error(_("can't use the builtin command '%s' as a term"), term);
+
+	/*
+	 * In theory, nothing prevents swapping completely good and bad,
+	 * but this situation could be confusing and hasn't been tested
+	 * enough. Forbid it for now.
+	 */
+
+	if ((strcmp(orig_term, "bad") && one_of(term, "bad", "new", NULL)) ||
+		 (strcmp(orig_term, "good") && one_of(term, "good", "old", NULL)))
+		return error(_("can't change the meaning of the term '%s'"), term);
+
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	int subcommand = 0;
@@ -19,6 +68,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &subcommand,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
+		OPT_CMDMODE(0, "check-term-format", &subcommand,
+			 N_("check format of the term"), CHECK_TERM_FMT),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -33,6 +84,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	switch (subcommand) {
 	case NEXT_ALL:
 		return bisect_next_all(prefix, no_checkout);
+	case CHECK_TERM_FMT:
+		if (argc != 2)
+			die(_("--check-term-format requires two arguments"));
+		return check_term_format(argv[0], argv[1]);
 	default:
 		die("BUG: unknown subcommand '%d'", subcommand);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 5d1cb00..7d7965d 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -564,38 +564,11 @@ write_terms () {
 	then
 		die "$(gettext "please use two different terms")"
 	fi
-	check_term_format "$TERM_BAD" bad
-	check_term_format "$TERM_GOOD" good
+	git bisect--helper --check-term-format "$TERM_BAD" bad || exit
+	git bisect--helper --check-term-format "$TERM_GOOD" good || exit
 	printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
 }
 
-check_term_format () {
-	term=$1
-	git check-ref-format refs/bisect/"$term" ||
-	die "$(eval_gettext "'\$term' is not a valid term")"
-	case "$term" in
-	help|start|terms|skip|next|reset|visualize|replay|log|run)
-		die "$(eval_gettext "can't use the builtin command '\$term' as a term")"
-		;;
-	bad|new)
-		if test "$2" != bad
-		then
-			# In theory, nothing prevents swapping
-			# completely good and bad, but this situation
-			# could be confusing and hasn't been tested
-			# enough. Forbid it for now.
-			die "$(eval_gettext "can't change the meaning of term '\$term'")"
-		fi
-		;;
-	good|old)
-		if test "$2" != good
-		then
-			die "$(eval_gettext "can't change the meaning of term '\$term'")"
-		fi
-		;;
-	esac
-}
-
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
-- 
2.8.1

Re: [PATCH v5 2/2] bisect: rewrite `check_term_format` shell function in C

[Eric Sunshine]

On Fri, May 6, 2016 at 10:49 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Reimplement the `check_term_format` shell function in C and add
> a `--check-term-format` subcommand to `git bisect--helper` to call it
> from git-bisect.sh
>
> Using `--check-term-format` subcommand is a temporary measure to port
> shell function to C so as to use the existing test suite. As more
> functions are ported, this subcommand will loose its existence and will

s/loose/lose/

Though, maybe it would be better to say:

    ...this subcommand will be retired...

> be called by some other method/subcommand. For eg. In conversion of
> write_terms() of git-bisect.sh, the subcommand will be removed and
> instead check_term_format() will be called in its C implementation while
> a new subcommand will be introduced for write_terms().
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -2,16 +2,65 @@
> +/*
> + * To check whether the string `term` belongs to the set of strings
> + * included in the variable arguments.

This is still a sentence fragment as pointed out last time[1]. You can
fix it with: s/To check/Check/

> + */
> +static int one_of(const char *term, ...)
> +{
> +       int res = 0;
> +       va_list matches;
> +       const char *match;
> +
> +       va_start(matches, term);
> +       while (!res && (match=va_arg(matches, const char *)))

Style: add spaces around '='

> +               res = !strcmp(term, match);
> +       va_end(matches);
> +
> +       return res;
> +}

The rest of the patch seems to address the issues raised by my
previous review[1] (aside from my comments[1,2] about this bottom-up
approach repeatedly adding and removing unnecessary complexity as each
little function is ported from shell to C, but that's a separate
philosophical discussion).

[1]: http://thread.gmane.org/gmane.comp.version-control.git/289476/focus=293501
[2]: http://thread.gmane.org/gmane.comp.version-control.git/289476/focus=293517

Re: [PATCH v5 0/2] bisect--helper: rewrite of check-term-format()

[Junio C Hamano]

Pranit Bauva <pranit.bauva@gmail.com> writes:

> A very interesting fact to note:
> I made a mistake by dropping of the first argument 'term' in the function
> one_of() and compiled and the test suite passed fully (which was pointed
> out by Christian). This shows that the test coverage is incomplete. 
> I am
> currently writing tests to improve on that coverage. It will be included
> separately.
>
> Link to v4: http://thread.gmane.org/gmane.comp.version-control.git/293488
>
> Changes wrt v4:
>  * Stick with 'subcommand' in the commit message.
>  * Rename enum as 'subcommand' to make it singular.

I've already said what needs to be said on this.

>  * s/bug/BUG/g
>  * Drop _() in the default of switch case
>  * Use some suggestions for commit message no. 2 and also include why I am
>    using subcommand.

I am not particularly impressed by the rationale, though.

As a starter project to show that you learned how to add a new mode
to bisect--helper, check-term-format may be a small enough function
that is easy to dip the toe into the codebase, so in that sense it
may be an appropriate first step, but otherwise it is not all that
interesting, especially when we _know_ that it will be discarded.

Re: [PATCH v5 0/2] bisect--helper: rewrite of check-term-format()

[Pranit Bauva]

On Sat, May 7, 2016 at 3:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> A very interesting fact to note:
>> I made a mistake by dropping of the first argument 'term' in the function
>> one_of() and compiled and the test suite passed fully (which was pointed
>> out by Christian). This shows that the test coverage is incomplete.
>> I am
>> currently writing tests to improve on that coverage. It will be included
>> separately.
>>
>> Link to v4: http://thread.gmane.org/gmane.comp.version-control.git/293488
>>
>> Changes wrt v4:
>>  * Stick with 'subcommand' in the commit message.
>>  * Rename enum as 'subcommand' to make it singular.
>
> I've already said what needs to be said on this.
>
>>  * s/bug/BUG/g
>>  * Drop _() in the default of switch case
>>  * Use some suggestions for commit message no. 2 and also include why I am
>>    using subcommand.
>
> I am not particularly impressed by the rationale, though.
>
> As a starter project to show that you learned how to add a new mode
> to bisect--helper, check-term-format may be a small enough function
> that is easy to dip the toe into the codebase, so in that sense it
> may be an appropriate first step, but otherwise it is not all that
> interesting, especially when we _know_ that it will be discarded.

I do understand that check-term-format was a suggestion just for me to
"Get a taste" for the project.
I have now also converted write_terms() function. I have made a PR[1]
which needs a few things to be addressed.
Just a summary of what the PR does:
 * Converts the shell function write() terms to C
 * Adds a subcommand for write_terms()
 * Remove the subcommand for check_term_format()
 * Call the method check_term_format() from inside write_terms().

Do you want me to include both of the functions check_term_format()
and write_terms() in the same commit or a different commit.

OR,
I completely missed your point and you want me to go the Eric Sunshine's way?

[1]: https://github.com/pranitbauva1997/git/pull/3

Re: [PATCH v5 0/2] bisect--helper: rewrite of check-term-format()

[Junio C Hamano]

Pranit Bauva <pranit.bauva@gmail.com> writes:

> I completely missed your point and you want me to go the Eric Sunshine's way?

I am neutral.

When I read your response to Eric's "top down" suggestion, I didn't
quite get much more than "I started going this way, and I do not
want to change to the other direction.", which was what I had the
most trouble with.  If your justification for the approach to start
from building a tiny bottom layer that will need to be dismantled
soon and repeat that (which sounds somewhat wasteful) were more
convincing, I may have felt differently.

Re: [PATCH v5 0/2] bisect--helper: rewrite of check-term-format()

[Pranit Bauva]

On Sun, May 8, 2016 at 7:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> I completely missed your point and you want me to go the Eric Sunshine's way?
>
> I am neutral.
>
> When I read your response to Eric's "top down" suggestion, I didn't
> quite get much more than "I started going this way, and I do not
> want to change to the other direction.", which was what I had the
> most trouble with.  If your justification for the approach to start
> from building a tiny bottom layer that will need to be dismantled
> soon and repeat that (which sounds somewhat wasteful) were more
> convincing, I may have felt differently.

Sorry if it seemed that "I have done quite some work and I don't want
to scrape it off and redo everything". This isn't a case for me. I
think of this as just a small part in the process of learning and my
efforts would be completely wasted as I can still reuse the methods I
wrote. This is still open for a "philosophical" discussion. I am
assuming 1e1ea69fa4e is how Eric is suggesting.

Why I think its better to go the subcommand way:
 * I can test **C implementation** of the function to verify whether
it was done in a proper way. By using a "top down" approach, I can
make the test suite passing by running the code from git-bisect.sh not
the re-written C code.
 * I have made a very few minor mistakes in conversion of
check_term_format() which just messed up my head (I actually spent 3
days before Christian pointed out that '!' was missing). Imagine
debugging the complete git-bisect.sh to find a very small error after
a complete implementation.
 * Using subcommands, I can easily verify whether I have done it the
right way or not.
 * It makes the review process very simple. The reviewers/testers can
verify that my method is working correctly and well computers can
detect errors better than humans.
 * I can convert small functions which can be reviewed easily rather
than dumping a big series.

What I think is that the bottom up approach will make life easier for
the me and for reviewer. Yes, it does make the code "unclean" for the
time being and if it is between releases then all the more painful.
Well, the latter part can be avoided by keeping it in next.

Please point out if I am mistaken about anything.

Regards,
Pranit Bauva

Re: [PATCH v5 0/2] bisect--helper: rewrite of check-term-format()

[Pranit Bauva]

On Sun, May 8, 2016 at 11:53 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Sun, May 8, 2016 at 7:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Pranit Bauva <pranit.bauva@gmail.com> writes:
>>
>>> I completely missed your point and you want me to go the Eric Sunshine's way?
>>
>> I am neutral.
>>
>> When I read your response to Eric's "top down" suggestion, I didn't
>> quite get much more than "I started going this way, and I do not
>> want to change to the other direction.", which was what I had the
>> most trouble with.  If your justification for the approach to start
>> from building a tiny bottom layer that will need to be dismantled
>> soon and repeat that (which sounds somewhat wasteful) were more
>> convincing, I may have felt differently.
>
> Sorry if it seemed that "I have done quite some work and I don't want
> to scrape it off and redo everything". This isn't a case for me. I
> think of this as just a small part in the process of learning and my
> efforts would be completely wasted as I can still reuse the methods I

efforts would **not** be completely wasted

> wrote. This is still open for a "philosophical" discussion. I am
> assuming 1e1ea69fa4e is how Eric is suggesting.
>
> Why I think its better to go the subcommand way:
>  * I can test **C implementation** of the function to verify whether
> it was done in a proper way. By using a "top down" approach, I can
> make the test suite passing by running the code from git-bisect.sh not
> the re-written C code.
>  * I have made a very few minor mistakes in conversion of
> check_term_format() which just messed up my head (I actually spent 3
> days before Christian pointed out that '!' was missing). Imagine
> debugging the complete git-bisect.sh to find a very small error after
> a complete implementation.
>  * Using subcommands, I can easily verify whether I have done it the
> right way or not.
>  * It makes the review process very simple. The reviewers/testers can
> verify that my method is working correctly and well computers can
> detect errors better than humans.
>  * I can convert small functions which can be reviewed easily rather
> than dumping a big series.
>
> What I think is that the bottom up approach will make life easier for
> the me and for reviewer. Yes, it does make the code "unclean" for the
> time being and if it is between releases then all the more painful.
> Well, the latter part can be avoided by keeping it in next.
>
> Please point out if I am mistaken about anything.

There is also a third way in which I can go which is kind of merge of
both the ways:

 * Make two branches, one will contain the top down approach and
another will contain the subcommand approach.

 * I will write convert a function as a subcommand and test it locally
as well as on Travis-Cl by a PR on my git.git repo. The
reviewers/testers can consult the Travis-CI for output of the tests.
This will also reduce the need of locally compiling/testing.

 * I will then just write the function as it is in my "top down"
branch and thus send the patch to the mailing list. These can be kept
in next until it finishes.

 * Then will remove linking of git-bisect.sh and thus finalize.

This will be a bit more intensive not in a coding way but in
maintaining the repo. So I will have more experience of maintaining a
git repo. :)

Regards,
Pranit Bauva

Re: [PATCH v5 0/2] bisect--helper: rewrite of check-term-format()

[Eric Sunshine]

On Sun, May 8, 2016 at 9:34 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Sun, May 8, 2016 at 11:53 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Sun, May 8, 2016 at 7:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Pranit Bauva <pranit.bauva@gmail.com> writes:
>>>> I completely missed your point and you want me to go the Eric Sunshine's way?
>>>
>>> I am neutral.
>>>
>>> When I read your response to Eric's "top down" suggestion, I didn't
>>> quite get much more than "I started going this way, and I do not
>>> want to change to the other direction.", which was what I had the
>>> most trouble with.  If your justification for the approach to start
>>> from building a tiny bottom layer that will need to be dismantled
>>> soon and repeat that (which sounds somewhat wasteful) were more
>>> convincing, I may have felt differently.
>>
>> Sorry if it seemed that "I have done quite some work and I don't want
>> to scrape it off and redo everything". This isn't a case for me. I
>> think of this as just a small part in the process of learning and my
>> efforts would be completely wasted as I can still reuse the methods I
>
> efforts would **not** be completely wasted
>
>> wrote. This is still open for a "philosophical" discussion. I am
>> assuming 1e1ea69fa4e is how Eric is suggesting.

Speaking of 1e1ea69 (pull: implement skeletal builtin pull,
2015-06-14), one of the (numerous) things Paul Tan did which impressed
me was to formally measure test suite coverage of the commands he was
converting to C, and then improve coverage where it was lacking. That
approach increases confidence in the conversion far more than fallible
human reviews do.

Setting aside the top-down vs. bottom-up discussion, as a reviewer
(and user) I'd be far more interested in seeing you spend a good
initial chunk of your project emulating Paul's approach to measuring
and improving test coverage (though I don't know how your GSoC mentors
feel about that).

Re: [PATCH v5 0/2] bisect--helper: rewrite of check-term-format()

[Christian Couder]

On Mon, May 16, 2016 at 2:35 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, May 8, 2016 at 9:34 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Sun, May 8, 2016 at 11:53 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> On Sun, May 8, 2016 at 7:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Pranit Bauva <pranit.bauva@gmail.com> writes:
>>>>> I completely missed your point and you want me to go the Eric Sunshine's way?
>>>>
>>>> I am neutral.
>>>>
>>>> When I read your response to Eric's "top down" suggestion, I didn't
>>>> quite get much more than "I started going this way, and I do not
>>>> want to change to the other direction.", which was what I had the
>>>> most trouble with.  If your justification for the approach to start
>>>> from building a tiny bottom layer that will need to be dismantled
>>>> soon and repeat that (which sounds somewhat wasteful) were more
>>>> convincing, I may have felt differently.
>>>
>>> Sorry if it seemed that "I have done quite some work and I don't want
>>> to scrape it off and redo everything". This isn't a case for me. I
>>> think of this as just a small part in the process of learning and my
>>> efforts would be completely wasted as I can still reuse the methods I
>>
>> efforts would **not** be completely wasted
>>
>>> wrote. This is still open for a "philosophical" discussion. I am
>>> assuming 1e1ea69fa4e is how Eric is suggesting.
>
> Speaking of 1e1ea69 (pull: implement skeletal builtin pull,
> 2015-06-14), one of the (numerous) things Paul Tan did which impressed
> me was to formally measure test suite coverage of the commands he was
> converting to C, and then improve coverage where it was lacking. That
> approach increases confidence in the conversion far more than fallible
> human reviews do.
>
> Setting aside the top-down vs. bottom-up discussion, as a reviewer
> (and user) I'd be far more interested in seeing you spend a good
> initial chunk of your project emulating Paul's approach to measuring
> and improving test coverage (though I don't know how your GSoC mentors
> feel about that).

If we could test each `git bisect-helper` subcommand that has been
converted to C separately, then I think it would be quite easy and a
good idea. And maybe Paul's approach is really great and easy to use,
but I haven't followed his GSoC, so I cannot tell.

As we take the approach of testing the whole thing, which is made of a
number of different source files (shell code in git-bisect.sh, C code
in `git bisect-helper` subcommands and in bisect.{c,h}), I am worried
that Pranit would spend too much time initially not only to measure
test coverage but especially to find ways to improve it.

The good thing with converting code incrementally to C is that one
does not need to understand everything first. While if one wants to
increase test coverage initially, it might be needed to understand
many things.

What I started asking though is to take advantage of bugs that are
found, and were not covered by the test suite, to improve test
coverage.

Also starting to convert functions right away can make the student
feel productive early and in turn make everyone else happy to see some
progress.

Re: [PATCH v5 0/2] bisect--helper: rewrite of check-term-format()

[Pranit Bauva]

Hey Eric,
Sorry for the late reply. I was on vacation.

On Mon, May 16, 2016 at 6:05 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, May 8, 2016 at 9:34 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Sun, May 8, 2016 at 11:53 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> On Sun, May 8, 2016 at 7:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Pranit Bauva <pranit.bauva@gmail.com> writes:
>>>>> I completely missed your point and you want me to go the Eric Sunshine's way?
>>>>
>>>> I am neutral.
>>>>
>>>> When I read your response to Eric's "top down" suggestion, I didn't
>>>> quite get much more than "I started going this way, and I do not
>>>> want to change to the other direction.", which was what I had the
>>>> most trouble with.  If your justification for the approach to start
>>>> from building a tiny bottom layer that will need to be dismantled
>>>> soon and repeat that (which sounds somewhat wasteful) were more
>>>> convincing, I may have felt differently.
>>>
>>> Sorry if it seemed that "I have done quite some work and I don't want
>>> to scrape it off and redo everything". This isn't a case for me. I
>>> think of this as just a small part in the process of learning and my
>>> efforts would be completely wasted as I can still reuse the methods I
>>
>> efforts would **not** be completely wasted
>>
>>> wrote. This is still open for a "philosophical" discussion. I am
>>> assuming 1e1ea69fa4e is how Eric is suggesting.
>
> Speaking of 1e1ea69 (pull: implement skeletal builtin pull,
> 2015-06-14), one of the (numerous) things Paul Tan did which impressed
> me was to formally measure test suite coverage of the commands he was
> converting to C, and then improve coverage where it was lacking. That
> approach increases confidence in the conversion far more than fallible
> human reviews do.
>
> Setting aside the top-down vs. bottom-up discussion, as a reviewer
> (and user) I'd be far more interested in seeing you spend a good
> initial chunk of your project emulating Paul's approach to measuring
> and improving test coverage (though I don't know how your GSoC mentors
> feel about that).

Just adding to the points mentioned by Christian.

I had initially planned to first improve test coverage and then start
with function conversion as I mentioned in my introductory mail[1]. I
also pointed out that I did some work searching about the tools to
test coverage (kcov as Matthieu) suggested and I found that it is not
that easy to set it up. Then Christian pointed it out (privately) that
I can do this afterwards too before the code is finally merged. And
also I am trying to see the test coverage as and when I am converting
each function.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/292308

Regards,
Pranit Bauva

[PATCH v6 0/3] bisect--helper: check_term_format() & write_terms()

[Pranit Bauva]

To show how I am going to approach conversion of shell function to its C
implementation by using the subcommand/cmdmode approach, I have first converted
check_terms_format() to a subcommand then I have converted write_terms() to
a subcommand and then remove the subcommand for check_terms_format() and
instead call that function from write_terms().

Also I investigated about the test coverage which I mentioned in my previous
version and there is nothing that could be done to improve it.

Minor changes wrt v5:
 * Use the term cmdmode instead of subcommand as pointed out by Junio. (I
   didn't understand his response in v4 then Christian and Johannes pointed
   it out to me what he meant).
 * Move the enum cmdmode (for subcommand) to function scope as suggested by
   Johannes Schindelin.
 * A few minor nits by Eric Sunshine.

Link for v5: http://thread.gmane.org/gmane.comp.version-control.git/293785

Pranit Bauva (3):
  bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
  bisect: rewrite `check_term_format` shell function in C
  bisect--helper: `write_terms` shell function in C

 builtin/bisect--helper.c | 97 +++++++++++++++++++++++++++++++++++++++++++++---
 git-bisect.sh            | 49 ++++--------------------
 2 files changed, 98 insertions(+), 48 deletions(-)

-- 
2.8.2

[PATCH v6 1/3] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

`--next-all` is meant to be used as a subcommand to support multiple
"operation mode" though the current implementation does not contain any
other subcommand along side with `--next-all` but further commits will
include some more subcommands.

Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229..8111c91 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -10,11 +10,11 @@ static const char * const git_bisect_helper_usage[] = {
 
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-	int next_all = 0;
+	enum { NEXT_ALL = 1 } cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
-		OPT_BOOL(0, "next-all", &next_all,
-			 N_("perform 'git bisect next'")),
+		OPT_CMDMODE(0, "next-all", &cmdmode,
+			 N_("perform 'git bisect next'"), NEXT_ALL),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_bisect_helper_usage, 0);
 
-	if (!next_all)
+	if (!cmdmode)
 		usage_with_options(git_bisect_helper_usage, options);
 
-	/* next-all */
-	return bisect_next_all(prefix, no_checkout);
+	switch (cmdmode) {
+	case NEXT_ALL:
+		return bisect_next_all(prefix, no_checkout);
+	default:
+		die("BUG: unknown subcommand '%d'", cmdmode);
+	}
+	return 0;
 }
-- 
2.8.2

Re: [PATCH v6 1/3] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Eric Sunshine]

On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> `--next-all` is meant to be used as a subcommand to support multiple
> "operation mode" though the current implementation does not contain any
> other subcommand along side with `--next-all` but further commits will
> include some more subcommands.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> -       if (!next_all)
> +       if (!cmdmode)
>                 usage_with_options(git_bisect_helper_usage, options);
>
> -       /* next-all */
> -       return bisect_next_all(prefix, no_checkout);
> +       switch (cmdmode) {
> +       case NEXT_ALL:
> +               return bisect_next_all(prefix, no_checkout);
> +       default:
> +               die("BUG: unknown subcommand '%d'", cmdmode);
> +       }
> +       return 0;

What happens if you remove this useless 'return 0'? Does the (or some)
compiler incorrectly complain about it falling off the end of the
function without returning a value?

>  }

Re: [PATCH v6 1/3] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

Hey Eric,

On Mon, May 16, 2016 at 12:37 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> `--next-all` is meant to be used as a subcommand to support multiple
>> "operation mode" though the current implementation does not contain any
>> other subcommand along side with `--next-all` but further commits will
>> include some more subcommands.
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> -       if (!next_all)
>> +       if (!cmdmode)
>>                 usage_with_options(git_bisect_helper_usage, options);
>>
>> -       /* next-all */
>> -       return bisect_next_all(prefix, no_checkout);
>> +       switch (cmdmode) {
>> +       case NEXT_ALL:
>> +               return bisect_next_all(prefix, no_checkout);
>> +       default:
>> +               die("BUG: unknown subcommand '%d'", cmdmode);
>> +       }
>> +       return 0;
>
> What happens if you remove this useless 'return 0'? Does the (or some)
> compiler incorrectly complain about it falling off the end of the
> function without returning a value?

I tried removing it. It works fine with gcc and clang. You can see the
build on travis-CI[1]. I am not sure of other compilers and also don't
know a way to test it either. You could use my branch on github[2] if
you want to test it on other compilers. I think its better to keep the
return 0 if we aren't sure whether it would work on every compiler.

[1]: https://travis-ci.org/pranitbauva1997/git/builds/131622175
[2]: https://github.com/pranitbauva1997/git/tree/return-try

Regards,
Pranit Bauva

[PATCH v6 2/3] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

Reimplement the `check_term_format` shell function in C and add
a `--check-term-format` subcommand to `git bisect--helper` to call it
from git-bisect.sh

Using `--check-term-format` subcommand is a temporary measure to port
shell function to 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 method/subcommand. For eg. In conversion of
write_terms() of git-bisect.sh, the subcommand will be removed and
instead check_term_format() will be called in its C implementation while
a new subcommand will be introduced for write_terms().

Helped-by: Johannes Schindelein <Johannes.Schindelein@gmx.de>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 31 ++-----------------------
 2 files changed, 60 insertions(+), 30 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 8111c91..3c748d1 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -2,19 +2,72 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "bisect.h"
+#include "refs.h"
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
+	N_("git bisect--helper --check-term-format <term> <orig_term>"),
 	NULL
 };
 
+/*
+ * Check whether the string `term` belongs to the set of strings
+ * included in the variable arguments.
+ */
+static int one_of(const char *term, ...)
+{
+	int res = 0;
+	va_list matches;
+	const char *match;
+
+	va_start(matches, term);
+	while (!res && (match = va_arg(matches, const char *)))
+		res = !strcmp(term, match);
+	va_end(matches);
+
+	return res;
+}
+
+static int check_term_format(const char *term, const char *orig_term)
+{
+	struct strbuf new_term = STRBUF_INIT;
+	strbuf_addf(&new_term, "refs/bisect/%s", term);
+
+	if (check_refname_format(new_term.buf, 0)) {
+		strbuf_release(&new_term);
+		return error(_("'%s' is not a valid term"), term);
+	}
+	strbuf_release(&new_term);
+
+	if (one_of(term, "help", "start", "skip", "next", "reset",
+			"visualize", "replay", "log", "run", NULL))
+		return error(_("can't use the builtin command '%s' as a term"), term);
+
+	/*
+	 * In theory, nothing prevents swapping completely good and bad,
+	 * but this situation could be confusing and hasn't been tested
+	 * enough. Forbid it for now.
+	 */
+
+	if ((strcmp(orig_term, "bad") && one_of(term, "bad", "new", NULL)) ||
+		 (strcmp(orig_term, "good") && one_of(term, "good", "old", NULL)))
+		return error(_("can't change the meaning of the term '%s'"), term);
+
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-	enum { NEXT_ALL = 1 } cmdmode = 0;
+	enum {
+		NEXT_ALL = 1,
+		CHECK_TERM_FMT
+	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
+		OPT_CMDMODE(0, "check-term-format", &cmdmode,
+			 N_("check format of the term"), CHECK_TERM_FMT),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -29,6 +82,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	switch (cmdmode) {
 	case NEXT_ALL:
 		return bisect_next_all(prefix, no_checkout);
+	case CHECK_TERM_FMT:
+		if (argc != 2)
+			die(_("--check-term-format requires two arguments"));
+		return check_term_format(argv[0], argv[1]);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 5d1cb00..7d7965d 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -564,38 +564,11 @@ write_terms () {
 	then
 		die "$(gettext "please use two different terms")"
 	fi
-	check_term_format "$TERM_BAD" bad
-	check_term_format "$TERM_GOOD" good
+	git bisect--helper --check-term-format "$TERM_BAD" bad || exit
+	git bisect--helper --check-term-format "$TERM_GOOD" good || exit
 	printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
 }
 
-check_term_format () {
-	term=$1
-	git check-ref-format refs/bisect/"$term" ||
-	die "$(eval_gettext "'\$term' is not a valid term")"
-	case "$term" in
-	help|start|terms|skip|next|reset|visualize|replay|log|run)
-		die "$(eval_gettext "can't use the builtin command '\$term' as a term")"
-		;;
-	bad|new)
-		if test "$2" != bad
-		then
-			# In theory, nothing prevents swapping
-			# completely good and bad, but this situation
-			# could be confusing and hasn't been tested
-			# enough. Forbid it for now.
-			die "$(eval_gettext "can't change the meaning of term '\$term'")"
-		fi
-		;;
-	good|old)
-		if test "$2" != good
-		then
-			die "$(eval_gettext "can't change the meaning of term '\$term'")"
-		fi
-		;;
-	esac
-}
-
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
-- 
2.8.2

Re: [PATCH v6 2/3] bisect: rewrite `check_term_format` shell function in C

[Junio C Hamano]

Pranit Bauva <pranit.bauva@gmail.com> writes:

> +	/*
> +	 * In theory, nothing prevents swapping completely good and bad,
> +	 * but this situation could be confusing and hasn't been tested
> +	 * enough. Forbid it for now.
> +	 */
> +
> +	if ((strcmp(orig_term, "bad") && one_of(term, "bad", "new", NULL)) ||
> +		 (strcmp(orig_term, "good") && one_of(term, "good", "old", NULL)))
> +		return error(_("can't change the meaning of the term '%s'"), term);

The above comes from below

> -	bad|new)
> -		if test "$2" != bad
> -		then
> -			die "$(eval_gettext "can't change the meaning ...

So it is not your fault, but it is quite hard to understand.

The original says "You are trying to use 'bad' (or 'new') for
something that is not 'bad'--which is confusing; do not do it".

I _think_ the reason I find C version harder to read is the use of
strcmp(orig_term, "bad") to say "orig_term is not BAD".  The shell
version visually tells with "!=" that the meaning of the new term is
*NOT* "bad", and does not ahve such a problem.

Perhaps if we rewrote it to

	if ((!strcmp(orig_term, "good") &&
             one_of(term, "bad", "new", NULL)) ||
             (!strcmp(orig_term, "bad") &&
             one_of(term, "good", "old", NULL)))

i.e. "If you are using "bad" or "new" to mean "good", that is not a
good idea", as a follow-up change after the dust settles, the result
might be easier to read.

But this is just commenting for future reference, not suggesting to
update this patch at all.  If we were to do the change, that must
come as a separate step.

Thanks.

Re: [PATCH v6 2/3] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

On Fri, May 13, 2016 at 4:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> +     /*
>> +      * In theory, nothing prevents swapping completely good and bad,
>> +      * but this situation could be confusing and hasn't been tested
>> +      * enough. Forbid it for now.
>> +      */
>> +
>> +     if ((strcmp(orig_term, "bad") && one_of(term, "bad", "new", NULL)) ||
>> +              (strcmp(orig_term, "good") && one_of(term, "good", "old", NULL)))
>> +             return error(_("can't change the meaning of the term '%s'"), term);
>
> The above comes from below
>
>> -     bad|new)
>> -             if test "$2" != bad
>> -             then
>> -                     die "$(eval_gettext "can't change the meaning ...
>
> So it is not your fault, but it is quite hard to understand.
>
> The original says "You are trying to use 'bad' (or 'new') for
> something that is not 'bad'--which is confusing; do not do it".
>
> I _think_ the reason I find C version harder to read is the use of
> strcmp(orig_term, "bad") to say "orig_term is not BAD".  The shell
> version visually tells with "!=" that the meaning of the new term is
> *NOT* "bad", and does not ahve such a problem.
>
> Perhaps if we rewrote it to
>
>         if ((!strcmp(orig_term, "good") &&
>              one_of(term, "bad", "new", NULL)) ||
>              (!strcmp(orig_term, "bad") &&
>              one_of(term, "good", "old", NULL)))
>
> i.e. "If you are using "bad" or "new" to mean "good", that is not a
> good idea", as a follow-up change after the dust settles, the result
> might be easier to read.

Not just that, it would also be fundamentally more correct as there is
a difference between " !strcmp("good") " and " strcmp("bad") ". Yours
approach suggests that it should be "good" specifically which is
technically more correct as then only it would be sensible to check
whether it is trying to change the meaning of the term.

> But this is just commenting for future reference, not suggesting to
> update this patch at all.  If we were to do the change, that must
> come as a separate step.
>
> Thanks.

Will do this change in a separate patch after the dust settles.

Regards,
Pranit Bauva

Re: [PATCH v6 2/3] bisect: rewrite `check_term_format` shell function in C

[Junio C Hamano]

Pranit Bauva <pranit.bauva@gmail.com> writes:

> Not just that, it would also be fundamentally more correct as there is
> a difference between " !strcmp("good") " and " strcmp("bad") ".

Not really.  As long as you are in tight control of the callers,
because we'd never call $2 with anything other than good and bad in
the original, there is no deep fundmental difference between them.

It is just "test bad != $2" makes it more clear that we are checking
if $2 is not 'bad', but strcmp(orig_term, "bad") doesn't do so as
strongly as that.

> Will do this change in a separate patch after the dust settles.

Thanks.

[PATCH v6 3/3] bisect--helper: `write_terms` shell function in C

[Pranit Bauva]

Reimplement the `write_terms` shell function in C and add a `write-terms`
subcommand to `git bisect--helper` to call it from git-bisect.sh . Also
remove the subcommand `--check-term-format` as it can now be called from
inside the function write_terms() C implementation.

Using `--write-terms` subcommand is a temporary measure to port shell
function to 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 method.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 37 ++++++++++++++++++++++++++++++-------
 git-bisect.sh            | 22 +++++++---------------
 2 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3c748d1..2b21c02 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -6,7 +6,7 @@
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
-	N_("git bisect--helper --check-term-format <term> <orig_term>"),
+	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
 	NULL
 };
 
@@ -56,18 +56,41 @@ static int check_term_format(const char *term, const char *orig_term)
 	return 0;
 }
 
+int write_terms(const char *bad, const char *good)
+{
+	struct strbuf content = STRBUF_INIT;
+	FILE *fp;
+	int res;
+
+	if (!strcmp(bad, good))
+		return error(_("please use two different terms"));
+
+	if (check_term_format(bad, "bad") || check_term_format(good, "good"))
+		return -1;
+
+	strbuf_addf(&content, "%s\n%s\n", bad, good);
+	fp = fopen(".git/BISECT_TERMS", "w");
+	if (!fp){
+		strbuf_release(&content);
+		die_errno(_("could not open the file to read terms"));
+	}
+	res = strbuf_write(&content, fp);
+	fclose(fp);
+	strbuf_release(&content);
+	return (res < 0) ? -1 : 0;
+}
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
-		CHECK_TERM_FMT
+		WRITE_TERMS
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
-		OPT_CMDMODE(0, "check-term-format", &cmdmode,
-			 N_("check format of the term"), CHECK_TERM_FMT),
+		OPT_CMDMODE(0, "write-terms", &cmdmode,
+			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -82,10 +105,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	switch (cmdmode) {
 	case NEXT_ALL:
 		return bisect_next_all(prefix, no_checkout);
-	case CHECK_TERM_FMT:
+	case WRITE_TERMS:
 		if (argc != 2)
-			die(_("--check-term-format requires two arguments"));
-		return check_term_format(argv[0], argv[1]);
+			die(_("--write-terms requires two arguments"));
+		return write_terms(argv[0], argv[1]);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 7d7965d..2dd7ec5 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -210,7 +210,7 @@ bisect_start() {
 	eval "$eval true" &&
 	if test $must_write_terms -eq 1
 	then
-		write_terms "$TERM_BAD" "$TERM_GOOD"
+		git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD"
 	fi &&
 	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
 	#
@@ -557,18 +557,6 @@ get_terms () {
 	fi
 }
 
-write_terms () {
-	TERM_BAD=$1
-	TERM_GOOD=$2
-	if test "$TERM_BAD" = "$TERM_GOOD"
-	then
-		die "$(gettext "please use two different terms")"
-	fi
-	git bisect--helper --check-term-format "$TERM_BAD" bad || exit
-	git bisect--helper --check-term-format "$TERM_GOOD" good || exit
-	printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
-}
-
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
@@ -582,13 +570,17 @@ check_and_set_terms () {
 		bad|good)
 			if ! test -s "$GIT_DIR/BISECT_TERMS"
 			then
-				write_terms bad good
+				TERM_BAD=bad
+				TERM_GOOD=good
+				git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD"
 			fi
 			;;
 		new|old)
 			if ! test -s "$GIT_DIR/BISECT_TERMS"
 			then
-				write_terms new old
+				TERM_BAD=new
+				TERM_GOOD=old
+				git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD"
 			fi
 			;;
 		esac ;;
-- 
2.8.2

Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C

[Eric Sunshine]

On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Reimplement the `write_terms` shell function in C and add a `write-terms`
> subcommand to `git bisect--helper` to call it from git-bisect.sh . Also
> remove the subcommand `--check-term-format` as it can now be called from
> inside the function write_terms() C implementation.
>
> Using `--write-terms` subcommand is a temporary measure to port shell
> function to 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 method.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -56,18 +56,41 @@ static int check_term_format(const char *term, const char *orig_term)
> +int write_terms(const char *bad, const char *good)

s/^/static/

> +{
> +       struct strbuf content = STRBUF_INIT;
> +       FILE *fp;
> +       int res;
> +
> +       if (!strcmp(bad, good))
> +               return error(_("please use two different terms"));
> +
> +       if (check_term_format(bad, "bad") || check_term_format(good, "good"))
> +               return -1;
> +
> +       strbuf_addf(&content, "%s\n%s\n", bad, good);

What's the point of the strbuf when you could more easily emit this
same content directly to the file via:

    fprintf(fp, "%s\n%s\n", bad, good);

> +       fp = fopen(".git/BISECT_TERMS", "w");

Hardcoding ".git/" is wrong for a variety of reasons: It won't be
correct with linked worktrees, or when GIT_DIR is pointing elsewhere,
or when ".git" is a symbolic link, etc. Check out the get_git_dir(),
get_git_common_dir(), etc. functions in cache.h instead.

> +       if (!fp){

Style: space before '{'

> +               strbuf_release(&content);
> +               die_errno(_("could not open the file to read terms"));

"read terms"? I thought this was writing.

Is dying here correct? I thought we established previously that you
should be reporting failure via normal -1 return value rather than
dying. Indeed, you're doing so below when strbuf_write() fails.

> +       }
> +       res = strbuf_write(&content, fp);
> +       fclose(fp);
> +       strbuf_release(&content);
> +       return (res < 0) ? -1 : 0;
> +}
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)

Style: insert blank line between functions

Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C

[Johannes Schindelin]

Hi,

On Mon, 16 May 2016, Eric Sunshine wrote:

> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> 
> > +       fp = fopen(".git/BISECT_TERMS", "w");
> 
> Hardcoding ".git/" is wrong for a variety of reasons: It won't be correct
> with linked worktrees, or when GIT_DIR is pointing elsewhere, or when ".git"
> is a symbolic link, etc. Check out the get_git_dir(), get_git_common_dir(),
> etc. functions in cache.h instead.

Maybe in this case, `git_path("BISECT_TERMS")` would be a good idea. Or even
better: follow the example of bisect.c and use
`GIT_PATH_FUNC(bisect_terms_path, "BISECT_TERMS")`.

> > +               strbuf_release(&content);
> > +               die_errno(_("could not open the file to read terms"));
> 
> "read terms"? I thought this was writing.
> 
> Is dying here correct? I thought we established previously that you
> should be reporting failure via normal -1 return value rather than
> dying. Indeed, you're doing so below when strbuf_write() fails.

The rule of thumb seems to be that die()ing is okay in builtin/*.c, but not
in *.c. So technically, it would be okay here, too. However, I think that
this code should be written with libification in mind, so I would also
prefer it to error() and return, to give the caller a chance to do other
things after an error occurred.

Ciao,
Dscho

Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C

[Eric Sunshine]

On Mon, May 16, 2016 at 9:16 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Mon, 16 May 2016, Eric Sunshine wrote:
>> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> > +       fp = fopen(".git/BISECT_TERMS", "w");
>>
>> Hardcoding ".git/" is wrong for a variety of reasons: It won't be correct
>> with linked worktrees, or when GIT_DIR is pointing elsewhere, or when ".git"
>> is a symbolic link, etc. Check out the get_git_dir(), get_git_common_dir(),
>> etc. functions in cache.h instead.
>
> Maybe in this case, `git_path("BISECT_TERMS")` would be a good idea. Or even
> better: follow the example of bisect.c and use
> `GIT_PATH_FUNC(bisect_terms_path, "BISECT_TERMS")`.

Thanks for pointing this out. My review time is severely limited these
days so I didn't go the distance of re-studying and re-digesting which
function was the correct one to use.

>> > +               strbuf_release(&content);
>> > +               die_errno(_("could not open the file to read terms"));
>>
>> Is dying here correct? I thought we established previously that you
>> should be reporting failure via normal -1 return value rather than
>> dying. Indeed, you're doing so below when strbuf_write() fails.
>
> The rule of thumb seems to be that die()ing is okay in builtin/*.c, but not
> in *.c. So technically, it would be okay here, too. However, I think that
> this code should be written with libification in mind, so I would also
> prefer it to error() and return, to give the caller a chance to do other
> things after an error occurred.

Agreed. Specific to the "established previously" I wrote above, I was
referring to [1] from just a few days ago which explained why 'return
-1' was preferable to die() in a similar case that had an odd mix of
'return -1' and die() in the same function.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/289476/focus=293556

Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C

[Johannes Schindelin]

Hi Eric,

On Mon, 16 May 2016, Eric Sunshine wrote:

> On Mon, May 16, 2016 at 9:16 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Mon, 16 May 2016, Eric Sunshine wrote:
> >> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> >> > +       fp = fopen(".git/BISECT_TERMS", "w");
> >>
> >> Hardcoding ".git/" is wrong for a variety of reasons: It won't be correct
> >> with linked worktrees, or when GIT_DIR is pointing elsewhere, or when ".git"
> >> is a symbolic link, etc. Check out the get_git_dir(), get_git_common_dir(),
> >> etc. functions in cache.h instead.
> >
> > Maybe in this case, `git_path("BISECT_TERMS")` would be a good idea. Or even
> > better: follow the example of bisect.c and use
> > `GIT_PATH_FUNC(bisect_terms_path, "BISECT_TERMS")`.
> 
> Thanks for pointing this out. My review time is severely limited these
> days so I didn't go the distance of re-studying and re-digesting which
> function was the correct one to use.

I am constantly amazed how much you manage to review, and how helpful your
comments are. I am glad if I could contribute a little.

> >> > +               strbuf_release(&content);
> >> > +               die_errno(_("could not open the file to read terms"));
> >>
> >> Is dying here correct? I thought we established previously that you
> >> should be reporting failure via normal -1 return value rather than
> >> dying. Indeed, you're doing so below when strbuf_write() fails.
> >
> > The rule of thumb seems to be that die()ing is okay in builtin/*.c, but not
> > in *.c. So technically, it would be okay here, too. However, I think that
> > this code should be written with libification in mind, so I would also
> > prefer it to error() and return, to give the caller a chance to do other
> > things after an error occurred.
> 
> Agreed. Specific to the "established previously" I wrote above, I was
> referring to [1] from just a few days ago which explained why 'return
> -1' was preferable to die() in a similar case that had an odd mix of
> 'return -1' and die() in the same function.
> 
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/289476/focus=293556

Yes, this is an excellent link. Maybe we would want something like this,
too?

-- snipsnap --
(From: Eric Sunshine <sunshine@sunshineco.com>)
CodingGuidelines: mention that die() is for fatal errors only

Let's formalize the rule when to use die() and when to use error().

diff --git a/Documentation/CodingGuidelines
b/Documentation/CodingGuidelines
index 0ddd368..5d2d65f 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -333,6 +333,10 @@ For C programs:
 
  - When you come up with an API, document it.
 
+ - Use die() only to signal an exceptional conditions which should
+   abort the program. All other error conditions should instead
+   return e.g. using "return error(...)".
+
  - The first #include in C files, except in platform specific compat/
    implementations, must be either "git-compat-util.h", "cache.h" or
    "builtin.h".  You do not have to include more than one of these.

Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C

[Eric Sunshine]

On Mon, May 16, 2016 at 12:45 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Mon, 16 May 2016, Eric Sunshine wrote:
>> On Mon, May 16, 2016 at 9:16 AM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> > On Mon, 16 May 2016, Eric Sunshine wrote:
>> Agreed. Specific to the "established previously" I wrote above, I was
>> referring to [1] from just a few days ago which explained why 'return
>> -1' was preferable to die() in a similar case that had an odd mix of
>> 'return -1' and die() in the same function.
>>
>> [1]: http://thread.gmane.org/gmane.comp.version-control.git/289476/focus=293556
>
> Yes, this is an excellent link. Maybe we would want something like this,
> too?
>
> -- snipsnap --
> diff --git a/Documentation/CodingGuidelines
> @@ -333,6 +333,10 @@ For C programs:
> + - Use die() only to signal an exceptional conditions which should
> +   abort the program. All other error conditions should instead
> +   return e.g. using "return error(...)".

Hmm, the bit you said earlier about "library" code vs. end-user code
would be important, too, I'd think. Also, I'd expect that this sort of
thing is implicitly understood by most programmers, and as this is
only the first time (in recent memory) that it has needed explanation
on the mailing list, I'm not sure that it deserves explicit mention in
CodingGuidelines (but perhaps I'm not a good judge of these things).

Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C

[Pranit Bauva]

Hey Johannes,

On Mon, May 16, 2016 at 6:46 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Mon, 16 May 2016, Eric Sunshine wrote:
>
>> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>
>> > +       fp = fopen(".git/BISECT_TERMS", "w");
>>
>> Hardcoding ".git/" is wrong for a variety of reasons: It won't be correct
>> with linked worktrees, or when GIT_DIR is pointing elsewhere, or when ".git"
>> is a symbolic link, etc. Check out the get_git_dir(), get_git_common_dir(),
>> etc. functions in cache.h instead.
>
> Maybe in this case, `git_path("BISECT_TERMS")` would be a good idea. Or even
> better: follow the example of bisect.c and use
> `GIT_PATH_FUNC(bisect_terms_path, "BISECT_TERMS")`.

Thanks. I will look into this.


>> > +               strbuf_release(&content);
>> > +               die_errno(_("could not open the file to read terms"));
>>
>> "read terms"? I thought this was writing.
>>
>> Is dying here correct? I thought we established previously that you
>> should be reporting failure via normal -1 return value rather than
>> dying. Indeed, you're doing so below when strbuf_write() fails.
>
> The rule of thumb seems to be that die()ing is okay in builtin/*.c, but not
> in *.c. So technically, it would be okay here, too. However, I think that
> this code should be written with libification in mind, so I would also
> prefer it to error() and return, to give the caller a chance to do other
> things after an error occurred.

Frankly, I have no idea what you mean by libification but I will use
error() since efforts for libification have already started.

Regards,
Pranit Bauva

> Ciao,
> Dscho

Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C

[Johannes Schindelin]

Hi Pranit,

On Fri, 20 May 2016, Pranit Bauva wrote:

> Frankly, I have no idea what you mean by libification but I will use
> error() since efforts for libification have already started.

The term "libification" is frequently used in the Git project: it means to
convert code that was formerly used in a standalone program to make it
usable as a library function.

The main differences:

- in the standalone program, you can get away with calling die() when
  anything goes wrong. Not so in a library function: you need to give the
  caller a chance to fail gracefully, or even to continue.

- in a standalone program, you can be sloppy and "let exit() clean up",
  i.e. omit free() calls or even close() calls. In library functions, this
  is not possible.

- in a standalone program, you may mess with the stdio handles. This is
  an absolute no-go in library functions.

There are other differences, of course. The gist is that you have to be a
lot more careful for library functions.

Ciao,
Johannes

Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C

[Christian Couder]

On Mon, May 23, 2016 at 1:07 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> On Fri, 20 May 2016, Pranit Bauva wrote:
>
>> Frankly, I have no idea what you mean by libification but I will use
>> error() since efforts for libification have already started.
>
> The term "libification" is frequently used in the Git project: it means to
> convert code that was formerly used in a standalone program to make it
> usable as a library function.

Yeah, and Git standalone programs in C are builtin/*.c files, while
Git lib code in C is in *.{c,h} files at the root of the repo.

The build also generates libgit.a from all the lib code, for example:

$ touch bisect.c
$ make CFLAGS="-g3 -Wall"
    CC bisect.o
    AR libgit.a
...

Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C

[Johannes Schindelin]

Hi Christian,

On Mon, 23 May 2016, Christian Couder wrote:

> On Mon, May 23, 2016 at 1:07 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Fri, 20 May 2016, Pranit Bauva wrote:
> >
> >> Frankly, I have no idea what you mean by libification but I will use
> >> error() since efforts for libification have already started.
> >
> > The term "libification" is frequently used in the Git project: it means to
> > convert code that was formerly used in a standalone program to make it
> > usable as a library function.
> 
> Yeah, and Git standalone programs in C are builtin/*.c files,

Do not forget the non-builtins, e.g. git-upload-pack. And scripts.

Ciao,
Johannes

Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C

[Pranit Bauva]

Hey Johannes,

On Mon, May 23, 2016 at 4:37 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Pranit,
>
> On Fri, 20 May 2016, Pranit Bauva wrote:
>
>> Frankly, I have no idea what you mean by libification but I will use
>> error() since efforts for libification have already started.
>
> The term "libification" is frequently used in the Git project: it means to
> convert code that was formerly used in a standalone program to make it
> usable as a library function.
>
> The main differences:
>
> - in the standalone program, you can get away with calling die() when
>   anything goes wrong. Not so in a library function: you need to give the
>   caller a chance to fail gracefully, or even to continue.
>
> - in a standalone program, you can be sloppy and "let exit() clean up",
>   i.e. omit free() calls or even close() calls. In library functions, this
>   is not possible.
>
> - in a standalone program, you may mess with the stdio handles. This is
>   an absolute no-go in library functions.
>
> There are other differences, of course. The gist is that you have to be a
> lot more careful for library functions.

Thanks for the explanation. It seems a bit clearer to me now.

Regards,
Pranit Bauva

>
> Ciao,
> Johannes

Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C

[Pranit Bauva]

Hey Eric,

On Mon, May 16, 2016 at 12:58 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Reimplement the `write_terms` shell function in C and add a `write-terms`
>> subcommand to `git bisect--helper` to call it from git-bisect.sh . Also
>> remove the subcommand `--check-term-format` as it can now be called from
>> inside the function write_terms() C implementation.
>>
>> Using `--write-terms` subcommand is a temporary measure to port shell
>> function to 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 method.
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -56,18 +56,41 @@ static int check_term_format(const char *term, const char *orig_term)
>> +int write_terms(const char *bad, const char *good)
>
> s/^/static/

Sure. Will include this in re-roll.

>> +{
>> +       struct strbuf content = STRBUF_INIT;
>> +       FILE *fp;
>> +       int res;
>> +
>> +       if (!strcmp(bad, good))
>> +               return error(_("please use two different terms"));
>> +
>> +       if (check_term_format(bad, "bad") || check_term_format(good, "good"))
>> +               return -1;
>> +
>> +       strbuf_addf(&content, "%s\n%s\n", bad, good);
>
> What's the point of the strbuf when you could more easily emit this
> same content directly to the file via:
>
>     fprintf(fp, "%s\n%s\n", bad, good);

fprintf seems way better and it is also extensively used in git's
source code. Will update.

>> +       fp = fopen(".git/BISECT_TERMS", "w");
>
> Hardcoding ".git/" is wrong for a variety of reasons: It won't be
> correct with linked worktrees, or when GIT_DIR is pointing elsewhere,
> or when ".git" is a symbolic link, etc. Check out the get_git_dir(),
> get_git_common_dir(), etc. functions in cache.h instead.
>
>> +       if (!fp){
>
> Style: space before '{'

Sorry. Might have missed this. Will include this in re-roll

>> +               strbuf_release(&content);
>> +               die_errno(_("could not open the file to read terms"));
>
> "read terms"? I thought this was writing.

Ya. It should be "write terms".

> Is dying here correct? I thought we established previously that you
> should be reporting failure via normal -1 return value rather than
> dying. Indeed, you're doing so below when strbuf_write() fails.

I was confused about this. I thought not able to open a file is an
exceptional situation (because it has never happened with me) and thus
I used die(). I will use error() as further justified by Johannes.

>> +       }
>> +       res = strbuf_write(&content, fp);
>> +       fclose(fp);
>> +       strbuf_release(&content);
>> +       return (res < 0) ? -1 : 0;
>> +}
>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>
> Style: insert blank line between functions

Sure.

Regards,
Pranit Bauva

[PATCH v7 0/3] bisect--helper: check_term_format() & write_terms()

[Pranit Bauva]

Here is the link to v6[1]. I have kept bisect_log() and bisect_voc() on hold
right now. It would be sent to the list whenever it is required which would
probably be during the end.

Important Changes wrt v6:

 * make the method write_terms() static

 * Use fprintf() instead of strbuf functions

 * Previously ".git/" was hardcoded. Now it uses GIT_PATH_FUNC

 * Use error() instead of die.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/294388
Pranit Bauva (3):
  bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
  bisect: rewrite `check_term_format` shell function in C
  bisect--helper: `write_terms` shell function in C

 builtin/bisect--helper.c | 96 +++++++++++++++++++++++++++++++++++++++++++++---
 git-bisect.sh            | 49 ++++--------------------
 2 files changed, 97 insertions(+), 48 deletions(-)

-- 
2.8.2

[PATCH v7 1/3] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

`--next-all` is meant to be used as a subcommand to support multiple
"operation mode" though the current implementation does not contain any
other subcommand along side with `--next-all` but further commits will
include some more subcommands.

Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229..8111c91 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -10,11 +10,11 @@ static const char * const git_bisect_helper_usage[] = {
 
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-	int next_all = 0;
+	enum { NEXT_ALL = 1 } cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
-		OPT_BOOL(0, "next-all", &next_all,
-			 N_("perform 'git bisect next'")),
+		OPT_CMDMODE(0, "next-all", &cmdmode,
+			 N_("perform 'git bisect next'"), NEXT_ALL),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_bisect_helper_usage, 0);
 
-	if (!next_all)
+	if (!cmdmode)
 		usage_with_options(git_bisect_helper_usage, options);
 
-	/* next-all */
-	return bisect_next_all(prefix, no_checkout);
+	switch (cmdmode) {
+	case NEXT_ALL:
+		return bisect_next_all(prefix, no_checkout);
+	default:
+		die("BUG: unknown subcommand '%d'", cmdmode);
+	}
+	return 0;
 }
-- 
2.8.2

[PATCH v7 2/3] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

Reimplement the `check_term_format` shell function in C and add
a `--check-term-format` subcommand to `git bisect--helper` to call it
from git-bisect.sh

Using `--check-term-format` subcommand is a temporary measure to port
shell function to 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 method/subcommand. For eg. In conversion of
write_terms() of git-bisect.sh, the subcommand will be removed and
instead check_term_format() will be called in its C implementation while
a new subcommand will be introduced for write_terms().

Helped-by: Johannes Schindelein <Johannes.Schindelein@gmx.de>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 31 ++-----------------------
 2 files changed, 60 insertions(+), 30 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 8111c91..3c748d1 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -2,19 +2,72 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "bisect.h"
+#include "refs.h"
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
+	N_("git bisect--helper --check-term-format <term> <orig_term>"),
 	NULL
 };
 
+/*
+ * Check whether the string `term` belongs to the set of strings
+ * included in the variable arguments.
+ */
+static int one_of(const char *term, ...)
+{
+	int res = 0;
+	va_list matches;
+	const char *match;
+
+	va_start(matches, term);
+	while (!res && (match = va_arg(matches, const char *)))
+		res = !strcmp(term, match);
+	va_end(matches);
+
+	return res;
+}
+
+static int check_term_format(const char *term, const char *orig_term)
+{
+	struct strbuf new_term = STRBUF_INIT;
+	strbuf_addf(&new_term, "refs/bisect/%s", term);
+
+	if (check_refname_format(new_term.buf, 0)) {
+		strbuf_release(&new_term);
+		return error(_("'%s' is not a valid term"), term);
+	}
+	strbuf_release(&new_term);
+
+	if (one_of(term, "help", "start", "skip", "next", "reset",
+			"visualize", "replay", "log", "run", NULL))
+		return error(_("can't use the builtin command '%s' as a term"), term);
+
+	/*
+	 * In theory, nothing prevents swapping completely good and bad,
+	 * but this situation could be confusing and hasn't been tested
+	 * enough. Forbid it for now.
+	 */
+
+	if ((strcmp(orig_term, "bad") && one_of(term, "bad", "new", NULL)) ||
+		 (strcmp(orig_term, "good") && one_of(term, "good", "old", NULL)))
+		return error(_("can't change the meaning of the term '%s'"), term);
+
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-	enum { NEXT_ALL = 1 } cmdmode = 0;
+	enum {
+		NEXT_ALL = 1,
+		CHECK_TERM_FMT
+	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
+		OPT_CMDMODE(0, "check-term-format", &cmdmode,
+			 N_("check format of the term"), CHECK_TERM_FMT),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -29,6 +82,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	switch (cmdmode) {
 	case NEXT_ALL:
 		return bisect_next_all(prefix, no_checkout);
+	case CHECK_TERM_FMT:
+		if (argc != 2)
+			die(_("--check-term-format requires two arguments"));
+		return check_term_format(argv[0], argv[1]);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 5d1cb00..7d7965d 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -564,38 +564,11 @@ write_terms () {
 	then
 		die "$(gettext "please use two different terms")"
 	fi
-	check_term_format "$TERM_BAD" bad
-	check_term_format "$TERM_GOOD" good
+	git bisect--helper --check-term-format "$TERM_BAD" bad || exit
+	git bisect--helper --check-term-format "$TERM_GOOD" good || exit
 	printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
 }
 
-check_term_format () {
-	term=$1
-	git check-ref-format refs/bisect/"$term" ||
-	die "$(eval_gettext "'\$term' is not a valid term")"
-	case "$term" in
-	help|start|terms|skip|next|reset|visualize|replay|log|run)
-		die "$(eval_gettext "can't use the builtin command '\$term' as a term")"
-		;;
-	bad|new)
-		if test "$2" != bad
-		then
-			# In theory, nothing prevents swapping
-			# completely good and bad, but this situation
-			# could be confusing and hasn't been tested
-			# enough. Forbid it for now.
-			die "$(eval_gettext "can't change the meaning of term '\$term'")"
-		fi
-		;;
-	good|old)
-		if test "$2" != good
-		then
-			die "$(eval_gettext "can't change the meaning of term '\$term'")"
-		fi
-		;;
-	esac
-}
-
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
-- 
2.8.2

[PATCH v7 3/3] bisect--helper: `write_terms` shell function in C

[Pranit Bauva]

Reimplement the `write_terms` shell function in C and add a `write-terms`
subcommand to `git bisect--helper` to call it from git-bisect.sh . Also
remove the subcommand `--check-term-format` as it can now be called from
inside the function write_terms() C implementation.

Using `--write-terms` subcommand is a temporary measure to port shell
function to 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 method.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 36 +++++++++++++++++++++++++++++-------
 git-bisect.sh            | 22 +++++++---------------
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3c748d1..8c7a5fc 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -4,9 +4,11 @@
 #include "bisect.h"
 #include "refs.h"
 
+static GIT_PATH_FUNC(git_path_bisect_write_terms, "BISECT_TERMS")
+
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
-	N_("git bisect--helper --check-term-format <term> <orig_term>"),
+	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
 	NULL
 };
 
@@ -56,18 +58,38 @@ static int check_term_format(const char *term, const char *orig_term)
 	return 0;
 }
 
+static int write_terms(const char *bad, const char *good)
+{
+	FILE *fp;
+	int res;
+
+	if (!strcmp(bad, good))
+		return error(_("please use two different terms"));
+
+	if (check_term_format(bad, "bad") || check_term_format(good, "good"))
+		return -1;
+
+	fp = fopen(git_path_bisect_write_terms(), "w");
+	if (!fp)
+		return error(_("could not open the file to write terms"));
+
+	res = fprintf(fp, "%s\n%s\n", bad, good);
+	fclose(fp);
+	return (res < 0) ? -1 : 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
-		CHECK_TERM_FMT
+		WRITE_TERMS
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
-		OPT_CMDMODE(0, "check-term-format", &cmdmode,
-			 N_("check format of the term"), CHECK_TERM_FMT),
+		OPT_CMDMODE(0, "write-terms", &cmdmode,
+			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -82,10 +104,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	switch (cmdmode) {
 	case NEXT_ALL:
 		return bisect_next_all(prefix, no_checkout);
-	case CHECK_TERM_FMT:
+	case WRITE_TERMS:
 		if (argc != 2)
-			die(_("--check-term-format requires two arguments"));
-		return check_term_format(argv[0], argv[1]);
+			die(_("--write-terms requires two arguments"));
+		return write_terms(argv[0], argv[1]);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 7d7965d..2dd7ec5 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -210,7 +210,7 @@ bisect_start() {
 	eval "$eval true" &&
 	if test $must_write_terms -eq 1
 	then
-		write_terms "$TERM_BAD" "$TERM_GOOD"
+		git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD"
 	fi &&
 	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
 	#
@@ -557,18 +557,6 @@ get_terms () {
 	fi
 }
 
-write_terms () {
-	TERM_BAD=$1
-	TERM_GOOD=$2
-	if test "$TERM_BAD" = "$TERM_GOOD"
-	then
-		die "$(gettext "please use two different terms")"
-	fi
-	git bisect--helper --check-term-format "$TERM_BAD" bad || exit
-	git bisect--helper --check-term-format "$TERM_GOOD" good || exit
-	printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
-}
-
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
@@ -582,13 +570,17 @@ check_and_set_terms () {
 		bad|good)
 			if ! test -s "$GIT_DIR/BISECT_TERMS"
 			then
-				write_terms bad good
+				TERM_BAD=bad
+				TERM_GOOD=good
+				git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD"
 			fi
 			;;
 		new|old)
 			if ! test -s "$GIT_DIR/BISECT_TERMS"
 			then
-				write_terms new old
+				TERM_BAD=new
+				TERM_GOOD=old
+				git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD"
 			fi
 			;;
 		esac ;;
-- 
2.8.2

Re: [PATCH v7 3/3] bisect--helper: `write_terms` shell function in C

[Eric Sunshine]

On Mon, May 23, 2016 at 10:48 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Reimplement the `write_terms` shell function in C and add a `write-terms`
> subcommand to `git bisect--helper` to call it from git-bisect.sh . Also
> remove the subcommand `--check-term-format` as it can now be called from
> inside the function write_terms() C implementation.
> [...]
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -56,18 +58,38 @@ static int check_term_format(const char *term, const char *orig_term)
> +static int write_terms(const char *bad, const char *good)
> +{
> +       FILE *fp;
> +       int res;
> +
> +       if (!strcmp(bad, good))
> +               return error(_("please use two different terms"));
> +
> +       if (check_term_format(bad, "bad") || check_term_format(good, "good"))
> +               return -1;
> +
> +       fp = fopen(git_path_bisect_write_terms(), "w");
> +       if (!fp)
> +               return error(_("could not open the file to write terms"));

By adding two pieces of information, you could make this error message
much more helpful for anyone trying to debug the failure.
Specifically:

(1) the pathname for which open() failed
(2) the actual system-level error

For (2), you could use strerror(errno) or, better yet, error_errno()
which recently graduated to 'master'.

> +       res = fprintf(fp, "%s\n%s\n", bad, good);
> +       fclose(fp);
> +       return (res < 0) ? -1 : 0;
> +}

Re: [PATCH v7 3/3] bisect--helper: `write_terms` shell function in C

[Pranit Bauva]

On Mon, May 23, 2016 at 9:31 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, May 23, 2016 at 10:48 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Reimplement the `write_terms` shell function in C and add a `write-terms`
>> subcommand to `git bisect--helper` to call it from git-bisect.sh . Also
>> remove the subcommand `--check-term-format` as it can now be called from
>> inside the function write_terms() C implementation.
>> [...]
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -56,18 +58,38 @@ static int check_term_format(const char *term, const char *orig_term)
>> +static int write_terms(const char *bad, const char *good)
>> +{
>> +       FILE *fp;
>> +       int res;
>> +
>> +       if (!strcmp(bad, good))
>> +               return error(_("please use two different terms"));
>> +
>> +       if (check_term_format(bad, "bad") || check_term_format(good, "good"))
>> +               return -1;
>> +
>> +       fp = fopen(git_path_bisect_write_terms(), "w");
>> +       if (!fp)
>> +               return error(_("could not open the file to write terms"));
>
> By adding two pieces of information, you could make this error message
> much more helpful for anyone trying to debug the failure.
> Specifically:
>
> (1) the pathname for which open() failed
> (2) the actual system-level error
>
> For (2), you could use strerror(errno) or, better yet, error_errno()
> which recently graduated to 'master'.

Yes it would definitely be better to make the error message more
helpful. I will dig more into this.

>> +       res = fprintf(fp, "%s\n%s\n", bad, good);
>> +       fclose(fp);
>> +       return (res < 0) ? -1 : 0;
>> +}

[PATCH v8 0/3] bisect--helper: check-term-format() & write_terms()

[Pranit Bauva]

Here is the link to v7[1]. This has a few modifications.

Changes wrt v7 as suggested by Eric are:

 * Include the name of the file in the error message which couldn't be opened.

 * Use error_errno() with which we can make the error message more valuable.

Pranit Bauva (3):
  bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
  bisect: rewrite `check_term_format` shell function in C
  bisect--helper: `write_terms` shell function in C

 builtin/bisect--helper.c | 96 +++++++++++++++++++++++++++++++++++++++++++++---
 git-bisect.sh            | 49 ++++--------------------
 2 files changed, 97 insertions(+), 48 deletions(-)

-- 
2.8.3

[PATCH v9 0/3] bisect--helper: check_term_format() and write_terms()

[Pranit Bauva]

Here is the link to v8[1]. This has a few modifications.

Changes wrt v8 as suggested by Christian are:

 * Remove the redundant `|| exit`

Pranit Bauva (3):
  bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
  bisect: rewrite `check_term_format` shell function in C
  bisect--helper: `write_terms` shell function in C

 builtin/bisect--helper.c | 96 +++++++++++++++++++++++++++++++++++++++++++++---
 git-bisect.sh            | 49 ++++--------------------
 2 files changed, 97 insertions(+), 48 deletions(-)

-- 
2.8.3

[PATCH v9 1/3] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

`--next-all` is meant to be used as a subcommand to support multiple
"operation mode" though the current implementation does not contain any
other subcommand along side with `--next-all` but further commits will
include some more subcommands.

Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229..8111c91 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -10,11 +10,11 @@ static const char * const git_bisect_helper_usage[] = {
 
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-	int next_all = 0;
+	enum { NEXT_ALL = 1 } cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
-		OPT_BOOL(0, "next-all", &next_all,
-			 N_("perform 'git bisect next'")),
+		OPT_CMDMODE(0, "next-all", &cmdmode,
+			 N_("perform 'git bisect next'"), NEXT_ALL),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_bisect_helper_usage, 0);
 
-	if (!next_all)
+	if (!cmdmode)
 		usage_with_options(git_bisect_helper_usage, options);
 
-	/* next-all */
-	return bisect_next_all(prefix, no_checkout);
+	switch (cmdmode) {
+	case NEXT_ALL:
+		return bisect_next_all(prefix, no_checkout);
+	default:
+		die("BUG: unknown subcommand '%d'", cmdmode);
+	}
+	return 0;
 }
-- 
2.8.3

[PATCH v9 2/3] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

Reimplement the `check_term_format` shell function in C and add
a `--check-term-format` subcommand to `git bisect--helper` to call it
from git-bisect.sh

Using `--check-term-format` subcommand is a temporary measure to port
shell function to 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 method/subcommand. For eg. In conversion of
write_terms() of git-bisect.sh, the subcommand will be removed and
instead check_term_format() will be called in its C implementation while
a new subcommand will be introduced for write_terms().

Helped-by: Johannes Schindelein <Johannes.Schindelein@gmx.de>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 31 ++-----------------------
 2 files changed, 60 insertions(+), 30 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 8111c91..3c748d1 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -2,19 +2,72 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "bisect.h"
+#include "refs.h"
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
+	N_("git bisect--helper --check-term-format <term> <orig_term>"),
 	NULL
 };
 
+/*
+ * Check whether the string `term` belongs to the set of strings
+ * included in the variable arguments.
+ */
+static int one_of(const char *term, ...)
+{
+	int res = 0;
+	va_list matches;
+	const char *match;
+
+	va_start(matches, term);
+	while (!res && (match = va_arg(matches, const char *)))
+		res = !strcmp(term, match);
+	va_end(matches);
+
+	return res;
+}
+
+static int check_term_format(const char *term, const char *orig_term)
+{
+	struct strbuf new_term = STRBUF_INIT;
+	strbuf_addf(&new_term, "refs/bisect/%s", term);
+
+	if (check_refname_format(new_term.buf, 0)) {
+		strbuf_release(&new_term);
+		return error(_("'%s' is not a valid term"), term);
+	}
+	strbuf_release(&new_term);
+
+	if (one_of(term, "help", "start", "skip", "next", "reset",
+			"visualize", "replay", "log", "run", NULL))
+		return error(_("can't use the builtin command '%s' as a term"), term);
+
+	/*
+	 * In theory, nothing prevents swapping completely good and bad,
+	 * but this situation could be confusing and hasn't been tested
+	 * enough. Forbid it for now.
+	 */
+
+	if ((strcmp(orig_term, "bad") && one_of(term, "bad", "new", NULL)) ||
+		 (strcmp(orig_term, "good") && one_of(term, "good", "old", NULL)))
+		return error(_("can't change the meaning of the term '%s'"), term);
+
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-	enum { NEXT_ALL = 1 } cmdmode = 0;
+	enum {
+		NEXT_ALL = 1,
+		CHECK_TERM_FMT
+	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
+		OPT_CMDMODE(0, "check-term-format", &cmdmode,
+			 N_("check format of the term"), CHECK_TERM_FMT),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -29,6 +82,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	switch (cmdmode) {
 	case NEXT_ALL:
 		return bisect_next_all(prefix, no_checkout);
+	case CHECK_TERM_FMT:
+		if (argc != 2)
+			die(_("--check-term-format requires two arguments"));
+		return check_term_format(argv[0], argv[1]);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 5d1cb00..7d7965d 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -564,38 +564,11 @@ write_terms () {
 	then
 		die "$(gettext "please use two different terms")"
 	fi
-	check_term_format "$TERM_BAD" bad
-	check_term_format "$TERM_GOOD" good
+	git bisect--helper --check-term-format "$TERM_BAD" bad || exit
+	git bisect--helper --check-term-format "$TERM_GOOD" good || exit
 	printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
 }
 
-check_term_format () {
-	term=$1
-	git check-ref-format refs/bisect/"$term" ||
-	die "$(eval_gettext "'\$term' is not a valid term")"
-	case "$term" in
-	help|start|terms|skip|next|reset|visualize|replay|log|run)
-		die "$(eval_gettext "can't use the builtin command '\$term' as a term")"
-		;;
-	bad|new)
-		if test "$2" != bad
-		then
-			# In theory, nothing prevents swapping
-			# completely good and bad, but this situation
-			# could be confusing and hasn't been tested
-			# enough. Forbid it for now.
-			die "$(eval_gettext "can't change the meaning of term '\$term'")"
-		fi
-		;;
-	good|old)
-		if test "$2" != good
-		then
-			die "$(eval_gettext "can't change the meaning of term '\$term'")"
-		fi
-		;;
-	esac
-}
-
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
-- 
2.8.3

[PATCH v9 3/3] bisect--helper: `write_terms` shell function in C

[Pranit Bauva]

Reimplement the `write_terms` shell function in C and add a `write-terms`
subcommand to `git bisect--helper` to call it from git-bisect.sh . Also
remove the subcommand `--check-term-format` as it can now be called from
inside the function write_terms() C implementation.

Also `|| exit` is added when calling write-terms subcommand from
git-bisect.sh so as to exit whenever there is an error.

Using `--write-terms` subcommand is a temporary measure to port shell
function to 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 method.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 36 +++++++++++++++++++++++++++++-------
 git-bisect.sh            | 22 +++++++---------------
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3c748d1..91027b0 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -4,9 +4,11 @@
 #include "bisect.h"
 #include "refs.h"
 
+static GIT_PATH_FUNC(git_path_bisect_write_terms, "BISECT_TERMS")
+
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
-	N_("git bisect--helper --check-term-format <term> <orig_term>"),
+	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
 	NULL
 };
 
@@ -56,18 +58,38 @@ static int check_term_format(const char *term, const char *orig_term)
 	return 0;
 }
 
+static int write_terms(const char *bad, const char *good)
+{
+	FILE *fp;
+	int res;
+
+	if (!strcmp(bad, good))
+		return error(_("please use two different terms"));
+
+	if (check_term_format(bad, "bad") || check_term_format(good, "good"))
+		return -1;
+
+	fp = fopen(git_path_bisect_write_terms(), "w");
+	if (!fp)
+		return error_errno(_("could not open the file BISECT_TERMS"));
+
+	res = fprintf(fp, "%s\n%s\n", bad, good);
+	fclose(fp);
+	return (res < 0) ? -1 : 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
-		CHECK_TERM_FMT
+		WRITE_TERMS
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
-		OPT_CMDMODE(0, "check-term-format", &cmdmode,
-			 N_("check format of the term"), CHECK_TERM_FMT),
+		OPT_CMDMODE(0, "write-terms", &cmdmode,
+			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -82,10 +104,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	switch (cmdmode) {
 	case NEXT_ALL:
 		return bisect_next_all(prefix, no_checkout);
-	case CHECK_TERM_FMT:
+	case WRITE_TERMS:
 		if (argc != 2)
-			die(_("--check-term-format requires two arguments"));
-		return check_term_format(argv[0], argv[1]);
+			die(_("--write-terms requires two arguments"));
+		return write_terms(argv[0], argv[1]);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 7d7965d..cd39bd0 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -210,7 +210,7 @@ bisect_start() {
 	eval "$eval true" &&
 	if test $must_write_terms -eq 1
 	then
-		write_terms "$TERM_BAD" "$TERM_GOOD"
+		git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD"
 	fi &&
 	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
 	#
@@ -557,18 +557,6 @@ get_terms () {
 	fi
 }
 
-write_terms () {
-	TERM_BAD=$1
-	TERM_GOOD=$2
-	if test "$TERM_BAD" = "$TERM_GOOD"
-	then
-		die "$(gettext "please use two different terms")"
-	fi
-	git bisect--helper --check-term-format "$TERM_BAD" bad || exit
-	git bisect--helper --check-term-format "$TERM_GOOD" good || exit
-	printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
-}
-
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
@@ -582,13 +570,17 @@ check_and_set_terms () {
 		bad|good)
 			if ! test -s "$GIT_DIR/BISECT_TERMS"
 			then
-				write_terms bad good
+				TERM_BAD=bad
+				TERM_GOOD=good
+				git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit
 			fi
 			;;
 		new|old)
 			if ! test -s "$GIT_DIR/BISECT_TERMS"
 			then
-				write_terms new old
+				TERM_BAD=new
+				TERM_GOOD=old
+				git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit
 			fi
 			;;
 		esac ;;
-- 
2.8.3

[PATCH v8 1/3] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

`--next-all` is meant to be used as a subcommand to support multiple
"operation mode" though the current implementation does not contain any
other subcommand along side with `--next-all` but further commits will
include some more subcommands.

Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229..8111c91 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -10,11 +10,11 @@ static const char * const git_bisect_helper_usage[] = {
 
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-	int next_all = 0;
+	enum { NEXT_ALL = 1 } cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
-		OPT_BOOL(0, "next-all", &next_all,
-			 N_("perform 'git bisect next'")),
+		OPT_CMDMODE(0, "next-all", &cmdmode,
+			 N_("perform 'git bisect next'"), NEXT_ALL),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_bisect_helper_usage, 0);
 
-	if (!next_all)
+	if (!cmdmode)
 		usage_with_options(git_bisect_helper_usage, options);
 
-	/* next-all */
-	return bisect_next_all(prefix, no_checkout);
+	switch (cmdmode) {
+	case NEXT_ALL:
+		return bisect_next_all(prefix, no_checkout);
+	default:
+		die("BUG: unknown subcommand '%d'", cmdmode);
+	}
+	return 0;
 }
-- 
2.8.3

[PATCH v8 2/3] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

Reimplement the `check_term_format` shell function in C and add
a `--check-term-format` subcommand to `git bisect--helper` to call it
from git-bisect.sh

Using `--check-term-format` subcommand is a temporary measure to port
shell function to 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 method/subcommand. For eg. In conversion of
write_terms() of git-bisect.sh, the subcommand will be removed and
instead check_term_format() will be called in its C implementation while
a new subcommand will be introduced for write_terms().

Helped-by: Johannes Schindelein <Johannes.Schindelein@gmx.de>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 31 ++-----------------------
 2 files changed, 60 insertions(+), 30 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 8111c91..3c748d1 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -2,19 +2,72 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "bisect.h"
+#include "refs.h"
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
+	N_("git bisect--helper --check-term-format <term> <orig_term>"),
 	NULL
 };
 
+/*
+ * Check whether the string `term` belongs to the set of strings
+ * included in the variable arguments.
+ */
+static int one_of(const char *term, ...)
+{
+	int res = 0;
+	va_list matches;
+	const char *match;
+
+	va_start(matches, term);
+	while (!res && (match = va_arg(matches, const char *)))
+		res = !strcmp(term, match);
+	va_end(matches);
+
+	return res;
+}
+
+static int check_term_format(const char *term, const char *orig_term)
+{
+	struct strbuf new_term = STRBUF_INIT;
+	strbuf_addf(&new_term, "refs/bisect/%s", term);
+
+	if (check_refname_format(new_term.buf, 0)) {
+		strbuf_release(&new_term);
+		return error(_("'%s' is not a valid term"), term);
+	}
+	strbuf_release(&new_term);
+
+	if (one_of(term, "help", "start", "skip", "next", "reset",
+			"visualize", "replay", "log", "run", NULL))
+		return error(_("can't use the builtin command '%s' as a term"), term);
+
+	/*
+	 * In theory, nothing prevents swapping completely good and bad,
+	 * but this situation could be confusing and hasn't been tested
+	 * enough. Forbid it for now.
+	 */
+
+	if ((strcmp(orig_term, "bad") && one_of(term, "bad", "new", NULL)) ||
+		 (strcmp(orig_term, "good") && one_of(term, "good", "old", NULL)))
+		return error(_("can't change the meaning of the term '%s'"), term);
+
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-	enum { NEXT_ALL = 1 } cmdmode = 0;
+	enum {
+		NEXT_ALL = 1,
+		CHECK_TERM_FMT
+	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
+		OPT_CMDMODE(0, "check-term-format", &cmdmode,
+			 N_("check format of the term"), CHECK_TERM_FMT),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -29,6 +82,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	switch (cmdmode) {
 	case NEXT_ALL:
 		return bisect_next_all(prefix, no_checkout);
+	case CHECK_TERM_FMT:
+		if (argc != 2)
+			die(_("--check-term-format requires two arguments"));
+		return check_term_format(argv[0], argv[1]);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 5d1cb00..7d7965d 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -564,38 +564,11 @@ write_terms () {
 	then
 		die "$(gettext "please use two different terms")"
 	fi
-	check_term_format "$TERM_BAD" bad
-	check_term_format "$TERM_GOOD" good
+	git bisect--helper --check-term-format "$TERM_BAD" bad || exit
+	git bisect--helper --check-term-format "$TERM_GOOD" good || exit
 	printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
 }
 
-check_term_format () {
-	term=$1
-	git check-ref-format refs/bisect/"$term" ||
-	die "$(eval_gettext "'\$term' is not a valid term")"
-	case "$term" in
-	help|start|terms|skip|next|reset|visualize|replay|log|run)
-		die "$(eval_gettext "can't use the builtin command '\$term' as a term")"
-		;;
-	bad|new)
-		if test "$2" != bad
-		then
-			# In theory, nothing prevents swapping
-			# completely good and bad, but this situation
-			# could be confusing and hasn't been tested
-			# enough. Forbid it for now.
-			die "$(eval_gettext "can't change the meaning of term '\$term'")"
-		fi
-		;;
-	good|old)
-		if test "$2" != good
-		then
-			die "$(eval_gettext "can't change the meaning of term '\$term'")"
-		fi
-		;;
-	esac
-}
-
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
-- 
2.8.3

Re: [PATCH v8 2/3] bisect: rewrite `check_term_format` shell function in C

[Johannes Schindelin]

Hi Pranit,

I think this patch series is now in a fine shape. Just one minor nit:

On Tue, 24 May 2016, Pranit Bauva wrote:

> +		OPT_CMDMODE(0, "check-term-format", &cmdmode,
> +			 N_("check format of the term"), CHECK_TERM_FMT),

Just like with cmd_mode and CMD_MODE, it would be more consistent to use
CHECK_TERM_FORMAT (instead of abbreviating it) when the option is already
spelled --check-term-format and the function name is check_term_format.

It not only causes the eyes to stumble less, being consistent in naming
causes less typos and also makes it easier to dig into the source code
(think: `git grep -i check.name.format`).

Not a big thing, but if you re-roll for any other reason, it would be good
to make the naming consistent, too.

Ciao,
Johannes

Re: [PATCH v8 2/3] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

Hey Johannes,

On Wed, May 25, 2016 at 10:34 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Pranit,
>
> I think this patch series is now in a fine shape. Just one minor nit:

Thanks!

> On Tue, 24 May 2016, Pranit Bauva wrote:
>
>> +             OPT_CMDMODE(0, "check-term-format", &cmdmode,
>> +                      N_("check format of the term"), CHECK_TERM_FMT),
>
> Just like with cmd_mode and CMD_MODE, it would be more consistent to use
> CHECK_TERM_FORMAT (instead of abbreviating it) when the option is already
> spelled --check-term-format and the function name is check_term_format.
>
> It not only causes the eyes to stumble less, being consistent in naming
> causes less typos and also makes it easier to dig into the source code
> (think: `git grep -i check.name.format`).
>
> Not a big thing, but if you re-roll for any other reason, it would be good
> to make the naming consistent, too.

Sure I would include this in a re-roll. Anywhich ways it will really
not make a big difference because after some time, "git grep -i
check.term.format" wouldn't return anything except for the method
name. Other occurrences would be vanished.

Regards,
Pranit Bauva

> Ciao,
> Johannes

Re: [PATCH v8 2/3] bisect: rewrite `check_term_format` shell function in C

[Lars Schneider]

On 24 May 2016, at 09:21, Pranit Bauva <pranit.bauva@gmail.com> wrote:

> Reimplement the `check_term_format` shell function in C and add
> a `--check-term-format` subcommand to `git bisect--helper` to call it
> from git-bisect.sh
> 
> Using `--check-term-format` subcommand is a temporary measure to port
> shell function to 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 method/subcommand. For eg. In conversion of
> write_terms() of git-bisect.sh, the subcommand will be removed and
> instead check_term_format() will be called in its C implementation while
> a new subcommand will be introduced for write_terms().
> 
> Helped-by: Johannes Schindelein <Johannes.Schindelein@gmx.de>
> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>

Hi Pranit,

please drop my Mentored-by until I contribute something
useful. I feel bad being mentioned in the same way as
Christian although he does all the work.

Thanks,
Lars

> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---

Re: [PATCH v8 2/3] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

Hey Lars,
Hope you enjoyed your vacations. :)

On Thu, Jun 16, 2016 at 12:40 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
> On 24 May 2016, at 09:21, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>
>> Reimplement the `check_term_format` shell function in C and add
>> a `--check-term-format` subcommand to `git bisect--helper` to call it
>> from git-bisect.sh
>>
>> Using `--check-term-format` subcommand is a temporary measure to port
>> shell function to 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 method/subcommand. For eg. In conversion of
>> write_terms() of git-bisect.sh, the subcommand will be removed and
>> instead check_term_format() will be called in its C implementation while
>> a new subcommand will be introduced for write_terms().
>>
>> Helped-by: Johannes Schindelein <Johannes.Schindelein@gmx.de>
>> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
>> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
>
> Hi Pranit,
>
> please drop my Mentored-by until I contribute something
> useful. I feel bad being mentioned in the same way as
> Christian although he does all the work.

Sure! I will looking forward for you to comment more!

Regards,
Pranit Bauva

[PATCH v8 3/3] bisect--helper: `write_terms` shell function in C

[Pranit Bauva]

Reimplement the `write_terms` shell function in C and add a `write-terms`
subcommand to `git bisect--helper` to call it from git-bisect.sh . Also
remove the subcommand `--check-term-format` as it can now be called from
inside the function write_terms() C implementation.

Also `|| exit` is added when calling write-terms subcommand from
git-bisect.sh so as to exit whenever there is an error.

Using `--write-terms` subcommand is a temporary measure to port shell
function to 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 method.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 36 +++++++++++++++++++++++++++++-------
 git-bisect.sh            | 22 +++++++---------------
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3c748d1..91027b0 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -4,9 +4,11 @@
 #include "bisect.h"
 #include "refs.h"
 
+static GIT_PATH_FUNC(git_path_bisect_write_terms, "BISECT_TERMS")
+
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
-	N_("git bisect--helper --check-term-format <term> <orig_term>"),
+	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
 	NULL
 };
 
@@ -56,18 +58,38 @@ static int check_term_format(const char *term, const char *orig_term)
 	return 0;
 }
 
+static int write_terms(const char *bad, const char *good)
+{
+	FILE *fp;
+	int res;
+
+	if (!strcmp(bad, good))
+		return error(_("please use two different terms"));
+
+	if (check_term_format(bad, "bad") || check_term_format(good, "good"))
+		return -1;
+
+	fp = fopen(git_path_bisect_write_terms(), "w");
+	if (!fp)
+		return error_errno(_("could not open the file BISECT_TERMS"));
+
+	res = fprintf(fp, "%s\n%s\n", bad, good);
+	fclose(fp);
+	return (res < 0) ? -1 : 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
-		CHECK_TERM_FMT
+		WRITE_TERMS
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
-		OPT_CMDMODE(0, "check-term-format", &cmdmode,
-			 N_("check format of the term"), CHECK_TERM_FMT),
+		OPT_CMDMODE(0, "write-terms", &cmdmode,
+			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -82,10 +104,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	switch (cmdmode) {
 	case NEXT_ALL:
 		return bisect_next_all(prefix, no_checkout);
-	case CHECK_TERM_FMT:
+	case WRITE_TERMS:
 		if (argc != 2)
-			die(_("--check-term-format requires two arguments"));
-		return check_term_format(argv[0], argv[1]);
+			die(_("--write-terms requires two arguments"));
+		return write_terms(argv[0], argv[1]);
 	default:
 		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 7d7965d..cd39bd0 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -210,7 +210,7 @@ bisect_start() {
 	eval "$eval true" &&
 	if test $must_write_terms -eq 1
 	then
-		write_terms "$TERM_BAD" "$TERM_GOOD"
+		git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit
 	fi &&
 	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
 	#
@@ -557,18 +557,6 @@ get_terms () {
 	fi
 }
 
-write_terms () {
-	TERM_BAD=$1
-	TERM_GOOD=$2
-	if test "$TERM_BAD" = "$TERM_GOOD"
-	then
-		die "$(gettext "please use two different terms")"
-	fi
-	git bisect--helper --check-term-format "$TERM_BAD" bad || exit
-	git bisect--helper --check-term-format "$TERM_GOOD" good || exit
-	printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
-}
-
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
@@ -582,13 +570,17 @@ check_and_set_terms () {
 		bad|good)
 			if ! test -s "$GIT_DIR/BISECT_TERMS"
 			then
-				write_terms bad good
+				TERM_BAD=bad
+				TERM_GOOD=good
+				git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit
 			fi
 			;;
 		new|old)
 			if ! test -s "$GIT_DIR/BISECT_TERMS"
 			then
-				write_terms new old
+				TERM_BAD=new
+				TERM_GOOD=old
+				git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit
 			fi
 			;;
 		esac ;;
-- 
2.8.3

Re: [PATCH v8 3/3] bisect--helper: `write_terms` shell function in C

[Christian Couder]

On Tue, May 24, 2016 at 9:21 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Reimplement the `write_terms` shell function in C and add a `write-terms`
> subcommand to `git bisect--helper` to call it from git-bisect.sh . Also
> remove the subcommand `--check-term-format` as it can now be called from
> inside the function write_terms() C implementation.
>
> Also `|| exit` is added when calling write-terms subcommand from
> git-bisect.sh so as to exit whenever there is an error.
>
> Using `--write-terms` subcommand is a temporary measure to port shell
> function to 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 method.
>
> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
>  builtin/bisect--helper.c | 36 +++++++++++++++++++++++++++++-------
>  git-bisect.sh            | 22 +++++++---------------
>  2 files changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3c748d1..91027b0 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -4,9 +4,11 @@
>  #include "bisect.h"
>  #include "refs.h"
>
> +static GIT_PATH_FUNC(git_path_bisect_write_terms, "BISECT_TERMS")
> +
>  static const char * const git_bisect_helper_usage[] = {
>         N_("git bisect--helper --next-all [--no-checkout]"),
> -       N_("git bisect--helper --check-term-format <term> <orig_term>"),
> +       N_("git bisect--helper --write-terms <bad_term> <good_term>"),
>         NULL
>  };
>
> @@ -56,18 +58,38 @@ static int check_term_format(const char *term, const char *orig_term)
>         return 0;
>  }
>
> +static int write_terms(const char *bad, const char *good)
> +{
> +       FILE *fp;
> +       int res;
> +
> +       if (!strcmp(bad, good))
> +               return error(_("please use two different terms"));
> +
> +       if (check_term_format(bad, "bad") || check_term_format(good, "good"))
> +               return -1;
> +
> +       fp = fopen(git_path_bisect_write_terms(), "w");
> +       if (!fp)
> +               return error_errno(_("could not open the file BISECT_TERMS"));
> +
> +       res = fprintf(fp, "%s\n%s\n", bad, good);
> +       fclose(fp);
> +       return (res < 0) ? -1 : 0;
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>         enum {
>                 NEXT_ALL = 1,
> -               CHECK_TERM_FMT
> +               WRITE_TERMS
>         } cmdmode = 0;
>         int no_checkout = 0;
>         struct option options[] = {
>                 OPT_CMDMODE(0, "next-all", &cmdmode,
>                          N_("perform 'git bisect next'"), NEXT_ALL),
> -               OPT_CMDMODE(0, "check-term-format", &cmdmode,
> -                        N_("check format of the term"), CHECK_TERM_FMT),
> +               OPT_CMDMODE(0, "write-terms", &cmdmode,
> +                        N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
>                 OPT_BOOL(0, "no-checkout", &no_checkout,
>                          N_("update BISECT_HEAD instead of checking out the current commit")),
>                 OPT_END()
> @@ -82,10 +104,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>         switch (cmdmode) {
>         case NEXT_ALL:
>                 return bisect_next_all(prefix, no_checkout);
> -       case CHECK_TERM_FMT:
> +       case WRITE_TERMS:
>                 if (argc != 2)
> -                       die(_("--check-term-format requires two arguments"));
> -               return check_term_format(argv[0], argv[1]);
> +                       die(_("--write-terms requires two arguments"));
> +               return write_terms(argv[0], argv[1]);
>         default:
>                 die("BUG: unknown subcommand '%d'", cmdmode);
>         }
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 7d7965d..cd39bd0 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -210,7 +210,7 @@ bisect_start() {
>         eval "$eval true" &&
>         if test $must_write_terms -eq 1
>         then
> -               write_terms "$TERM_BAD" "$TERM_GOOD"
> +               git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit

This `|| exit` is not needed because...

>         fi &&
>         echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit

... there is an `|| exit` on the line above (which is chained using
`&&` to the previous lines).

Re: [PATCH v8 3/3] bisect--helper: `write_terms` shell function in C

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index 7d7965d..cd39bd0 100755
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -210,7 +210,7 @@ bisect_start() {
>>         eval "$eval true" &&
>>         if test $must_write_terms -eq 1
>>         then
>> -               write_terms "$TERM_BAD" "$TERM_GOOD"
>> +               git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit
>
> This `|| exit` is not needed because...
>
>>         fi &&
>>         echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
>
> ... there is an `|| exit` on the line above (which is chained using
> `&&` to the previous lines).

Could you please trim parts that you are not commenting on when
quoting?  Thanks.

Re: [PATCH v8 3/3] bisect--helper: `write_terms` shell function in C

[Pranit Bauva]

Hey Christian,

On Tue, May 24, 2016 at 1:03 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Tue, May 24, 2016 at 9:21 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index 7d7965d..cd39bd0 100755
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -210,7 +210,7 @@ bisect_start() {
>>         eval "$eval true" &&
>>         if test $must_write_terms -eq 1
>>         then
>> -               write_terms "$TERM_BAD" "$TERM_GOOD"
>> +               git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit
>
> This `|| exit` is not needed because...
>
>>         fi &&
>>         echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
>
> ... there is an `|| exit` on the line above (which is chained using
> `&&` to the previous lines).

Nice notice. I will send a re-roll

Regards,
Pranit Bauva