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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
