t5505-remote.29 not working correctly

Junio,

I ran across this flaw while investigating something else.

Here's the copy&pasted diff which adds '&&' in two places:

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index a82c5ff..a3406dd 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -510,12 +510,12 @@ test_expect_success 'remote prune to cause a dangling symr
        ) 2>err &&
        grep "has become dangling" err &&
 
-       : And the dangling symref will not cause other annoying errors
+       : And the dangling symref will not cause other annoying errors &&
        (
                cd seven &&
                git branch -a
        ) 2>err &&
-       ! grep "points nowhere" err
+       ! grep "points nowhere" err &&
        (
                cd seven &&
                test_must_fail git branch nomore origin


The first grep causes the failure:

   ...
   (
   	cd seven &&
   	git remote prune origin
   ) 2>err &&
   grep "has become dangling" err &&
   ...

'git remote prune origin' is not printing "has become dangling".

-brandon

Junio C Hamano· Mar 16, 2010, 03:26 UTC · re: Brandon Casey · lore

Re: t5505-remote.29 not working correctly

Brandon Casey <brandon.casey.ctr@nrlssc.navy.mil> writes:

> The test titled 'remote prune to cause a dangling symref' is
> not linked together with &&'s.  When the &&'s are added, it
> does not complete successfully.

I haven't tried merging the result to a more recent codebase, though.

-- >8 --
Subject: [PATCH] warn_dangling_symref(): send the warning to standard error stream

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-remote.c  |    2 +-
 refs.c            |    7 ++++---
 refs.h            |    2 +-
 t/t5505-remote.sh |    2 +-
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index ac69d37..e67221d 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -789,7 +789,7 @@ static int prune(int argc, const char **argv)
 
 			printf(" * [%s] %s\n", dry_run ? "would prune" : "pruned",
 			       abbrev_ref(refname, "refs/remotes/"));
-			warn_dangling_symref(dangling_msg, refname);
+			warn_dangling_symref(stderr, dangling_msg, refname);
 		}
 
 		/* NEEDSWORK: free remote */
diff --git a/refs.c b/refs.c
index 6eb5f53..a0e7da7 100644
--- a/refs.c
+++ b/refs.c
@@ -286,6 +286,7 @@ static struct ref_list *get_ref_dir(const char *base, struct ref_list *list)
 }
 
 struct warn_if_dangling_data {
+	FILE *fp;
 	const char *refname;
 	const char *msg_fmt;
 };
@@ -304,13 +305,13 @@ static int warn_if_dangling_symref(const char *refname, const unsigned char *sha
 	if (!resolves_to || strcmp(resolves_to, d->refname))
 		return 0;
 
-	printf(d->msg_fmt, refname);
+	fprintf(d->fp, d->msg_fmt, refname);
 	return 0;
 }
 
-void warn_dangling_symref(const char *msg_fmt, const char *refname)
+void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)
 {
-	struct warn_if_dangling_data data = { refname, msg_fmt };
+	struct warn_if_dangling_data data = { fp, refname, msg_fmt };
 	for_each_rawref(warn_if_dangling_symref, &data);
 }
 
diff --git a/refs.h b/refs.h
index 29bdcec..64a8e62 100644
--- a/refs.h
+++ b/refs.h
@@ -27,7 +27,7 @@ extern int for_each_remote_ref(each_ref_fn, void *);
 /* can be used to learn about broken ref and symref */
 extern int for_each_rawref(each_ref_fn, void *);
 
