threads / patch / 18308

patchconfig: --replace-all with one argument exits properly with a better message.

Subject: [PATCH] config: --replace-all with one argument exits properly with a better message.

## tl;dr

7 messages between Mar 14, 2009 and Mar 16, 2009. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Carlos Rica· Mar 14, 2009, 02:42 UTC · lore

'config --replace-all ONE_ARG' was being treated as 'config NAME VALUE', showing the error "key does not contain a section: --replace-all".

Now it exits before with an error message asking for the missing value. Documentation is updated and a new test is added to ensure that configuration remains the same when no value is provided.

Signed-off-by: Carlos Rica <jasampler@gmail.com>
---
 Documentation/git-config.txt |    2 +-
 builtin-config.c             |    2 ++
 t/t1300-repo-config.sh       |    9 ++++++++-
 3 files changed, 11 insertions(+), 2 deletions(-)
Show changes to 3 files +11 −2

Documentation/git-config.txt, builtin-config.c, t/t1300-repo-config.sh

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 82ce89e..7131ee3 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git config' [<file-option>] [type] [-z|--null] name [value [value_regex]]
 'git config' [<file-option>] [type] --add name value
-'git config' [<file-option>] [type] --replace-all name [value [value_regex]]
+'git config' [<file-option>] [type] --replace-all name value [value_regex]
 'git config' [<file-option>] [type] [-z|--null] --get name [value_regex]
 'git config' [<file-option>] [type] [-z|--null] --get-all name [value_regex]
 'git config' [<file-option>] [type] [-z|--null] --get-regexp name_regex [value_regex]
diff --git a/builtin-config.c b/builtin-config.c
index d52a057..005b6ea 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -386,6 +386,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			return git_config_set_multivar(argv[2], NULL, NULL, 1);
 		else if (!strcmp(argv[1], "--get"))
 			return get_value(argv[2], NULL);
