On 2017-03-07 13:33, Junio C Hamano wrote:
James Melvin <jmel...@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
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
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.
+
+ 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
