threads / patch / 19851

patchadd: allow configurations to be overriden by command line

Subject: [PATCH] add: allow configurations to be overriden by command line

## tl;dr

7 messages between Jun 18, 2009 and Jun 22, 2009. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Stephen Boyd· Jun 18, 2009, 09:17 UTC · lore

Don't call git_config after parsing the command line options, otherwise the config settings will override any settings made by the command line.

This can be seen by setting add.ignore_errors and then specifying --no-ignore-errors when using git-add.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 builtin-add.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin-add.c +2 −2
diff --git a/builtin-add.c b/builtin-add.c
index c1b229a..56e5221 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -298,6 +298,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int add_new_files;
 	int require_pathspec;
 
+	git_config(add_config, NULL);
+
 	argc = parse_options(argc, argv, prefix, builtin_add_options,
 			  builtin_add_usage, PARSE_OPT_KEEP_ARGV0);
 	if (patch_interactive)
@@ -305,8 +307,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (add_interactive)
 		exit(interactive_add(argc - 1, argv + 1, prefix));
 
-	git_config(add_config, NULL);
-
 	if (edit_interactive)
 		return(edit_patch(argc, argv, prefix));
 	argc--;

-- 
1.6.3.2.306.g4f4fa
Junio C Hamano· Jun 18, 2009, 16:40 UTC · re: Stephen Boyd · lore

Re: [PATCH] add: allow configurations to be overriden by command line

Stephen Boyd <bebarino@gmail.com> writes:
> Don't call git_config after parsing the command line options, otherwise
> the config settings will override any settings made by the command line.
Amusing.

This dates back to the original 0d78153 (Do "git add" as a builtin, 2006-05-17). It had two git_config() calls (one before command line parsing, and then another).

Then 11be42a (Make git-mv a builtin, 2006-07-26) noticed this duplicate calls, and removed the wrong one.

The calling sequence has been the same ever since; parse-opt-ification 5c46f75 (Port builtin-add.c to use the new option parser., 2007-10-03) did not affect it either.

Perhaps this deserves a test. It is an ancient breakage nobody noticed so far.

Thanks
Stephen Boyd· Jun 18, 2009, 18:14 UTC · re: Junio C Hamano · lore

Re: [PATCH] add: allow configurations to be overriden by command line

Junio C Hamano wrote:
>
> Perhaps this deserves a test.  It is an ancient breakage nobody noticed so
> far

I didn't think it would be very useful, which is why I left it out. But hey, at least it stops it from happening again. Squash it in if you want.

Show changes to t/t3700-add.sh +9 −0
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 6ce8256..91c1f7a 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -242,4 +242,13 @@ test_expect_success 'git add to resolve conflicts on otherwise ignored path' '
 	git add track-this
 '
 
+test_expect_success POSIXPERM '--no-ignore-errors overrides config' '
+	git config add.ignore-errors 1 &&
+	git reset --hard &&
+	date >foo1 &&
+	test_must_fail git add --verbose --no-ignore-errors . &&
+	! ( git ls-files foo1 | grep foo1 ) &&
+	git config add.ignore-errors 0
+'
+
 test_done
Junio C Hamano· Jun 18, 2009, 18:34 UTC · re: Stephen Boyd · lore

Re: [PATCH] add: allow configurations to be overriden by command line

Stephen Boyd <bebarino@gmail.com> writes:
Show 21 quoted lines
> I didn't think it would be very useful, which is why I left it out. But
> hey, at least it stops it from happening again. Squash it in if you want.
>
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index 6ce8256..91c1f7a 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -242,4 +242,13 @@ test_expect_success 'git add to resolve conflicts on otherwise ignored path' '
>  	git add track-this
>  '
>  
> +test_expect_success POSIXPERM '--no-ignore-errors overrides config' '
> +	git config add.ignore-errors 1 &&
> +	git reset --hard &&
> +	date >foo1 &&
> +	test_must_fail git add --verbose --no-ignore-errors . &&
> +	! ( git ls-files foo1 | grep foo1 ) &&
> +	git config add.ignore-errors 0
> +'
> +
>  test_done
Why do you need POSIXPERM for this?
Stephen Boyd· Jun 18, 2009, 18:58 UTC · re: Junio C Hamano · lore

