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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 22, 2016, 15:09 UTC
Message-ID: <alpine.DEB.2.20.1603221552100.4690@virtualbox>
In-Reply-To: <010201539d57ae98-ce4860a6-f7b6-4e06-b556-3c1340cd7749-000000@eu-west-1.amazonses.com>

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

Show 5 quoted lines

> 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;
}

Show 13 quoted lines

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

Show 6 quoted lines

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

Show 9 quoted lines

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

Show 7 quoted lines

> +	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);

Show 5 quoted lines

>  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;

Show 7 quoted lines

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

Show 9 quoted lines

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

Show 20 quoted lines

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