git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Eric Wong <normalperson@yhbt.net>, git@vger.kernel.org
Subject: Re: [PATCH] pack-objects: warn on split packs disabling bitmaps
Date: Wed, 27 Apr 2016 22:15:09 -0400	[thread overview]
Message-ID: <20160428021509.GB9707@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqqfuu67j9t.fsf@gitster.mtv.corp.google.com>

On Wed, Apr 27, 2016 at 03:56:46PM -0700, Junio C Hamano wrote:

> Eric Wong <normalperson@yhbt.net> writes:
> 
> > It can be tempting for a server admin to want a stable set of
> > long-lived packs for dumb clients; but also want to enable
> > bitmaps to serve smart clients more quickly.
> >
> > Unfortunately, such a configuration is impossible;
> > so at least warn users of this incompatibility since
> > commit 21134714787a02a37da15424d72c0119b2b8ed71
> > ("pack-objects: turn off bitmaps when we split packs").
> >
> > Tested the warning by inspecting the output of:
> >
> > 	make -C t t5310-pack-bitmaps.sh GIT_TEST_OPTS=-v
> 
> While I do not think the update in this patch is wrong, this makes
> me wonder what happens to a server admin who wants a stable set of
> long-lived objects in a pack, and other objects in another pack that
> is frequently recreated, by first packing objects needed for the
> latest released version into a single pack and marking it with .keep
> and then running "git repack" to create the second pack for the
> remainder.
> 
> There is no automatic split involved, so we would get a bitmap file
> for each of these two packs.  Would that pose a problem?  The pack
> with the newer part of the history would not satisfy "all of the
> reachable objects are in the same pack" condition.

You will not get two bitmaps in such a case. In add_object_entry(), if
we exclude an object via want_object_in_pack(), we will issue a warning
and turn off bitmaps.  That includes finding that one of the reachable
objects we would need is in a .keep pack.

You could in theory construct a broken non-closed bitmap by feeding an
arbitrary set of objects to pack-objects, but we turn off bitmap writing
entirely unless --all is used. So the test in add_object_entry() should
be sufficient for all cases there (it's actually too conservative; it's
possible that the object is not reachable from the other objects that
are going into the pack, but this is so impractical that it's not even
worth trying to catch).

The split-pack check from 211347147 had to come separately, because we
only find out about the split much later during the write phase (and
again, it is too conservative; it's _possible_ that the objects being
split aren't reachable from anything in the previous pack, but it's
exceedingly unlikely and not worth caring about).

Adding a warning as Eric's patch does is a definite improvement, and
something I should have done in the original (we _could_ just use the
same no_closure_warning, as that is technically what is going on, but I
think it is nice to mention pack-splitting, which is more likely to lead
the user in the right direction to fixing it).

> [discussion of doc fixes]

I do not mind overly if pack-splitting mentions disabling bitmaps. But I
think it may be simpler to keep the documentation in the bitmap section,
rather than trying to cross-reference in both directions.  It is the
bitmap code which is not prepared to work with pack-splits (and other
things, like .keep), not the other way around.

-Peff

  reply	other threads:[~2016-04-28  2:15 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-04-27 21:53 [PATCH] pack-objects: warn on split packs disabling bitmaps Eric Wong
2016-04-27 22:56 ` Junio C Hamano
2016-04-28  2:15   ` Jeff King [this message]
2016-04-28  7:28   ` [PATCH v2] " Eric Wong
2016-04-28  2:25 ` [PATCH] " Jeff King
2016-04-28  8:02   ` Eric Wong

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

  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=20160428021509.GB9707@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=normalperson@yhbt.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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).