threads / patch / 2243

patchComplain loudly, dying, when a ref is invalid

Subject: [TENTATIVE PATCH] Complain loudly, dying, when a ref is invalid

## tl;dr

5 messages between Oct 27, 2005 and Feb 28, 2006. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Johannes Schindelin· Oct 27, 2005, 17:40 UTC · lore

for_each_ref() used to ignore refs which point nowhere. Making git-fsck-objects be happy about them.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

	Of course, it may be by design. But then, it does not feel 
	right to me. If this is intended behaviour, please don't let me 
	die dumb.

 refs.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

applies-to: d29f6558d14b1da55642659c9874d4f52dac18bf 047627834b0ecb7ec0e0a98066b4b28688c374bc

Show changes to refs.c +2 −2

diff --git a/refs.c b/refs.c
index a52b038..d74ede9 100644
--- a/refs.c
+++ b/refs.c
@@ -194,9 +194,9 @@ static int do_for_each_ref(const char *b
 				continue;
 			}
 			if (read_ref(git_path("%s", path), sha1) < 0)
-				continue;
+				die("%s points nowhere!", path);
 			if (!has_sha1_file(sha1))
-				continue;
+				die("%s does not point to a valid commit object!", path);
 			retval = fn(path, sha1);
 			if (retval)
 				break;
---
0.99.8.GIT

Junio C Hamano· Oct 27, 2005, 19:01 UTC · re: Johannes Schindelin · lore

Re: [TENTATIVE PATCH] Complain loudly, dying, when a ref is invalid

The only case that I can think of that this patch may make a difference in behaviour in a valid repository is the HEAD pointer that points at nowhere (i.e. refs/heads/master) immediately after git-init-db, so in that sense this change may be safe for most purposes.

However, we might want to write a tool to iterate over what we have under .git/refs/ and warn about them to help the user fix broken branches/tags, and do_for_each_ref() dying like this would not help that usage.

Not that the current loop is any better for that purpose. We silently ignore not just dangling ref and ref not storing 40-byte hex, but files starting with a period '.', names longer than 255 bytes, and unreadable ones, all of which we would probably want to warn about in such a tool.

-jc

Johannes Schindelin· Oct 27, 2005, 19:28 UTC · re: Junio C Hamano · lore

Re: [TENTATIVE PATCH] Complain loudly, dying, when a ref is invalid

Hi,

On Thu, 27 Oct 2005, Junio C Hamano wrote:

Show 5 quoted lines

> Not that the current loop is any better for that purpose.  We
> silently ignore not just dangling ref and ref not storing
> 40-byte hex, but files starting with a period '.',  names longer
> than 255 bytes, and unreadable ones, all of which we would
> probably want to warn about in such a tool.

Okay, how about 'fprintf(stderr, "Warning: ...\n"); continue;' instead of 'die("...");' then?

Ciao, Dscho

Junio C Hamano· Oct 27, 2005, 20:45 UTC · re: Johannes Schindelin · lore

Re: [TENTATIVE PATCH] Complain loudly, dying, when a ref is invalid

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 12 quoted lines

> Hi,
>
> On Thu, 27 Oct 2005, Junio C Hamano wrote:
>
>> Not that the current loop is any better for that purpose.  We
>> silently ignore not just dangling ref and ref not storing
>> 40-byte hex, but files starting with a period '.',  names longer
>> than 255 bytes, and unreadable ones, all of which we would
>> probably want to warn about in such a tool.
>
> Okay, how about 'fprintf(stderr, "Warning: ...\n"); continue;' instead of 
> 'die("...");' then?

Yup.  That sounds sensible.

Johannes Schindelin· Feb 28, 2006, 21:16 UTC · re: Junio C Hamano · lore

[PATCH] Warn about invalid refs

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

On Thu, 27 Oct 2005, Junio C Hamano wrote:

	> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
	> 
	> > On Thu, 27 Oct 2005, Junio C Hamano wrote:
	> >
	> >> Not that the current loop is any better for that purpose.  
	> >> We silently ignore not just dangling ref and ref not storing
	> >> 40-byte hex, but files starting with a period '.', names 
	> >> longer than 255 bytes, and unreadable ones, all of which we 
	> >> would probably want to warn about in such a tool.
	> >
	> > Okay, how about 'fprintf(stderr, "Warning: ...\n"); continue;' 
	> > instead of 'die("...");' then?
	> 
	> Yup.  That sounds sensible.

	Sorry for taking so long...

 refs.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

Show changes to refs.c +7 −2

diff --git a/refs.c b/refs.c
index 826ae7a..982ebf8 100644
--- a/refs.c
+++ b/refs.c
@@ -151,10 +151,15 @@ static int do_for_each_ref(const char *b
 					break;
 				continue;
 			}
-			if (read_ref(git_path("%s", path), sha1) < 0)
+			if (read_ref(git_path("%s", path), sha1) < 0) {
+				fprintf(stderr, "%s points nowhere!", path);
 				continue;
-			if (!has_sha1_file(sha1))
+			}
+			if (!has_sha1_file(sha1)) {
+				fprintf(stderr, "%s does not point to a valid "
+						"commit object!", path);
 				continue;
+			}
 			retval = fn(path, sha1);
 			if (retval)
 				break;