[PATCH 0/2] fsck: don't ignore broken reflog entries

Previously, if a reflog entry's old or new SHA-1 was not resolvable to
an object, that SHA-1 was silently ignored. Instead, report such cases
as errors.

This patch series is also available from my GitHub account [1], branch
"fsck-reflog-entries".

Michael

[1] https://github.com/mhagger/git

Michael Haggerty (2):
  fsck_handle_reflog_sha1(): new function
  fsck: report errors if reflog entries point at invalid objects

 builtin/fsck.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

-- 
2.1.4

Previously, if a reflog entry's old or new SHA-1 was not resolvable to
an object, that SHA-1 was silently ignored. Instead, report such cases
as errors.

Signed-off-by: Michael Haggerty <redacted>
---
 builtin/fsck.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index b1b6c60..2679793 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c

@@ -451,7 +451,7 @@ static void fsck_dir(int i, char *path)
 
 static int default_refs;
 
-static void fsck_handle_reflog_sha1(unsigned char *sha1)
+static void fsck_handle_reflog_sha1(const char *refname, unsigned char *sha1)
 {
 	struct object *obj;
 

@@ -460,6 +460,9 @@ static void fsck_handle_reflog_sha1(unsigned char *sha1)
 		if (obj) {
 			obj->used = 1;
 			mark_object_reachable(obj);
+		} else {
+			error("%s: invalid reflog entry %s", refname, sha1_to_hex(sha1));
+			errors_found |= ERROR_REACHABLE;
 		}
 	}
 }

@@ -468,19 +471,21 @@ static int fsck_handle_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 		const char *email, unsigned long timestamp, int tz,
 		const char *message, void *cb_data)
 {
+	const char *refname = cb_data;
+
 	if (verbose)
 		fprintf(stderr, "Checking reflog %s->%s\n",
 			sha1_to_hex(osha1), sha1_to_hex(nsha1));
 
-	fsck_handle_reflog_sha1(osha1);
-	fsck_handle_reflog_sha1(nsha1);
+	fsck_handle_reflog_sha1(refname, osha1);
+	fsck_handle_reflog_sha1(refname, nsha1);
 	return 0;
 }
 
 static int fsck_handle_reflog(const char *logname, const struct object_id *oid,
 			      int flag, void *cb_data)
 {
-	for_each_reflog_ent(logname, fsck_handle_reflog_ent, NULL);
+	for_each_reflog_ent(logname, fsck_handle_reflog_ent, (void *)logname);
 	return 0;
 }
 

-- 
2.1.4

New function, extracted from fsck_handle_reflog_ent(). The extra
is_null_sha1() test for the new reference is currently unnecessary, as
reflogs are deleted when the reference itself is deleted. But it
doesn't hurt, either.

Signed-off-by: Michael Haggerty <redacted>
---
 builtin/fsck.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 4e8e2ee..b1b6c60 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c

@@ -451,28 +451,29 @@ static void fsck_dir(int i, char *path)
 
 static int default_refs;
 
-static int fsck_handle_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
-		const char *email, unsigned long timestamp, int tz,
-		const char *message, void *cb_data)
+static void fsck_handle_reflog_sha1(unsigned char *sha1)
 {
 	struct object *obj;
 
-	if (verbose)
-		fprintf(stderr, "Checking reflog %s->%s\n",
-			sha1_to_hex(osha1), sha1_to_hex(nsha1));
-
-	if (!is_null_sha1(osha1)) {
-		obj = lookup_object(osha1);
+	if (!is_null_sha1(sha1)) {
+		obj = lookup_object(sha1);
 		if (obj) {
 			obj->used = 1;
 			mark_object_reachable(obj);
 		}
 	}
-	obj = lookup_object(nsha1);
-	if (obj) {
-		obj->used = 1;
-		mark_object_reachable(obj);
-	}
+}
+
+static int fsck_handle_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+		const char *email, unsigned long timestamp, int tz,
+		const char *message, void *cb_data)
+{
+	if (verbose)
+		fprintf(stderr, "Checking reflog %s->%s\n",
+			sha1_to_hex(osha1), sha1_to_hex(nsha1));
+
+	fsck_handle_reflog_sha1(osha1);
+	fsck_handle_reflog_sha1(nsha1);
 	return 0;
 }
 

-- 
2.1.4

Hi Michael,

On 2015-06-08 08:40, Michael Haggerty wrote:

New function, extracted from fsck_handle_reflog_ent(). The extra
is_null_sha1() test for the new reference is currently unnecessary, as
reflogs are deleted when the reference itself is deleted. But it
doesn't hurt, either.

This patch is probably easier to read with the `--patience` flag (at least I find the patch obviously good in that form):

-- snipsnap --

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 4e8e2ee..b1b6c60 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c

@@ -451,28 +451,29 @@ static void fsck_dir(int i, char *path)
 
 static int default_refs;
 
+static void fsck_handle_reflog_sha1(unsigned char *sha1)
+{
+	struct object *obj;
+
+	if (!is_null_sha1(sha1)) {
+		obj = lookup_object(sha1);
+		if (obj) {
+			obj->used = 1;
+			mark_object_reachable(obj);
+		}
+	}
+}
+
 static int fsck_handle_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 		const char *email, unsigned long timestamp, int tz,
 		const char *message, void *cb_data)
 {
-	struct object *obj;
-
 	if (verbose)
 		fprintf(stderr, "Checking reflog %s->%s\n",
 			sha1_to_hex(osha1), sha1_to_hex(nsha1));
 
-	if (!is_null_sha1(osha1)) {
-		obj = lookup_object(osha1);
-		if (obj) {
-			obj->used = 1;
-			mark_object_reachable(obj);
-		}
-	}
-	obj = lookup_object(nsha1);
-	if (obj) {
-		obj->used = 1;
-		mark_object_reachable(obj);
-	}
+	fsck_handle_reflog_sha1(osha1);
+	fsck_handle_reflog_sha1(nsha1);
 	return 0;
 }
 

