t5505-remote.29 not working correctly

t5505-remote.29 not working correctly

[Brandon Casey]

Junio,

I ran across this flaw while investigating something else.

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.

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

Re: t5505-remote.29 not working correctly

[Junio C Hamano]

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.

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

Brandon Casey noticed tht t5505 had accidentally broken its && chain,
hiding inconsistency between the code that writes the warning to the
standard output and the test that expects to see the warning on the
standard error.

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

Re: t5505-remote.29 not working correctly

[Junio C Hamano]

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.

Actually this is a bit deeper than that.  We don't get _any_ output to
either standard output nor standard error with the recent code.

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.

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

[Junio C Hamano]

Brandon Casey noticed that t5505 had accidentally broken its && chain,
hiding inconsistency between the code that writes the warning to the
standard output and the test that expects to see the warning on the
standard error, which was introduced by f8948e2 (remote prune: warn
dangling symrefs, 2009-02-08).

It turns out that the issue is deeper than that.  After f8948e2, a symref
that is dangling is marked with a NULL sha1, and the idea of using NULL
sha1 to mean a deleted ref was scrapped, but somehow a follow-up eafb452
(do_one_ref(): null_sha1 check is not about broken ref, 2009-07-22)
incorrectly reorganized do_one_ref(), still thinking NULL sha1 is never
used in the code.

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