-extern void warn_dangling_symref(const char *msg_fmt, const char *refname);
+extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname);
 
 /*
  * Extra refs will be listed by for_each_ref() before any actual refs
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 2067dc5..2cad416 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -415,7 +415,7 @@ test_expect_success 'remote prune to cause a dangling symref' '
 	) 2>err &&
 	grep "has become dangling" err &&
 
-	: And the dangling symref will not cause other annoying errors
+	: And the dangling symref will not cause other annoying errors &&
 	(
 		cd seven &&
 		git branch -a

-- 
1.7.0.2.445.g0ae494

Junio C Hamano· Mar 16, 2010, 05:16 UTC · re: Junio C Hamano · lore

Re: t5505-remote.29 not working correctly

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

> Brandon Casey <brandon.casey.ctr@nrlssc.navy.mil> writes:
>
>> The test titled 'remote prune to cause a dangling symref' is
>> not linked together with &&'s.  When the &&'s are added, it
>> does not complete successfully.
>
> Hmm, it looks like f8948e2 (remote prune: warn dangling symrefs,
> 2009-02-08) is internally inconsistent.  This is a fix directly on top of
> that commit.

If we change the test to:

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index a82c5ff..ca88e29 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -507,8 +507,8 @@ test_expect_success 'remote prune to cause a dangling symref' '
 	(
 		cd seven &&
 		git remote prune origin
-	) 2>err &&
-	grep "has become dangling" err &&
+	) >err 2>&1 &&
+	grep "has become dangling" err || exit
 
 	: And the dangling symref will not cause other annoying errors
 	(

so it does not matter whether the output is set to standard output or
standard error, we can bisect this, which points at eafb452 (do_one_ref():
null_sha1 check is not about broken ref, 2009-07-22).

v1.6.3 and onwards seem to be broken.

Junio C Hamano· Mar 16, 2010, 07:40 UTC · re: Junio C Hamano · lore

[PATCH] refs: ref entry with NULL sha1 is can be a dangling symref

Fix this by:

 - adopt Brandon's fix to t5505 test;

 - introduce REF_BROKEN flag to mark a ref that fails to resolve (dangling
   symref);

 - move the check for broken ref back inside the "if we are skipping
   dangling refs" code block.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I deliberately left hashclr() because other codepaths are likely to be
   using it as a signal that the ref is invalid.  They can be lazily
   converted to pay attention to REF_BROKEN bit instead of doing 20-byte
   comparison to null SHA-1.

 refs.c            |   11 +++++++----
 t/t5505-remote.sh |    6 +++---
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 63e30d7..a7518b6 100644
--- a/refs.c
+++ b/refs.c
@@ -6,6 +6,7 @@
 
 /* ISSYMREF=01 and ISPACKED=02 are public interfaces */
 #define REF_KNOWS_PEELED 04
+#define REF_BROKEN 010
 
 struct ref_list {
 	struct ref_list *next;
@@ -275,8 +276,10 @@ static struct ref_list *get_ref_dir(const char *base, struct ref_list *list)
 				list = get_ref_dir(ref, list);
 				continue;
 			}
-			if (!resolve_ref(ref, sha1, 1, &flag))
+			if (!resolve_ref(ref, sha1, 1, &flag)) {
 				hashclr(sha1);
+				flag |= REF_BROKEN;
+			}
 			list = add_ref(ref, sha1, flag, list, NULL);
 		}
 		free(ref);
@@ -539,10 +542,10 @@ static int do_one_ref(const char *base, each_ref_fn fn, int trim,
 {
 	if (strncmp(base, entry->name, trim))
 		return 0;
-	/* Is this a "negative ref" that represents a deleted ref? */
-	if (is_null_sha1(entry->sha1))
-		return 0;
+
 	if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {
+		if (entry->flag & REF_BROKEN)
+			return 0; /* ignore dangling symref */
 		if (!has_sha1_file(entry->sha1)) {
 			error("%s does not point to a valid object!", entry->name);
 			return 0;
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index a82c5ff..2692050 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -507,15 +507,15 @@ test_expect_success 'remote prune to cause a dangling symref' '
 	(
 		cd seven &&
 		git remote prune origin
-	) 2>err &&
+	) >err 2>&1 &&
 	grep "has become dangling" err &&
 
-	: And the dangling symref will not cause other annoying errors
+	: And the dangling symref will not cause other annoying errors &&
 	(
 		cd seven &&
 		git branch -a
 	) 2>err &&
-	! grep "points nowhere" err
+	! grep "points nowhere" err &&
 	(
 		cd seven &&
 		test_must_fail git branch nomore origin