[PATCH] Adds 'stash.index' configuration option

[PATCH] Adds 'stash.index' configuration option

[David Pisoni]

Setting 'stash.index' config option changes 'git-stash pop|apply' to  
behave
as if '--index' switch is always supplied.
'git-stash pop|apply' provides a --no-index switch to circumvent  
config default.

Signed-off-by: David Pisoni <dpisoni@gmail.com>
---
Documentation/config.txt    |    5 +++++
Documentation/git-stash.txt |   10 +++++++---
git-stash.sh                |    7 ++++++-
t/t3903-stash.sh            |   28 ++++++++++++++++++++++++++++
4 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 285c7f7..d794c40 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1746,6 +1746,11 @@ showbranch.default::
	The default set of branches for linkgit:git-show-branch[1].
	See linkgit:git-show-branch[1].

+stash.index::
+    A boolean to make linkgit:git-stash[1] default to the behavior of  
--index
+	when applying a stash to the working copy.  Can be circumvented by  
using
+	--no-index switch to linkgit:git-stash[1].  Defaults to false.
+
status.relativePaths::
	By default, linkgit:git-status[1] shows paths relative to the
	current directory. Setting this variable to `false` shows paths
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 15f051f..de086ee 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -11,7 +11,7 @@ SYNOPSIS
'git stash' list [<options>]
'git stash' show [<stash>]
'git stash' drop [-q|--quiet] [<stash>]
-'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
+'git stash' ( pop | apply ) [--[no-]index] [-q|--quiet] [<stash>]
'git stash' branch <branchname> [<stash>]
'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]  
[<message>]]
'git stash' clear
@@ -89,7 +89,7 @@ show [<stash>]::
	it will accept any format known to 'git diff' (e.g., `git stash show
	-p stash@\{1}` to view the second most recent stash in patch form).

-pop [--index] [-q|--quiet] [<stash>]::
+pop [--[no-]index] [-q|--quiet] [<stash>]::

	Remove a single stashed state from the stash list and apply it
	on top of the current working tree state, i.e., do the inverse
@@ -105,10 +105,14 @@ tree's changes, but also the index's ones.  
However, this can fail, when you
have conflicts (which are stored in the index, where you therefore can  
no
longer apply the changes as they were originally).
+
+If the configuration option `stash.index` is set `pop` will behave as  
if the
+`--index` option is always in use, unless explicitly overridden with
+`--no-index`.
++
When no `<stash>` is given, `stash@\{0}` is assumed, otherwise  
`<stash>` must
be a reference of the form `stash@\{<revision>}`.

-apply [--index] [-q|--quiet] [<stash>]::
+apply [--[no-]index] [-q|--quiet] [<stash>]::

	Like `pop`, but do not remove the state from the stash list. Unlike  
`pop`,
	`<stash>` may be any commit that looks like a commit created by
diff --git a/git-stash.sh b/git-stash.sh
index 0a94036..7711bf6 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -31,6 +31,8 @@ else
        reset_color=
fi

+CONFIG_INDEX="$(git config stash.index)"
+
no_changes () {
	git diff-index --quiet --cached HEAD --ignore-submodules -- &&
	git diff-files --quiet --ignore-submodules
@@ -256,7 +258,7 @@ parse_flags_and_rev()

	IS_STASH_LIKE=
	IS_STASH_REF=
-	INDEX_OPTION=
+	INDEX_OPTION=$CONFIG_INDEX
	s=
	w_commit=
	b_commit=
@@ -277,6 +279,9 @@ parse_flags_and_rev()
			--index)
				INDEX_OPTION=--index
			;;
+			--no-index)
+				unset INDEX_OPTION
+			;;
			-*)
				FLAGS="${FLAGS}${FLAGS:+ }$opt"
			;;
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5c72540..4999682 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -84,6 +84,34 @@ test_expect_success 'apply stashed changes  
(including index)' '
	test 1 = $(git show HEAD:file)
'

