git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Derrick Stolee <stolee@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, avarab@gmail.com,
	Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 3/3] packfile: close_all_packs to close_object_store
Date: Mon, 20 May 2019 07:55:10 -0400	[thread overview]
Message-ID: <0c13a91f-e45e-d177-758a-30e9517a664f@gmail.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1905201141000.46@tvgsbejvaqbjf.bet>

On 5/20/2019 6:01 AM, Johannes Schindelin wrote:
> Hi Stolee,
> 
> *really* minor nit: the commit subject probably wants to have a "rename"
> after the colon ;-)

I did put that there, but then the subject line was too long. I'm not
opposed to putting it back.
 
> The patch looks sensible to me. Since Junio asked for a sanity check
> whether all of the call sites of `close_all_packs()` actually want to
> close the MIDX and the commit graph, too, I'll do the "speak out loud"
> type of patch review here (spoiler: all of them check out):

Thanks for the detail here!

>> diff --git a/builtin/repack.c b/builtin/repack.c
>> index 67f8978043..4de8b6600c 100644
>> --- a/builtin/repack.c
>> +++ b/builtin/repack.c
>> @@ -419,7 +419,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>>  	if (!names.nr && !po_args.quiet)
>>  		printf_ln(_("Nothing new to pack."));
>>
>> -	close_all_packs(the_repository->objects);
>> +	close_object_store(the_repository->objects);
>>
>>  	/*
>>  	 * Ok we have prepared all new packfiles.
> 
> Ah, the joys of un-dynamic patch review. What you, dear reader, cannot see
> in this hunk is that the code comment at the end continues thusly:
> 
>          * First see if there are packs of the same name and if so
>          * if we can move them out of the way (this can happen if we
>          * repacked immediately after packing fully.
>          */
> 
> Meaning: we're about to rename some pack files. So the pack file handles
> need to be closed, all right, but what about the other object store
> handles? There is no mention of the commit graph (more on that below), but
> the loop following the code comment contains this:
> 
>                         if (!midx_cleared) {
>                                 clear_midx_file(the_repository);
>                                 midx_cleared = 1;
>                         }
> 
> So yes, I would give this a check.
> 
> It does puzzle me, I have to admit, that there is no (opt-in) code block
> to re-write the commit graph. After all, the commit graph references the
> pack files, right? So if they are repacked, it would at least be
> invalidated at this point...

The commit-graph does not directly reference the packs. The file will still be
valid, except if we GC'd some commits that it references. We have the ability
to rewrite the graph in 'git gc'.

The MIDX does reference packs by name, so it needs to be cleared before we delete
packs. This _could_ be done with more care: we only need to delete it if a pack
it references is queued for deletion. However, you can do that using the
'git multi-pack-index expire|repack' pattern currently cooking.

Thanks,
-Stolee


  reply	other threads:[~2019-05-20 11:55 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-17 18:41 [PATCH 0/3] Close commit-graph before calling 'gc' Derrick Stolee via GitGitGadget
2019-05-17 18:41 ` [PATCH 1/3] commit-graph: use raw_object_store when closing Derrick Stolee via GitGitGadget
2019-05-17 18:41 ` [PATCH 2/3] packfile: close commit-graph in close_all_packs Derrick Stolee via GitGitGadget
2019-05-17 18:41 ` [PATCH 3/3] packfile: close_all_packs to close_object_store Derrick Stolee via GitGitGadget
2019-05-20 10:01   ` Johannes Schindelin
2019-05-20 11:55     ` Derrick Stolee [this message]
2019-05-28 16:29       ` Junio C Hamano
2019-05-19  2:04 ` [PATCH 0/3] Close commit-graph before calling 'gc' Junio C Hamano
2019-05-20 10:13   ` Johannes Schindelin
2019-05-20 11:05   ` Derrick Stolee
2019-05-20  9:40 ` 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

  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=0c13a91f-e45e-d177-758a-30e9517a664f@gmail.com \
    --to=stolee@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    /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).