Re: [PATCH] repack: Add options to preserve and prune old pack files

From: jmelvin@codeaurora.org <jmelvin@codeaurora.org>
Date: Mar 9, 2017, 17:50 UTC
Message-ID: <1d816bbb08b228ece9a74ffcdfb7a5b1@codeaurora.org>
In-Reply-To: <xmqq4lz4968p.fsf@gitster.mtv.corp.google.com>

On 2017-03-07 13:33, Junio C Hamano wrote:

Show 18 quoted lines

> James Melvin <jmelvin@codeaurora.org> writes:
> 
>> These options are designed to prevent stale file handle exceptions
>> during git operations which can happen on users of NFS repos when
>> repacking is done on them. The strategy is to preserve old pack files
>> around until the next repack with the hopes that they will become
>> unreferenced by then and not cause any exceptions to running processes
>> when they are finally deleted (pruned).
> 
> I find it a very sensible strategy to work around NFS, but it does
> not explain why the directory the old ones are moved to need to be
> configurable.  It feels to me that a boolean that causes the old
> ones renamed s/^pack-/^old-&/ in the same directory (instead of
> pruning them right away) would risk less chances of mistakes (e.g.
> making "preserved" subdirectory on a separate device mounted there
> in a hope to reduce disk usage of the primary repository, which
> may defeat the whole point of moving the still-active file around
> instead of removing them).

Moving the preserved pack files to a separate directory only helped make the pack directory cleaner, but I agree that having the old* pack files in the same directory is a better approach as it would ensure that it's still on the same mounted device. I'll update the logic to reflect that.

As for the naming convention of the preserved pack files, there is already some logic to remove "old-" files in repack. Currently this is the naming convention I have for them:

pack-<sha1>.old-<ext> pack-7412ee739b8a20941aa1c2fd03abcc7336b330ba.old-pack

One advantage of that is the extension is no longer an expected one, differentiating it from current pack files.

That said, if that is not a concern, I could prefix them with "preserved" instead of "old" to differentiate them from the other logic that cleans up "old-*". What are your thoughts on that?

preserved-<sha1>.<ext> preserved-7412ee739b8a20941aa1c2fd03abcc7336b330ba.pack

Show 18 quoted lines

> And if we make "preserve-old" a boolean, perhaps the presence of
> "prune-preserved" would serve as a substitute for it, iow, perhaps
> we may only need --prune-preserved option (and repack.prunePreserved
> configuration variable)?
> 
>> diff --git a/builtin/repack.c b/builtin/repack.c
>> index 677bc7c81..f1a0c97f3 100644
>> --- a/builtin/repack.c
>> +++ b/builtin/repack.c
>> @@ -10,8 +10,10 @@
>> 
>>  static int delta_base_offset = 1;
>>  static int pack_kept_objects = -1;
>> +static int preserve_oldpacks = 0;
>> +static int prune_preserved = 0;
> 
> We avoid initializing statics to 0 or NULL and instead let BSS take
> care of them...

Will do

Show 18 quoted lines

>>  static int write_bitmaps;
>> -static char *packdir, *packtmp;
>> +static char *packdir, *packtmp, *preservedir;
> 
> ... just like what you did here.
> 
>> @@ -108,6 +110,27 @@ static void get_non_kept_pack_filenames(struct s
>> ...
>> +static void preserve_pack(const char *file_path, const char 
>> *file_name,  const char *file_ext)
>> +{
>> +	char *fname_old;
>> +
>> +	if (mkdir(preservedir, 0700) && errno != EEXIST)
>> +		error(_("failed to create preserve directory"));
> 
> You do not want to do the rest of this function after issuing this
> error, no?  Because ...

Agreed, I'll update.

Show 20 quoted lines

>> +
>> +	fname_old = mkpathdup("%s/%s.old-%s", preservedir, file_name, 
>> ++file_ext);
>> +	rename(file_path, fname_old);
> 
> ... this rename(2) would fail, whose error return you would catch
> and act on.
> 
>> +	free(fname_old);
>> +}
>> +
>> +static void remove_preserved_dir(void) {
>> +	struct strbuf buf = STRBUF_INIT;
>> +
>> +	strbuf_addstr(&buf, preservedir);
>> +	remove_dir_recursively(&buf, 0);
> 
> This is a wrong helper function to use on files and directories
> inside .git/; the function is about removing paths in the working
> tree.

Will update.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project