+test_expect_success 'apply stashed changes (including index) with  
index config set' '
+    git config stash.index yes &&
+	git reset --hard HEAD^ &&
+	echo 7 > other-file &&
+	git add other-file &&
+	test_tick &&
+	git commit -m other-file &&
+	git stash apply &&
+	test 3 = $(cat file) &&
+	test 2 = $(git show :file) &&
+	test 1 = $(git show HEAD:file) &&
+	git config --unset stash.index
+'
+
+test_expect_success 'apply stashed changes (excluding index) with  
index config set' '
+    git config stash.index yes &&
+	git reset --hard &&
+	echo 8 >other-file &&
+	git add other-file &&
+	test_tick &&
+	git commit -m other-file &&
+	git stash apply --no-index &&
+	test 3 = $(cat file) &&
+	test 1 = $(git show :file) &&
+	test 1 = $(git show HEAD:file) &&
+	git config --unset stash.index
+'
+
test_expect_success 'unstashing in a subdirectory' '
	git reset --hard HEAD &&
	mkdir subdir &&
-- 
1.7.5

Re: [PATCH] Adds 'stash.index' configuration option

[Junio C Hamano]

David Pisoni <dpisoni@gmail.com> writes:

> Setting 'stash.index' config option changes 'git-stash pop|apply' to
> behave
> as if '--index' switch is always supplied.
> 'git-stash pop|apply' provides a --no-index switch to circumvent
> config default.
>
> Signed-off-by: David Pisoni <dpisoni@gmail.com>

Your MUA utterly mangled your whitespaces and linebreaks.  Please be
careful when you send a re-rolled series of this patch.  The section "MUA
Specific hints" of

  http://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html

may be of help.  

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 285c7f7..d794c40 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1746,6 +1746,11 @@ showbranch.default::
> 	The default set of branches for linkgit:git-show-branch[1].
> 	See linkgit:git-show-branch[1].
>
> +stash.index::
> +    A boolean to make linkgit:git-stash[1] default to the behavior of
> --index
> +	when applying a stash to the working copy.  Can be
> circumvented by using
> +	--no-index switch to linkgit:git-stash[1].  Defaults to false.

This says "boolean", so all of these should mean "I do not want the
command to default to --index":

	[stash]
                index = false
                index = 0

and all of these mean "I do want the default --index":

	[stash]
		index
                index = yes
                index = 1

I however do not think your implementation actually handle these
correctly.

See below for one possible correct implementation, also the comment on the
necessity to test both sides of the coin.

> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 15f051f..de086ee 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -11,7 +11,7 @@ SYNOPSIS
> 'git stash' list [<options>]
> 'git stash' show [<stash>]
> 'git stash' drop [-q|--quiet] [<stash>]
> -'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
> +'git stash' ( pop | apply ) [--[no-]index] [-q|--quiet] [<stash>]

The --no-index is a very welcome addition, even if we did not have this
new configuration variable.  An alias "git pop" defined thusly:

	[alias]
        	pop = stash pop --index

can countermand it on demand with "git pop --no-index".

Please make it a separate patch, that comes before the addition of the
configuration variable.  And then another patch to add the configuration
variable on top of it.

> diff --git a/git-stash.sh b/git-stash.sh
> index 0a94036..7711bf6 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -31,6 +31,8 @@ else
>        reset_color=
> fi
>
> +CONFIG_INDEX="$(git config stash.index)"

This needs to be something like

	CONFIG_INDEX=$(git config --bool stash.index)
	
Then you would check the value against "true" later like this:

> @@ -256,7 +258,7 @@ parse_flags_and_rev()
>
> 	IS_STASH_LIKE=
> 	IS_STASH_REF=
> -	INDEX_OPTION=
> +	INDEX_OPTION=$CONFIG_INDEX

	if test "$CONFIG_INDEX" = true
	then
		INDEX_OPTION=--index
	else
        	INDEX_OPTION=
	fi

> @@ -277,6 +279,9 @@ parse_flags_and_rev()
> 			--index)
> 				INDEX_OPTION=--index
> 			;;
> +			--no-index)
> +				unset INDEX_OPTION
> +			;;

I'd rather sees this done as

			--no-index)
				INDEX_OPTION=
				;;

as a later part of the existing code says

	if test -n "$INDEX_OPTION" && ...

IOW, the code switches on the value of the variable, not on whether if it
is set or unset.

> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 5c72540..4999682 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -84,6 +84,34 @@ test_expect_success 'apply stashed changes
> (including index)' '
> 	test 1 = $(git show HEAD:file)
> '
>
> +test_expect_success 'apply stashed changes (including index) with
> index config set' '
> +    git config stash.index yes &&
> +	git reset --hard HEAD^ &&
> +	echo 7 > other-file &&

