{"thread":{"id":"27329","subject":"[PATCH] Adds 'stash.index' configuration option","startedAt":"2011-05-11T22:57:33Z","lastAt":"2011-05-16T12:54:14Z","messageCount":14,"participants":["David Pisoni","Junio C Hamano","Michael J Gruber","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"167696","messageId":"D80C1130-8DE6-457E-B203-FCF25B8ED72C@gmail.com","threadId":"27329","inReplyTo":null,"subject":"[PATCH] Adds 'stash.index' configuration option","fromName":"David Pisoni","fromEmail":"dpisoni@gmail.com","sentAt":"2011-05-11T22:57:33Z","receivedAt":"2011-05-11T22:57:33Z","isPatch":true,"sender":{"key":"dpisoni@gmail.com","avatar":"https://gravatar.com/avatar/2af1be5a0f0b4c1ad2b0dec46699369bf778eb6b3a215832a58dd971fceac4a3?d=mp&s=160"},"body":"\nSetting 'stash.index' config option changes 'git-stash pop|apply' to  \nbehave\nas if '--index' switch is always supplied.\n'git-stash pop|apply' provides a --no-index switch to circumvent  \nconfig default.\n\nSigned-off-by: David Pisoni <dpisoni@gmail.com>\n---\nDocumentation/config.txt    |    5 +++++\nDocumentation/git-stash.txt |   10 +++++++---\ngit-stash.sh                |    7 ++++++-\nt/t3903-stash.sh            |   28 ++++++++++++++++++++++++++++\n4 files changed, 46 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 285c7f7..d794c40 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1746,6 +1746,11 @@ showbranch.default::\n\tThe default set of branches for linkgit:git-show-branch[1].\n\tSee linkgit:git-show-branch[1].\n\n+stash.index::\n+    A boolean to make linkgit:git-stash[1] default to the behavior of  \n--index\n+\twhen applying a stash to the working copy.  Can be circumvented by  \nusing\n+\t--no-index switch to linkgit:git-stash[1].  Defaults to false.\n+\nstatus.relativePaths::\n\tBy default, linkgit:git-status[1] shows paths relative to the\n\tcurrent directory. Setting this variable to `false` shows paths\ndiff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt\nindex 15f051f..de086ee 100644\n--- a/Documentation/git-stash.txt\n+++ b/Documentation/git-stash.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n'git stash' list [<options>]\n'git stash' show [<stash>]\n'git stash' drop [-q|--quiet] [<stash>]\n-'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]\n+'git stash' ( pop | apply ) [--[no-]index] [-q|--quiet] [<stash>]\n'git stash' branch <branchname> [<stash>]\n'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]  \n[<message>]]\n'git stash' clear\n@@ -89,7 +89,7 @@ show [<stash>]::\n\tit will accept any format known to 'git diff' (e.g., `git stash show\n\t-p stash@\\{1}` to view the second most recent stash in patch form).\n\n-pop [--index] [-q|--quiet] [<stash>]::\n+pop [--[no-]index] [-q|--quiet] [<stash>]::\n\n\tRemove a single stashed state from the stash list and apply it\n\ton top of the current working tree state, i.e., do the inverse\n@@ -105,10 +105,14 @@ tree's changes, but also the index's ones.  \nHowever, this can fail, when you\nhave conflicts (which are stored in the index, where you therefore can  \nno\nlonger apply the changes as they were originally).\n+\n+If the configuration option `stash.index` is set `pop` will behave as  \nif the\n+`--index` option is always in use, unless explicitly overridden with\n+`--no-index`.\n++\nWhen no `<stash>` is given, `stash@\\{0}` is assumed, otherwise  \n`<stash>` must\nbe a reference of the form `stash@\\{<revision>}`.\n\n-apply [--index] [-q|--quiet] [<stash>]::\n+apply [--[no-]index] [-q|--quiet] [<stash>]::\n\n\tLike `pop`, but do not remove the state from the stash list. Unlike  \n`pop`,\n\t`<stash>` may be any commit that looks like a commit created by\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 0a94036..7711bf6 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -31,6 +31,8 @@ else\n        reset_color=\nfi\n\n+CONFIG_INDEX=\"$(git config stash.index)\"\n+\nno_changes () {\n\tgit diff-index --quiet --cached HEAD --ignore-submodules -- &&\n\tgit diff-files --quiet --ignore-submodules\n@@ -256,7 +258,7 @@ parse_flags_and_rev()\n\n\tIS_STASH_LIKE=\n\tIS_STASH_REF=\n-\tINDEX_OPTION=\n+\tINDEX_OPTION=$CONFIG_INDEX\n\ts=\n\tw_commit=\n\tb_commit=\n@@ -277,6 +279,9 @@ parse_flags_and_rev()\n\t\t\t--index)\n\t\t\t\tINDEX_OPTION=--index\n\t\t\t;;\n+\t\t\t--no-index)\n+\t\t\t\tunset INDEX_OPTION\n+\t\t\t;;\n\t\t\t-*)\n\t\t\t\tFLAGS=\"${FLAGS}${FLAGS:+ }$opt\"\n\t\t\t;;\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 5c72540..4999682 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -84,6 +84,34 @@ test_expect_success 'apply stashed changes  \n(including index)' '\n\ttest 1 = $(git show HEAD:file)\n'\n\n+test_expect_success 'apply stashed changes (including index) with  \nindex config set' '\n+    git config stash.index yes &&\n+\tgit reset --hard HEAD^ &&\n+\techo 7 > other-file &&\n+\tgit add other-file &&\n+\ttest_tick &&\n+\tgit commit -m other-file &&\n+\tgit stash apply &&\n+\ttest 3 = $(cat file) &&\n+\ttest 2 = $(git show :file) &&\n+\ttest 1 = $(git show HEAD:file) &&\n+\tgit config --unset stash.index\n+'\n+\n+test_expect_success 'apply stashed changes (excluding index) with  \nindex config set' '\n+    git config stash.index yes &&\n+\tgit reset --hard &&\n+\techo 8 >other-file &&\n+\tgit add other-file &&\n+\ttest_tick &&\n+\tgit commit -m other-file &&\n+\tgit stash apply --no-index &&\n+\ttest 3 = $(cat file) &&\n+\ttest 1 = $(git show :file) &&\n+\ttest 1 = $(git show HEAD:file) &&\n+\tgit config --unset stash.index\n+'\n+\ntest_expect_success 'unstashing in a subdirectory' '\n\tgit reset --hard HEAD &&\n\tmkdir subdir &&\n-- \n1.7.5\n"},{"id":"167697","messageId":"7vfwoker7i.fsf@alter.siamese.dyndns.org","threadId":"27329","inReplyTo":"D80C1130-8DE6-457E-B203-FCF25B8ED72C@gmail.com","subject":"Re: [PATCH] Adds 'stash.index' configuration option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-11T23:46:09Z","receivedAt":"2011-05-11T23:46:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Pisoni <dpisoni@gmail.com> writes:\n\n> Setting 'stash.index' config option changes 'git-stash pop|apply' to\n> behave\n> as if '--index' switch is always supplied.\n> 'git-stash pop|apply' provides a --no-index switch to circumvent\n> config default.\n>\n> Signed-off-by: David Pisoni <dpisoni@gmail.com>\n\nYour MUA utterly mangled your whitespaces and linebreaks.  Please be\ncareful when you send a re-rolled series of this patch.  The section \"MUA\nSpecific hints\" of\n\n  http://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html\n\nmay be of help.  \n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 285c7f7..d794c40 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1746,6 +1746,11 @@ showbranch.default::\n> \tThe default set of branches for linkgit:git-show-branch[1].\n> \tSee linkgit:git-show-branch[1].\n>\n> +stash.index::\n> +    A boolean to make linkgit:git-stash[1] default to the behavior of\n> --index\n> +\twhen applying a stash to the working copy.  Can be\n> circumvented by using\n> +\t--no-index switch to linkgit:git-stash[1].  Defaults to false.\n\nThis says \"boolean\", so all of these should mean \"I do not want the\ncommand to default to --index\":\n\n\t[stash]\n                index = false\n                index = 0\n\nand all of these mean \"I do want the default --index\":\n\n\t[stash]\n\t\tindex\n                index = yes\n                index = 1\n\nI however do not think your implementation actually handle these\ncorrectly.\n\nSee below for one possible correct implementation, also the comment on the\nnecessity to test both sides of the coin.\n\n> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt\n> index 15f051f..de086ee 100644\n> --- a/Documentation/git-stash.txt\n> +++ b/Documentation/git-stash.txt\n> @@ -11,7 +11,7 @@ SYNOPSIS\n> 'git stash' list [<options>]\n> 'git stash' show [<stash>]\n> 'git stash' drop [-q|--quiet] [<stash>]\n> -'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]\n> +'git stash' ( pop | apply ) [--[no-]index] [-q|--quiet] [<stash>]\n\nThe --no-index is a very welcome addition, even if we did not have this\nnew configuration variable.  An alias \"git pop\" defined thusly:\n\n\t[alias]\n        \tpop = stash pop --index\n\ncan countermand it on demand with \"git pop --no-index\".\n\nPlease make it a separate patch, that comes before the addition of the\nconfiguration variable.  And then another patch to add the configuration\nvariable on top of it.\n\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 0a94036..7711bf6 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -31,6 +31,8 @@ else\n>        reset_color=\n> fi\n>\n> +CONFIG_INDEX=\"$(git config stash.index)\"\n\nThis needs to be something like\n\n\tCONFIG_INDEX=$(git config --bool stash.index)\n\t\nThen you would check the value against \"true\" later like this:\n\n> @@ -256,7 +258,7 @@ parse_flags_and_rev()\n>\n> \tIS_STASH_LIKE=\n> \tIS_STASH_REF=\n> -\tINDEX_OPTION=\n> +\tINDEX_OPTION=$CONFIG_INDEX\n\n\tif test \"$CONFIG_INDEX\" = true\n\tthen\n\t\tINDEX_OPTION=--index\n\telse\n        \tINDEX_OPTION=\n\tfi\n\n> @@ -277,6 +279,9 @@ parse_flags_and_rev()\n> \t\t\t--index)\n> \t\t\t\tINDEX_OPTION=--index\n> \t\t\t;;\n> +\t\t\t--no-index)\n> +\t\t\t\tunset INDEX_OPTION\n> +\t\t\t;;\n\nI'd rather sees this done as\n\n\t\t\t--no-index)\n\t\t\t\tINDEX_OPTION=\n\t\t\t\t;;\n\nas a later part of the existing code says\n\n\tif test -n \"$INDEX_OPTION\" && ...\n\nIOW, the code switches on the value of the variable, not on whether if it\nis set or unset.\n\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 5c72540..4999682 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -84,6 +84,34 @@ test_expect_success 'apply stashed changes\n> (including index)' '\n> \ttest 1 = $(git show HEAD:file)\n> '\n>\n> +test_expect_success 'apply stashed changes (including index) with\n> index config set' '\n> +    git config stash.index yes &&\n> +\tgit reset --hard HEAD^ &&\n> +\techo 7 > other-file &&\n\nWe write redirect from/to without an extra space, like you did\nin the next test to echo 8 into the same file.\n\n> +\tgit add other-file &&\n> +\ttest_tick &&\n> +\tgit commit -m other-file &&\n> +\tgit stash apply &&\n> +\ttest 3 = $(cat file) &&\n> +\ttest 2 = $(git show :file) &&\n> +\ttest 1 = $(git show HEAD:file) &&\n> +\tgit config --unset stash.index\n> +'\n\nUse test_when_finished early to arrange so that this unset will happen\neven when a step somewhere in the test fails.\n\nIt is a common mistake to write tests that only show off a shiny new toy,\nmaking sure the feature kicks in when it should, and totally forget to\nmake sure the feature does not kick in when it should not.  Always remeber\nto test both sides of the coin.\n\nCheck what should happen with at least these combinations:\n\n        stash.index set to...           command line says...\n ----------------------------------------------------------------\n        (not set at all)                (nothing)\n        (not set at all)                --index\n *      (not set at all)                --no-index\n *      (not set at all)                --index --no-index\n *      (not set at all)                --no-index --index\n *      false                           --index\n *      false                           --no-index\n *      true                            --index\n *      true                            --no-index\n ----------------------------------------------------------------\n\nThe cases marked with \"*\" are the possibilities your patch opens and need\nto have tests so that they are not broken when other people later change\nthe code (e.g. the command line parsing may break \"later one wins\" rule if\ndone carelessly).\n\nThanks.\n"},{"id":"167698","messageId":"7vboz8epbp.fsf@alter.siamese.dyndns.org","threadId":"27329","inReplyTo":"7vfwoker7i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Adds 'stash.index' configuration option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-12T00:26:50Z","receivedAt":"2011-05-12T00:26:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> Setting 'stash.index' config option changes 'git-stash pop|apply' to behave\n>> as if '--index' switch is always supplied.\n\nOne thing I forgot to say.  \"stash.index\" invites \"index _what_?\"\nNaming it to \"stash.useIndex\" may avoid such reaction.\n\nAlso, the current code has this comment:\n\n    #   INDEX_OPTION is set to --index if --index is specified.\n\nbut it probably makes sense to change it (in the first patch in the series\nthat adds --no-index support) to a boolean whose value can be either true\nor empty.\n\nThe reason why the very original code used INDEX_OPTION=--index may be\nbecause it did something like \"git some-cmd $INDEX_OPTION\", but that is\nnot what the current code does, and using \"either '--index' or ''\" as a\nform of boolean is confusing.\n\nIn other words, something like....\n\n\n\n git-stash.sh |   37 ++++++++++++++++++++-----------------\n 1 files changed, 20 insertions(+), 17 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 0a94036..eed2d1e 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -239,7 +239,7 @@ show_stash () {\n #   i_tree is set to the index tree\n #\n #   GIT_QUIET is set to t if -q is specified\n-#   INDEX_OPTION is set to --index if --index is specified.\n+#   INDEX_OPTION is set to 'true' if applying/popping also to the index.\n #   FLAGS is set to the remaining flags\n #\n # dies if:\n@@ -271,14 +271,17 @@ parse_flags_and_rev()\n \tfor opt\n \tdo\n \t\tcase \"$opt\" in\n-\t\t\t-q|--quiet)\n-\t\t\t\tGIT_QUIET=-t\n+\t\t-q|--quiet)\n+\t\t\tGIT_QUIET=-t\n+\t\t\t;;\n+\t\t--index)\n+\t\t\tINDEX_OPTION=true\n \t\t\t;;\n-\t\t\t--index)\n-\t\t\t\tINDEX_OPTION=--index\n+\t\t--no-index)\n+\t\t\tINDEX_OPTION=\n \t\t\t;;\n-\t\t\t-*)\n-\t\t\t\tFLAGS=\"${FLAGS}${FLAGS:+ }$opt\"\n+\t\t-*)\n+\t\t\tFLAGS=\"${FLAGS}${FLAGS:+ }$opt\"\n \t\t\t;;\n \t\tesac\n \tdone\n@@ -286,15 +289,15 @@ parse_flags_and_rev()\n \tset -- $REV\n \n \tcase $# in\n-\t\t0)\n-\t\t\thave_stash || die \"No stash found.\"\n-\t\t\tset -- ${ref_stash}@{0}\n+\t0)\n+\t\thave_stash || die \"No stash found.\"\n+\t\tset -- ${ref_stash}@{0}\n \t\t;;\n-\t\t1)\n-\t\t\t:\n+\t1)\n+\t\t:\n \t\t;;\n-\t\t*)\n-\t\t\tdie \"Too many revisions specified: $REV\"\n+\t*)\n+\t\tdie \"Too many revisions specified: $REV\"\n \t\t;;\n \tesac\n \n@@ -342,8 +345,8 @@ apply_stash () {\n \t\tdie 'Cannot apply a stash in the middle of a merge'\n \n \tunstashed_index_tree=\n-\tif test -n \"$INDEX_OPTION\" && test \"$b_tree\" != \"$i_tree\" &&\n-\t\t\ttest \"$c_tree\" != \"$i_tree\"\n+\tif test true = \"$INDEX_OPTION\" &&\n+\t\ttest \"$b_tree\" != \"$i_tree\" && test \"$c_tree\" != \"$i_tree\"\n \tthen\n \t\tgit diff-tree --binary $s^2^..$s^2 | git apply --cached\n \t\ttest $? -ne 0 &&\n@@ -387,7 +390,7 @@ apply_stash () {\n \telse\n \t\t# Merge conflict; keep the exit status from merge-recursive\n \t\tstatus=$?\n-\t\tif test -n \"$INDEX_OPTION\"\n+\t\tif test true = \"$INDEX_OPTION\"\n \t\tthen\n \t\t\techo >&2 'Index was not unstashed.'\n \t\tfi\n\n\n\n\n\t\n"},{"id":"167699","messageId":"30791C70-81D4-44D7-B2E7-814D001F3E12@gmail.com","threadId":"27329","inReplyTo":"7vboz8epbp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Adds 'stash.index' configuration option","fromName":"David Pisoni","fromEmail":"dpisoni@gmail.com","sentAt":"2011-05-12T00:48:58Z","receivedAt":"2011-05-12T00:48:58Z","isPatch":true,"sender":{"key":"dpisoni@gmail.com","avatar":"https://gravatar.com/avatar/2af1be5a0f0b4c1ad2b0dec46699369bf778eb6b3a215832a58dd971fceac4a3?d=mp&s=160"},"body":"\nOn May 11, 2011, at 17.26 , Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>>> Setting 'stash.index' config option changes 'git-stash pop|apply'  \n>>> to behave\n>>> as if '--index' switch is always supplied.\n>\n> One thing I forgot to say.  \"stash.index\" invites \"index _what_?\"\n> Naming it to \"stash.useIndex\" may avoid such reaction.\n\nNo objection.  I want the feature (scratching my own itch here), but I  \ndon't really care what it's called. :)\n\n>\n> Also, the current code has this comment:\n>\n>    #   INDEX_OPTION is set to --index if --index is specified.\n>\n> but it probably makes sense to change it (in the first patch in the  \n> series\n> that adds --no-index support) to a boolean whose value can be either  \n> true\n> or empty.\n\nIt seemed a little wonky to me also, but this is my first foray into  \nhacking on git and was concerned someone was depending on this,  \nstrange though it may be. git-blame fingers ef763129d for this oddity,  \ndating August 2010.\n\n>\n> The reason why the very original code used INDEX_OPTION=--index may be\n> because it did something like \"git some-cmd $INDEX_OPTION\", but that  \n> is\n> not what the current code does, and using \"either '--index' or ''\"  \n> as a\n> form of boolean is confusing.\n\nI agree.  I like your change, also.\nDoes this feature make sense to you overall?\n\n<SNIP>\n\nThanks,\nDavid\n"},{"id":"167700","messageId":"7v7h9weo1v.fsf@alter.siamese.dyndns.org","threadId":"27329","inReplyTo":"30791C70-81D4-44D7-B2E7-814D001F3E12@gmail.com","subject":"Re: [PATCH] Adds 'stash.index' configuration option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-12T00:54:20Z","receivedAt":"2011-05-12T00:54:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Pisoni <dpisoni@gmail.com> writes:\n\n> I agree.  I like your change, also.\n> Does this feature make sense to you overall?\n\nI am very much in favor of --no-index in the sense that I would prefer to\nhave it in the system than not having it.\n\nI am neutral to the configuration variable in the sense that I wouldn't\nmiss it if we don't have it and I wouldn't spend too much of my own\nbrain-cycle to add such a variable, but I wouldn't be disturbed too much\nby it if we had it in the system.\n"},{"id":"167707","messageId":"4DCB88C1.20105@drmicha.warpmail.net","threadId":"27329","inReplyTo":"D80C1130-8DE6-457E-B203-FCF25B8ED72C@gmail.com","subject":"Re: [PATCH] Adds 'stash.index' configuration option","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-12T07:14:09Z","receivedAt":"2011-05-12T07:14:09Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"David Pisoni venit, vidit, dixit 12.05.2011 00:57:\n> \n> Setting 'stash.index' config option changes 'git-stash pop|apply' to  \n> behave\n> as if '--index' switch is always supplied.\n> 'git-stash pop|apply' provides a --no-index switch to circumvent  \n> config default.\n\nThis is yet another incarnation of\n\nfoo.bar = true\n\nmeaning that command \"git foo\" defaults to \"git foo --bar\". (Admittedly,\nthis is about subcommands of foo.)\n\nIt has the same problems (possibly breaking scripts). But more\nimportantly, it inflates the code with every such incarnation we add.\nHave we really agreed that we introduce these one-by-one rather than\ndoing something generic like\n\nuiopts.<cmd> = <optionlist>\n\nwith which you would do\n\nuiopts.stash = \"--index\"\n\nand hopefully be script-safe (again, ignoring the subcommand issue)?\n\nMichael\n"},{"id":"167708","messageId":"20110512080425.GA11870@sigill.intra.peff.net","threadId":"27329","inReplyTo":"4DCB88C1.20105@drmicha.warpmail.net","subject":"Re: [PATCH] Adds 'stash.index' configuration option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-12T08:04:25Z","receivedAt":"2011-05-12T08:04:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 12, 2011 at 09:14:09AM +0200, Michael J Gruber wrote:\n\n> This is yet another incarnation of\n> \n> foo.bar = true\n> \n> meaning that command \"git foo\" defaults to \"git foo --bar\". (Admittedly,\n> this is about subcommands of foo.)\n> \n> It has the same problems (possibly breaking scripts). But more\n> importantly, it inflates the code with every such incarnation we add.\n> Have we really agreed that we introduce these one-by-one rather than\n> doing something generic like\n> \n> uiopts.<cmd> = <optionlist>\n> \n> with which you would do\n> \n> uiopts.stash = \"--index\"\n> \n> and hopefully be script-safe (again, ignoring the subcommand issue)?\n\nI would love to see something like this, but have we yet figured out all\nof the issues, like:\n\n  1. How do scripts wanting to call git programs suppress expansion of\n     uiopts when they want predictable behavior?\n\n  2. Depending on the solution to (1), how do scripts specify that they\n     _do_ want to allow uiopts (e.g., because they know they are\n     presenting the output to the user) for certain commands?\n\n  3. Depending on (1) and (2), how do scripts differentiate when some\n     options are OK in uiopts, but others are not? For example, it may\n     be desirable for an invocation of diff-tree to have renames turned\n     on by the user, but not for them to change the output format.\n\nAs much as it sucks to have a config option for each individual option,\nthere is at least some oversight of which options will not cause too\nmuch of a problem when triggered automatically.\n\n-Peff\n"},{"id":"167710","messageId":"4DCB96F9.2020700@drmicha.warpmail.net","threadId":"27329","inReplyTo":"20110512080425.GA11870@sigill.intra.peff.net","subject":"Re: [PATCH] Adds 'stash.index' configuration option","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-12T08:14:49Z","receivedAt":"2011-05-12T08:14:49Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 12.05.2011 10:04:\n> On Thu, May 12, 2011 at 09:14:09AM +0200, Michael J Gruber wrote:\n> \n>> This is yet another incarnation of\n>>\n>> foo.bar = true\n>>\n>> meaning that command \"git foo\" defaults to \"git foo --bar\". (Admittedly,\n>> this is about subcommands of foo.)\n>>\n>> It has the same problems (possibly breaking scripts). But more\n>> importantly, it inflates the code with every such incarnation we add.\n>> Have we really agreed that we introduce these one-by-one rather than\n>> doing something generic like\n>>\n>> uiopts.<cmd> = <optionlist>\n>>\n>> with which you would do\n>>\n>> uiopts.stash = \"--index\"\n>>\n>> and hopefully be script-safe (again, ignoring the subcommand issue)?\n> \n> I would love to see something like this, but have we yet figured out all\n> of the issues, like:\n> \n>   1. How do scripts wanting to call git programs suppress expansion of\n>      uiopts when they want predictable behavior?\n> \n>   2. Depending on the solution to (1), how do scripts specify that they\n>      _do_ want to allow uiopts (e.g., because they know they are\n>      presenting the output to the user) for certain commands?\n> \n>   3. Depending on (1) and (2), how do scripts differentiate when some\n>      options are OK in uiopts, but others are not? For example, it may\n>      be desirable for an invocation of diff-tree to have renames turned\n>      on by the user, but not for them to change the output format.\n> \n\nWe haven't figured that out, but was the consensus: \"Whatever, let's\njust keep adding single options.\" ?\n\n> As much as it sucks to have a config option for each individual option,\n> there is at least some oversight of which options will not cause too\n> much of a problem when triggered automatically.\n\nI just think we have too many commands which are ui and are used in\nscripts (e.g. log, commit, stash, just to name a few) for being able to\ndecide that ourselves. Are we saying that people using \"git stash\" in a\nscript have to deal themselves with a breakage caused by \"--index\" being\na default for some users now?\n\nWith a generic approach, we could protect all git-sh-setup using scripts\nright from the start, for example, while still allowing to override some\noptions or to protect only a few (based on the explicit wishes of a\nuiopts-aware script).\n\nMichael\n"},{"id":"167712","messageId":"20110512082210.GA16813@sigill.intra.peff.net","threadId":"27329","inReplyTo":"4DCB96F9.2020700@drmicha.warpmail.net","subject":"Re: [PATCH] Adds 'stash.index' configuration option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-12T08:22:10Z","receivedAt":"2011-05-12T08:22:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 12, 2011 at 10:14:49AM +0200, Michael J Gruber wrote:\n\n> > I would love to see something like this, but have we yet figured out all\n> > of the issues, like:\n> > \n> >   1. How do scripts wanting to call git programs suppress expansion of\n> >      uiopts when they want predictable behavior?\n> > \n> >   2. Depending on the solution to (1), how do scripts specify that they\n> >      _do_ want to allow uiopts (e.g., because they know they are\n> >      presenting the output to the user) for certain commands?\n> > \n> >   3. Depending on (1) and (2), how do scripts differentiate when some\n> >      options are OK in uiopts, but others are not? For example, it may\n> >      be desirable for an invocation of diff-tree to have renames turned\n> >      on by the user, but not for them to change the output format.\n> > \n> \n> We haven't figured that out, but was the consensus: \"Whatever, let's\n> just keep adding single options.\" ?\n\nI don't know. But short of coming up with a more global solution, what\ndo you want to do in the meantime? Forbid new config options of this\nsort? I didn't see any consensus on that, either.\n\nI'm not trying to be hostile, btw. I don't know what the right solution\nis.\n\n> > As much as it sucks to have a config option for each individual option,\n> > there is at least some oversight of which options will not cause too\n> > much of a problem when triggered automatically.\n> \n> I just think we have too many commands which are ui and are used in\n> scripts (e.g. log, commit, stash, just to name a few) for being able to\n> decide that ourselves. Are we saying that people using \"git stash\" in a\n> script have to deal themselves with a breakage caused by \"--index\" being\n> a default for some users now?\n\nI intentionally withheld any judgement on whether \"stash --index\" is a\nsafe option to add or not. I think that is a separate issue from whether\none should add such options, if they are considered safe.\n\n> With a generic approach, we could protect all git-sh-setup using scripts\n> right from the start, for example, while still allowing to override some\n> options or to protect only a few (based on the explicit wishes of a\n> uiopts-aware script).\n\nAbsolutely a solution like that would be better. Do you have a\nparticular proposal in mind? I know we've discussed it before, but I\ndidn't remember ever reaching any consensus on the right solution.\n\n-Peff\n"},{"id":"167734","messageId":"4DCBF01F.9040009@warpmail.net","threadId":"27329","inReplyTo":"20110512082210.GA16813@sigill.intra.peff.net","subject":"RFC proposal: set git defaults options from config","fromName":"Michael J Gruber","fromEmail":"drmicha@warpmail.net","sentAt":"2011-05-12T14:35:11Z","receivedAt":"2011-05-12T14:35:11Z","isPatch":false,"sender":{"key":"drmicha@warpmail.net","avatar":null},"body":"Mechanism\n=========\n\nI propose the following mechanism for setting default command line\noptions from the config:\n\noptions.<cmd> = <value>\n\nis a \"multivar\" in git-config speak, i.e. it can appear multiple times.\nWhen running \"git <cmd> <opts>\", our wrapper executes\n\ngit <cmd> <values> <opt>\n\nwhere <values> is determined by the following rule in pseudocode:\n\nif $GIT_OPTIONS_<cmd> is unset:\n  <values> := empty\nelse:\n  for <value> in $(git config --get-all options.cmd):\n    if <value> matches the regexp in $GIT_OPTIONS_<CMD>:\n      append <value> to <values>\n\nExamples\n========\n\n* By default, no options can be overriden from config (other than those\nwhich have config vars already, of course).\n\n* A script which wants to protect options \"foo\" and \"bar\" of \"cmd\" from\nbeing set by config sets GIT_OPTIONS_CMD=\"!(foo|bar)\".\n\n* A script which wants to allow overriding options \"foo\" and \"bar\" of\n\"cmd\" by config (but nothing else) sets GIT_OPTIONS_CMD=\"foo|bar\"\n\nNOTES\n=====\n\n* This can be done by commit_pager_choice() or by a call right after\nthat in those places.\n* regexp notation/version to be decided\n* We should probably do this for long options only (and insert\n\"--<value>\" rather than \"<value>\" to spare the \"--\" in config).\n* We should probably do a prefix match.\n* We could use GIT_OPTIONS_<CMD>_ALLOW and GIT_OPTIONS_<CMD>_DENY rather\nthan rely on negated regexps (if DENY matches deny, otherwise if ALLOW\nmatches allow, otherwise deny).\n* We can get rid of a few config vars then...and may need to clean up\nour option names.\n\nTaking cover...\n\nMichael\n"},{"id":"167764","messageId":"2235D93D-4F02-42D7-88B1-74F692D58AA5@gmail.com","threadId":"27329","inReplyTo":"4DCBF01F.9040009@warpmail.net","subject":"Re: RFC proposal: set git defaults options from config","fromName":"David Pisoni","fromEmail":"dpisoni@gmail.com","sentAt":"2011-05-12T22:36:16Z","receivedAt":"2011-05-12T22:36:16Z","isPatch":false,"sender":{"key":"dpisoni@gmail.com","avatar":"https://gravatar.com/avatar/2af1be5a0f0b4c1ad2b0dec46699369bf778eb6b3a215832a58dd971fceac4a3?d=mp&s=160"},"body":"This has some interesting implications.  Consider the case at hand:\ngit-stash --index is a boolean switch.  It was not the default state,  \nand it lacked any configuration override, so there was no '--no-index'  \nswitch provided.  If we make this change to git, presumably EVERY  \nboolean flag like this in all the git subcommands needs to be backed  \nwith a '--no' counterpart.\n\nThinking this through a little further, there is the potential to want  \nto override the configured value (in the case of non-booleans) with an  \nexplicit command line switch.  So now we have \"precedence rules\" for  \nsubcommand options. Probably simple to handle this for single vars,  \nbut harder for multivars.\n\nMy $0.02,\nDavid\n\nOn May 12, 2011, at 7.35 , Michael J Gruber wrote:\n\n> Mechanism\n> =========\n>\n> I propose the following mechanism for setting default command line\n> options from the config:\n>\n> options.<cmd> = <value>\n>\n> is a \"multivar\" in git-config speak, i.e. it can appear multiple  \n> times.\n> When running \"git <cmd> <opts>\", our wrapper executes\n>\n> git <cmd> <values> <opt>\n>\n> where <values> is determined by the following rule in pseudocode:\n>\n> if $GIT_OPTIONS_<cmd> is unset:\n>  <values> := empty\n> else:\n>  for <value> in $(git config --get-all options.cmd):\n>    if <value> matches the regexp in $GIT_OPTIONS_<CMD>:\n>      append <value> to <values>\n>\n> Examples\n> ========\n>\n> * By default, no options can be overriden from config (other than  \n> those\n> which have config vars already, of course).\n>\n> * A script which wants to protect options \"foo\" and \"bar\" of \"cmd\"  \n> from\n> being set by config sets GIT_OPTIONS_CMD=\"!(foo|bar)\".\n>\n> * A script which wants to allow overriding options \"foo\" and \"bar\" of\n> \"cmd\" by config (but nothing else) sets GIT_OPTIONS_CMD=\"foo|bar\"\n>\n> NOTES\n> =====\n>\n> * This can be done by commit_pager_choice() or by a call right after\n> that in those places.\n> * regexp notation/version to be decided\n> * We should probably do this for long options only (and insert\n> \"--<value>\" rather than \"<value>\" to spare the \"--\" in config).\n> * We should probably do a prefix match.\n> * We could use GIT_OPTIONS_<CMD>_ALLOW and GIT_OPTIONS_<CMD>_DENY  \n> rather\n> than rely on negated regexps (if DENY matches deny, otherwise if ALLOW\n> matches allow, otherwise deny).\n> * We can get rid of a few config vars then...and may need to clean up\n> our option names.\n>\n> Taking cover...\n>\n> Michael\n"},{"id":"167984","messageId":"20110516110256.GB23889@sigill.intra.peff.net","threadId":"27329","inReplyTo":"4DCBF01F.9040009@warpmail.net","subject":"Re: RFC proposal: set git defaults options from config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-16T11:02:56Z","receivedAt":"2011-05-16T11:02:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 12, 2011 at 04:35:11PM +0200, Michael J Gruber wrote:\n\n> Mechanism\n> =========\n> \n> I propose the following mechanism for setting default command line\n> options from the config:\n> \n> options.<cmd> = <value>\n> \n> is a \"multivar\" in git-config speak, i.e. it can appear multiple times.\n> When running \"git <cmd> <opts>\", our wrapper executes\n> \n> git <cmd> <values> <opt>\n> \n> where <values> is determined by the following rule in pseudocode:\n> \n> if $GIT_OPTIONS_<cmd> is unset:\n>   <values> := empty\n> else:\n>   for <value> in $(git config --get-all options.cmd):\n>     if <value> matches the regexp in $GIT_OPTIONS_<CMD>:\n>       append <value> to <values>\n\nAs a user, how would I active this for all commands when not running a\nscript? I see why you defensively say \"if unset, don't enable this\nfeature at all\".  As a user, should I have to set GIT_OPTIONS_CMD for\neverything that I want to configure? I hope not.\n\nI think we need one extra variable to say generally \"I am in strict\nplumbing mode\" or \"I am in user mode\". So you would want something like:\n\n  if $GIT_STRICT is unset:\n    <values> := $(git config --get-all options.cmd)\n  else if $GIT_OPTIONS_<cmd> is unset:\n    <values> := empty\n  else:\n    [match values by regex as you do]\n\nBut then you have a question of when GIT_STRICT gets set. An obvious\nplace is to set it in the git wrapper, so that \"git foo\" will have its\nsubcommands properly strict.\n\nBut that doesn't help scripts which are not called from the git wrapper;\nthey need to set GIT_STRICT themselves, so we need a phase-in period for\nthem to do so.\n\n> NOTES\n> =====\n> \n> * This can be done by commit_pager_choice() or by a call right after\n> that in those places.\n\nAh, so reading this, I have a sense that you were intending to make the\nequivalent of GIT_STRICT be \"am I running a pager\" (or \"am I outputting\nto a terminal)?\n\nWhich is somewhat safer, as it is purely something for programs to opt\ninto. And as a heuristic, it's mostly good. I can come up with examples\nwhere a script might not want to allow some options to be passed, even\nthough output is to the user, but they are probably stretching (e.g.,\nsomething like \"--allow-textconv\" in a script that is meant to restrict\nthe users rights).\n\n> * regexp notation/version to be decided\n\nI think I would just as soon have a list of allowed options. We're\nhopefully not doing the regex over the value of the option, like\n\"--pretty=foo is OK, but --pretty=bar is not\". It seems like this\nunnecessarily complicate the common case (you don't care what the value\nis, but you have to tack on (|=.*) to every option matcher), and the\nadded flexibility is probably not going to be useful.\n\nSo I expect options regex are just going to look like:\n\n  --(foo|bar|baz|bleep)\n\nat which point we might as well just make it a list. And for the sake of\nsanity, we may want to provide some default lists for scripts to OK,\nlike some minimal set of rev limiting options or something, so that\nscripts don't end up specifying the same sets over and over.\n\n> * We should probably do this for long options only (and insert\n> \"--<value>\" rather than \"<value>\" to spare the \"--\" in config).\n\nYeah. Anything that doesn't have a long option and is useful enough to\nbe used in this way should probably get one.\n\n> Taking cover...\n\nI dunno. It's not so bad. But I think we probably want to start with an\nenvironment variable to say \"I am a script, be strict\", let scripts\nstart picking that up, and then phase in the ability to turn on options\nselectively.\n\n-Peff\n"},{"id":"167985","messageId":"20110516110545.GC23889@sigill.intra.peff.net","threadId":"27329","inReplyTo":"2235D93D-4F02-42D7-88B1-74F692D58AA5@gmail.com","subject":"Re: RFC proposal: set git defaults options from config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-16T11:05:45Z","receivedAt":"2011-05-16T11:05:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 12, 2011 at 03:36:16PM -0700, David Pisoni wrote:\n\n> This has some interesting implications.  Consider the case at hand:\n> git-stash --index is a boolean switch.  It was not the default state,\n> and it lacked any configuration override, so there was no\n> '--no-index' switch provided.  If we make this change to git,\n> presumably EVERY boolean flag like this in all the git subcommands\n> needs to be backed with a '--no' counterpart.\n\nMost of them already are, by virtue of parse-options. And I don't\nthink it's a bad thing for those that don't have one to get one.\n\n> Thinking this through a little further, there is the potential to\n> want to override the configured value (in the case of non-booleans)\n> with an explicit command line switch.  So now we have \"precedence\n> rules\" for subcommand options. Probably simple to handle this for\n> single vars, but harder for multivars.\n\nFor single vars, which are most of it, it is pretty simple. For\nmultivars, mostly \"--no-$option\" should reset the multivar list\nexplicitly. I expect there are some oddballs where that is not the case,\nthough. For example, we just recently found some confusion with\nresetting \"git status\" to its default after seeing \"--porcelain\".\nSo there would probably be some cleanup work there.\n\n-Peff\n"},{"id":"167990","messageId":"4DD11E76.1010707@drmicha.warpmail.net","threadId":"27329","inReplyTo":"20110516110256.GB23889@sigill.intra.peff.net","subject":"Re: RFC proposal: set git defaults options from config","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-16T12:54:14Z","receivedAt":"2011-05-16T12:54:14Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 16.05.2011 13:02:\n> On Thu, May 12, 2011 at 04:35:11PM +0200, Michael J Gruber wrote:\n> \n>> Mechanism\n>> =========\n>>\n>> I propose the following mechanism for setting default command line\n>> options from the config:\n>>\n>> options.<cmd> = <value>\n>>\n>> is a \"multivar\" in git-config speak, i.e. it can appear multiple times.\n>> When running \"git <cmd> <opts>\", our wrapper executes\n>>\n>> git <cmd> <values> <opt>\n>>\n>> where <values> is determined by the following rule in pseudocode:\n>>\n>> if $GIT_OPTIONS_<cmd> is unset:\n>>   <values> := empty\n>> else:\n>>   for <value> in $(git config --get-all options.cmd):\n>>     if <value> matches the regexp in $GIT_OPTIONS_<CMD>:\n>>       append <value> to <values>\n> \n> As a user, how would I active this for all commands when not running a\n> script? I see why you defensively say \"if unset, don't enable this\n> feature at all\".  As a user, should I have to set GIT_OPTIONS_CMD for\n> everything that I want to configure? I hope not.\n\nYeah, sorry, I was a bit dense. I meant to activate it by default and\nshut it off from git-sh-setup by default so that scripts are not\naffected (but can choose to enable it selevtively).\n\n> I think we need one extra variable to say generally \"I am in strict\n> plumbing mode\" or \"I am in user mode\". So you would want something like:\n\n> \n>   if $GIT_STRICT is unset:\n>     <values> := $(git config --get-all options.cmd)\n>   else if $GIT_OPTIONS_<cmd> is unset:\n>     <values> := empty\n>   else:\n>     [match values by regex as you do]\n> \n> But then you have a question of when GIT_STRICT gets set. An obvious\n> place is to set it in the git wrapper, so that \"git foo\" will have its\n> subcommands properly strict.\n\nYep.\n\n> But that doesn't help scripts which are not called from the git wrapper;\n> they need to set GIT_STRICT themselves, so we need a phase-in period for\n> them to do so.\n\ngit-sh-setup\n\nThe phase-in is still needed for scripts which do use sh-setup, of course.\n\n>> NOTES\n>> =====\n>>\n>> * This can be done by commit_pager_choice() or by a call right after\n>> that in those places.\n> \n> Ah, so reading this, I have a sense that you were intending to make the\n> equivalent of GIT_STRICT be \"am I running a pager\" (or \"am I outputting\n> to a terminal)?\n\nAs a default for the phase-in-phase I was hoping that would be safe enough.\n\n> Which is somewhat safer, as it is purely something for programs to opt\n> into. And as a heuristic, it's mostly good. I can come up with examples\n> where a script might not want to allow some options to be passed, even\n> though output is to the user, but they are probably stretching (e.g.,\n> something like \"--allow-textconv\" in a script that is meant to restrict\n> the users rights).\n> \n>> * regexp notation/version to be decided\n> \n> I think I would just as soon have a list of allowed options. We're\n> hopefully not doing the regex over the value of the option, like\n> \"--pretty=foo is OK, but --pretty=bar is not\". It seems like this\n> unnecessarily complicate the common case (you don't care what the value\n> is, but you have to tack on (|=.*) to every option matcher), and the\n> added flexibility is probably not going to be useful.\n> \n> So I expect options regex are just going to look like:\n> \n>   --(foo|bar|baz|bleep)\n> \n> at which point we might as well just make it a list. And for the sake of\n> sanity, we may want to provide some default lists for scripts to OK,\n> like some minimal set of rev limiting options or something, so that\n> scripts don't end up specifying the same sets over and over.\n> \n>> * We should probably do this for long options only (and insert\n>> \"--<value>\" rather than \"<value>\" to spare the \"--\" in config).\n> \n> Yeah. Anything that doesn't have a long option and is useful enough to\n> be used in this way should probably get one.\n\nAgreed!\n\n>> Taking cover...\n> \n> I dunno. It's not so bad. But I think we probably want to start with an\n> environment variable to say \"I am a script, be strict\", let scripts\n> start picking that up, and then phase in the ability to turn on options\n> selectively.\n\nGIT_BE_STRICT_I_AM_BRITISH\n\nMichael\n"}]}