[PATCH 0/3] Fix problems with rebase -i when core.commentchar is defined

[PATCH 0/3] Fix problems with rebase -i when core.commentchar is defined

[Johannes Schindelin]

The Git for Windows project recently got a bug report that detailed how
`git rebase -i` fails when core.commentchar=auto:

	https://groups.google.com/forum/#!topic/git-for-windows/eOZKjkgyX1Q

This patch series fixes rebase -i's handling of core.commentchar.


Johannes Schindelin (3):
  rebase -i: identify problems with core.commentchar
  stripspace: respect repository config
  rebase -i: handle core.commentChar=auto

 builtin/stripspace.c          |  4 +++-
 git-rebase--interactive.sh    | 13 +++++++++++--
 t/t0030-stripspace.sh         |  7 +++++++
 t/t3404-rebase-interactive.sh | 12 ++++++++++++
 4 files changed, 33 insertions(+), 3 deletions(-)


base-commit: 1310affe024fba407bff55dbe65cd6d670c8a32d
Published-As: https://github.com/dscho/git/releases/tag/rebase-i-commentchar-v1
Fetch-It-Via: git fetch https://github.com/dscho/git rebase-i-commentchar-v1

-- 
2.10.1.583.g721a9e0

[PATCH 1/3] rebase -i: identify problems with core.commentchar

[Johannes Schindelin]

The interactive rebase does not currently play well with
core.commentchar. Let's add some tests to identify those problems.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t0030-stripspace.sh         |  7 +++++++
 t/t3404-rebase-interactive.sh | 12 ++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index 29e91d8..202ac07 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -432,6 +432,13 @@ test_expect_success '-c with changed comment char' '
 	test_cmp expect actual
 '
 
+test_expect_failure '-c with comment char defined in .git/config' '
+	test_config core.commentchar = &&
+	printf "= foo\n" >expect &&
+	printf "foo" | git stripspace -c >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'avoid SP-HT sequence in commented line' '
 	printf "#\tone\n#\n# two\n" >expect &&
 	printf "\tone\n\ntwo\n" | git stripspace -c >actual &&
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e38e296..e080399 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -976,6 +976,18 @@ test_expect_success 'rebase -i respects core.commentchar' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_failure 'rebase -i respects core.commentchar=auto' '
+	test_config core.commentchar auto &&
+	write_script copy-edit-script.sh <<-\EOF &&
+	cp "$1" edit-script
+	EOF
+	test_set_editor "$(pwd)/copy-edit-script.sh" &&
+	test_when_finished "git rebase --abort || :" &&
+	git rebase -i HEAD^ &&
+	grep "^#" edit-script &&
+	test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
+'
+
 test_expect_success 'rebase -i, with <onto> and <upstream> specified as :/quuxery' '
 	test_when_finished "git branch -D torebase" &&
 	git checkout -b torebase branch1 &&
-- 
2.10.1.583.g721a9e0

[PATCH 2/3] stripspace: respect repository config

[Johannes Schindelin]

When eff80a9 (Allow custom "comment char", 2013-01-16) taught the
`stripspace` command to respect the config setting `core.commentChar`,
it forgot that this variable may be defined in .git/config.

So when rebasing interactively with a commentChar defined in the current
repository's config, the help text at the bottom of the edit script
potentially used an incorrect comment character. This was not only
funny-looking, but also resulted in tons of warnings like this one:

	Warning: the command isn't recognized in the following line
	 - #

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/stripspace.c  | 4 +++-
 t/t0030-stripspace.sh | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 15e716e..1e62a00 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
 	if (argc)
 		usage_with_options(stripspace_usage, options);
 
-	if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
+	if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
+		setup_git_directory_gently(NULL);
 		git_config(git_default_config, NULL);
+	}
 
 	if (strbuf_read(&buf, 0, 1024) < 0)
 		die_errno("could not read the input");
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index 202ac07..67f77df 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -432,7 +432,7 @@ test_expect_success '-c with changed comment char' '
 	test_cmp expect actual
 '
 
-test_expect_failure '-c with comment char defined in .git/config' '
+test_expect_success '-c with comment char defined in .git/config' '
 	test_config core.commentchar = &&
 	printf "= foo\n" >expect &&
 	printf "foo" | git stripspace -c >actual &&
-- 
2.10.1.583.g721a9e0

[PATCH 3/3] rebase -i: handle core.commentChar=auto

[Johannes Schindelin]

When 84c9dc2 (commit: allow core.commentChar=auto for character auto
selection, 2014-05-17) extended the core.commentChar functionality to
allow for the value 'auto', it forgot that rebase -i was already taught to
handle core.commentChar, and in turn forgot to let rebase -i handle that
new value gracefully.

Reported by Taufiq Hoven.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh    | 13 +++++++++++--
 t/t3404-rebase-interactive.sh |  2 +-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ca994c5..6bb740c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -93,8 +93,17 @@ eval '
 GIT_CHERRY_PICK_HELP="$resolvemsg"
 export GIT_CHERRY_PICK_HELP
 
-comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
-: ${comment_char:=#}
+comment_char=$(git config --get core.commentchar 2>/dev/null)
+case "$comment_char" in
+''|auto)
+	comment_char=#
+	;;
+?)
+	;;
+*)
+	comment_char=$(comment_char | cut -c1)
+	;;
+esac
 
 warn () {
 	printf '%s\n' "$*" >&2
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e080399..0f3d177 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -976,7 +976,7 @@ test_expect_success 'rebase -i respects core.commentchar' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
-test_expect_failure 'rebase -i respects core.commentchar=auto' '
+test_expect_success 'rebase -i respects core.commentchar=auto' '
 	test_config core.commentchar auto &&
 	write_script copy-edit-script.sh <<-\EOF &&
 	cp "$1" edit-script
-- 
2.10.1.583.g721a9e0

Re: [PATCH 0/3] Fix problems with rebase -i when core.commentchar is defined

[Jacob Keller]

On Mon, Nov 21, 2016 at 6:18 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> The Git for Windows project recently got a bug report that detailed how
> `git rebase -i` fails when core.commentchar=auto:
>
>         https://groups.google.com/forum/#!topic/git-for-windows/eOZKjkgyX1Q
>
> This patch series fixes rebase -i's handling of core.commentchar.
>
>
> Johannes Schindelin (3):
>   rebase -i: identify problems with core.commentchar
>   stripspace: respect repository config
>   rebase -i: handle core.commentChar=auto
>
>  builtin/stripspace.c          |  4 +++-
>  git-rebase--interactive.sh    | 13 +++++++++++--
>  t/t0030-stripspace.sh         |  7 +++++++
>  t/t3404-rebase-interactive.sh | 12 ++++++++++++
>  4 files changed, 33 insertions(+), 3 deletions(-)
>
>
> base-commit: 1310affe024fba407bff55dbe65cd6d670c8a32d
> Published-As: https://github.com/dscho/git/releases/tag/rebase-i-commentchar-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git rebase-i-commentchar-v1
>
> --
> 2.10.1.583.g721a9e0
>

The series makes sense to me.

Thanks,
Jake

Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> index 29e91d8..202ac07 100755
> --- a/t/t0030-stripspace.sh
> +++ b/t/t0030-stripspace.sh
> @@ -432,6 +432,13 @@ test_expect_success '-c with changed comment char' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_failure '-c with comment char defined in .git/config' '
> +	test_config core.commentchar = &&
> +	printf "= foo\n" >expect &&
> +	printf "foo" | git stripspace -c >actual &&

We'd want "\n" on this printf to match the one before as well, as
this test is not about "does stripspace complete an incomplete
line?", I think.  

I could amend it while queuing, but I need to know if I am missing a
reason why this must be an incomplete line before doing so.

> +	test_cmp expect actual
> +'
> +

Is this a recent regression?  When applied on top of 'maint' or
older, it seems to pass just fine.

    ... Goes and looks ...

Interesting.  Peff's b9605bc4f2 ("config: only read .git/config from
configured repos", 2016-09-12) is where this starts failing, which
is understandable given the code change to builtin/stripspace.c in
[2/3].  

The analysis of the log message in [2/3] is wrong and needs
updating, though.  In the old world order it worked by accident to
call git_config() without calling setup_git_directory(); after
b9605bc4f2, that no longer is valid and is exposed as a bug.  

Your [2/3] is a good fix for that change.

In any case, well spotted.

>  test_expect_success 'avoid SP-HT sequence in commented line' '
>  	printf "#\tone\n#\n# two\n" >expect &&
>  	printf "\tone\n\ntwo\n" | git stripspace -c >actual &&
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index e38e296..e080399 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -976,6 +976,18 @@ test_expect_success 'rebase -i respects core.commentchar' '
>  	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>  '
>  
> +test_expect_failure 'rebase -i respects core.commentchar=auto' '
> +	test_config core.commentchar auto &&
> +	write_script copy-edit-script.sh <<-\EOF &&
> +	cp "$1" edit-script
> +	EOF
> +	test_set_editor "$(pwd)/copy-edit-script.sh" &&
> +	test_when_finished "git rebase --abort || :" &&
> +	git rebase -i HEAD^ &&
> +	grep "^#" edit-script &&

This was added for debugging that was forgotten?

> +	test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"

This says "There shouldn't be any line left once we remove
'#'-commented lines, empty lines and pick insns.".  OK.

The correction in [3/3] seems good.

> +'
> +
>  test_expect_success 'rebase -i, with <onto> and <upstream> specified as :/quuxery' '
>  	test_when_finished "git branch -D torebase" &&
>  	git checkout -b torebase branch1 &&

Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar

[Junio C Hamano]

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

>> +test_expect_failure '-c with comment char defined in .git/config' '
>> +	test_config core.commentchar = &&
>> +	printf "= foo\n" >expect &&
>> +	printf "foo" | git stripspace -c >actual &&
>
> We'd want "\n" on this printf to match the one before as well, as
> this test is not about "does stripspace complete an incomplete
> line?", I think.  
>
> I could amend it while queuing, but I need to know if I am missing a
> reason why this must be an incomplete line before doing so.
>
>> +	test_cmp expect actual
>> +'
>> +
>
> Is this a recent regression?  When applied on top of 'maint' or
> older, it seems to pass just fine.

I think we can force failure by running this test somewhere other
than the top level of the working tree.  A set of proposed amends
incoming ...

Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto

[Johannes Sixt]

Am 21.11.2016 um 15:18 schrieb Johannes Schindelin:
> -comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
> -: ${comment_char:=#}
> +comment_char=$(git config --get core.commentchar 2>/dev/null)
> +case "$comment_char" in
> +''|auto)
> +	comment_char=#
> +	;;
> +?)
> +	;;
> +*)
> +	comment_char=$(comment_char | cut -c1)

comment_char is a command? Did you mean

	comment_char=$(echo "$comment_char" | cut -c1)

