{"thread":{"id":"40418","subject":"[PATCH 0/4] Fix gc failure when a remote HEAD goes stale","startedAt":"2015-09-24T09:13:24Z","lastAt":"2015-10-08T20:10:37Z","messageCount":28,"participants":["Johannes Schindelin","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"270629","messageId":"cover.1443085919.git.johannes.schindelin@gmx.de","threadId":"40418","inReplyTo":null,"subject":"[PATCH 0/4] Fix gc failure when a remote HEAD goes stale","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-09-24T09:13:24Z","receivedAt":"2015-09-24T09:13:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"There has been a report in the Git for Windows project that gc fails\nsometimes: https://github.com/git-for-windows/git/issues/423\n\nIt turns out that there are cases when a remote HEAD can go stale and\nit is not the user's fault at all. It can happen, for example, if the\nactive branch in the remote repository gets renamed.\n\nGit's garbage collector should handle this gracefully. The best this\ndeveloper could come up with, is to simply ignore and delete the\nnow-broken refs.\n\n\nJohannes Schindelin (4):\n  gc: demonstrate failure with stale remote HEAD\n  pack-objects: do not get distracted by stale refs\n  mark_reachable_objects(): optionally collect broken refs\n  gc: remove broken refs\n\n builtin/pack-objects.c |  1 +\n builtin/prune.c        | 12 +++++++++++-\n builtin/reflog.c       |  2 +-\n reachable.c            | 26 ++++++++++++++++++++------\n reachable.h            |  3 ++-\n t/t6500-gc.sh          | 15 +++++++++++++++\n 6 files changed, 50 insertions(+), 9 deletions(-)\n\n-- \n2.5.2.windows.2\n"},{"id":"270631","messageId":"85350603b9f5986381777c386415aff2005ca498.1443085919.git.johannes.schindelin@gmx.de","threadId":"40418","inReplyTo":"cover.1443085919.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/4] gc: demonstrate failure with stale remote HEAD","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-09-24T09:13:35Z","receivedAt":"2015-09-24T09:13:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n t/t6500-gc.sh | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 63194d8..b736774 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -30,4 +30,19 @@ test_expect_success 'gc -h with invalid configuration' '\n \ttest_i18ngrep \"[Uu]sage\" broken/usage\n '\n \n+test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '\n+\tgit init remote &&\n+\t(\n+\t\tcd remote &&\n+\t\ttest_commit initial &&\n+\t\tgit clone . ../client &&\n+\t\tgit branch -m develop &&\n+\t\tcd ../client &&\n+\t\tgit fetch --prune &&\n+\t\tgit gc &&\n+\t\tgit branch --list -r origin/HEAD >actual &&\n+\t\ttest_line_count = 0 actual\n+\t)\n+'\n+\n test_done\n-- \n2.5.2.windows.2\n"},{"id":"270630","messageId":"a3a883ce9d7a8bf4220beb640caca5bed0f7d492.1443085919.git.johannes.schindelin@gmx.de","threadId":"40418","inReplyTo":"cover.1443085919.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/4] pack-objects: do not get distracted by stale refs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-09-24T09:13:44Z","receivedAt":"2015-09-24T09:13:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It is quite possible for, say, a remote HEAD to become stale, e.g. when\nthe default branch was renamed.\n\nWe should still be able to pack our objects when such a thing happens;\nsimply ignore invalid refs (because they cannot matter for the packing\nprocess anyway).\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/pack-objects.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1c63f8f..ef2f794 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2499,6 +2499,7 @@ static void get_object_list(int ac, const char **av)\n \tint flags = 0;\n \n \tinit_revisions(&revs, NULL);\n+\trevs.ignore_missing = 1;\n \tsave_commit_buffer = 0;\n \tsetup_revisions(ac, av, &revs, NULL);\n \n-- \n2.5.2.windows.2\n"},{"id":"270632","messageId":"bcceeae4c8f7f690502651dd6cc537b2f4918fac.1443085919.git.johannes.schindelin@gmx.de","threadId":"40418","inReplyTo":"cover.1443085919.git.johannes.schindelin@gmx.de","subject":"[PATCH 3/4] mark_reachable_objects(): optionally collect broken refs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-09-24T09:13:52Z","receivedAt":"2015-09-24T09:13:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The behavior of `mark_reachable_objects()` without this patch is that it\ndies if it encounters a broken ref. This is sometimes undesirable, e.g.\nwhen garbage collecting in a repository with a stale remote HEAD.\n\nSo let's introduce an optional parameter to collect such broken refs. The\nbehavior of the function is unchanged if that parameter is `NULL`.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/prune.c  |  2 +-\n builtin/reflog.c |  2 +-\n reachable.c      | 26 ++++++++++++++++++++------\n reachable.h      |  3 ++-\n 4 files changed, 24 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 10b03d3..d6f664f 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -136,7 +136,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \tif (show_progress)\n \t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n \n-\tmark_reachable_objects(&revs, 1, expire, progress);\n+\tmark_reachable_objects(&revs, 1, expire, progress, NULL);\n \tstop_progress(&progress);\n \tfor_each_loose_file_in_objdir(get_object_directory(), prune_object,\n \t\t\t\t      prune_cruft, prune_subdir, NULL);\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex f96ca2a..cb8758a 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -583,7 +583,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tinit_revisions(&cb.cmd.revs, prefix);\n \t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n \t\t\tprintf(\"Marking reachable objects...\");\n-\t\tmark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);\n+\t\tmark_reachable_objects(&cb.cmd.revs, 0, 0, NULL, NULL);\n \t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n \t\t\tputchar('\\n');\n \t}\ndiff --git a/reachable.c b/reachable.c\nindex 9cff25b..1fc7ada 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -15,6 +15,11 @@ struct connectivity_progress {\n \tunsigned long count;\n };\n \n+struct add_one_data {\n+\tstruct rev_info *revs;\n+\tstruct string_list *broken_refs;\n+};\n+\n static void update_progress(struct connectivity_progress *cp)\n {\n \tcp->count++;\n@@ -25,10 +30,14 @@ static void update_progress(struct connectivity_progress *cp)\n static int add_one_ref(const char *path, const struct object_id *oid,\n \t\t       int flag, void *cb_data)\n {\n-\tstruct object *object = parse_object_or_die(oid->hash, path);\n-\tstruct rev_info *revs = (struct rev_info *)cb_data;\n+\tstruct add_one_data *data = (struct add_one_data *)cb_data;\n+\tstruct object *object = data->broken_refs ? parse_object(oid->hash) :\n+\t\tparse_object_or_die(oid->hash, path);\n \n-\tadd_pending_object(revs, object, \"\");\n+\tif (!object)\n+\t\tstring_list_append(data->broken_refs, path);\n+\telse\n+\t\tadd_pending_object(data->revs, object, \"\");\n \n \treturn 0;\n }\n@@ -153,9 +162,11 @@ int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n \n void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t\t\t    unsigned long mark_recent,\n-\t\t\t    struct progress *progress)\n+\t\t\t    struct progress *progress,\n+\t\t\t    struct string_list *broken_refs)\n {\n \tstruct connectivity_progress cp;\n+\tstruct add_one_data data;\n \n \t/*\n \t * Set up revision parsing, and mark us as being interested\n@@ -168,11 +179,14 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t/* Add all refs from the index file */\n \tadd_index_objects_to_pending(revs, 0);\n \n+\tdata.revs = revs;\n+\tdata.broken_refs = broken_refs;\n+\n \t/* Add all external refs */\n-\tfor_each_ref(add_one_ref, revs);\n+\tfor_each_ref(add_one_ref, &data);\n \n \t/* detached HEAD is not included in the list above */\n-\thead_ref(add_one_ref, revs);\n+\thead_ref(add_one_ref, &data);\n \n \t/* Add all reflog info */\n \tif (mark_reflog)\ndiff --git a/reachable.h b/reachable.h\nindex d23efc3..39de1c7 100644\n--- a/reachable.h\n+++ b/reachable.h\n@@ -5,6 +5,7 @@ struct progress;\n extern int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n \t\t\t\t\t\t  unsigned long timestamp);\n extern void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n-\t\t\t\t   unsigned long mark_recent, struct progress *);\n+\t\t\t\t   unsigned long mark_recent, struct progress *,\n+\t\t\t\t   struct string_list *broken_refs);\n \n #endif\n-- \n2.5.2.windows.2\n"},{"id":"270633","messageId":"8855bae256311446dffd221ab33c1163eb104c4a.1443085919.git.johannes.schindelin@gmx.de","threadId":"40418","inReplyTo":"cover.1443085919.git.johannes.schindelin@gmx.de","subject":"[PATCH 4/4] gc: remove broken refs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-09-24T09:14:02Z","receivedAt":"2015-09-24T09:14:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When encountering broken refs, such as a stale remote HEAD (which can\nhappen if the active branch was renamed in the remote), it is more\nhelpful to remove those refs than to exit with an error.\n\nThis fixes https://github.com/git-for-windows/git/issues/423\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/prune.c | 12 +++++++++++-\n t/t6500-gc.sh   |  2 +-\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex d6f664f..1a30f65 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -6,6 +6,7 @@\n #include \"reachable.h\"\n #include \"parse-options.h\"\n #include \"progress.h\"\n+#include \"refs.h\"\n \n static const char * const prune_usage[] = {\n \tN_(\"git prune [-n] [-v] [--expire <time>] [--] [<head>...]\"),\n@@ -100,6 +101,7 @@ static void remove_temporary_files(const char *path)\n int cmd_prune(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n+\tstruct string_list broken_refs = STRING_LIST_INIT_DUP;\n \tstruct progress *progress = NULL;\n \tconst struct option options[] = {\n \t\tOPT__DRY_RUN(&show_only, N_(\"do not remove, show only\")),\n@@ -110,6 +112,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \tchar *s;\n+\tint i;\n \n \texpire = ULONG_MAX;\n \tsave_commit_buffer = 0;\n@@ -136,7 +139,14 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \tif (show_progress)\n \t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n \n-\tmark_reachable_objects(&revs, 1, expire, progress, NULL);\n+\trevs.ignore_missing = 1;\n+\tmark_reachable_objects(&revs, 1, expire, progress, &broken_refs);\n+\tfor (i = 0; i < broken_refs.nr; i++) {\n+\t\tchar *path = broken_refs.items[i].string;\n+\t\tprintf(\"Removing stale ref %s\\n\", path);\n+\t\tif (!show_only && delete_ref(path, NULL, REF_NODEREF))\n+\t\t\tdie(\"Could not remove stale ref %s\", path);\n+\t}\n \tstop_progress(&progress);\n \tfor_each_loose_file_in_objdir(get_object_directory(), prune_object,\n \t\t\t\t      prune_cruft, prune_subdir, NULL);\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex b736774..0ae4271 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -30,7 +30,7 @@ test_expect_success 'gc -h with invalid configuration' '\n \ttest_i18ngrep \"[Uu]sage\" broken/usage\n '\n \n-test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '\n+test_expect_success 'gc removes broken refs/remotes/<name>/HEAD' '\n \tgit init remote &&\n \t(\n \t\tcd remote &&\n-- \n2.5.2.windows.2\n"},{"id":"270638","messageId":"20150924170320.GA14695@sigill.intra.peff.net","threadId":"40418","inReplyTo":"a3a883ce9d7a8bf4220beb640caca5bed0f7d492.1443085919.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/4] pack-objects: do not get distracted by stale refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-24T17:03:21Z","receivedAt":"2015-09-24T17:03:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 24, 2015 at 11:13:44AM +0200, Johannes Schindelin wrote:\n\n> It is quite possible for, say, a remote HEAD to become stale, e.g. when\n> the default branch was renamed.\n> \n> We should still be able to pack our objects when such a thing happens;\n> simply ignore invalid refs (because they cannot matter for the packing\n> process anyway).\n> [...]\n>  \tinit_revisions(&revs, NULL);\n> +\trevs.ignore_missing = 1;\n\nI think this is dangerous, because a repack may drop unreferenced\nhistory. Imagine you have a corrupted repository where you are missing\nthe commit at the tip of \"master\", but still have \"master^\" and the rest\nof the history, and \"git gc --auto\" triggers.\n\nRight now, pack-objects will barf and refuse to pack. Your repo is still\ncorrupted, but you can use git-fsck to recover the history minus the\nsingle commit. With your patch, the repack will delete the entire\nhistory. Of course there are complications like other branches\nreferencing the history, reflogs if it is a non-bare repo, and the\n2-week expiration time. But there's definitely a potential to make\nthings worse. I think commands that drop objects (like repack and prune)\nneed to be more careful than other commands about refusing to run when\nthere is something fishy in the repository.\n\nI think we would want to introduce a flag to ignore dangling\nsymbolic refs when running for_each_ref, as those should be harmless. In\nfact, I think we could omit symrefs completely for reachability\nanalysis; we would hit the ref they refer to anyway during the rest of\nthe traversal.\n\n-Peff\n"},{"id":"270640","messageId":"20150924175615.GA15117@sigill.intra.peff.net","threadId":"40418","inReplyTo":"bcceeae4c8f7f690502651dd6cc537b2f4918fac.1443085919.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 3/4] mark_reachable_objects(): optionally collect broken refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-24T17:56:15Z","receivedAt":"2015-09-24T17:56:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 24, 2015 at 11:13:52AM +0200, Johannes Schindelin wrote:\n\n> The behavior of `mark_reachable_objects()` without this patch is that it\n> dies if it encounters a broken ref. This is sometimes undesirable, e.g.\n> when garbage collecting in a repository with a stale remote HEAD.\n> \n> So let's introduce an optional parameter to collect such broken refs. The\n> behavior of the function is unchanged if that parameter is `NULL`.\n\nSimilar comment to the last one. :)\n\nI suspect the issues you are seeing are largely new due to the\nref-paranoia work I did (merged in 05e816e37). We used to ignore broken\nrefs at the for_each_ref() level, but now we feed them to the calling\ncode (which generally chokes).\n\nSo in that sense, a simpler fix than your series would be to simply\nrevert 8d42299 and ff4056bbc. :)\n\nBut I think those new checks are valuable, and we really just need to\ngracefully ignore the dangling-symref case (which is _a_ breakage, but\nnot a dangerous one).\n\n-Peff\n"},{"id":"270641","messageId":"20150924175758.GB15117@sigill.intra.peff.net","threadId":"40418","inReplyTo":"8855bae256311446dffd221ab33c1163eb104c4a.1443085919.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 4/4] gc: remove broken refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-24T17:57:59Z","receivedAt":"2015-09-24T17:57:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 24, 2015 at 11:14:02AM +0200, Johannes Schindelin wrote:\n\n> When encountering broken refs, such as a stale remote HEAD (which can\n> happen if the active branch was renamed in the remote), it is more\n> helpful to remove those refs than to exit with an error.\n\nFor the same reasons as in my earlier responses, I think it's dangerous\nto remove broken refs (it makes a small corruption much worse). It seems\nreasonably sane to remove a dangling symref, though if we teach\nfor_each_ref to gracefully skip them, then leaving them in place isn't a\nproblem.\n\n-Peff\n"},{"id":"270727","messageId":"xmqqr3lnuzqu.fsf@gitster.mtv.corp.google.com","threadId":"40418","inReplyTo":"20150924175758.GB15117@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] gc: remove broken refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-25T00:08:41Z","receivedAt":"2015-09-25T00:08:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> For the same reasons as in my earlier responses, I think it's dangerous\n> to remove broken refs (it makes a small corruption much worse). It seems\n> reasonably sane to remove a dangling symref, though if we teach\n> for_each_ref to gracefully skip them, then leaving them in place isn't a\n> problem.\n\nOne thing I wondered was if we can reliably tell between a ref that\nwanted to be a real ref that records a broken object name and a ref\nthat wanted to be a symbolic ref that points a bogus thing, and if\nwe can't, should we worry about it too much.  The former is more\nserious, as the history behind the commit it wanted to but failed to\nrecord is at risk of being pruned.\n\nOne case that is clearly safe is \"ref: refs/heads/gone\"; it is not\nlikely to be the result of attempting to write a real object name\ngone bad by whatever filesystem corruption.  On the other hand, an\nobviously problematic case is an empty file.  We cannot tell if the\n\"broken\" ref used to anchor the tip of a real history (which is\nabout to be lost with Dscho's patch 1/4) or was merely pointing at\nanother ref (which will not harm the object database if ignored).\n\nSo the rule should be\n\n    If resolve_ref_unsafe_1() says it is a symbolic ref, if\n    check_ref_format() is OK with the ref it points at, and if that\n    pointee is missing, then it is safe to skip.\n\nAll other funnies should trigger the safety.\n\nThe collection of \"broken and can be removed\" refs introduced by 3/4\nmay also have to take that into account, I think.\n"},{"id":"270735","messageId":"20150925013537.GA7522@sigill.intra.peff.net","threadId":"40418","inReplyTo":"xmqqr3lnuzqu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/4] gc: remove broken refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-25T01:35:37Z","receivedAt":"2015-09-25T01:35:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 24, 2015 at 05:08:41PM -0700, Junio C Hamano wrote:\n\n> One thing I wondered was if we can reliably tell between a ref that\n> wanted to be a real ref that records a broken object name and a ref\n> that wanted to be a symbolic ref that points a bogus thing, and if\n> we can't, should we worry about it too much.  The former is more\n> serious, as the history behind the commit it wanted to but failed to\n> record is at risk of being pruned.\n> \n> One case that is clearly safe is \"ref: refs/heads/gone\"; it is not\n> likely to be the result of attempting to write a real object name\n> gone bad by whatever filesystem corruption.  On the other hand, an\n> obviously problematic case is an empty file.  We cannot tell if the\n> \"broken\" ref used to anchor the tip of a real history (which is\n> about to be lost with Dscho's patch 1/4) or was merely pointing at\n> another ref (which will not harm the object database if ignored).\n> \n> So the rule should be\n> \n>     If resolve_ref_unsafe_1() says it is a symbolic ref, if\n>     check_ref_format() is OK with the ref it points at, and if that\n>     pointee is missing, then it is safe to skip.\n> \n> All other funnies should trigger the safety.\n\nRight, I agree with that rule. If we don't know what it is, we should\nerr on the conservative side (and that _shouldn't_ happen, because does\nnot generate files in .git/refs that it cannot itself understand). But\n\"ref: \" is clearly something we understand.\n\n-Peff\n"},{"id":"270812","messageId":"cover.1443448748.git.johannes.schindelin@gmx.de","threadId":"40418","inReplyTo":"xmqqr3lnuzqu.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2 0/4] Fix gc failure when a remote HEAD goes stale","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-09-28T14:01:00Z","receivedAt":"2015-09-28T14:01:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"There has been a report in the Git for Windows project that gc fails\nsometimes: https://github.com/git-for-windows/git/issues/423\n\nIt turns out that there are cases when a remote HEAD can go stale and\nit is not the user's fault at all. It can happen, for example, if the\nactive branch in the remote repository gets renamed.\n\nGit's garbage collector should handle this gracefully. The best this\ndeveloper could come up with, is to simply warn and delete broken\nsymrefs.\n\nThanks to Junio and Peff for their really valuable sanity check.\n\nInterdiff vs v1 below diffstat.\n\nJohannes Schindelin (4):\n  gc: demonstrate failure with stale remote HEAD\n  pack-objects: do not get distracted by broken symrefs\n  mark_reachable_objects(): optionally collect broken symrefs\n  gc: remove broken refs\n\n builtin/prune.c  | 12 +++++++++++-\n builtin/reflog.c |  2 +-\n reachable.c      | 31 +++++++++++++++++++++++++------\n reachable.h      |  3 ++-\n t/t6500-gc.sh    | 15 +++++++++++++++\n 5 files changed, 54 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex ef2f794..1c63f8f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2499,7 +2499,6 @@ static void get_object_list(int ac, const char **av)\n \tint flags = 0;\n \n \tinit_revisions(&revs, NULL);\n-\trevs.ignore_missing = 1;\n \tsave_commit_buffer = 0;\n \tsetup_revisions(ac, av, &revs, NULL);\n \ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 1a30f65..337b12a 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -101,7 +101,7 @@ static void remove_temporary_files(const char *path)\n int cmd_prune(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n-\tstruct string_list broken_refs = STRING_LIST_INIT_DUP;\n+\tstruct string_list broken_symrefs = STRING_LIST_INIT_DUP;\n \tstruct progress *progress = NULL;\n \tconst struct option options[] = {\n \t\tOPT__DRY_RUN(&show_only, N_(\"do not remove, show only\")),\n@@ -140,9 +140,9 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n \n \trevs.ignore_missing = 1;\n-\tmark_reachable_objects(&revs, 1, expire, progress, &broken_refs);\n-\tfor (i = 0; i < broken_refs.nr; i++) {\n-\t\tchar *path = broken_refs.items[i].string;\n+\tmark_reachable_objects(&revs, 1, expire, progress, &broken_symrefs);\n+\tfor (i = 0; i < broken_symrefs.nr; i++) {\n+\t\tchar *path = broken_symrefs.items[i].string;\n \t\tprintf(\"Removing stale ref %s\\n\", path);\n \t\tif (!show_only && delete_ref(path, NULL, REF_NODEREF))\n \t\t\tdie(\"Could not remove stale ref %s\", path);\ndiff --git a/reachable.c b/reachable.c\nindex 1fc7ada..25c4932 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -17,7 +17,7 @@ struct connectivity_progress {\n \n struct add_one_data {\n \tstruct rev_info *revs;\n-\tstruct string_list *broken_refs;\n+\tstruct string_list *broken_symrefs;\n };\n \n static void update_progress(struct connectivity_progress *cp)\n@@ -31,13 +31,18 @@ static int add_one_ref(const char *path, const struct object_id *oid,\n \t\t       int flag, void *cb_data)\n {\n \tstruct add_one_data *data = (struct add_one_data *)cb_data;\n-\tstruct object *object = data->broken_refs ? parse_object(oid->hash) :\n-\t\tparse_object_or_die(oid->hash, path);\n+\tstruct object *object;\n \n-\tif (!object)\n-\t\tstring_list_append(data->broken_refs, path);\n-\telse\n-\t\tadd_pending_object(data->revs, object, \"\");\n+\tif ((flag & REF_ISSYMREF) && (flag & REF_ISBROKEN)) {\n+\t\tif (data->broken_symrefs)\n+\t\t\tstring_list_append(data->broken_symrefs, path);\n+\t\telse\n+\t\t\twarning(\"ref is broken: %s\", path);\n+\t\treturn 0;\n+\t}\n+\n+\tobject = parse_object_or_die(oid->hash, path);\n+\tadd_pending_object(data->revs, object, \"\");\n \n \treturn 0;\n }\n@@ -163,7 +168,7 @@ int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t\t\t    unsigned long mark_recent,\n \t\t\t    struct progress *progress,\n-\t\t\t    struct string_list *broken_refs)\n+\t\t\t    struct string_list *broken_symrefs)\n {\n \tstruct connectivity_progress cp;\n \tstruct add_one_data data;\n@@ -180,7 +185,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \tadd_index_objects_to_pending(revs, 0);\n \n \tdata.revs = revs;\n-\tdata.broken_refs = broken_refs;\n+\tdata.broken_symrefs = broken_symrefs;\n \n \t/* Add all external refs */\n \tfor_each_ref(add_one_ref, &data);\ndiff --git a/reachable.h b/reachable.h\nindex 39de1c7..06f1400 100644\n--- a/reachable.h\n+++ b/reachable.h\n@@ -6,6 +6,6 @@ extern int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n \t\t\t\t\t\t  unsigned long timestamp);\n extern void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t\t\t\t   unsigned long mark_recent, struct progress *,\n-\t\t\t\t   struct string_list *broken_refs);\n+\t\t\t\t   struct string_list *broken_symrefs);\n \n #endif\n-- \n2.5.3.windows.1.3.gc322723\n"},{"id":"270813","messageId":"85350603b9f5986381777c386415aff2005ca498.1443448748.git.johannes.schindelin@gmx.de","threadId":"40418","inReplyTo":"cover.1443448748.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 1/4] gc: demonstrate failure with stale remote HEAD","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-09-28T14:01:13Z","receivedAt":"2015-09-28T14:01:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n t/t6500-gc.sh | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 63194d8..b736774 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -30,4 +30,19 @@ test_expect_success 'gc -h with invalid configuration' '\n \ttest_i18ngrep \"[Uu]sage\" broken/usage\n '\n \n+test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '\n+\tgit init remote &&\n+\t(\n+\t\tcd remote &&\n+\t\ttest_commit initial &&\n+\t\tgit clone . ../client &&\n+\t\tgit branch -m develop &&\n+\t\tcd ../client &&\n+\t\tgit fetch --prune &&\n+\t\tgit gc &&\n+\t\tgit branch --list -r origin/HEAD >actual &&\n+\t\ttest_line_count = 0 actual\n+\t)\n+'\n+\n test_done\n-- \n2.5.3.windows.1.3.gc322723\n"},{"id":"270814","messageId":"b76346dee63408c37c8afe5366ce7133cdf29be2.1443448748.git.johannes.schindelin@gmx.de","threadId":"40418","inReplyTo":"cover.1443448748.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 2/4] pack-objects: do not get distracted by broken symrefs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-09-28T14:01:25Z","receivedAt":"2015-09-28T14:01:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It is quite possible for, say, a remote HEAD to become broken, e.g.\nwhen the default branch was renamed.\n\nWe should still be able to pack our objects when such a thing happens;\nsimply ignore broken symrefs (because they cannot matter for the packing\nprocess anyway).\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n reachable.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex 9cff25b..64289f3 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -25,9 +25,15 @@ static void update_progress(struct connectivity_progress *cp)\n static int add_one_ref(const char *path, const struct object_id *oid,\n \t\t       int flag, void *cb_data)\n {\n-\tstruct object *object = parse_object_or_die(oid->hash, path);\n \tstruct rev_info *revs = (struct rev_info *)cb_data;\n+\tstruct object *object;\n \n+\tif ((flag & REF_ISSYMREF) && (flag & REF_ISBROKEN)) {\n+\t\twarning(\"ref is broken: %s\", path);\n+\t\treturn 0;\n+\t}\n+\n+\tobject = parse_object_or_die(oid->hash, path);\n \tadd_pending_object(revs, object, \"\");\n \n \treturn 0;\n-- \n2.5.3.windows.1.3.gc322723\n"},{"id":"270815","messageId":"b0403395b261a70b7e0d31e02420930adc8d97ea.1443448748.git.johannes.schindelin@gmx.de","threadId":"40418","inReplyTo":"cover.1443448748.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 3/4] mark_reachable_objects(): optionally collect broken symrefs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-09-28T14:01:35Z","receivedAt":"2015-09-28T14:01:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The behavior of `mark_reachable_objects()` without this patch is that\nit warns if it encounters a broken symref. Sometimes the caller wants\nto act upon broken symrefs, e.g. when garbage collecting in a repository\nwith a broken remote HEAD.\n\nSo let's introduce an optional parameter to collect broken symrefs. The\nbehavior of the function is unchanged if that parameter is `NULL`.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/prune.c  |  2 +-\n builtin/reflog.c |  2 +-\n reachable.c      | 25 +++++++++++++++++++------\n reachable.h      |  3 ++-\n 4 files changed, 23 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 10b03d3..d6f664f 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -136,7 +136,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \tif (show_progress)\n \t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n \n-\tmark_reachable_objects(&revs, 1, expire, progress);\n+\tmark_reachable_objects(&revs, 1, expire, progress, NULL);\n \tstop_progress(&progress);\n \tfor_each_loose_file_in_objdir(get_object_directory(), prune_object,\n \t\t\t\t      prune_cruft, prune_subdir, NULL);\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex f96ca2a..cb8758a 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -583,7 +583,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tinit_revisions(&cb.cmd.revs, prefix);\n \t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n \t\t\tprintf(\"Marking reachable objects...\");\n-\t\tmark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);\n+\t\tmark_reachable_objects(&cb.cmd.revs, 0, 0, NULL, NULL);\n \t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n \t\t\tputchar('\\n');\n \t}\ndiff --git a/reachable.c b/reachable.c\nindex 64289f3..25c4932 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -15,6 +15,11 @@ struct connectivity_progress {\n \tunsigned long count;\n };\n \n+struct add_one_data {\n+\tstruct rev_info *revs;\n+\tstruct string_list *broken_symrefs;\n+};\n+\n static void update_progress(struct connectivity_progress *cp)\n {\n \tcp->count++;\n@@ -25,16 +30,19 @@ static void update_progress(struct connectivity_progress *cp)\n static int add_one_ref(const char *path, const struct object_id *oid,\n \t\t       int flag, void *cb_data)\n {\n-\tstruct rev_info *revs = (struct rev_info *)cb_data;\n+\tstruct add_one_data *data = (struct add_one_data *)cb_data;\n \tstruct object *object;\n \n \tif ((flag & REF_ISSYMREF) && (flag & REF_ISBROKEN)) {\n-\t\twarning(\"ref is broken: %s\", path);\n+\t\tif (data->broken_symrefs)\n+\t\t\tstring_list_append(data->broken_symrefs, path);\n+\t\telse\n+\t\t\twarning(\"ref is broken: %s\", path);\n \t\treturn 0;\n \t}\n \n \tobject = parse_object_or_die(oid->hash, path);\n-\tadd_pending_object(revs, object, \"\");\n+\tadd_pending_object(data->revs, object, \"\");\n \n \treturn 0;\n }\n@@ -159,9 +167,11 @@ int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n \n void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t\t\t    unsigned long mark_recent,\n-\t\t\t    struct progress *progress)\n+\t\t\t    struct progress *progress,\n+\t\t\t    struct string_list *broken_symrefs)\n {\n \tstruct connectivity_progress cp;\n+\tstruct add_one_data data;\n \n \t/*\n \t * Set up revision parsing, and mark us as being interested\n@@ -174,11 +184,14 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t/* Add all refs from the index file */\n \tadd_index_objects_to_pending(revs, 0);\n \n+\tdata.revs = revs;\n+\tdata.broken_symrefs = broken_symrefs;\n+\n \t/* Add all external refs */\n-\tfor_each_ref(add_one_ref, revs);\n+\tfor_each_ref(add_one_ref, &data);\n \n \t/* detached HEAD is not included in the list above */\n-\thead_ref(add_one_ref, revs);\n+\thead_ref(add_one_ref, &data);\n \n \t/* Add all reflog info */\n \tif (mark_reflog)\ndiff --git a/reachable.h b/reachable.h\nindex d23efc3..06f1400 100644\n--- a/reachable.h\n+++ b/reachable.h\n@@ -5,6 +5,7 @@ struct progress;\n extern int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n \t\t\t\t\t\t  unsigned long timestamp);\n extern void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n-\t\t\t\t   unsigned long mark_recent, struct progress *);\n+\t\t\t\t   unsigned long mark_recent, struct progress *,\n+\t\t\t\t   struct string_list *broken_symrefs);\n \n #endif\n-- \n2.5.3.windows.1.3.gc322723\n"},{"id":"270816","messageId":"fb29b39adf14a702c5127d722d86647a3dc9c891.1443448748.git.johannes.schindelin@gmx.de","threadId":"40418","inReplyTo":"cover.1443448748.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 4/4] gc: remove broken symrefs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-09-28T14:02:08Z","receivedAt":"2015-09-28T14:02:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When encountering broken symrefs, such as a stale remote HEAD (which can\nhappen if the active branch was renamed in the remote), it is more\nhelpful to remove those symrefs than to exit with an error.\n\nThis fixes https://github.com/git-for-windows/git/issues/423\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/prune.c | 12 +++++++++++-\n t/t6500-gc.sh   |  2 +-\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex d6f664f..337b12a 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -6,6 +6,7 @@\n #include \"reachable.h\"\n #include \"parse-options.h\"\n #include \"progress.h\"\n+#include \"refs.h\"\n \n static const char * const prune_usage[] = {\n \tN_(\"git prune [-n] [-v] [--expire <time>] [--] [<head>...]\"),\n@@ -100,6 +101,7 @@ static void remove_temporary_files(const char *path)\n int cmd_prune(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n+\tstruct string_list broken_symrefs = STRING_LIST_INIT_DUP;\n \tstruct progress *progress = NULL;\n \tconst struct option options[] = {\n \t\tOPT__DRY_RUN(&show_only, N_(\"do not remove, show only\")),\n@@ -110,6 +112,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \tchar *s;\n+\tint i;\n \n \texpire = ULONG_MAX;\n \tsave_commit_buffer = 0;\n@@ -136,7 +139,14 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \tif (show_progress)\n \t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n \n-\tmark_reachable_objects(&revs, 1, expire, progress, NULL);\n+\trevs.ignore_missing = 1;\n+\tmark_reachable_objects(&revs, 1, expire, progress, &broken_symrefs);\n+\tfor (i = 0; i < broken_symrefs.nr; i++) {\n+\t\tchar *path = broken_symrefs.items[i].string;\n+\t\tprintf(\"Removing stale ref %s\\n\", path);\n+\t\tif (!show_only && delete_ref(path, NULL, REF_NODEREF))\n+\t\t\tdie(\"Could not remove stale ref %s\", path);\n+\t}\n \tstop_progress(&progress);\n \tfor_each_loose_file_in_objdir(get_object_directory(), prune_object,\n \t\t\t\t      prune_cruft, prune_subdir, NULL);\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex b736774..0ae4271 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -30,7 +30,7 @@ test_expect_success 'gc -h with invalid configuration' '\n \ttest_i18ngrep \"[Uu]sage\" broken/usage\n '\n \n-test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '\n+test_expect_success 'gc removes broken refs/remotes/<name>/HEAD' '\n \tgit init remote &&\n \t(\n \t\tcd remote &&\n-- \n2.5.3.windows.1.3.gc322723\n"},{"id":"270829","messageId":"xmqqpp12croq.fsf@gitster.mtv.corp.google.com","threadId":"40418","inReplyTo":"fb29b39adf14a702c5127d722d86647a3dc9c891.1443448748.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 4/4] gc: remove broken symrefs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-28T18:41:25Z","receivedAt":"2015-09-28T18:41:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> When encountering broken symrefs, such as a stale remote HEAD (which can\n> happen if the active branch was renamed in the remote), it is more\n> helpful to remove those symrefs than to exit with an error.\n\nI think this depends on the perspective.  One side of me says that a\nremote HEAD that points at refs/remotes/origin/topic that no longer\nexists is still giving me a valuable information and it should take\na conscious action by the user to remove it, or evne better to\nrepoint it to a more useful place.  And from that point of view,\nremoving is not all that helpful.  Keeping them and not allowing\nthem to exit with an error would be a real improvement.\n\nOn the other hand, I can certainly understand a view that considers\nthat such a dangling symbolic ref is merely a cruft like any other\ncruft, and \"gc\" is all about removing cruft.\n\nIt just feels to me that this is a bit more valuable than other\nkinds of cruft, but maybe it is just me.\n\n\n\n>\n> This fixes https://github.com/git-for-windows/git/issues/423\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>  builtin/prune.c | 12 +++++++++++-\n>  t/t6500-gc.sh   |  2 +-\n>  2 files changed, 12 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/prune.c b/builtin/prune.c\n> index d6f664f..337b12a 100644\n> --- a/builtin/prune.c\n> +++ b/builtin/prune.c\n> @@ -6,6 +6,7 @@\n>  #include \"reachable.h\"\n>  #include \"parse-options.h\"\n>  #include \"progress.h\"\n> +#include \"refs.h\"\n>  \n>  static const char * const prune_usage[] = {\n>  \tN_(\"git prune [-n] [-v] [--expire <time>] [--] [<head>...]\"),\n> @@ -100,6 +101,7 @@ static void remove_temporary_files(const char *path)\n>  int cmd_prune(int argc, const char **argv, const char *prefix)\n>  {\n>  \tstruct rev_info revs;\n> +\tstruct string_list broken_symrefs = STRING_LIST_INIT_DUP;\n>  \tstruct progress *progress = NULL;\n>  \tconst struct option options[] = {\n>  \t\tOPT__DRY_RUN(&show_only, N_(\"do not remove, show only\")),\n> @@ -110,6 +112,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n>  \t\tOPT_END()\n>  \t};\n>  \tchar *s;\n> +\tint i;\n>  \n>  \texpire = ULONG_MAX;\n>  \tsave_commit_buffer = 0;\n> @@ -136,7 +139,14 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n>  \tif (show_progress)\n>  \t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n>  \n> -\tmark_reachable_objects(&revs, 1, expire, progress, NULL);\n> +\trevs.ignore_missing = 1;\n> +\tmark_reachable_objects(&revs, 1, expire, progress, &broken_symrefs);\n> +\tfor (i = 0; i < broken_symrefs.nr; i++) {\n> +\t\tchar *path = broken_symrefs.items[i].string;\n> +\t\tprintf(\"Removing stale ref %s\\n\", path);\n> +\t\tif (!show_only && delete_ref(path, NULL, REF_NODEREF))\n> +\t\t\tdie(\"Could not remove stale ref %s\", path);\n> +\t}\n>  \tstop_progress(&progress);\n>  \tfor_each_loose_file_in_objdir(get_object_directory(), prune_object,\n>  \t\t\t\t      prune_cruft, prune_subdir, NULL);\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index b736774..0ae4271 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -30,7 +30,7 @@ test_expect_success 'gc -h with invalid configuration' '\n>  \ttest_i18ngrep \"[Uu]sage\" broken/usage\n>  '\n>  \n> -test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '\n> +test_expect_success 'gc removes broken refs/remotes/<name>/HEAD' '\n>  \tgit init remote &&\n>  \t(\n>  \t\tcd remote &&\n"},{"id":"270831","messageId":"xmqqh9mecrap.fsf@gitster.mtv.corp.google.com","threadId":"40418","inReplyTo":"xmqqpp12croq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 4/4] gc: remove broken symrefs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-28T18:49:50Z","receivedAt":"2015-09-28T18:49:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n>\n>> When encountering broken symrefs, such as a stale remote HEAD (which can\n>> happen if the active branch was renamed in the remote), it is more\n>> helpful to remove those symrefs than to exit with an error.\n>\n> I think this depends on the perspective.  One side of me says that a\n> remote HEAD that points at refs/remotes/origin/topic that no longer\n> exists is still giving me a valuable information and it should take\n> a conscious action by the user to remove it, or evne better to\n> repoint it to a more useful place.  And from that point of view,\n> removing is not all that helpful.  Keeping them and not allowing\n> them to exit with an error would be a real improvement.\n>\n> On the other hand, I can certainly understand a view that considers\n> that such a dangling symbolic ref is merely a cruft like any other\n> cruft, and \"gc\" is all about removing cruft.\n>\n> It just feels to me that this is a bit more valuable than other\n> kinds of cruft, but maybe it is just me.\n\nSorry, it is a bad habit of me to send out without concluding\nremark, leaving the recipient hanging without knowing what the next\nstep should be.\n\nI meant to say that I plan to, and I indeed did, queue these 4\nwithout changes.  I am not opposed to the removal so strongly to\nreject [4/4].\n\nThe above comment was that I just do not know if this is the right\nthing to do, or it will be hurting users.\n\nThanks.\n\n>\n>>\n>> This fixes https://github.com/git-for-windows/git/issues/423\n>>\n>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n>> ---\n>>  builtin/prune.c | 12 +++++++++++-\n>>  t/t6500-gc.sh   |  2 +-\n>>  2 files changed, 12 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/builtin/prune.c b/builtin/prune.c\n>> index d6f664f..337b12a 100644\n>> --- a/builtin/prune.c\n>> +++ b/builtin/prune.c\n>> @@ -6,6 +6,7 @@\n>>  #include \"reachable.h\"\n>>  #include \"parse-options.h\"\n>>  #include \"progress.h\"\n>> +#include \"refs.h\"\n>>  \n>>  static const char * const prune_usage[] = {\n>>  \tN_(\"git prune [-n] [-v] [--expire <time>] [--] [<head>...]\"),\n>> @@ -100,6 +101,7 @@ static void remove_temporary_files(const char *path)\n>>  int cmd_prune(int argc, const char **argv, const char *prefix)\n>>  {\n>>  \tstruct rev_info revs;\n>> +\tstruct string_list broken_symrefs = STRING_LIST_INIT_DUP;\n>>  \tstruct progress *progress = NULL;\n>>  \tconst struct option options[] = {\n>>  \t\tOPT__DRY_RUN(&show_only, N_(\"do not remove, show only\")),\n>> @@ -110,6 +112,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n>>  \t\tOPT_END()\n>>  \t};\n>>  \tchar *s;\n>> +\tint i;\n>>  \n>>  \texpire = ULONG_MAX;\n>>  \tsave_commit_buffer = 0;\n>> @@ -136,7 +139,14 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n>>  \tif (show_progress)\n>>  \t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n>>  \n>> -\tmark_reachable_objects(&revs, 1, expire, progress, NULL);\n>> +\trevs.ignore_missing = 1;\n>> +\tmark_reachable_objects(&revs, 1, expire, progress, &broken_symrefs);\n>> +\tfor (i = 0; i < broken_symrefs.nr; i++) {\n>> +\t\tchar *path = broken_symrefs.items[i].string;\n>> +\t\tprintf(\"Removing stale ref %s\\n\", path);\n>> +\t\tif (!show_only && delete_ref(path, NULL, REF_NODEREF))\n>> +\t\t\tdie(\"Could not remove stale ref %s\", path);\n>> +\t}\n>>  \tstop_progress(&progress);\n>>  \tfor_each_loose_file_in_objdir(get_object_directory(), prune_object,\n>>  \t\t\t\t      prune_cruft, prune_subdir, NULL);\n>> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n>> index b736774..0ae4271 100755\n>> --- a/t/t6500-gc.sh\n>> +++ b/t/t6500-gc.sh\n>> @@ -30,7 +30,7 @@ test_expect_success 'gc -h with invalid configuration' '\n>>  \ttest_i18ngrep \"[Uu]sage\" broken/usage\n>>  '\n>>  \n>> -test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '\n>> +test_expect_success 'gc removes broken refs/remotes/<name>/HEAD' '\n>>  \tgit init remote &&\n>>  \t(\n>>  \t\tcd remote &&\n"},{"id":"270832","messageId":"20150928190332.GA17258@sigill.intra.peff.net","threadId":"40418","inReplyTo":"fb29b39adf14a702c5127d722d86647a3dc9c891.1443448748.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 4/4] gc: remove broken symrefs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-28T19:03:32Z","receivedAt":"2015-09-28T19:03:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 28, 2015 at 04:02:08PM +0200, Johannes Schindelin wrote:\n\n> @@ -136,7 +139,14 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n>  \tif (show_progress)\n>  \t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n>  \n> -\tmark_reachable_objects(&revs, 1, expire, progress, NULL);\n> +\trevs.ignore_missing = 1;\n> +\tmark_reachable_objects(&revs, 1, expire, progress, &broken_symrefs);\n\nYou should not need this ignore_missing anymore, right?\n\nIt is the dangerous thing I mentioned earlier, but I am puzzled why it\ndoes not cause t5312 to fail.\n\n-Peff\n"},{"id":"270836","messageId":"da381393c0b6174cc07b4333308cf611@dscho.org","threadId":"40418","inReplyTo":"xmqqh9mecrap.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 4/4] gc: remove broken symrefs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-09-28T19:58:08Z","receivedAt":"2015-09-28T19:58:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn 2015-09-28 20:49, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n>>\n>>> When encountering broken symrefs, such as a stale remote HEAD (which can\n>>> happen if the active branch was renamed in the remote), it is more\n>>> helpful to remove those symrefs than to exit with an error.\n>>\n>> I think this depends on the perspective.  One side of me says that a\n>> remote HEAD that points at refs/remotes/origin/topic that no longer\n>> exists is still giving me a valuable information and it should take\n>> a conscious action by the user to remove it, or evne better to\n>> repoint it to a more useful place.  And from that point of view,\n>> removing is not all that helpful.  Keeping them and not allowing\n>> them to exit with an error would be a real improvement.\n>>\n>> On the other hand, I can certainly understand a view that considers\n>> that such a dangling symbolic ref is merely a cruft like any other\n>> cruft, and \"gc\" is all about removing cruft.\n>>\n>> It just feels to me that this is a bit more valuable than other\n>> kinds of cruft, but maybe it is just me.\n> \n> Sorry, it is a bad habit of me to send out without concluding\n> remark, leaving the recipient hanging without knowing what the next\n> step should be.\n> \n> I meant to say that I plan to, and I indeed did, queue these 4\n> without changes.  I am not opposed to the removal so strongly to\n> reject [4/4].\n> \n> The above comment was that I just do not know if this is the right\n> thing to do, or it will be hurting users.\n\nOh, I appreciate your feedback. I am actually not all *that* certain that removing the broken symref is the correct thing. It is this sort of fruitful exchange that allows me to throw out an idea and be relatively certain that something better will come out of v3 or v8 of the patch series than what I had in mind.\n\nTo be honest, the most important outcome is probably 2/4 -- which should be enough to fix the issue reported by the Git for Windows user. I could adjust the test so that it no longer insists that `origin/HEAD` be deleted, but still requires that `git gc` succeeds.\n\nI would have no problem to let this sit for a couple of days until the final verdict.\n\nCiao,\nDscho\n"},{"id":"270837","messageId":"737df8378d1c83c97a53e5591a2a6ffb@dscho.org","threadId":"40418","inReplyTo":"20150928190332.GA17258@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] gc: remove broken symrefs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-09-28T20:05:59Z","receivedAt":"2015-09-28T20:05:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn 2015-09-28 21:03, Jeff King wrote:\n> On Mon, Sep 28, 2015 at 04:02:08PM +0200, Johannes Schindelin wrote:\n> \n>> @@ -136,7 +139,14 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n>>  \tif (show_progress)\n>>  \t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n>>\n>> -\tmark_reachable_objects(&revs, 1, expire, progress, NULL);\n>> +\trevs.ignore_missing = 1;\n>> +\tmark_reachable_objects(&revs, 1, expire, progress, &broken_symrefs);\n> \n> You should not need this ignore_missing anymore, right?\n> \n> It is the dangerous thing I mentioned earlier, but I am puzzled why it\n> does not cause t5312 to fail.\n\nGaaaaaah! I edited this out at least twice, yet it creeps back in. Bah!\n\nJunio, would you mind fixing this up locally? If you want me to submit v3, just let me know, I will take care of it tomorrow.\n\nCiao,\nDscho\n"},{"id":"271184","messageId":"xmqq612lx963.fsf@gitster.mtv.corp.google.com","threadId":"40418","inReplyTo":"da381393c0b6174cc07b4333308cf611@dscho.org","subject":"Re: [PATCH v2 4/4] gc: remove broken symrefs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-05T22:06:12Z","receivedAt":"2015-10-05T22:06:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> Oh, I appreciate your feedback. I am actually not all *that* certain\n> that removing the broken symref is the correct thing. It is this sort\n> of fruitful exchange that allows me to throw out an idea and be\n> relatively certain that something better will come out of v3 or v8 of\n> the patch series than what I had in mind.\n>\n> To be honest, the most important outcome is probably 2/4 -- which\n> should be enough to fix the issue reported by the Git for Windows\n> user. I could adjust the test so that it no longer insists that\n> origin/HEAD` be deleted, but still requires that `git gc` succeeds.\n>\n> I would have no problem to let this sit for a couple of days until\n> the final verdict.\n\n... and a few days have passed.  I am tempted to do the easy bits\nfirst, discarding the parts that both of us feel iffy for now\nwithout prejudice, keeping the first two commits with a bit of\ntweak.\n\nThe primary tweak is to t6500 in the first patch, which I retitled\nbelow (and the patch shows s/failure/success/ but in the first step\nthat only adds a failing test, it would of course expect a failure).\n\nAnd with \"pack-objects: do not get distracted\", the test will start\npassing.\n\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex b736774..5d7d414 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -30,7 +30,7 @@ test_expect_success 'gc -h with invalid configuration' '\n \ttest_i18ngrep \"[Uu]sage\" broken/usage\n '\n \n-test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '\n+test_expect_success 'gc is not aborted due to a stale symref' '\n \tgit init remote &&\n \t(\n \t\tcd remote &&\n@@ -39,9 +39,7 @@ test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '\n \t\tgit branch -m develop &&\n \t\tcd ../client &&\n \t\tgit fetch --prune &&\n-\t\tgit gc &&\n-\t\tgit branch --list -r origin/HEAD >actual &&\n-\t\ttest_line_count = 0 actual\n+\t\tgit gc\n \t)\n '\n \ndiff --git a/reachable.c b/reachable.c\nindex 6356ae8..4cfd0de 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -28,7 +28,7 @@ static int add_one_ref(const char *path, const unsigned char *sha1, int flag, vo\n \tstruct object *object;\n \n \tif ((flag & REF_ISSYMREF) && (flag & REF_ISBROKEN)) {\n-\t\twarning(\"ref is broken: %s\", path);\n+\t\twarning(\"symbolic ref is dangling: %s\", path);\n \t\treturn 0;\n \t}\n \n"},{"id":"271225","messageId":"cover.1444139796.git.johannes.schindelin@gmx.de","threadId":"40418","inReplyTo":"xmqqr3lnuzqu.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v3 0/2] Fix gc failure when a remote HEAD goes stale","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-06T13:57:58Z","receivedAt":"2015-10-06T13:57:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"There has been a report in the Git for Windows project that gc fails\nsometimes: https://github.com/git-for-windows/git/issues/423\n\nIt turns out that there are cases when a remote HEAD can go stale and\nit is not the user's fault at all. It can happen, for example, if the\nactive branch in the remote repository gets renamed.\n\nThanks to Junio and Peff for their really valuable sanity check.\n\nInterdiff re: v2 after the diffstat (3/4 and 4/4 were dropped, I am\nno longer removing the broken symrefs).\n\n\nJohannes Schindelin (2):\n  gc: demonstrate failure with stale remote HEAD\n  pack-objects: do not get distracted by broken symrefs\n\n reachable.c   |  8 +++++++-\n t/t6500-gc.sh | 13 +++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 337b12a..10b03d3 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -6,7 +6,6 @@\n #include \"reachable.h\"\n #include \"parse-options.h\"\n #include \"progress.h\"\n-#include \"refs.h\"\n \n static const char * const prune_usage[] = {\n \tN_(\"git prune [-n] [-v] [--expire <time>] [--] [<head>...]\"),\n@@ -101,7 +100,6 @@ static void remove_temporary_files(const char *path)\n int cmd_prune(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n-\tstruct string_list broken_symrefs = STRING_LIST_INIT_DUP;\n \tstruct progress *progress = NULL;\n \tconst struct option options[] = {\n \t\tOPT__DRY_RUN(&show_only, N_(\"do not remove, show only\")),\n@@ -112,7 +110,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \tchar *s;\n-\tint i;\n \n \texpire = ULONG_MAX;\n \tsave_commit_buffer = 0;\n@@ -139,14 +136,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \tif (show_progress)\n \t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n \n-\trevs.ignore_missing = 1;\n-\tmark_reachable_objects(&revs, 1, expire, progress, &broken_symrefs);\n-\tfor (i = 0; i < broken_symrefs.nr; i++) {\n-\t\tchar *path = broken_symrefs.items[i].string;\n-\t\tprintf(\"Removing stale ref %s\\n\", path);\n-\t\tif (!show_only && delete_ref(path, NULL, REF_NODEREF))\n-\t\t\tdie(\"Could not remove stale ref %s\", path);\n-\t}\n+\tmark_reachable_objects(&revs, 1, expire, progress);\n \tstop_progress(&progress);\n \tfor_each_loose_file_in_objdir(get_object_directory(), prune_object,\n \t\t\t\t      prune_cruft, prune_subdir, NULL);\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex cb8758a..f96ca2a 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -583,7 +583,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tinit_revisions(&cb.cmd.revs, prefix);\n \t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n \t\t\tprintf(\"Marking reachable objects...\");\n-\t\tmark_reachable_objects(&cb.cmd.revs, 0, 0, NULL, NULL);\n+\t\tmark_reachable_objects(&cb.cmd.revs, 0, 0, NULL);\n \t\tif (flags & EXPIRE_REFLOGS_VERBOSE)\n \t\t\tputchar('\\n');\n \t}\ndiff --git a/reachable.c b/reachable.c\nindex 25c4932..43616d4 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -15,11 +15,6 @@ struct connectivity_progress {\n \tunsigned long count;\n };\n \n-struct add_one_data {\n-\tstruct rev_info *revs;\n-\tstruct string_list *broken_symrefs;\n-};\n-\n static void update_progress(struct connectivity_progress *cp)\n {\n \tcp->count++;\n@@ -30,19 +25,16 @@ static void update_progress(struct connectivity_progress *cp)\n static int add_one_ref(const char *path, const struct object_id *oid,\n \t\t       int flag, void *cb_data)\n {\n-\tstruct add_one_data *data = (struct add_one_data *)cb_data;\n+\tstruct rev_info *revs = (struct rev_info *)cb_data;\n \tstruct object *object;\n \n \tif ((flag & REF_ISSYMREF) && (flag & REF_ISBROKEN)) {\n-\t\tif (data->broken_symrefs)\n-\t\t\tstring_list_append(data->broken_symrefs, path);\n-\t\telse\n-\t\t\twarning(\"ref is broken: %s\", path);\n+\t\twarning(\"symbolic ref is dangling: %s\", path);\n \t\treturn 0;\n \t}\n \n \tobject = parse_object_or_die(oid->hash, path);\n-\tadd_pending_object(data->revs, object, \"\");\n+\tadd_pending_object(revs, object, \"\");\n \n \treturn 0;\n }\n@@ -167,11 +159,9 @@ int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n \n void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t\t\t    unsigned long mark_recent,\n-\t\t\t    struct progress *progress,\n-\t\t\t    struct string_list *broken_symrefs)\n+\t\t\t    struct progress *progress)\n {\n \tstruct connectivity_progress cp;\n-\tstruct add_one_data data;\n \n \t/*\n \t * Set up revision parsing, and mark us as being interested\n@@ -184,14 +174,11 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t/* Add all refs from the index file */\n \tadd_index_objects_to_pending(revs, 0);\n \n-\tdata.revs = revs;\n-\tdata.broken_symrefs = broken_symrefs;\n-\n \t/* Add all external refs */\n-\tfor_each_ref(add_one_ref, &data);\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, &data);\n+\thead_ref(add_one_ref, revs);\n \n \t/* Add all reflog info */\n \tif (mark_reflog)\ndiff --git a/reachable.h b/reachable.h\nindex 06f1400..d23efc3 100644\n--- a/reachable.h\n+++ b/reachable.h\n@@ -5,7 +5,6 @@ struct progress;\n extern int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n \t\t\t\t\t\t  unsigned long timestamp);\n extern void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n-\t\t\t\t   unsigned long mark_recent, struct progress *,\n-\t\t\t\t   struct string_list *broken_symrefs);\n+\t\t\t\t   unsigned long mark_recent, struct progress *);\n \n #endif\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 0ae4271..5d7d414 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -30,7 +30,7 @@ test_expect_success 'gc -h with invalid configuration' '\n \ttest_i18ngrep \"[Uu]sage\" broken/usage\n '\n \n-test_expect_success 'gc removes broken refs/remotes/<name>/HEAD' '\n+test_expect_success 'gc is not aborted due to a stale symref' '\n \tgit init remote &&\n \t(\n \t\tcd remote &&\n@@ -39,9 +39,7 @@ test_expect_success 'gc removes broken refs/remotes/<name>/HEAD' '\n \t\tgit branch -m develop &&\n \t\tcd ../client &&\n \t\tgit fetch --prune &&\n-\t\tgit gc &&\n-\t\tgit branch --list -r origin/HEAD >actual &&\n-\t\ttest_line_count = 0 actual\n+\t\tgit gc\n \t)\n '\n \n-- \n2.6.1.windows.1\n"},{"id":"271226","messageId":"f6e1778a42460ac5fbcbd3c9ee76edcf52358299.1444139796.git.johannes.schindelin@gmx.de","threadId":"40418","inReplyTo":"cover.1444139796.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 1/2] gc: demonstrate failure with stale remote HEAD","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-06T13:58:42Z","receivedAt":"2015-10-06T13:58:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n t/t6500-gc.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 63194d8..9a3a285 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -30,4 +30,17 @@ test_expect_success 'gc -h with invalid configuration' '\n \ttest_i18ngrep \"[Uu]sage\" broken/usage\n '\n \n+test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '\n+\tgit init remote &&\n+\t(\n+\t\tcd remote &&\n+\t\ttest_commit initial &&\n+\t\tgit clone . ../client &&\n+\t\tgit branch -m develop &&\n+\t\tcd ../client &&\n+\t\tgit fetch --prune &&\n+\t\tgit gc\n+\t)\n+'\n+\n test_done\n-- \n2.6.1.windows.1\n"},{"id":"271227","messageId":"f04229b0881eb7518a306ae07fe11ffc9589b1f1.1444139796.git.johannes.schindelin@gmx.de","threadId":"40418","inReplyTo":"cover.1444139796.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 2/2] pack-objects: do not get distracted by broken symrefs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-06T13:59:11Z","receivedAt":"2015-10-06T13:59:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It is quite possible for, say, a remote HEAD to become broken, e.g.\nwhen the default branch was renamed.\n\nWe should still be able to pack our objects when such a thing happens;\nsimply ignore broken symrefs (because they cannot matter for the packing\nprocess anyway).\n\nThis fixes https://github.com/git-for-windows/git/issues/423\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n reachable.c   | 8 +++++++-\n t/t6500-gc.sh | 2 +-\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex 9cff25b..43616d4 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -25,9 +25,15 @@ static void update_progress(struct connectivity_progress *cp)\n static int add_one_ref(const char *path, const struct object_id *oid,\n \t\t       int flag, void *cb_data)\n {\n-\tstruct object *object = parse_object_or_die(oid->hash, path);\n \tstruct rev_info *revs = (struct rev_info *)cb_data;\n+\tstruct object *object;\n \n+\tif ((flag & REF_ISSYMREF) && (flag & REF_ISBROKEN)) {\n+\t\twarning(\"symbolic ref is dangling: %s\", path);\n+\t\treturn 0;\n+\t}\n+\n+\tobject = parse_object_or_die(oid->hash, path);\n \tadd_pending_object(revs, object, \"\");\n \n \treturn 0;\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 9a3a285..5d7d414 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -30,7 +30,7 @@ test_expect_success 'gc -h with invalid configuration' '\n \ttest_i18ngrep \"[Uu]sage\" broken/usage\n '\n \n-test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '\n+test_expect_success 'gc is not aborted due to a stale symref' '\n \tgit init remote &&\n \t(\n \t\tcd remote &&\n-- \n2.6.1.windows.1\n"},{"id":"271266","messageId":"xmqq8u7evahd.fsf@gitster.mtv.corp.google.com","threadId":"40418","inReplyTo":"f04229b0881eb7518a306ae07fe11ffc9589b1f1.1444139796.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 2/2] pack-objects: do not get distracted by broken symrefs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-07T17:45:18Z","receivedAt":"2015-10-07T17:45:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> It is quite possible for, say, a remote HEAD to become broken, e.g.\n> when the default branch was renamed.\n>\n> We should still be able to pack our objects when such a thing happens;\n> simply ignore broken symrefs (because they cannot matter for the packing\n> process anyway).\n>\n> This fixes https://github.com/git-for-windows/git/issues/423\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n\nIt seems that the result of applying these two patches and log\nmessages of them are the same with what I queued on 'pu', except\nthat the first of these two patches adds a test with a wrong name\nand then this one does \"oops, that was misnamed\".  So I'll keep what\nis already queued.\n\nThanks.\n\n>  reachable.c   | 8 +++++++-\n>  t/t6500-gc.sh | 2 +-\n>  2 files changed, 8 insertions(+), 2 deletions(-)\n>\n> diff --git a/reachable.c b/reachable.c\n> index 9cff25b..43616d4 100644\n> --- a/reachable.c\n> +++ b/reachable.c\n> @@ -25,9 +25,15 @@ static void update_progress(struct connectivity_progress *cp)\n>  static int add_one_ref(const char *path, const struct object_id *oid,\n>  \t\t       int flag, void *cb_data)\n>  {\n> -\tstruct object *object = parse_object_or_die(oid->hash, path);\n>  \tstruct rev_info *revs = (struct rev_info *)cb_data;\n> +\tstruct object *object;\n>  \n> +\tif ((flag & REF_ISSYMREF) && (flag & REF_ISBROKEN)) {\n> +\t\twarning(\"symbolic ref is dangling: %s\", path);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tobject = parse_object_or_die(oid->hash, path);\n>  \tadd_pending_object(revs, object, \"\");\n>  \n>  \treturn 0;\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index 9a3a285..5d7d414 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -30,7 +30,7 @@ test_expect_success 'gc -h with invalid configuration' '\n>  \ttest_i18ngrep \"[Uu]sage\" broken/usage\n>  '\n>  \n> -test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '\n> +test_expect_success 'gc is not aborted due to a stale symref' '\n>  \tgit init remote &&\n>  \t(\n>  \t\tcd remote &&\n"},{"id":"271341","messageId":"5ee759ac37b2561eef9ab60d446ac463@dscho.org","threadId":"40418","inReplyTo":"xmqq8u7evahd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/2] pack-objects: do not get distracted by broken symrefs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-08T19:15:36Z","receivedAt":"2015-10-08T19:15:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn 2015-10-07 19:45, Junio C Hamano wrote:\n> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n> \n>> It is quite possible for, say, a remote HEAD to become broken, e.g.\n>> when the default branch was renamed.\n>>\n>> We should still be able to pack our objects when such a thing happens;\n>> simply ignore broken symrefs (because they cannot matter for the packing\n>> process anyway).\n>>\n>> This fixes https://github.com/git-for-windows/git/issues/423\n>>\n>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n>> ---\n> \n> It seems that the result of applying these two patches and log\n> messages of them are the same with what I queued on 'pu', except\n> that the first of these two patches adds a test with a wrong name\n> and then this one does \"oops, that was misnamed\".  So I'll keep what\n> is already queued.\n\nSorry for fixing up the wrong commit. I honestly meant to fix up the first one. And thank you for fixing it up in `pu` already; I should have known better and check first whether you fixed it.\n\nHowever, there was one more change I made: I wanted to have that link to https://github.com/git-for-windows/git/issues/423 in the commit message to better link the original report with the commit.\n\nWould yo kindly add the line\n\n    This fixes https://github.com/git-for-windows/git/issues/423\n\nbefore the Signed-off-by lines?\n\nThanks,\nDscho\n"},{"id":"271347","messageId":"xmqqr3l5qh8f.fsf@gitster.mtv.corp.google.com","threadId":"40418","inReplyTo":"5ee759ac37b2561eef9ab60d446ac463@dscho.org","subject":"Re: [PATCH v3 2/2] pack-objects: do not get distracted by broken symrefs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-08T19:42:56Z","receivedAt":"2015-10-08T19:42:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> Would yo kindly add the line\n>\n>     This fixes https://github.com/git-for-windows/git/issues/423\n>\n> before the Signed-off-by lines?\n\nOh, sorry, I missed that one.  Fixed.\n\nThanks.\n"},{"id":"271350","messageId":"e1ad395fac8ab0da54338f6eba4ad838@dscho.org","threadId":"40418","inReplyTo":"xmqqr3l5qh8f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/2] pack-objects: do not get distracted by broken symrefs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-08T20:10:37Z","receivedAt":"2015-10-08T20:10:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn 2015-10-08 21:42, Junio C Hamano wrote:\n> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n> \n>> Would yo kindly add the line\n>>\n>>     This fixes https://github.com/git-for-windows/git/issues/423\n>>\n>> before the Signed-off-by lines?\n> \n> Oh, sorry, I missed that one.  Fixed.\n> \n> Thanks.\n\nThank you!\nDscho\n"}]}