We write redirect from/to without an extra space, like you did
in the next test to echo 8 into the same file.

> +	git add other-file &&
> +	test_tick &&
> +	git commit -m other-file &&
> +	git stash apply &&
> +	test 3 = $(cat file) &&
> +	test 2 = $(git show :file) &&
> +	test 1 = $(git show HEAD:file) &&
> +	git config --unset stash.index
> +'

Use test_when_finished early to arrange so that this unset will happen
even when a step somewhere in the test fails.

It is a common mistake to write tests that only show off a shiny new toy,
making sure the feature kicks in when it should, and totally forget to
make sure the feature does not kick in when it should not.  Always remeber
to test both sides of the coin.

Check what should happen with at least these combinations:

        stash.index set to...           command line says...
 ----------------------------------------------------------------
        (not set at all)                (nothing)
        (not set at all)                --index
 *      (not set at all)                --no-index
 *      (not set at all)                --index --no-index
 *      (not set at all)                --no-index --index
 *      false                           --index
 *      false                           --no-index
 *      true                            --index
 *      true                            --no-index
 ----------------------------------------------------------------

The cases marked with "*" are the possibilities your patch opens and need
to have tests so that they are not broken when other people later change
the code (e.g. the command line parsing may break "later one wins" rule if
done carelessly).

Thanks.

Re: [PATCH] Adds 'stash.index' configuration option

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> Setting 'stash.index' config option changes 'git-stash pop|apply' to behave
>> as if '--index' switch is always supplied.

One thing I forgot to say.  "stash.index" invites "index _what_?"
Naming it to "stash.useIndex" may avoid such reaction.

Also, the current code has this comment:

    #   INDEX_OPTION is set to --index if --index is specified.

but it probably makes sense to change it (in the first patch in the series
that adds --no-index support) to a boolean whose value can be either true
or empty.

The reason why the very original code used INDEX_OPTION=--index may be
because it did something like "git some-cmd $INDEX_OPTION", but that is
not what the current code does, and using "either '--index' or ''" as a
form of boolean is confusing.

In other words, something like....



 git-stash.sh |   37 ++++++++++++++++++++-----------------
 1 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 0a94036..eed2d1e 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -239,7 +239,7 @@ show_stash () {
 #   i_tree is set to the index tree
 #
 #   GIT_QUIET is set to t if -q is specified
-#   INDEX_OPTION is set to --index if --index is specified.
+#   INDEX_OPTION is set to 'true' if applying/popping also to the index.
 #   FLAGS is set to the remaining flags
 #
 # dies if:
@@ -271,14 +271,17 @@ parse_flags_and_rev()
 	for opt
 	do
 		case "$opt" in
-			-q|--quiet)
-				GIT_QUIET=-t
+		-q|--quiet)
+			GIT_QUIET=-t
+			;;
+		--index)
+			INDEX_OPTION=true
 			;;
-			--index)
-				INDEX_OPTION=--index
+		--no-index)
+			INDEX_OPTION=
 			;;
-			-*)
-				FLAGS="${FLAGS}${FLAGS:+ }$opt"
+		-*)
+			FLAGS="${FLAGS}${FLAGS:+ }$opt"
 			;;
 		esac
 	done
@@ -286,15 +289,15 @@ parse_flags_and_rev()
 	set -- $REV
 
 	case $# in
-		0)
-			have_stash || die "No stash found."
-			set -- ${ref_stash}@{0}
+	0)
+		have_stash || die "No stash found."
+		set -- ${ref_stash}@{0}
 		;;
-		1)
-			:
+	1)
+		:
 		;;
-		*)
-			die "Too many revisions specified: $REV"
+	*)
+		die "Too many revisions specified: $REV"
 		;;
 	esac
 
