{"thread":{"id":"34765","subject":"[BUG] Shallow fetch can result in broken git repo","startedAt":"2013-08-26T00:22:02Z","lastAt":"2013-08-26T06:09:30Z","messageCount":3,"participants":["Kacper Kornet","Nguyễn Thái Ngọc Duy","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"225876","messageId":"20130826002202.GA26940@camk.edu.pl","threadId":"34765","inReplyTo":null,"subject":"[BUG] Shallow fetch can result in broken git repo","fromName":"Kacper Kornet","fromEmail":"kornet@camk.edu.pl","sentAt":"2013-08-26T00:22:02Z","receivedAt":"2013-08-26T00:22:02Z","isPatch":false,"sender":{"key":"kornet@camk.edu.pl","avatar":null},"body":"Starting from \"6035d6a fetch-pack: prepare updated shallow file before\nfetching the pack\" the shallow fetches of commits with tags can result\nin broken git repo.  The following script illustrates the problem:\n\n#!/bin/sh\n\nmkdir repo1 repo2\ncd repo1\ngit init\nfor i in `seq 1 3`; do\n        echo $i > foo\n        git add foo\n        git commit -m \"Commit $i\"\ndone\ngit tag tag HEAD~1\n\ncd ../repo2\ngit init\ngit fetch --depth=2 ../repo1 master:branch\ngit fsck\n\nThe function fetch_pack in this case is called twice. During the\nsecond called alternate_shallow_file contains \"\\0\" (it is set to this\nvalue by commit_lock_file(&shallow_lock) during first call of\nfetch_pack). In the result the file .git/shallow, created during first\ncall to fetch_pack, is removed and the git repo ends with broken link.\n\nThe two possible fixes which I see are:\n\n1) Replace back if (alternate_shallow_file) condition in fetch pack with \n   if (args->depth > 0) \n\n2) alternate_shallow_file should be copy of shallow_lock.filename not a\n   reference to it\n\nBut I'm not able to determine by myself which one (if any) is a correct fix\nto the problem.\n\n-- \n  Kacper\n"},{"id":"225877","messageId":"1377483446-24834-1-git-send-email-pclouds@gmail.com","threadId":"34765","inReplyTo":"20130826002202.GA26940@camk.edu.pl","subject":"[PATCH] fetch-pack: do not remove .git/shallow file when --depth is not specified","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-26T02:17:26Z","receivedAt":"2013-08-26T02:17:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"fetch_pack() can remove .git/shallow file when a shallow repository\nbecomes a full one again. This behavior is triggered incorrectly when\ntags are also fetched because fetch_pack() will be called twice. At\nthe first fetch_pack() call:\n\n - shallow_lock is set up\n - alternate_shallow_file points to shallow_lock.filename, which is\n   \"shallow.lock\"\n - commit_lock_file is called, which sets shallow_lock.filename to \"\".\n   alternate_shallow_file also becomes \"\" because it points to the\n   same memory.\n\nAt the second call, setup_alternate_shallow() is not called and\nalternate_shallow_file remains \"\". It's mistaken as unshallow case and\n.git/shallow is removed. The end result is a broken repository.\n\nFix this by always initializing alternate_shallow_file when\nfetch_pack() is called. As an extra measure, check if args->depth > 0\nbefore commit/rollback shallow file.\n\nReported-by: Kacper Kornet <kornet@camk.edu.pl>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n > The two possible fixes which I see are:\n >\n > 1) Replace back if (alternate_shallow_file) condition in fetch pack with\n >    if (args->depth > 0)\n >\n > 2) alternate_shallow_file should be copy of shallow_lock.filename not a\n >    reference to it\n\n 3) Move alternate_shallow_file to struct fetch_pack_args, which will\n    always be zero'd by memset\n\n I think #1 is better. It's the original condition before 6035d6a\n replaces it with \"if (alternate_shallow_file)\". Apparently I did not\n see that fetch_pack() could be called twice. #3 is also an option,\n but we still need static \"shallow_lock\" anyway, so I disregarded it.\n\n fetch-pack.c          |  4 +++-\n t/t5500-fetch-pack.sh | 16 ++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 6b5467c..76190a8 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -888,6 +888,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\tpacket_flush(fd[1]);\n \tif (args->depth > 0)\n \t\tsetup_alternate_shallow();\n+\telse\n+\t\talternate_shallow_file = NULL;\n \tif (get_pack(args, fd, pack_lockfile))\n \t\tdie(\"git fetch-pack: fetch failed.\");\n \n@@ -978,7 +980,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t}\n \tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought, pack_lockfile);\n \n-\tif (alternate_shallow_file) {\n+\tif (args->depth > 0 && alternate_shallow_file) {\n \t\tif (*alternate_shallow_file == '\\0') { /* --unshallow */\n \t\t\tunlink_or_warn(git_path(\"shallow\"));\n \t\t\trollback_lock_file(&shallow_lock);\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex fd2598e..a80584e 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -505,4 +505,20 @@ test_expect_success 'test --all, --depth, and explicit tag' '\n \t) >out-adt 2>error-adt\n '\n \n+test_expect_success 'shallow fetch with tags does not break the repository' '\n+\tmkdir repo1 &&\n+\t(\n+\t\tcd repo1 &&\n+\t\tgit init &&\n+\t\ttest_commit 1 &&\n+\t\ttest_commit 2 &&\n+\t\ttest_commit 3 &&\n+\t\tmkdir repo2 &&\n+\t\tcd repo2 &&\n+\t\tgit init &&\n+\t\tgit fetch --depth=2 ../.git master:branch &&\n+\t\tgit fsck\n+\t)\n+'\n+\n test_done\n-- \n1.8.2.82.gc24b958\n"},{"id":"225882","messageId":"xmqqk3j9j84l.fsf@gitster.dls.corp.google.com","threadId":"34765","inReplyTo":"1377483446-24834-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] fetch-pack: do not remove .git/shallow file when --depth is not specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-26T06:09:30Z","receivedAt":"2013-08-26T06:09:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n>  > The two possible fixes which I see are:\n>  >\n>  > 1) Replace back if (alternate_shallow_file) condition in fetch pack with\n>  >    if (args->depth > 0)\n>  >\n>  > 2) alternate_shallow_file should be copy of shallow_lock.filename not a\n>  >    reference to it\n>\n>  3) Move alternate_shallow_file to struct fetch_pack_args, which will\n>     always be zero'd by memset\n>\n>  I think #1 is better. It's the original condition before 6035d6a\n>  replaces it with \"if (alternate_shallow_file)\". Apparently I did not\n>  see that fetch_pack() could be called twice. #3 is also an option,\n>  but we still need static \"shallow_lock\" anyway, so I disregarded it.\n\nThanks.\n"}]}