{"thread":{"id":"29490","subject":"Alternates corruption issue","startedAt":"2012-01-31T14:05:29Z","lastAt":"2012-02-03T21:53:07Z","messageCount":19,"participants":["Richard Purdie","Jeff King","Junio C Hamano","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"183414","messageId":"1328018729.13744.26.camel@ted","threadId":"29490","inReplyTo":null,"subject":"Alternates corruption issue","fromName":"Richard Purdie","fromEmail":"richard.purdie@linuxfoundation.org","sentAt":"2012-01-31T14:05:29Z","receivedAt":"2012-01-31T14:05:29Z","isPatch":false,"sender":{"key":"richard.purdie@linuxfoundation.org","avatar":null},"body":"I have a problem with git clone commands using alternates failing by\nmixing up different repositories. I have a situation where I could end\nup with both:\n\n/srv/mirrors/repo\n/srv/mirrors/repo.git\n\nas bare clones.\n\nI then try cloning \"repo\" with alternates with the command:\n\n$ git clone -s -n /srv/mirrors/repo /tmp/foo\nCloning into /tmp/foo...\ndone.\n\n$ cat /tmp/foo/.git/objects/info/alternates\n/srv/mirrors/repo.git/objects\n\nNote how I'm now referencing repo.git, not repo. This doesn't work as\nexpected giving some very bizarre results when actually using the\nrepository.\n\nI appreciate this is a rather bizarre corner case but its one that is\nbreaking the build system I work with. Ideally people would use a\nconsistent URL for the same repository but we have an example where they\nhaven't and this really shouldn't break like this.\n\nLooking at the code, the cause seems to be\n\nclone.c:get_repo_path():\n\n\tstatic char *suffix[] = { \"/.git\", \".git\", \"\" };\n\nsince its looking in order for:\n repo/.git (fails)\n repo.git (suceeds, incorrect)\n repo (never looked at)\n\nI'm not sure what would break if that order were to change, swapping the\nlast two options.\n\nI can \"force\" the issue by running:\n\ngit clone -s -n /srv/mirrors/repo/ /tmp/foo\n\nbut this results in the slightly odd looking:\n\n$ cat /tmp/foo/.git/objects/info/alternates\n/srv/mirrors/repo//objects\n\nwhich does at least work.\n\nAny idea if its possible to fix the root cause of this problem?\n\nCheers,\n\nRichard\n"},{"id":"183427","messageId":"20120131193922.GA31551@sigill.intra.peff.net","threadId":"29490","inReplyTo":"1328018729.13744.26.camel@ted","subject":"Re: Alternates corruption issue","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-31T19:39:22Z","receivedAt":"2012-01-31T19:39:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 31, 2012 at 02:05:29PM +0000, Richard Purdie wrote:\n\n> I have a problem with git clone commands using alternates failing by\n> mixing up different repositories. I have a situation where I could end\n> up with both:\n> \n> /srv/mirrors/repo\n> /srv/mirrors/repo.git\n\nI think the alternates thing is just a side effect. The real problem is\nthat clone is picking the wrong repo to clone from! So a simple complete\ntest would be something like:\n\n    # make confusing bare repos\n    git init --bare bare &&\n    git init --bare bare.git &&\n\n    # now make some commits; after this\n    # segment, \"bare\" should have its tip\n    # at commit \"one\", and \"bare.git\" should\n    # be at \"two\".\n    git init source &&\n    (cd source &&\n     echo content >file &&\n     git add file &&\n     git commit -m one &&\n     git push ../bare HEAD &&\n     echo content >>file &&\n     git commit -a -m two &&\n     git push ../bare.git HEAD\n    ) &&\n\n    # now try to clone bare and see which one we get;\n    # we would expect the tip to be at \"one\", but it's not\n    git clone bare clone &&\n    cd clone &&\n    echo one >expect &&\n    git log -1 --format=%s >actual &&\n    diff -u expect actual\n\n    # we cloned the wrong repository!\n\n> Looking at the code, the cause seems to be\n> \n> clone.c:get_repo_path():\n> \n> \tstatic char *suffix[] = { \"/.git\", \".git\", \"\" };\n> \n> since its looking in order for:\n>  repo/.git (fails)\n>  repo.git (suceeds, incorrect)\n>  repo (never looked at)\n\nYeah, this seems brain-dead. It's good that git looks through a magic\nlist to guess what I meant, but shouldn't the first thing it tries be\nthe literal string that I gave, and only _then_ kick in the magic?\n\nBut even weirder, this doesn't match the lookup in other parts of git. I\nsee four distinct places in git where we do repository lookup:\n\n  1. get_repo_path used by clone (i.e., this)\n\n  2. enter_repo used by upload-pack, receive-pack, and git-daemon; this\n     looks first for \".git/.git\", to find a non-bare repo which is named\n     like a bare one.\n\n  3. setup_git_directory_*, used by most git programs. This starts\n     looking in the current directory and going up, looking first for\n     \".git\", and then checking for a bare repo. So if you were to do\n     \"cd bare && git foo\", then we would correctly find the current\n     directory, not \"bare.git\".\n\n  4. setup_explicit_git_dir, which gets called if you use --git-dir or\n     set GIT_DIR. This expects to find the repo directly in the\n     directory you specified, with no magic.\n\nSo I think (3) and (4) are sensible, but I tend to think that (1) and\n(2) are wrong, and should prefer the directory you gave before the\nmagic.\n\n> I'm not sure what would break if that order were to change, swapping the\n> last two options.\n\nI believe that would work in your case, but it seems like the most\ncorrect thing would actually be:\n\n  { \"\", \"/.git\", \".git\" }\n\nThat is:\n\n  1. Try the literal path the user gave as a repo\n\n  2. Otherwise, try it as the root of a working tree (containing .git)\n\n  3. Otherwise, assume they were too lazy to type \".git\" and include it\n\nIf you simply swap the last two, then there is one obscure case you\nwould miss: a bare repo which contains a .git directory. Of course, that\nseems like an insane thing to have, but if you did have it, I would\nexpect git to follow the rules I outlined above. That being said,\nif you follow the rules in setup_git_directory_gently_1, it actually\nprefers \"/.git\" to a bare repo. So probably your swap is better, as it\nmatches the lookup here with the regular setup_git_directory one.\n\nOne slight complication is that get_repo_path only checks whether the\npaths exist, not whether they are actually git repositories. So by\nsimply swapping the order, you are breaking some other obscure cases,\nlike where a \"foo\" directory exists but is _not_ a git repo, and a\n\"foo.git\" exists alongside it (which sounds a bit crazy, but I could see\nsomebody using that scheme if they wanted didn't want the git repository\nto live inside the worktree for some reason).\n\nOne way of dealing with that would be to make get_repo_path a little\nmore robust by only selecting paths which actually look like git\ndirectories.\n\nThe patch below does this, and makes my test above pass. But I think\nbefore talking too much about code, it would be best to decide on what\nthe lookup order _should_ be, and then we can write tests and implement\nto them.\n\n-Peff\n\n---\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 9084feb..0fbbae9 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -108,7 +108,7 @@ static const char *argv_submodule[] = {\n \n static char *get_repo_path(const char *repo, int *is_bundle)\n {\n-\tstatic char *suffix[] = { \"/.git\", \".git\", \"\" };\n+\tstatic char *suffix[] = { \"/.git\", \"\", \".git\" };\n \tstatic char *bundle_suffix[] = { \".bundle\", \"\" };\n \tstruct stat st;\n \tint i;\n@@ -118,7 +118,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)\n \t\tpath = mkpath(\"%s%s\", repo, suffix[i]);\n \t\tif (stat(path, &st))\n \t\t\tcontinue;\n-\t\tif (S_ISDIR(st.st_mode)) {\n+\t\tif (S_ISDIR(st.st_mode) && is_git_directory(path)) {\n \t\t\t*is_bundle = 0;\n \t\t\treturn xstrdup(absolute_path(path));\n \t\t} else if (S_ISREG(st.st_mode) && st.st_size > 8) {\ndiff --git a/cache.h b/cache.h\nindex 9bd8c2d..7b2857f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -436,6 +436,7 @@ extern char *get_object_directory(void);\n extern char *get_index_file(void);\n extern char *get_graft_file(void);\n extern int set_git_dir(const char *path);\n+extern int is_git_directory(const char *path);\n extern const char *get_git_namespace(void);\n extern const char *strip_namespace(const char *namespaced_ref);\n extern const char *get_git_work_tree(void);\ndiff --git a/path.c b/path.c\nindex b6f71d1..1ca6567 100644\n--- a/path.c\n+++ b/path.c\n@@ -293,7 +293,7 @@ const char *enter_repo(const char *path, int strict)\n \n \tif (!strict) {\n \t\tstatic const char *suffix[] = {\n-\t\t\t\".git/.git\", \"/.git\", \".git\", \"\", NULL,\n+\t\t\t\"/.git\", \"\", \".git/.git\", \".git\", NULL,\n \t\t};\n \t\tconst char *gitfile;\n \t\tint len = strlen(path);\n@@ -324,8 +324,11 @@ const char *enter_repo(const char *path, int strict)\n \t\t\treturn NULL;\n \t\tlen = strlen(used_path);\n \t\tfor (i = 0; suffix[i]; i++) {\n+\t\t\tstruct stat st;\n \t\t\tstrcpy(used_path + len, suffix[i]);\n-\t\t\tif (!access(used_path, F_OK)) {\n+\t\t\tif (!stat(used_path, &st) &&\n+\t\t\t    (S_ISREG(st.st_mode) ||\n+\t\t\t     (S_ISDIR(st.st_mode) && is_git_directory(used_path)))) {\n \t\t\t\tstrcat(validated_path, suffix[i]);\n \t\t\t\tbreak;\n \t\t\t}\ndiff --git a/setup.c b/setup.c\nindex 61c22e6..7a3618f 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -247,7 +247,7 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n  *    a proper \"ref:\", or a regular file HEAD that has a properly\n  *    formatted sha1 object name.\n  */\n-static int is_git_directory(const char *suspect)\n+int is_git_directory(const char *suspect)\n {\n \tchar path[PATH_MAX];\n \tsize_t len = strlen(suspect);\n"},{"id":"183430","messageId":"7v1uqf8vqu.fsf@alter.siamese.dyndns.org","threadId":"29490","inReplyTo":"20120131193922.GA31551@sigill.intra.peff.net","subject":"Re: Alternates corruption issue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-31T20:25:45Z","receivedAt":"2012-01-31T20:25:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I believe that would work in your case, but it seems like the most\n> correct thing would actually be:\n>\n>   { \"\", \"/.git\", \".git\" }\n>\n> That is:\n>\n>   1. Try the literal path the user gave as a repo\n>\n>   2. Otherwise, try it as the root of a working tree (containing .git)\n>\n>   3. Otherwise, assume they were too lazy to type \".git\" and include it\n\nThat sounds sensible, together with this, to which I agree with:\n\n> One way of dealing with that would be to make get_repo_path a little\n> more robust by only selecting paths which actually look like git\n> directories.\n\n... but ...\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 9084feb..0fbbae9 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -108,7 +108,7 @@ static const char *argv_submodule[] = {\n>  \n>  static char *get_repo_path(const char *repo, int *is_bundle)\n>  {\n> -\tstatic char *suffix[] = { \"/.git\", \".git\", \"\" };\n> +\tstatic char *suffix[] = { \"/.git\", \"\", \".git\" };\n\n... this does not match that simple and clear guideline.\n\nShouldn't this simply be { \"\", \"/.git\", \".git\" }?\n\n> diff --git a/path.c b/path.c\n> index b6f71d1..1ca6567 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -293,7 +293,7 @@ const char *enter_repo(const char *path, int strict)\n>  \n>  \tif (!strict) {\n>  \t\tstatic const char *suffix[] = {\n> -\t\t\t\".git/.git\", \"/.git\", \".git\", \"\", NULL,\n> +\t\t\t\"/.git\", \"\", \".git/.git\", \".git\", NULL,\n>  \t\t};\n\nNeither does this.\n\nShouldn't this be { \"\", \"/.git\", \".git\", \".git/.git\", NULL }?\n\nI must be missing something from your description...\n"},{"id":"183431","messageId":"20120131204417.GA30969@sigill.intra.peff.net","threadId":"29490","inReplyTo":"7v1uqf8vqu.fsf@alter.siamese.dyndns.org","subject":"Re: Alternates corruption issue","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-31T20:44:17Z","receivedAt":"2012-01-31T20:44:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 31, 2012 at 12:25:45PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I believe that would work in your case, but it seems like the most\n> > correct thing would actually be:\n> >\n> >   { \"\", \"/.git\", \".git\" }\n> >\n> > That is:\n> >\n> >   1. Try the literal path the user gave as a repo\n> >\n> >   2. Otherwise, try it as the root of a working tree (containing .git)\n> >\n> >   3. Otherwise, assume they were too lazy to type \".git\" and include it\n> \n> That sounds sensible, together with this, to which I agree with:\n> [...]\n> ... but ...\n> [...]\n> > -\tstatic char *suffix[] = { \"/.git\", \".git\", \"\" };\n> > +\tstatic char *suffix[] = { \"/.git\", \"\", \".git\" };\n> \n> ... this does not match that simple and clear guideline.\n> \n> Shouldn't this simply be { \"\", \"/.git\", \".git\" }?\n\nNo, it does not match. While the sequence I outlined above makes the\nmost sense to me, it does not match what setup_git_directory does, which\nprefers \"foo/.git\" to using \"foo\" as a bare repo. I think being\nconsistent between all of the lookup points makes sense. The patch took\nthe least-invasive approach and aligned clone and enter_repo with\nsetup_git_directory.\n\nHowever, we could also tweak setup_git_directory to prefer bare repos\nover \".git\" to keep things consistent. While it makes me feel good from\na theoretical standpoint (because the rules above seem simple and\nintuitive to me), I'm not sure it's a good idea in practice.\n\nThe case we would \"catch\" with such a change is when you have a \".git\"\ndirectory inside a bare repo. Right now, we prefer the \".git\" inside it,\nand ignore the containing bare repo. With such a change, we would prefer\nthe outer bare repo. Which makes sense to me. It does break a use\ncase like this:\n\n  # make a new repo\n  git init\n  cd .git\n  # now track parts of the repo\n  git init\n  git add config\n  git commit -m 'tracking our repo config'\n\nbut I'm not sure if that is sane or not.\n\nBut also consider false positives. What if you have a repository that\nlooks like a git repo (i.e., has \"objects\", \"refs\", and \"HEAD\" in it),\nbut also has \".git\". Right now we say \"Oh, it has .git, that must be the\nrepo\". But with the proposed change, we could accidentally find the\nenclosing repo.\n\nNow, the chances of is_git_directory being wrong seem quite slim to me.\nBut then, the chances of somebody actually have a repo with a \".git\"\n_inside_ it seem pretty slim to me. So I think we are really dealing\nwith a tiny corner case, and it is perhaps anybody's guess whether\nanyone is depending on the current behavior in the wild. I don't overly\ncare either way, but when in doubt, I tend to stick with the existing\nbehavior.\n\n> >  \tif (!strict) {\n> >  \t\tstatic const char *suffix[] = {\n> > -\t\t\t\".git/.git\", \"/.git\", \".git\", \"\", NULL,\n> > +\t\t\t\"/.git\", \"\", \".git/.git\", \".git\", NULL,\n> >  \t\t};\n> \n> Neither does this.\n> \n> Shouldn't this be { \"\", \"/.git\", \".git\", \".git/.git\", NULL }?\n\nRight, same case.\n\n> I must be missing something from your description...\n\nI mentioned the issue in my original message, but perhaps didn't\nemphasize it very well.\n\n-Peff\n"},{"id":"183433","messageId":"20120131214047.GA13547@burratino","threadId":"29490","inReplyTo":"20120131204417.GA30969@sigill.intra.peff.net","subject":"Re: Alternates corruption issue","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-31T21:40:47Z","receivedAt":"2012-01-31T21:40:47Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> No, it does not match. While the sequence I outlined above makes the\n> most sense to me, it does not match what setup_git_directory does, which\n> prefers \"foo/.git\" to using \"foo\" as a bare repo. I think being\n> consistent between all of the lookup points makes sense. The patch took\n> the least-invasive approach and aligned clone and enter_repo with\n> setup_git_directory.\n>\n> However, we could also tweak setup_git_directory to prefer bare repos\n> over \".git\" to keep things consistent. While it makes me feel good from\n> a theoretical standpoint (because the rules above seem simple and\n> intuitive to me), I'm not sure it's a good idea in practice.\n\nWait, don't these two functions serve two completely different purposes?\n\nOne is the implementation of (A):\n\n\tcd foo\n\tgit rev-parse --git-dir\n\nThe other implements (B):\n\n\tgit ls-remote foo\n\nIf \"foo\" is actually a bare repository that moonlights as a worktree for\na non-bare repository, then:\n\n 1) Whoever set up these directories is completely insane[*].  Maybe we\n    should emit a warning.\n\n 2) As a naive user, I would expect (A) to give a different result\n    from (B).\n\nHope that helps,\nJonathan\n\n[*] ok, ok, they can be confused instead of insane:\nhttp://bugs.debian.org/399041\n"},{"id":"183434","messageId":"20120131214740.GA2465@sigill.intra.peff.net","threadId":"29490","inReplyTo":"20120131214047.GA13547@burratino","subject":"Re: Alternates corruption issue","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-31T21:47:40Z","receivedAt":"2012-01-31T21:47:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 31, 2012 at 03:40:47PM -0600, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > No, it does not match. While the sequence I outlined above makes the\n> > most sense to me, it does not match what setup_git_directory does, which\n> > prefers \"foo/.git\" to using \"foo\" as a bare repo. I think being\n> > consistent between all of the lookup points makes sense. The patch took\n> > the least-invasive approach and aligned clone and enter_repo with\n> > setup_git_directory.\n> >\n> > However, we could also tweak setup_git_directory to prefer bare repos\n> > over \".git\" to keep things consistent. While it makes me feel good from\n> > a theoretical standpoint (because the rules above seem simple and\n> > intuitive to me), I'm not sure it's a good idea in practice.\n> \n> Wait, don't these two functions serve two completely different purposes?\n\nYes, but I would expect the lookup to be the same.\n\n> One is the implementation of (A):\n> \n> \tcd foo\n> \tgit rev-parse --git-dir\n> \n> The other implements (B):\n> \n> \tgit ls-remote foo\n\nRight. But would you expect:\n\n  git ls-remote foo\n\nto behave the same as:\n\n  cd foo\n  git ls-remote .\n\nand would you furthermore expect that to behave the same as:\n\n  cd foo\n  git rev-parse --git-dir\n\n?\n\nMaybe I am crazy, but I see all of them as ways of specifying \"I am\ninterested in repository foo\", and any lookup magic should be the same\nin all cases.\n\n> If \"foo\" is actually a bare repository that moonlights as a worktree for\n> a non-bare repository, then:\n> \n>  1) Whoever set up these directories is completely insane[*].  Maybe we\n>     should emit a warning.\n\nYes, I think we are dealing with an insane corner case.\n\n>  2) As a naive user, I would expect (A) to give a different result\n>     from (B).\n\nWhy?\n\n-Peff\n"},{"id":"183435","messageId":"20120131215501.GF13252@burratino","threadId":"29490","inReplyTo":"20120131214740.GA2465@sigill.intra.peff.net","subject":"Re: Alternates corruption issue","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-31T21:55:01Z","receivedAt":"2012-01-31T21:55:01Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Right. But would you expect:\n>\n>   git ls-remote foo\n>\n> to behave the same as:\n>\n>   cd foo\n>   git ls-remote .\n>\n> and would you furthermore expect that to behave the same as:\n>\n>   cd foo\n>   git rev-parse --git-dir\n>\n> ?\n\nNah.  I never run \"git ls-remote .\". ;-)\n\n[...]\n>>  2) As a naive user, I would expect (A) to give a different result\n>>     from (B).\n>\n> Why?\n\nNormally git commands expect me to be in (possibly a deeply nested\nsubdirectory) of the worktree.  The typical case is a non-bare\nrepository.  I have been taught that it walks to the toplevel and\nfinds a \".git\" directory.\n\nBy contrast, the path passed to git transport commands like \"git fetch\notherhost:/foo/bar/baz.git\" is a path to the git repository metadata.\nI am not allowed to pass a path to a subdirectory, for example.\n\nAs a user, when would the distinction ever come up?  One is a fuzzy\n\"which repository am I working in\", and the other is a precise \"point\nme to the metadata directory, but I allow you to abbreviate a little\nby leaving out the trailing .git\".\n"},{"id":"183436","messageId":"20120131220510.GA3253@sigill.intra.peff.net","threadId":"29490","inReplyTo":"20120131215501.GF13252@burratino","subject":"Re: Alternates corruption issue","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-31T22:05:10Z","receivedAt":"2012-01-31T22:05:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 31, 2012 at 03:55:01PM -0600, Jonathan Nieder wrote:\n\n> >>  2) As a naive user, I would expect (A) to give a different result\n> >>     from (B).\n> >\n> > Why?\n> \n> Normally git commands expect me to be in (possibly a deeply nested\n> subdirectory) of the worktree.  The typical case is a non-bare\n> repository.  I have been taught that it walks to the toplevel and\n> finds a \".git\" directory.\n> \n> By contrast, the path passed to git transport commands like \"git fetch\n> otherhost:/foo/bar/baz.git\" is a path to the git repository metadata.\n> I am not allowed to pass a path to a subdirectory, for example.\n\nTrue. But I consider that to make the walk-backwards-from-pwd case\nsimply a superset. That is, in (A) we are walking backwards and trying\nto apply the lookup rule from (B) individually to each directory we\nconsider (though even that is not entirely true, as we don't look for\nparallel \"$PWD.git\" directories in the walk).\n\nI'll admit I don't care that much, though. This is extremely unlikely to\ncome up. The real issue is fixing the fact that we prefer \"foo.git\" to\n\"foo\", even when the user told us \"foo\". I am content to leave the rest\nof it as-is, which is what my original patch did.\n\n-Peff\n"},{"id":"183437","messageId":"20120131222258.GG13252@burratino","threadId":"29490","inReplyTo":"20120131220510.GA3253@sigill.intra.peff.net","subject":"Re: Alternates corruption issue","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-31T22:22:58Z","receivedAt":"2012-01-31T22:22:58Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> True. But I consider that to make the walk-backwards-from-pwd case\n> simply a superset. That is, in (A) we are walking backwards and trying\n> to apply the lookup rule from (B) individually to each directory we\n> consider (though even that is not entirely true, as we don't look for\n> parallel \"$PWD.git\" directories in the walk).\n\nThat parenthesis is important.  Although in a nicer world maybe we\nwould want some symmetry like this, (A) and (B) really have nothing to\ndo with each other.\n\nForgetting the algorithms:\n\n (A) means \"find what repository we are in the worktree for, or,\n barring that, what bare repository we are in\".\n\n (B) means \"find which repository the user pointed to.  To be extra\n nice, we allow a '.git' extension to be left out, so the URL used\n doesn't have to include the redundant information that this is a\n git repository, and we even allow pointing to the toplevel of a\n worktree instead of a repository, too.\"\n\nNotice that the above description does not exactly match the actual\nbehavior of git.  For example, if someone has a directory layout like\nthis:\n\n  repo-manipulator/\n     .git/\n     src/\n     testcases/\n       repo1.git/\n       repo2.git/\n\nand git is run from repo1.git, according to the description above, the\nnaive user _wanted_ git commands to apply to the toplevel repository.\nAnd in practice, I think that's often true, though changing the\nbehavior of git to match that would not be worth the downsides.\n\n> I'll admit I don't care that much, though. This is extremely unlikely to\n> come up.\n\nI admit part of the reason I care is that just putting \"\" first would\nprobably taken care of the more important part of\n<http://bugs.debian.org/399041>.\n\nAnyway, thanks for explaining.  Hopefully I can get to this soon and\nfactor out a common function for get_repo_path and enter_repo to call\nso playing with the ordering becomes a little less scary. ;-)\n\nJonathan\n"},{"id":"183438","messageId":"20120131224240.GA3844@sigill.intra.peff.net","threadId":"29490","inReplyTo":"20120131222258.GG13252@burratino","subject":"Re: Alternates corruption issue","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-31T22:42:40Z","receivedAt":"2012-01-31T22:42:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 31, 2012 at 04:22:58PM -0600, Jonathan Nieder wrote:\n\n> Forgetting the algorithms:\n> \n>  (A) means \"find what repository we are in the worktree for, or,\n>  barring that, what bare repository we are in\".\n> \n>  (B) means \"find which repository the user pointed to.  To be extra\n>  nice, we allow a '.git' extension to be left out, so the URL used\n>  doesn't have to include the redundant information that this is a\n>  git repository, and we even allow pointing to the toplevel of a\n>  worktree instead of a repository, too.\"\n\nI can see that as a plausible interpretation. But I think your \"means\"\nis kind of vague.  In other words, we are assigning a meaning that is\nnot actually documented anywhere, and which we are hoping matches user\nexpectations. So the correct behavior kind of depends on your mental\nmodel. IOW, I find your mental model plausible, but it is not the one\nthat I had.\n\nBut like I said, I'm OK either way. I'll prepare this fix with the least\nbehavior change possible. Then doing your change on top should be a\ntrivial rearrangement of the lookup arrays (the \"hard\" part for your\nproposed change is adding in the is_git_directory() calls[1], but I'll\ndo that as part of mine).\n\n> I admit part of the reason I care is that just putting \"\" first would\n> probably taken care of the more important part of\n> <http://bugs.debian.org/399041>.\n\nWould that fix it? If I understand it, the repo in question is bare with\na \".git\" directory inside it. Your proposed change would make \"git\nls-remote foo\" work, but wouldn't \"cd foo && git log\" still be broken?\nAt the very least, it would show a completely different repo than\nls-remote.\n\n-Peff\n\n[1] Without the is_git_directory() check, the ordering you propose\n    breaks things very badly. Because if I point to a non-bare directory\n    \"foo\", we say \"OK, foo exists\" and never look at \"foo/.git\". So\n    instead, we have to say \"OK, foo exists, but hey, it looks nothing\n    like a .git directory\" and keep trying.\n"},{"id":"183442","messageId":"20120131225909.GA13780@burratino","threadId":"29490","inReplyTo":"20120131224240.GA3844@sigill.intra.peff.net","subject":"Re: Alternates corruption issue","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-31T22:59:09Z","receivedAt":"2012-01-31T22:59:09Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Tue, Jan 31, 2012 at 04:22:58PM -0600, Jonathan Nieder wrote:\n\n>> I admit part of the reason I care is that just putting \"\" first would\n>> probably taken care of the more important part of\n>> <http://bugs.debian.org/399041>.\n>\n> Would that fix it? If I understand it, the repo in question is bare with\n> a \".git\" directory inside it.\n\nThe layout was foo/.git/.git.  The real repository was made with plain\n\"git init\" or \"git clone\", and then \"git svn init\" or similar was run\nfrom within .git, creating a .git subdirectory.  (Something more must\nhave happened, actually, since the tree was weirdly sparse:\n\n\t$ find .git/.git\n\t.git/.git\n\t.git/.git/svn\n\t.git/.git/svn/git-svn\n\t.git/.git/svn/git-svn/.rev_db\n\n)\n\nIn the current scheme, to access such a broken repository remotely,\nyou have to use a URL \".../foo\".  URLs pointing to foo/.git (like gitk\nonce used) end up rewritten as .git/.git.  Of course, your patch with\nthe is_git_directory() will fix this specific case. :)\n\nNot a huge deal, especially since \"git svn init\" prints out where it\nis writing these days.\n"},{"id":"183672","messageId":"20120202215913.GA26727@sigill.intra.peff.net","threadId":"29490","inReplyTo":"20120131222258.GG13252@burratino","subject":"Re: Alternates corruption issue","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T21:59:13Z","receivedAt":"2012-02-02T21:59:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 31, 2012 at 04:22:58PM -0600, Jonathan Nieder wrote:\n\n> Anyway, thanks for explaining.  Hopefully I can get to this soon and\n> factor out a common function for get_repo_path and enter_repo to call\n> so playing with the ordering becomes a little less scary. ;-)\n\nSo here's what I think we should apply to fix the particular issue that\nRichard mentioned at the start of this thread.\n\nBesides tweaking the ordering, the main contribution is a set of tests\nthat actually check some of these ambiguous cases (especially checking\nthe fact that both code paths behave identically!). I didn't factor the\nlogic into a common function, but doing so should be a little safer on\ntop of these tests, if you're still interested.\n\n-- >8 --\nSubject: standardize and improve lookup rules for external local repos\n\nWhen you specify a local repository on the command line of\nclone, ls-remote, upload-pack, receive-pack, or upload-archive,\nor in a request to git-daemon, we perform a little bit of\nlookup magic, doing things like looking in working trees for\n.git directories and appending \".git\" for bare repos.\n\nFor clone, this magic happens in get_repo_path. For\neverything else, it happens in enter_repo. In both cases,\nthere are some ambiguous or confusing cases that aren't\nhandled well, and there is one case that is not handled the\nsame by both methods.\n\nThis patch tries to provide (and test!) standard, sensible\nlookup rules for both code paths. The intended changes are:\n\n  1. When looking up \"foo\", we have always preferred\n     a working tree \"foo\" (containing \"foo/.git\" over the\n     bare \"foo.git\". But we did not prefer a bare \"foo\" over\n     \"foo.git\". With this patch, we do so.\n\n  2. We would select directories that existed but didn't\n     actually look like git repositories. With this patch,\n     we make sure a selected directory looks like a git\n     repo. Not only is this more sensible in general, but it\n     will help anybody who is negatively affected by change\n     (1) negatively (e.g., if they had \"foo.git\" next to its\n     separate work tree \"foo\", and expect to keep finding\n     \"foo.git\" when they reference \"foo\").\n\n  3. The enter_repo code path would, given \"foo\", look for\n     \"foo.git/.git\" (i.e., do the \".git\" append magic even\n     for a repo with working tree). The clone code path did\n     not; with this patch, they now behave the same.\n\nIn the unlikely case of a working tree overlaying a bare\nrepo (i.e., a \".git\" directory _inside_ a bare repo), we\ncontinue to treat it as a working tree (prefering the\n\"inner\" .git over the bare repo). This is mainly because the\ncombination seems nonsensical, and I'd rather stick with\nexisting behavior on the off chance that somebody is relying\non it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI could go either way with the \"prefer foo to foo/.git\" thing we've been\ndiscussing. I left it as-is here out of a sense of being conservative.\nBut I have no objection at all to you doing a patch on top that\njustifies the change. You'll just need to tweak the \"check\" line of the\nfinal test.\n\n builtin/clone.c           |    4 +-\n cache.h                   |    1 +\n path.c                    |    7 ++-\n setup.c                   |    2 +-\n t/t5900-repo-selection.sh |  100 +++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 109 insertions(+), 5 deletions(-)\n create mode 100755 t/t5900-repo-selection.sh\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex c62d4b5..637611a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -107,7 +107,7 @@ static const char *argv_submodule[] = {\n \n static char *get_repo_path(const char *repo, int *is_bundle)\n {\n-\tstatic char *suffix[] = { \"/.git\", \".git\", \"\" };\n+\tstatic char *suffix[] = { \"/.git\", \"\", \".git/.git\", \".git\" };\n \tstatic char *bundle_suffix[] = { \".bundle\", \"\" };\n \tstruct stat st;\n \tint i;\n@@ -117,7 +117,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)\n \t\tpath = mkpath(\"%s%s\", repo, suffix[i]);\n \t\tif (stat(path, &st))\n \t\t\tcontinue;\n-\t\tif (S_ISDIR(st.st_mode)) {\n+\t\tif (S_ISDIR(st.st_mode) && is_git_directory(path)) {\n \t\t\t*is_bundle = 0;\n \t\t\treturn xstrdup(absolute_path(path));\n \t\t} else if (S_ISREG(st.st_mode) && st.st_size > 8) {\ndiff --git a/cache.h b/cache.h\nindex 9bd8c2d..ae0396b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -432,6 +432,7 @@ extern char *git_work_tree_cfg;\n extern int is_inside_work_tree(void);\n extern int have_git_dir(void);\n extern const char *get_git_dir(void);\n+extern int is_git_directory(const char *path);\n extern char *get_object_directory(void);\n extern char *get_index_file(void);\n extern char *get_graft_file(void);\ndiff --git a/path.c b/path.c\nindex b6f71d1..6f2aa69 100644\n--- a/path.c\n+++ b/path.c\n@@ -293,7 +293,7 @@ const char *enter_repo(const char *path, int strict)\n \n \tif (!strict) {\n \t\tstatic const char *suffix[] = {\n-\t\t\t\".git/.git\", \"/.git\", \".git\", \"\", NULL,\n+\t\t\t\"/.git\", \"\", \".git/.git\", \".git\", NULL,\n \t\t};\n \t\tconst char *gitfile;\n \t\tint len = strlen(path);\n@@ -324,8 +324,11 @@ const char *enter_repo(const char *path, int strict)\n \t\t\treturn NULL;\n \t\tlen = strlen(used_path);\n \t\tfor (i = 0; suffix[i]; i++) {\n+\t\t\tstruct stat st;\n \t\t\tstrcpy(used_path + len, suffix[i]);\n-\t\t\tif (!access(used_path, F_OK)) {\n+\t\t\tif (!stat(used_path, &st) &&\n+\t\t\t    (S_ISREG(st.st_mode) ||\n+\t\t\t    (S_ISDIR(st.st_mode) && is_git_directory(used_path)))) {\n \t\t\t\tstrcat(validated_path, suffix[i]);\n \t\t\t\tbreak;\n \t\t\t}\ndiff --git a/setup.c b/setup.c\nindex 61c22e6..7a3618f 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -247,7 +247,7 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n  *    a proper \"ref:\", or a regular file HEAD that has a properly\n  *    formatted sha1 object name.\n  */\n-static int is_git_directory(const char *suspect)\n+int is_git_directory(const char *suspect)\n {\n \tchar path[PATH_MAX];\n \tsize_t len = strlen(suspect);\ndiff --git a/t/t5900-repo-selection.sh b/t/t5900-repo-selection.sh\nnew file mode 100755\nindex 0000000..3d5b418\n--- /dev/null\n+++ b/t/t5900-repo-selection.sh\n@@ -0,0 +1,100 @@\n+#!/bin/sh\n+\n+test_description='selecting remote repo in ambiguous cases'\n+. ./test-lib.sh\n+\n+reset() {\n+\trm -rf foo foo.git fetch clone\n+}\n+\n+make_tree() {\n+\tgit init \"$1\" &&\n+\t(cd \"$1\" && test_commit \"$1\")\n+}\n+\n+make_bare() {\n+\tgit init --bare \"$1\" &&\n+\t(cd \"$1\" &&\n+\t tree=`git hash-object -w -t tree /dev/null` &&\n+\t commit=$(echo \"$1\" | git commit-tree $tree) &&\n+\t git update-ref HEAD $commit\n+\t)\n+}\n+\n+get() {\n+\tgit init --bare fetch &&\n+\t(cd fetch && git fetch \"../$1\") &&\n+\tgit clone \"$1\" clone\n+}\n+\n+check() {\n+\techo \"$1\" >expect &&\n+\t(cd fetch && git log -1 --format=%s FETCH_HEAD) >actual.fetch &&\n+\t(cd clone && git log -1 --format=%s HEAD) >actual.clone &&\n+\ttest_cmp expect actual.fetch &&\n+\ttest_cmp expect actual.clone\n+}\n+\n+test_expect_success 'find .git dir in worktree' '\n+\treset &&\n+\tmake_tree foo &&\n+\tget foo &&\n+\tcheck foo\n+'\n+\n+test_expect_success 'automagically add .git suffix' '\n+\treset &&\n+\tmake_bare foo.git &&\n+\tget foo &&\n+\tcheck foo.git\n+'\n+\n+test_expect_success 'automagically add .git suffix to worktree' '\n+\treset &&\n+\tmake_tree foo.git &&\n+\tget foo &&\n+\tcheck foo.git\n+'\n+\n+test_expect_success 'prefer worktree foo over bare foo.git' '\n+\treset &&\n+\tmake_tree foo &&\n+\tmake_bare foo.git &&\n+\tget foo &&\n+\tcheck foo\n+'\n+\n+test_expect_success 'prefer bare foo over bare foo.git' '\n+\treset &&\n+\tmake_bare foo &&\n+\tmake_bare foo.git &&\n+\tget foo &&\n+\tcheck foo\n+'\n+\n+test_expect_success 'disambiguate with full foo.git' '\n+\treset &&\n+\tmake_bare foo &&\n+\tmake_bare foo.git &&\n+\tget foo.git &&\n+\tcheck foo.git\n+'\n+\n+test_expect_success 'we are not fooled by non-git foo directory' '\n+\treset &&\n+\tmake_bare foo.git &&\n+\tmkdir foo &&\n+\tget foo &&\n+\tcheck foo.git\n+'\n+\n+test_expect_success 'prefer inner .git over outer bare' '\n+\treset &&\n+\tmake_tree foo &&\n+\tmake_bare foo.git &&\n+\tmv foo/.git foo.git &&\n+\tget foo.git &&\n+\tcheck foo\n+'\n+\n+test_done\n-- \n1.7.9.rc1.28.gf4be5\n"},{"id":"183682","messageId":"7vzkd0u4ik.fsf@alter.siamese.dyndns.org","threadId":"29490","inReplyTo":"20120202215913.GA26727@sigill.intra.peff.net","subject":"Re: Alternates corruption issue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-03T00:47:31Z","receivedAt":"2012-02-03T00:47:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> @@ -324,8 +324,11 @@ const char *enter_repo(const char *path, int strict)\n>  \t\t\treturn NULL;\n>  \t\tlen = strlen(used_path);\n>  \t\tfor (i = 0; suffix[i]; i++) {\n> +\t\t\tstruct stat st;\n>  \t\t\tstrcpy(used_path + len, suffix[i]);\n> -\t\t\tif (!access(used_path, F_OK)) {\n> +\t\t\tif (!stat(used_path, &st) &&\n> +\t\t\t    (S_ISREG(st.st_mode) ||\n> +\t\t\t    (S_ISDIR(st.st_mode) && is_git_directory(used_path)))) {\n\nHmm, how would this change interact with\n\n>  \t\t\t\tstrcat(validated_path, suffix[i]);\n>  \t\t\t\tbreak;\n>  \t\t\t}\n\n\tgitfile = read_gitfile(used_path);\n\nthat appear after the context in the patch?\n"},{"id":"183707","messageId":"20120203120215.GA31441@sigill.intra.peff.net","threadId":"29490","inReplyTo":"7vzkd0u4ik.fsf@alter.siamese.dyndns.org","subject":"Re: Alternates corruption issue","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-03T12:02:15Z","receivedAt":"2012-02-03T12:02:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 02, 2012 at 04:47:31PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > @@ -324,8 +324,11 @@ const char *enter_repo(const char *path, int strict)\n> >  \t\t\treturn NULL;\n> >  \t\tlen = strlen(used_path);\n> >  \t\tfor (i = 0; suffix[i]; i++) {\n> > +\t\t\tstruct stat st;\n> >  \t\t\tstrcpy(used_path + len, suffix[i]);\n> > -\t\t\tif (!access(used_path, F_OK)) {\n> > +\t\t\tif (!stat(used_path, &st) &&\n> > +\t\t\t    (S_ISREG(st.st_mode) ||\n> > +\t\t\t    (S_ISDIR(st.st_mode) && is_git_directory(used_path)))) {\n> \n> Hmm, how would this change interact with\n> \n> >  \t\t\t\tstrcat(validated_path, suffix[i]);\n> >  \t\t\t\tbreak;\n> >  \t\t\t}\n> \n> \tgitfile = read_gitfile(used_path);\n> \n> that appear after the context in the patch?\n\nIt assumes that any file named \".git\" is worth reading and selecting.\nAnd then later we actually read_gitfile to find out if it's worth-while.\nThere is no change of behavior from before the patch, as we would\nsimilarly notice the file (without checking if it's a real gitfile) and\nthen later read it and possibly fail.\n\nHowever, with the ordering change, there is a technically a regression\nin one case: a random file \"foo\" next to a repo \"foo.git\". Saying \"git\nls-remote foo\" used to prefer \"foo.git\", and will now select the file\n\"foo\" only to fail.\n\nThe code-path in clone's get_repo_path handles this properly (it checks\nthat the path is really a valid gitfile before finishing the loop). The\ngitfile-reading from later in enter_repo could be hoisted into the loop.\nIf was trying to make a less-invasive change; if we're going to do that\nmuch rewriting, it probably makes sense to factor out the logic from\nget_repo_path and have them share the code.\n\nThanks for noticing. I saw this issue when I was writing the original\nversion of the patch, and meant to revisit it and at least document it\nin the commit message, but I ended up forgetting.\n\n-Peff\n"},{"id":"183736","messageId":"1328280019.3895.112.camel@ted","threadId":"29490","inReplyTo":"20120202215913.GA26727@sigill.intra.peff.net","subject":"Re: Alternates corruption issue","fromName":"Richard Purdie","fromEmail":"richard.purdie@linuxfoundation.org","sentAt":"2012-02-03T14:40:19Z","receivedAt":"2012-02-03T14:40:19Z","isPatch":false,"sender":{"key":"richard.purdie@linuxfoundation.org","avatar":null},"body":"On Thu, 2012-02-02 at 16:59 -0500, Jeff King wrote:\n> On Tue, Jan 31, 2012 at 04:22:58PM -0600, Jonathan Nieder wrote:\n> \n> > Anyway, thanks for explaining.  Hopefully I can get to this soon and\n> > factor out a common function for get_repo_path and enter_repo to call\n> > so playing with the ordering becomes a little less scary. ;-)\n> \n> So here's what I think we should apply to fix the particular issue that\n> Richard mentioned at the start of this thread.\n> \n> Besides tweaking the ordering, the main contribution is a set of tests\n> that actually check some of these ambiguous cases (especially checking\n> the fact that both code paths behave identically!). I didn't factor the\n> logic into a common function, but doing so should be a little safer on\n> top of these tests, if you're still interested.\n\nI didn't have much to add to the discussion yesterday but this solution\nlooks good to me and should resolve the problems I was seeing.\n\nThanks!\n\nRichard\n"},{"id":"183744","messageId":"7v8vkjstq2.fsf@alter.siamese.dyndns.org","threadId":"29490","inReplyTo":"20120203120215.GA31441@sigill.intra.peff.net","subject":"Re: Alternates corruption issue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-03T17:38:13Z","receivedAt":"2012-02-03T17:38:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> However, with the ordering change, there is a technically a regression\n> in one case: a random file \"foo\" next to a repo \"foo.git\". Saying \"git\n> ls-remote foo\" used to prefer \"foo.git\", and will now select the file\n> \"foo\" only to fail.\n\nYeah, very true X-<.\n\n> Thanks for noticing. I saw this issue when I was writing the original\n> version of the patch, and meant to revisit it and at least document it\n> in the commit message, but I ended up forgetting.\n\nNo, thanks for working on this.\n"},{"id":"183762","messageId":"20120203212906.GB1890@sigill.intra.peff.net","threadId":"29490","inReplyTo":"7v8vkjstq2.fsf@alter.siamese.dyndns.org","subject":"Re: Alternates corruption issue","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-03T21:29:06Z","receivedAt":"2012-02-03T21:29:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 03, 2012 at 09:38:13AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > However, with the ordering change, there is a technically a regression\n> > in one case: a random file \"foo\" next to a repo \"foo.git\". Saying \"git\n> > ls-remote foo\" used to prefer \"foo.git\", and will now select the file\n> > \"foo\" only to fail.\n> \n> Yeah, very true X-<.\n> \n> > Thanks for noticing. I saw this issue when I was writing the original\n> > version of the patch, and meant to revisit it and at least document it\n> > in the commit message, but I ended up forgetting.\n> \n> No, thanks for working on this.\n\nWhat do you want to do with it, then? Take my patch and ignore the\ngitfile issue, or have me refactor it more heavily? I could go either\nway.\n\n-Peff\n"},{"id":"183768","messageId":"7vmx8zmvq2.fsf@alter.siamese.dyndns.org","threadId":"29490","inReplyTo":"20120203212906.GB1890@sigill.intra.peff.net","subject":"Re: Alternates corruption issue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-03T21:51:33Z","receivedAt":"2012-02-03T21:51:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> No, thanks for working on this.\n>\n> What do you want to do with it, then? Take my patch and ignore the\n> gitfile issue, or have me refactor it more heavily? I could go either\n> way.\n\nHow about queuing it as is in next and see if anybody screams loudly\nenough so that we can fix-up if necessary in-tree?\n"},{"id":"183769","messageId":"20120203215307.GA3472@sigill.intra.peff.net","threadId":"29490","inReplyTo":"7vmx8zmvq2.fsf@alter.siamese.dyndns.org","subject":"Re: Alternates corruption issue","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-03T21:53:07Z","receivedAt":"2012-02-03T21:53:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 03, 2012 at 01:51:33PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> No, thanks for working on this.\n> >\n> > What do you want to do with it, then? Take my patch and ignore the\n> > gitfile issue, or have me refactor it more heavily? I could go either\n> > way.\n> \n> How about queuing it as is in next and see if anybody screams loudly\n> enough so that we can fix-up if necessary in-tree?\n\nSounds good to me. Jonathan might be interested in building on top of\nit, too.\n\n-Peff\n"}]}