@@ -342,8 +345,8 @@ apply_stash () {
 		die 'Cannot apply a stash in the middle of a merge'
 
 	unstashed_index_tree=
-	if test -n "$INDEX_OPTION" && test "$b_tree" != "$i_tree" &&
-			test "$c_tree" != "$i_tree"
+	if test true = "$INDEX_OPTION" &&
+		test "$b_tree" != "$i_tree" && test "$c_tree" != "$i_tree"
 	then
 		git diff-tree --binary $s^2^..$s^2 | git apply --cached
 		test $? -ne 0 &&
@@ -387,7 +390,7 @@ apply_stash () {
 	else
 		# Merge conflict; keep the exit status from merge-recursive
 		status=$?
-		if test -n "$INDEX_OPTION"
+		if test true = "$INDEX_OPTION"
 		then
 			echo >&2 'Index was not unstashed.'
 		fi




	

Re: [PATCH] Adds 'stash.index' configuration option

[David Pisoni]

On May 11, 2011, at 17.26 , Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> Setting 'stash.index' config option changes 'git-stash pop|apply'  
>>> to behave
>>> as if '--index' switch is always supplied.
>
> One thing I forgot to say.  "stash.index" invites "index _what_?"
> Naming it to "stash.useIndex" may avoid such reaction.

No objection.  I want the feature (scratching my own itch here), but I  
don't really care what it's called. :)

>
> Also, the current code has this comment:
>
>    #   INDEX_OPTION is set to --index if --index is specified.
>
> but it probably makes sense to change it (in the first patch in the  
> series
> that adds --no-index support) to a boolean whose value can be either  
> true
> or empty.

It seemed a little wonky to me also, but this is my first foray into  
hacking on git and was concerned someone was depending on this,  
strange though it may be. git-blame fingers ef763129d for this oddity,  
dating August 2010.

>
> The reason why the very original code used INDEX_OPTION=--index may be
> because it did something like "git some-cmd $INDEX_OPTION", but that  
> is
> not what the current code does, and using "either '--index' or ''"  
> as a
> form of boolean is confusing.

I agree.  I like your change, also.
Does this feature make sense to you overall?

<SNIP>

Thanks,
David

Re: [PATCH] Adds 'stash.index' configuration option

[Junio C Hamano]

David Pisoni <dpisoni@gmail.com> writes:

> I agree.  I like your change, also.
> Does this feature make sense to you overall?

I am very much in favor of --no-index in the sense that I would prefer to
have it in the system than not having it.

I am neutral to the configuration variable in the sense that I wouldn't
miss it if we don't have it and I wouldn't spend too much of my own
brain-cycle to add such a variable, but I wouldn't be disturbed too much
by it if we had it in the system.

Re: [PATCH] Adds 'stash.index' configuration option

[Michael J Gruber]

David Pisoni venit, vidit, dixit 12.05.2011 00:57:
> 
> Setting 'stash.index' config option changes 'git-stash pop|apply' to  
> behave
> as if '--index' switch is always supplied.
> 'git-stash pop|apply' provides a --no-index switch to circumvent  
> config default.

This is yet another incarnation of

foo.bar = true

meaning that command "git foo" defaults to "git foo --bar". (Admittedly,
this is about subcommands of foo.)

It has the same problems (possibly breaking scripts). But more
importantly, it inflates the code with every such incarnation we add.
Have we really agreed that we introduce these one-by-one rather than
doing something generic like

uiopts.<cmd> = <optionlist>

with which you would do

uiopts.stash = "--index"

and hopefully be script-safe (again, ignoring the subcommand issue)?

Michael

Re: [PATCH] Adds 'stash.index' configuration option

[Jeff King]

On Thu, May 12, 2011 at 09:14:09AM +0200, Michael J Gruber wrote:

> This is yet another incarnation of
> 
> foo.bar = true
> 
> meaning that command "git foo" defaults to "git foo --bar". (Admittedly,
> this is about subcommands of foo.)
> 
> It has the same problems (possibly breaking scripts). But more
> importantly, it inflates the code with every such incarnation we add.
> Have we really agreed that we introduce these one-by-one rather than
> doing something generic like
> 
> uiopts.<cmd> = <optionlist>
> 
> with which you would do
> 
> uiopts.stash = "--index"
> 
> and hopefully be script-safe (again, ignoring the subcommand issue)?

I would love to see something like this, but have we yet figured out all
of the issues, like:

  1. How do scripts wanting to call git programs suppress expansion of
     uiopts when they want predictable behavior?

  2. Depending on the solution to (1), how do scripts specify that they
     _do_ want to allow uiopts (e.g., because they know they are
     presenting the output to the user) for certain commands?

  3. Depending on (1) and (2), how do scripts differentiate when some
     options are OK in uiopts, but others are not? For example, it may
     be desirable for an invocation of diff-tree to have renames turned
     on by the user, but not for them to change the output format.

As much as it sucks to have a config option for each individual option,
there is at least some oversight of which options will not cause too
much of a problem when triggered automatically.

-Peff

Re: [PATCH] Adds 'stash.index' configuration option

[Michael J Gruber]

Jeff King venit, vidit, dixit 12.05.2011 10:04:
> On Thu, May 12, 2011 at 09:14:09AM +0200, Michael J Gruber wrote:
> 
>> This is yet another incarnation of
>>
>> foo.bar = true
>>
>> meaning that command "git foo" defaults to "git foo --bar". (Admittedly,
>> this is about subcommands of foo.)
>>
>> It has the same problems (possibly breaking scripts). But more
>> importantly, it inflates the code with every such incarnation we add.
>> Have we really agreed that we introduce these one-by-one rather than
>> doing something generic like
>>
>> uiopts.<cmd> = <optionlist>
>>
>> with which you would do
>>
>> uiopts.stash = "--index"
>>
>> and hopefully be script-safe (again, ignoring the subcommand issue)?
> 
> I would love to see something like this, but have we yet figured out all
> of the issues, like:
> 
>   1. How do scripts wanting to call git programs suppress expansion of
>      uiopts when they want predictable behavior?
> 
>   2. Depending on the solution to (1), how do scripts specify that they
>      _do_ want to allow uiopts (e.g., because they know they are
>      presenting the output to the user) for certain commands?
> 
>   3. Depending on (1) and (2), how do scripts differentiate when some
>      options are OK in uiopts, but others are not? For example, it may
>      be desirable for an invocation of diff-tree to have renames turned
>      on by the user, but not for them to change the output format.
> 

We haven't figured that out, but was the consensus: "Whatever, let's
just keep adding single options." ?

> As much as it sucks to have a config option for each individual option,
> there is at least some oversight of which options will not cause too
> much of a problem when triggered automatically.

I just think we have too many commands which are ui and are used in
scripts (e.g. log, commit, stash, just to name a few) for being able to
decide that ourselves. Are we saying that people using "git stash" in a
script have to deal themselves with a breakage caused by "--index" being
a default for some users now?

With a generic approach, we could protect all git-sh-setup using scripts
right from the start, for example, while still allowing to override some
options or to protect only a few (based on the explicit wishes of a
uiopts-aware script).

Michael

Re: [PATCH] Adds 'stash.index' configuration option

[Jeff King]

On Thu, May 12, 2011 at 10:14:49AM +0200, Michael J Gruber wrote:

> > I would love to see something like this, but have we yet figured out all
> > of the issues, like:
> > 
> >   1. How do scripts wanting to call git programs suppress expansion of
> >      uiopts when they want predictable behavior?
> > 
> >   2. Depending on the solution to (1), how do scripts specify that they
> >      _do_ want to allow uiopts (e.g., because they know they are
> >      presenting the output to the user) for certain commands?
> > 
> >   3. Depending on (1) and (2), how do scripts differentiate when some
> >      options are OK in uiopts, but others are not? For example, it may
> >      be desirable for an invocation of diff-tree to have renames turned
> >      on by the user, but not for them to change the output format.
> > 
> 
> We haven't figured that out, but was the consensus: "Whatever, let's
> just keep adding single options." ?

I don't know. But short of coming up with a more global solution, what
do you want to do in the meantime? Forbid new config options of this
sort? I didn't see any consensus on that, either.

I'm not trying to be hostile, btw. I don't know what the right solution
is.

> > As much as it sucks to have a config option for each individual option,
> > there is at least some oversight of which options will not cause too
> > much of a problem when triggered automatically.
> 
> I just think we have too many commands which are ui and are used in
> scripts (e.g. log, commit, stash, just to name a few) for being able to
> decide that ourselves. Are we saying that people using "git stash" in a
> script have to deal themselves with a breakage caused by "--index" being
> a default for some users now?

I intentionally withheld any judgement on whether "stash --index" is a
safe option to add or not. I think that is a separate issue from whether
one should add such options, if they are considered safe.

> With a generic approach, we could protect all git-sh-setup using scripts
> right from the start, for example, while still allowing to override some
> options or to protect only a few (based on the explicit wishes of a
> uiopts-aware script).

Absolutely a solution like that would be better. Do you have a
particular proposal in mind? I know we've discussed it before, but I
didn't remember ever reaching any consensus on the right solution.

-Peff

RFC proposal: set git defaults options from config

[Michael J Gruber]

Mechanism
=========

I propose the following mechanism for setting default command line
options from the config:

options.<cmd> = <value>

is a "multivar" in git-config speak, i.e. it can appear multiple times.
When running "git <cmd> <opts>", our wrapper executes

git <cmd> <values> <opt>

where <values> is determined by the following rule in pseudocode:

if $GIT_OPTIONS_<cmd> is unset:
  <values> := empty
else:
  for <value> in $(git config --get-all options.cmd):
    if <value> matches the regexp in $GIT_OPTIONS_<CMD>:
      append <value> to <values>

Examples
========

* By default, no options can be overriden from config (other than those
which have config vars already, of course).

* A script which wants to protect options "foo" and "bar" of "cmd" from
being set by config sets GIT_OPTIONS_CMD="!(foo|bar)".

* A script which wants to allow overriding options "foo" and "bar" of
"cmd" by config (but nothing else) sets GIT_OPTIONS_CMD="foo|bar"

NOTES
=====

* This can be done by commit_pager_choice() or by a call right after
that in those places.
* regexp notation/version to be decided
* We should probably do this for long options only (and insert
"--<value>" rather than "<value>" to spare the "--" in config).
* We should probably do a prefix match.
* We could use GIT_OPTIONS_<CMD>_ALLOW and GIT_OPTIONS_<CMD>_DENY rather
than rely on negated regexps (if DENY matches deny, otherwise if ALLOW
matches allow, otherwise deny).
* We can get rid of a few config vars then...and may need to clean up
our option names.

Taking cover...

Michael

Re: RFC proposal: set git defaults options from config

[David Pisoni]

This has some interesting implications.  Consider the case at hand:
git-stash --index is a boolean switch.  It was not the default state,  
and it lacked any configuration override, so there was no '--no-index'  
switch provided.  If we make this change to git, presumably EVERY  
boolean flag like this in all the git subcommands needs to be backed  
with a '--no' counterpart.

Thinking this through a little further, there is the potential to want  
to override the configured value (in the case of non-booleans) with an  
explicit command line switch.  So now we have "precedence rules" for  
subcommand options. Probably simple to handle this for single vars,  
but harder for multivars.

My $0.02,
David

On May 12, 2011, at 7.35 , Michael J Gruber wrote:

> Mechanism
> =========
>
> I propose the following mechanism for setting default command line
> options from the config:
>
> options.<cmd> = <value>
>
> is a "multivar" in git-config speak, i.e. it can appear multiple  
> times.
> When running "git <cmd> <opts>", our wrapper executes
>
> git <cmd> <values> <opt>
>
> where <values> is determined by the following rule in pseudocode:
>
> if $GIT_OPTIONS_<cmd> is unset:
>  <values> := empty
> else:
>  for <value> in $(git config --get-all options.cmd):
>    if <value> matches the regexp in $GIT_OPTIONS_<CMD>:
>      append <value> to <values>
>
> Examples
> ========
>
> * By default, no options can be overriden from config (other than  
> those
> which have config vars already, of course).
>
> * A script which wants to protect options "foo" and "bar" of "cmd"  
> from
> being set by config sets GIT_OPTIONS_CMD="!(foo|bar)".
>
> * A script which wants to allow overriding options "foo" and "bar" of
> "cmd" by config (but nothing else) sets GIT_OPTIONS_CMD="foo|bar"
>
> NOTES
> =====
>
> * This can be done by commit_pager_choice() or by a call right after
> that in those places.
> * regexp notation/version to be decided
> * We should probably do this for long options only (and insert
> "--<value>" rather than "<value>" to spare the "--" in config).
> * We should probably do a prefix match.
> * We could use GIT_OPTIONS_<CMD>_ALLOW and GIT_OPTIONS_<CMD>_DENY  
> rather
> than rely on negated regexps (if DENY matches deny, otherwise if ALLOW
> matches allow, otherwise deny).
> * We can get rid of a few config vars then...and may need to clean up
> our option names.
>
> Taking cover...
>
> Michael

Re: RFC proposal: set git defaults options from config

[Jeff King]

On Thu, May 12, 2011 at 03:36:16PM -0700, David Pisoni wrote:

> This has some interesting implications.  Consider the case at hand:
> git-stash --index is a boolean switch.  It was not the default state,
> and it lacked any configuration override, so there was no
> '--no-index' switch provided.  If we make this change to git,
> presumably EVERY boolean flag like this in all the git subcommands
> needs to be backed with a '--no' counterpart.

Most of them already are, by virtue of parse-options. And I don't
think it's a bad thing for those that don't have one to get one.

> Thinking this through a little further, there is the potential to
> want to override the configured value (in the case of non-booleans)
> with an explicit command line switch.  So now we have "precedence
> rules" for subcommand options. Probably simple to handle this for
> single vars, but harder for multivars.

For single vars, which are most of it, it is pretty simple. For
multivars, mostly "--no-$option" should reset the multivar list
explicitly. I expect there are some oddballs where that is not the case,
though. For example, we just recently found some confusion with
resetting "git status" to its default after seeing "--porcelain".
So there would probably be some cleanup work there.

-Peff

Re: RFC proposal: set git defaults options from config

[Jeff King]

On Thu, May 12, 2011 at 04:35:11PM +0200, Michael J Gruber wrote:

> Mechanism
> =========
> 
> I propose the following mechanism for setting default command line
> options from the config:
> 
> options.<cmd> = <value>
> 
> is a "multivar" in git-config speak, i.e. it can appear multiple times.
> When running "git <cmd> <opts>", our wrapper executes
> 
> git <cmd> <values> <opt>
> 
> where <values> is determined by the following rule in pseudocode:
> 
> if $GIT_OPTIONS_<cmd> is unset:
>   <values> := empty
> else:
>   for <value> in $(git config --get-all options.cmd):
>     if <value> matches the regexp in $GIT_OPTIONS_<CMD>:
>       append <value> to <values>

As a user, how would I active this for all commands when not running a
script? I see why you defensively say "if unset, don't enable this
feature at all".  As a user, should I have to set GIT_OPTIONS_CMD for
everything that I want to configure? I hope not.

I think we need one extra variable to say generally "I am in strict
plumbing mode" or "I am in user mode". So you would want something like:

  if $GIT_STRICT is unset:
    <values> := $(git config --get-all options.cmd)
  else if $GIT_OPTIONS_<cmd> is unset:
    <values> := empty
  else:
    [match values by regex as you do]

But then you have a question of when GIT_STRICT gets set. An obvious
place is to set it in the git wrapper, so that "git foo" will have its
subcommands properly strict.

But that doesn't help scripts which are not called from the git wrapper;
they need to set GIT_STRICT themselves, so we need a phase-in period for
them to do so.

> NOTES
> =====
> 
> * This can be done by commit_pager_choice() or by a call right after
> that in those places.

Ah, so reading this, I have a sense that you were intending to make the
equivalent of GIT_STRICT be "am I running a pager" (or "am I outputting
to a terminal)?

Which is somewhat safer, as it is purely something for programs to opt
into. And as a heuristic, it's mostly good. I can come up with examples
where a script might not want to allow some options to be passed, even
though output is to the user, but they are probably stretching (e.g.,
something like "--allow-textconv" in a script that is meant to restrict
the users rights).

> * regexp notation/version to be decided

I think I would just as soon have a list of allowed options. We're
hopefully not doing the regex over the value of the option, like
"--pretty=foo is OK, but --pretty=bar is not". It seems like this
unnecessarily complicate the common case (you don't care what the value
is, but you have to tack on (|=.*) to every option matcher), and the
added flexibility is probably not going to be useful.

So I expect options regex are just going to look like:

  --(foo|bar|baz|bleep)

at which point we might as well just make it a list. And for the sake of
sanity, we may want to provide some default lists for scripts to OK,
like some minimal set of rev limiting options or something, so that
scripts don't end up specifying the same sets over and over.

> * We should probably do this for long options only (and insert
> "--<value>" rather than "<value>" to spare the "--" in config).

Yeah. Anything that doesn't have a long option and is useful enough to
be used in this way should probably get one.

> Taking cover...

I dunno. It's not so bad. But I think we probably want to start with an
environment variable to say "I am a script, be strict", let scripts
start picking that up, and then phase in the ability to turn on options
selectively.

-Peff

Re: RFC proposal: set git defaults options from config

[Michael J Gruber]

Jeff King venit, vidit, dixit 16.05.2011 13:02:
> On Thu, May 12, 2011 at 04:35:11PM +0200, Michael J Gruber wrote:
> 
>> Mechanism
>> =========
>>
>> I propose the following mechanism for setting default command line
>> options from the config:
>>
>> options.<cmd> = <value>
>>
>> is a "multivar" in git-config speak, i.e. it can appear multiple times.
>> When running "git <cmd> <opts>", our wrapper executes
>>
>> git <cmd> <values> <opt>
>>
>> where <values> is determined by the following rule in pseudocode:
>>
>> if $GIT_OPTIONS_<cmd> is unset:
>>   <values> := empty
>> else:
>>   for <value> in $(git config --get-all options.cmd):
>>     if <value> matches the regexp in $GIT_OPTIONS_<CMD>:
>>       append <value> to <values>
> 
> As a user, how would I active this for all commands when not running a
> script? I see why you defensively say "if unset, don't enable this
> feature at all".  As a user, should I have to set GIT_OPTIONS_CMD for
> everything that I want to configure? I hope not.

Yeah, sorry, I was a bit dense. I meant to activate it by default and
shut it off from git-sh-setup by default so that scripts are not
affected (but can choose to enable it selevtively).

> I think we need one extra variable to say generally "I am in strict
> plumbing mode" or "I am in user mode". So you would want something like:

> 
>   if $GIT_STRICT is unset:
>     <values> := $(git config --get-all options.cmd)
>   else if $GIT_OPTIONS_<cmd> is unset:
>     <values> := empty
>   else:
>     [match values by regex as you do]
> 
> But then you have a question of when GIT_STRICT gets set. An obvious
> place is to set it in the git wrapper, so that "git foo" will have its
> subcommands properly strict.

Yep.

> But that doesn't help scripts which are not called from the git wrapper;
> they need to set GIT_STRICT themselves, so we need a phase-in period for
> them to do so.

git-sh-setup

The phase-in is still needed for scripts which do use sh-setup, of course.

>> NOTES
>> =====
>>
>> * This can be done by commit_pager_choice() or by a call right after
>> that in those places.
> 
> Ah, so reading this, I have a sense that you were intending to make the
> equivalent of GIT_STRICT be "am I running a pager" (or "am I outputting
> to a terminal)?

As a default for the phase-in-phase I was hoping that would be safe enough.

> Which is somewhat safer, as it is purely something for programs to opt
> into. And as a heuristic, it's mostly good. I can come up with examples
> where a script might not want to allow some options to be passed, even
> though output is to the user, but they are probably stretching (e.g.,
> something like "--allow-textconv" in a script that is meant to restrict
> the users rights).
> 
>> * regexp notation/version to be decided
> 
> I think I would just as soon have a list of allowed options. We're
> hopefully not doing the regex over the value of the option, like
> "--pretty=foo is OK, but --pretty=bar is not". It seems like this
> unnecessarily complicate the common case (you don't care what the value
> is, but you have to tack on (|=.*) to every option matcher), and the
> added flexibility is probably not going to be useful.
> 
> So I expect options regex are just going to look like:
> 
>   --(foo|bar|baz|bleep)
> 
> at which point we might as well just make it a list. And for the sake of
> sanity, we may want to provide some default lists for scripts to OK,
> like some minimal set of rev limiting options or something, so that
> scripts don't end up specifying the same sets over and over.
> 
>> * We should probably do this for long options only (and insert
>> "--<value>" rather than "<value>" to spare the "--" in config).
> 
> Yeah. Anything that doesn't have a long option and is useful enough to
> be used in this way should probably get one.

Agreed!

>> Taking cover...
> 
> I dunno. It's not so bad. But I think we probably want to start with an
> environment variable to say "I am a script, be strict", let scripts
> start picking that up, and then phase in the ability to turn on options
> selectively.

GIT_BE_STRICT_I_AM_BRITISH

Michael