[PATCH] Add a test checking if send-pack updated local tracking branches correctly

[PATCH] Add a test checking if send-pack updated local tracking branches correctly

[Alex Riesen]

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 t/t5404-tracking-branches.sh |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)
 create mode 100755 t/t5404-tracking-branches.sh

diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
new file mode 100755
index 0000000..20718d4
--- /dev/null
+++ b/t/t5404-tracking-branches.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+test_description='tracking branch update checks for git push'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo 1 >file &&
+	git add file &&
+	git commit -m 1 &&
+	git branch b1 &&
+	git branch b2 &&
+	git clone . aa &&
+	git checkout b1 &&
+	echo b1 >>file &&
+	git commit -a -m b1 &&
+	git checkout b2 &&
+	echo b2 >>file &&
+	git commit -a -m b2
+'
+
+test_expect_success 'check tracking branches updated correctly after push' '
+	cd aa &&
+	b1=$(git rev-parse origin/b1) &&
+	b2=$(git rev-parse origin/b2) &&
+	git checkout -b b1 origin/b1 &&
+	echo aa-b1 >>file &&
+	git commit -a -m aa-b1 &&
+	git checkout -b b2 origin/b2 &&
+	echo aa-b2 >>file &&
+	git commit -a -m aa-b2 &&
+	git checkout master &&
+	echo aa-master >>file &&
+	git commit -a -m aa-master &&
+	git push &&
+	test "$(git rev-parse origin/b1)" = "$b1" &&
+	test "$(git rev-parse origin/b2)" = "$b2"
+'
+
+test_done
-- 
1.5.3.5.648.g1e92c

[PATCH] Update the tracking references only if they were succesfully updated on remote

[Alex Riesen]

It fixes the bug where local tracing branches were filled with zeroed SHA-1
if the remote branch was not updated because, for instance, it was not
an ancestor of the local (i.e. had other changes).

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Jeff, I think your change (334f4831e5a77) was either not complete or
got broken some time later.

 send-pack.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index b74fd45..d56d980 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -349,7 +349,8 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha
 
 	if (!dry_run && remote && ret == 0) {
 		for (ref = remote_refs; ref; ref = ref->next)
-			update_tracking_ref(remote, ref);
+			if (!is_null_sha1(ref->new_sha1))
+				update_tracking_ref(remote, ref);
 	}
 
 	if (!new_refs && ret == 0)
-- 
1.5.3.5.648.g1e92c

Re: [PATCH] Update the tracking references only if they were succesfully updated on remote

[Jeff King]

On Mon, Nov 12, 2007 at 10:39:38PM +0100, Alex Riesen wrote:

> It fixes the bug where local tracing branches were filled with zeroed SHA-1
> if the remote branch was not updated because, for instance, it was not
> an ancestor of the local (i.e. had other changes).
> 
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> ---
> 
> Jeff, I think your change (334f4831e5a77) was either not complete or
> got broken some time later.

Yes, some of the error information placed in 'ret' was getting lost.
Daniel and I discussed some fixes, but haven't done a final patch yet.
Your patch doesn't work because the assumption that
is_null_sha1(refs->new_sha1) signals error is not correct. This is also
the case for deleted refs, which means that we are failing to update
tracking branches for successfully deleted refs.

I'd like to have a patch that accurately tracks per-ref errors,
including ones from the remote. That not only will give us more accurate
status reporting, but fixes like this will be much easier. Let me see if
I can put something together.

-Peff

Re: [PATCH] Update the tracking references only if they were succesfully updated on remote

[Alex Riesen]

Jeff King, Tue, Nov 13, 2007 08:52:40 +0100:
> On Mon, Nov 12, 2007 at 10:39:38PM +0100, Alex Riesen wrote:
> 
> > It fixes the bug where local tracing branches were filled with zeroed SHA-1
> > if the remote branch was not updated because, for instance, it was not
> > an ancestor of the local (i.e. had other changes).
> > 
> > Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> > ---
> > 
> > Jeff, I think your change (334f4831e5a77) was either not complete or
> > got broken some time later.
> 
> Yes, some of the error information placed in 'ret' was getting lost.
> Daniel and I discussed some fixes, but haven't done a final patch yet.
> Your patch doesn't work because the assumption that
> is_null_sha1(refs->new_sha1) signals error is not correct. This is also
> the case for deleted refs, which means that we are failing to update
> tracking branches for successfully deleted refs.