It could be written without forking a process:

	comment_char=${comment_char%${comment_char#?}}

(aka "remove from the end what remains after removing the first character")

-- Hannes

Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> comment_char is a command? Did you mean
>
> 	comment_char=$(echo "$comment_char" | cut -c1)

;-)

> It could be written without forking a process:
>
> 	comment_char=${comment_char%${comment_char#?}}
>
> (aka "remove from the end what remains after removing the first character")

Hopefully nobody would include any glob metacharacters in there,
e.g. "core.commentchar='=*'", which would break that?

Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar

[Jeff King]

On Mon, Nov 21, 2016 at 10:15:43AM -0800, Junio C Hamano wrote:

> > +	test_cmp expect actual
> > +'
> > +
> 
> Is this a recent regression?  When applied on top of 'maint' or
> older, it seems to pass just fine.
> 
>     ... Goes and looks ...
> 
> Interesting.  Peff's b9605bc4f2 ("config: only read .git/config from
> configured repos", 2016-09-12) is where this starts failing, which
> is understandable given the code change to builtin/stripspace.c in
> [2/3]. 
> 
> The analysis of the log message in [2/3] is wrong and needs
> updating, though.  In the old world order it worked by accident to
> call git_config() without calling setup_git_directory(); after
> b9605bc4f2, that no longer is valid and is exposed as a bug.  

Yeah, I noticed that while reading the patch. My b9605bc4f2 did regress
this case, but called out the fact that "cd subdir && git stripspace"
would never have worked. So one step back, 2 steps forward, and Dscho's
patch is the right step forward.

-Peff

Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto

[Johannes Sixt]

Am 21.11.2016 um 19:40 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>> It could be written without forking a process:
>>
>> 	comment_char=${comment_char%${comment_char#?}}
>>
>> (aka "remove from the end what remains after removing the first character")
>
> Hopefully nobody would include any glob metacharacters in there,
> e.g. "core.commentchar='=*'", which would break that?

Heh. I tested a few variations, but not this one. Make it

  	comment_char=${comment_char%"${comment_char#?}"}

;)

-- Hannes

[PATCH 1/3] rebase -i: highlight problems with core.commentchar

[Junio C Hamano]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The interactive rebase does not currently play well with
core.commentchar. Let's add some tests to highlight those problems
that will be fixed in the remainder of the series.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0030-stripspace.sh         |  9 +++++++++
 t/t3404-rebase-interactive.sh | 11 +++++++++++
 2 files changed, 20 insertions(+)

diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index 29e91d861c..c1f6411eb2 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '
 	test_cmp expect actual
 '
 
+test_expect_failure '-c with comment char defined in .git/config' '
+	test_config core.commentchar = &&
+	printf "= foo\n" >expect &&
+	printf "foo" | (
+		mkdir sub && cd sub && git stripspace -c
+	) >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'avoid SP-HT sequence in commented line' '
 	printf "#\tone\n#\n# two\n" >expect &&
 	printf "\tone\n\ntwo\n" | git stripspace -c >actual &&
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d6d65a3a94..d941f0a69f 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -983,6 +983,17 @@ test_expect_success 'rebase -i respects core.commentchar' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_failure 'rebase -i respects core.commentchar=auto' '
+	test_config core.commentchar auto &&
+	write_script copy-edit-script.sh <<-\EOF &&
+	cp "$1" edit-script
+	EOF
+	test_set_editor "$(pwd)/copy-edit-script.sh" &&
+	test_when_finished "git rebase --abort || :" &&
+	git rebase -i HEAD^ &&
+	test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
+'
+
 test_expect_success 'rebase -i, with <onto> and <upstream> specified as :/quuxery' '
 	test_when_finished "git branch -D torebase" &&
 	git checkout -b torebase branch1 &&
-- 
2.11.0-rc2-154-g95ba452916

[PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The way "git stripspace" reads the configuration was not quite
correct, in that it forgot to probe for a possibly existing
repository (note: stripspace is designed to be usable outside the
repository as well) before doing so.  Due to this, .git/config was
read only when the command was run from the top-level of the working
tree.  

A recent change b9605bc4f2 ("config: only read .git/config from
configured repos", 2016-09-12) stopped reading the repository-local
configuration file ".git/config" unless the repository discovery
process is done, and ".git/config" is no longer read even when run
from the top-level, which exposed the bug even more.

When rebasing interactively with a commentChar defined in the
current repository's config, the help text at the bottom of the edit
script potentially used an incorrect comment character. This was not
only funny-looking, but also resulted in tons of warnings like this
one:

	Warning: the command isn't recognized in the following line
	 - #

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/stripspace.c  | 4 +++-
 t/t0030-stripspace.sh | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 15e716ef43..1e62a008cb 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
 	if (argc)
 		usage_with_options(stripspace_usage, options);
 
-	if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
+	if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
+		setup_git_directory_gently(NULL);
 		git_config(git_default_config, NULL);
+	}
 
 	if (strbuf_read(&buf, 0, 1024) < 0)
 		die_errno("could not read the input");
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index c1f6411eb2..bbf3e39e3d 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -432,7 +432,7 @@ test_expect_success '-c with changed comment char' '
 	test_cmp expect actual
 '
 
-test_expect_failure '-c with comment char defined in .git/config' '
+test_expect_success '-c with comment char defined in .git/config' '
 	test_config core.commentchar = &&
 	printf "= foo\n" >expect &&
 	printf "foo" | (
-- 
2.11.0-rc2-154-g95ba452916

[PATCH 3/3] rebase -i: handle core.commentChar=auto

[Junio C Hamano]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When 84c9dc2 (commit: allow core.commentChar=auto for character auto
selection, 2014-05-17) extended the core.commentChar functionality to
allow for the value 'auto', it forgot that rebase -i was already taught to
handle core.commentChar, and in turn forgot to let rebase -i handle that
new value gracefully.

Reported by Taufiq Hoven.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-rebase--interactive.sh    | 13 +++++++++++--
 t/t3404-rebase-interactive.sh |  2 +-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 655ebaa471..c167bc36b3 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -93,8 +93,17 @@ eval '
 GIT_CHERRY_PICK_HELP="$resolvemsg"
 export GIT_CHERRY_PICK_HELP
 
-comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
-: ${comment_char:=#}
+comment_char=$(git config --get core.commentchar 2>/dev/null)
+case "$comment_char" in
+'' | auto)
+	comment_char="#"
+	;;
+?)
+	;;
+*)
+	comment_char=$(echo "$comment_char" | cut -c1)
+	;;
+esac
 
 warn () {
 	printf '%s\n' "$*" >&2
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d941f0a69f..5d0a7dca9d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -983,7 +983,7 @@ test_expect_success 'rebase -i respects core.commentchar' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
-test_expect_failure 'rebase -i respects core.commentchar=auto' '
+test_expect_success 'rebase -i respects core.commentchar=auto' '
 	test_config core.commentchar auto &&
 	write_script copy-edit-script.sh <<-\EOF &&
 	cp "$1" edit-script
-- 
2.11.0-rc2-154-g95ba452916

Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 21.11.2016 um 19:40 schrieb Junio C Hamano:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>> It could be written without forking a process:
>>>
>>> 	comment_char=${comment_char%${comment_char#?}}
>>>
>>> (aka "remove from the end what remains after removing the first character")
>>
>> Hopefully nobody would include any glob metacharacters in there,
>> e.g. "core.commentchar='=*'", which would break that?
>
> Heh. I tested a few variations, but not this one. Make it
>
>  	comment_char=${comment_char%"${comment_char#?}"}

I tried a few implementations of shells, and decided that this is
not worth the portability hassle.

Setting comment-char to multi-letter sequence is not supported at
all, and this code is protecting itself from that nonsense---it does
not need to be fast, and Dscho already gives a fast path for a
single letter case in his patch.

Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Nov 21, 2016 at 10:15:43AM -0800, Junio C Hamano wrote:
>
>> > +	test_cmp expect actual
>> > +'
>> > +
>> 
>> Is this a recent regression?  When applied on top of 'maint' or
>> older, it seems to pass just fine.
>> 
>>     ... Goes and looks ...
>> 
>> Interesting.  Peff's b9605bc4f2 ("config: only read .git/config from
>> configured repos", 2016-09-12) is where this starts failing, which
>> is understandable given the code change to builtin/stripspace.c in
>> [2/3]. 
>> 
>> The analysis of the log message in [2/3] is wrong and needs
>> updating, though.  In the old world order it worked by accident to
>> call git_config() without calling setup_git_directory(); after
>> b9605bc4f2, that no longer is valid and is exposed as a bug.  
>
> Yeah, I noticed that while reading the patch. My b9605bc4f2 did regress
> this case, but called out the fact that "cd subdir && git stripspace"
> would never have worked. So one step back, 2 steps forward, and Dscho's
> patch is the right step forward.

Yes, absolutely.

I sent out a set of proposed amends, and the one for this step 1/3
runs the command inside a subdirectory to force it not to find the
.git/config file relative to its pwd, which can reveal the existing
breakage without help by b9605bc4f2 ;-) hence can be forked for
older maintenance tracks.

Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto

[Johannes Sixt]

Am 21.11.2016 um 20:07 schrieb Junio C Hamano:
> Setting comment-char to multi-letter sequence is not supported at
> all, and this code is protecting itself from that nonsense---it does
> not need to be fast, and Dscho already gives a fast path for a
> single letter case in his patch.

Fair enough, no problem.

-- Hannes

Re: [PATCH 1/3] rebase -i: highlight problems with core.commentchar

[Junio C Hamano]

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The interactive rebase does not currently play well with
> core.commentchar. Let's add some tests to highlight those problems
> that will be fixed in the remainder of the series.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

Sorry, I should have commented here after --- line what changes were
proposed by this set of amends.

>  t/t0030-stripspace.sh         |  9 +++++++++
>  t/t3404-rebase-interactive.sh | 11 +++++++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> index 29e91d861c..c1f6411eb2 100755
> --- a/t/t0030-stripspace.sh
> +++ b/t/t0030-stripspace.sh
> @@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_failure '-c with comment char defined in .git/config' '
> +	test_config core.commentchar = &&
> +	printf "= foo\n" >expect &&
> +	printf "foo" | (
> +		mkdir sub && cd sub && git stripspace -c
> +	) >actual &&
> +	test_cmp expect actual
> +'

I noticed that the lack of "\n" at the end was merely copied from the
previous existing test (i.e. "-c with with changed comment char"),
which was already wrong the same way, so I kept that part as-is.

Running "stripspace" in a subdirectory is to avoid the ".git/config
was read without repository discovery as long as the command runs at
the top-level of the working tree" accident so that this highlights
the breakage with or without Peff's b9605bc4f2 ("config: only read
.git/config from configured repos", 2016-09-12).


> +
>  test_expect_success 'avoid SP-HT sequence in commented line' '
>  	printf "#\tone\n#\n# two\n" >expect &&
>  	printf "\tone\n\ntwo\n" | git stripspace -c >actual &&
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index d6d65a3a94..d941f0a69f 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -983,6 +983,17 @@ test_expect_success 'rebase -i respects core.commentchar' '
>  	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>  '
>  
> +test_expect_failure 'rebase -i respects core.commentchar=auto' '
> +	test_config core.commentchar auto &&
> +	write_script copy-edit-script.sh <<-\EOF &&
> +	cp "$1" edit-script
> +	EOF
> +	test_set_editor "$(pwd)/copy-edit-script.sh" &&
> +	test_when_finished "git rebase --abort || :" &&
> +	git rebase -i HEAD^ &&
> +	test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
> +'
> +

Removed from here is a leftover debugging bit (grep "^#"
edit-script).

Re: [PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The way "git stripspace" reads the configuration was not quite
> correct, in that it forgot to probe for a possibly existing
> repository (note: stripspace is designed to be usable outside the
> repository as well) before doing so.  Due to this, .git/config was
> read only when the command was run from the top-level of the working
> tree.  
>
> A recent change b9605bc4f2 ("config: only read .git/config from
> configured repos", 2016-09-12) stopped reading the repository-local
> configuration file ".git/config" unless the repository discovery
> process is done, and ".git/config" is no longer read even when run
> from the top-level, which exposed the bug even more.

The above two paragraphs are rewritten from the original to explain
how this seemed to work (by accident) and its breakage surfaced in
"rebase -i" after b9605bc4f2 ("config: only read .git/config from
configured repos", 2016-09-12) better.  The use of stripspace in
"rebase-i" was done after cd_to_toplevel and it happened to work
before that commit.

Otherwise there is no change from the original.

> When rebasing interactively with a commentChar defined in the
> current repository's config, the help text at the bottom of the edit
> script potentially used an incorrect comment character. This was not
> only funny-looking, but also resulted in tons of warnings like this
> one:
>
> 	Warning: the command isn't recognized in the following line
> 	 - #
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/stripspace.c  | 4 +++-
>  t/t0030-stripspace.sh | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> index 15e716ef43..1e62a008cb 100644
> --- a/builtin/stripspace.c
> +++ b/builtin/stripspace.c
> @@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
>  	if (argc)
>  		usage_with_options(stripspace_usage, options);
>  
> -	if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
> +	if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> +		setup_git_directory_gently(NULL);
>  		git_config(git_default_config, NULL);
> +	}
>  
>  	if (strbuf_read(&buf, 0, 1024) < 0)
>  		die_errno("could not read the input");
> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> index c1f6411eb2..bbf3e39e3d 100755
> --- a/t/t0030-stripspace.sh
> +++ b/t/t0030-stripspace.sh
> @@ -432,7 +432,7 @@ test_expect_success '-c with changed comment char' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure '-c with comment char defined in .git/config' '
> +test_expect_success '-c with comment char defined in .git/config' '
>  	test_config core.commentchar = &&
>  	printf "= foo\n" >expect &&
>  	printf "foo" | (

Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto

[Junio C Hamano]

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

> @@ -93,8 +93,17 @@ eval '
>  GIT_CHERRY_PICK_HELP="$resolvemsg"
>  export GIT_CHERRY_PICK_HELP
>  
> -comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
> -: ${comment_char:=#}
> +comment_char=$(git config --get core.commentchar 2>/dev/null)
> +case "$comment_char" in
> +'' | auto)
> +	comment_char="#"
> +	;;
> +?)
> +	;;
> +*)
> +	comment_char=$(echo "$comment_char" | cut -c1)
> +	;;
> +esac

Amended in is a fix for a typo the other Johannes noticed.

Thanks.

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index d941f0a69f..5d0a7dca9d 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -983,7 +983,7 @@ test_expect_success 'rebase -i respects core.commentchar' '
>  	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>  '
>  
> -test_expect_failure 'rebase -i respects core.commentchar=auto' '
> +test_expect_success 'rebase -i respects core.commentchar=auto' '
>  	test_config core.commentchar auto &&
>  	write_script copy-edit-script.sh <<-\EOF &&
>  	cp "$1" edit-script

Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar

[Jeff King]

On Mon, Nov 21, 2016 at 11:12:39AM -0800, Junio C Hamano wrote:

> > Yeah, I noticed that while reading the patch. My b9605bc4f2 did regress
> > this case, but called out the fact that "cd subdir && git stripspace"
> > would never have worked. So one step back, 2 steps forward, and Dscho's
> > patch is the right step forward.
> 
> Yes, absolutely.
> 
> I sent out a set of proposed amends, and the one for this step 1/3
> runs the command inside a subdirectory to force it not to find the
> .git/config file relative to its pwd, which can reveal the existing
> breakage without help by b9605bc4f2 ;-) hence can be forked for
> older maintenance tracks.

Makes sense, and your amended patch looks good.

-Peff

Re: [PATCH 2/3] stripspace: respect repository config

[Duy Nguyen]

On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> When eff80a9 (Allow custom "comment char", 2013-01-16) taught the
> `stripspace` command to respect the config setting `core.commentChar`,
> it forgot that this variable may be defined in .git/config.
>
> So when rebasing interactively with a commentChar defined in the current
> repository's config, the help text at the bottom of the edit script
> potentially used an incorrect comment character. This was not only
> funny-looking, but also resulted in tons of warnings like this one:
>
>         Warning: the command isn't recognized in the following line
>          - #
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/stripspace.c  | 4 +++-
>  t/t0030-stripspace.sh | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> index 15e716e..1e62a00 100644
> --- a/builtin/stripspace.c
> +++ b/builtin/stripspace.c
> @@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
>         if (argc)
>                 usage_with_options(stripspace_usage, options);
>
> -       if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
> +       if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> +               setup_git_directory_gently(NULL);
>                 git_config(git_default_config, NULL);
> +       }

This conditional config file reading is a trap for similar bugs to
happen again. Is there any reason we should not just mark the command
RUN_SETUP_GENTLY in git.c and call git_config() here unconditionally?

>
>         if (strbuf_read(&buf, 0, 1024) < 0)
>                 die_errno("could not read the input");
-- 
Duy

Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto

[Duy Nguyen]

On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> When 84c9dc2 (commit: allow core.commentChar=auto for character auto
> selection, 2014-05-17) extended the core.commentChar functionality to
> allow for the value 'auto', it forgot that rebase -i was already taught to
> handle core.commentChar,

I think this is 180bad3 (rebase -i: respect core.commentchar -
2013-02-11) and a couple followup fixes. My bad.

> and in turn forgot to let rebase -i handle that
> new value gracefully.
>
> Reported by Taufiq Hoven.

Another report in the same area: a merge conflict always writes the
"Conflicts:" section in the commit message with '#' as comment char
when core.commentChar is auto. I've known this for a while, but it has
not bitten me hard enough to do something about it,

>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  git-rebase--interactive.sh    | 13 +++++++++++--
>  t/t3404-rebase-interactive.sh |  2 +-
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index ca994c5..6bb740c 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -93,8 +93,17 @@ eval '
>  GIT_CHERRY_PICK_HELP="$resolvemsg"
>  export GIT_CHERRY_PICK_HELP
>
> -comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
> -: ${comment_char:=#}
> +comment_char=$(git config --get core.commentchar 2>/dev/null)
> +case "$comment_char" in
> +''|auto)
> +       comment_char=#

My first thought was "this kinda defeats the purpose of auto, doesn't
it". But 'auto' here is irrelevant because we control the leading
character of all lines in the todo file. 'auto' makes little sense in
this context, falling back to any comment char would do.

So, ack (after you fix the $(comment_char other people have mentioned,
of course).
-- 
Duy

Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto

[Johannes Schindelin]

Hi Hannes,

On Mon, 21 Nov 2016, Johannes Sixt wrote:

> Am 21.11.2016 um 15:18 schrieb Johannes Schindelin:
> > -comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
> > -: ${comment_char:=#}
> > +comment_char=$(git config --get core.commentchar 2>/dev/null)
> > +case "$comment_char" in
> > +''|auto)
> > +	comment_char=#
> > +	;;
> > +?)
> > +	;;
> > +*)
> > +	comment_char=$(comment_char | cut -c1)
> 
> comment_char is a command? Did you mean
> 
> 	comment_char=$(echo "$comment_char" | cut -c1)

D'oh.

> It could be written without forking a process:
> 
> 	comment_char=${comment_char%${comment_char#?}}
> 
> (aka "remove from the end what remains after removing the first character")

I was considering this, actually, but it is rather unreadable. Better
rewrite it in C, to begin with.

Thanks,
Dscho

Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar

[Johannes Schindelin]

Hi Junio,

On Mon, 21 Nov 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> > index 29e91d8..202ac07 100755
> > --- a/t/t0030-stripspace.sh
> > +++ b/t/t0030-stripspace.sh
> > @@ -432,6 +432,13 @@ test_expect_success '-c with changed comment char' '
> >  	test_cmp expect actual
> >  '
> >  
> > +test_expect_failure '-c with comment char defined in .git/config' '
> > +	test_config core.commentchar = &&
> > +	printf "= foo\n" >expect &&
> > +	printf "foo" | git stripspace -c >actual &&
> 
> We'd want "\n" on this printf to match the one before as well,

True.

> The analysis of the log message in [2/3] is wrong and needs
> updating, though.

Thanks for following up on that, and for fixing the commit message.

> > +test_expect_failure 'rebase -i respects core.commentchar=auto' '
> > +	test_config core.commentchar auto &&
> > +	write_script copy-edit-script.sh <<-\EOF &&
> > +	cp "$1" edit-script
> > +	EOF
> > +	test_set_editor "$(pwd)/copy-edit-script.sh" &&
> > +	test_when_finished "git rebase --abort || :" &&
> > +	git rebase -i HEAD^ &&
> > +	grep "^#" edit-script &&
> 
> This was added for debugging that was forgotten?

No, this was me trying to ensure that the comment character '#' *is* used.
As opposed to somehow testing any edit script that contains no commented
lines whatsoever.

But if you don't care, I won't, either.

Ciao,
Dscho

Re: [PATCH 1/3] rebase -i: highlight problems with core.commentchar

[Johannes Schindelin]

Hi Junio,

On Mon, 21 Nov 2016, Junio C Hamano wrote:

> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> index 29e91d861c..c1f6411eb2 100755
> --- a/t/t0030-stripspace.sh
> +++ b/t/t0030-stripspace.sh
> @@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_failure '-c with comment char defined in .git/config' '
> +	test_config core.commentchar = &&
> +	printf "= foo\n" >expect &&
> +	printf "foo" | (

Could I ask you to sneak in a \n here?

Thanks,
Dscho

Re: [PATCH 2/3] stripspace: respect repository config

[Johannes Schindelin]

Hi Junio,

On Mon, 21 Nov 2016, Junio C Hamano wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> The way "git stripspace" reads the configuration was not quite
> correct, in that it forgot to probe for a possibly existing
> repository (note: stripspace is designed to be usable outside the
> repository as well) before doing so.  Due to this, .git/config was
> read only when the command was run from the top-level of the working
> tree.  
> 
> A recent change b9605bc4f2 ("config: only read .git/config from
> configured repos", 2016-09-12) stopped reading the repository-local
> configuration file ".git/config" unless the repository discovery
> process is done, and ".git/config" is no longer read even when run
> from the top-level, which exposed the bug even more.
> 
> When rebasing interactively with a commentChar defined in the
> current repository's config, the help text at the bottom of the edit
> script potentially used an incorrect comment character. This was not
> only funny-looking, but also resulted in tons of warnings like this
> one:
> 
> 	Warning: the command isn't recognized in the following line
> 	 - #
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Thanks for the corrected commit message!
Dscho

Re: [PATCH 2/3] stripspace: respect repository config

[Johannes Schindelin]

Hi Duy,

On Tue, 22 Nov 2016, Duy Nguyen wrote:

> On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > When eff80a9 (Allow custom "comment char", 2013-01-16) taught the
> > `stripspace` command to respect the config setting `core.commentChar`,
> > it forgot that this variable may be defined in .git/config.
> >
> > So when rebasing interactively with a commentChar defined in the current
> > repository's config, the help text at the bottom of the edit script
> > potentially used an incorrect comment character. This was not only
> > funny-looking, but also resulted in tons of warnings like this one:
> >
> >         Warning: the command isn't recognized in the following line
> >          - #
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/stripspace.c  | 4 +++-
> >  t/t0030-stripspace.sh | 2 +-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> > index 15e716e..1e62a00 100644
> > --- a/builtin/stripspace.c
> > +++ b/builtin/stripspace.c
> > @@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
> >         if (argc)
> >                 usage_with_options(stripspace_usage, options);
> >
> > -       if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
> > +       if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> > +               setup_git_directory_gently(NULL);
> >                 git_config(git_default_config, NULL);
> > +       }
> 
> This conditional config file reading is a trap for similar bugs to
> happen again. Is there any reason we should not just mark the command
> RUN_SETUP_GENTLY in git.c and call git_config() here unconditionally?

As I plan to slip these patches into Git for Windows v2.11.0, i.e. making
this a last-minute hot fix, I want to err on the side of caution. So I'd
rather keep this conditional (it might regress on the performance front,
or something).

Ciao,
Dscho

Re: [PATCH 1/3] rebase -i: highlight problems with core.commentchar

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Mon, 21 Nov 2016, Junio C Hamano wrote:
>
>> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
>> index 29e91d861c..c1f6411eb2 100755
>> --- a/t/t0030-stripspace.sh
>> +++ b/t/t0030-stripspace.sh
>> @@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '
>>  	test_cmp expect actual
>>  '
>>  
>> +test_expect_failure '-c with comment char defined in .git/config' '
>> +	test_config core.commentchar = &&
>> +	printf "= foo\n" >expect &&
>> +	printf "foo" | (
>
> Could I ask you to sneak in a \n here?

The one before that _is_ about fixing incomplete line, but this and
the one before this one are not, so I think we should fix them at
the same time.

But does it break anything to leave it as-is?  If not, I'd prefer to
leave this (and one before this one) for a later clean-up patch post
release.

Thanks

Re: [PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> This conditional config file reading is a trap for similar bugs to
>> happen again. Is there any reason we should not just mark the command
>> RUN_SETUP_GENTLY in git.c and call git_config() here unconditionally?
>
> As I plan to slip these patches into Git for Windows v2.11.0, i.e. making
> this a last-minute hot fix, I want to err on the side of caution.

So do I.  As a hot-fix, I'd prefer the patch I queued yesterday.

I think we want to audit the ones without RUN_SETUP* in the command
table in order to hunt for regression aka "a fix revealed a bug that
was covered by .git/config accidentally getting read when run from
the top-level of the working tree", though. We may find unreported
breakages that we may have to fix.

Re: [PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

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

> I think we want to audit the ones without RUN_SETUP* in the command
> table in order to hunt for regression aka "a fix revealed a bug that
> was covered by .git/config accidentally getting read when run from
> the top-level of the working tree", though. We may find unreported
> breakages that we may have to fix.

So I took a brief look at the "PROGRAM_OBJS += ..." in the Makefile
for non-builtin commands, and commands[] array in git.c for builtin
commands to see how bad it looks.


Archive & Upload-archive:

"cd Documentation && git archive --remote=origin" immediately hits
"BUG: setup_git_env called without repository" if your Git is built
with b1ef400eec ("setup_git_env: avoid blind fall-back to ".git"",
2016-10-20), which will not be part of the upcoming release.  And
'origin' will probably not be understood from the local config.

I think we can do the "gently" thing there, as we may be retrieving
a remote archive outside a local repository.  We'd need to tweak
"output" with prefix to compensate for the case in which the command
is run from a subdirectory, and probably we need to futz with the
setup_prefix parameter to write_archive(), as a local caller now
will know if we are in nongit situation.

On the upload-archive side that serves "--remote" request, there is
enter_repo() so we should be covered OK.


Mailinfo:

We may want a "gently" thing there to pick up local configuration.
i18n.commitencoding and mailinfo.scissors in local repository would
be ignored otherwise.


Splitspace:

Dscho fixed this one.


Verify-pack:

This calls git_config() but these days farms out its operation to
"index-pack", so we should be OK.  We may even want to lose the call
to git_config() which does not affect anything.

Re: [PATCH 2/3] stripspace: respect repository config

[Jeff King]

On Tue, Nov 22, 2016 at 11:10:25AM -0800, Junio C Hamano wrote:

> Archive & Upload-archive:
> 
> "cd Documentation && git archive --remote=origin" immediately hits
> "BUG: setup_git_env called without repository" if your Git is built
> with b1ef400eec ("setup_git_env: avoid blind fall-back to ".git"",
> 2016-10-20), which will not be part of the upcoming release.  And
> 'origin' will probably not be understood from the local config.

Yeah, I think "cd Documentation && git archive --remote" has always been
broken. It doesn't barf until b1ef400eec, but yes, v2.11-rc2 breaks
finding "origin" in the local config.

I agree that doing the "gently" setup will fix the config problem, but
there's more to do to make it work with b1ef400eec outside of a repo.
It looks like read_remotes_file() and read_branches_file() in remote.c
need to learn to silently return when we are not in a git repository (or
alternatively, remote_get_1() should learn not to call them in such a
case).

I doubt this is a critical case (it only kicks in if you are outside a
repo _and_ your name looks like a remote name and not a URL, _and_ it is
not defined by your config, and the result is that we die("BUG") rather
than reporting "you don't have such a remote"). But any time it is
possible to hit a die("BUG"), we should be fixing it.

> I think we can do the "gently" thing there, as we may be retrieving
> a remote archive outside a local repository.  We'd need to tweak
> "output" with prefix to compensate for the case in which the command
> is run from a subdirectory, and probably we need to futz with the
> setup_prefix parameter to write_archive(), as a local caller now
> will know if we are in nongit situation.

Makes sense. I think OPT_FILENAME() would handle "output", but yeah, it
looks like write_archive() already tries to be clever about the setup.

> On the upload-archive side that serves "--remote" request, there is
> enter_repo() so we should be covered OK.

Right, that has to run in a repo, and enter_repo() should handle it.

> Mailinfo:
> 
> We may want a "gently" thing there to pick up local configuration.
> i18n.commitencoding and mailinfo.scissors in local repository would
> be ignored otherwise.

Yeah, agreed.

> Verify-pack:
> 
> This calls git_config() but these days farms out its operation to
> "index-pack", so we should be OK.  We may even want to lose the call
> to git_config() which does not affect anything.

I'd be slightly worried that there is a core.* config option that
affects us in a subtle way (e.g., some portability flag for how we do
run_command()). I don't think there is one now, but it seems like a
potential maintenance pitfall. IOW, I think I'd generally prefer to err
on the side of having everything do a gentle setup and load at least the
default config, to avoid surprises.

I don't think it's a high priority, though.

So what do you want to do for v2.11? I think there are minor regressions
in stripspace, archive, and mailinfo with respect to reading
.git/config. The right solution in each case is to do a gentle repo
setup. We have patches for stripspace already, but not yet for the other
two. They _should_ be fairly trivial, but we are getting awfully close
to the release. Do you want to do another round of -rc3? Ship with the
minor regressions and fix them up in v2.11.1? Last-minute revert the
original config topic that introduced the regressions (I'm biased
against that as the author, but I also think it's dangerous; it's a big
enough topic that it might introduce new problems)?

-Peff

Re: [PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So what do you want to do for v2.11? I think there are minor regressions
> in stripspace, archive, and mailinfo with respect to reading
> .git/config. The right solution in each case is to do a gentle repo
> setup. We have patches for stripspace already, but not yet for the other
> two. They _should_ be fairly trivial, but we are getting awfully close
> to the release. 

Right.

> Do you want to do another round of -rc3? Ship with the
> minor regressions and fix them up in v2.11.1?

I am leaning towards the former (though we may also end up doing the
latter).

Here is an initial attempt.  It turns out that write_archive() must
be callable outside a repository to respond to "git archive --list",
which happens in parse_archive_args(), and the "have-repository?"
check cannot be moved before that to the caller.

No tests yet.  Help appreciated ;-)


 archive.c                | 9 ++-------
 archive.h                | 2 +-
 builtin/archive.c        | 6 +++---
 builtin/upload-archive.c | 2 +-
 git.c                    | 4 ++--
 5 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/archive.c b/archive.c
index dde1ab4c79..901b10264f 100644
--- a/archive.c
+++ b/archive.c
@@ -504,15 +504,11 @@ static int parse_archive_args(int argc, const char **argv,
 }
 
 int write_archive(int argc, const char **argv, const char *prefix,
-		  int setup_prefix, const char *name_hint, int remote)
+		  const char *name_hint, int remote)
 {
-	int nongit = 0;
 	const struct archiver *ar = NULL;
 	struct archiver_args args;
 
-	if (setup_prefix && prefix == NULL)
-		prefix = setup_git_directory_gently(&nongit);
-
 	git_config_get_bool("uploadarchive.allowunreachable", &remote_allow_unreachable);
 	git_config(git_default_config, NULL);
 
@@ -520,7 +516,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
 	init_zip_archiver();
 
 	argc = parse_archive_args(argc, argv, &ar, &args, name_hint, remote);
-	if (nongit) {
+	if (!startup_info->have_repository) {
 		/*
 		 * We know this will die() with an error, so we could just
 		 * die ourselves; but its error message will be more specific
@@ -528,7 +524,6 @@ int write_archive(int argc, const char **argv, const char *prefix,
 		 */
 		setup_git_directory();
 	}
-
 	parse_treeish_arg(argv, &args, prefix, remote);
 	parse_pathspec_arg(argv + 1, &args);
 
diff --git a/archive.h b/archive.h
index 4a791e1fed..415e0152e2 100644
--- a/archive.h
+++ b/archive.h
@@ -36,7 +36,7 @@ typedef int (*write_archive_entry_fn_t)(struct archiver_args *args,
 					unsigned int mode);
 
 extern int write_archive_entries(struct archiver_args *args, write_archive_entry_fn_t write_entry);
-extern int write_archive(int argc, const char **argv, const char *prefix, int setup_prefix, const char *name_hint, int remote);
+extern int write_archive(int argc, const char **argv, const char *prefix, const char *name_hint, int remote);
 
 const char *archive_format_from_filename(const char *filename);
 extern void *sha1_file_to_archive(const struct archiver_args *args,
diff --git a/builtin/archive.c b/builtin/archive.c
index 49f491413a..f863465a0f 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -85,8 +85,8 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 	const char *output = NULL;
 	const char *remote = NULL;
 	struct option local_opts[] = {
-		OPT_STRING('o', "output", &output, N_("file"),
-			N_("write the archive to this file")),
+		OPT_FILENAME('o', "output", &output,
+			     N_("write the archive to this file")),
 		OPT_STRING(0, "remote", &remote, N_("repo"),
 			N_("retrieve the archive from remote repository <repo>")),
 		OPT_STRING(0, "exec", &exec, N_("command"),
@@ -105,5 +105,5 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 
 	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
 
-	return write_archive(argc, argv, prefix, 1, output, 0);
+	return write_archive(argc, argv, prefix, output, 0);
 }
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index dc872f6a94..cde06977b7 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -43,7 +43,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 	}
 
 	/* parse all options sent by the client */
-	return write_archive(sent_argv.argc, sent_argv.argv, prefix, 0, NULL, 1);
+	return write_archive(sent_argv.argc, sent_argv.argv, prefix, NULL, 1);
 }
 
 __attribute__((format (printf, 1, 2)))
diff --git a/git.c b/git.c
index efa1059fe0..e8b2baf2d1 100644
--- a/git.c
+++ b/git.c
@@ -396,7 +396,7 @@ static struct cmd_struct commands[] = {
 	{ "am", cmd_am, RUN_SETUP | NEED_WORK_TREE },
 	{ "annotate", cmd_annotate, RUN_SETUP },
 	{ "apply", cmd_apply, RUN_SETUP_GENTLY },
-	{ "archive", cmd_archive },
+	{ "archive", cmd_archive, RUN_SETUP_GENTLY },
 	{ "bisect--helper", cmd_bisect__helper, RUN_SETUP },
 	{ "blame", cmd_blame, RUN_SETUP },
 	{ "branch", cmd_branch, RUN_SETUP },
@@ -445,7 +445,7 @@ static struct cmd_struct commands[] = {
 	{ "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX },
 	{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
 	{ "ls-tree", cmd_ls_tree, RUN_SETUP },
-	{ "mailinfo", cmd_mailinfo },
+	{ "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY },
 	{ "mailsplit", cmd_mailsplit },
 	{ "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
 	{ "merge-base", cmd_merge_base, RUN_SETUP },

Re: [PATCH 2/3] stripspace: respect repository config

[Jeff King]

On Tue, Nov 22, 2016 at 12:24:15PM -0800, Junio C Hamano wrote:

> > Do you want to do another round of -rc3? Ship with the
> > minor regressions and fix them up in v2.11.1?
> 
> I am leaning towards the former (though we may also end up doing the
> latter).

I think I'd lead towards -rc3, as well. Our schedule is somewhat
arbitrary anyway, and one week is less important than somebody picking
up the new release and trying to puzzle out a regression that we already
have a fix for.

> Here is an initial attempt.  It turns out that write_archive() must
> be callable outside a repository to respond to "git archive --list",
> which happens in parse_archive_args(), and the "have-repository?"
> check cannot be moved before that to the caller.

Right, that makes sense. t5000 already tests git-archive outside of a
repo, although it does it in a funny way (it sets GIT_DIR rather than
relying on GIT_CEILING_DIRECTORIES). We should cover that and make sure
that --remote works outside of a repo. Those work now, but we want to
make sure we don't regress them.

And then we want to test that --remote can read a configured remote
name.

And then post-v2.11, we'd want to check that --remote=foo outside of a
repo produces a sane error instead of looking for a bogus
$GIT_DIR/remotes/foo.

Something like this, which does all but the last (and that should
probably happen separately post-release).

---
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 80b238734..09df7f045 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -94,6 +94,20 @@ check_tar() {
 	'
 }
 
+# run "$@" inside a non-git directory
+nongit () {
+	test -d non-repo ||
+	mkdir non-repo ||
+	return 1
+
+	(
+		GIT_CEILING_DIRECTORIES=$(pwd) &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd non-repo &&
+		"$@"
+	)
+}
+
 test_expect_success \
     'populate workdir' \
     'mkdir a &&
@@ -179,6 +193,12 @@ test_expect_success 'git archive --remote' \
     'git archive --remote=. HEAD >b5.tar &&
     test_cmp_bin b.tar b5.tar'
 
+test_expect_success 'git archive --remote with configured remote' '
+	git config remote.foo.url . &&
+	git archive --remote=foo HEAD >b5-nick.tar &&
+	test_cmp_bin b.tar b5-nick.tar
+'
+
 test_expect_success \
     'validate file modification time' \
     'mkdir extract &&
@@ -197,9 +217,15 @@ test_expect_success 'git archive with --output, override inferred format' '
 	test_cmp_bin b.tar d4.zip
 '
 
-test_expect_success \
-    'git archive --list outside of a git repo' \
-    'GIT_DIR=some/non-existing/directory git archive --list'
+test_expect_success 'git archive --list outside of a git repo' '
+	nongit git archive --list
+'
+
+test_expect_success 'git archive --remote outside of a git repo' '
+	git archive HEAD >expect.tar &&
+	nongit git archive --remote="$PWD" HEAD >actual.tar &&
+	test_cmp_bin expect.tar actual.tar
+'
 
 test_expect_success 'clients cannot access unreachable commits' '
 	test_commit unreachable &&

Re: [PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Nov 22, 2016 at 12:24:15PM -0800, Junio C Hamano wrote:
>
>> > Do you want to do another round of -rc3? Ship with the
>> > minor regressions and fix them up in v2.11.1?
>> 
>> I am leaning towards the former (though we may also end up doing the
>> latter).
>
> I think I'd lead towards -rc3, as well. Our schedule is somewhat
> arbitrary anyway, and one week is less important than somebody picking
> up the new release and trying to puzzle out a regression that we already
> have a fix for.

OK.  The "mailinfo" part turns out a bit more than RUN_SETUP_GENTLY
as it takes paths from the command line that needs to be adjusted
for the prefix.

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 22 Nov 2016 13:13:16 -0800
Subject: [PATCH] mailinfo: read local configuration

Since b9605bc4f2 ("config: only read .git/config from configured
repos", 2016-09-12), we do not read from ".git/config" unless we
know we are in a repository.  "git mailinfo" however didn't do the
repository discovery and instead relied on the old behaviour.  This
was mostly OK because it was merely run as a helper program by other
porcelain scripts that first chdir's up to the root of the working
tree.

Teach the command to run a "gentle" version of repository discovery
so that local configuration variables like mailinfo.scissors are
honoured.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/mailinfo.c  | 19 +++++++++++++++----
 git.c               |  2 +-
 t/t5100-mailinfo.sh | 13 +++++++++++++
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index f6df274111..e3b62f2fc7 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -11,15 +11,20 @@
 static const char mailinfo_usage[] =
 	"git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding=<encoding> | -n] [--scissors | --no-scissors] <msg> <patch> < mail >info";
 
+static char *prefix_copy(const char *prefix, const char *filename)
+{
+	if (!prefix || is_absolute_path(filename))
+		return xstrdup(filename);
+	return xstrdup(prefix_filename(prefix, strlen(prefix), filename));
+}
+
 int cmd_mailinfo(int argc, const char **argv, const char *prefix)
 {
 	const char *def_charset;
 	struct mailinfo mi;
 	int status;
+	char *msgfile, *patchfile;
 
-	/* NEEDSWORK: might want to do the optional .git/ directory
-	 * discovery
-	 */
 	setup_mailinfo(&mi);
 
 	def_charset = get_commit_output_encoding();
@@ -54,8 +59,14 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)
 
 	mi.input = stdin;
 	mi.output = stdout;
-	status = !!mailinfo(&mi, argv[1], argv[2]);
+
+	msgfile = prefix_copy(prefix, argv[1]);
+	patchfile = prefix_copy(prefix, argv[2]);
+
+	status = !!mailinfo(&mi, msgfile, patchfile);
 	clear_mailinfo(&mi);
 
+	free(msgfile);
+	free(patchfile);
 	return status;
 }
diff --git a/git.c b/git.c
index efa1059fe0..fd3abf85d2 100644
--- a/git.c
+++ b/git.c
@@ -445,7 +445,7 @@ static struct cmd_struct commands[] = {
 	{ "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX },
 	{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
 	{ "ls-tree", cmd_ls_tree, RUN_SETUP },
-	{ "mailinfo", cmd_mailinfo },
+	{ "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY },
 	{ "mailsplit", cmd_mailsplit },
 	{ "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
 	{ "merge-base", cmd_merge_base, RUN_SETUP },
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index e6b995161e..7171f67539 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -158,4 +158,17 @@ test_expect_success 'mailinfo handles rfc2822 comment' '
 	test_cmp "$DATA/comment.expect" comment/info
 '
 
+test_expect_success 'mailinfo with mailinfo.scissors config' '
+	test_config mailinfo.scissors true &&
+	(
+		mkdir sub &&
+		cd sub &&
+		git mailinfo ../msg0014.sc ../patch0014.sc <../0014 >../info0014.sc
+	) &&
+	test_cmp "$DATA/msg0014--scissors" msg0014.sc &&
+	test_cmp "$DATA/patch0014--scissors" patch0014.sc &&
+	test_cmp "$DATA/info0014--scissors" info0014.sc
+'
+
+
 test_done
-- 
2.11.0-rc2-170-g4c384f318f

Re: [PATCH 2/3] stripspace: respect repository config

[Jeff King]

On Tue, Nov 22, 2016 at 04:19:20PM -0500, Jeff King wrote:

> > > Do you want to do another round of -rc3? Ship with the
> > > minor regressions and fix them up in v2.11.1?
> > 
> > I am leaning towards the former (though we may also end up doing the
> > latter).
> 
> I think I'd lead towards -rc3, as well. Our schedule is somewhat

I meant s/lead/lean/, of course.

> Something like this, which does all but the last (and that should
> probably happen separately post-release).

As usual, I'm utterly confused by the $(pwd) versus $PWD thing. So let
me review what I did to see if it makes sense. :)

> +# run "$@" inside a non-git directory
> +nongit () {
> +	test -d non-repo ||
> +	mkdir non-repo ||
> +	return 1
> +
> +	(
> +		GIT_CEILING_DIRECTORIES=$(pwd) &&
> +		export GIT_CEILING_DIRECTORIES &&
> +		cd non-repo &&
> +		"$@"
> +	)
> +}

I copied this bit from t1515, which sets up a similar scenario. I think
it _probably_ works either way because of the environment-variable
conversion magic that Windows bash does.

> +test_expect_success 'git archive --remote outside of a git repo' '
> +	git archive HEAD >expect.tar &&
> +	nongit git archive --remote="$PWD" HEAD >actual.tar &&
> +	test_cmp_bin expect.tar actual.tar
> +'

I'm not sure if it matters here. I almost wrote $TRASH_DIRECTORY, but
that I think is in the same form as $PWD. Either would be fine.

-Peff

Re: [PATCH 2/3] stripspace: respect repository config

[Jeff King]

On Tue, Nov 22, 2016 at 01:22:24PM -0800, Junio C Hamano wrote:

> OK.  The "mailinfo" part turns out a bit more than RUN_SETUP_GENTLY
> as it takes paths from the command line that needs to be adjusted
> for the prefix.

I wondered at first if our lack of parse-options here was holding us
back from using OPT_FILENAME. Though even if that was the case, it is
probably better to do the minimal fix in this instance, rather than
converting the option parsing.

However, the paths which need munged are not even options, they are
arguments. So we wouldn't be able to rely on OPT_FILENAME anyway. I'm
surprised we haven't had to deal with this elsewhere, but I guess most
commands don't take arbitrary filenames (things like `add` take
pathspecs, and then the pathspec machinery takes care of the details).

The only other case I could think of is git-apply, and it does use
prefix_filename() correctly.

> +static char *prefix_copy(const char *prefix, const char *filename)
> +{
> +	if (!prefix || is_absolute_path(filename))
> +		return xstrdup(filename);
> +	return xstrdup(prefix_filename(prefix, strlen(prefix), filename));
> +}

So this is more or less a copy of parse-option's fix_filename(), which
makes sense.

I noticed that git-apply does not check is_absolute_path(), but that's
OK, as prefix_filename() does. So I think it would be correct to drop it
here, but it doesn't matter much either way.

>  int cmd_mailinfo(int argc, const char **argv, const char *prefix)
>  {
>  	const char *def_charset;
>  	struct mailinfo mi;
>  	int status;
> +	char *msgfile, *patchfile;
>  
> -	/* NEEDSWORK: might want to do the optional .git/ directory
> -	 * discovery
> -	 */
>  	setup_mailinfo(&mi);

This part looks straightforward and correct. Dropping the NEEDSWORK is a
nice bonus.

> +test_expect_success 'mailinfo with mailinfo.scissors config' '
> +	test_config mailinfo.scissors true &&
> +	(
> +		mkdir sub &&
> +		cd sub &&
> +		git mailinfo ../msg0014.sc ../patch0014.sc <../0014 >../info0014.sc
> +	) &&
> +	test_cmp "$DATA/msg0014--scissors" msg0014.sc &&
> +	test_cmp "$DATA/patch0014--scissors" patch0014.sc &&
> +	test_cmp "$DATA/info0014--scissors" info0014.sc
> +'

And this test makes sense. Even without "sub", it would show the
regression, but it's a good idea to test the sub-directory case to cover
the path-munging.

In the "archive --remote" test I added, we may want to do the same to
show that "--output" points at the correct path.

-Peff

Re: [PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +test_expect_success 'mailinfo with mailinfo.scissors config' '
>> +	test_config mailinfo.scissors true &&
>> +	(
>> +		mkdir sub &&
>> +		cd sub &&
>> +		git mailinfo ../msg0014.sc ../patch0014.sc <../0014 >../info0014.sc
>> +	) &&
>> +	test_cmp "$DATA/msg0014--scissors" msg0014.sc &&
>> +	test_cmp "$DATA/patch0014--scissors" patch0014.sc &&
>> +	test_cmp "$DATA/info0014--scissors" info0014.sc
>> +'
>
> And this test makes sense. Even without "sub", it would show the
> regression, but it's a good idea to test the sub-directory case to cover
> the path-munging.

Yup.  Obviously during my initial attempt I was scratching my head
wondering where these two files went--they were later found inside
t/ directory which was really bad ;-)

> In the "archive --remote" test I added, we may want to do the same to
> show that "--output" points at the correct path.

Perhaps something like this.  By going down one level, we make sure
that it is not sufficient to accidentally read from .git/config to
find out what 'foo' is, and also ../b5-nick.tar that is relative to
the prefix (aka 'a/') ends up at the top-level.

 t/t5000-tar-tree.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 09df7f0458..830bf2a2f6 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -195,7 +195,10 @@ test_expect_success 'git archive --remote' \
 
 test_expect_success 'git archive --remote with configured remote' '
 	git config remote.foo.url . &&
-	git archive --remote=foo HEAD >b5-nick.tar &&
+	(
+		cd a &&
+		git archive --remote=foo --output=../b5-nick.tar HEAD
+	) &&
 	test_cmp_bin b.tar b5-nick.tar
 '
 

Re: [PATCH 2/3] stripspace: respect repository config

[Jeff King]

On Tue, Nov 22, 2016 at 01:55:07PM -0800, Junio C Hamano wrote:

> > And this test makes sense. Even without "sub", it would show the
> > regression, but it's a good idea to test the sub-directory case to cover
> > the path-munging.
> 
> Yup.  Obviously during my initial attempt I was scratching my head
> wondering where these two files went--they were later found inside
> t/ directory which was really bad ;-)

Heh. Yeah, it's nice when the failure mode doesn't escape the trash
directory, but it's probably not worth worrying about too much.

> > In the "archive --remote" test I added, we may want to do the same to
> > show that "--output" points at the correct path.
> 
> Perhaps something like this.  By going down one level, we make sure
> that it is not sufficient to accidentally read from .git/config to
> find out what 'foo' is, and also ../b5-nick.tar that is relative to
> the prefix (aka 'a/') ends up at the top-level.

Yeah, your modification looks good.

-Peff

Re: [PATCH 1/3] rebase -i: highlight problems with core.commentchar

[Johannes Schindelin]

Hi Junio,

On Tue, 22 Nov 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Mon, 21 Nov 2016, Junio C Hamano wrote:
> >
> >> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> >> index 29e91d861c..c1f6411eb2 100755
> >> --- a/t/t0030-stripspace.sh
> >> +++ b/t/t0030-stripspace.sh
> >> @@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '
> >>  	test_cmp expect actual
> >>  '
> >>  
> >> +test_expect_failure '-c with comment char defined in .git/config' '
> >> +	test_config core.commentchar = &&
> >> +	printf "= foo\n" >expect &&
> >> +	printf "foo" | (
> >
> > Could I ask you to sneak in a \n here?
> 
> The one before that _is_ about fixing incomplete line, but this and
> the one before this one are not, so I think we should fix them at
> the same time.
> 
> But does it break anything to leave it as-is?  If not, I'd prefer to
> leave this (and one before this one) for a later clean-up patch post
> release.

Fair enough.

As a matter of fact, we do not even need to change it later. If it ain't
broke, don't fix it.

Ciao,
Dscho