* [PATCH 0/2] fsck: don't ignore broken reflog entries
@ 2015-06-08 13:40 Michael Haggerty
2015-06-08 13:40 ` [PATCH 1/2] fsck_handle_reflog_sha1(): new function Michael Haggerty
2015-06-08 13:40 ` [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects Michael Haggerty
0 siblings, 2 replies; 11+ messages in thread
From: Michael Haggerty @ 2015-06-08 13:40 UTC (permalink / raw
To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git, 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] fsck_handle_reflog_sha1(): new function
2015-06-08 13:40 [PATCH 0/2] fsck: don't ignore broken reflog entries Michael Haggerty
@ 2015-06-08 13:40 ` Michael Haggerty
2015-06-08 14:18 ` Johannes Schindelin
2015-06-08 15:07 ` Junio C Hamano
2015-06-08 13:40 ` [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects Michael Haggerty
1 sibling, 2 replies; 11+ messages in thread
From: Michael Haggerty @ 2015-06-08 13:40 UTC (permalink / raw
To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects
2015-06-08 13:40 [PATCH 0/2] fsck: don't ignore broken reflog entries Michael Haggerty
2015-06-08 13:40 ` [PATCH 1/2] fsck_handle_reflog_sha1(): new function Michael Haggerty
@ 2015-06-08 13:40 ` Michael Haggerty
2015-06-08 14:27 ` Johannes Schindelin
1 sibling, 1 reply; 11+ messages in thread
From: Michael Haggerty @ 2015-06-08 13:40 UTC (permalink / raw
To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] fsck_handle_reflog_sha1(): new function
2015-06-08 13:40 ` [PATCH 1/2] fsck_handle_reflog_sha1(): new function Michael Haggerty
@ 2015-06-08 14:18 ` Johannes Schindelin
2015-06-08 15:07 ` Junio C Hamano
1 sibling, 0 replies; 11+ messages in thread
From: Johannes Schindelin @ 2015-06-08 14:18 UTC (permalink / raw
To: Michael Haggerty; +Cc: Junio C Hamano, Jeff King, git
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects
2015-06-08 13:40 ` [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects Michael Haggerty
@ 2015-06-08 14:27 ` Johannes Schindelin
2015-06-08 15:09 ` Michael Haggerty
0 siblings, 1 reply; 11+ messages in thread
From: Johannes Schindelin @ 2015-06-08 14:27 UTC (permalink / raw
To: Michael Haggerty; +Cc: Junio C Hamano, Jeff King, git
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] fsck_handle_reflog_sha1(): new function
2015-06-08 13:40 ` [PATCH 1/2] fsck_handle_reflog_sha1(): new function Michael Haggerty
2015-06-08 14:18 ` Johannes Schindelin
@ 2015-06-08 15:07 ` Junio C Hamano
2015-06-08 15:17 ` Michael Haggerty
1 sibling, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2015-06-08 15:07 UTC (permalink / raw
To: Michael Haggerty; +Cc: Jeff King, Johannes Schindelin, git
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects
2015-06-08 14:27 ` Johannes Schindelin
@ 2015-06-08 15:09 ` Michael Haggerty
2015-06-08 16:00 ` Johannes Schindelin
0 siblings, 1 reply; 11+ messages in thread
From: Michael Haggerty @ 2015-06-08 15:09 UTC (permalink / raw
To: Johannes Schindelin; +Cc: Junio C Hamano, Jeff King, git
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] fsck_handle_reflog_sha1(): new function
2015-06-08 15:07 ` Junio C Hamano
@ 2015-06-08 15:17 ` Michael Haggerty
0 siblings, 0 replies; 11+ messages in thread
From: Michael Haggerty @ 2015-06-08 15:17 UTC (permalink / raw
To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects
2015-06-08 15:09 ` Michael Haggerty
@ 2015-06-08 16:00 ` Johannes Schindelin
2015-06-08 16:56 ` Jeff King
0 siblings, 1 reply; 11+ messages in thread
From: Johannes Schindelin @ 2015-06-08 16:00 UTC (permalink / raw
To: Michael Haggerty; +Cc: Junio C Hamano, Jeff King, git
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects
2015-06-08 16:00 ` Johannes Schindelin
@ 2015-06-08 16:56 ` Jeff King
2015-06-08 17:08 ` Johannes Schindelin
0 siblings, 1 reply; 11+ messages in thread
From: Jeff King @ 2015-06-08 16:56 UTC (permalink / raw
To: Johannes Schindelin; +Cc: Michael Haggerty, Junio C Hamano, git
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects
2015-06-08 16:56 ` Jeff King
@ 2015-06-08 17:08 ` Johannes Schindelin
0 siblings, 0 replies; 11+ messages in thread
From: Johannes Schindelin @ 2015-06-08 17:08 UTC (permalink / raw
To: Jeff King; +Cc: Michael Haggerty, Junio C Hamano, git
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-06-08 17:08 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-08 13:40 [PATCH 0/2] fsck: don't ignore broken reflog entries Michael Haggerty
2015-06-08 13:40 ` [PATCH 1/2] fsck_handle_reflog_sha1(): new function Michael Haggerty
2015-06-08 14:18 ` Johannes Schindelin
2015-06-08 15:07 ` Junio C Hamano
2015-06-08 15:17 ` Michael Haggerty
2015-06-08 13:40 ` [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects Michael Haggerty
2015-06-08 14:27 ` Johannes Schindelin
2015-06-08 15:09 ` Michael Haggerty
2015-06-08 16:00 ` Johannes Schindelin
2015-06-08 16:56 ` Jeff King
2015-06-08 17:08 ` Johannes Schindelin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.