Yep. I extended the test with this case, checking for deletion of the
remote reference and its tracking reference. Will send it separetely.
The mainline fails the second test, my patch the third (first being
the test setup).

> I'd like to have a patch that accurately tracks per-ref errors,
> including ones from the remote. That not only will give us more accurate
> status reporting, but fixes like this will be much easier. Let me see if
> I can put something together.

I actually started almost like that: by placing an error bit in struct
ref.  But than it looked like new_sha1 was just not updated for not
sent references, and the deleted ref are checked if their
peer_ref->new_sha1 is null_sha1.

Date: Mon, 12 Nov 2007 17:09:24 +0100
Subject: [PATCH] Do not update the tracking references if they could not be updated on remote

---
 cache.h     |    5 +++--
 send-pack.c |   20 +++++++++++++-------
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/cache.h b/cache.h
index f0a25c7..081b697 100644
--- a/cache.h
+++ b/cache.h
@@ -493,8 +493,9 @@ struct ref {
 	struct ref *next;
 	unsigned char old_sha1[20];
 	unsigned char new_sha1[20];
-	unsigned char force;
-	unsigned char merge;
+	unsigned char force:1;
+	unsigned char merge:1;
+	unsigned char failed:1;
 	struct ref *peer_ref; /* when renaming */
 	char name[FLEX_ARRAY]; /* more */
 };
