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

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

[Junio C Hamano]

Johannes Schindelin  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 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 1/3] rebase -i: highlight problems with core.commentchar

[Junio C Hamano]

Junio C Hamano  writes:

> From: Johannes Schindelin 
>
> 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 
> Signed-off-by: Junio C Hamano 
> ---

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

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

[Junio C Hamano]

From: Johannes Schindelin 

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 
Signed-off-by: Junio C Hamano 
---
 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  and  specified as 
:/quuxery' '
test_when_finished "git branch -D torebase" &&
git checkout -b torebase branch1 &&
-- 
2.11.0-rc2-154-g95ba452916