Re: [PATCH 17/20] abbrev: unify the handling of empty values

[Martin Ågren]

On 9 June 2018 at 16:24, Martin Ågren  wrote:
> On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason  wrote:
>> For no good reason the --abbrev= command-line option was less strict
>> than the core.abbrev config option, which came down to the latter
>> using git_config_int() which rejects an empty string, but the rest of
>> the parsing using strtoul() which will convert it to 0.
>
> It will still be less strict in that it accepts trailing garbage, e.g.,
> `--abbrev=7a`. Probably ok to leave it at that in this series, but
> possibly useful to mention here that this only makes these options "less
> differently strict".

Hmpf, please ignore. That's what I get for looking at a few patches,
taking a break, picking it up again and completely forgetting what's
going on...

Re: [PATCH 17/20] abbrev: unify the handling of empty values

[Martin Ågren]

On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason  wrote:
> For no good reason the --abbrev= command-line option was less strict
> than the core.abbrev config option, which came down to the latter
> using git_config_int() which rejects an empty string, but the rest of
> the parsing using strtoul() which will convert it to 0.

It will still be less strict in that it accepts trailing garbage, e.g.,
`--abbrev=7a`. Probably ok to leave it at that in this series, but
possibly useful to mention here that this only makes these options "less
differently strict".

I also notice that the documentation of `--abbrev` starts with "Instead
of showing the full 40-byte hexadecimal object name" which doesn't seem
right. I get much shorter IDs and I can't see that I'd have any
configuration causing this. Anyway, that might not be anything this
series needs to do anything about.

> +   if (!strcmp(arg, ""))
> +   die("--abbrev expects a value, got '%s'", arg);

> +   if (!strcmp(arg, ""))
> +   return opterror(opt, "expects a value", 0);

> +   if (!strcmp(optarg, ""))
> +   die("--abbrev expects a value, got '%s'", optarg);

> +   test_must_fail git branch -v --abbrev= 2>stderr &&
> +   test_i18ngrep "expects a value" stderr &&

> +   test_must_fail git log --abbrev= -1 --pretty=format:%h 2>stderr &&
> +   test_i18ngrep "expects a value" stderr &&

> +   test_must_fail git diff --raw --abbrev= HEAD~ 2>stderr &&
> +   test_i18ngrep "expects a value" stderr &&

Mismatch re i18n-ed or not between implementations and tests?

Martin

[PATCH 17/20] abbrev: unify the handling of empty values

[Ævar Arnfjörð Bjarmason]

For no good reason the --abbrev= command-line option was less strict
than the core.abbrev config option, which came down to the latter
using git_config_int() which rejects an empty string, but the rest of
the parsing using strtoul() which will convert it to 0.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 diff.c |  2 ++
 parse-options-cb.c |  2 ++
 revision.c |  2 ++
 t/t0014-abbrev.sh  | 22 --
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/diff.c b/diff.c
index 75935322f1..cab79d24ab 100644
--- a/diff.c
+++ b/diff.c
@@ -4802,6 +4802,8 @@ int diff_opt_parse(struct diff_options *options,
options->abbrev = DEFAULT_ABBREV;
else if (skip_prefix(arg, "--abbrev=", )) {
char *end;
+   if (!strcmp(arg, ""))
+   die("--abbrev expects a value, got '%s'", arg);
options->abbrev = strtoul(arg, , 10);
if (*end)
die("--abbrev expects a numerical value, got '%s'", 
arg);
diff --git a/parse-options-cb.c b/parse-options-cb.c
index e3cd87fbd6..aa9984f164 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -16,6 +16,8 @@ int parse_opt_abbrev_cb(const struct option *opt, const char 
*arg, int unset)
if (!arg) {
v = unset ? 0 : DEFAULT_ABBREV;
} else {
+   if (!strcmp(arg, ""))
+   return opterror(opt, "expects a value", 0);
v = strtol(arg, (char **), 10);
if (*arg)
return opterror(opt, "expects a numerical value", 0);
diff --git a/revision.c b/revision.c
index aa87afa77f..d39a292895 100644
--- a/revision.c
+++ b/revision.c
@@ -2048,6 +2048,8 @@ static int handle_revision_opt(struct rev_info *revs, int 
argc, const char **arg
revs->abbrev = DEFAULT_ABBREV;
} else if (skip_prefix(arg, "--abbrev=", )) {
char *end;
+   if (!strcmp(optarg, ""))
+   die("--abbrev expects a value, got '%s'", optarg);
revs->abbrev = strtoul(optarg, , 10);
if (*end)
die("--abbrev expects a numerical value, got '%s'", 
optarg);
diff --git a/t/t0014-abbrev.sh b/t/t0014-abbrev.sh
index 203fe316b9..8448f78560 100755
--- a/t/t0014-abbrev.sh
+++ b/t/t0014-abbrev.sh
@@ -38,23 +38,17 @@ test_expect_success 'abbrev empty value handling differs ' '
test_must_fail git -c core.abbrev= log -1 --pretty=format:%h 2>stderr &&
test_i18ngrep "bad numeric config value.*invalid unit" stderr &&
 
-   git branch -v --abbrev= | cut_tr_d_n_field_n 3 >branch &&
-   test_byte_count = 40 branch &&
+   test_must_fail git branch -v --abbrev= 2>stderr &&
+   test_i18ngrep "expects a value" stderr &&
 
-   git log --abbrev= -1 --pretty=format:%h >log &&
-   test_byte_count = 4 log &&
+   test_must_fail git log --abbrev= -1 --pretty=format:%h 2>stderr &&
+   test_i18ngrep "expects a value" stderr &&
 
-   git diff --raw --abbrev= HEAD~ >diff &&
-   cut_tr_d_n_field_n 3 diff.3 &&
-   test_byte_count = 4 diff.3 &&
-   cut_tr_d_n_field_n 4 diff.4 &&
-   test_byte_count = 4 diff.4 &&
+   test_must_fail git diff --raw --abbrev= HEAD~ 2>stderr &&
+   test_i18ngrep "expects a value" stderr &&
 
-   test_must_fail git diff --raw --abbrev= --no-index X Y >diff &&
-   cut_tr_d_n_field_n 3 diff.3 &&
-   test_byte_count = 4 diff.3 &&
-   cut_tr_d_n_field_n 4 diff.4 &&
-   test_byte_count = 4 diff.4
+   test_must_fail git diff --raw --abbrev= --no-index X Y 2>stderr &&
+   test_i18ngrep "expects a value" stderr
 '
 
 test_expect_success 'abbrev non-integer value handling differs ' '
-- 
2.17.0.290.gded63e768a