Re: [PATCH v14 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Pranit Bauva writes:
>>> +static int bisect_terms(struct bisect_terms *terms, const char **argv, int
>>> argc)
>>> +{
>>> + int i;
>>> +
>>> + if (get_terms(terms)) {
>>> + fprintf(stderr, _("no terms defined\n"));
>>> + return -1;
>>> + }
>>> + if (argc == 0) {
>>> + printf(_("Your current terms are %s for the old state\nand "
>>> +"%s for the new state.\n"), terms->term_good.buf,
>>> +terms->term_bad.buf);
>>> + return 0;
>>> + }
>>> +
>>> + for (i = 0; i < argc; i++) {
>>> + if (!strcmp(argv[i], "--term-good"))
>>> + printf("%s\n", terms->term_good.buf);
>>> + else if (!strcmp(argv[i], "--term-bad"))
>>> + printf("%s\n", terms->term_bad.buf);
>>> + else
>>> + printf(_("invalid argument %s for 'git bisect "
>>> + "terms'.\nSupported options are: "
>>> + "--term-good|--term-old and "
>>> + "--term-bad|--term-new."), argv[i]);
>>> + }
>>
>> The original took only one and gave one answer (and errored out when
>> the user asked for more), but this one loops. I can see either way
>> is OK and do not think of a good reason to favor one over the other;
>> unless there is a strong reason why you need this extended behaviour
>> that allows users to ask multiple questions, I'd say we should keep
>> the original behaviour.
>
> True! I can just use return error() instead of printf. Also I noticed
> that this is printing to stdout while the original printed it to
> stderr. Thanks!
The original you removed because the above can take it over is this
in your patch.
-bisect_terms () {
- get_terms
- if ! test -s "$GIT_DIR/BISECT_TERMS"
- then
- die "$(gettext "no terms defined")"
- fi
- case "$#" in
- 0)
- gettextln "Your current terms are $TERM_GOOD for the old state
-and $TERM_BAD for the new state."
- ;;
- 1)
- arg=$1
- case "$arg" in
- --term-good|--term-old)
- printf '%s\n' "$TERM_GOOD"
- ;;
- --term-bad|--term-new)
- printf '%s\n' "$TERM_BAD"
- ;;
- *)
- die "$(eval_gettext "invalid argument \$arg for
'git bisect terms'.
-Supported options are: --term-good|--term-old and --term-bad|--term-new.")"
- ;;
- esac
- ;;
- *)
- usage ;;
- esac
-}
-
The fprintf() that says "no terms defined" can be made error(). The
"invalid argument" message used to be die in the original, and
should be sent to the standard error stream as you noticed.
But a bigger difference is that the original made sure that the
caller asked one question at a time. "terms --term-good --term-bad"
was responded with a "usage". That is no longer true in the
rewrite.
Re: [PATCH v14 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Hey Junio,
On Thu, Aug 25, 2016 at 11:35 PM, Junio C Hamano wrote:
> Pranit Bauva writes:
>
>> +static int bisect_terms(struct bisect_terms *terms, const char **argv, int
>> argc)
>> +{
>> + int i;
>> +
>> + if (get_terms(terms)) {
>> + fprintf(stderr, _("no terms defined\n"));
>> + return -1;
>> + }
>> + if (argc == 0) {
>> + printf(_("Your current terms are %s for the old state\nand "
>> +"%s for the new state.\n"), terms->term_good.buf,
>> +terms->term_bad.buf);
>> + return 0;
>> + }
>> +
>> + for (i = 0; i < argc; i++) {
>> + if (!strcmp(argv[i], "--term-good"))
>> + printf("%s\n", terms->term_good.buf);
>> + else if (!strcmp(argv[i], "--term-bad"))
>> + printf("%s\n", terms->term_bad.buf);
>> + else
>> + printf(_("invalid argument %s for 'git bisect "
>> + "terms'.\nSupported options are: "
>> + "--term-good|--term-old and "
>> + "--term-bad|--term-new."), argv[i]);
>> + }
>
> The original took only one and gave one answer (and errored out when
> the user asked for more), but this one loops. I can see either way
> is OK and do not think of a good reason to favor one over the other;
> unless there is a strong reason why you need this extended behaviour
> that allows users to ask multiple questions, I'd say we should keep
> the original behaviour.
True! I can just use return error() instead of printf. Also I noticed
that this is printing to stdout while the original printed it to
stderr. Thanks!
Regards,
Pranit Bauva
--
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
Re: [PATCH v14 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Pranit Bauva writes:
> +static int bisect_terms(struct bisect_terms *terms, const char **argv, int
> argc)
> +{
> + int i;
> +
> + if (get_terms(terms)) {
> + fprintf(stderr, _("no terms defined\n"));
> + return -1;
> + }
> + if (argc == 0) {
> + printf(_("Your current terms are %s for the old state\nand "
> +"%s for the new state.\n"), terms->term_good.buf,
> +terms->term_bad.buf);
> + return 0;
> + }
> +
> + for (i = 0; i < argc; i++) {
> + if (!strcmp(argv[i], "--term-good"))
> + printf("%s\n", terms->term_good.buf);
> + else if (!strcmp(argv[i], "--term-bad"))
> + printf("%s\n", terms->term_bad.buf);
> + else
> + printf(_("invalid argument %s for 'git bisect "
> + "terms'.\nSupported options are: "
> + "--term-good|--term-old and "
> + "--term-bad|--term-new."), argv[i]);
> + }
The original took only one and gave one answer (and errored out when
the user asked for more), but this one loops. I can see either way
is OK and do not think of a good reason to favor one over the other;
unless there is a strong reason why you need this extended behaviour
that allows users to ask multiple questions, I'd say we should keep
the original behaviour.
--
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
[PATCH v14 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Reimplement the `get_terms` and `bisect_terms` shell function in C and
add `bisect-terms` subcommand to `git bisect--helper` to call it from
git-bisect.sh .
Using `--bisect-terms` subcommand is a temporary measure to port shell
function in C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired but its implementation will
be called by some other methods.
Mentored-by: Lars Schneider
Mentored-by: Christian Couder
Signed-off-by: Pranit Bauva
---
builtin/bisect--helper.c | 59 ++--
git-bisect.sh| 35 ++--
2 files changed, 59 insertions(+), 35 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index aca3908..44adf6b 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -23,6 +23,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-write
[]"),
N_("git bisect--helper --bisect-check-and-set-terms
"),
N_("git bisect--helper --bisect-next-check []
term_bad, fp) == EOF ||
+ strbuf_getline(&terms->term_good, fp) == EOF;
+
+ fclose(fp);
+ return res ? -1 : 0;
+}
+
+static int bisect_terms(struct bisect_terms *terms, const char **argv, int
argc)
+{
+ int i;
+
+ if (get_terms(terms)) {
+ fprintf(stderr, _("no terms defined\n"));
+ return -1;
+ }
+ if (argc == 0) {
+ printf(_("Your current terms are %s for the old state\nand "
+ "%s for the new state.\n"), terms->term_good.buf,
+ terms->term_bad.buf);
+ return 0;
+ }
+
+ for (i = 0; i < argc; i++) {
+ if (!strcmp(argv[i], "--term-good"))
+ printf("%s\n", terms->term_good.buf);
+ else if (!strcmp(argv[i], "--term-bad"))
+ printf("%s\n", terms->term_bad.buf);
+ else
+ printf(_("invalid argument %s for 'git bisect "
+ "terms'.\nSupported options are: "
+ "--term-good|--term-old and "
+ "--term-bad|--term-new."), argv[i]);
+ }
+
+ return 0;
+}
+
int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
{
enum {
@@ -354,7 +401,8 @@ int cmd_bisect__helper(int argc, const char **argv, const
char *prefix)
CHECK_EXPECTED_REVS,
BISECT_WRITE,
CHECK_AND_SET_TERMS,
- BISECT_NEXT_CHECK
+ BISECT_NEXT_CHECK,
+ BISECT_TERMS
} cmdmode = 0;
int no_checkout = 0, res = 0;
struct option options[] = {
@@ -374,6 +422,8 @@ int cmd_bisect__helper(int argc, const char **argv, const
char *prefix)
N_("check and set terms in a bisection state"),
CHECK_AND_SET_TERMS),
OPT_CMDMODE(0, "bisect-next-check", &cmdmode,
N_("check whether bad or good terms exist"),
BISECT_NEXT_CHECK),
+ OPT_CMDMODE(0, "bisect-terms", &cmdmode,
+N_("print out the bisect terms"), BISECT_TERMS),
OPT_BOOL(0, "no-checkout", &no_checkout,
N_("update BISECT_HEAD instead of checking out the
current commit")),
OPT_END()
@@ -382,7 +432,7 @@ int cmd_bisect__helper(int argc, const char **argv, const
char *prefix)
bisect_terms_init(&terms);
argc = parse_options(argc, argv, prefix, options,
-git_bisect_helper_usage, 0);
+git_bisect_helper_usage, PARSE_OPT_KEEP_UNKNOWN);
if (!cmdmode)
usage_with_options(git_bisect_helper_usage, options);
@@ -431,6 +481,11 @@ int cmd_bisect__helper(int argc, const char **argv, const
char *prefix)
strbuf_addstr(&terms.term_bad, argv[1]);
res = bisect_next_check(&terms, argc == 3 ? argv[2] : NULL);
break;
+ case BISECT_TERMS:
+ if (argc > 1)
+ die(_("--bisect-terms requires 0 or 1 argument"));
+ res = bisect_terms(&terms, argv, argc);
+ break;
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index fe6c9d0..d6c8b5a 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -355,7 +355,7 @@ bisect_replay () {
"$TERM_GOOD"|"$TERM_BAD"|skip)
git bisect--helper --bisect-write "$command" "$rev"
"$TERM_GOOD" "$TERM_BAD" || exit;;
terms)
- bisect_terms $rev ;;
+ git bisect--helper --bisect-terms $rev || exit;;
*)
die "$(gettext "?? what are you talking about?")" ;;
