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

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

[Michael Haggerty]

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

[PATCH 1/2] fsck_handle_reflog_sha1(): new function

[Michael Haggerty]

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 <mhagger@alum.mit.edu>
---
 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

Re: [PATCH 1/2] fsck_handle_reflog_sha1(): new function

[Johannes Schindelin]

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

Re: [PATCH 1/2] fsck_handle_reflog_sha1(): new function

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> 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.

I think we would crash with today's code in such a situation, but
wouldn't we want to diagnose a 0{40} object name on the "new" side
of the reflog entry as an error in the endgame state?

I do share uneasiness with Dscho that this is tightening what used
to be an OK state into an error, but I haven't thought about how
serious it would be.

Thanks.

Re: [PATCH 1/2] fsck_handle_reflog_sha1(): new function

[Michael Haggerty]

On 06/08/2015 05:07 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> 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.
> 
> I think we would crash with today's code in such a situation, but
> wouldn't we want to diagnose a 0{40} object name on the "new" side
> of the reflog entry as an error in the endgame state?

Good point. new_sha1 == NULL_SHA1 should be diagnosed and reported with
a distinct error message.

> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH 2/2] fsck: report errors if reflog entries point at invalid objects

[Michael Haggerty]

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 <mhagger@alum.mit.edu>
---
 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

Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects

[Johannes Schindelin]

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

Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects

[Michael Haggerty]

On 06/08/2015 04:27 PM, Johannes Schindelin wrote:
> 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.

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

Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects

[Johannes Schindelin]

Hi Michael,

On 2015-06-08 17:09, Michael Haggerty wrote:
> On 06/08/2015 04:27 PM, Johannes Schindelin wrote:
>> 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.
> 
> 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

Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects

[Jeff King]

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

> >> 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

Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects

[Johannes Schindelin]

Hi Peff,

On 2015-06-08 18:56, Jeff King wrote:
> On Mon, Jun 08, 2015 at 06:00:09PM +0200, Johannes Schindelin wrote:
> 
>> >> 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