Re: Re: [PATCH v4 1/2] push: Don't push a repository with unpushed submodules

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Aug 22, 2011, 19:47 UTC
Message-ID: <20110822194728.GA11745@sandbox-rc>
In-Reply-To: <7vmxf3xnsf.fsf@alter.siamese.dyndns.org>

Hi,

On Sat, Aug 20, 2011 at 11:48:48PM -0700, Junio C Hamano wrote:

Show 10 quoted lines

> After removing the change to combine-diff.c from your two-patch series, I
> applied them on top of this one, and queued the result in 'pu'.
> 
> While I tried to be careful while doing this callback-for-combine-diff
> patch so that a callback function written for two-way diff can be used
> without any change as long as it does not care about the LHS (i.e. "one")
> of the filepair, please double check. I didn't read your change to
> submodule.c very carefully (and I didn't have to change it).
> 
> The result seems to pass your new tests ;-).

Very nice. Today I had a deeper look into the current tests for on-demand and found a bug in them. Cleaning them up also revealed a bug in the current code. Junio could you please squash this[1] in the last patch (on-demand option).

I analysed the cause of this bug and it seems that we are not allowed to iterate revisions using init_revisions() and setup_revisions() more than once. I tracked this down to the SEEN flag in the struct object. Junio since you are one person listed in the api docs could you maybe quickly explain to me what this flag is used for?

I quickly tried to implement a reset_revision_walk function which will reset this flag but it seems that this breaks some expectations in the code since I got a segfault.

Cheers Heiko

[1]

diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 35820ec..b0e94f7 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -124,6 +124,10 @@ test_expect_success 'push unpushed submodules' '
 		cd work &&
 		git checkout master &&
 		git push --recurse-submodules=on-demand ../pub.git master
+		cd gar/bage &&
+		git rev-parse master >expected &&
+		git rev-parse origin/master >actual &&
+		test_cmp expected actual
 	)
 '
 
@@ -132,10 +136,14 @@ test_expect_success 'push unpushed submodules when not needed' '
 		cd work &&
 		(
 			cd gar/bage &&
-			>junk4 &&
-			git add junk4 &&
-			git commit -m "junk4" &&
-			git push
+			git checkout master &&
+			>junk5 &&
+			git add junk5 &&
+			git commit -m "junk5" &&
+			git push &&
+			git rev-parse master >expected &&
+			git rev-parse origin/master >actual &&
+			test_cmp expected actual
 		) &&
 		git add gar/bage &&
 		git commit -m "updated submodule" &&
@@ -143,4 +151,20 @@ test_expect_success 'push unpushed submodules when not needed' '
 	)
 '
 
+test_expect_failure 'push unpushed submodules on-demand fails when submodule not pushable' '
+	(
+		cd work &&
+		(
+			cd gar/bage &&
+			git checkout HEAD~0 &&
+			>junk6 &&
+			git add junk6 &&
+			git commit -m "junk6"
+		) &&
+		git add gar/bage &&
+		git commit -m "updated submodule" &&
+		test_must_fail git push --recurse-submodules=on-demand ../pub.git master
+	)
+'
+
 test_done

-- 
1.7.6.46.g0f058