[PATCH] repack: place temporary packs under .git/objects/pack/

Previous thread: git subcommand sigint gotcha by Joey Hess on Monday, October 18, 2010 - 9:53 pm. (10 messages)

Next thread: [PATCH] t/t9001-send-email.sh: fix stderr redirection in 'Invalid In-Reply-To' by Antonio Ospite on Tuesday, October 19, 2010 - 2:50 am. (2 messages)
From: Marat Radchenko
Date: Monday, October 18, 2010 - 11:28 pm

Console log:

Counting objects: 5137063, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (892984/892984), done.
fatal: unable to rename temporary pack file: Invalid cross-device link
error: failed to run repack

Reason:
write_pack_file in builtin/pack-objects.c first writes tmp file in
"pack/tmp_pack_XXXXXX". 

Then, it tries to rename it to "%s-%s.pack":

snprintf(tmpname, sizeof(tmpname), "%s-%s.pack",
                                 base_name, sha1_to_hex(sha1));
...
if (rename(pack_tmp_name, tmpname))
                                die_errno("unable to rename temporary pack
file");

and happily fails because that crosses device boundary. Same problem with
index file. Suggested fix: do those renames inside pack directory or avoid
them at all.
-- 
View this message in context: http://git.661346.n2.nabble.com/BUG-git-repack-fails-if-git-objects-pack-is-on-a-separ...
Sent from the git mailing list archive at Nabble.com.
--

From: Thomas Rast
Date: Monday, October 18, 2010 - 11:59 pm

What's your git version?  Since 8b4eb6b (Do not perform
cross-directory renames when creating packs, 2008-09-22), which was
first released in 1.6.0.3 and 1.6.1, it should only create and rename
files within the pack/ subdirectory.  Which is what you are implying
in the above snippet, but this should never result in a cross-device
rename, so maybe I'm missing something.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--

From: Marat Radchenko
Date: Tuesday, October 19, 2010 - 12:29 am

It should, yep, but it doesn't.


That rename call attempts to put file in _current_ directory.

Proof:
mkdir foo && touch foo/bar && strace mv foo/bar baz 2>&1 | grep rename
outputs rename("foo/bar", "baz")


-- 
View this message in context: http://git.661346.n2.nabble.com/BUG-git-repack-fails-if-git-objects-pack-is-on-a-separ...
Sent from the git mailing list archive at Nabble.com.
--

From: Marat Radchenko
Date: Tuesday, October 19, 2010 - 12:40 am

Testcase:

# tell it where is other partition
OTHER_PARTITION=/path/to/other/partition/

# init repo
mkdir foo && cd foo && git init

# put some stuff and run gc to create packfile
cp /etc/fstab . && git add fstab && git commit -m "initial" && git gc

# move pack dir to other partition and install symlink
mv .git/objects/pack $OTHER_PARTITION && ln -s $OTHER_PARTITION/pack
.git/objects/pack

# run gc (it fails)
strace -f git gc 2>&1 | grep rename

[pid  3792] rename(".git/packed-refs.lock", ".git/packed-refs") = 0
[pid  3793] rename(".git/logs/refs/heads/master.lock",
".git/logs/refs/heads/master") = 0
[pid  3793] rename(".git/logs/HEAD.lock", ".git/logs/HEAD") = 0
[pid  3808] rename(".git/objects/pack/tmp_pack_8ZhS92",
"/home/marat/foo/.git/objects/.tmp-3795-pack-da13ceb9f70c2762595a7a2932411db339bdab46.pack")
= -1 EXDEV (Invalid cross-device link)
[pid  3808] write(2, "fatal: unable to rename temporar"..., 71fatal: unable
to rename temporary pack file: Invalid cross-device link

So you see, it tries to move packfile from .git/objects/pack to .git/objects
-- 
View this message in context: http://git.661346.n2.nabble.com/BUG-git-repack-fails-if-git-objects-pack-is-on-a-separ...
Sent from the git mailing list archive at Nabble.com.
--

From: Thomas Rast
Date: Tuesday, October 19, 2010 - 2:50 am

git-pack-objects is already careful to start out its temporary packs
under .git/objects/pack/ (cf. 8b4eb6b, Do not perform cross-directory
renames when creating packs, 2008-09-22), but git-repack did not
respond in kind so the effort was lost when the filesystem boundary is
exactly at that directory.

Let git-repack pass a path under .git/objects/pack/ as the base for
its temporary packs.

This means we might need the $PACKDIR sooner (before the pack-objects
invocation), so move the mkdir up just to be safe.

Also note that the only use of *.pack is in the find invocation way
before the pack-objects call, so the temporary packs will not suddenly
show up in any wildcards because of the directory change.

Reported-by: Marat Radchenko <marat@slonopotamus.org>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 git-repack.sh |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/git-repack.sh b/git-repack.sh
index 769baaf..624feec 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -52,7 +52,7 @@ true)
 esac
 
 PACKDIR="$GIT_OBJECT_DIRECTORY/pack"
-PACKTMP="$GIT_OBJECT_DIRECTORY/.tmp-$$-pack"
+PACKTMP="$PACKDIR/.tmp-$$-pack"
 rm -f "$PACKTMP"-*
 trap 'rm -f "$PACKTMP"-*' 0 1 2 3 15
 
@@ -82,6 +82,8 @@ case ",$all_into_one," in
 	;;
 esac
 
+mkdir -p "$PACKDIR" || exit
+
 args="$args $local ${GIT_QUIET:+-q} $no_reuse$extra"
 names=$(git pack-objects --keep-true-parents --honor-pack-keep --non-empty --all --reflog $args </dev/null "$PACKTMP") ||
 	exit 1
@@ -90,7 +92,6 @@ if [ -z "$names" ]; then
 fi
 
 # Ok we have prepared all new packfiles.
-mkdir -p "$PACKDIR" || exit
 
 # 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
-- 
1.7.3.1.271.ged4d2

--

From: Marat Radchenko
Date: Tuesday, October 19, 2010 - 3:17 am

Either i'm doing something wrong or given patch doesn't fix testcase [1] i
gave before. Tested by applying patch to git master and running
<git_master>/git --exec-path=<git_master> gc

[1]
http://git.661346.n2.nabble.com/BUG-git-repack-fails-if-git-objects-pack-is-on-a-separ...
-- 
View this message in context: http://git.661346.n2.nabble.com/BUG-git-repack-fails-if-git-objects-pack-is-on-a-separ...
Sent from the git mailing list archive at Nabble.com.
--

From: Junio C Hamano
Date: Tuesday, October 19, 2010 - 9:33 am

Looks good; thanks.
--

Previous thread: git subcommand sigint gotcha by Joey Hess on Monday, October 18, 2010 - 9:53 pm. (10 messages)

Next thread: [PATCH] t/t9001-send-email.sh: fix stderr redirection in 'Invalid In-Reply-To' by Antonio Ospite on Tuesday, October 19, 2010 - 2:50 am. (2 messages)