[PATCH 0/2] Fix issue with format-patch and diff.submodule

[PATCH 0/2] Fix issue with format-patch and diff.submodule

[Doug Kelly]

A colleague found an issue that when using diff.submodule=log in his
.gitconfig, format-patch would use the log format for submodule changes,
which would be ignored or error out when processed by git-am.
format-patch now ignores the diff.submodule option and a testcase for
this specific issue now exists.

Since this seems like a bug in current versions, I have based and
tested this on the "maint" branch, but there's no reason it shouldn't
apply cleanly to master as well.

Apologies for any rawness to the first round of this change.
"Long time listener; first time caller." Any feedback is appreciated.

Doug Kelly (2):
  t4255: test am submodule with diff.submodule
  format-patch: ignore diff.submodule setting

 builtin/log.c           |  2 +-
 t/t4255-am-submodule.sh | 83 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+), 1 deletion(-)

-- 
2.0.5

[PATCH 1/2] t4255: test am submodule with diff.submodule

[Doug Kelly]

git am will break when using diff.submodule=log; add some test cases
to illustrate this breakage as simply as possible.  There are
currently two ways this can fail:

* With errors ("unrecognized input"), if only change
* Silently (no submodule change), if other files change

Test for both conditions and ensure without diff.submodule this works.

Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
---
 t/t4255-am-submodule.sh | 83 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 8bde7db..d9a1d79 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -18,4 +18,87 @@ am_3way () {
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
 test_submodule_switch "am_3way"
 
+test_expect_success 'setup diff.submodule' '
+	echo one >one &&
+	git add one &&
+	test_tick &&
+	git commit -m initial &&
+	git rev-parse HEAD >initial &&
+
+	git init submodule &&
+	(cd submodule &&
+		echo two >two &&
+		git add two &&
+		test_tick &&
+		git commit -m "initial submodule" &&
+		git rev-parse HEAD >../initial-submodule) &&
+	git submodule add ./submodule &&
+	test_tick &&
+	git commit -m first &&
+	git rev-parse HEAD >first &&
+
+	(cd submodule &&
+		echo three >three &&
+		git add three &&
+		test_tick &&
+		git commit -m "first submodule" &&
+		git rev-parse HEAD >../first-submodule) &&
+	git add submodule &&
+	test_tick &&
+	git commit -m second &&
+	git rev-parse HEAD >second &&
+
+	(cd submodule &&
+		git mv two four &&
+		test_tick &&
+		git commit -m "second submodule" &&
+		git rev-parse HEAD >../second-submodule) &&
+	git add submodule &&
+	echo four >four &&
+	git add four &&
+	test_tick &&
+	git commit -m third &&
+	git rev-parse HEAD >third &&
+	git submodule update --init
+'
+
+INITIAL=$(cat initial)
+SECOND=$(cat second)
+THIRD=$(cat third)
+
+run_test() {
+	START_COMMIT=$1
+	EXPECT=$2
+	(git am --abort || true) &&
+	git reset --hard $START_COMMIT &&
+	rm -f *.patch &&
+	git format-patch -1 &&
+	git reset --hard $START_COMMIT^ &&
+	git submodule update &&
+	git am *.patch &&
+	git submodule update &&
+	(cd submodule && git rev-parse HEAD >../actual) &&
+	test_cmp $EXPECT actual
+}
+
+test_expect_success 'diff.submodule unset' '
+	(git config --unset diff.submodule || true) &&
+	run_test $SECOND 'first-submodule'
+'
+
+test_expect_success 'diff.submodule unset with extra file' '
+	(git config --unset diff.submodule || true) &&
+	run_test $THIRD 'second-submodule'
+'
+
+test_expect_success 'diff.submodule=log' '
+	git config diff.submodule log &&
+	run_test $SECOND 'first-submodule'
+'
+
+test_expect_success 'diff.submodule=log with extra file' '
+	git config diff.submodule log &&
+	run_test $THIRD 'second-submodule'
+'
+
 test_done
-- 
2.0.5

[PATCH 2/2] format-patch: ignore diff.submodule setting

[Doug Kelly]

diff.submodule when set to log produces output which git-am cannot
handle. Ignore this setting when generating patch output.

Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
---
 builtin/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 734aab3..cb14db4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -705,7 +705,7 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff") ||
-	    !strcmp(var, "color.ui")) {
+	    !strcmp(var, "color.ui") || !strcmp(var, "diff.submodule")) {
 		return 0;
 	}
 	if (!strcmp(var, "format.numbered")) {
-- 
2.0.5

Re: [PATCH 1/2] t4255: test am submodule with diff.submodule

[Eric Sunshine]

On Fri, Dec 26, 2014 at 6:11 PM, Doug Kelly <dougk.ff7@gmail.com> wrote:
> git am will break when using diff.submodule=log; add some test cases
> to illustrate this breakage as simply as possible.  There are
> currently two ways this can fail:
>
> * With errors ("unrecognized input"), if only change
> * Silently (no submodule change), if other files change
>
> Test for both conditions and ensure without diff.submodule this works.
>
> Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
> ---
> diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
> index 8bde7db..d9a1d79 100755
> --- a/t/t4255-am-submodule.sh
> +++ b/t/t4255-am-submodule.sh
> @@ -18,4 +18,87 @@ am_3way () {
>  KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
>  test_submodule_switch "am_3way"
>
> +test_expect_success 'setup diff.submodule' '

Since the tests are actually expected to fail at this point (before
you've fixed the problem), use test_expect_failure. The follow-up
patch, which fixes the problem, should flip them to
test_expect_success.

> +       echo one >one &&
> +       git add one &&
> +       test_tick &&
> +       git commit -m initial &&

Rather than performing these steps manually (here and below), perhaps
test_commit would be suitable and more succinct.

> +       git rev-parse HEAD >initial &&

Other scripts in the test suite don't bother with this indirection.
Instead, they assign the variable here, then reference it in
subsequent tests (and no need to redirect to a file).

    INITIAL=$(git rev-parse HEAD) &&

> +
> +       git init submodule &&
> +       (cd submodule &&
> +               echo two >two &&
> +               git add two &&
> +               test_tick &&
> +               git commit -m "initial submodule" &&
> +               git rev-parse HEAD >../initial-submodule) &&

Style: Format the subshell like this:

    (
        ...commands...
    ) &&

> +       git submodule add ./submodule &&
> +       test_tick &&
> +       git commit -m first &&
> +       git rev-parse HEAD >first &&

Is file 'first' ever used anywhere?

> +       (cd submodule &&
> +               echo three >three &&
> +               git add three &&
> +               test_tick &&
> +               git commit -m "first submodule" &&
> +               git rev-parse HEAD >../first-submodule) &&
> +       git add submodule &&
> +       test_tick &&
> +       git commit -m second &&
> +       git rev-parse HEAD >second &&
> +
> +       (cd submodule &&
> +               git mv two four &&
> +               test_tick &&
> +               git commit -m "second submodule" &&
> +               git rev-parse HEAD >../second-submodule) &&
> +       git add submodule &&
> +       echo four >four &&
> +       git add four &&
> +       test_tick &&
> +       git commit -m third &&
> +       git rev-parse HEAD >third &&
> +       git submodule update --init
> +'
> +
> +INITIAL=$(cat initial)
> +SECOND=$(cat second)
> +THIRD=$(cat third)

No need for this extra level of indirection. See above.

> +run_test() {
> +       START_COMMIT=$1
> +       EXPECT=$2

Although it's not specifically wrong here, someone adding code above
these two lines later on may not notice the broken &&-chain, so it
would be a good idea to keep the &&-chain intact.

> +       (git am --abort || true) &&
> +       git reset --hard $START_COMMIT &&
> +       rm -f *.patch &&
> +       git format-patch -1 &&
> +       git reset --hard $START_COMMIT^ &&
> +       git submodule update &&
> +       git am *.patch &&
> +       git submodule update &&
> +       (cd submodule && git rev-parse HEAD >../actual) &&
> +       test_cmp $EXPECT actual
> +}
> +
> +test_expect_success 'diff.submodule unset' '
> +       (git config --unset diff.submodule || true) &&
> +       run_test $SECOND 'first-submodule'

Note that you're already inside a single-quoted string here, so
'first-submodule' is not quite doing what you expect. Double quotes
would be more appropriate. Or, better, drop the quoting of
first-submodule altogether since it's unnecessary.

> +'
> +
> +test_expect_success 'diff.submodule unset with extra file' '
> +       (git config --unset diff.submodule || true) &&
> +       run_test $THIRD 'second-submodule'
> +'
> +
> +test_expect_success 'diff.submodule=log' '
> +       git config diff.submodule log &&
> +       run_test $SECOND 'first-submodule'
> +'
> +
> +test_expect_success 'diff.submodule=log with extra file' '
> +       git config diff.submodule log &&
> +       run_test $THIRD 'second-submodule'
> +'
> +
>  test_done
> --
> 2.0.5

Re: [PATCH 1/2] t4255: test am submodule with diff.submodule

[Doug Kelly]

On Sat, Dec 27, 2014 at 6:37 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Dec 26, 2014 at 6:11 PM, Doug Kelly <dougk.ff7@gmail.com> wrote:
> > git am will break when using diff.submodule=log; add some test cases
> > to illustrate this breakage as simply as possible.  There are
> > currently two ways this can fail:
> >
> > * With errors ("unrecognized input"), if only change
> > * Silently (no submodule change), if other files change
> >
> > Test for both conditions and ensure without diff.submodule this works.
> >
> > Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
> > ---
> > diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
> > index 8bde7db..d9a1d79 100755
> > --- a/t/t4255-am-submodule.sh
> > +++ b/t/t4255-am-submodule.sh
> > @@ -18,4 +18,87 @@ am_3way () {
> >  KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
> >  test_submodule_switch "am_3way"
> >
> > +test_expect_success 'setup diff.submodule' '
>
> Since the tests are actually expected to fail at this point (before
> you've fixed the problem), use test_expect_failure. The follow-up
> patch, which fixes the problem, should flip them to
> test_expect_success.

Okay, that's easy enough to do.

>
> > +       echo one >one &&
> > +       git add one &&
> > +       test_tick &&
> > +       git commit -m initial &&
>
> Rather than performing these steps manually (here and below), perhaps
> test_commit would be suitable and more succinct.
>
> > +       git rev-parse HEAD >initial &&
>
> Other scripts in the test suite don't bother with this indirection.
> Instead, they assign the variable here, then reference it in
> subsequent tests (and no need to redirect to a file).
>
>     INITIAL=$(git rev-parse HEAD) &&
>

Both good comments; wasn't sure if that'd work.  But easy to do!

> > +
> > +       git init submodule &&
> > +       (cd submodule &&
> > +               echo two >two &&
> > +               git add two &&
> > +               test_tick &&
> > +               git commit -m "initial submodule" &&
> > +               git rev-parse HEAD >../initial-submodule) &&
>
> Style: Format the subshell like this:
>
>     (
>         ...commands...
>     ) &&

Yeah, I was unsure on this style (docs were unclear), but that's easy to follow.

>
> > +       git submodule add ./submodule &&
> > +       test_tick &&
> > +       git commit -m first &&
> > +       git rev-parse HEAD >first &&
>
> Is file 'first' ever used anywhere?

Nope; somewhat vestigial code (that could probably be cleaned out --
and should).
Originally, I was testing the first commit, and then I realized that
the second commit
updating the submodule introduces the failure I was looking for.

>
> > +       (cd submodule &&
> > +               echo three >three &&
> > +               git add three &&
> > +               test_tick &&
> > +               git commit -m "first submodule" &&
> > +               git rev-parse HEAD >../first-submodule) &&
> > +       git add submodule &&
> > +       test_tick &&
> > +       git commit -m second &&
> > +       git rev-parse HEAD >second &&
> > +
> > +       (cd submodule &&
> > +               git mv two four &&
> > +               test_tick &&
> > +               git commit -m "second submodule" &&
> > +               git rev-parse HEAD >../second-submodule) &&
> > +       git add submodule &&
> > +       echo four >four &&
> > +       git add four &&
> > +       test_tick &&
> > +       git commit -m third &&
> > +       git rev-parse HEAD >third &&
> > +       git submodule update --init
> > +'
> > +
> > +INITIAL=$(cat initial)
> > +SECOND=$(cat second)
> > +THIRD=$(cat third)
>
> No need for this extra level of indirection. See above.
>
> > +run_test() {
> > +       START_COMMIT=$1
> > +       EXPECT=$2
>
> Although it's not specifically wrong here, someone adding code above
> these two lines later on may not notice the broken &&-chain, so it
> would be a good idea to keep the &&-chain intact.

OK, easy enough.

>
> > +       (git am --abort || true) &&
> > +       git reset --hard $START_COMMIT &&
> > +       rm -f *.patch &&
> > +       git format-patch -1 &&
> > +       git reset --hard $START_COMMIT^ &&
> > +       git submodule update &&
> > +       git am *.patch &&
> > +       git submodule update &&
> > +       (cd submodule && git rev-parse HEAD >../actual) &&
> > +       test_cmp $EXPECT actual
> > +}
> > +
> > +test_expect_success 'diff.submodule unset' '
> > +       (git config --unset diff.submodule || true) &&
> > +       run_test $SECOND 'first-submodule'
>
> Note that you're already inside a single-quoted string here, so
> 'first-submodule' is not quite doing what you expect. Double quotes
> would be more appropriate. Or, better, drop the quoting of
> first-submodule altogether since it's unnecessary.

Yep. Good note.

>
> > +'
> > +
> > +test_expect_success 'diff.submodule unset with extra file' '
> > +       (git config --unset diff.submodule || true) &&
> > +       run_test $THIRD 'second-submodule'
> > +'
> > +
> > +test_expect_success 'diff.submodule=log' '
> > +       git config diff.submodule log &&
> > +       run_test $SECOND 'first-submodule'
> > +'
> > +
> > +test_expect_success 'diff.submodule=log with extra file' '
> > +       git config diff.submodule log &&
> > +       run_test $THIRD 'second-submodule'
> > +'
> > +
> >  test_done
> > --
> > 2.0.5

One other note that might simplify this extra test case that I thought about
while driving home yesterday evening was changing lib-submodule-update to
set diff.submodule=log inside prolog().  This wouldn't provide very
clear failure causes, but it would perhaps reduce duplicated code and simplify
the test case.

Thanks for the feedback, though, I'll send out a new version momentarily.

[PATCH v2 1/2] t4255: test am submodule with diff.submodule

[Doug Kelly]

git am will break when using diff.submodule=log; add some test cases
to illustrate this breakage as simply as possible.  There are
currently two ways this can fail:

* With errors ("unrecognized input"), if only change
* Silently (no submodule change), if other files change

Test for both conditions and ensure without diff.submodule this works.

Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
---
 t/t4255-am-submodule.sh | 84 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 8bde7db..a2dc083 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -18,4 +18,88 @@ am_3way () {
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
 test_submodule_switch "am_3way"
 
+test_expect_success 'setup diff.submodule' '
+	echo one >one &&
+	git add one &&
+	test_tick &&
+	git commit -m initial &&
+	INITIAL=$(git rev-parse HEAD) &&
+
+	git init submodule &&
+	(
+		cd submodule &&
+		echo two >two &&
+		git add two &&
+		test_tick &&
+		git commit -m "initial submodule" &&
+		git rev-parse HEAD >../initial-submodule
+	) &&
+	git submodule add ./submodule &&
+	test_tick &&
+	git commit -m first &&
+
+	(
+		cd submodule &&
+		echo three >three &&
+		git add three &&
+		test_tick &&
+		git commit -m "first submodule" &&
+		git rev-parse HEAD >../first-submodule
+	) &&
+	git add submodule &&
+	test_tick &&
+	git commit -m second &&
+	SECOND=$(git rev-parse HEAD) &&
+
+	(
+		cd submodule &&
+		git mv two four &&
+		test_tick &&
+		git commit -m "second submodule" &&
+		git rev-parse HEAD >../second-submodule
+	) &&
+	git add submodule &&
+	echo four >four &&
+	git add four &&
+	test_tick &&
+	git commit -m third &&
+	THIRD=$(git rev-parse HEAD) &&
+	git submodule update --init
+'
+
+run_test() {
+	START_COMMIT=$1 &&
+	EXPECT=$2 &&
+	(git am --abort || true) &&
+	git reset --hard $START_COMMIT &&
+	rm -f *.patch &&
+	git format-patch -1 &&
+	git reset --hard $START_COMMIT^ &&
+	git submodule update &&
+	git am *.patch &&
+	git submodule update &&
+	(cd submodule && git rev-parse HEAD >../actual) &&
+	test_cmp $EXPECT actual
+}
+
+test_expect_success 'diff.submodule unset' '
+	(git config --unset diff.submodule || true) &&
+	run_test $SECOND first-submodule
+'
+
+test_expect_success 'diff.submodule unset with extra file' '
+	(git config --unset diff.submodule || true) &&
+	run_test $THIRD second-submodule
+'
+
+test_expect_failure 'diff.submodule=log' '
+	git config diff.submodule log &&
+	run_test $SECOND first-submodule
+'
+
+test_expect_failure 'diff.submodule=log with extra file' '
+	git config diff.submodule log &&
+	run_test $THIRD second-submodule
+'
+
 test_done
-- 
2.0.5

[PATCH v2 2/2] format-patch: ignore diff.submodule setting

[Doug Kelly]

diff.submodule when set to log produces output which git-am cannot
handle. Ignore this setting when generating patch output.

Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
---
 builtin/log.c           | 2 +-
 t/t4255-am-submodule.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 734aab3..cb14db4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -705,7 +705,7 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff") ||
-	    !strcmp(var, "color.ui")) {
+	    !strcmp(var, "color.ui") || !strcmp(var, "diff.submodule")) {
 		return 0;
 	}
 	if (!strcmp(var, "format.numbered")) {
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index a2dc083..b7ec0f1 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -92,12 +92,12 @@ test_expect_success 'diff.submodule unset with extra file' '
 	run_test $THIRD second-submodule
 '
 
-test_expect_failure 'diff.submodule=log' '
+test_expect_success 'diff.submodule=log' '
 	git config diff.submodule log &&
 	run_test $SECOND first-submodule
 '
 
-test_expect_failure 'diff.submodule=log with extra file' '
+test_expect_success 'diff.submodule=log with extra file' '
 	git config diff.submodule log &&
 	run_test $THIRD second-submodule
 '
-- 
2.0.5

Re: [PATCH v2 1/2] t4255: test am submodule with diff.submodule

[Eric Sunshine]

On Sat, Dec 27, 2014 at 07:04:23PM -0600, Doug Kelly wrote:
> git am will break when using diff.submodule=log; add some test cases
> to illustrate this breakage as simply as possible.  There are
> currently two ways this can fail:
> 
> * With errors ("unrecognized input"), if only change
> * Silently (no submodule change), if other files change
> 
> Test for both conditions and ensure without diff.submodule this works.
> 
> Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
> ---

Here below the "---" line is a good place to explain what changed
since the last version of the patch (or do so in the cover letter of
the new patch series). It's also helpful to reviewers to provide a
link to the previous round, like this[1].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/261830

More below.

>  t/t4255-am-submodule.sh | 84 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
> index 8bde7db..a2dc083 100755
> --- a/t/t4255-am-submodule.sh
> +++ b/t/t4255-am-submodule.sh
> @@ -18,4 +18,88 @@ am_3way () {
>  KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
>  test_submodule_switch "am_3way"
>  
> +test_expect_success 'setup diff.submodule' '
> +	echo one >one &&
> +	git add one &&
> +	test_tick &&
> +	git commit -m initial &&

Perhaps squash in the following to improve succinctness?

-- >8 --
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index b7ec0f1..b58e776 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -19,50 +19,37 @@ KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
 test_submodule_switch "am_3way"
 
 test_expect_success 'setup diff.submodule' '
-	echo one >one &&
-	git add one &&
-	test_tick &&
-	git commit -m initial &&
+	test_commit one &&
 	INITIAL=$(git rev-parse HEAD) &&
 
 	git init submodule &&
 	(
 		cd submodule &&
-		echo two >two &&
-		git add two &&
-		test_tick &&
-		git commit -m "initial submodule" &&
+		test_commit two &&
 		git rev-parse HEAD >../initial-submodule
 	) &&
 	git submodule add ./submodule &&
-	test_tick &&
 	git commit -m first &&
 
 	(
 		cd submodule &&
-		echo three >three &&
-		git add three &&
-		test_tick &&
-		git commit -m "first submodule" &&
+		test_commit three &&
 		git rev-parse HEAD >../first-submodule
 	) &&
 	git add submodule &&
-	test_tick &&
 	git commit -m second &&
 	SECOND=$(git rev-parse HEAD) &&
 
 	(
 		cd submodule &&
-		git mv two four &&
+		git mv two.t four.t &&
 		test_tick &&
 		git commit -m "second submodule" &&
 		git rev-parse HEAD >../second-submodule
 	) &&
+	test_commit four &&
 	git add submodule &&
-	echo four >four &&
-	git add four &&
-	test_tick &&
-	git commit -m third &&
+	git commit --amend --no-edit &&
 	THIRD=$(git rev-parse HEAD) &&
 	git submodule update --init
 '
-- 
2.2.1.302.gdfcd89f

Re: [PATCH 1/2] t4255: test am submodule with diff.submodule

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +       (git am --abort || true) &&

Why (x || y)?  Is 'x' so unreliable that we do not know how should exit?
Should this be "test_must_fail git am --abort"?

>> +       (cd submodule && git rev-parse HEAD >../actual) &&

"git -C submodule rev-parse HEAD >actual" perhaps?

>> +test_expect_success 'diff.submodule unset' '
>> +       (git config --unset diff.submodule || true) &&

I think test_config and test_unconfig were invented for things like
this (same for all the other use of "git config").

[PATCH v3 1/2] t4255: test am submodule with diff.submodule

[Doug Kelly]

git am will break when using diff.submodule=log; add some test cases
to illustrate this breakage as simply as possible.  There are
currently two ways this can fail:

* With errors ("unrecognized input"), if only change
* Silently (no submodule change), if other files change

Test for both conditions and ensure without diff.submodule this works.

Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
Thanks-to: Eric Sunshine <sunshine@sunshineco.com>
Thanks-to: Junio C Hamano <gitster@pobox.com>
---
Updated with Eric Sunshine's comments and changes to reduce complexity,
and also changed to include Junio's suggestions for using test_config,
test_unconfig, and test_might_fail (since we don't know if a previous
am failed or not -- we always want to clean up first).

 t/t4255-am-submodule.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 8bde7db..523accf 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -18,4 +18,76 @@ am_3way () {
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
 test_submodule_switch "am_3way"
 
+test_expect_success 'setup diff.submodule' '
+	test_commit one &&
+	INITIAL=$(git rev-parse HEAD) &&
+
+	git init submodule &&
+	(
+		cd submodule &&
+		test_commit two &&
+		git rev-parse HEAD >../initial-submodule
+	) &&
+	git submodule add ./submodule &&
+	git commit -m first &&
+
+	(
+		cd submodule &&
+		test_commit three &&
+		git rev-parse HEAD >../first-submodule
+	) &&
+	git add submodule &&
+	test_tick &&
+	git commit -m second &&
+	SECOND=$(git rev-parse HEAD) &&
+
+	(
+		cd submodule &&
+		git mv two.t four.t &&
+		test_tick &&
+		git commit -m "second submodule" &&
+		git rev-parse HEAD >../second-submodule
+	) &&
+	test_commit four &&
+	git add submodule &&
+	git commit --amend --no-edit &&
+	THIRD=$(git rev-parse HEAD) &&
+	git submodule update --init
+'
+
+run_test() {
+	START_COMMIT=$1 &&
+	EXPECT=$2 &&
+	test_might_fail git am --abort &&
+	git reset --hard $START_COMMIT &&
+	rm -f *.patch &&
+	git format-patch -1 &&
+	git reset --hard $START_COMMIT^ &&
+	git submodule update &&
+	git am *.patch &&
+	git submodule update &&
+	git -C submodule rev-parse HEAD >actual &&
+	test_cmp $EXPECT actual
+}
+
+test_expect_success 'diff.submodule unset' '
+	test_unconfig diff.submodule &&
+	run_test $SECOND first-submodule
+'
+
+test_expect_success 'diff.submodule unset with extra file' '
+	test_unconfig diff.submodule &&
+	run_test $THIRD second-submodule
+'
+
+test_expect_failure 'diff.submodule=log' '
+	test_config diff.submodule log &&
+	run_test $SECOND first-submodule
+'
+
+test_expect_failure 'diff.submodule=log with extra file' '
+	test_config diff.submodule log &&
+	run_test $THIRD second-submodule
+'
+
 test_done
-- 
2.0.5

[PATCH v3 2/2] format-patch: ignore diff.submodule setting

[Doug Kelly]

diff.submodule when set to log produces output which git-am cannot
handle. Ignore this setting when generating patch output.

Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
---
 builtin/log.c           | 2 +-
 t/t4255-am-submodule.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 734aab3..cb14db4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -705,7 +705,7 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff") ||
-	    !strcmp(var, "color.ui")) {
+	    !strcmp(var, "color.ui") || !strcmp(var, "diff.submodule")) {
 		return 0;
 	}
 	if (!strcmp(var, "format.numbered")) {
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 523accf..31cbdba 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -80,12 +80,12 @@ test_expect_success 'diff.submodule unset with extra file' '
 	run_test $THIRD second-submodule
 '
 
-test_expect_failure 'diff.submodule=log' '
+test_expect_success 'diff.submodule=log' '
 	test_config diff.submodule log &&
 	run_test $SECOND first-submodule
 '
 
-test_expect_failure 'diff.submodule=log with extra file' '
+test_expect_success 'diff.submodule=log with extra file' '
 	test_config diff.submodule log &&
 	run_test $THIRD second-submodule
 '
-- 
2.0.5

Re: [PATCH 1/2] t4255: test am submodule with diff.submodule

[Doug Kelly]

On Mon, Dec 29, 2014 at 9:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>> +       (git am --abort || true) &&
>
> Why (x || y)?  Is 'x' so unreliable that we do not know how should exit?
> Should this be "test_must_fail git am --abort"?
>
Updated to test_might_fail -- we don't know if a merge is in progress or not.
We still need to clean up, but disregard failure if a merge isn't in progress.

>>> +       (cd submodule && git rev-parse HEAD >../actual) &&
>
> "git -C submodule rev-parse HEAD >actual" perhaps?
>
Seems sane to me.

>>> +test_expect_success 'diff.submodule unset' '
>>> +       (git config --unset diff.submodule || true) &&
>
> I think test_config and test_unconfig were invented for things like
> this (same for all the other use of "git config").
Yep, much nicer. :) Also updated to test_commit as suggested by Eric.

Thanks!

--Doug

Re: [PATCH v3 1/2] t4255: test am submodule with diff.submodule

[Eric Sunshine]

On Wed, Jan 7, 2015 at 2:31 PM, Doug Kelly <dougk.ff7@gmail.com> wrote:
> git am will break when using diff.submodule=log; add some test cases
> to illustrate this breakage as simply as possible.  There are
> currently two ways this can fail:
>
> * With errors ("unrecognized input"), if only change
> * Silently (no submodule change), if other files change
>
> Test for both conditions and ensure without diff.submodule this works.
>
> Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
> Thanks-to: Eric Sunshine <sunshine@sunshineco.com>
> Thanks-to: Junio C Hamano <gitster@pobox.com>

On this project, it's customary to say "Helped-by:" rather than
"Thanks-to:". Also, place your sign-off last.

> ---
> Updated with Eric Sunshine's comments and changes to reduce complexity,
> and also changed to include Junio's suggestions for using test_config,
> test_unconfig, and test_might_fail (since we don't know if a previous
> am failed or not -- we always want to clean up first).

Looking much better. Thanks. A couple minor comments below...

> diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
> index 8bde7db..523accf 100755
> --- a/t/t4255-am-submodule.sh
> +++ b/t/t4255-am-submodule.sh
> @@ -18,4 +18,76 @@ am_3way () {
>  KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
>  test_submodule_switch "am_3way"
>
> +test_expect_success 'setup diff.submodule' '
> +       test_commit one &&
> +       INITIAL=$(git rev-parse HEAD) &&
> +
> +       git init submodule &&
> +       (
> +               cd submodule &&
> +               test_commit two &&
> +               git rev-parse HEAD >../initial-submodule
> +       ) &&
> +       git submodule add ./submodule &&
> +       git commit -m first &&
> +
> +       (
> +               cd submodule &&
> +               test_commit three &&
> +               git rev-parse HEAD >../first-submodule
> +       ) &&
> +       git add submodule &&
> +       test_tick &&

You can drop this test_tick (as I did in my "squash"[1]).

> +       git commit -m second &&
> +       SECOND=$(git rev-parse HEAD) &&
> +
> +       (
> +               cd submodule &&
> +               git mv two.t four.t &&
> +               test_tick &&

And this one (which I overlooked in [1]).

The reason I suggest dropping the test_tick invocations is that they
do not impact these tests at all, yet their presence misleads the
reader into thinking that they are somehow significant.

> +               git commit -m "second submodule" &&
> +               git rev-parse HEAD >../second-submodule
> +       ) &&
> +       test_commit four &&
> +       git add submodule &&
> +       git commit --amend --no-edit &&
> +       THIRD=$(git rev-parse HEAD) &&
> +       git submodule update --init
> +'

[1]: http://article.gmane.org/gmane.comp.version-control.git/261852

[PATCH v4 1/2] t4255: test am submodule with diff.submodule

[Doug Kelly]

git am will break when using diff.submodule=log; add some test cases
to illustrate this breakage as simply as possible.  There are
currently two ways this can fail:

* With errors ("unrecognized input"), if only change
* Silently (no submodule change), if other files change

Test for both conditions and ensure without diff.submodule this works.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
---
Updated to remove test_ticks and clean the commit message.

 t/t4255-am-submodule.sh | 70 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 8bde7db..a38c305 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -18,4 +18,74 @@ am_3way () {
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
 test_submodule_switch "am_3way"
 
+test_expect_success 'setup diff.submodule' '
+	test_commit one &&
+	INITIAL=$(git rev-parse HEAD) &&
+
+	git init submodule &&
+	(
+		cd submodule &&
+		test_commit two &&
+		git rev-parse HEAD >../initial-submodule
+	) &&
+	git submodule add ./submodule &&
+	git commit -m first &&
+
+	(
+		cd submodule &&
+		test_commit three &&
+		git rev-parse HEAD >../first-submodule
+	) &&
+	git add submodule &&
+	git commit -m second &&
+	SECOND=$(git rev-parse HEAD) &&
+
+	(
+		cd submodule &&
+		git mv two.t four.t &&
+		git commit -m "second submodule" &&
+		git rev-parse HEAD >../second-submodule
+	) &&
+	test_commit four &&
+	git add submodule &&
+	git commit --amend --no-edit &&
+	THIRD=$(git rev-parse HEAD) &&
+	git submodule update --init
+'
+
+run_test() {
+	START_COMMIT=$1 &&
+	EXPECT=$2 &&
+	test_might_fail git am --abort &&
+	git reset --hard $START_COMMIT &&
+	rm -f *.patch &&
+	git format-patch -1 &&
+	git reset --hard $START_COMMIT^ &&
+	git submodule update &&
+	git am *.patch &&
+	git submodule update &&
+	git -C submodule rev-parse HEAD >actual &&
+	test_cmp $EXPECT actual
+}
+
+test_expect_success 'diff.submodule unset' '
+	test_unconfig diff.submodule &&
+	run_test $SECOND first-submodule
+'
+
+test_expect_success 'diff.submodule unset with extra file' '
+	test_unconfig diff.submodule &&
+	run_test $THIRD second-submodule
+'
+
+test_expect_failure 'diff.submodule=log' '
+	test_config diff.submodule log &&
+	run_test $SECOND first-submodule
+'
+
+test_expect_failure 'diff.submodule=log with extra file' '
+	test_config diff.submodule log &&
+	run_test $THIRD second-submodule
+'
+
 test_done
-- 
2.0.5

[PATCH v4 2/2] format-patch: ignore diff.submodule setting

[Doug Kelly]

diff.submodule when set to log produces output which git-am cannot
handle. Ignore this setting when generating patch output.

Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
---
 builtin/log.c           | 2 +-
 t/t4255-am-submodule.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 734aab3..cb14db4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -705,7 +705,7 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff") ||
-	    !strcmp(var, "color.ui")) {
+	    !strcmp(var, "color.ui") || !strcmp(var, "diff.submodule")) {
 		return 0;
 	}
 	if (!strcmp(var, "format.numbered")) {
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index a38c305..27ea698 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -78,12 +78,12 @@ test_expect_success 'diff.submodule unset with extra file' '
 	run_test $THIRD second-submodule
 '
 
-test_expect_failure 'diff.submodule=log' '
+test_expect_success 'diff.submodule=log' '
 	test_config diff.submodule log &&
 	run_test $SECOND first-submodule
 '
 
-test_expect_failure 'diff.submodule=log with extra file' '
+test_expect_success 'diff.submodule=log with extra file' '
 	test_config diff.submodule log &&
 	run_test $THIRD second-submodule
 '
-- 
2.0.5

Re: [PATCH 1/2] t4255: test am submodule with diff.submodule

[Junio C Hamano]

Doug Kelly <dougk.ff7@gmail.com> writes:

> On Mon, Dec 29, 2014 at 9:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>>> +       (git am --abort || true) &&
>>
>> Why (x || y)?  Is 'x' so unreliable that we do not know how should exit?
>> Should this be "test_must_fail git am --abort"?
>>
> Updated to test_might_fail -- we don't know if a merge is in progress or not.
> We still need to clean up, but disregard failure if a merge isn't in progress.

Ah, OK.  But even with "test_might_fail", it may not be clear why it
might fail, so it would be easier to maintain if we can read "we
don't know if a merge is in progress" next to the "test_might_fail".

For now we can add a comment, but in the longer term it might not be
a bad idea to change test_might_fail to require two args, one is a
command to run and the other is a text that explains why the outcome
is unknown.

Thanks for clarifying.

[PATCH v5 1/2] t4255: test am submodule with diff.submodule

[Doug Kelly]

git am will break when using diff.submodule=log; add some test cases
to illustrate this breakage as simply as possible.  There are
currently two ways this can fail:

* With errors ("unrecognized input"), if only change
* Silently (no submodule change), if other files change

Test for both conditions and ensure without diff.submodule this works.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
---
Added a comment for why test_might_fail is used to abort merges
in progress.

 t/t4255-am-submodule.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 8bde7db..450d261 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -18,4 +18,76 @@ am_3way () {
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
 test_submodule_switch "am_3way"
 
+test_expect_success 'setup diff.submodule' '
+	test_commit one &&
+	INITIAL=$(git rev-parse HEAD) &&
+
+	git init submodule &&
+	(
+		cd submodule &&
+		test_commit two &&
+		git rev-parse HEAD >../initial-submodule
+	) &&
+	git submodule add ./submodule &&
+	git commit -m first &&
+
+	(
+		cd submodule &&
+		test_commit three &&
+		git rev-parse HEAD >../first-submodule
+	) &&
+	git add submodule &&
+	git commit -m second &&
+	SECOND=$(git rev-parse HEAD) &&
+
+	(
+		cd submodule &&
+		git mv two.t four.t &&
+		git commit -m "second submodule" &&
+		git rev-parse HEAD >../second-submodule
+	) &&
+	test_commit four &&
+	git add submodule &&
+	git commit --amend --no-edit &&
+	THIRD=$(git rev-parse HEAD) &&
+	git submodule update --init
+'
+
+run_test() {
+	START_COMMIT=$1 &&
+	EXPECT=$2 &&
+	# Abort any merges in progress: the previous
+	# test may have failed, and we should clean up.
+	test_might_fail git am --abort &&
+	git reset --hard $START_COMMIT &&
+	rm -f *.patch &&
+	git format-patch -1 &&
+	git reset --hard $START_COMMIT^ &&
+	git submodule update &&
+	git am *.patch &&
+	git submodule update &&
+	git -C submodule rev-parse HEAD >actual &&
+	test_cmp $EXPECT actual
+}
+
+test_expect_success 'diff.submodule unset' '
+	test_unconfig diff.submodule &&
+	run_test $SECOND first-submodule
+'
+
+test_expect_success 'diff.submodule unset with extra file' '
+	test_unconfig diff.submodule &&
+	run_test $THIRD second-submodule
+'
+
+test_expect_failure 'diff.submodule=log' '
+	test_config diff.submodule log &&
+	run_test $SECOND first-submodule
+'
+
+test_expect_failure 'diff.submodule=log with extra file' '
+	test_config diff.submodule log &&
+	run_test $THIRD second-submodule
+'
+
 test_done
-- 
2.0.5

[PATCH v5 2/2] format-patch: ignore diff.submodule setting

[Doug Kelly]

diff.submodule when set to log produces output which git-am cannot
handle. Ignore this setting when generating patch output.

Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
---
 builtin/log.c           | 2 +-
 t/t4255-am-submodule.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 734aab3..cb14db4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -705,7 +705,7 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff") ||
-	    !strcmp(var, "color.ui")) {
+	    !strcmp(var, "color.ui") || !strcmp(var, "diff.submodule")) {
 		return 0;
 	}
 	if (!strcmp(var, "format.numbered")) {
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 450d261..0ba8194 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -80,12 +80,12 @@ test_expect_success 'diff.submodule unset with extra file' '
 	run_test $THIRD second-submodule
 '
 
-test_expect_failure 'diff.submodule=log' '
+test_expect_success 'diff.submodule=log' '
 	test_config diff.submodule log &&
 	run_test $SECOND first-submodule
 '
 
-test_expect_failure 'diff.submodule=log with extra file' '
+test_expect_success 'diff.submodule=log with extra file' '
 	test_config diff.submodule log &&
 	run_test $THIRD second-submodule
 '
-- 
2.0.5