git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Johannes Schindelin <johannes.schindelin@gmx.de>
To: Michael Haggerty <mhagger@alum.mit.edu>
Cc: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
	git@vger.kernel.org
Subject: Re: [PATCH 2/2] fsck: report errors if reflog entries point at invalid objects
Date: Mon, 08 Jun 2015 18:00:09 +0200	[thread overview]
Message-ID: <c5720357601be135485ef546cae7ffdb@www.dscho.org> (raw)
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

  reply	other threads:[~2015-06-08 16:00 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2015-06-08 16:56         ` Jeff King
2015-06-08 17:08           ` Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c5720357601be135485ef546cae7ffdb@www.dscho.org \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=mhagger@alum.mit.edu \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).