+		else if (!strcmp(argv[1], "--replace-all"))
+			return error("missing value for --replace-all");
 		else if (!strcmp(argv[1], "--get-all")) {
 			do_all = 1;
 			return get_value(argv[2], NULL);
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 3c06842..9c81e04 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -118,7 +118,14 @@ EOF
 
 test_expect_success 'multiple unset is correct' 'cmp .git/config expect'
 
-mv .git/config2 .git/config
+cp .git/config2 .git/config
+
+test_expect_success '--replace-all missing value' '
+	test_must_fail git config --replace-all beta.haha &&
+	test_cmp .git/config2 .git/config
+'
+
+rm .git/config2
 
 test_expect_success '--replace-all' \
 	'git config --replace-all beta.haha gamma'

-- 
1.5.4.3
Junio C Hamano· Mar 14, 2009, 20:53 UTC · re: Carlos Rica · lore

Re: [PATCH] config: --replace-all with one argument exits properly with a better message.

Carlos Rica <jasampler@gmail.com> writes:
> 'config --replace-all ONE_ARG' was being treated as 'config NAME VALUE',
> showing the error "key does not contain a section: --replace-all".

Hmm, I am getting "error: wrong number of arguments" followed by the long and somewhat annoying "usage" from the parseopt table dump.

Ahh, that is because I am running the version from 'next', which contains the fc/parseopt-config topic that rewrites the option parser and error checking almost completely.

Can you work with Felipe to see if this is still needed, or needs to be fixed in a different way? It could be that your tests may already pass over there on 'next'. I didn't check.

Felipe Contreras· Mar 14, 2009, 21:34 UTC · re: Junio C Hamano · lore

Re: [PATCH] config: --replace-all with one argument exits properly with a better message.

On Sat, Mar 14, 2009 at 10:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> Carlos Rica <jasampler@gmail.com> writes:
>
>> 'config --replace-all ONE_ARG' was being treated as 'config NAME VALUE',
>> showing the error "key does not contain a section: --replace-all".
>
> Hmm, I am getting "error: wrong number of arguments" followed by the long
> and somewhat annoying "usage" from the parseopt table dump.
If you find it annoying why don't you remove the usage?
Show 7 quoted lines
> Ahh, that is because I am running the version from 'next', which contains
> the fc/parseopt-config topic that rewrites the option parser and error
> checking almost completely.
>
> Can you work with Felipe to see if this is still needed, or needs to be
> fixed in a different way?  It could be that your tests may already pass
> over there on 'next'.  I didn't check.

The new code is already checking correctly that --replace-all needs at least two arguments. However, the "usage" is incorrect and of course the test will come in handy.

Cheers.
-- 
Felipe Contreras
Junio C Hamano· Mar 15, 2009, 01:53 UTC · re: Felipe Contreras · lore

Re: [PATCH] config: --replace-all with one argument exits properly with a better message.

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 10 quoted lines
> On Sat, Mar 14, 2009 at 10:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Carlos Rica <jasampler@gmail.com> writes:
>>
>>> 'config --replace-all ONE_ARG' was being treated as 'config NAME VALUE',
>>> showing the error "key does not contain a section: --replace-all".
>>
>> Hmm, I am getting "error: wrong number of arguments" followed by the long
>> and somewhat annoying "usage" from the parseopt table dump.
>
> If you find it annoying why don't you remove the usage?
Because the primary target audience of the help text is not me?
Show 7 quoted lines
>> Can you work with Felipe to see if this is still needed, or needs to be
>> fixed in a different way?  It could be that your tests may already pass
>> over there on 'next'.  I didn't check.
>
> The new code is already checking correctly that --replace-all needs at
> least two arguments. However, the "usage" is incorrect and of course
> the test will come in handy.

So perhaps you can pick a part of it and send in an update to your parseoptification series? I think the series is ready for 'master' sometime next week if not sooner.

Felipe Contreras· Mar 15, 2009, 10:26 UTC · re: Junio C Hamano · lore

Re: [PATCH] config: --replace-all with one argument exits properly with a better message.

On Sun, Mar 15, 2009 at 3:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Sat, Mar 14, 2009 at 10:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Carlos Rica <jasampler@gmail.com> writes:
>>>
>>>> 'config --replace-all ONE_ARG' was being treated as 'config NAME VALUE',
>>>> showing the error "key does not contain a section: --replace-all".
>>>
>>> Hmm, I am getting "error: wrong number of arguments" followed by the long
>>> and somewhat annoying "usage" from the parseopt table dump.
>>
>> If you find it annoying why don't you remove the usage?
>
> Because the primary target audience of the help text is not me?

Ok. I don't think it makes a big difference to leave it on or off. People not familiar with 'git config' might find it handy, but I admit that I also find it a bit annoying, mainly because the error message gets lost in the noise.

Show 11 quoted lines
>>> Can you work with Felipe to see if this is still needed, or needs to be
>>> fixed in a different way?  It could be that your tests may already pass
>>> over there on 'next'.  I didn't check.
>>
>> The new code is already checking correctly that --replace-all needs at
>> least two arguments. However, the "usage" is incorrect and of course
>> the test will come in handy.
>
> So perhaps you can pick a part of it and send in an update to your
> parseoptification series?  I think the series is ready for 'master'
> sometime next week if not sooner.

Or maybe Carlos can beat me to do it since it seems he is interested. Otherwise yeah, I'll do it.

-- 
Felipe Contreras
Carlos Rica· Mar 16, 2009, 14:41 UTC · re: Felipe Contreras · lore

Re: [PATCH] config: --replace-all with one argument exits properly with a better message.

Hi Felipe, I didn't know that you were writing the parse options for config. I tried it a year ago and I leave it unfinished because (if I remember correctly) options like -4, -5, -6... and those: http://thread.gmane.org/gmane.comp.version-control.git/78480

On Sun, Mar 15, 2009 at 11:26 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 35 quoted lines
> On Sun, Mar 15, 2009 at 3:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> On Sat, Mar 14, 2009 at 10:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Carlos Rica <jasampler@gmail.com> writes:
>>>>
>>>>> 'config --replace-all ONE_ARG' was being treated as 'config NAME VALUE',
>>>>> showing the error "key does not contain a section: --replace-all".
>>>>
>>>> Hmm, I am getting "error: wrong number of arguments" followed by the long
>>>> and somewhat annoying "usage" from the parseopt table dump.
>>>
>>> If you find it annoying why don't you remove the usage?
>>
>> Because the primary target audience of the help text is not me?
>
> Ok. I don't think it makes a big difference to leave it on or off.
> People not familiar with 'git config' might find it handy, but I admit
> that I also find it a bit annoying, mainly because the error message
> gets lost in the noise.
>
>>>> Can you work with Felipe to see if this is still needed, or needs to be
>>>> fixed in a different way?  It could be that your tests may already pass
>>>> over there on 'next'.  I didn't check.
>>>
>>> The new code is already checking correctly that --replace-all needs at
>>> least two arguments. However, the "usage" is incorrect and of course
>>> the test will come in handy.
>>
>> So perhaps you can pick a part of it and send in an update to your
>> parseoptification series?  I think the series is ready for 'master'
>> sometime next week if not sooner.
>
> Or maybe Carlos can beat me to do it since it seems he is interested.
> Otherwise yeah, I'll do it.
Of course, I'm looking at your code in "pu" to see how could apply this.
Felipe Contreras· Mar 16, 2009, 15:25 UTC · re: Carlos Rica · lore

Re: [PATCH] config: --replace-all with one argument exits properly with a better message.

On Mon, Mar 16, 2009 at 4:41 PM, Carlos Rica <jasampler@gmail.com> wrote:
> Hi Felipe, I didn't know that you were writing the parse options for
> config. I tried it a year ago and I leave it unfinished because (if I
> remember correctly) options like -4, -5, -6... and those:
> http://thread.gmane.org/gmane.comp.version-control.git/78480

I found the same issue, but Johannes suggested to use PARSE_OPT_STOP_AT_NON_OPTION :)

-- 
Felipe Contreras

← back to recent threads