{"thread":{"id":"13751","subject":"[PATCH 3/3] Make \"git-remote rm\" delete refs acccording to fetch specs","startedAt":"2008-06-01T03:59:01Z","lastAt":"2008-06-02T17:10:25Z","messageCount":7,"participants":["Shawn O. Pearce","Junio C Hamano","Jon Loeliger"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"78269","messageId":"20080601035901.GC32176@spearce.org","threadId":"13751","inReplyTo":null,"subject":"[PATCH 3/3] Make \"git-remote rm\" delete refs acccording to fetch specs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-06-01T03:59:01Z","receivedAt":"2008-06-01T03:59:01Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"A remote may be configured to fetch into tracking branches that\ndon't match its name.  A user may have created a remote by hand\nthat will fetch to a different tracking branch namespace:\n\n  [remote \"alt\"]\n    url = git://repo.or.cz/alt-git.git\n    fetch = refs/heads/*:refs/remotes/origin/*\n\nWhen deleting remote alt we should clean up the refs whose names\nstart with \"refs/remotes/origin/\", even though the remote itself\nwas named alt by the user.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n\n I'm not sure this is the right thing to do here.  If another\n remote is still using these tracking refs (such as the example\n in my prior patch of this series) we may not want to remove these\n when we remove the remote.\n\n builtin-remote.c |   11 ++++++-----\n 1 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex e5cfc88..6dbc3d4 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -268,7 +268,7 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)\n }\n \n struct branches_for_remote {\n-\tconst char *prefix;\n+\tstruct remote *remote;\n \tstruct path_list *branches;\n };\n \n@@ -276,8 +276,11 @@ static int add_branch_for_removal(const char *refname,\n \tconst unsigned char *sha1, int flags, void *cb_data)\n {\n \tstruct branches_for_remote *branches = cb_data;\n+\tstruct refspec refspec;\n \n-\tif (!prefixcmp(refname, branches->prefix)) {\n+\tmemset(&refspec, 0, sizeof(refspec));\n+\trefspec.dst = (char *)refname;\n+\tif (!remote_find_tracking(branches->remote, &refspec)) {\n \t\tstruct path_list_item *item;\n \n \t\t/* make sure that symrefs are deleted */\n@@ -352,9 +355,7 @@ static int rm(int argc, const char **argv)\n \t * the branches one by one, since for_each_ref() relies on cached\n \t * refs, which are invalidated when deleting a branch.\n \t */\n-\tstrbuf_reset(&buf);\n-\tstrbuf_addf(&buf, \"refs/remotes/%s/\", remote->name);\n-\tcb_data.prefix = buf.buf;\n+\tcb_data.remote = remote;\n \ti = for_each_ref(add_branch_for_removal, &cb_data);\n \tstrbuf_release(&buf);\n \n-- \n1.5.6.rc0.158.g7c7a1\n"},{"id":"78270","messageId":"20080601042804.GA32293@spearce.org","threadId":"13751","inReplyTo":"20080601035901.GC32176@spearce.org","subject":"[PATCH 3/3 v2] Make \"git-remote rm\" delete refs acccording to fetch specs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-06-01T04:28:04Z","receivedAt":"2008-06-01T04:28:04Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"A remote may be configured to fetch into tracking branches that\ndon't match its name.  A user may have created a remote by hand\nthat will fetch to a different tracking branch namespace:\n\n  [remote \"alt\"]\n    url = git://repo.or.cz/alt-git.git\n    fetch = refs/heads/*:refs/remotes/origin/*\n\nWhen deleting remote alt we should clean up the refs whose names\nstart with \"refs/remotes/origin/\", even though the remote itself\nwas named alt by the user.\n\nTo avoid deleting refs used by another remote we only clear refs\nthat are unique to this remote.  This prevents `git prune rm alt`\nfrom removing the refs used by say origin if alt was just using a\ndifferent URL for the same repository.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n\n This is a longer, but better version of this patch.  Instead of\n blindly deleting the refs we remove them only if this is the last\n remote that would write to the local tracking ref.\n\n builtin-remote.c |   68 ++++++++++++++++++++++++++++++++++++++++++-----------\n 1 files changed, 54 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex e5cfc88..c49f00f 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -267,27 +267,65 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)\n \treturn 0;\n }\n \n+struct known_remote {\n+\tstruct known_remote *next;\n+\tstruct remote *remote;\n+};\n+\n+struct known_remotes {\n+\tstruct remote *to_delete;\n+\tstruct known_remote *list;\n+};\n+\n+static int add_known_remote(struct remote *remote, void *cb_data)\n+{\n+\tstruct known_remotes *all = cb_data;\n+\tstruct known_remote *r;\n+\n+\tif (!strcmp(all->to_delete->name, remote->name))\n+\t\treturn 0;\n+\n+\tr = xmalloc(sizeof(*r));\n+\tr->remote = remote;\n+\tr->next = all->list;\n+\tall->list = r;\n+\treturn 0;\n+}\n+\n struct branches_for_remote {\n-\tconst char *prefix;\n+\tstruct remote *remote;\n \tstruct path_list *branches;\n+\tstruct known_remotes *keep;\n };\n \n static int add_branch_for_removal(const char *refname,\n \tconst unsigned char *sha1, int flags, void *cb_data)\n {\n \tstruct branches_for_remote *branches = cb_data;\n+\tstruct refspec refspec;\n+\tstruct path_list_item *item;\n+\tstruct known_remote *kr;\n \n-\tif (!prefixcmp(refname, branches->prefix)) {\n-\t\tstruct path_list_item *item;\n+\tmemset(&refspec, 0, sizeof(refspec));\n+\trefspec.dst = (char *)refname;\n+\tif (remote_find_tracking(branches->remote, &refspec))\n+\t\treturn 0;\n+\n+\t/* don't delete a branch if another remote also uses it */\n+\tfor (kr = branches->keep->list; kr; kr = kr->next) {\n+\t\tmemset(&refspec, 0, sizeof(refspec));\n+\t\trefspec.dst = (char *)refname;\n+\t\tif (!remote_find_tracking(kr->remote, &refspec))\n+\t\t\treturn 0;\n+\t}\n \n-\t\t/* make sure that symrefs are deleted */\n-\t\tif (flags & REF_ISSYMREF)\n-\t\t\treturn unlink(git_path(refname));\n+\t/* make sure that symrefs are deleted */\n+\tif (flags & REF_ISSYMREF)\n+\t\treturn unlink(git_path(refname));\n \n-\t\titem = path_list_append(refname, branches->branches);\n-\t\titem->util = xmalloc(20);\n-\t\thashcpy(item->util, sha1);\n-\t}\n+\titem = path_list_append(refname, branches->branches);\n+\titem->util = xmalloc(20);\n+\thashcpy(item->util, sha1);\n \n \treturn 0;\n }\n@@ -313,8 +351,9 @@ static int rm(int argc, const char **argv)\n \t};\n \tstruct remote *remote;\n \tstruct strbuf buf;\n+\tstruct known_remotes known_remotes = { NULL, NULL };\n \tstruct path_list branches = { NULL, 0, 0, 1 };\n-\tstruct branches_for_remote cb_data = { NULL, &branches };\n+\tstruct branches_for_remote cb_data = { NULL, &branches, &known_remotes };\n \tint i;\n \n \tif (argc != 2)\n@@ -324,6 +363,9 @@ static int rm(int argc, const char **argv)\n \tif (!remote)\n \t\tdie(\"No such remote: %s\", argv[1]);\n \n+\tknown_remotes.to_delete = remote;\n+\tfor_each_remote(add_known_remote, &known_remotes);\n+\n \tstrbuf_init(&buf, 0);\n \tstrbuf_addf(&buf, \"remote.%s\", remote->name);\n \tif (git_config_rename_section(buf.buf, NULL) < 1)\n@@ -352,9 +394,7 @@ static int rm(int argc, const char **argv)\n \t * the branches one by one, since for_each_ref() relies on cached\n \t * refs, which are invalidated when deleting a branch.\n \t */\n-\tstrbuf_reset(&buf);\n-\tstrbuf_addf(&buf, \"refs/remotes/%s/\", remote->name);\n-\tcb_data.prefix = buf.buf;\n+\tcb_data.remote = remote;\n \ti = for_each_ref(add_branch_for_removal, &cb_data);\n \tstrbuf_release(&buf);\n \n-- \n1.5.6.rc0.158.g7c7a1\n"},{"id":"78271","messageId":"20080601044047.GF12896@spearce.org","threadId":"13751","inReplyTo":"20080601042804.GA32293@spearce.org","subject":"Re: [PATCH 3/3 v2] Make \"git-remote rm\" delete refs acccording to fetch specs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-06-01T04:40:47Z","receivedAt":"2008-06-01T04:40:47Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> To avoid deleting refs used by another remote we only clear refs\n> that are unique to this remote.  This prevents `git prune rm alt`\n> from removing the refs used by say origin if alt was just using a\n> different URL for the same repository.\n\nHmm.  You probably should squash this test case into that patch.\n\n--8<--\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 0d7ed1f..96e461b 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -182,6 +182,18 @@ test_expect_success 'add alt && prune' '\n \t git rev-parse --verify refs/remotes/origin/side2)\n '\n \n+test_expect_success 'rm alt' '\n+\t(cd alttst &&\n+\t git remote rm alt &&\n+\t tokens_match z z$(git config remote.alt.url) &&\n+\t tokens_match z z$(git config remote.alt.fetch) &&\n+\t git rev-parse --verify refs/remotes/origin/side2 &&\n+\t git remote rm origin &&\n+\t tokens_match z z$(git config remote.alt.url) &&\n+\t tokens_match z z$(git config remote.alt.fetch) &&\n+\t ! git rev-parse --verify refs/remotes/origin/side2)\n+'\n+\n cat > one/expect << EOF\n   apis/master\n   apis/side\n\n-- \nShawn.\n"},{"id":"78274","messageId":"7vprr1d8p2.fsf@gitster.siamese.dyndns.org","threadId":"13751","inReplyTo":"20080601042804.GA32293@spearce.org","subject":"Re: [PATCH 3/3 v2] Make \"git-remote rm\" delete refs acccording to fetch specs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-01T05:29:29Z","receivedAt":"2008-06-01T05:29:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n>  This is a longer, but better version of this patch.  Instead of\n>  blindly deleting the refs we remove them only if this is the last\n>  remote that would write to the local tracking ref.\n\nIf this is a better version than the previous one, then probably \"git\nremote prune\" patch to unconditionally remove ones that do not exist in\none of the remotes that fetch into the tracking namespace also needs to be\nrethought, doesn't it?  Another remote may still have it but you obviously\ndo not know that until you check.\n\nAdmittedly, next fetch from the other remote may or may not resurrect them\nand either way it is not a big deal.\n\nI think this is exactly the same issue as this improvement in [3/3] deals\nwith.  If \"git remote rm\" of one remote removed the shared tracked refs,\nnext fetch from the other remote would resurrect them if the other remote\nstill exists.  It may probably feel better to be extra careful like this\nimproved patch, but I doubt it would matter in practice.  After all,\npeople who creates such a configuration would know what they are doing.\n"},{"id":"78275","messageId":"20080601063051.GH12896@spearce.org","threadId":"13751","inReplyTo":"7vprr1d8p2.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3 v2] Make \"git-remote rm\" delete refs acccording to fetch specs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-06-01T06:30:51Z","receivedAt":"2008-06-01T06:30:51Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n> \n> >  This is a longer, but better version of this patch.  Instead of\n> >  blindly deleting the refs we remove them only if this is the last\n> >  remote that would write to the local tracking ref.\n> \n> If this is a better version than the previous one, then probably \"git\n> remote prune\" patch to unconditionally remove ones that do not exist in\n> one of the remotes that fetch into the tracking namespace also needs to be\n> rethought, doesn't it?\n\nNope.  I've thought about this already.  ;-)\n \n> Admittedly, next fetch from the other remote may or may not resurrect them\n> and either way it is not a big deal.\n\nCorrect.\n\n> I think this is exactly the same issue as this improvement in [3/3] deals\n> with.  If \"git remote rm\" of one remote removed the shared tracked refs,\n> next fetch from the other remote would resurrect them if the other remote\n> still exists.  It may probably feel better to be extra careful like this\n> improved patch, but I doubt it would matter in practice.  After all,\n> people who creates such a configuration would know what they are doing.\n\nI don't think it matters in practice.\n\nIf the user has configured two different remotes with different URLs\nfetching to the same set of tracking branches, well, they get what\nthey get when their prune and fetch start fighting over a tracking\nbranch existing or disappearing.\n\nToday I found these misfeatures in git-remote because I sort of do\nwhat I showed in my second patch.  I have two remotes which fetch to\nthe same tracking branches, as they fetch from the same repository,\njust over two different protocols.  There never should be a time\nwhere I can see a difference between them, unless its just a race\ncondition where someone else created or deleted a branch between\nmy two sequential git-fetch/git-remote calls.\n\nI think the behavior in 2/2 and 3/3v2 is the best we can do, short of\ncontacting all other remotes which go to the same tracking branch.\nBut that may not be possible.  One reason for using two different\nremote configurations to the same tracking branches is to make\nthe URL differ.  Think fetching against repo.or.cz using git://\nand also http://, where http:// is necessary when you are stuck\nbehind a @#U*!@(#*!@#*!@(@!! proxy server.  We cannot contact both\nremotes to verify the branch really is safe to prune.\n\n-- \nShawn.\n"},{"id":"78279","messageId":"7vve0tbluy.fsf@gitster.siamese.dyndns.org","threadId":"13751","inReplyTo":"20080601063051.GH12896@spearce.org","subject":"Re: [PATCH 3/3 v2] Make \"git-remote rm\" delete refs acccording to fetch specs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-01T08:28:05Z","receivedAt":"2008-06-01T08:28:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> I think the behavior in 2/2 and 3/3v2 is the best we can do,...\n\nOh, I think you already know I agree with that.  I was just pointing out\nthat the extra niceness would not have mattered that much in practice, but\nif the code is already written and is not crappy, why not ;-)\n\nNot blindly assuming that the remote nickname matches the actualy used\nnamespace under refs/remotes _is_ a good bugfix regardless of the issue of\noverlapping configuration. \n"},{"id":"78385","messageId":"1212426625.3990.19.camel@ld0161-tx32","threadId":"13751","inReplyTo":"20080601063051.GH12896@spearce.org","subject":"Re: [PATCH 3/3 v2] Make \"git-remote rm\" delete refs acccording to fetch specs","fromName":"Jon Loeliger","fromEmail":"jdl@freescale.com","sentAt":"2008-06-02T17:10:25Z","receivedAt":"2008-06-02T17:10:25Z","isPatch":true,"sender":{"key":"jdl@jdl.com","avatar":"https://gravatar.com/avatar/75ce9a10b151acd2c28ec4ab2136dba7b2ff1634530bd04b155981a749d08a64?d=mp&s=160"},"body":"On Sun, 2008-06-01 at 02:30 -0400, Shawn O. Pearce wrote:\n> Junio C Hamano <gitster@pobox.com> wrote:\n> > \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n> > \n> > >  This is a longer, but better version of this patch.  Instead of\n> > >  blindly deleting the refs we remove them only if this is the last\n> > >  remote that would write to the local tracking ref.\n> > \n> > If this is a better version than the previous one, then probably \"git\n> > remote prune\" patch to unconditionally remove ones that do not exist in\n> > one of the remotes that fetch into the tracking namespace also needs to be\n> > rethought, doesn't it?\n> \n> Nope.  I've thought about this already.  ;-)\n>  \n> > Admittedly, next fetch from the other remote may or may not resurrect them\n> > and either way it is not a big deal.\n> \n> Correct.\n> \n> > I think this is exactly the same issue as this improvement in [3/3] deals\n> > with.  If \"git remote rm\" of one remote removed the shared tracked refs,\n> > next fetch from the other remote would resurrect them if the other remote\n> > still exists.  It may probably feel better to be extra careful like this\n> > improved patch, but I doubt it would matter in practice.  After all,\n> > people who creates such a configuration would know what they are doing.\n> \n> I don't think it matters in practice.\n> \n> If the user has configured two different remotes with different URLs\n> fetching to the same set of tracking branches, well, they get what\n> they get when their prune and fetch start fighting over a tracking\n> branch existing or disappearing.\n> \n> Today I found these misfeatures in git-remote because I sort of do\n> what I showed in my second patch.  I have two remotes which fetch to\n> the same tracking branches, as they fetch from the same repository,\n> just over two different protocols.  There never should be a time\n> where I can see a difference between them, unless its just a race\n> condition where someone else created or deleted a branch between\n> my two sequential git-fetch/git-remote calls.\n\nThis all seems sort of fragile to me.  Like maybe there\nis a different name-space concept itching to get out here.\nLike a [refspec \"foo\"] node in the config file or something\nthat clearly states the expected namespace for an remote.\n\n> I think the behavior in 2/2 and 3/3v2 is the best we can do, short of\n> contacting all other remotes which go to the same tracking branch.\n> But that may not be possible.  One reason for using two different\n> remote configurations to the same tracking branches is to make\n> the URL differ.  Think fetching against repo.or.cz using git://\n> and also http://, where http:// is necessary when you are stuck\n> behind a @#U*!@(#*!@#*!@(@!! proxy server.  We cannot contact both\n> remotes to verify the branch really is safe to prune.\n\nDidn't we settle on an alternate URL mechanism that covered\nthis case already?  Or was that a different case/\n\nCRS,\njdl\n"}]}