Re: [PATCH] add: allow configurations to be overriden by command line

Junio C Hamano wrote:
> Why do you need POSIXPERM for this?
>

I copied a previous test and it was doing chmod 0 foo2. This test is actually depending on that earlier test which is probably bad style, sorry. Here's a more insulated version.

Show changes to t/t3700-add.sh +13 −0
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 6ce8256..66bdc24 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -221,6 +221,19 @@ test_expect_success POSIXPERM 'git add (add.ignore-errors = false)' '
        test_must_fail git add --verbose . &&
        ! ( git ls-files foo1 | grep foo1 )
 '
+rm -f foo2
+
+test_expect_success POSIXPERM '--no-ignore-errors overrides config' '
+       git config add.ignore-errors 1 &&
+       git reset --hard &&
+       date >foo1 &&
+       date >foo2 &&
+       chmod 0 foo2 &&
+       test_must_fail git add --verbose --no-ignore-errors . &&
+       ! ( git ls-files foo1 | grep foo1 ) &&
+       git config add.ignore-errors 0
+'
+rm -f foo2
 
 test_expect_success BSLASHPSPEC "git add 'fo\\[ou\\]bar' ignores foobar" '
        git reset --hard &&
Johannes Sixt· Jun 22, 2009, 07:30 UTC · re: Stephen Boyd · lore

[PATCH] t3700-add: add a POSIXPERM prerequisite to a new test

From: Johannes Sixt <j6t@kdbg.org>

The new test does a 'chmod 0', which does not have the intended effect on Windows.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Stephen Boyd schrieb:
> Junio C Hamano wrote:
>> Why do you need POSIXPERM for this?
> 
> I copied a previous test and it was doing chmod 0 foo2.
...
Show 6 quoted lines
> +test_expect_success POSIXPERM '--no-ignore-errors overrides config' '
> +       git config add.ignore-errors 1 &&
> +       git reset --hard &&
> +       date >foo1 &&
> +       date >foo2 &&
> +       chmod 0 foo2 &&

I can only guess that you missed this 'chmod 0' despite Stephen's explanation and dropped POSIXPERM when you applied the patch.

-- Hannes
 t/t3700-add.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to t/t3700-add.sh +1 −1
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 6ae5a2c..85eb0fb 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -223,7 +223,7 @@ test_expect_success POSIXPERM 'git add
 '
 rm -f foo2

-test_expect_success '--no-ignore-errors overrides config' '
+test_expect_success POSIXPERM '--no-ignore-errors overrides config' '
        git config add.ignore-errors 1 &&
        git reset --hard &&
        date >foo1 &&

-- 
1.6.3.2.1450.g146e1
Junio C Hamano· Jun 22, 2009, 07:42 UTC · re: Johannes Sixt · lore

Re: [PATCH] t3700-add: add a POSIXPERM prerequisite to a new test

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 22 quoted lines
> From: Johannes Sixt <j6t@kdbg.org>
>
> The new test does a 'chmod 0', which does not have the intended
> effect on Windows.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> Stephen Boyd schrieb:
>> Junio C Hamano wrote:
>>> Why do you need POSIXPERM for this?
>> 
>> I copied a previous test and it was doing chmod 0 foo2.
> ...
>> +test_expect_success POSIXPERM '--no-ignore-errors overrides config' '
>> +       git config add.ignore-errors 1 &&
>> +       git reset --hard &&
>> +       date >foo1 &&
>> +       date >foo2 &&
>> +       chmod 0 foo2 &&
>
> I can only guess that you missed this 'chmod 0' despite Stephen's
> explanation and dropped POSIXPERM when you applied the patch.

Actually, I wanted to add the test to 'maint' and 'maint-$v' for values of $v that are lower than 1.6.3, so I did want to drop POSIXPERM on the branch the patch initially applied; I forgot to make an evil merge when I merged the result up, crossing 1.6.2/1.6.3 boundary..

Will queue on 'maint'.  Thanks.

← back to recent threads