diff --git a/send-pack.c b/send-pack.c
index b74fd45..c7a4c0e 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -196,8 +196,8 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 		return;
 
 	if (!remote_find_tracking(remote, &rs)) {
-		fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
-		if (is_null_sha1(ref->peer_ref->new_sha1)) {
+		fprintf(stderr, "updating local tracking ref '%s' with %s\n", rs.dst, sha1_to_hex(ref->new_sha1));
+		if (is_null_sha1(ref->new_sha1)) {
 			if (delete_ref(rs.dst, NULL))
 				error("Failed to delete");
 		} else
@@ -211,6 +211,7 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha
 {
 	struct ref *ref;
 	int new_refs;
+#define REF_NOT_UPTODATE (-3)
 	int ret = 0;
 	int ask_for_status_report = 0;
 	int allow_deleting_refs = 0;
@@ -246,6 +247,7 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha
 	for (ref = remote_refs; ref; ref = ref->next) {
 		char old_hex[60], *new_hex;
 		int will_delete_ref;
+		ref->failed = 0;
 
 		if (!ref->peer_ref)
 			continue;
@@ -253,6 +255,7 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha
 
 		will_delete_ref = is_null_sha1(ref->peer_ref->new_sha1);
 		if (will_delete_ref && !allow_deleting_refs) {
+			ref->failed = 1;
 			error("remote does not support deleting refs");
 			ret = -2;
 			continue;
@@ -297,13 +300,12 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha
 				 * commits at the remote end and likely
 				 * we were not up to date to begin with.
 				 */
+				ref->failed = 1;
 				error("remote '%s' is not an ancestor of\n"
-				      " local  '%s'.\n"
-				      " Maybe you are not up-to-date and "
-				      "need to pull first?",
+				      " local  '%s'",
 				      ref->name,
 				      ref->peer_ref->name);
-				ret = -2;
+				ret = REF_NOT_UPTODATE;
 				continue;
 			}
 		}
@@ -337,6 +339,9 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha
 		}
 	}
 
+	if (ret == REF_NOT_UPTODATE)
+		fprintf(stderr, "\nMaybe you are not up-to-date and need to pull first?\n\n");
+
 	packet_flush(out);
 	if (new_refs && !dry_run)
 		ret = pack_objects(out, remote_refs);
@@ -349,7 +354,8 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha
 
 	if (!dry_run && remote && ret == 0) {
 		for (ref = remote_refs; ref; ref = ref->next)
-			update_tracking_ref(remote, ref);
+			if (!ref->failed)
+				update_tracking_ref(remote, ref);
 	}
 
 	if (!new_refs && ret == 0)
-- 
1.5.3.5.668.g22088

[PATCH] Add a test for deleting remote branches

[Alex Riesen]

Check if the tracking and the remote branches are both actually deleted.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

As promised.

 t/t5404-tracking-branches.sh |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
index 20718d4..f55143a 100755
--- a/t/t5404-tracking-branches.sh
+++ b/t/t5404-tracking-branches.sh
@@ -10,6 +10,7 @@ test_expect_success 'setup' '
 	git commit -m 1 &&
 	git branch b1 &&
 	git branch b2 &&
+	git branch b3 &&
 	git clone . aa &&
 	git checkout b1 &&
 	echo b1 >>file &&
@@ -19,6 +20,8 @@ test_expect_success 'setup' '
 	git commit -a -m b2
 '
 
+start_dir="$(pwd)"
+
 test_expect_success 'check tracking branches updated correctly after push' '
 	cd aa &&
 	b1=$(git rev-parse origin/b1) &&
@@ -37,4 +40,17 @@ test_expect_success 'check tracking branches updated correctly after push' '
 	test "$(git rev-parse origin/b2)" = "$b2"
 '
 
+test_expect_success 'delete remote branch' '
+	git push origin :refs/heads/b3 &&
+	{
+		git rev-parse origin/b3 --
+		test $? != 0
+        } &&
+	cd "$start_dir" &&
+	{
+		git rev-parse refs/heads/b3 --
+		test $? != 0
+        }
+'
+
 test_done
-- 
1.5.3.5.668.g22088

[PATCH] Improved and extended t5404

[Alex Riesen]

Ignore exit code of git push in t5404, as it is not relevant for the
test: it already checks whether the references updated correctly.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

This one is on top of what is in next. It also include the check for
deleting remote braches I sent before. Regarding this one: if a remote
branch is deleted, shouldn't the matching tracking branch be removed
as well? The code in master seem to do that.

 t/t5404-tracking-branches.sh |   29 ++++++++++++++++++++++++++---
 1 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
index 20718d4..a51bbdc 100755
--- a/t/t5404-tracking-branches.sh
+++ b/t/t5404-tracking-branches.sh
@@ -10,6 +10,7 @@ test_expect_success 'setup' '
 	git commit -m 1 &&
 	git branch b1 &&
 	git branch b2 &&
+	git branch b3 &&
 	git clone . aa &&
 	git checkout b1 &&
 	echo b1 >>file &&
@@ -19,10 +20,13 @@ test_expect_success 'setup' '
 	git commit -a -m b2
 '
 
+start_dir="$(pwd)"
+
 test_expect_success 'check tracking branches updated correctly after push' '
 	cd aa &&
 	b1=$(git rev-parse origin/b1) &&
 	b2=$(git rev-parse origin/b2) &&
+	b3=$(git rev-parse origin/b3) &&
 	git checkout -b b1 origin/b1 &&
 	echo aa-b1 >>file &&
 	git commit -a -m aa-b1 &&
@@ -32,9 +36,28 @@ test_expect_success 'check tracking branches updated correctly after push' '
 	git checkout master &&
 	echo aa-master >>file &&
 	git commit -a -m aa-master &&
-	git push &&
-	test "$(git rev-parse origin/b1)" = "$b1" &&
-	test "$(git rev-parse origin/b2)" = "$b2"
+	{
+		git push
+		test "$(git rev-parse origin/b1)" = "$b1" &&
+		test "$(git rev-parse origin/b2)" = "$b2" &&
+		test "$(git rev-parse origin/b3)" = "$b3" &&
+		test "$(git rev-parse origin/master)" = \
+		"$(git rev-parse master)"
+	}
+'
+
+test_expect_success 'delete remote branch' '
+	git push origin :refs/heads/b3
+	{
+		git rev-parse origin/b3
+		test $? != 0 || \
+		say "Hmm... Maybe tracking ref should be deleted?"
+        } &&
+	cd "$start_dir" &&
+	{
+		git rev-parse refs/heads/b3
+		test $? != 0
+        }
 '
 
 test_done

Re: [PATCH] Improved and extended t5404

[Jeff King]

On Wed, Nov 14, 2007 at 12:02:34AM +0100, Alex Riesen wrote:

> This one is on top of what is in next. It also include the check for
> deleting remote braches I sent before. Regarding this one: if a remote
> branch is deleted, shouldn't the matching tracking branch be removed
> as well? The code in master seem to do that.

Yes, it should (the code in update_tracking_ref seems to handle that
case, but I haven't tested, so I may have bungled something). I am
literally walking out the door, now, though, so I will be out of touch
for at least a day.

-Peff

Re: [PATCH] Improved and extended t5404

[Jeff King]

On Tue, Nov 13, 2007 at 06:10:48PM -0500, Jeff King wrote:

> > This one is on top of what is in next. It also include the check for
> > deleting remote braches I sent before. Regarding this one: if a remote
> > branch is deleted, shouldn't the matching tracking branch be removed
> > as well? The code in master seem to do that.
> 
> Yes, it should (the code in update_tracking_ref seems to handle that
> case, but I haven't tested, so I may have bungled something). I am
> literally walking out the door, now, though, so I will be out of touch
> for at least a day.

After I became disconnected, I looked at my 'next', and the reason for
the failure to delete the ref seems to be your is_null_sha1
error-checking patch, which Junio put in next. But maybe you have
figured that out in the intervening time. :)

-Peff

[PATCH] Add test that checks diverse aspects of updating remote and tracking branches

[Alex Riesen]

Because we haven't settled on what the exit status from
"git push" command itself should be in such a partial
failure case, do not check the exit status from it for
now.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Jeff King, Thu, Nov 15, 2007 05:26:26 +0100:
> On Tue, Nov 13, 2007 at 06:10:48PM -0500, Jeff King wrote:
> 
> > > This one is on top of what is in next. It also include the check for
> > > deleting remote braches I sent before. Regarding this one: if a remote
> > > branch is deleted, shouldn't the matching tracking branch be removed
> > > as well? The code in master seem to do that.
> > 
> > Yes, it should (the code in update_tracking_ref seems to handle that
> > case, but I haven't tested, so I may have bungled something). I am
> > literally walking out the door, now, though, so I will be out of touch
> > for at least a day.
> 
> After I became disconnected, I looked at my 'next', and the reason for
> the failure to delete the ref seems to be your is_null_sha1
> error-checking patch, which Junio put in next. But maybe you have
> figured that out in the intervening time. :)

I didn't. But Junio already has all your patches in pu, so I activated
the deletion test and rebased it on top of your patches in his tree
(jk/send-pack, according to merge commit). Tried: works.

 t/t5404-tracking-branches.sh |   28 +++++++++++++++++++++++++---
 1 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
index 20718d4..a6f60ac 100755
--- a/t/t5404-tracking-branches.sh
+++ b/t/t5404-tracking-branches.sh
@@ -10,6 +10,7 @@ test_expect_success 'setup' '
 	git commit -m 1 &&
 	git branch b1 &&
 	git branch b2 &&
+	git branch b3 &&
 	git clone . aa &&
 	git checkout b1 &&
 	echo b1 >>file &&
@@ -19,10 +20,13 @@ test_expect_success 'setup' '
 	git commit -a -m b2
 '
 
+start_dir="$(pwd)"
+
 test_expect_success 'check tracking branches updated correctly after push' '
 	cd aa &&
 	b1=$(git rev-parse origin/b1) &&
 	b2=$(git rev-parse origin/b2) &&
+	b3=$(git rev-parse origin/b3) &&
 	git checkout -b b1 origin/b1 &&
 	echo aa-b1 >>file &&
 	git commit -a -m aa-b1 &&
@@ -32,9 +36,27 @@ test_expect_success 'check tracking branches updated correctly after push' '
 	git checkout master &&
 	echo aa-master >>file &&
 	git commit -a -m aa-master &&
-	git push &&
-	test "$(git rev-parse origin/b1)" = "$b1" &&
-	test "$(git rev-parse origin/b2)" = "$b2"
+	{
+		git push
+		test "$(git rev-parse origin/b1)" = "$b1" &&
+		test "$(git rev-parse origin/b2)" = "$b2" &&
+		test "$(git rev-parse origin/b3)" = "$b3" &&
+		test "$(git rev-parse origin/master)" = \
+		"$(git rev-parse master)"
+	}
+'
+
+test_expect_success 'delete remote branch' '
+	git push origin :refs/heads/b3
+	{
+		git rev-parse --verify origin/b3
+		test $? != 0
+        } &&
+	cd "$start_dir" &&
+	{
+		git rev-parse refs/heads/b3
+		test $? != 0
+        }
 '
 
 test_done
-- 
1.5.3.5.692.ge1737

Re: [PATCH] Improved and extended t5404

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Ignore exit code of git push in t5404, as it is not relevant for the
> test

This proposed log message solicits a "Huh? -- Since when
ignoring exit code is an improvement?" reaction.  If this push
is expected to error out, then wouldn't you want to make sure it
errors out as expected?  If the problem is that the exit status
is unreliable, maybe we need to make it reliable instead?

Re: [PATCH] Improved and extended t5404

[Alex Riesen]

Junio C Hamano, Wed, Nov 14, 2007 01:02:20 +0100:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
> > Ignore exit code of git push in t5404, as it is not relevant for the
> > test
> 
> This proposed log message solicits a "Huh? -- Since when
> ignoring exit code is an improvement?" reaction.  If this push
> is expected to error out, then wouldn't you want to make sure it
> errors out as expected?  If the problem is that the exit status
> is unreliable, maybe we need to make it reliable instead?

Well, it is kind of undefined. git push just updated some remote
references and failed on the others. It has had some failures, so it
returns non-0. And as I said, it really is not about the operation,
but about if the tracking and remote branches are set as we want them.

Re: [PATCH] Improved and extended t5404

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Junio C Hamano, Wed, Nov 14, 2007 01:02:20 +0100:
>> Alex Riesen <raa.lkml@gmail.com> writes:
>> 
>> > Ignore exit code of git push in t5404, as it is not relevant for the
>> > test
>> 
>> This proposed log message solicits a "Huh? -- Since when
>> ignoring exit code is an improvement?" reaction.  If this push
>> is expected to error out, then wouldn't you want to make sure it
>> errors out as expected?  If the problem is that the exit status
>> is unreliable, maybe we need to make it reliable instead?
>
> Well, it is kind of undefined. git push just updated some remote
> references and failed on the others. It has had some failures, so it
> returns non-0. And as I said, it really is not about the operation,
> but about if the tracking and remote branches are set as we want them.

Ok, I guess the proposed log message was not clear enough, at
least for me.

Will apply.

Re: [PATCH] Improved and extended t5404

[Johannes Schindelin]

Hi,

On Wed, 14 Nov 2007, Alex Riesen wrote:

> Junio C Hamano, Wed, Nov 14, 2007 01:02:20 +0100:
> > Alex Riesen <raa.lkml@gmail.com> writes:
> > 
> > > Ignore exit code of git push in t5404, as it is not relevant for the 
> > > test
> > 
> > This proposed log message solicits a "Huh? -- Since when ignoring exit 
> > code is an improvement?" reaction.  If this push is expected to error 
> > out, then wouldn't you want to make sure it errors out as expected?  
> > If the problem is that the exit status is unreliable, maybe we need to 
> > make it reliable instead?
> 
> Well, it is kind of undefined. git push just updated some remote 
> references and failed on the others. It has had some failures, so it 
> returns non-0. And as I said, it really is not about the operation, but 
> about if the tracking and remote branches are set as we want them.

If you know it should fail, why not make the test dependent on that 
failure?  I mean, should git-push have a bug and not fail, it would be 
nice to catch this early...

Ciao,
Dscho

Re: [PATCH] Improved and extended t5404

[Alex Riesen]

Johannes Schindelin, Wed, Nov 14, 2007 18:10:25 +0100:
> On Wed, 14 Nov 2007, Alex Riesen wrote:
> > Junio C Hamano, Wed, Nov 14, 2007 01:02:20 +0100:
> > > Alex Riesen <raa.lkml@gmail.com> writes:
> > > 
> > > > Ignore exit code of git push in t5404, as it is not relevant for the 
> > > > test
> > > 
> > > This proposed log message solicits a "Huh? -- Since when ignoring exit 
> > > code is an improvement?" reaction.  If this push is expected to error 
> > > out, then wouldn't you want to make sure it errors out as expected?  
> > > If the problem is that the exit status is unreliable, maybe we need to 
> > > make it reliable instead?
> > 
> > Well, it is kind of undefined. git push just updated some remote 
> > references and failed on the others. It has had some failures, so it 
> > returns non-0. And as I said, it really is not about the operation, but 
> > about if the tracking and remote branches are set as we want them.
> 
> If you know it should fail, why not make the test dependent on that 
> failure?  I mean, should git-push have a bug and not fail, it would be 
> nice to catch this early...
> 

Well, I do not know it _should_ fail. Personally, I would not even
care: I see no way to cover with just one exit code multiple
failures. Some references were updated and I don't even know which.
So I'd better check whatever exit code.

Re: [PATCH] Improved and extended t5404

[Alex Riesen]

Alex Riesen, Wed, Nov 14, 2007 20:45:22 +0100:
> Johannes Schindelin, Wed, Nov 14, 2007 18:10:25 +0100:
> > On Wed, 14 Nov 2007, Alex Riesen wrote:
> > > Junio C Hamano, Wed, Nov 14, 2007 01:02:20 +0100:
> > > > Alex Riesen <raa.lkml@gmail.com> writes:
> > > > 
> > > > > Ignore exit code of git push in t5404, as it is not relevant for the 
> > > > > test
> > > > 
> > > > This proposed log message solicits a "Huh? -- Since when ignoring exit 
> > > > code is an improvement?" reaction.  If this push is expected to error 
> > > > out, then wouldn't you want to make sure it errors out as expected?  
> > > > If the problem is that the exit status is unreliable, maybe we need to 
> > > > make it reliable instead?
> > > 
> > > Well, it is kind of undefined. git push just updated some remote 
> > > references and failed on the others. It has had some failures, so it 
> > > returns non-0. And as I said, it really is not about the operation, but 
> > > about if the tracking and remote branches are set as we want them.
> > 
> > If you know it should fail, why not make the test dependent on that 
> > failure?  I mean, should git-push have a bug and not fail, it would be 
> > nice to catch this early...
> > 
> 
> Well, I do not know it _should_ fail. Personally, I would not even
> care: I see no way to cover with just one exit code multiple
> failures. Some references were updated and I don't even know which.
> So I'd better check whatever exit code.

"I'd better check whatever was updated and damn the exit code"

Re: [PATCH] Improved and extended t5404

[Johannes Schindelin]

Hi,

On Wed, 14 Nov 2007, Alex Riesen wrote:

> Alex Riesen, Wed, Nov 14, 2007 20:45:22 +0100:
>
> > Well, I do not know it _should_ fail. Personally, I would not even 
> > care: I see no way to cover with just one exit code multiple failures. 
> > Some references were updated and I don't even know which. So I'd 
> > better check whatever exit code.
> 
> "I'd better check whatever was updated and damn the exit code"

My point was: why not check both?  I mean, you know if it fails in your 
case.  Better to test for this behaviour, than to have it succeed here, 
but fail there.

It's really easy, too: if it does not succeed, it fails.  Just test for 
it.

Ciao,
Dscho "consistency is good"

Re: [PATCH] Improved and extended t5404

[Jeff King]

On Wed, Nov 14, 2007 at 08:19:29AM +0100, Alex Riesen wrote:

> Well, it is kind of undefined. git push just updated some remote
> references and failed on the others. It has had some failures, so it
> returns non-0. And as I said, it really is not about the operation,
> but about if the tracking and remote branches are set as we want them.

My goal with the recent patches is that _any_ failure will cause a non-0
exit code (but you have to read the stderr output to find out which, if
any, refs were successful).

-Peff

Re: [PATCH] Improved and extended t5404

[Jeff King]

On Wed, Nov 14, 2007 at 11:18:01PM -0500, Jeff King wrote:

> My goal with the recent patches is that _any_ failure will cause a non-0
> exit code (but you have to read the stderr output to find out which, if
> any, refs were successful).

BTW, since there seems to be some debate on how this _should_ work, I
think the "signal failure if anything failed" approach is the better.

Why?

Because either way you do it, there is an ambiguity, and I would rather
that ambiguity lie with the "failure" case. If I see exit code '0', I
_know_ that all of my refs were updated. If I see exit code '1', then
there was some failure detected, but my refs might or might not have
been updated. But that ambiguity _already_ exists. Consider the case
where we send refs, but the connection dies in the middle. We have to
signal error, then, but for all we know the other side was about to
"successfully updated all refs". So you can only ever _know_ success,
and with failure, you simply guess (and presumably retry).

-Peff

Re: [PATCH] Improved and extended t5404

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... So you can only ever _know_ success,
> and with failure, you simply guess (and presumably retry).

Good thinking.  I agree.

Re: [PATCH] Improved and extended t5404

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Ignore exit code of git push in t5404, as it is not relevant for the
> test: it already checks whether the references updated correctly.

I think the Subject: goes a lot better with a description like this:

	Add test that checks the case where X does Y and make
	sure Z happens.

	Because we haven't settled on what the exit status from
	"git push" command itself should be in such a partial
	failure case, do not check the exit status from it for
	now.

> diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
> index 20718d4..a51bbdc 100755
> --- a/t/t5404-tracking-branches.sh
> +++ b/t/t5404-tracking-branches.sh
> @@ -10,6 +10,7 @@ test_expect_success 'setup' '
>  	git commit -m 1 &&
>  	git branch b1 &&
>  	git branch b2 &&
> +	git branch b3 &&
>  	git clone . aa &&
>  	git checkout b1 &&
> ...

So it makes another ref "b3" point at the initial commit,... 

> ...
>  test_expect_success 'check tracking branches updated correctly after push' '
>  	cd aa &&
>  	b1=$(git rev-parse origin/b1) &&
>  	b2=$(git rev-parse origin/b2) &&
> +	b3=$(git rev-parse origin/b3) &&
>  	git checkout -b b1 origin/b1 &&
>  	echo aa-b1 >>file &&
>  	git commit -a -m aa-b1 &&

... then records what was cloned,...

> @@ -32,9 +36,28 @@ test_expect_success 'check tracking branches updated correctly after push' '
>  	git checkout master &&
>  	echo aa-master >>file &&
>  	git commit -a -m aa-master &&
> +	{
> +		git push
> +		test "$(git rev-parse origin/b1)" = "$b1" &&
> +		test "$(git rev-parse origin/b2)" = "$b2" &&
> +		test "$(git rev-parse origin/b3)" = "$b3" &&
> +		test "$(git rev-parse origin/master)" = \
> +		"$(git rev-parse master)"
> +	}
> +'

... and checks that untouched "b3" stays the same (iow, tests
up-to-date case).

> +
> +test_expect_success 'delete remote branch' '
> +	git push origin :refs/heads/b3
> +	{
> +		git rev-parse origin/b3
> +		test $? != 0 || \
> +		say "Hmm... Maybe tracking ref should be deleted?"
> +        } &&

Ah, you meant that tracking should be deleted so this should be
fixed in the code but the test is disabled for now.  Let's be a
bit more explicit about such a temporary disabled test, like
this:

	git push origin :refs/heads/b3

	# The remote-tracking branch origin/b3 should be deleted;
        # we need to update the code and enable this test.
        : git rev-parse --verify origin/b3 &&

> +	cd "$start_dir" &&
> +	{
> +		git rev-parse refs/heads/b3
> +		test $? != 0
> +        }
>  '

[PATCH] Add test that checks diverse aspects of updating remote and tracking branches

[Alex Riesen]

Because we haven't settled on what the exit status from
"git push" command itself should be in such a partial
failure case, do not check the exit status from it for
now.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Junio C Hamano, Wed, Nov 14, 2007 22:52:19 +0100:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
> > Ignore exit code of git push in t5404, as it is not relevant for the
> > test: it already checks whether the references updated correctly.
> 
> I think the Subject: goes a lot better with a description like this:
> 
> 	Add test that checks the case where X does Y and make
> 	sure Z happens.

Add test that checks diverse aspects of updating remote and tracking
branches.

> 	Because we haven't settled on what the exit status from
> 	"git push" command itself should be in such a partial
> 	failure case, do not check the exit status from it for
> 	now.

This I'll leave as is.

> > +	git branch b3 &&
> 
> So it makes another ref "b3" point at the initial commit,... 

Right

> > +	b3=$(git rev-parse origin/b3) &&
> 
> ... then records what was cloned,...

Precisely

> > +		test "$(git rev-parse origin/b3)" = "$b3" &&
> 
> ... and checks that untouched "b3" stays the same (iow, tests
> up-to-date case).

Yep.

> > +
> > +test_expect_success 'delete remote branch' '
> > +	git push origin :refs/heads/b3
> > +	{
> > +		git rev-parse origin/b3
> > +		test $? != 0 || \
> > +		say "Hmm... Maybe tracking ref should be deleted?"
> > +        } &&
> 
> Ah, you meant that tracking should be deleted so this should be
> fixed in the code but the test is disabled for now.  Let's be a
> bit more explicit about such a temporary disabled test, like
> this:
> 
> 	git push origin :refs/heads/b3
> 
> 	# The remote-tracking branch origin/b3 should be deleted;
>         # we need to update the code and enable this test.
>         : git rev-parse --verify origin/b3 &&

Nice, will take this. Except we have to check for absence of the
tracking branch. git-rev-parse must fail.

 t/t5404-tracking-branches.sh |   64 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 64 insertions(+), 0 deletions(-)
 create mode 100755 t/t5404-tracking-branches.sh

diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
new file mode 100755
index 0000000..d861a14
--- /dev/null
+++ b/t/t5404-tracking-branches.sh
@@ -0,0 +1,64 @@
+#!/bin/sh
+
+test_description='tracking branch update checks for git push'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo 1 >file &&
+	git add file &&
+	git commit -m 1 &&
+	git branch b1 &&
+	git branch b2 &&
+	git branch b3 &&
+	git clone . aa &&
+	git checkout b1 &&
+	echo b1 >>file &&
+	git commit -a -m b1 &&
+	git checkout b2 &&
+	echo b2 >>file &&
+	git commit -a -m b2
+'
+
+start_dir="$(pwd)"
+
+test_expect_success 'check tracking branches updated correctly after push' '
+	cd aa &&
+	b1=$(git rev-parse origin/b1) &&
+	b2=$(git rev-parse origin/b2) &&
+	b3=$(git rev-parse origin/b3) &&
+	git checkout -b b1 origin/b1 &&
+	echo aa-b1 >>file &&
+	git commit -a -m aa-b1 &&
+	git checkout -b b2 origin/b2 &&
+	echo aa-b2 >>file &&
+	git commit -a -m aa-b2 &&
+	git checkout master &&
+	echo aa-master >>file &&
+	git commit -a -m aa-master &&
+	{
+		git push
+		test "$(git rev-parse origin/b1)" = "$b1" &&
+		test "$(git rev-parse origin/b2)" = "$b2" &&
+		test "$(git rev-parse origin/b3)" = "$b3" &&
+		test "$(git rev-parse origin/master)" = \
+		"$(git rev-parse master)"
+	}
+'
+
+test_expect_success 'delete remote branch' '
+	git push origin :refs/heads/b3
+	{
+	# The remote-tracking branch origin/b3 should be deleted;
+	# we need to update the code and enable this test.
+		: git rev-parse --verify origin/b3
+		: test $? != 0
+        } &&
+	cd "$start_dir" &&
+	{
+		git rev-parse refs/heads/b3
+		test $? != 0
+        }
+'
+
+test_done
-- 
1.5.3.5.692.ge1737

Re: [PATCH] Improved and extended t5404

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Ignore exit code of git push in t5404, as it is not relevant for the
> test: it already checks whether the references updated correctly.

I think the Subject: goes a lot better with a description like this:

	Enhance the test to check the case where X does Y and to
	make sure Z happens.

	Because we haven't settled on what the exit status from
	"git push" command itself should be in such a partial
	failure case, do not check the exit status from it for
	now.

> diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
> index 20718d4..a51bbdc 100755
> --- a/t/t5404-tracking-branches.sh
> +++ b/t/t5404-tracking-branches.sh
> @@ -10,6 +10,7 @@ test_expect_success 'setup' '
>  	git commit -m 1 &&
>  	git branch b1 &&
>  	git branch b2 &&
> +	git branch b3 &&
>  	git clone . aa &&
>  	git checkout b1 &&
> ...

So it makes another ref "b3" point at the initial commit,... 

> ...
>  test_expect_success 'check tracking branches updated correctly after push' '
>  	cd aa &&
>  	b1=$(git rev-parse origin/b1) &&
>  	b2=$(git rev-parse origin/b2) &&
> +	b3=$(git rev-parse origin/b3) &&
>  	git checkout -b b1 origin/b1 &&
>  	echo aa-b1 >>file &&
>  	git commit -a -m aa-b1 &&

... then records what was cloned,...

> @@ -32,9 +36,28 @@ test_expect_success 'check tracking branches updated correctly after push' '
>  	git checkout master &&
>  	echo aa-master >>file &&
>  	git commit -a -m aa-master &&
> +	{
> +		git push
> +		test "$(git rev-parse origin/b1)" = "$b1" &&
> +		test "$(git rev-parse origin/b2)" = "$b2" &&
> +		test "$(git rev-parse origin/b3)" = "$b3" &&
> +		test "$(git rev-parse origin/master)" = \
> +		"$(git rev-parse master)"
> +	}
> +'

... and checks that untouched "b3" stays the same (iow, tests
up-to-date case).

> +
> +test_expect_success 'delete remote branch' '
> +	git push origin :refs/heads/b3
> +	{
> +		git rev-parse origin/b3
> +		test $? != 0 || \
> +		say "Hmm... Maybe tracking ref should be deleted?"
> +        } &&

Ah, you meant that tracking should be deleted so this should be
fixed in the code but the test is disabled for now.  Let's be a
bit more explicit about such a temporary disabled test, like
this:

	git push origin :refs/heads/b3

	# The remote-tracking branch origin/b3 should be deleted;
        # we need to update the code and enable this test.
        : git rev-parse --verify origin/b3 &&

> +	cd "$start_dir" &&
> +	{
> +		git rev-parse refs/heads/b3
> +		test $? != 0
> +        }
>  '