Re: [PATCH v15 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Hey Stephan,
On Wed, Dec 7, 2016 at 4:35 AM, Stephan Beyer wrote:
> Hey Pranit,
>
> On 12/06/2016 10:14 PM, Pranit Bauva wrote:
+
+ if (argc == 0) {
+ printf(_("Your current terms are %s for the old state\nand "
+"%s for the new state.\n"), terms->term_good,
+terms->term_bad);
>>>
>>> Very minor: It improves the readability if you'd split the string after
>>> the \n and put the "and "in the next line.
>>
>> Ah. This is because of the message. If I do the other way, then it
>> won't match the output in one of the tests in t/t6030 thus, I am
>> keeping it that way in order to avoid modifying the file t/t6030.
>
> I think I was unclear here. I was referring to the coding/layouting
> style, not to the string. I mean like writing:
>
> printf(_("Your current terms are %s for the old state\n"
> "and "%s for the new state.\n"),
>terms->term_good, terms->term_bad);
>
> The string fed to _() is the same, but it is split in a different (imho
> more readable) way: after the "\n", not after the "and ".
Thanks for clearing it out. This seems a sensible change.
+ die(_("invalid argument %s for 'git bisect "
+ "terms'.\nSupported options are: "
+ "--term-good|--term-old and "
+ "--term-bad|--term-new."), argv[i]);
>>>
>>> Hm, "return error(...)" and "die(...)" seems to be quasi-equivalent in
>>> this case. Because I am always looking from a library perspective, I'd
>>> prefer "return error(...)".
>>
>> I should use return error()
>
> When you reroll your patches, please also check if you always put _()
> around your error()s ;) (Hmmm... On the other hand, it might be arguable
> if translations are useful for errors that only occur when people hack
> git-bisect or use the bisect--helper directly... This makes me feel like
> all those errors should be prefixed by some "BUG: " marker since the
> ordinary user only sees them when there is a bug. But I don't feel in
> the position to decide or recommend such a thing, so it's just a thought.)
It is seems a good change, I will do it. Let other's comment on what
they think in the next re-roll.
Regards,
Pranit Bauva
Re: [PATCH v15 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Hey Pranit,
On 12/06/2016 10:14 PM, Pranit Bauva wrote:
>>> +
>>> + if (argc == 0) {
>>> + printf(_("Your current terms are %s for the old state\nand "
>>> +"%s for the new state.\n"), terms->term_good,
>>> +terms->term_bad);
>>
>> Very minor: It improves the readability if you'd split the string after
>> the \n and put the "and "in the next line.
>
> Ah. This is because of the message. If I do the other way, then it
> won't match the output in one of the tests in t/t6030 thus, I am
> keeping it that way in order to avoid modifying the file t/t6030.
I think I was unclear here. I was referring to the coding/layouting
style, not to the string. I mean like writing:
printf(_("Your current terms are %s for the old state\n"
"and "%s for the new state.\n"),
terms->term_good, terms->term_bad);
The string fed to _() is the same, but it is split in a different (imho
more readable) way: after the "\n", not after the "and ".
>>> + die(_("invalid argument %s for 'git bisect "
>>> + "terms'.\nSupported options are: "
>>> + "--term-good|--term-old and "
>>> + "--term-bad|--term-new."), argv[i]);
>>
>> Hm, "return error(...)" and "die(...)" seems to be quasi-equivalent in
>> this case. Because I am always looking from a library perspective, I'd
>> prefer "return error(...)".
>
> I should use return error()
When you reroll your patches, please also check if you always put _()
around your error()s ;) (Hmmm... On the other hand, it might be arguable
if translations are useful for errors that only occur when people hack
git-bisect or use the bisect--helper directly... This makes me feel like
all those errors should be prefixed by some "BUG: " marker since the
ordinary user only sees them when there is a bug. But I don't feel in
the position to decide or recommend such a thing, so it's just a thought.)
~Stephan
Re: [PATCH v15 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Hey Stephan,
On Fri, Nov 18, 2016 at 3:02 AM, Stephan Beyer wrote:
> Hi,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 317d671..6a5878c 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
> [...]
>> +static int bisect_terms(struct bisect_terms *terms, const char **argv, int
>> argc)
>> +{
>> + int i;
>> + const char bisect_term_usage[] =
>> +"git bisect--helper --bisect-terms [--term-good | --term-bad | ]"
>> +"--term-old | --term-new";
>
> Three things:
>
> (1) Is that indentation intentional?
Yes it was intentional but now I cannot recollect why. I think it was
because I found something similar. Nevertheless, I will fix this
indentation/
> (2) You have a "]" at the end of the first part of the string instead of
> the end of the second part.
This should be corrected.
> (3) After the correction, bisect_term_usage and
> git_bisect_helper_usage[7] are the same strings. I don't recommend to
> use git_bisect_helper_usage[7] instead because keeping the index
> up-to-date is a maintenance hell. (At the end of your patch series it is
> a 3 instead of a 7.) However, if - for whatever reason - the usage of
> bisect--helper --bisect-terms changes, you always have to sync the two
> strings which is also nasty
>
>> +
>> + if (get_terms(terms))
>> + return error(_("no terms defined"));
>> +
>> + if (argc > 1) {
>> + usage(bisect_term_usage);
>> + return -1;
>> + }
>
> ...and since you only use it once, why not simply do something like
>
> return error(_("--bisect-term requires exactly one argument"));
>
> and drop the definition of bisect_term_usage.
Sure that would be better.
>> +
>> + if (argc == 0) {
>> + printf(_("Your current terms are %s for the old state\nand "
>> +"%s for the new state.\n"), terms->term_good,
>> +terms->term_bad);
>
> Very minor: It improves the readability if you'd split the string after
> the \n and put the "and "in the next line.
Ah. This is because of the message. If I do the other way, then it
won't match the output in one of the tests in t/t6030 thus, I am
keeping it that way in order to avoid modifying the file t/t6030.
>> + return 0;
>> + }
>> +
>> + for (i = 0; i < argc; i++) {
>> + if (!strcmp(argv[i], "--term-good"))
>> + printf("%s\n", terms->term_good);
>> + else if (!strcmp(argv[i], "--term-bad"))
>> + printf("%s\n", terms->term_bad);
>> + else
>> + die(_("invalid argument %s for 'git bisect "
>> + "terms'.\nSupported options are: "
>> + "--term-good|--term-old and "
>> + "--term-bad|--term-new."), argv[i]);
>
> Hm, "return error(...)" and "die(...)" seems to be quasi-equivalent in
> this case. Because I am always looking from a library perspective, I'd
> prefer "return error(...)".
I should use return error()
>> @@ -429,6 +492,11 @@ int cmd_bisect__helper(int argc, const char **argv,
>> const char *prefix)
>> terms.term_bad = xstrdup(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;
>
> Also here: "terms" is leaking...
Will have to free it.
> ~Stephan
Re: [PATCH v15 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Hi,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 317d671..6a5878c 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
[...]
> +static int bisect_terms(struct bisect_terms *terms, const char **argv, int
> argc)
> +{
> + int i;
> + const char bisect_term_usage[] =
> +"git bisect--helper --bisect-terms [--term-good | --term-bad | ]"
> +"--term-old | --term-new";
Three things:
(1) Is that indentation intentional?
(2) You have a "]" at the end of the first part of the string instead of
the end of the second part.
(3) After the correction, bisect_term_usage and
git_bisect_helper_usage[7] are the same strings. I don't recommend to
use git_bisect_helper_usage[7] instead because keeping the index
up-to-date is a maintenance hell. (At the end of your patch series it is
a 3 instead of a 7.) However, if - for whatever reason - the usage of
bisect--helper --bisect-terms changes, you always have to sync the two
strings which is also nasty
> +
> + if (get_terms(terms))
> + return error(_("no terms defined"));
> +
> + if (argc > 1) {
> + usage(bisect_term_usage);
> + return -1;
> + }
...and since you only use it once, why not simply do something like
return error(_("--bisect-term requires exactly one argument"));
and drop the definition of bisect_term_usage.
> +
> + if (argc == 0) {
> + printf(_("Your current terms are %s for the old state\nand "
> +"%s for the new state.\n"), terms->term_good,
> +terms->term_bad);
Very minor: It improves the readability if you'd split the string after
the \n and put the "and "in the next line.
> + return 0;
> + }
> +
> + for (i = 0; i < argc; i++) {
> + if (!strcmp(argv[i], "--term-good"))
> + printf("%s\n", terms->term_good);
> + else if (!strcmp(argv[i], "--term-bad"))
> + printf("%s\n", terms->term_bad);
> + else
> + die(_("invalid argument %s for 'git bisect "
> + "terms'.\nSupported options are: "
> + "--term-good|--term-old and "
> + "--term-bad|--term-new."), argv[i]);
Hm, "return error(...)" and "die(...)" seems to be quasi-equivalent in
this case. Because I am always looking from a library perspective, I'd
prefer "return error(...)".
> @@ -429,6 +492,11 @@ int cmd_bisect__helper(int argc, const char **argv,
> const char *prefix)
> terms.term_bad = xstrdup(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;
Also here: "terms" is leaking...
~Stephan
[PATCH v15 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.
Also use error() to report "no terms defined" and accordingly change the
test in t6030.
Mentored-by: Lars Schneider
Mentored-by: Christian Couder
Signed-off-by: Pranit Bauva
---
builtin/bisect--helper.c| 72 +++--
git-bisect.sh | 35 ++
t/t6030-bisect-porcelain.sh | 2 +-
3 files changed, 73 insertions(+), 36 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 317d671..6a5878c 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 = strbuf_detach(&str, NULL);
+ strbuf_getline_lf(&str, fp);
+ terms->term_good = strbuf_detach(&str, NULL);
+ goto finish;
+finish:
+ if (fp)
+ fclose(fp);
+ strbuf_release(&str);
+ return res;
+}
+
+static int bisect_terms(struct bisect_terms *terms, const char **argv, int
argc)
+{
+ int i;
+ const char bisect_term_usage[] =
+"git bisect--helper --bisect-terms [--term-good | --term-bad | ]"
+"--term-old | --term-new";
+
+ if (get_terms(terms))
+ return error(_("no terms defined"));
+
+ if (argc > 1) {
+ usage(bisect_term_usage);
+ 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,
+ terms->term_bad);
+ return 0;
+ }
+
+ for (i = 0; i < argc; i++) {
+ if (!strcmp(argv[i], "--term-good"))
+ printf("%s\n", terms->term_good);
+ else if (!strcmp(argv[i], "--term-bad"))
+ printf("%s\n", terms->term_bad);
+ else
+ die(_("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 {
@@ -353,7 +413,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[] = {
@@ -373,6 +434,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()
@@ -380,7 +443,7 @@ int cmd_bisect__helper(int argc, const char **argv, const
char *prefix)
struct bisect_terms 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);
@@ -429,6 +492,11 @@ int cmd_bisect__helper(int argc, const char **argv, const
char *prefix)
terms.term_bad = xstrdup(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
