git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git mailing list <git@vger.kernel.org>, Jeff King <peff@peff.net>
Subject: Re: [PATCH v2 2/2] repack: repack promisor objects if -a or -A is set
Date: Thu, 9 Aug 2018 11:12:23 -0700	[thread overview]
Message-ID: <CAGf8dg+5ywyQVfuPfbRrKFdAatst25307ctQvkWqCDKHka7z4g@mail.gmail.com> (raw)
In-Reply-To: <xmqqftzncvxv.fsf@gitster-ct.c.googlers.com>

On Thu, Aug 9, 2018 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Hmm, it is clever to auto-start the pack-objects (and notice there
> wasn't anything needing to pack).  Two things that are worth noting
> are:
>
>  - The code takes advantage of the fact that cmd.in being left as -1
>    is a sign that start_command() was never called (well, it could
>    be that it was called but failed to open pipe---but then we would
>    have died inside write_oid(), so we can ignore taht case).  This
>    is not relying on its implementation detail---cmd.in would be
>    replaced by a real fd to the pipe if start_command() was called.
>
>  - We know that pack-objects reads all its standard input through to
>    the EOF before emitting a single byte to its standard output, and
>    that is why we can use bidi pipe and not worry about deadlocking
>    caused by not reading from cmd.out at all (before closing cmd.in,
>    that is).
>
> So I kind of like the cleverness of the design of this code.

Thanks for checking this.

> For now, as we do not know what is the appropriate thing to leave in
> the file, empty is fine, but perhaps in the future we may want to
> carry forward the info from the existing .promisor file?  What does
> it initially contain?  Transport invoked with from_promisor bit
> seems to call index-pack with "--promisor" option with no argument,
> so this is probably in line with that.  Do we in the future need to
> teach "--promisor" option to pack-objects we invoke here, which will
> be passed to the index-pack it internally performs, so that we do
> not have to do this by hand here?

There might be more than one .promisor file that we are repacking, so
we would also have to deal with the order. It might be better to
document that the contents of .promisor files are lost upon repack.

> In any case, don't we need to update the doc for the "--promisor"
> option of "index-pack"?  The same comment for the new options added
> in the same topic, e.g. "--from-promisor" and "--no-dependents"
> options added to "fetch-pack".  It seems that the topic that ended
> at 0c16cd499d was done rather hastily and under-documented?

Yes, you're right - I'll make a patch documenting these.

>> @@ -440,6 +513,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>>
>>       /* End of pack replacement. */
>>
>> +     reprepare_packed_git(the_repository);
>> +
>
> I do not think this call hurts, but why does this become suddenly
> necessary with _this_ patch?  Is it an unrelated bugfix?

Hmm...I thought I mentioned somewhere that we need
reprepare_packed_git now because for_each_packed_object calls
prepare_packed_git, but apparently I didn't. I would add the following
to the commit message (if you'd rather not do it yourself, let me know
and I'll send a v3 with the new commit message):

Because for_each_packed_object() calls prepare_packed_git(), a call to
reprepare_packed_git() now has to be inserted after all the packfile
manipulation - if not, old information about packed objects would
remain in the_repository->objects->packed_git.

  reply	other threads:[~2018-08-09 18:12 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-07 20:12 [PATCH 0/2] Repacking of promisor packfiles Jonathan Tan
2018-08-07 20:12 ` [PATCH 1/2] repack: refactor setup of pack-objects cmd Jonathan Tan
2018-08-07 20:12 ` [PATCH 2/2] repack: repack promisor objects if -a or -A is set Jonathan Tan
2018-08-07 20:57   ` Junio C Hamano
2018-08-07 23:23     ` Jonathan Tan
2018-08-08  0:25       ` Junio C Hamano
2018-08-08 18:45         ` Jonathan Tan
2018-08-08 15:50       ` Jeff King
2018-08-08 16:17         ` Junio C Hamano
2018-08-07 22:37   ` Junio C Hamano
2018-08-07 23:25     ` Jonathan Tan
2018-08-08 16:34       ` Junio C Hamano
2018-08-08 22:34 ` [PATCH v2 0/2] Repacking of promisor packfiles Jonathan Tan
2018-08-08 22:34   ` [PATCH v2 1/2] repack: refactor setup of pack-objects cmd Jonathan Tan
2018-08-08 22:34   ` [PATCH v2 2/2] repack: repack promisor objects if -a or -A is set Jonathan Tan
2018-08-09 17:05     ` Junio C Hamano
2018-08-09 18:12       ` Jonathan Tan [this message]
2018-08-18 16:05     ` Duy Nguyen

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=CAGf8dg+5ywyQVfuPfbRrKFdAatst25307ctQvkWqCDKHka7z4g@mail.gmail.com \
    --to=jonathantanmy@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --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).