patchAdds 'stash.index' configuration option

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(-)

David Pisoni <dpisoni@gmail.com> writes:

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.  

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.

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.

This needs to be something like

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

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

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.

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

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.

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

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(-)

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

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

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.

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

<SNIP>

Thanks, David

David Pisoni <dpisoni@gmail.com> writes:

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.

David Pisoni venit, vidit, dixit 12.05.2011 00:57:

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

On Thu, May 12, 2011 at 09:14:09AM +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.

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

Jeff King venit, vidit, dixit 12.05.2011 10:04:

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

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

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

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.

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.

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

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

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:

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

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.

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

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

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.

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).

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.

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

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

Jeff King venit, vidit, dixit 16.05.2011 13:02:

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).

Yep.

git-sh-setup

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

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

Agreed!

GIT_BE_STRICT_I_AM_BRITISH

Michael