# [PATCH 0/2] fsck: don't ignore broken reflog entries 11 messages from 2015-06-08 to 2015-06-08. Participants: Michael Haggerty, Johannes Schindelin, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/39560 ## Michael Haggerty, 2015-06-08 13:40 Subject: [PATCH 0/2] fsck: don't ignore broken reflog entries Message-ID: URL: https://gitlist.dev/e/cover.1433769878.git.mhagger%40alum.mit.edu ``` 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 ``` ## Michael Haggerty, 2015-06-08 13:40 Subject: [PATCH 1/2] fsck_handle_reflog_sha1(): new function Message-ID: <1c6f5540fe974016c1547163c7b891707019154d.1433769878.git.mhagger@alum.mit.edu> URL: https://gitlist.dev/e/1c6f5540fe974016c1547163c7b891707019154d.1433769878.git.mhagger%40alum.mit.edu In-Reply-To: ``` 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 --- 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 ``` ## Michael Haggerty, 2015-06-08 13:40 Subject: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects Message-ID: <2ba9ee48a062b049c8b64fed4caba32c42f03031.1433769878.git.mhagger@alum.mit.edu> URL: https://gitlist.dev/e/2ba9ee48a062b049c8b64fed4caba32c42f03031.1433769878.git.mhagger%40alum.mit.edu In-Reply-To: ``` 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 --- 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 ``` ## Johannes Schindelin, 2015-06-08 14:18 Subject: Re: [PATCH 1/2] fsck_handle_reflog_sha1(): new function Message-ID: URL: https://gitlist.dev/e/b3e391d36b36b67aca1fa5db4350f219%40www.dscho.org In-Reply-To: <1c6f5540fe974016c1547163c7b891707019154d.1433769878.git.mhagger@alum.mit.edu> ``` 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 ``` ## Johannes Schindelin, 2015-06-08 14:27 Subject: Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects Message-ID: URL: https://gitlist.dev/e/e9d2f8cc371aa41e5d9095abd3cb7edb%40www.dscho.org In-Reply-To: <2ba9ee48a062b049c8b64fed4caba32c42f03031.1433769878.git.mhagger@alum.mit.edu> ``` 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 ``` ## Junio C Hamano, 2015-06-08 15:07 Subject: Re: [PATCH 1/2] fsck_handle_reflog_sha1(): new function Message-ID: URL: https://gitlist.dev/e/xmqqwpzeck72.fsf%40gitster.dls.corp.google.com In-Reply-To: <1c6f5540fe974016c1547163c7b891707019154d.1433769878.git.mhagger@alum.mit.edu> ``` Michael Haggerty 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. ``` ## Michael Haggerty, 2015-06-08 15:09 Subject: Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects Message-ID: <5575B03C.6040008@alum.mit.edu> URL: https://gitlist.dev/e/5575B03C.6040008%40alum.mit.edu In-Reply-To: ``` 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 ``` ## Michael Haggerty, 2015-06-08 15:17 Subject: Re: [PATCH 1/2] fsck_handle_reflog_sha1(): new function Message-ID: <5575B1F7.8030400@alum.mit.edu> URL: https://gitlist.dev/e/5575B1F7.8030400%40alum.mit.edu In-Reply-To: ``` On 06/08/2015 05:07 PM, Junio C Hamano wrote: > Michael Haggerty 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 ``` ## Johannes Schindelin, 2015-06-08 16:00 Subject: Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects Message-ID: URL: https://gitlist.dev/e/c5720357601be135485ef546cae7ffdb%40www.dscho.org In-Reply-To: <5575B03C.6040008@alum.mit.edu> ``` 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 ``` ## Jeff King, 2015-06-08 16:56 Subject: Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects Message-ID: <20150608165643.GA6863@peff.net> URL: https://gitlist.dev/e/20150608165643.GA6863%40peff.net In-Reply-To: ``` 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 ``` ## Johannes Schindelin, 2015-06-08 17:08 Subject: Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects Message-ID: <9adde72489de0b1ad66bd2c4730b18e9@www.dscho.org> URL: https://gitlist.dev/e/9adde72489de0b1ad66bd2c4730b18e9%40www.dscho.org In-Reply-To: <20150608165643.GA6863@peff.net> ``` 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 ```