git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Cc: Max Kirillov <max@max630.net>,
	Michael Haggerty <mhagger@alum.mit.edu>,
	git@vger.kernel.org
Subject: Re: [RFC PATCH] pack-refs: fail on falsely sorted packed-refs
Date: Thu, 14 Feb 2019 01:06:57 -0500
Message-ID: <20190214060657.GA20997@sigill.intra.peff.net> (raw)
In-Reply-To: <87lg2kj91a.fsf@evledraar.gmail.com>

On Wed, Feb 13, 2019 at 11:08:01AM +0100, Ævar Arnfjörð Bjarmason wrote:

> > I happened to have a not really sorted packed-refs file. As you might guess,
> > it was quite wtf-ing experience. It worked, mostly, but there was one branch
> > which just did not resolve, regardless of existing and being presented in
> > for-each-refs output.
> >
> > I don't know where the corruption came from. I should admit it could even be a manual
> > editing but last time I did it (in that reporitory) was several years ago so it is unlikely.
> >
> > I am not sure what should be the proper fix. I did a minimal detection, so that
> > it does not go unnoticed. Probably next step would be either fixing in `git fsck` call.
> >
> >  refs/packed-backend.c               | 15 +++++++++++++++
> >  t/t3212-pack-refs-broken-sorting.sh | 26 ++++++++++++++++++++++++++
> >  2 files changed, 41 insertions(+)
> >  create mode 100755 t/t3212-pack-refs-broken-sorting.sh
> 
> This is not an area I'm very familiar with. So mostly commeting on
> cosmetic issues with the patch. FWIW the "years back" issue you had
> could be that an issue didn't manifest until now, i.e. in a sorted file
> format you can get lucky and not see corruption for a while with a
> random insert.

It actually shouldn't be that old a breakage. Until 02b920f3f7
(read_packed_refs(): ensure that references are ordered when read,
2017-09-25), we did not assume the file was sorted (even though we
always wrote it out sorted). And we continue to not assume the file is
sorted unless it is written out with an explicit "sorted" trait in the
header (which we started doing in that commit, too).

So a years-old manual edit would not have the "sorted" trait, and should
not have manifested as a problem, even now.  Likewise for a years-old
bug. It would have to be a bug in a _new_ writer which writes out the
sorted trait.  If there is such a bug in our implementation, this would
be the first report we've seen. Given the number of times pack-refs has
been run, without further evidence I'm inclined to think it was some
weird manual edit, or maybe an alternate implementation (though one
would _hope_ they would not write out the sorted trait without actually
sorting!).

I agree with all of the cosmetic issues you mentioned. As far as what
the patch itself does, I think it's OK. We could probably go further and
actually sort it (or even just write it out without a "sorted" trait,
which means the next read would load it all into memory and sort it).
That's a little friendlier, since just dying leaves the user to fix it
up themselves. But given that we expect this code to trigger
approximately never, it's probably not worth spending much time on a
fancy solution.

-Peff

  parent reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-30 23:13 Max Kirillov
2019-01-30 23:31 ` Eric Sunshine
2019-01-31  8:21   ` Max Kirillov
2019-02-08 21:22   ` [PATCH v2] " Max Kirillov
2019-02-08 21:40     ` Eric Sunshine
2019-02-13  4:24       ` Max Kirillov
     [not found]     ` <CAMy9T_EX_L80-V4zD626nFCxw6qa90+pZwcbd6wHw9ZHcj2rNA@mail.gmail.com>
2019-02-13  4:23       ` Max Kirillov
2019-02-13 10:08 ` [RFC PATCH] " Ævar Arnfjörð Bjarmason
2019-02-13 10:56   ` SZEDER Gábor
2019-02-23  7:10     ` Max Kirillov
2019-02-14  6:06   ` Jeff King [this message]
2019-02-23  7:09   ` Max Kirillov

Reply instructions:

You may reply publically 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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=20190214060657.GA20997@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=max@max630.net \
    --cc=mhagger@alum.mit.edu \
    /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

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox