[BUG] Shallow fetch can result in broken git repo

[BUG] Shallow fetch can result in broken git repo

[Kacper Kornet]

Starting from "6035d6a fetch-pack: prepare updated shallow file before
fetching the pack" the shallow fetches of commits with tags can result
in broken git repo.  The following script illustrates the problem:

#!/bin/sh

mkdir repo1 repo2
cd repo1
git init
for i in `seq 1 3`; do
        echo $i > foo
        git add foo
        git commit -m "Commit $i"
done
git tag tag HEAD~1

cd ../repo2
git init
git fetch --depth=2 ../repo1 master:branch
git fsck

The function fetch_pack in this case is called twice. During the
second called alternate_shallow_file contains "\0" (it is set to this
value by commit_lock_file(&shallow_lock) during first call of
fetch_pack). In the result the file .git/shallow, created during first
call to fetch_pack, is removed and the git repo ends with broken link.

The two possible fixes which I see are:

1) Replace back if (alternate_shallow_file) condition in fetch pack with 
   if (args->depth > 0) 

2) alternate_shallow_file should be copy of shallow_lock.filename not a
   reference to it

But I'm not able to determine by myself which one (if any) is a correct fix
to the problem.

-- 
  Kacper

[PATCH] fetch-pack: do not remove .git/shallow file when --depth is not specified

[Nguyễn Thái Ngọc Duy]

fetch_pack() can remove .git/shallow file when a shallow repository
becomes a full one again. This behavior is triggered incorrectly when
tags are also fetched because fetch_pack() will be called twice. At
the first fetch_pack() call:

 - shallow_lock is set up
 - alternate_shallow_file points to shallow_lock.filename, which is
   "shallow.lock"
 - commit_lock_file is called, which sets shallow_lock.filename to "".
   alternate_shallow_file also becomes "" because it points to the
   same memory.

At the second call, setup_alternate_shallow() is not called and
alternate_shallow_file remains "". It's mistaken as unshallow case and
.git/shallow is removed. The end result is a broken repository.

Fix this by always initializing alternate_shallow_file when
fetch_pack() is called. As an extra measure, check if args->depth > 0
before commit/rollback shallow file.

Reported-by: Kacper Kornet <kornet@camk.edu.pl>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 > The two possible fixes which I see are:
 >
 > 1) Replace back if (alternate_shallow_file) condition in fetch pack with
 >    if (args->depth > 0)
 >
 > 2) alternate_shallow_file should be copy of shallow_lock.filename not a
 >    reference to it

 3) Move alternate_shallow_file to struct fetch_pack_args, which will
    always be zero'd by memset

 I think #1 is better. It's the original condition before 6035d6a
 replaces it with "if (alternate_shallow_file)". Apparently I did not
 see that fetch_pack() could be called twice. #3 is also an option,
 but we still need static "shallow_lock" anyway, so I disregarded it.

 fetch-pack.c          |  4 +++-
 t/t5500-fetch-pack.sh | 16 ++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 6b5467c..76190a8 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -888,6 +888,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 		packet_flush(fd[1]);
 	if (args->depth > 0)
 		setup_alternate_shallow();
+	else
+		alternate_shallow_file = NULL;
 	if (get_pack(args, fd, pack_lockfile))
 		die("git fetch-pack: fetch failed.");
 
@@ -978,7 +980,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 	}
 	ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought, pack_lockfile);
 
-	if (alternate_shallow_file) {
+	if (args->depth > 0 && alternate_shallow_file) {
 		if (*alternate_shallow_file == '\0') { /* --unshallow */
 			unlink_or_warn(git_path("shallow"));
 			rollback_lock_file(&shallow_lock);
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index fd2598e..a80584e 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -505,4 +505,20 @@ test_expect_success 'test --all, --depth, and explicit tag' '
 	) >out-adt 2>error-adt
 '
 
+test_expect_success 'shallow fetch with tags does not break the repository' '
+	mkdir repo1 &&
+	(
+		cd repo1 &&
+		git init &&
+		test_commit 1 &&
+		test_commit 2 &&
+		test_commit 3 &&
+		mkdir repo2 &&
+		cd repo2 &&
+		git init &&
+		git fetch --depth=2 ../.git master:branch &&
+		git fsck
+	)
+'
+
 test_done
-- 
1.8.2.82.gc24b958

Re: [PATCH] fetch-pack: do not remove .git/shallow file when --depth is not specified

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

>  > The two possible fixes which I see are:
>  >
>  > 1) Replace back if (alternate_shallow_file) condition in fetch pack with
>  >    if (args->depth > 0)
>  >
>  > 2) alternate_shallow_file should be copy of shallow_lock.filename not a
>  >    reference to it
>
>  3) Move alternate_shallow_file to struct fetch_pack_args, which will
>     always be zero'd by memset
>
>  I think #1 is better. It's the original condition before 6035d6a
>  replaces it with "if (alternate_shallow_file)". Apparently I did not
>  see that fetch_pack() could be called twice. #3 is also an option,
>  but we still need static "shallow_lock" anyway, so I disregarded it.

Thanks.