{"thread":{"id":"12710","subject":"tracking repository","startedAt":"2008-03-15T19:35:12Z","lastAt":"2008-03-17T16:23:26Z","messageCount":16,"participants":["kenneth johansson","Junio C Hamano","Daniel Barkalow"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"72193","messageId":"frh8dg$t9j$1@ger.gmane.org","threadId":"12710","inReplyTo":null,"subject":"tracking repository","fromName":"kenneth johansson","fromEmail":"ken@kenjo.org","sentAt":"2008-03-15T19:35:12Z","receivedAt":"2008-03-15T19:35:12Z","isPatch":false,"sender":{"key":"ken@kenjo.org","avatar":null},"body":"I was trying to create a repository used only to track different linux git \nrepositories. The goal with this was to maximize object sharing and having \none local copy of the data. \n\nBut I think I managed to paint myself into a corner. Here was my initial \nsetup. \n\ncreate a directory \n\"mkdir linux\"\n\ncreate a git data base\n\"cd linux; git --bare init\"\n\nAdd a few linux repositories with\n\"git --bare remote add [name] [url]\"\n\nthen download the objects/branches with\n\"git remote update\"\n\nThis works great and it will track all changes in the remote repositories \nwithout me having to worry about it aborting due to merge issues with my \nlocal branch or remote  doing rebase on some branch.\n\nThe problem is that it is useless :( I can't find any way to use a \nrepository with only remotes in it. Is there a way to make a clone of a \nremote branch in a repository ??\n\nNow it is not entirely useless since I can reuse the objects downloaded by \nsetting GIT_OBJECT_DIRECTORY. This works quite well until \"git gc --prune\" \nis used. I leave it up to the reader to figure out what happens then :(\n\nSo I guess there should be some warning about using GIT_OBJECT_DIRECTORY \nthat points to the same object store for different repositories. It's \nobvious but still a warning in the man page could be helpful.\n\nGIT_ALTERNATE_OBJECT_DIRECTORIES works much better. The only potential \nproblem I see is if I set this is set in my environment when I login and \nthen do operation on my tracking repository's it will now point into it's \nown object directory. I have not tried that yet.\n\nThe downside of only using the objects is that I need to setup the remotes \nagain in my clone. \n\nNow has anybody tried to do something similar? is there a better way?\n"},{"id":"72200","messageId":"7vabkzmltc.fsf@gitster.siamese.dyndns.org","threadId":"12710","inReplyTo":"frh8dg$t9j$1@ger.gmane.org","subject":"Re: tracking repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-16T02:42:23Z","receivedAt":"2008-03-16T02:42:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"kenneth johansson <ken@kenjo.org> writes:\n\n> This works great and it will track all changes in the remote repositories \n> without me having to worry about it aborting due to merge issues with my \n> local branch or remote  doing rebase on some branch.\n>\n> The problem is that it is useless :( I can't find any way to use a \n> repository with only remotes in it. Is there a way to make a clone of a \n> remote branch in a repository ??\n\nUsually a clone with a work tree (\"git clone $elsewhere\") is configured to\nkeep copies of branches at the remote in remotes/origin in order to track\nthem, and that is done by having this in its .git/config:\n\n\t[remote \"origin\"]\n\t\turl = $elsewhere\n\t\tfetch = +refs/heads/*:refs/remotes/origin/*\n\t[branch \"master\"]\n        \tremote = origin\n                merge = refs/heads/master\n\nThis lets you to have your own work on your own \"master\", and have changes\non the other end merged when you \"git pull\" from there, while keeping\ntrack of other branches on the other end in remotes/origin/ namespace.\n\nYou do not want to have any of your own work in this repository, however,\nso there is no reason to separate the remote ones in remotes/origin/\nnamespace.  You would want \"mirroring\".\n\nYou can have in your $GIT_DIR/config something like this:\n\n        [remote \"origin\"]\n\t\turl = $elsewhere\n                fetch = +refs/heads/*:refs/heads/*\n\nYou can edit the configuration file yourself to read like above, and then\n\"git fetch\" will keep a copy of remote \"master\" branch in your local\n\"master\" (and similarly to all the branches over there).\n\nModern git allows this setup via \"git remote add --mirror\"; it is merely a\nconvenience wrapper and it is perfectly fine to edit the configuration\nfile yourself without using it.\n"},{"id":"72236","messageId":"1205697779.12760.20.camel@duo","threadId":"12710","inReplyTo":"7vabkzmltc.fsf@gitster.siamese.dyndns.org","subject":"Re: tracking repository","fromName":"kenneth johansson","fromEmail":"ken@kenjo.org","sentAt":"2008-03-16T20:02:59Z","receivedAt":"2008-03-16T20:02:59Z","isPatch":false,"sender":{"key":"ken@kenjo.org","avatar":null},"body":"On Sat, 2008-03-15 at 19:42 -0700, Junio C Hamano wrote:\n\n> You do not want to have any of your own work in this repository, however,\n> so there is no reason to separate the remote ones in remotes/origin/\n> namespace.  You would want \"mirroring\".\n> \n> You can have in your $GIT_DIR/config something like this:\n> \n>         [remote \"origin\"]\n> \t\turl = $elsewhere\n>                 fetch = +refs/heads/*:refs/heads/*\n\n> Modern git allows this setup via \"git remote add --mirror\"; it is merely a\n> convenience wrapper and it is perfectly fine to edit the configuration\n> file yourself without using it.\n\nI tried using the option --mirror but then the config end up in a way\nthat \nall remote repositories master branch maps to exactly the same name. \n\nHowever changing the config file manually I did get one that works more\nor \nless as I intended. \n\nSo with the below config I can create an empty directory and in that do \n\"git --bare init\"\ncopy in the config file and any objects I have laying around. \nthen simply put a cron job that once a day do a\n\"git remote update\"\n\nthe resulting repository is then possible to clone. And as long as no\nrepacking is done the object data will be shared. But to share data even\nafter a repack I guess I need to use GIT_ALTERNATE_OBJECT_DIRECTORIES\nfor my local clone. And then I need to be very careful when doing any\npruning on the download repository since my local clone could need data\nthat is no longer needed in the download repository. \n\nWhat would a safe procedure be ?? copy all data in the object hierarchy\nfrom the download repository to my local clones then do a gc in them\nstarting from the download repository. \n\n------------\n[core]\n\trepositoryformatversion = 0\n\tfilemode = true\n\tbare = true\n[remote \"linus\"]\n\turl =\ngit://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git\n\tfetch = +refs/heads/*:refs/heads/*\n[remote \"stable_2.6.12\"]\n\turl =\ngit://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.12.y.git\n\tfetch = +refs/heads/*:refs/heads/stable_2.6.12_*\n[remote \"stable_2.6.13\"]\n\turl =\ngit://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.13.y.git\n\tfetch = +refs/heads/*:refs/heads/stable_2.6.13_*\n[remote \"stable_2.6.14\"]\n\turl =\ngit://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.14.y.git\n\tfetch = +refs/heads/*:refs/heads/stable_2.6.14_*\n[remote \"stable_2.6.15\"]\n\turl =\ngit://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.15.y.git\n\tfetch = +refs/heads/*:refs/heads/stable_2.6.15_*\n[remote \"stable_2.6.16\"]\n\turl =\ngit://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.16.y.git\n\tfetch = +refs/heads/*:refs/heads/stable_2.6.16_*\n[remote \"stable_2.6.17\"]\n\turl =\ngit://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.17.y.git\n\tfetch = +refs/heads/*:refs/heads/stable_2.6.17_*\n[remote \"stable_2.6.18\"]\n\turl =\ngit://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.18.y.git\n\tfetch = +refs/heads/*:refs/heads/stable_2.6.18_*\n[remote \"stable_2.6.19\"]\n\turl =\ngit://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.19.y.git\n\tfetch = +refs/heads/*:refs/heads/stable_2.6.19_*\n[remote \"stable_2.6.20\"]\n\turl =\ngit://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.20.y.git\n\tfetch = +refs/heads/*:refs/heads/stable_2.6.20_*\n[remote \"stable_2.6.21\"]\n\turl =\ngit://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.21.y.git\n\tfetch = +refs/heads/*:refs/heads/stable_2.6.21_*\n[remote \"stable_2.6.22\"]\n\turl =\ngit://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.22.y.git\n\tfetch = +refs/heads/*:refs/heads/stable_2.6.22_*\n[remote \"stable_2.6.23\"]\n\turl =\ngit://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.23.y.git\n\tfetch = +refs/heads/*:refs/heads/stable_2.6.23_*\n[remote \"stable_2.6.24\"]\n\turl =\ngit://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.24.y.git\n\tfetch = +refs/heads/*:refs/heads/stable_2.6.24_*\n\n------------\n"},{"id":"72234","messageId":"7vwso2ieuu.fsf@gitster.siamese.dyndns.org","threadId":"12710","inReplyTo":"1205697779.12760.20.camel@duo","subject":"Re: tracking repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-16T20:38:33Z","receivedAt":"2008-03-16T20:38:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"kenneth johansson <ken@kenjo.org> writes:\n\n> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git\n> \tfetch = +refs/heads/*:refs/heads/*\n> [remote \"stable_2.6.12\"]\n> \turl =\n> git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.12.y.git\n> \tfetch = +refs/heads/*:refs/heads/stable_2.6.12_*\n\nDaniel, I think we are looking at a regression.  The latter style, * at\nthe end but not immediately following a slash, should never have worked.\nWildcard expansion function should be erroring out when it sees something\nlike this.\n\nOnce we fix that regression, the above would stop working (in)correctly.\nRewrite it to something like this right now will make it keep working:\n\n \tfetch = +refs/heads/*:refs/heads/stable_2.6.12/*\n"},{"id":"72244","messageId":"alpine.LNX.1.00.0803161716470.19665@iabervon.org","threadId":"12710","inReplyTo":"7vwso2ieuu.fsf@gitster.siamese.dyndns.org","subject":"Re: tracking repository","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-16T21:28:36Z","receivedAt":"2008-03-16T21:28:36Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 16 Mar 2008, Junio C Hamano wrote:\n\n> kenneth johansson <ken@kenjo.org> writes:\n> \n> > git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git\n> > \tfetch = +refs/heads/*:refs/heads/*\n> > [remote \"stable_2.6.12\"]\n> > \turl =\n> > git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.12.y.git\n> > \tfetch = +refs/heads/*:refs/heads/stable_2.6.12_*\n> \n> Daniel, I think we are looking at a regression.  The latter style, * at\n> the end but not immediately following a slash, should never have worked.\n> Wildcard expansion function should be erroring out when it sees something\n> like this.\n\nI'm not sure any older code actually enforced this, either\n\nWe don't currently have any concept of an invalid refspec; we just have \nthings that fall back to not being patterns and not being possible to \nmatch (due to one or the other side being invalid as a ref name).\n\nHere's a patch to make the pattern logic require a slash before the *:\n---------\ncommit 7aa15c359bcfc7a3c87345435b81ef41e1f59800\nAuthor: Daniel Barkalow <barkalow@iabervon.org>\nDate:   Sun Mar 16 17:26:41 2008 -0400\n\n    Require / before * in pattern refspecs\n    \n    We don't want to have \"+refs/heads/*:refs/heads/something_*\" match\n    \"refs/heads/master\" to \"refs/heads/something_master\".\n    \n    Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>\n\ndiff --git a/remote.c b/remote.c\nindex f3f7375..fffde34 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -404,18 +404,17 @@ struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)\n \t\t\trs[i].force = 1;\n \t\t\tsp++;\n \t\t}\n-\t\tgp = strchr(sp, '*');\n+\t\tgp = strstr(sp, \"/*\");\n \t\tep = strchr(sp, ':');\n \t\tif (gp && ep && gp > ep)\n \t\t\tgp = NULL;\n \t\tif (ep) {\n \t\t\tif (ep[1]) {\n-\t\t\t\tconst char *glob = strchr(ep + 1, '*');\n+\t\t\t\tconst char *glob = strstr(ep + 1, \"/*\");\n \t\t\t\tif (!glob)\n \t\t\t\t\tgp = NULL;\n \t\t\t\tif (gp)\n-\t\t\t\t\trs[i].dst = xstrndup(ep + 1,\n-\t\t\t\t\t\t\t     glob - ep - 1);\n+\t\t\t\t\trs[i].dst = xstrndup(ep + 1, glob - ep);\n \t\t\t\telse\n \t\t\t\t\trs[i].dst = xstrdup(ep + 1);\n \t\t\t}\n@@ -424,7 +423,7 @@ struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)\n \t\t}\n \t\tif (gp) {\n \t\t\trs[i].pattern = 1;\n-\t\t\tep = gp;\n+\t\t\tep = gp + 1;\n \t\t}\n \t\trs[i].src = xstrndup(sp, ep - sp);\n \t}\n"},{"id":"72245","messageId":"7vwso2gwnf.fsf@gitster.siamese.dyndns.org","threadId":"12710","inReplyTo":"alpine.LNX.1.00.0803161716470.19665@iabervon.org","subject":"Re: tracking repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-16T21:57:08Z","receivedAt":"2008-03-16T21:57:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> I'm not sure any older code actually enforced this, either\n\nI am fairly sure the old code was written with the intention in mind (I\nwrote it, in other words).  It meant to accept refs/<anything>/* and no\nother wildcard.\n\nDoes your patch require * to be at the end?\n"},{"id":"72247","messageId":"alpine.LNX.1.00.0803161812340.19665@iabervon.org","threadId":"12710","inReplyTo":"7vwso2gwnf.fsf@gitster.siamese.dyndns.org","subject":"Re: tracking repository","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-16T22:18:58Z","receivedAt":"2008-03-16T22:18:58Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 16 Mar 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > I'm not sure any older code actually enforced this, either\n> \n> I am fairly sure the old code was written with the intention in mind (I\n> wrote it, in other words).  It meant to accept refs/<anything>/* and no\n> other wildcard.\n\nI know that's all it was supposed to accept, but I don't remember seeing \nanything to enforce that. Actually, I don't now remember what the old code \nlooked like at all, so I might be wrong about that.\n\nIs \"refs/*:refs/*\" (mirror everything, including weird stuff) supposed to \nbe prohibited?\n\n> Does your patch require * to be at the end?\n\nLooks like it just ignores anything after a *. Want checks for that as \nwell?\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"72250","messageId":"7vzlsyfgjg.fsf@gitster.siamese.dyndns.org","threadId":"12710","inReplyTo":"alpine.LNX.1.00.0803161812340.19665@iabervon.org","subject":"Re: tracking repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-16T22:30:27Z","receivedAt":"2008-03-16T22:30:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> Is \"refs/*:refs/*\" (mirror everything, including weird stuff) supposed to \n> be prohibited?\n\nNo.  In fact \"remote add --mirror\" actively creates such.  See my other\nmessage about design level issues.\n\n>> Does your patch require * to be at the end?\n>\n> Looks like it just ignores anything after a *. Want checks for that as \n> well?\n\nSurely.  Starting strict and making it looser later is much easier than\nstarting loose and incoherent and having to deal with the resulting mess\nthe code appears to allow people to make in their configuration.\n"},{"id":"72253","messageId":"7vbq5eff3e.fsf@gitster.siamese.dyndns.org","threadId":"12710","inReplyTo":"7vzlsyfgjg.fsf@gitster.siamese.dyndns.org","subject":"Re: tracking repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-16T23:01:41Z","receivedAt":"2008-03-16T23:01:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n>\n>> Is \"refs/*:refs/*\" (mirror everything, including weird stuff) supposed to \n>> be prohibited?\n>\n> No.  In fact \"remote add --mirror\" actively creates such.  See my other\n> message about design level issues.\n\nI think something like this is needed.  It still has an independent issue\nthat this is now called by \"git remote show\" or \"git remote prune\", and it\nwill die with a nonsense \"refusing to create\" error message, though.\n\nThe error, as far as I can tell, is half about a misconfigured config\n(e.g. \"fetch = refs/heads/*:refs/remotes/[]?/*\") and half about screwy\nremote repository (e.g. a misnamed \"[]?\" branch on the remote end can try\nto update a broken \"refs/remotes/origin/[]?\" even the configuration is a\nperfectly valid \"fetch = refs/heads/*:refs/remotes/origin/*\").  It may\nmake sense to reword the error message to \"ignoring\" from \"refusing\" and\ndo just that without dying here.  I dunno.\n\n remote.c |    9 ++++++---\n 1 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex f3f7375..fbcb03c 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1007,9 +1007,12 @@ int get_fetch_map(const struct ref *remote_refs,\n \t}\n \n \tfor (rm = ref_map; rm; rm = rm->next) {\n-\t\tif (rm->peer_ref && check_ref_format(rm->peer_ref->name + 5))\n-\t\t\tdie(\"* refusing to create funny ref '%s' locally\",\n-\t\t\t    rm->peer_ref->name);\n+\t\tif (rm->peer_ref) {\n+\t\t\tint st = check_ref_format(rm->peer_ref->name + 5);\n+\t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL)\n+\t\t\t\tdie(\"* refusing to create funny ref '%s'\"\n+\t\t\t\t    \" locally\", rm->peer_ref->name);\n+\t\t}\n \t}\n \n \tif (ref_map)\n"},{"id":"72256","messageId":"alpine.LNX.1.00.0803161904360.19665@iabervon.org","threadId":"12710","inReplyTo":"7vbq5eff3e.fsf@gitster.siamese.dyndns.org","subject":"Re: tracking repository","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-16T23:11:48Z","receivedAt":"2008-03-16T23:11:48Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 16 Mar 2008, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Daniel Barkalow <barkalow@iabervon.org> writes:\n> >\n> >> Is \"refs/*:refs/*\" (mirror everything, including weird stuff) supposed to \n> >> be prohibited?\n> >\n> > No.  In fact \"remote add --mirror\" actively creates such.  See my other\n> > message about design level issues.\n> \n> I think something like this is needed.  It still has an independent issue\n> that this is now called by \"git remote show\" or \"git remote prune\", and it\n> will die with a nonsense \"refusing to create\" error message, though.\n> \n> The error, as far as I can tell, is half about a misconfigured config\n> (e.g. \"fetch = refs/heads/*:refs/remotes/[]?/*\") and half about screwy\n> remote repository (e.g. a misnamed \"[]?\" branch on the remote end can try\n> to update a broken \"refs/remotes/origin/[]?\" even the configuration is a\n> perfectly valid \"fetch = refs/heads/*:refs/remotes/origin/*\").  It may\n> make sense to reword the error message to \"ignoring\" from \"refusing\" and\n> do just that without dying here.  I dunno.\n\nYeah, I think that's right. (And this patch is also right)\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"72260","messageId":"7v4pb6dx0r.fsf@gitster.siamese.dyndns.org","threadId":"12710","inReplyTo":"alpine.LNX.1.00.0803161904360.19665@iabervon.org","subject":"Re: tracking repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-17T00:17:24Z","receivedAt":"2008-03-17T00:17:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> On Sun, 16 Mar 2008, Junio C Hamano wrote:\n> ...\n>> ...  It still has an independent issue\n>> that this is now called by \"git remote show\" or \"git remote prune\", and it\n>> will die with a nonsense \"refusing to create\" error message, though.\n>> \n>> The error, as far as I can tell, is half about a misconfigured config\n>> (e.g. \"fetch = refs/heads/*:refs/remotes/[]?/*\") and half about screwy\n>> remote repository (e.g. a misnamed \"[]?\" branch on the remote end can try\n>> to update a broken \"refs/remotes/origin/[]?\" even the configuration is a\n>> perfectly valid \"fetch = refs/heads/*:refs/remotes/origin/*\").  It may\n>> make sense to reword the error message to \"ignoring\" from \"refusing\" and\n>> do just that without dying here.  I dunno.\n>\n> Yeah, I think that's right. (And this patch is also right)\n\nWhich means that an error checking (i.e. dying) needs to be added to\nwhatever reads from config to find \"refs/heads/*:refs/remotes/[]?/*\" to\ncover the first half.  That's an configuration error and we should not\njust say \"ignoring\" but actively urge the user to correct, no?\n"},{"id":"72261","messageId":"7vlk4ichm4.fsf@gitster.siamese.dyndns.org","threadId":"12710","inReplyTo":"alpine.LNX.1.00.0803161716470.19665@iabervon.org","subject":"Re: tracking repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-17T00:35:31Z","receivedAt":"2008-03-17T00:35:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> We don't currently have any concept of an invalid refspec;\n\nWe don't? or just that parse_ref_spec() does not detect one?\n\n> ... we just have \n> things that fall back to not being patterns and not being possible to \n> match (due to one or the other side being invalid as a ref name).\n\nI am afraid that is an invitation for more bugs and confusions.\n\nIt probably is not too late to fix this; users would rather want to see\ntheir misconfigurations clearly flagged as such, rather than the code\nletting bogosity through silently and doing something that does not\nexactly match what they configured.\n\n> diff --git a/remote.c b/remote.c\n> index f3f7375..fffde34 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -404,18 +404,17 @@ struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)\n>  \t\t\trs[i].force = 1;\n>  \t\t\tsp++;\n>  \t\t}\n> -\t\tgp = strchr(sp, '*');\n> +\t\tgp = strstr(sp, \"/*\");\n>  \t\tep = strchr(sp, ':');\n>  \t\tif (gp && ep && gp > ep)\n>  \t\t\tgp = NULL;\n\nHow would this trigger?  We find * (or /*) but that is the one on the LHS,\nwhich means the spec was like \"refs/heads/foobar:refs/remotes/origin/*\",\nand it makes me wonder if we should mark this as an configuration error.\n\nDid erroring out on \"gp && ep && gp > ep\" here have issues (i.e. reject a\nvalid configuration)?\n\n>  \t\tif (ep) {\n>  \t\t\tif (ep[1]) {\n> -\t\t\t\tconst char *glob = strchr(ep + 1, '*');\n> +\t\t\t\tconst char *glob = strstr(ep + 1, \"/*\");\n>  \t\t\t\tif (!glob)\n>  \t\t\t\t\tgp = NULL;\n>  \t\t\t\tif (gp)\n> -\t\t\t\t\trs[i].dst = xstrndup(ep + 1,\n> -\t\t\t\t\t\t\t     glob - ep - 1);\n> +\t\t\t\t\trs[i].dst = xstrndup(ep + 1, glob - ep);\n\nThis truncates \"refs/heads/*:refs/remotes/origin/*/bar\" as if it did not\nhave \"/bar\" without any error indication.  The same questions apply.\n"},{"id":"72266","messageId":"alpine.LNX.1.00.0803162143230.19665@iabervon.org","threadId":"12710","inReplyTo":"7vlk4ichm4.fsf@gitster.siamese.dyndns.org","subject":"Re: tracking repository","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-17T02:13:36Z","receivedAt":"2008-03-17T02:13:36Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 16 Mar 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > We don't currently have any concept of an invalid refspec;\n> \n> We don't? or just that parse_ref_spec() does not detect one?\n\nI don't think we ever formalized anything; it just makes sure not to \nactually create anything bad, and doesn't give any feedback on the \nconfiguration.\n\n> > ... we just have \n> > things that fall back to not being patterns and not being possible to \n> > match (due to one or the other side being invalid as a ref name).\n> \n> I am afraid that is an invitation for more bugs and confusions.\n> \n> It probably is not too late to fix this; users would rather want to see\n> their misconfigurations clearly flagged as such, rather than the code\n> letting bogosity through silently and doing something that does not\n> exactly match what they configured.\n\nI think I wasn't sure that aborting on invalid input wouldn't cause worse \nproblems. There were actually a number of tests, IIRC, that required that \ncertain configurations silently did nothing, which I mostly left alone.\n\nAlso, it's possible that we're parsing refspecs because we're using \"git \nremote\" to modify the configuration to replace an invalid refspec, and it \nwould be unfortunate to die() because the remote has an invalid refspec.\n \n> > diff --git a/remote.c b/remote.c\n> > index f3f7375..fffde34 100644\n> > --- a/remote.c\n> > +++ b/remote.c\n> > @@ -404,18 +404,17 @@ struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)\n> >  \t\t\trs[i].force = 1;\n> >  \t\t\tsp++;\n> >  \t\t}\n> > -\t\tgp = strchr(sp, '*');\n> > +\t\tgp = strstr(sp, \"/*\");\n> >  \t\tep = strchr(sp, ':');\n> >  \t\tif (gp && ep && gp > ep)\n> >  \t\t\tgp = NULL;\n> \n> How would this trigger?  We find * (or /*) but that is the one on the LHS,\n> which means the spec was like \"refs/heads/foobar:refs/remotes/origin/*\",\n> and it makes me wonder if we should mark this as an configuration error.\n\nIt's that case (there's a *, but not until after the :); I think the \nhistory was that the code first just looked for <a>:<b>, and used it for \nnon-pattern matches, and then started using <a>/*:<b>/* as a pattern \nfirst, and then I made the C version match that shell version.\n\n> Did erroring out on \"gp && ep && gp > ep\" here have issues (i.e. reject a\n> valid configuration)?\n\nNope.\n\n> >  \t\tif (ep) {\n> >  \t\t\tif (ep[1]) {\n> > -\t\t\t\tconst char *glob = strchr(ep + 1, '*');\n> > +\t\t\t\tconst char *glob = strstr(ep + 1, \"/*\");\n> >  \t\t\t\tif (!glob)\n> >  \t\t\t\t\tgp = NULL;\n> >  \t\t\t\tif (gp)\n> > -\t\t\t\t\trs[i].dst = xstrndup(ep + 1,\n> > -\t\t\t\t\t\t\t     glob - ep - 1);\n> > +\t\t\t\t\trs[i].dst = xstrndup(ep + 1, glob - ep);\n> \n> This truncates \"refs/heads/*:refs/remotes/origin/*/bar\" as if it did not\n> have \"/bar\" without any error indication.  The same questions apply.\n\nThat one was just an oversight. I was just glad I didn't have to make it \nactually support that refspec and have it do the obvious (but annoying to \nimplement) thing.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"72268","messageId":"alpine.LNX.1.00.0803162234270.19665@iabervon.org","threadId":"12710","inReplyTo":"7vlk4ichm4.fsf@gitster.siamese.dyndns.org","subject":"Re: tracking repository","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-17T02:37:53Z","receivedAt":"2008-03-17T02:37:53Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 16 Mar 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > We don't currently have any concept of an invalid refspec;\n> \n> We don't? or just that parse_ref_spec() does not detect one?\n> \n> > ... we just have \n> > things that fall back to not being patterns and not being possible to \n> > match (due to one or the other side being invalid as a ref name).\n> \n> I am afraid that is an invitation for more bugs and confusions.\n\nYeah, we're definitely too lenient. t3200-branch has been using the \nrefspec \"=\" since July without anybody noticing that it's wrong.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"72271","messageId":"7v3aqpdc4n.fsf@gitster.siamese.dyndns.org","threadId":"12710","inReplyTo":"alpine.LNX.1.00.0803162234270.19665@iabervon.org","subject":"Re: tracking repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-17T07:48:40Z","receivedAt":"2008-03-17T07:48:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> On Sun, 16 Mar 2008, Junio C Hamano wrote:\n>\n>> Daniel Barkalow <barkalow@iabervon.org> writes:\n>> \n>> > We don't currently have any concept of an invalid refspec;\n>> \n>> We don't? or just that parse_ref_spec() does not detect one?\n>> \n>> > ... we just have \n>> > things that fall back to not being patterns and not being possible to \n>> > match (due to one or the other side being invalid as a ref name).\n>> \n>> I am afraid that is an invitation for more bugs and confusions.\n>\n> Yeah, we're definitely too lenient. t3200-branch has been using the \n> refspec \"=\" since July without anybody noticing that it's wrong.\n\nYou mean these that came in 6f084a5 (branch --track: code cleanup and\nsaner handling of local branches, 2007-07-10), right?\n\nWill you fix them while you come up with a patch to tighten the parsing?\nFixing these does seem to trigger problems in later parts of the test\nsequence.\n\n---\n\n t/t3200-branch.sh |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 38a90ad..48b8a45 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -155,10 +155,10 @@ test_expect_success 'test tracking setup via config' \\\n \n test_expect_success 'avoid ambiguous track' '\n \tgit config branch.autosetupmerge true &&\n-\tgit config remote.ambi1.url = lalala &&\n-\tgit config remote.ambi1.fetch = refs/heads/lalala:refs/heads/master &&\n-\tgit config remote.ambi2.url = lilili &&\n-\tgit config remote.ambi2.fetch = refs/heads/lilili:refs/heads/master &&\n+\tgit config remote.ambi1.url lalala &&\n+\tgit config remote.ambi1.fetch refs/heads/lalala:refs/heads/master &&\n+\tgit config remote.ambi2.url lilili &&\n+\tgit config remote.ambi2.fetch refs/heads/lilili:refs/heads/master &&\n \tgit branch all1 master &&\n \ttest -z \"$(git config branch.all1.merge)\"\n '\n"},{"id":"72299","messageId":"alpine.LNX.1.00.0803171219260.19665@iabervon.org","threadId":"12710","inReplyTo":"7v3aqpdc4n.fsf@gitster.siamese.dyndns.org","subject":"Re: tracking repository","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-17T16:23:26Z","receivedAt":"2008-03-17T16:23:26Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 17 Mar 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > On Sun, 16 Mar 2008, Junio C Hamano wrote:\n> >\n> >> Daniel Barkalow <barkalow@iabervon.org> writes:\n> >> \n> >> > We don't currently have any concept of an invalid refspec;\n> >> \n> >> We don't? or just that parse_ref_spec() does not detect one?\n> >> \n> >> > ... we just have \n> >> > things that fall back to not being patterns and not being possible to \n> >> > match (due to one or the other side being invalid as a ref name).\n> >> \n> >> I am afraid that is an invitation for more bugs and confusions.\n> >\n> > Yeah, we're definitely too lenient. t3200-branch has been using the \n> > refspec \"=\" since July without anybody noticing that it's wrong.\n> \n> You mean these that came in 6f084a5 (branch --track: code cleanup and\n> saner handling of local branches, 2007-07-10), right?\n\nYup. That actually makes me wonder if git-config should complain if it \ndoesn't do anything due to the value_regex not matching anything.\n\n> Will you fix them while you come up with a patch to tighten the parsing?\n> Fixing these does seem to trigger problems in later parts of the test\n> sequence.\n\nYeah, we just need to remove those options once that case finishes (or I \nsuppose that test could go at the end).\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}