-- 
1.8.5.2.msysgit.0.4.gd08ed18

Hi Michael,

On 2015-06-08 08:40, Michael Haggerty wrote:

Previously, if a reflog entry's old or new SHA-1 was not resolvable to
an object, that SHA-1 was silently ignored. Instead, report such cases
as errors.

I like the idea, but I am a bit uncertain whether it would constitute "too backwards-incompatible" a change to make this an error. I think it could be argued both ways: it *is* an improvement, but it could also possibly disrupt scripts that work pretty nicely at the moment.

My fsck-api branch will help with this, of course, as users whose scripts break could be (temporarily) demote the error to a warning. I planned to work on it this week and would be happy to rebase it onto this here patch series.

Ciao,
Dscho

On 06/08/2015 04:27 PM, Johannes Schindelin wrote:

On 2015-06-08 08:40, Michael Haggerty wrote:

quoted

Previously, if a reflog entry's old or new SHA-1 was not resolvable
to an object, that SHA-1 was silently ignored. Instead, report such
cases as errors.

I like the idea, but I am a bit uncertain whether it would constitute
"too backwards-incompatible" a change to make this an error. I think
it could be argued both ways: it *is* an improvement, but it could
also possibly disrupt scripts that work pretty nicely at the moment.

What kind of script are you worried about? One that mucks around inside
the object database / reflog files? If people do that, all bets are off,
no? Plus,

* This change only causes fsck to output an extra line (and exit with
  a a non-zero retcode).

* Repair is only a

      git reflog expire --expire-unreachable=now --all

  away, I think.

[...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Hi Michael,

On 2015-06-08 17:09, Michael Haggerty wrote:

On 06/08/2015 04:27 PM, Johannes Schindelin wrote:

quoted

On 2015-06-08 08:40, Michael Haggerty wrote:

quoted

Previously, if a reflog entry's old or new SHA-1 was not resolvable
to an object, that SHA-1 was silently ignored. Instead, report such
cases as errors.

I like the idea, but I am a bit uncertain whether it would constitute
"too backwards-incompatible" a change to make this an error. I think
it could be argued both ways: it *is* an improvement, but it could
also possibly disrupt scripts that work pretty nicely at the moment.

What kind of script are you worried about?

I was concerned about scripts that work on repositories whose reflogs become inconsistent for whatever reason (that happened a lot to me in the past, IIRC it had something to do with bare repositories and/or shared object databases).

Now, if I was to run a script in, say, cron to verify that all of my repositories (possibly on a network drive, for shared team use), I could imagine that I actually want to error out if the reflogs become inconsistent. But then, I could also imagine that I care more about the script being quiet when everything is okay except for the reflogs. 

* This change only causes fsck to output an extra line (and exit with
  a a non-zero retcode).

It is that non-zero exit status that would make my hypothetical cron script start to fail.

* Repair is only a

      git reflog expire --expire-unreachable=now --all

  away, I think.

True.

Plus, as I mentioned, it could be considered a bug fix that fsck now reports this problem.

The more I think about it, the more I think it is actually a bug fix.

Thanks,
Dscho

On Mon, Jun 08, 2015 at 06:00:09PM +0200, Johannes Schindelin wrote:

quoted

quoted

I like the idea, but I am a bit uncertain whether it would constitute
"too backwards-incompatible" a change to make this an error. I think
it could be argued both ways: it *is* an improvement, but it could
also possibly disrupt scripts that work pretty nicely at the moment.

What kind of script are you worried about?

I was concerned about scripts that work on repositories whose reflogs
become inconsistent for whatever reason (that happened a lot to me in
the past, IIRC it had something to do with bare repositories and/or
shared object databases).

I think these repositories are already broken. You cannot run `git gc`
in such a repository, as it will barf when trying to walk the reflog
tips during `git repack`.

We run into this exact situation at GitHub because of our shared object
databases. Our per-fork repack code basically has to do:

  if ! git repack ...; then
    git reflog expire --expire-unreachable=all --all &&
    git repack ... ||
    die "ok, it really is broken"
  fi

-Peff

Hi Peff,

On 2015-06-08 18:56, Jeff King wrote:

On Mon, Jun 08, 2015 at 06:00:09PM +0200, Johannes Schindelin wrote:

quoted

quoted

quoted

I like the idea, but I am a bit uncertain whether it would constitute
"too backwards-incompatible" a change to make this an error. I think
it could be argued both ways: it *is* an improvement, but it could
also possibly disrupt scripts that work pretty nicely at the moment.

What kind of script are you worried about?

I was concerned about scripts that work on repositories whose reflogs
become inconsistent for whatever reason (that happened a lot to me in
the past, IIRC it had something to do with bare repositories and/or
shared object databases).

I think these repositories are already broken. You cannot run `git gc`
in such a repository, as it will barf when trying to walk the reflog
tips during `git repack`.

We run into this exact situation at GitHub because of our shared object
databases. Our per-fork repack code basically has to do:

  if ! git repack ...; then
    git reflog expire --expire-unreachable=all --all &&
    git repack ... ||
    die "ok, it really is broken"
  fi

Good point. So if I needed any more convincing that Michael's patch is a bug fix (as opposed to a backwards-incompatible change), this did it.

Ciao,
Dscho