{"thread":{"id":"37460","subject":"[PATCH] reachable.c: add HEAD to reachability starting commits","startedAt":"2014-08-30T20:58:35Z","lastAt":"2014-09-03T16:14:10Z","messageCount":5,"participants":["Max Kirillov","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"248664","messageId":"1409432315-20803-1-git-send-email-max@max630.net","threadId":"37460","inReplyTo":null,"subject":"[PATCH] reachable.c: add HEAD to reachability starting commits","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-08-30T20:58:35Z","receivedAt":"2014-08-30T20:58:35Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"HEAD is not explicitly used as a starting commit for\ncalculating reachability, so if it's detached and reflogs\nare disabled it may be pruned.\n\nAdd test which demonstrates it.\n\nSigned-off-by: Max Kirillov <max@max630.net>\n---\nHi.\n\nThis is a followup of http://thread.gmane.org/gmane.comp.version-control.git/255996\nI digged it a bit more and look what I found!\n\nActually, I think would be nice if someone familiar with the code greps for\nfor_each_ref() usage and for each case verify if head should be added.\n reachable.c               |  3 +++\n t/t5312-prune-detached.sh | 19 +++++++++++++++++++\n 2 files changed, 22 insertions(+)\n create mode 100755 t/t5312-prune-detached.sh\n\ndiff --git a/reachable.c b/reachable.c\nindex 654a8c5..6f6835b 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -229,6 +229,9 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t/* Add all external refs */\n \tfor_each_ref(add_one_ref, revs);\n \n+\t/* detached HEAD is not included in the list above */\n+\thead_ref(add_one_ref, revs);\n+\n \t/* Add all reflog info */\n \tif (mark_reflog)\n \t\tfor_each_reflog(add_one_reflog, revs);\ndiff --git a/t/t5312-prune-detached.sh b/t/t5312-prune-detached.sh\nnew file mode 100755\nindex 0000000..fac93e1\n--- /dev/null\n+++ b/t/t5312-prune-detached.sh\n@@ -0,0 +1,19 @@\n+#!/bin/sh\n+\n+test_description='no prune detached head without reflog'\n+. ./test-lib.sh\n+\n+test_expect_success 'make repo' '\n+\tgit config core.logAllRefUpdates false\n+\tgit commit --allow-empty -m commit1 &&\n+\tgit commit --allow-empty -m commit2 &&\n+\tgit checkout  --detach master &&\n+\tgit commit --allow-empty -m commit3\n+'\n+\n+test_expect_success 'prune does not delete anything' '\n+\tgit prune -n >prune_actual &&\n+\t: >prune_expected &&\n+\ttest_cmp prune_expected prune_actual'\n+\n+test_done\n-- \n2.0.1.1697.g73c6810\n"},{"id":"248685","messageId":"20140831152806.GC17449@peff.net","threadId":"37460","inReplyTo":"1409432315-20803-1-git-send-email-max@max630.net","subject":"Re: [PATCH] reachable.c: add HEAD to reachability starting commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-31T15:28:06Z","receivedAt":"2014-08-31T15:28:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 30, 2014 at 11:58:35PM +0300, Max Kirillov wrote:\n\n> HEAD is not explicitly used as a starting commit for\n> calculating reachability, so if it's detached and reflogs\n> are disabled it may be pruned.\n\nEek, you're right. I think nobody noticed because the HEAD reflog\nusually picks it up (and you do not usually detach HEAD on a bare repo).\nBut I agree we should include it to cover this case.\n\n> diff --git a/reachable.c b/reachable.c\n> index 654a8c5..6f6835b 100644\n> --- a/reachable.c\n> +++ b/reachable.c\n> @@ -229,6 +229,9 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n>  \t/* Add all external refs */\n>  \tfor_each_ref(add_one_ref, revs);\n>  \n> +\t/* detached HEAD is not included in the list above */\n> +\thead_ref(add_one_ref, revs);\n> +\n>  \t/* Add all reflog info */\n>  \tif (mark_reflog)\n>  \t\tfor_each_reflog(add_one_reflog, revs);\n\nLooks obviously correct.\n\n> diff --git a/t/t5312-prune-detached.sh b/t/t5312-prune-detached.sh\n> new file mode 100755\n> index 0000000..fac93e1\n> --- /dev/null\n> +++ b/t/t5312-prune-detached.sh\n> @@ -0,0 +1,19 @@\n> +#!/bin/sh\n> +\n> +test_description='no prune detached head without reflog'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'make repo' '\n> +\tgit config core.logAllRefUpdates false\n> +\tgit commit --allow-empty -m commit1 &&\n> +\tgit commit --allow-empty -m commit2 &&\n> +\tgit checkout  --detach master &&\n> +\tgit commit --allow-empty -m commit3\n> +'\n> +\n> +test_expect_success 'prune does not delete anything' '\n> +\tgit prune -n >prune_actual &&\n> +\t: >prune_expected &&\n> +\ttest_cmp prune_expected prune_actual'\n> +\n> +test_done\n\nYour test looks reasonable, but is there any reason it cannot go in\nt5304 with the other prune tests?\n\n-Peff\n"},{"id":"248693","messageId":"1409519804-15960-1-git-send-email-max@max630.net","threadId":"37460","inReplyTo":"20140831152806.GC17449@peff.net","subject":"[PATCH v2] reachable.c: add HEAD to reachability starting commits","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-08-31T21:16:44Z","receivedAt":"2014-08-31T21:16:44Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"HEAD is not explicitly used as a starting commit for\ncalculating reachability, so if it's detached and reflogs\nare disabled it may be pruned.\n\nAdd tests which demonstrate it. Test 'prune: prune former HEAD after checking\nout branch' also reverts changes to repository.\n\nSigned-off-by: Max Kirillov <max@max630.net>\n---\nInserted test into existing script.\n reachable.c      |  3 +++\n t/t5304-prune.sh | 21 +++++++++++++++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/reachable.c b/reachable.c\nindex 654a8c5..6f6835b 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -229,6 +229,9 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t/* Add all external refs */\n \tfor_each_ref(add_one_ref, revs);\n \n+\t/* detached HEAD is not included in the list above */\n+\thead_ref(add_one_ref, revs);\n+\n \t/* Add all reflog info */\n \tif (mark_reflog)\n \t\tfor_each_reflog(add_one_reflog, revs);\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 377d3d3..77cf064 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -104,6 +104,27 @@ test_expect_success 'prune: prune unreachable heads' '\n \n '\n \n+test_expect_success 'prune: do not prune detached HEAD with no reflog' '\n+\n+\tgit config core.logAllRefUpdates false &&\n+\ttest ! -e .git/logs &&\n+\tgit checkout --detach --quiet &&\n+\tgit commit --allow-empty -m \"detached commit\" &&\n+\tgit prune -n >prune_actual &&\n+\t: >prune_expected &&\n+\ttest_cmp prune_actual prune_expected\n+\n+'\n+\n+test_expect_success 'prune: prune former HEAD after checking out branch' '\n+\n+\thead_sha1=`git rev-parse HEAD` &&\n+\tgit checkout --quiet master &&\n+\tgit prune -v >prune_actual &&\n+\tgrep -q \"$head_sha1\" prune_actual\n+\n+'\n+\n test_expect_success 'prune: do not prune heads listed as an argument' '\n \n \t: > file2 &&\n-- \n2.0.1.1697.g73c6810\n"},{"id":"248721","messageId":"xmqq4mwp3oeb.fsf@gitster.dls.corp.google.com","threadId":"37460","inReplyTo":"1409519804-15960-1-git-send-email-max@max630.net","subject":"Re: [PATCH v2] reachable.c: add HEAD to reachability starting commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-02T17:47:40Z","receivedAt":"2014-09-02T17:47:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov <max@max630.net> writes:\n\n> HEAD is not explicitly used as a starting commit for\n> calculating reachability, so if it's detached and reflogs\n> are disabled it may be pruned.\n>\n> Add tests which demonstrate it. Test 'prune: prune former HEAD after checking\n> out branch' also reverts changes to repository.\n>\n> Signed-off-by: Max Kirillov <max@max630.net>\n> ---\n> Inserted test into existing script.\n>  reachable.c      |  3 +++\n>  t/t5304-prune.sh | 21 +++++++++++++++++++++\n>  2 files changed, 24 insertions(+)\n>\n> diff --git a/reachable.c b/reachable.c\n> index 654a8c5..6f6835b 100644\n> --- a/reachable.c\n> +++ b/reachable.c\n> @@ -229,6 +229,9 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n>  \t/* Add all external refs */\n>  \tfor_each_ref(add_one_ref, revs);\n>  \n> +\t/* detached HEAD is not included in the list above */\n> +\thead_ref(add_one_ref, revs);\n> +\n\nUnlike the change to rev-parse, which I haven't decided if I like\nit, this is definitely the right thing to do.\n\nThanks for catching.\n\n>  \t/* Add all reflog info */\n>  \tif (mark_reflog)\n>  \t\tfor_each_reflog(add_one_reflog, revs);\n> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\n> index 377d3d3..77cf064 100755\n> --- a/t/t5304-prune.sh\n> +++ b/t/t5304-prune.sh\n> @@ -104,6 +104,27 @@ test_expect_success 'prune: prune unreachable heads' '\n>  \n>  '\n>  \n> +test_expect_success 'prune: do not prune detached HEAD with no reflog' '\n> +\n> +\tgit config core.logAllRefUpdates false &&\n> +\ttest ! -e .git/logs &&\n\nThis test is somewhat iffy; if you had it just before \"git prune\", I\nwould understand that it matches what the title of the test claims,\nthough.\n\n> +\tgit checkout --detach --quiet &&\n> +\tgit commit --allow-empty -m \"detached commit\" &&\n> +\tgit prune -n >prune_actual &&\n> +\t: >prune_expected &&\n> +\ttest_cmp prune_actual prune_expected\n> +\n> +'\n> +\n> +test_expect_success 'prune: prune former HEAD after checking out branch' '\n> +\n> +\thead_sha1=`git rev-parse HEAD` &&\n\nFavor $(...) over `...`.\n\n> +\tgit checkout --quiet master &&\n> +\tgit prune -v >prune_actual &&\n> +\tgrep -q \"$head_sha1\" prune_actual\n\nPlease don't use \"grep -q\"; unless running the test with \"-v\", we\nwouldn't have to see the output anyway.\n\n> +\n> +'\n> +\n>  test_expect_success 'prune: do not prune heads listed as an argument' '\n>  \n>  \t: > file2 &&\n"},{"id":"248788","messageId":"1409760850-27322-1-git-send-email-max@max630.net","threadId":"37460","inReplyTo":"xmqq4mwp3oeb.fsf@gitster.dls.corp.google.com","subject":"[PATCH v3] reachable.c: add HEAD to reachability starting commits","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-09-03T16:14:10Z","receivedAt":"2014-09-03T16:14:10Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"HEAD is not explicitly used as a starting commit for\ncalculating reachability, so if it's detached and reflogs\nare disabled it may be pruned.\n\nAdd tests which demonstrate it. Test 'prune: prune former HEAD after checking\nout branch' also reverts changes to repository.\n\nSigned-off-by: Max Kirillov <max@max630.net>\n---\nFixed the notes.\n\n(This filesystem stuff would not be needed if I had a separated repository\nwithout reflogs from scratch, like in v1. Having to handle leftouts from\nprevious test cases does not add clarity here. Maybe it worth extracting all\ncases with no reflogs into a separated script.)\n reachable.c      |  3 +++\n t/t5304-prune.sh | 22 ++++++++++++++++++++++\n 2 files changed, 25 insertions(+)\n\ndiff --git a/reachable.c b/reachable.c\nindex 654a8c5..6f6835b 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -229,6 +229,9 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t/* Add all external refs */\n \tfor_each_ref(add_one_ref, revs);\n \n+\t/* detached HEAD is not included in the list above */\n+\thead_ref(add_one_ref, revs);\n+\n \t/* Add all reflog info */\n \tif (mark_reflog)\n \t\tfor_each_reflog(add_one_reflog, revs);\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 377d3d3..01c6a3f 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -104,6 +104,28 @@ test_expect_success 'prune: prune unreachable heads' '\n \n '\n \n+test_expect_success 'prune: do not prune detached HEAD with no reflog' '\n+\n+\tgit checkout --detach --quiet &&\n+\tgit commit --allow-empty -m \"detached commit\" &&\n+\t# verify that there is no reflogs\n+\t# (should be removed and disabled by previous test)\n+\ttest ! -e .git/logs &&\n+\tgit prune -n >prune_actual &&\n+\t: >prune_expected &&\n+\ttest_cmp prune_actual prune_expected\n+\n+'\n+\n+test_expect_success 'prune: prune former HEAD after checking out branch' '\n+\n+\thead_sha1=$(git rev-parse HEAD) &&\n+\tgit checkout --quiet master &&\n+\tgit prune -v >prune_actual &&\n+\tgrep \"$head_sha1\" prune_actual\n+\n+'\n+\n test_expect_success 'prune: do not prune heads listed as an argument' '\n \n \t: > file2 &&\n-- \n2.0.1.1697.g73c6810\n"}]}