{"thread":{"id":"44522","subject":"[PATCH 0/3] Fix problems with rebase -i when core.commentchar is defined","startedAt":"2016-11-21T14:18:29Z","lastAt":"2016-11-23T11:05:47Z","messageCount":40,"participants":["Johannes Schindelin","Jacob Keller","Junio C Hamano","Johannes Sixt","Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"306215","messageId":"cover.1479737858.git.johannes.schindelin@gmx.de","threadId":"44522","inReplyTo":null,"subject":"[PATCH 0/3] Fix problems with rebase -i when core.commentchar is defined","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-11-21T14:18:01Z","receivedAt":"2016-11-21T14:18:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The Git for Windows project recently got a bug report that detailed how\n`git rebase -i` fails when core.commentchar=auto:\n\n\thttps://groups.google.com/forum/#!topic/git-for-windows/eOZKjkgyX1Q\n\nThis patch series fixes rebase -i's handling of core.commentchar.\n\n\nJohannes Schindelin (3):\n rebase -i: identify problems with core.commentchar\n stripspace: respect repository config\n rebase -i: handle core.commentChar=auto\n\n builtin/stripspace.c | 4 +++-\n git-rebase--interactive.sh | 13 +++++++++++--\n t/t0030-stripspace.sh | 7 +++++++\n t/t3404-rebase-interactive.sh | 12 ++++++++++++\n 4 files changed, 33 insertions(+), 3 deletions(-)\n\n\nbase-commit: 1310affe024fba407bff55dbe65cd6d670c8a32d\nPublished-As: https://github.com/dscho/git/releases/tag/rebase-i-commentchar-v1\nFetch-It-Via: git fetch https://github.com/dscho/git rebase-i-commentchar-v1\n\n-- \n2.10.1.583.g721a9e0\n\n"},{"id":"306216","messageId":"5567548295222401fab10d3f2901c1787afbfd07.1479737858.git.johannes.schindelin@gmx.de","threadId":"44522","inReplyTo":"cover.1479737858.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/3] stripspace: respect repository config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-11-21T14:18:24Z","receivedAt":"2016-11-21T14:18:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When eff80a9 (Allow custom \"comment char\", 2013-01-16) taught the\n`stripspace` command to respect the config setting `core.commentChar`,\nit forgot that this variable may be defined in .git/config.\n\nSo when rebasing interactively with a commentChar defined in the current\nrepository's config, the help text at the bottom of the edit script\npotentially used an incorrect comment character. This was not only\nfunny-looking, but also resulted in tons of warnings like this one:\n\n\tWarning: the command isn't recognized in the following line\n\t - #\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/stripspace.c | 4 +++-\n t/t0030-stripspace.sh | 2 +-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/stripspace.c b/builtin/stripspace.c\nindex 15e716e..1e62a00 100644\n--- a/builtin/stripspace.c\n+++ b/builtin/stripspace.c\n@@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n \tif (argc)\n \t\tusage_with_options(stripspace_usage, options);\n \n-\tif (mode == STRIP_COMMENTS || mode == COMMENT_LINES)\n+\tif (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {\n+\t\tsetup_git_directory_gently(NULL);\n \t\tgit_config(git_default_config, NULL);\n+\t}\n \n \tif (strbuf_read(&buf, 0, 1024) < 0)\n \t\tdie_errno(\"could not read the input\");\ndiff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh\nindex 202ac07..67f77df 100755\n--- a/t/t0030-stripspace.sh\n+++ b/t/t0030-stripspace.sh\n@@ -432,7 +432,7 @@ test_expect_success '-c with changed comment char' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure '-c with comment char defined in .git/config' '\n+test_expect_success '-c with comment char defined in .git/config' '\n \ttest_config core.commentchar = &&\n \tprintf \"= foo\\n\" >expect &&\n \tprintf \"foo\" | git stripspace -c >actual &&\n-- \n2.10.1.583.g721a9e0\n\n\n"},{"id":"306217","messageId":"f47dce15719954d6d4d8a550856757366871143e.1479737858.git.johannes.schindelin@gmx.de","threadId":"44522","inReplyTo":"cover.1479737858.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/3] rebase -i: identify problems with core.commentchar","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-11-21T14:18:05Z","receivedAt":"2016-11-21T14:18:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The interactive rebase does not currently play well with\ncore.commentchar. Let's add some tests to identify those problems.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t0030-stripspace.sh | 7 +++++++\n t/t3404-rebase-interactive.sh | 12 ++++++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh\nindex 29e91d8..202ac07 100755\n--- a/t/t0030-stripspace.sh\n+++ b/t/t0030-stripspace.sh\n@@ -432,6 +432,13 @@ test_expect_success '-c with changed comment char' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure '-c with comment char defined in .git/config' '\n+\ttest_config core.commentchar = &&\n+\tprintf \"= foo\\n\" >expect &&\n+\tprintf \"foo\" | git stripspace -c >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'avoid SP-HT sequence in commented line' '\n \tprintf \"#\\tone\\n#\\n# two\\n\" >expect &&\n \tprintf \"\\tone\\n\\ntwo\\n\" | git stripspace -c >actual &&\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex e38e296..e080399 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -976,6 +976,18 @@ test_expect_success 'rebase -i respects core.commentchar' '\n \ttest B = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+test_expect_failure 'rebase -i respects core.commentchar=auto' '\n+\ttest_config core.commentchar auto &&\n+\twrite_script copy-edit-script.sh <<-\\EOF &&\n+\tcp \"$1\" edit-script\n+\tEOF\n+\ttest_set_editor \"$(pwd)/copy-edit-script.sh\" &&\n+\ttest_when_finished \"git rebase --abort || :\" &&\n+\tgit rebase -i HEAD^ &&\n+\tgrep \"^#\" edit-script &&\n+\ttest -z \"$(grep -ve \"^#\" -e \"^\\$\" -e \"^pick\" edit-script)\"\n+'\n+\n test_expect_success 'rebase -i, with and specified as :/quuxery' '\n \ttest_when_finished \"git branch -D torebase\" &&\n \tgit checkout -b torebase branch1 &&\n-- \n2.10.1.583.g721a9e0\n\n\n"},{"id":"306218","messageId":"9ef529a4fbb60990a91d7bbfdd49c6d20d49e442.1479737858.git.johannes.schindelin@gmx.de","threadId":"44522","inReplyTo":"cover.1479737858.git.johannes.schindelin@gmx.de","subject":"[PATCH 3/3] rebase -i: handle core.commentChar=auto","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-11-21T14:18:29Z","receivedAt":"2016-11-21T14:19:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When 84c9dc2 (commit: allow core.commentChar=auto for character auto\nselection, 2014-05-17) extended the core.commentChar functionality to\nallow for the value 'auto', it forgot that rebase -i was already taught to\nhandle core.commentChar, and in turn forgot to let rebase -i handle that\nnew value gracefully.\n\nReported by Taufiq Hoven.\n\nSigned-off-by: Johannes Schindelin \n---\n git-rebase--interactive.sh | 13 +++++++++++--\n t/t3404-rebase-interactive.sh | 2 +-\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex ca994c5..6bb740c 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -93,8 +93,17 @@ eval '\n GIT_CHERRY_PICK_HELP=\"$resolvemsg\"\n export GIT_CHERRY_PICK_HELP\n \n-comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)\n-: ${comment_char:=#}\n+comment_char=$(git config --get core.commentchar 2>/dev/null)\n+case \"$comment_char\" in\n+''|auto)\n+\tcomment_char=#\n+\t;;\n+?)\n+\t;;\n+*)\n+\tcomment_char=$(comment_char | cut -c1)\n+\t;;\n+esac\n \n warn () {\n \tprintf '%s\\n' \"$*\" >&2\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex e080399..0f3d177 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -976,7 +976,7 @@ test_expect_success 'rebase -i respects core.commentchar' '\n \ttest B = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n-test_expect_failure 'rebase -i respects core.commentchar=auto' '\n+test_expect_success 'rebase -i respects core.commentchar=auto' '\n \ttest_config core.commentchar auto &&\n \twrite_script copy-edit-script.sh <<-\\EOF &&\n \tcp \"$1\" edit-script\n-- \n2.10.1.583.g721a9e0\n"},{"id":"306221","messageId":"CA+P7+xqq7rNy82_RAoyiJjrC7TwboVJ8UYohARfE5fWNhELeAw@mail.gmail.com","threadId":"44522","inReplyTo":"cover.1479737858.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/3] Fix problems with rebase -i when core.commentchar is defined","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-11-21T16:58:54Z","receivedAt":"2016-11-21T16:59:42Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Nov 21, 2016 at 6:18 AM, Johannes Schindelin\n wrote:\n> The Git for Windows project recently got a bug report that detailed how\n> `git rebase -i` fails when core.commentchar=auto:\n>\n> https://groups.google.com/forum/#!topic/git-for-windows/eOZKjkgyX1Q\n>\n> This patch series fixes rebase -i's handling of core.commentchar.\n>\n>\n> Johannes Schindelin (3):\n> rebase -i: identify problems with core.commentchar\n> stripspace: respect repository config\n> rebase -i: handle core.commentChar=auto\n>\n> builtin/stripspace.c | 4 +++-\n> git-rebase--interactive.sh | 13 +++++++++++--\n> t/t0030-stripspace.sh | 7 +++++++\n> t/t3404-rebase-interactive.sh | 12 ++++++++++++\n> 4 files changed, 33 insertions(+), 3 deletions(-)\n>\n>\n> base-commit: 1310affe024fba407bff55dbe65cd6d670c8a32d\n> Published-As: https://github.com/dscho/git/releases/tag/rebase-i-commentchar-v1\n> Fetch-It-Via: git fetch https://github.com/dscho/git rebase-i-commentchar-v1\n>\n> --\n> 2.10.1.583.g721a9e0\n>\n\nThe series makes sense to me.\n\nThanks,\nJake\n"},{"id":"306226","messageId":"xmqqbmx8k8c0.fsf@gitster.mtv.corp.google.com","threadId":"44522","inReplyTo":"f47dce15719954d6d4d8a550856757366871143e.1479737858.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-21T18:15:43Z","receivedAt":"2016-11-21T18:15:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh\n> index 29e91d8..202ac07 100755\n> --- a/t/t0030-stripspace.sh\n> +++ b/t/t0030-stripspace.sh\n> @@ -432,6 +432,13 @@ test_expect_success '-c with changed comment char' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_failure '-c with comment char defined in .git/config' '\n> +\ttest_config core.commentchar = &&\n> +\tprintf \"= foo\\n\" >expect &&\n> +\tprintf \"foo\" | git stripspace -c >actual &&\n\nWe'd want \"\\n\" on this printf to match the one before as well, as\nthis test is not about \"does stripspace complete an incomplete\nline?\", I think. \n\nI could amend it while queuing, but I need to know if I am missing a\nreason why this must be an incomplete line before doing so.\n\n> +\ttest_cmp expect actual\n> +'\n> +\n\nIs this a recent regression? When applied on top of 'maint' or\nolder, it seems to pass just fine.\n\n ... Goes and looks ...\n\nInteresting. Peff's b9605bc4f2 (\"config: only read .git/config from\nconfigured repos\", 2016-09-12) is where this starts failing, which\nis understandable given the code change to builtin/stripspace.c in\n[2/3]. \n\nThe analysis of the log message in [2/3] is wrong and needs\nupdating, though. In the old world order it worked by accident to\ncall git_config() without calling setup_git_directory(); after\nb9605bc4f2, that no longer is valid and is exposed as a bug. \n\nYour [2/3] is a good fix for that change.\n\nIn any case, well spotted.\n\n> test_expect_success 'avoid SP-HT sequence in commented line' '\n> \tprintf \"#\\tone\\n#\\n# two\\n\" >expect &&\n> \tprintf \"\\tone\\n\\ntwo\\n\" | git stripspace -c >actual &&\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index e38e296..e080399 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -976,6 +976,18 @@ test_expect_success 'rebase -i respects core.commentchar' '\n> \ttest B = $(git cat-file commit HEAD^ | sed -ne \\$p)\n> '\n> \n> +test_expect_failure 'rebase -i respects core.commentchar=auto' '\n> +\ttest_config core.commentchar auto &&\n> +\twrite_script copy-edit-script.sh <<-\\EOF &&\n> +\tcp \"$1\" edit-script\n> +\tEOF\n> +\ttest_set_editor \"$(pwd)/copy-edit-script.sh\" &&\n> +\ttest_when_finished \"git rebase --abort || :\" &&\n> +\tgit rebase -i HEAD^ &&\n> +\tgrep \"^#\" edit-script &&\n\nThis was added for debugging that was forgotten?\n\n> +\ttest -z \"$(grep -ve \"^#\" -e \"^\\$\" -e \"^pick\" edit-script)\"\n\nThis says \"There shouldn't be any line left once we remove\n'#'-commented lines, empty lines and pick insns.\". OK.\n\nThe correction in [3/3] seems good.\n\n> +'\n> +\n> test_expect_success 'rebase -i, with and specified as :/quuxery' '\n> \ttest_when_finished \"git branch -D torebase\" &&\n> \tgit checkout -b torebase branch1 &&\n"},{"id":"306228","messageId":"xmqq7f7wk7x9.fsf@gitster.mtv.corp.google.com","threadId":"44522","inReplyTo":"xmqqbmx8k8c0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-21T18:24:34Z","receivedAt":"2016-11-21T18:24:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +test_expect_failure '-c with comment char defined in .git/config' '\n>> +\ttest_config core.commentchar = &&\n>> +\tprintf \"= foo\\n\" >expect &&\n>> +\tprintf \"foo\" | git stripspace -c >actual &&\n>\n> We'd want \"\\n\" on this printf to match the one before as well, as\n> this test is not about \"does stripspace complete an incomplete\n> line?\", I think. \n>\n> I could amend it while queuing, but I need to know if I am missing a\n> reason why this must be an incomplete line before doing so.\n>\n>> +\ttest_cmp expect actual\n>> +'\n>> +\n>\n> Is this a recent regression? When applied on top of 'maint' or\n> older, it seems to pass just fine.\n\nI think we can force failure by running this test somewhere other\nthan the top level of the working tree. A set of proposed amends\nincoming ...\n"},{"id":"306229","messageId":"975c0002-8afe-efa6-d967-72f2a0d21169@kdbg.org","threadId":"44522","inReplyTo":"9ef529a4fbb60990a91d7bbfdd49c6d20d49e442.1479737858.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-11-21T18:26:51Z","receivedAt":"2016-11-21T18:26:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 21.11.2016 um 15:18 schrieb Johannes Schindelin:\n> -comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)\n> -: ${comment_char:=#}\n> +comment_char=$(git config --get core.commentchar 2>/dev/null)\n> +case \"$comment_char\" in\n> +''|auto)\n> +\tcomment_char=#\n> +\t;;\n> +?)\n> +\t;;\n> +*)\n> +\tcomment_char=$(comment_char | cut -c1)\n\ncomment_char is a command? Did you mean\n\n\tcomment_char=$(echo \"$comment_char\" | cut -c1)\n\nIt could be written without forking a process:\n\n\tcomment_char=${comment_char%${comment_char#?}}\n\n(aka \"remove from the end what remains after removing the first character\")\n\n-- Hannes\n\n"},{"id":"306230","messageId":"xmqq37ikk762.fsf@gitster.mtv.corp.google.com","threadId":"44522","inReplyTo":"975c0002-8afe-efa6-d967-72f2a0d21169@kdbg.org","subject":"Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-21T18:40:53Z","receivedAt":"2016-11-21T18:41:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> comment_char is a command? Did you mean\n>\n> \tcomment_char=$(echo \"$comment_char\" | cut -c1)\n\n;-)\n\n> It could be written without forking a process:\n>\n> \tcomment_char=${comment_char%${comment_char#?}}\n>\n> (aka \"remove from the end what remains after removing the first character\")\n\nHopefully nobody would include any glob metacharacters in there,\ne.g. \"core.commentchar='=*'\", which would break that?\n\n\n"},{"id":"306231","messageId":"20161121184931.36nuhg2h7u6mjsld@sigill.intra.peff.net","threadId":"44522","inReplyTo":"xmqqbmx8k8c0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-21T18:49:32Z","receivedAt":"2016-11-21T18:50:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 21, 2016 at 10:15:43AM -0800, Junio C Hamano wrote:\n\n> > +\ttest_cmp expect actual\n> > +'\n> > +\n> \n> Is this a recent regression? When applied on top of 'maint' or\n> older, it seems to pass just fine.\n> \n> ... Goes and looks ...\n> \n> Interesting. Peff's b9605bc4f2 (\"config: only read .git/config from\n> configured repos\", 2016-09-12) is where this starts failing, which\n> is understandable given the code change to builtin/stripspace.c in\n> [2/3]. \n> \n> The analysis of the log message in [2/3] is wrong and needs\n> updating, though. In the old world order it worked by accident to\n> call git_config() without calling setup_git_directory(); after\n> b9605bc4f2, that no longer is valid and is exposed as a bug. \n\nYeah, I noticed that while reading the patch. My b9605bc4f2 did regress\nthis case, but called out the fact that \"cd subdir && git stripspace\"\nwould never have worked. So one step back, 2 steps forward, and Dscho's\npatch is the right step forward.\n\n-Peff\n"},{"id":"306233","messageId":"8b783f3d-8fda-b5cb-239f-895b10a22a73@kdbg.org","threadId":"44522","inReplyTo":"xmqq37ikk762.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-11-21T18:58:41Z","receivedAt":"2016-11-21T18:58:50Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 21.11.2016 um 19:40 schrieb Junio C Hamano:\n> Johannes Sixt writes:\n>> It could be written without forking a process:\n>>\n>> \tcomment_char=${comment_char%${comment_char#?}}\n>>\n>> (aka \"remove from the end what remains after removing the first character\")\n>\n> Hopefully nobody would include any glob metacharacters in there,\n> e.g. \"core.commentchar='=*'\", which would break that?\n\nHeh. I tested a few variations, but not this one. Make it\n\n \tcomment_char=${comment_char%\"${comment_char#?}\"}\n\n;)\n\n-- Hannes\n\n"},{"id":"306234","messageId":"20161121190514.18574-1-gitster@pobox.com","threadId":"44522","inReplyTo":"xmqq7f7wk7x9.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 1/3] rebase -i: highlight problems with core.commentchar","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-21T19:05:12Z","receivedAt":"2016-11-21T19:05:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Johannes Schindelin \n\nThe interactive rebase does not currently play well with\ncore.commentchar. Let's add some tests to highlight those problems\nthat will be fixed in the remainder of the series.\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Junio C Hamano \n---\n t/t0030-stripspace.sh | 9 +++++++++\n t/t3404-rebase-interactive.sh | 11 +++++++++++\n 2 files changed, 20 insertions(+)\n\ndiff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh\nindex 29e91d861c..c1f6411eb2 100755\n--- a/t/t0030-stripspace.sh\n+++ b/t/t0030-stripspace.sh\n@@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure '-c with comment char defined in .git/config' '\n+\ttest_config core.commentchar = &&\n+\tprintf \"= foo\\n\" >expect &&\n+\tprintf \"foo\" | (\n+\t\tmkdir sub && cd sub && git stripspace -c\n+\t) >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'avoid SP-HT sequence in commented line' '\n \tprintf \"#\\tone\\n#\\n# two\\n\" >expect &&\n \tprintf \"\\tone\\n\\ntwo\\n\" | git stripspace -c >actual &&\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex d6d65a3a94..d941f0a69f 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -983,6 +983,17 @@ test_expect_success 'rebase -i respects core.commentchar' '\n \ttest B = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+test_expect_failure 'rebase -i respects core.commentchar=auto' '\n+\ttest_config core.commentchar auto &&\n+\twrite_script copy-edit-script.sh <<-\\EOF &&\n+\tcp \"$1\" edit-script\n+\tEOF\n+\ttest_set_editor \"$(pwd)/copy-edit-script.sh\" &&\n+\ttest_when_finished \"git rebase --abort || :\" &&\n+\tgit rebase -i HEAD^ &&\n+\ttest -z \"$(grep -ve \"^#\" -e \"^\\$\" -e \"^pick\" edit-script)\"\n+'\n+\n test_expect_success 'rebase -i, with and specified as :/quuxery' '\n \ttest_when_finished \"git branch -D torebase\" &&\n \tgit checkout -b torebase branch1 &&\n-- \n2.11.0-rc2-154-g95ba452916\n\n"},{"id":"306235","messageId":"20161121190514.18574-2-gitster@pobox.com","threadId":"44522","inReplyTo":"20161121190514.18574-1-gitster@pobox.com","subject":"[PATCH 2/3] stripspace: respect repository config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-21T19:05:13Z","receivedAt":"2016-11-21T19:05:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Johannes Schindelin \n\nThe way \"git stripspace\" reads the configuration was not quite\ncorrect, in that it forgot to probe for a possibly existing\nrepository (note: stripspace is designed to be usable outside the\nrepository as well) before doing so. Due to this, .git/config was\nread only when the command was run from the top-level of the working\ntree. \n\nA recent change b9605bc4f2 (\"config: only read .git/config from\nconfigured repos\", 2016-09-12) stopped reading the repository-local\nconfiguration file \".git/config\" unless the repository discovery\nprocess is done, and \".git/config\" is no longer read even when run\nfrom the top-level, which exposed the bug even more.\n\nWhen rebasing interactively with a commentChar defined in the\ncurrent repository's config, the help text at the bottom of the edit\nscript potentially used an incorrect comment character. This was not\nonly funny-looking, but also resulted in tons of warnings like this\none:\n\n\tWarning: the command isn't recognized in the following line\n\t - #\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Junio C Hamano \n---\n builtin/stripspace.c | 4 +++-\n t/t0030-stripspace.sh | 2 +-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/stripspace.c b/builtin/stripspace.c\nindex 15e716ef43..1e62a008cb 100644\n--- a/builtin/stripspace.c\n+++ b/builtin/stripspace.c\n@@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n \tif (argc)\n \t\tusage_with_options(stripspace_usage, options);\n \n-\tif (mode == STRIP_COMMENTS || mode == COMMENT_LINES)\n+\tif (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {\n+\t\tsetup_git_directory_gently(NULL);\n \t\tgit_config(git_default_config, NULL);\n+\t}\n \n \tif (strbuf_read(&buf, 0, 1024) < 0)\n \t\tdie_errno(\"could not read the input\");\ndiff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh\nindex c1f6411eb2..bbf3e39e3d 100755\n--- a/t/t0030-stripspace.sh\n+++ b/t/t0030-stripspace.sh\n@@ -432,7 +432,7 @@ test_expect_success '-c with changed comment char' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure '-c with comment char defined in .git/config' '\n+test_expect_success '-c with comment char defined in .git/config' '\n \ttest_config core.commentchar = &&\n \tprintf \"= foo\\n\" >expect &&\n \tprintf \"foo\" | (\n-- \n2.11.0-rc2-154-g95ba452916\n\n"},{"id":"306236","messageId":"20161121190514.18574-3-gitster@pobox.com","threadId":"44522","inReplyTo":"20161121190514.18574-1-gitster@pobox.com","subject":"[PATCH 3/3] rebase -i: handle core.commentChar=auto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-21T19:05:14Z","receivedAt":"2016-11-21T19:05:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Johannes Schindelin \n\nWhen 84c9dc2 (commit: allow core.commentChar=auto for character auto\nselection, 2014-05-17) extended the core.commentChar functionality to\nallow for the value 'auto', it forgot that rebase -i was already taught to\nhandle core.commentChar, and in turn forgot to let rebase -i handle that\nnew value gracefully.\n\nReported by Taufiq Hoven.\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Junio C Hamano \n---\n git-rebase--interactive.sh | 13 +++++++++++--\n t/t3404-rebase-interactive.sh | 2 +-\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 655ebaa471..c167bc36b3 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -93,8 +93,17 @@ eval '\n GIT_CHERRY_PICK_HELP=\"$resolvemsg\"\n export GIT_CHERRY_PICK_HELP\n \n-comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)\n-: ${comment_char:=#}\n+comment_char=$(git config --get core.commentchar 2>/dev/null)\n+case \"$comment_char\" in\n+'' | auto)\n+\tcomment_char=\"#\"\n+\t;;\n+?)\n+\t;;\n+*)\n+\tcomment_char=$(echo \"$comment_char\" | cut -c1)\n+\t;;\n+esac\n \n warn () {\n \tprintf '%s\\n' \"$*\" >&2\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex d941f0a69f..5d0a7dca9d 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -983,7 +983,7 @@ test_expect_success 'rebase -i respects core.commentchar' '\n \ttest B = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n-test_expect_failure 'rebase -i respects core.commentchar=auto' '\n+test_expect_success 'rebase -i respects core.commentchar=auto' '\n \ttest_config core.commentchar auto &&\n \twrite_script copy-edit-script.sh <<-\\EOF &&\n \tcp \"$1\" edit-script\n-- \n2.11.0-rc2-154-g95ba452916\n\n"},{"id":"306237","messageId":"xmqqy40circz.fsf@gitster.mtv.corp.google.com","threadId":"44522","inReplyTo":"8b783f3d-8fda-b5cb-239f-895b10a22a73@kdbg.org","subject":"Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-21T19:07:40Z","receivedAt":"2016-11-21T19:07:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 21.11.2016 um 19:40 schrieb Junio C Hamano:\n>> Johannes Sixt writes:\n>>> It could be written without forking a process:\n>>>\n>>> \tcomment_char=${comment_char%${comment_char#?}}\n>>>\n>>> (aka \"remove from the end what remains after removing the first character\")\n>>\n>> Hopefully nobody would include any glob metacharacters in there,\n>> e.g. \"core.commentchar='=*'\", which would break that?\n>\n> Heh. I tested a few variations, but not this one. Make it\n>\n> \tcomment_char=${comment_char%\"${comment_char#?}\"}\n\nI tried a few implementations of shells, and decided that this is\nnot worth the portability hassle.\n\nSetting comment-char to multi-letter sequence is not supported at\nall, and this code is protecting itself from that nonsense---it does\nnot need to be fast, and Dscho already gives a fast path for a\nsingle letter case in his patch.\n\n\n"},{"id":"306238","messageId":"xmqqtwb0ir4o.fsf@gitster.mtv.corp.google.com","threadId":"44522","inReplyTo":"20161121184931.36nuhg2h7u6mjsld@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-21T19:12:39Z","receivedAt":"2016-11-21T19:12:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Nov 21, 2016 at 10:15:43AM -0800, Junio C Hamano wrote:\n>\n>> > +\ttest_cmp expect actual\n>> > +'\n>> > +\n>> \n>> Is this a recent regression? When applied on top of 'maint' or\n>> older, it seems to pass just fine.\n>> \n>> ... Goes and looks ...\n>> \n>> Interesting. Peff's b9605bc4f2 (\"config: only read .git/config from\n>> configured repos\", 2016-09-12) is where this starts failing, which\n>> is understandable given the code change to builtin/stripspace.c in\n>> [2/3]. \n>> \n>> The analysis of the log message in [2/3] is wrong and needs\n>> updating, though. In the old world order it worked by accident to\n>> call git_config() without calling setup_git_directory(); after\n>> b9605bc4f2, that no longer is valid and is exposed as a bug. \n>\n> Yeah, I noticed that while reading the patch. My b9605bc4f2 did regress\n> this case, but called out the fact that \"cd subdir && git stripspace\"\n> would never have worked. So one step back, 2 steps forward, and Dscho's\n> patch is the right step forward.\n\nYes, absolutely.\n\nI sent out a set of proposed amends, and the one for this step 1/3\nruns the command inside a subdirectory to force it not to find the\n.git/config file relative to its pwd, which can reveal the existing\nbreakage without help by b9605bc4f2 ;-) hence can be forked for\nolder maintenance tracks.\n"},{"id":"306239","messageId":"e397b563-bbd1-9a90-b294-d5c0607aa587@kdbg.org","threadId":"44522","inReplyTo":"xmqqy40circz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-11-21T19:14:00Z","receivedAt":"2016-11-21T19:14:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 21.11.2016 um 20:07 schrieb Junio C Hamano:\n> Setting comment-char to multi-letter sequence is not supported at\n> all, and this code is protecting itself from that nonsense---it does\n> not need to be fast, and Dscho already gives a fast path for a\n> single letter case in his patch.\n\nFair enough, no problem.\n\n-- Hannes\n\n"},{"id":"306240","messageId":"xmqqpoloinrf.fsf@gitster.mtv.corp.google.com","threadId":"44522","inReplyTo":"20161121190514.18574-1-gitster@pobox.com","subject":"Re: [PATCH 1/3] rebase -i: highlight problems with core.commentchar","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-21T20:25:24Z","receivedAt":"2016-11-21T20:26:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> From: Johannes Schindelin \n>\n> The interactive rebase does not currently play well with\n> core.commentchar. Let's add some tests to highlight those problems\n> that will be fixed in the remainder of the series.\n>\n> Signed-off-by: Johannes Schindelin \n> Signed-off-by: Junio C Hamano \n> ---\n\nSorry, I should have commented here after --- line what changes were\nproposed by this set of amends.\n\n> t/t0030-stripspace.sh | 9 +++++++++\n> t/t3404-rebase-interactive.sh | 11 +++++++++++\n> 2 files changed, 20 insertions(+)\n>\n> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh\n> index 29e91d861c..c1f6411eb2 100755\n> --- a/t/t0030-stripspace.sh\n> +++ b/t/t0030-stripspace.sh\n> @@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_failure '-c with comment char defined in .git/config' '\n> +\ttest_config core.commentchar = &&\n> +\tprintf \"= foo\\n\" >expect &&\n> +\tprintf \"foo\" | (\n> +\t\tmkdir sub && cd sub && git stripspace -c\n> +\t) >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nI noticed that the lack of \"\\n\" at the end was merely copied from the\nprevious existing test (i.e. \"-c with with changed comment char\"),\nwhich was already wrong the same way, so I kept that part as-is.\n\nRunning \"stripspace\" in a subdirectory is to avoid the \".git/config\nwas read without repository discovery as long as the command runs at\nthe top-level of the working tree\" accident so that this highlights\nthe breakage with or without Peff's b9605bc4f2 (\"config: only read\n.git/config from configured repos\", 2016-09-12).\n\n\n> +\n> test_expect_success 'avoid SP-HT sequence in commented line' '\n> \tprintf \"#\\tone\\n#\\n# two\\n\" >expect &&\n> \tprintf \"\\tone\\n\\ntwo\\n\" | git stripspace -c >actual &&\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index d6d65a3a94..d941f0a69f 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -983,6 +983,17 @@ test_expect_success 'rebase -i respects core.commentchar' '\n> \ttest B = $(git cat-file commit HEAD^ | sed -ne \\$p)\n> '\n> \n> +test_expect_failure 'rebase -i respects core.commentchar=auto' '\n> +\ttest_config core.commentchar auto &&\n> +\twrite_script copy-edit-script.sh <<-\\EOF &&\n> +\tcp \"$1\" edit-script\n> +\tEOF\n> +\ttest_set_editor \"$(pwd)/copy-edit-script.sh\" &&\n> +\ttest_when_finished \"git rebase --abort || :\" &&\n> +\tgit rebase -i HEAD^ &&\n> +\ttest -z \"$(grep -ve \"^#\" -e \"^\\$\" -e \"^pick\" edit-script)\"\n> +'\n> +\n\nRemoved from here is a leftover debugging bit (grep \"^#\"\nedit-script).\n\n"},{"id":"306241","messageId":"xmqqlgwcinlp.fsf@gitster.mtv.corp.google.com","threadId":"44522","inReplyTo":"20161121190514.18574-2-gitster@pobox.com","subject":"Re: [PATCH 2/3] stripspace: respect repository config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-21T20:28:50Z","receivedAt":"2016-11-21T20:28:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> From: Johannes Schindelin \n>\n> The way \"git stripspace\" reads the configuration was not quite\n> correct, in that it forgot to probe for a possibly existing\n> repository (note: stripspace is designed to be usable outside the\n> repository as well) before doing so. Due to this, .git/config was\n> read only when the command was run from the top-level of the working\n> tree. \n>\n> A recent change b9605bc4f2 (\"config: only read .git/config from\n> configured repos\", 2016-09-12) stopped reading the repository-local\n> configuration file \".git/config\" unless the repository discovery\n> process is done, and \".git/config\" is no longer read even when run\n> from the top-level, which exposed the bug even more.\n\nThe above two paragraphs are rewritten from the original to explain\nhow this seemed to work (by accident) and its breakage surfaced in\n\"rebase -i\" after b9605bc4f2 (\"config: only read .git/config from\nconfigured repos\", 2016-09-12) better. The use of stripspace in\n\"rebase-i\" was done after cd_to_toplevel and it happened to work\nbefore that commit.\n\nOtherwise there is no change from the original.\n\n> When rebasing interactively with a commentChar defined in the\n> current repository's config, the help text at the bottom of the edit\n> script potentially used an incorrect comment character. This was not\n> only funny-looking, but also resulted in tons of warnings like this\n> one:\n>\n> \tWarning: the command isn't recognized in the following line\n> \t - #\n>\n> Signed-off-by: Johannes Schindelin \n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/stripspace.c | 4 +++-\n> t/t0030-stripspace.sh | 2 +-\n> 2 files changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/stripspace.c b/builtin/stripspace.c\n> index 15e716ef43..1e62a008cb 100644\n> --- a/builtin/stripspace.c\n> +++ b/builtin/stripspace.c\n> @@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n> \tif (argc)\n> \t\tusage_with_options(stripspace_usage, options);\n> \n> -\tif (mode == STRIP_COMMENTS || mode == COMMENT_LINES)\n> +\tif (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {\n> +\t\tsetup_git_directory_gently(NULL);\n> \t\tgit_config(git_default_config, NULL);\n> +\t}\n> \n> \tif (strbuf_read(&buf, 0, 1024) < 0)\n> \t\tdie_errno(\"could not read the input\");\n> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh\n> index c1f6411eb2..bbf3e39e3d 100755\n> --- a/t/t0030-stripspace.sh\n> +++ b/t/t0030-stripspace.sh\n> @@ -432,7 +432,7 @@ test_expect_success '-c with changed comment char' '\n> \ttest_cmp expect actual\n> '\n> \n> -test_expect_failure '-c with comment char defined in .git/config' '\n> +test_expect_success '-c with comment char defined in .git/config' '\n> \ttest_config core.commentchar = &&\n> \tprintf \"= foo\\n\" >expect &&\n> \tprintf \"foo\" | (\n"},{"id":"306242","messageId":"xmqqh970ink7.fsf@gitster.mtv.corp.google.com","threadId":"44522","inReplyTo":"20161121190514.18574-3-gitster@pobox.com","subject":"Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-21T20:29:44Z","receivedAt":"2016-11-21T20:29:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> @@ -93,8 +93,17 @@ eval '\n> GIT_CHERRY_PICK_HELP=\"$resolvemsg\"\n> export GIT_CHERRY_PICK_HELP\n> \n> -comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)\n> -: ${comment_char:=#}\n> +comment_char=$(git config --get core.commentchar 2>/dev/null)\n> +case \"$comment_char\" in\n> +'' | auto)\n> +\tcomment_char=\"#\"\n> +\t;;\n> +?)\n> +\t;;\n> +*)\n> +\tcomment_char=$(echo \"$comment_char\" | cut -c1)\n> +\t;;\n> +esac\n\nAmended in is a fix for a typo the other Johannes noticed.\n\nThanks.\n\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index d941f0a69f..5d0a7dca9d 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -983,7 +983,7 @@ test_expect_success 'rebase -i respects core.commentchar' '\n> \ttest B = $(git cat-file commit HEAD^ | sed -ne \\$p)\n> '\n> \n> -test_expect_failure 'rebase -i respects core.commentchar=auto' '\n> +test_expect_success 'rebase -i respects core.commentchar=auto' '\n> \ttest_config core.commentchar auto &&\n> \twrite_script copy-edit-script.sh <<-\\EOF &&\n> \tcp \"$1\" edit-script\n"},{"id":"306267","messageId":"20161121233828.op3skzgujzcwwllg@sigill.intra.peff.net","threadId":"44522","inReplyTo":"xmqqtwb0ir4o.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-21T23:38:29Z","receivedAt":"2016-11-21T23:38:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 21, 2016 at 11:12:39AM -0800, Junio C Hamano wrote:\n\n> > Yeah, I noticed that while reading the patch. My b9605bc4f2 did regress\n> > this case, but called out the fact that \"cd subdir && git stripspace\"\n> > would never have worked. So one step back, 2 steps forward, and Dscho's\n> > patch is the right step forward.\n> \n> Yes, absolutely.\n> \n> I sent out a set of proposed amends, and the one for this step 1/3\n> runs the command inside a subdirectory to force it not to find the\n> .git/config file relative to its pwd, which can reveal the existing\n> breakage without help by b9605bc4f2 ;-) hence can be forked for\n> older maintenance tracks.\n\nMakes sense, and your amended patch looks good.\n\n-Peff\n"},{"id":"306293","messageId":"CACsJy8D5oBR+vo2B+Ro2Q4SX0CG3jME4Gfs1_6AohccmpNvD0A@mail.gmail.com","threadId":"44522","inReplyTo":"5567548295222401fab10d3f2901c1787afbfd07.1479737858.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/3] stripspace: respect repository config","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-11-22T10:10:19Z","receivedAt":"2016-11-22T10:11:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin\n wrote:\n> When eff80a9 (Allow custom \"comment char\", 2013-01-16) taught the\n> `stripspace` command to respect the config setting `core.commentChar`,\n> it forgot that this variable may be defined in .git/config.\n>\n> So when rebasing interactively with a commentChar defined in the current\n> repository's config, the help text at the bottom of the edit script\n> potentially used an incorrect comment character. This was not only\n> funny-looking, but also resulted in tons of warnings like this one:\n>\n> Warning: the command isn't recognized in the following line\n> - #\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/stripspace.c | 4 +++-\n> t/t0030-stripspace.sh | 2 +-\n> 2 files changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/stripspace.c b/builtin/stripspace.c\n> index 15e716e..1e62a00 100644\n> --- a/builtin/stripspace.c\n> +++ b/builtin/stripspace.c\n> @@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n> if (argc)\n> usage_with_options(stripspace_usage, options);\n>\n> - if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)\n> + if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {\n> + setup_git_directory_gently(NULL);\n> git_config(git_default_config, NULL);\n> + }\n\nThis conditional config file reading is a trap for similar bugs to\nhappen again. Is there any reason we should not just mark the command\nRUN_SETUP_GENTLY in git.c and call git_config() here unconditionally?\n\n>\n> if (strbuf_read(&buf, 0, 1024) < 0)\n> die_errno(\"could not read the input\");\n-- \nDuy\n"},{"id":"306294","messageId":"CACsJy8D0uDs8vZgOZ0HkJS7wOuc7TPU9xcB8BTWNsa11At04Ug@mail.gmail.com","threadId":"44522","inReplyTo":"9ef529a4fbb60990a91d7bbfdd49c6d20d49e442.1479737858.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-11-22T10:31:43Z","receivedAt":"2016-11-22T10:32:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin\n wrote:\n> When 84c9dc2 (commit: allow core.commentChar=auto for character auto\n> selection, 2014-05-17) extended the core.commentChar functionality to\n> allow for the value 'auto', it forgot that rebase -i was already taught to\n> handle core.commentChar,\n\nI think this is 180bad3 (rebase -i: respect core.commentchar -\n2013-02-11) and a couple followup fixes. My bad.\n\n> and in turn forgot to let rebase -i handle that\n> new value gracefully.\n>\n> Reported by Taufiq Hoven.\n\nAnother report in the same area: a merge conflict always writes the\n\"Conflicts:\" section in the commit message with '#' as comment char\nwhen core.commentChar is auto. I've known this for a while, but it has\nnot bitten me hard enough to do something about it,\n\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> git-rebase--interactive.sh | 13 +++++++++++--\n> t/t3404-rebase-interactive.sh | 2 +-\n> 2 files changed, 12 insertions(+), 3 deletions(-)\n>\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index ca994c5..6bb740c 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -93,8 +93,17 @@ eval '\n> GIT_CHERRY_PICK_HELP=\"$resolvemsg\"\n> export GIT_CHERRY_PICK_HELP\n>\n> -comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)\n> -: ${comment_char:=#}\n> +comment_char=$(git config --get core.commentchar 2>/dev/null)\n> +case \"$comment_char\" in\n> +''|auto)\n> + comment_char=#\n\nMy first thought was \"this kinda defeats the purpose of auto, doesn't\nit\". But 'auto' here is irrelevant because we control the leading\ncharacter of all lines in the todo file. 'auto' makes little sense in\nthis context, falling back to any comment char would do.\n\nSo, ack (after you fix the $(comment_char other people have mentioned,\nof course).\n-- \nDuy\n"},{"id":"306304","messageId":"alpine.DEB.2.20.1611221703170.3746@virtualbox","threadId":"44522","inReplyTo":"975c0002-8afe-efa6-d967-72f2a0d21169@kdbg.org","subject":"Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-11-22T16:04:18Z","receivedAt":"2016-11-22T16:04:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Mon, 21 Nov 2016, Johannes Sixt wrote:\n\n> Am 21.11.2016 um 15:18 schrieb Johannes Schindelin:\n> > -comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)\n> > -: ${comment_char:=#}\n> > +comment_char=$(git config --get core.commentchar 2>/dev/null)\n> > +case \"$comment_char\" in\n> > +''|auto)\n> > +\tcomment_char=#\n> > +\t;;\n> > +?)\n> > +\t;;\n> > +*)\n> > +\tcomment_char=$(comment_char | cut -c1)\n> \n> comment_char is a command? Did you mean\n> \n> \tcomment_char=$(echo \"$comment_char\" | cut -c1)\n\nD'oh.\n\n> It could be written without forking a process:\n> \n> \tcomment_char=${comment_char%${comment_char#?}}\n> \n> (aka \"remove from the end what remains after removing the first character\")\n\nI was considering this, actually, but it is rather unreadable. Better\nrewrite it in C, to begin with.\n\nThanks,\nDscho\n"},{"id":"306305","messageId":"alpine.DEB.2.20.1611221706470.3746@virtualbox","threadId":"44522","inReplyTo":"xmqqbmx8k8c0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-11-22T16:09:08Z","receivedAt":"2016-11-22T16:09:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 21 Nov 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh\n> > index 29e91d8..202ac07 100755\n> > --- a/t/t0030-stripspace.sh\n> > +++ b/t/t0030-stripspace.sh\n> > @@ -432,6 +432,13 @@ test_expect_success '-c with changed comment char' '\n> > \ttest_cmp expect actual\n> > '\n> > \n> > +test_expect_failure '-c with comment char defined in .git/config' '\n> > +\ttest_config core.commentchar = &&\n> > +\tprintf \"= foo\\n\" >expect &&\n> > +\tprintf \"foo\" | git stripspace -c >actual &&\n> \n> We'd want \"\\n\" on this printf to match the one before as well,\n\nTrue.\n\n> The analysis of the log message in [2/3] is wrong and needs\n> updating, though.\n\nThanks for following up on that, and for fixing the commit message.\n\n> > +test_expect_failure 'rebase -i respects core.commentchar=auto' '\n> > +\ttest_config core.commentchar auto &&\n> > +\twrite_script copy-edit-script.sh <<-\\EOF &&\n> > +\tcp \"$1\" edit-script\n> > +\tEOF\n> > +\ttest_set_editor \"$(pwd)/copy-edit-script.sh\" &&\n> > +\ttest_when_finished \"git rebase --abort || :\" &&\n> > +\tgit rebase -i HEAD^ &&\n> > +\tgrep \"^#\" edit-script &&\n> \n> This was added for debugging that was forgotten?\n\nNo, this was me trying to ensure that the comment character '#' *is* used.\nAs opposed to somehow testing any edit script that contains no commented\nlines whatsoever.\n\nBut if you don't care, I won't, either.\n\nCiao,\nDscho\n"},{"id":"306306","messageId":"alpine.DEB.2.20.1611221709180.3746@virtualbox","threadId":"44522","inReplyTo":"20161121190514.18574-1-gitster@pobox.com","subject":"Re: [PATCH 1/3] rebase -i: highlight problems with core.commentchar","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-11-22T16:09:57Z","receivedAt":"2016-11-22T16:10:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 21 Nov 2016, Junio C Hamano wrote:\n\n> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh\n> index 29e91d861c..c1f6411eb2 100755\n> --- a/t/t0030-stripspace.sh\n> +++ b/t/t0030-stripspace.sh\n> @@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_failure '-c with comment char defined in .git/config' '\n> +\ttest_config core.commentchar = &&\n> +\tprintf \"= foo\\n\" >expect &&\n> +\tprintf \"foo\" | (\n\nCould I ask you to sneak in a \\n here?\n\nThanks,\nDscho\n"},{"id":"306307","messageId":"alpine.DEB.2.20.1611221710470.3746@virtualbox","threadId":"44522","inReplyTo":"20161121190514.18574-2-gitster@pobox.com","subject":"Re: [PATCH 2/3] stripspace: respect repository config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-11-22T16:11:00Z","receivedAt":"2016-11-22T16:11:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 21 Nov 2016, Junio C Hamano wrote:\n\n> From: Johannes Schindelin \n> \n> The way \"git stripspace\" reads the configuration was not quite\n> correct, in that it forgot to probe for a possibly existing\n> repository (note: stripspace is designed to be usable outside the\n> repository as well) before doing so. Due to this, .git/config was\n> read only when the command was run from the top-level of the working\n> tree. \n> \n> A recent change b9605bc4f2 (\"config: only read .git/config from\n> configured repos\", 2016-09-12) stopped reading the repository-local\n> configuration file \".git/config\" unless the repository discovery\n> process is done, and \".git/config\" is no longer read even when run\n> from the top-level, which exposed the bug even more.\n> \n> When rebasing interactively with a commentChar defined in the\n> current repository's config, the help text at the bottom of the edit\n> script potentially used an incorrect comment character. This was not\n> only funny-looking, but also resulted in tons of warnings like this\n> one:\n> \n> \tWarning: the command isn't recognized in the following line\n> \t - #\n> \n> Signed-off-by: Johannes Schindelin \n> Signed-off-by: Junio C Hamano \n\nThanks for the corrected commit message!\nDscho\n"},{"id":"306308","messageId":"alpine.DEB.2.20.1611221712480.3746@virtualbox","threadId":"44522","inReplyTo":"CACsJy8D5oBR+vo2B+Ro2Q4SX0CG3jME4Gfs1_6AohccmpNvD0A@mail.gmail.com","subject":"Re: [PATCH 2/3] stripspace: respect repository config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-11-22T16:13:59Z","receivedAt":"2016-11-22T16:14:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Tue, 22 Nov 2016, Duy Nguyen wrote:\n\n> On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin\n> wrote:\n> > When eff80a9 (Allow custom \"comment char\", 2013-01-16) taught the\n> > `stripspace` command to respect the config setting `core.commentChar`,\n> > it forgot that this variable may be defined in .git/config.\n> >\n> > So when rebasing interactively with a commentChar defined in the current\n> > repository's config, the help text at the bottom of the edit script\n> > potentially used an incorrect comment character. This was not only\n> > funny-looking, but also resulted in tons of warnings like this one:\n> >\n> > Warning: the command isn't recognized in the following line\n> > - #\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > builtin/stripspace.c | 4 +++-\n> > t/t0030-stripspace.sh | 2 +-\n> > 2 files changed, 4 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/builtin/stripspace.c b/builtin/stripspace.c\n> > index 15e716e..1e62a00 100644\n> > --- a/builtin/stripspace.c\n> > +++ b/builtin/stripspace.c\n> > @@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n> > if (argc)\n> > usage_with_options(stripspace_usage, options);\n> >\n> > - if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)\n> > + if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {\n> > + setup_git_directory_gently(NULL);\n> > git_config(git_default_config, NULL);\n> > + }\n> \n> This conditional config file reading is a trap for similar bugs to\n> happen again. Is there any reason we should not just mark the command\n> RUN_SETUP_GENTLY in git.c and call git_config() here unconditionally?\n\nAs I plan to slip these patches into Git for Windows v2.11.0, i.e. making\nthis a last-minute hot fix, I want to err on the side of caution. So I'd\nrather keep this conditional (it might regress on the performance front,\nor something).\n\nCiao,\nDscho\n"},{"id":"306312","messageId":"xmqqshqjqw3z.fsf@gitster.mtv.corp.google.com","threadId":"44522","inReplyTo":"alpine.DEB.2.20.1611221712480.3746@virtualbox","subject":"Re: [PATCH 2/3] stripspace: respect repository config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-22T17:10:08Z","receivedAt":"2016-11-22T17:10:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> This conditional config file reading is a trap for similar bugs to\n>> happen again. Is there any reason we should not just mark the command\n>> RUN_SETUP_GENTLY in git.c and call git_config() here unconditionally?\n>\n> As I plan to slip these patches into Git for Windows v2.11.0, i.e. making\n> this a last-minute hot fix, I want to err on the side of caution.\n\nSo do I. As a hot-fix, I'd prefer the patch I queued yesterday.\n\nI think we want to audit the ones without RUN_SETUP* in the command\ntable in order to hunt for regression aka \"a fix revealed a bug that\nwas covered by .git/config accidentally getting read when run from\nthe top-level of the working tree\", though. We may find unreported\nbreakages that we may have to fix.\n"},{"id":"306313","messageId":"xmqqwpfvqwbq.fsf@gitster.mtv.corp.google.com","threadId":"44522","inReplyTo":"alpine.DEB.2.20.1611221709180.3746@virtualbox","subject":"Re: [PATCH 1/3] rebase -i: highlight problems with core.commentchar","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-22T17:05:29Z","receivedAt":"2016-11-22T17:12:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Mon, 21 Nov 2016, Junio C Hamano wrote:\n>\n>> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh\n>> index 29e91d861c..c1f6411eb2 100755\n>> --- a/t/t0030-stripspace.sh\n>> +++ b/t/t0030-stripspace.sh\n>> @@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '\n>> \ttest_cmp expect actual\n>> '\n>> \n>> +test_expect_failure '-c with comment char defined in .git/config' '\n>> +\ttest_config core.commentchar = &&\n>> +\tprintf \"= foo\\n\" >expect &&\n>> +\tprintf \"foo\" | (\n>\n> Could I ask you to sneak in a \\n here?\n\nThe one before that _is_ about fixing incomplete line, but this and\nthe one before this one are not, so I think we should fix them at\nthe same time.\n\nBut does it break anything to leave it as-is? If not, I'd prefer to\nleave this (and one before this one) for a later clean-up patch post\nrelease.\n\nThanks\n\n\n"},{"id":"306337","messageId":"xmqqzikrpbz2.fsf@gitster.mtv.corp.google.com","threadId":"44522","inReplyTo":"xmqqshqjqw3z.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] stripspace: respect repository config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-22T19:10:25Z","receivedAt":"2016-11-22T19:10:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I think we want to audit the ones without RUN_SETUP* in the command\n> table in order to hunt for regression aka \"a fix revealed a bug that\n> was covered by .git/config accidentally getting read when run from\n> the top-level of the working tree\", though. We may find unreported\n> breakages that we may have to fix.\n\nSo I took a brief look at the \"PROGRAM_OBJS += ...\" in the Makefile\nfor non-builtin commands, and commands[] array in git.c for builtin\ncommands to see how bad it looks.\n\n\nArchive & Upload-archive:\n\n\"cd Documentation && git archive --remote=origin\" immediately hits\n\"BUG: setup_git_env called without repository\" if your Git is built\nwith b1ef400eec (\"setup_git_env: avoid blind fall-back to \".git\"\",\n2016-10-20), which will not be part of the upcoming release. And\n'origin' will probably not be understood from the local config.\n\nI think we can do the \"gently\" thing there, as we may be retrieving\na remote archive outside a local repository. We'd need to tweak\n\"output\" with prefix to compensate for the case in which the command\nis run from a subdirectory, and probably we need to futz with the\nsetup_prefix parameter to write_archive(), as a local caller now\nwill know if we are in nongit situation.\n\nOn the upload-archive side that serves \"--remote\" request, there is\nenter_repo() so we should be covered OK.\n\n\nMailinfo:\n\nWe may want a \"gently\" thing there to pick up local configuration.\ni18n.commitencoding and mailinfo.scissors in local repository would\nbe ignored otherwise.\n\n\nSplitspace:\n\nDscho fixed this one.\n\n\nVerify-pack:\n\nThis calls git_config() but these days farms out its operation to\n\"index-pack\", so we should be OK. We may even want to lose the call\nto git_config() which does not affect anything.\n\n"},{"id":"306346","messageId":"20161122195015.rztaixpz7zvdxb4u@sigill.intra.peff.net","threadId":"44522","inReplyTo":"xmqqzikrpbz2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] stripspace: respect repository config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-22T19:50:15Z","receivedAt":"2016-11-22T19:50:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 22, 2016 at 11:10:25AM -0800, Junio C Hamano wrote:\n\n> Archive & Upload-archive:\n> \n> \"cd Documentation && git archive --remote=origin\" immediately hits\n> \"BUG: setup_git_env called without repository\" if your Git is built\n> with b1ef400eec (\"setup_git_env: avoid blind fall-back to \".git\"\",\n> 2016-10-20), which will not be part of the upcoming release. And\n> 'origin' will probably not be understood from the local config.\n\nYeah, I think \"cd Documentation && git archive --remote\" has always been\nbroken. It doesn't barf until b1ef400eec, but yes, v2.11-rc2 breaks\nfinding \"origin\" in the local config.\n\nI agree that doing the \"gently\" setup will fix the config problem, but\nthere's more to do to make it work with b1ef400eec outside of a repo.\nIt looks like read_remotes_file() and read_branches_file() in remote.c\nneed to learn to silently return when we are not in a git repository (or\nalternatively, remote_get_1() should learn not to call them in such a\ncase).\n\nI doubt this is a critical case (it only kicks in if you are outside a\nrepo _and_ your name looks like a remote name and not a URL, _and_ it is\nnot defined by your config, and the result is that we die(\"BUG\") rather\nthan reporting \"you don't have such a remote\"). But any time it is\npossible to hit a die(\"BUG\"), we should be fixing it.\n\n> I think we can do the \"gently\" thing there, as we may be retrieving\n> a remote archive outside a local repository. We'd need to tweak\n> \"output\" with prefix to compensate for the case in which the command\n> is run from a subdirectory, and probably we need to futz with the\n> setup_prefix parameter to write_archive(), as a local caller now\n> will know if we are in nongit situation.\n\nMakes sense. I think OPT_FILENAME() would handle \"output\", but yeah, it\nlooks like write_archive() already tries to be clever about the setup.\n\n> On the upload-archive side that serves \"--remote\" request, there is\n> enter_repo() so we should be covered OK.\n\nRight, that has to run in a repo, and enter_repo() should handle it.\n\n> Mailinfo:\n> \n> We may want a \"gently\" thing there to pick up local configuration.\n> i18n.commitencoding and mailinfo.scissors in local repository would\n> be ignored otherwise.\n\nYeah, agreed.\n\n> Verify-pack:\n> \n> This calls git_config() but these days farms out its operation to\n> \"index-pack\", so we should be OK. We may even want to lose the call\n> to git_config() which does not affect anything.\n\nI'd be slightly worried that there is a core.* config option that\naffects us in a subtle way (e.g., some portability flag for how we do\nrun_command()). I don't think there is one now, but it seems like a\npotential maintenance pitfall. IOW, I think I'd generally prefer to err\non the side of having everything do a gentle setup and load at least the\ndefault config, to avoid surprises.\n\nI don't think it's a high priority, though.\n\nSo what do you want to do for v2.11? I think there are minor regressions\nin stripspace, archive, and mailinfo with respect to reading\n.git/config. The right solution in each case is to do a gentle repo\nsetup. We have patches for stripspace already, but not yet for the other\ntwo. They _should_ be fairly trivial, but we are getting awfully close\nto the release. Do you want to do another round of -rc3? Ship with the\nminor regressions and fix them up in v2.11.1? Last-minute revert the\noriginal config topic that introduced the regressions (I'm biased\nagainst that as the author, but I also think it's dangerous; it's a big\nenough topic that it might introduce new problems)?\n\n-Peff\n"},{"id":"306352","messageId":"xmqqoa17p8k0.fsf@gitster.mtv.corp.google.com","threadId":"44522","inReplyTo":"20161122195015.rztaixpz7zvdxb4u@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] stripspace: respect repository config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-22T20:24:15Z","receivedAt":"2016-11-22T20:24:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So what do you want to do for v2.11? I think there are minor regressions\n> in stripspace, archive, and mailinfo with respect to reading\n> .git/config. The right solution in each case is to do a gentle repo\n> setup. We have patches for stripspace already, but not yet for the other\n> two. They _should_ be fairly trivial, but we are getting awfully close\n> to the release. \n\nRight.\n\n> Do you want to do another round of -rc3? Ship with the\n> minor regressions and fix them up in v2.11.1?\n\nI am leaning towards the former (though we may also end up doing the\nlatter).\n\nHere is an initial attempt. It turns out that write_archive() must\nbe callable outside a repository to respond to \"git archive --list\",\nwhich happens in parse_archive_args(), and the \"have-repository?\"\ncheck cannot be moved before that to the caller.\n\nNo tests yet. Help appreciated ;-)\n\n\n archive.c | 9 ++-------\n archive.h | 2 +-\n builtin/archive.c | 6 +++---\n builtin/upload-archive.c | 2 +-\n git.c | 4 ++--\n 5 files changed, 9 insertions(+), 14 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex dde1ab4c79..901b10264f 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -504,15 +504,11 @@ static int parse_archive_args(int argc, const char **argv,\n }\n \n int write_archive(int argc, const char **argv, const char *prefix,\n-\t\t int setup_prefix, const char *name_hint, int remote)\n+\t\t const char *name_hint, int remote)\n {\n-\tint nongit = 0;\n \tconst struct archiver *ar = NULL;\n \tstruct archiver_args args;\n \n-\tif (setup_prefix && prefix == NULL)\n-\t\tprefix = setup_git_directory_gently(&nongit);\n-\n \tgit_config_get_bool(\"uploadarchive.allowunreachable\", &remote_allow_unreachable);\n \tgit_config(git_default_config, NULL);\n \n@@ -520,7 +516,7 @@ int write_archive(int argc, const char **argv, const char *prefix,\n \tinit_zip_archiver();\n \n \targc = parse_archive_args(argc, argv, &ar, &args, name_hint, remote);\n-\tif (nongit) {\n+\tif (!startup_info->have_repository) {\n \t\t/*\n \t\t * We know this will die() with an error, so we could just\n \t\t * die ourselves; but its error message will be more specific\n@@ -528,7 +524,6 @@ int write_archive(int argc, const char **argv, const char *prefix,\n \t\t */\n \t\tsetup_git_directory();\n \t}\n-\n \tparse_treeish_arg(argv, &args, prefix, remote);\n \tparse_pathspec_arg(argv + 1, &args);\n \ndiff --git a/archive.h b/archive.h\nindex 4a791e1fed..415e0152e2 100644\n--- a/archive.h\n+++ b/archive.h\n@@ -36,7 +36,7 @@ typedef int (*write_archive_entry_fn_t)(struct archiver_args *args,\n \t\t\t\t\tunsigned int mode);\n \n extern int write_archive_entries(struct archiver_args *args, write_archive_entry_fn_t write_entry);\n-extern int write_archive(int argc, const char **argv, const char *prefix, int setup_prefix, const char *name_hint, int remote);\n+extern int write_archive(int argc, const char **argv, const char *prefix, const char *name_hint, int remote);\n \n const char *archive_format_from_filename(const char *filename);\n extern void *sha1_file_to_archive(const struct archiver_args *args,\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex 49f491413a..f863465a0f 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -85,8 +85,8 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \tconst char *output = NULL;\n \tconst char *remote = NULL;\n \tstruct option local_opts[] = {\n-\t\tOPT_STRING('o', \"output\", &output, N_(\"file\"),\n-\t\t\tN_(\"write the archive to this file\")),\n+\t\tOPT_FILENAME('o', \"output\", &output,\n+\t\t\t N_(\"write the archive to this file\")),\n \t\tOPT_STRING(0, \"remote\", &remote, N_(\"repo\"),\n \t\t\tN_(\"retrieve the archive from remote repository \")),\n \t\tOPT_STRING(0, \"exec\", &exec, N_(\"command\"),\n@@ -105,5 +105,5 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \n \tsetvbuf(stderr, NULL, _IOLBF, BUFSIZ);\n \n-\treturn write_archive(argc, argv, prefix, 1, output, 0);\n+\treturn write_archive(argc, argv, prefix, output, 0);\n }\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex dc872f6a94..cde06977b7 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -43,7 +43,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \t}\n \n \t/* parse all options sent by the client */\n-\treturn write_archive(sent_argv.argc, sent_argv.argv, prefix, 0, NULL, 1);\n+\treturn write_archive(sent_argv.argc, sent_argv.argv, prefix, NULL, 1);\n }\n \n __attribute__((format (printf, 1, 2)))\ndiff --git a/git.c b/git.c\nindex efa1059fe0..e8b2baf2d1 100644\n--- a/git.c\n+++ b/git.c\n@@ -396,7 +396,7 @@ static struct cmd_struct commands[] = {\n \t{ \"am\", cmd_am, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"annotate\", cmd_annotate, RUN_SETUP },\n \t{ \"apply\", cmd_apply, RUN_SETUP_GENTLY },\n-\t{ \"archive\", cmd_archive },\n+\t{ \"archive\", cmd_archive, RUN_SETUP_GENTLY },\n \t{ \"bisect--helper\", cmd_bisect__helper, RUN_SETUP },\n \t{ \"blame\", cmd_blame, RUN_SETUP },\n \t{ \"branch\", cmd_branch, RUN_SETUP },\n@@ -445,7 +445,7 @@ static struct cmd_struct commands[] = {\n \t{ \"ls-files\", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX },\n \t{ \"ls-remote\", cmd_ls_remote, RUN_SETUP_GENTLY },\n \t{ \"ls-tree\", cmd_ls_tree, RUN_SETUP },\n-\t{ \"mailinfo\", cmd_mailinfo },\n+\t{ \"mailinfo\", cmd_mailinfo, RUN_SETUP_GENTLY },\n \t{ \"mailsplit\", cmd_mailsplit },\n \t{ \"merge\", cmd_merge, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"merge-base\", cmd_merge_base, RUN_SETUP },\n"},{"id":"306354","messageId":"20161122211920.3qdohxdktykqhyga@sigill.intra.peff.net","threadId":"44522","inReplyTo":"xmqqoa17p8k0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] stripspace: respect repository config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-22T21:19:21Z","receivedAt":"2016-11-22T21:19:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 22, 2016 at 12:24:15PM -0800, Junio C Hamano wrote:\n\n> > Do you want to do another round of -rc3? Ship with the\n> > minor regressions and fix them up in v2.11.1?\n> \n> I am leaning towards the former (though we may also end up doing the\n> latter).\n\nI think I'd lead towards -rc3, as well. Our schedule is somewhat\narbitrary anyway, and one week is less important than somebody picking\nup the new release and trying to puzzle out a regression that we already\nhave a fix for.\n\n> Here is an initial attempt. It turns out that write_archive() must\n> be callable outside a repository to respond to \"git archive --list\",\n> which happens in parse_archive_args(), and the \"have-repository?\"\n> check cannot be moved before that to the caller.\n\nRight, that makes sense. t5000 already tests git-archive outside of a\nrepo, although it does it in a funny way (it sets GIT_DIR rather than\nrelying on GIT_CEILING_DIRECTORIES). We should cover that and make sure\nthat --remote works outside of a repo. Those work now, but we want to\nmake sure we don't regress them.\n\nAnd then we want to test that --remote can read a configured remote\nname.\n\nAnd then post-v2.11, we'd want to check that --remote=foo outside of a\nrepo produces a sane error instead of looking for a bogus\n$GIT_DIR/remotes/foo.\n\nSomething like this, which does all but the last (and that should\nprobably happen separately post-release).\n\n---\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 80b238734..09df7f045 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -94,6 +94,20 @@ check_tar() {\n \t'\n }\n \n+# run \"$@\" inside a non-git directory\n+nongit () {\n+\ttest -d non-repo ||\n+\tmkdir non-repo ||\n+\treturn 1\n+\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd non-repo &&\n+\t\t\"$@\"\n+\t)\n+}\n+\n test_expect_success \\\n 'populate workdir' \\\n 'mkdir a &&\n@@ -179,6 +193,12 @@ test_expect_success 'git archive --remote' \\\n 'git archive --remote=. HEAD >b5.tar &&\n test_cmp_bin b.tar b5.tar'\n \n+test_expect_success 'git archive --remote with configured remote' '\n+\tgit config remote.foo.url . &&\n+\tgit archive --remote=foo HEAD >b5-nick.tar &&\n+\ttest_cmp_bin b.tar b5-nick.tar\n+'\n+\n test_expect_success \\\n 'validate file modification time' \\\n 'mkdir extract &&\n@@ -197,9 +217,15 @@ test_expect_success 'git archive with --output, override inferred format' '\n \ttest_cmp_bin b.tar d4.zip\n '\n \n-test_expect_success \\\n- 'git archive --list outside of a git repo' \\\n- 'GIT_DIR=some/non-existing/directory git archive --list'\n+test_expect_success 'git archive --list outside of a git repo' '\n+\tnongit git archive --list\n+'\n+\n+test_expect_success 'git archive --remote outside of a git repo' '\n+\tgit archive HEAD >expect.tar &&\n+\tnongit git archive --remote=\"$PWD\" HEAD >actual.tar &&\n+\ttest_cmp_bin expect.tar actual.tar\n+'\n \n test_expect_success 'clients cannot access unreachable commits' '\n \ttest_commit unreachable &&\n"},{"id":"306355","messageId":"xmqqeg23p5v3.fsf@gitster.mtv.corp.google.com","threadId":"44522","inReplyTo":"20161122211920.3qdohxdktykqhyga@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] stripspace: respect repository config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-22T21:22:24Z","receivedAt":"2016-11-22T21:22:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Nov 22, 2016 at 12:24:15PM -0800, Junio C Hamano wrote:\n>\n>> > Do you want to do another round of -rc3? Ship with the\n>> > minor regressions and fix them up in v2.11.1?\n>> \n>> I am leaning towards the former (though we may also end up doing the\n>> latter).\n>\n> I think I'd lead towards -rc3, as well. Our schedule is somewhat\n> arbitrary anyway, and one week is less important than somebody picking\n> up the new release and trying to puzzle out a regression that we already\n> have a fix for.\n\nOK. The \"mailinfo\" part turns out a bit more than RUN_SETUP_GENTLY\nas it takes paths from the command line that needs to be adjusted\nfor the prefix.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Tue, 22 Nov 2016 13:13:16 -0800\nSubject: [PATCH] mailinfo: read local configuration\n\nSince b9605bc4f2 (\"config: only read .git/config from configured\nrepos\", 2016-09-12), we do not read from \".git/config\" unless we\nknow we are in a repository. \"git mailinfo\" however didn't do the\nrepository discovery and instead relied on the old behaviour. This\nwas mostly OK because it was merely run as a helper program by other\nporcelain scripts that first chdir's up to the root of the working\ntree.\n\nTeach the command to run a \"gentle\" version of repository discovery\nso that local configuration variables like mailinfo.scissors are\nhonoured.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/mailinfo.c | 19 +++++++++++++++----\n git.c | 2 +-\n t/t5100-mailinfo.sh | 13 +++++++++++++\n 3 files changed, 29 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/mailinfo.c b/builtin/mailinfo.c\nindex f6df274111..e3b62f2fc7 100644\n--- a/builtin/mailinfo.c\n+++ b/builtin/mailinfo.c\n@@ -11,15 +11,20 @@\n static const char mailinfo_usage[] =\n \t\"git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding= | -n] [--scissors | --no-scissors] < mail >info\";\n \n+static char *prefix_copy(const char *prefix, const char *filename)\n+{\n+\tif (!prefix || is_absolute_path(filename))\n+\t\treturn xstrdup(filename);\n+\treturn xstrdup(prefix_filename(prefix, strlen(prefix), filename));\n+}\n+\n int cmd_mailinfo(int argc, const char **argv, const char *prefix)\n {\n \tconst char *def_charset;\n \tstruct mailinfo mi;\n \tint status;\n+\tchar *msgfile, *patchfile;\n \n-\t/* NEEDSWORK: might want to do the optional .git/ directory\n-\t * discovery\n-\t */\n \tsetup_mailinfo(&mi);\n \n \tdef_charset = get_commit_output_encoding();\n@@ -54,8 +59,14 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)\n \n \tmi.input = stdin;\n \tmi.output = stdout;\n-\tstatus = !!mailinfo(&mi, argv[1], argv[2]);\n+\n+\tmsgfile = prefix_copy(prefix, argv[1]);\n+\tpatchfile = prefix_copy(prefix, argv[2]);\n+\n+\tstatus = !!mailinfo(&mi, msgfile, patchfile);\n \tclear_mailinfo(&mi);\n \n+\tfree(msgfile);\n+\tfree(patchfile);\n \treturn status;\n }\ndiff --git a/git.c b/git.c\nindex efa1059fe0..fd3abf85d2 100644\n--- a/git.c\n+++ b/git.c\n@@ -445,7 +445,7 @@ static struct cmd_struct commands[] = {\n \t{ \"ls-files\", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX },\n \t{ \"ls-remote\", cmd_ls_remote, RUN_SETUP_GENTLY },\n \t{ \"ls-tree\", cmd_ls_tree, RUN_SETUP },\n-\t{ \"mailinfo\", cmd_mailinfo },\n+\t{ \"mailinfo\", cmd_mailinfo, RUN_SETUP_GENTLY },\n \t{ \"mailsplit\", cmd_mailsplit },\n \t{ \"merge\", cmd_merge, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"merge-base\", cmd_merge_base, RUN_SETUP },\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex e6b995161e..7171f67539 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -158,4 +158,17 @@ test_expect_success 'mailinfo handles rfc2822 comment' '\n \ttest_cmp \"$DATA/comment.expect\" comment/info\n '\n \n+test_expect_success 'mailinfo with mailinfo.scissors config' '\n+\ttest_config mailinfo.scissors true &&\n+\t(\n+\t\tmkdir sub &&\n+\t\tcd sub &&\n+\t\tgit mailinfo ../msg0014.sc ../patch0014.sc <../0014 >../info0014.sc\n+\t) &&\n+\ttest_cmp \"$DATA/msg0014--scissors\" msg0014.sc &&\n+\ttest_cmp \"$DATA/patch0014--scissors\" patch0014.sc &&\n+\ttest_cmp \"$DATA/info0014--scissors\" info0014.sc\n+'\n+\n+\n test_done\n-- \n2.11.0-rc2-170-g4c384f318f\n\n"},{"id":"306356","messageId":"20161122212416.kv5srurlnidqufqr@sigill.intra.peff.net","threadId":"44522","inReplyTo":"20161122211920.3qdohxdktykqhyga@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] stripspace: respect repository config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-22T21:24:16Z","receivedAt":"2016-11-22T21:24:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 22, 2016 at 04:19:20PM -0500, Jeff King wrote:\n\n> > > Do you want to do another round of -rc3? Ship with the\n> > > minor regressions and fix them up in v2.11.1?\n> > \n> > I am leaning towards the former (though we may also end up doing the\n> > latter).\n> \n> I think I'd lead towards -rc3, as well. Our schedule is somewhat\n\nI meant s/lead/lean/, of course.\n\n> Something like this, which does all but the last (and that should\n> probably happen separately post-release).\n\nAs usual, I'm utterly confused by the $(pwd) versus $PWD thing. So let\nme review what I did to see if it makes sense. :)\n\n> +# run \"$@\" inside a non-git directory\n> +nongit () {\n> +\ttest -d non-repo ||\n> +\tmkdir non-repo ||\n> +\treturn 1\n> +\n> +\t(\n> +\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n> +\t\texport GIT_CEILING_DIRECTORIES &&\n> +\t\tcd non-repo &&\n> +\t\t\"$@\"\n> +\t)\n> +}\n\nI copied this bit from t1515, which sets up a similar scenario. I think\nit _probably_ works either way because of the environment-variable\nconversion magic that Windows bash does.\n\n> +test_expect_success 'git archive --remote outside of a git repo' '\n> +\tgit archive HEAD >expect.tar &&\n> +\tnongit git archive --remote=\"$PWD\" HEAD >actual.tar &&\n> +\ttest_cmp_bin expect.tar actual.tar\n> +'\n\nI'm not sure if it matters here. I almost wrote $TRASH_DIRECTORY, but\nthat I think is in the same form as $PWD. Either would be fine.\n\n-Peff\n"},{"id":"306357","messageId":"20161122214305.yrn4uqh4dzzafkd2@sigill.intra.peff.net","threadId":"44522","inReplyTo":"xmqqeg23p5v3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] stripspace: respect repository config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-22T21:43:06Z","receivedAt":"2016-11-22T21:43:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 22, 2016 at 01:22:24PM -0800, Junio C Hamano wrote:\n\n> OK. The \"mailinfo\" part turns out a bit more than RUN_SETUP_GENTLY\n> as it takes paths from the command line that needs to be adjusted\n> for the prefix.\n\nI wondered at first if our lack of parse-options here was holding us\nback from using OPT_FILENAME. Though even if that was the case, it is\nprobably better to do the minimal fix in this instance, rather than\nconverting the option parsing.\n\nHowever, the paths which need munged are not even options, they are\narguments. So we wouldn't be able to rely on OPT_FILENAME anyway. I'm\nsurprised we haven't had to deal with this elsewhere, but I guess most\ncommands don't take arbitrary filenames (things like `add` take\npathspecs, and then the pathspec machinery takes care of the details).\n\nThe only other case I could think of is git-apply, and it does use\nprefix_filename() correctly.\n\n> +static char *prefix_copy(const char *prefix, const char *filename)\n> +{\n> +\tif (!prefix || is_absolute_path(filename))\n> +\t\treturn xstrdup(filename);\n> +\treturn xstrdup(prefix_filename(prefix, strlen(prefix), filename));\n> +}\n\nSo this is more or less a copy of parse-option's fix_filename(), which\nmakes sense.\n\nI noticed that git-apply does not check is_absolute_path(), but that's\nOK, as prefix_filename() does. So I think it would be correct to drop it\nhere, but it doesn't matter much either way.\n\n> int cmd_mailinfo(int argc, const char **argv, const char *prefix)\n> {\n> \tconst char *def_charset;\n> \tstruct mailinfo mi;\n> \tint status;\n> +\tchar *msgfile, *patchfile;\n> \n> -\t/* NEEDSWORK: might want to do the optional .git/ directory\n> -\t * discovery\n> -\t */\n> \tsetup_mailinfo(&mi);\n\nThis part looks straightforward and correct. Dropping the NEEDSWORK is a\nnice bonus.\n\n> +test_expect_success 'mailinfo with mailinfo.scissors config' '\n> +\ttest_config mailinfo.scissors true &&\n> +\t(\n> +\t\tmkdir sub &&\n> +\t\tcd sub &&\n> +\t\tgit mailinfo ../msg0014.sc ../patch0014.sc <../0014 >../info0014.sc\n> +\t) &&\n> +\ttest_cmp \"$DATA/msg0014--scissors\" msg0014.sc &&\n> +\ttest_cmp \"$DATA/patch0014--scissors\" patch0014.sc &&\n> +\ttest_cmp \"$DATA/info0014--scissors\" info0014.sc\n> +'\n\nAnd this test makes sense. Even without \"sub\", it would show the\nregression, but it's a good idea to test the sub-directory case to cover\nthe path-munging.\n\nIn the \"archive --remote\" test I added, we may want to do the same to\nshow that \"--output\" points at the correct path.\n\n-Peff\n"},{"id":"306358","messageId":"xmqq7f7vp4ck.fsf@gitster.mtv.corp.google.com","threadId":"44522","inReplyTo":"20161122214305.yrn4uqh4dzzafkd2@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] stripspace: respect repository config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-22T21:55:07Z","receivedAt":"2016-11-22T21:56:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +test_expect_success 'mailinfo with mailinfo.scissors config' '\n>> +\ttest_config mailinfo.scissors true &&\n>> +\t(\n>> +\t\tmkdir sub &&\n>> +\t\tcd sub &&\n>> +\t\tgit mailinfo ../msg0014.sc ../patch0014.sc <../0014 >../info0014.sc\n>> +\t) &&\n>> +\ttest_cmp \"$DATA/msg0014--scissors\" msg0014.sc &&\n>> +\ttest_cmp \"$DATA/patch0014--scissors\" patch0014.sc &&\n>> +\ttest_cmp \"$DATA/info0014--scissors\" info0014.sc\n>> +'\n>\n> And this test makes sense. Even without \"sub\", it would show the\n> regression, but it's a good idea to test the sub-directory case to cover\n> the path-munging.\n\nYup. Obviously during my initial attempt I was scratching my head\nwondering where these two files went--they were later found inside\nt/ directory which was really bad ;-)\n\n> In the \"archive --remote\" test I added, we may want to do the same to\n> show that \"--output\" points at the correct path.\n\nPerhaps something like this. By going down one level, we make sure\nthat it is not sufficient to accidentally read from .git/config to\nfind out what 'foo' is, and also ../b5-nick.tar that is relative to\nthe prefix (aka 'a/') ends up at the top-level.\n\n t/t5000-tar-tree.sh | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 09df7f0458..830bf2a2f6 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -195,7 +195,10 @@ test_expect_success 'git archive --remote' \\\n \n test_expect_success 'git archive --remote with configured remote' '\n \tgit config remote.foo.url . &&\n-\tgit archive --remote=foo HEAD >b5-nick.tar &&\n+\t(\n+\t\tcd a &&\n+\t\tgit archive --remote=foo --output=../b5-nick.tar HEAD\n+\t) &&\n \ttest_cmp_bin b.tar b5-nick.tar\n '\n \n"},{"id":"306367","messageId":"20161123001259.y7wjo75bhdgxmeme@sigill.intra.peff.net","threadId":"44522","inReplyTo":"xmqq7f7vp4ck.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] stripspace: respect repository config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-23T00:12:59Z","receivedAt":"2016-11-23T00:13:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 22, 2016 at 01:55:07PM -0800, Junio C Hamano wrote:\n\n> > And this test makes sense. Even without \"sub\", it would show the\n> > regression, but it's a good idea to test the sub-directory case to cover\n> > the path-munging.\n> \n> Yup. Obviously during my initial attempt I was scratching my head\n> wondering where these two files went--they were later found inside\n> t/ directory which was really bad ;-)\n\nHeh. Yeah, it's nice when the failure mode doesn't escape the trash\ndirectory, but it's probably not worth worrying about too much.\n\n> > In the \"archive --remote\" test I added, we may want to do the same to\n> > show that \"--output\" points at the correct path.\n> \n> Perhaps something like this. By going down one level, we make sure\n> that it is not sufficient to accidentally read from .git/config to\n> find out what 'foo' is, and also ../b5-nick.tar that is relative to\n> the prefix (aka 'a/') ends up at the top-level.\n\nYeah, your modification looks good.\n\n-Peff\n"},{"id":"306374","messageId":"alpine.DEB.2.20.1611231204410.3746@virtualbox","threadId":"44522","inReplyTo":"xmqqwpfvqwbq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] rebase -i: highlight problems with core.commentchar","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-11-23T11:05:19Z","receivedAt":"2016-11-23T11:05:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 22 Nov 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > On Mon, 21 Nov 2016, Junio C Hamano wrote:\n> >\n> >> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh\n> >> index 29e91d861c..c1f6411eb2 100755\n> >> --- a/t/t0030-stripspace.sh\n> >> +++ b/t/t0030-stripspace.sh\n> >> @@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '\n> >> \ttest_cmp expect actual\n> >> '\n> >> \n> >> +test_expect_failure '-c with comment char defined in .git/config' '\n> >> +\ttest_config core.commentchar = &&\n> >> +\tprintf \"= foo\\n\" >expect &&\n> >> +\tprintf \"foo\" | (\n> >\n> > Could I ask you to sneak in a \\n here?\n> \n> The one before that _is_ about fixing incomplete line, but this and\n> the one before this one are not, so I think we should fix them at\n> the same time.\n> \n> But does it break anything to leave it as-is? If not, I'd prefer to\n> leave this (and one before this one) for a later clean-up patch post\n> release.\n\nFair enough.\n\nAs a matter of fact, we do not even need to change it later. If it ain't\nbroke, don't fix it.\n\nCiao,\nDscho\n"}]}