{"thread":{"id":"25661","subject":"Minor bug with git sparse checkout code","startedAt":"2010-11-07T16:47:13Z","lastAt":"2010-11-09T02:47:01Z","messageCount":6,"participants":["Thomas Rinderknecht","Nguyễn Thái Ngọc Duy","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"155332","messageId":"loom.20101107T172926-284@post.gmane.org","threadId":"25661","inReplyTo":null,"subject":"Minor bug with git sparse checkout code","fromName":"Thomas Rinderknecht","fromEmail":"thomasr@sailguy.org","sentAt":"2010-11-07T16:47:13Z","receivedAt":"2010-11-07T16:47:13Z","isPatch":false,"sender":{"key":"thomasr@sailguy.org","avatar":null},"body":"Hello,\n\nFirst, let me begin by just saying thanks to all the git developers. We switched\nfrom cvs to git a few months ago, and it really is working well for us now that\nwe have overcome the initial learning curve.\n\nWe make extensive use of the sparse checkout functionality because our\nrepository is large and most developers don't need to see the entire source\ntree. Using sparse checkout helps reduce disk usage, and also helps maintain\nreasonable performance for commands such as \"git status\" that tend to get bogged\ndown when talking to slow network storage appliances.\n\nI found what appears to be a bug in the sparse-checkout code. I am more than\nhappy to file a bug report on a bug tracking website, but I couldn't find one\nwith a quick web search. I hope that it is acceptable to post the bug report\nhere. Please let me know if further information is required, or if there is a\nbetter way to file the bug report.\n\nShort description of the problem:\n---------------------------------\nWe have two subdirectories with similar names, for example \"libs/lib1\", and\n\"libs/lib1a\". We want to check out just \"libs/lib1\". The sparse-checkout file\ncontains only a single line \"libs/lib1/\", but git still checks out both\nsubdirectories (it appears to be doing a partial match)\n\nSequence for reproducing the bug:\n---------------------------------\n(1) create a repository with similar directory names\nmkdir /tmp/git_bug\ncd /tmp/git_bug\ngit init\nmkdir a1\ntouch a1/base\nmkdir a1-extra\ntouch a1-extra/more\ngit add .\ngit commit -m \"test\"\n\n(2) clone the repository, but don't check out the files\nmkdir /tmp/git_bug2\ncd /tmp/git_bug2\ngit clone -n /tmp/git_bug .\ngit config core.sparsecheckout true\necho \"a1/\" .git/info/sparse-checkout\ngit checkout\nls\n\n\nNote: We are currently using git version 1.7.2.3 ... I have not verified the bug\nusing the latest developmental code. Please accept my apologies if the issue has\nalready been addressed.\n\nThanks again,\n\nThomas R.\n"},{"id":"155334","messageId":"loom.20101107T181452-152@post.gmane.org","threadId":"25661","inReplyTo":"loom.20101107T172926-284@post.gmane.org","subject":"Re: Minor bug with git sparse checkout code","fromName":"Thomas Rinderknecht","fromEmail":"thomasr@sailguy.org","sentAt":"2010-11-07T17:19:27Z","receivedAt":"2010-11-07T17:19:27Z","isPatch":false,"sender":{"key":"thomasr@sailguy.org","avatar":null},"body":"Hello,\n\nThe description of the bug in my previous e-mail was correct, but the 2nd part\nof steps for replicating the bug were incorrect. Please use the following\nsequences to reproduce the bug (this time I actually verified it completely...)\n\n(1) create the reference repository (same as previous e-mail)\nmkdir /tmp/git_bug\ncd /tmp/git_bug\ngit init\nmkdir a1\ntouch a1/base\nmkdir a1-extra\ntouch a1-extra/more\ngit add .\ngit commit -m \"test\"\n\n(2) replicate the bug\ncd /tmp/git_bug2\ngit clone -n /tmp/git_bug .\ngit config core.sparsecheckout true\necho \"a1/\" > .git/info/sparse-checkout\ngit read-tree -u -m HEAD\nls\n\nThe previous example was using \"git checkout\" instead of \"git read-tree\", and\nthe \"echo\" command was missing an output redirect operator. It actually was\nchecking out both directories, but for the wrong reason. With this example, if\nthe two directories are named as shown, they both get checked out, but if you\nmodify step (1), and name the other repository as \"b1-extra\", then only the \"a1\"\ndirectory gets checked out in step (2).\n\nSorry about the confusion..\n"},{"id":"155337","messageId":"1289153098-15684-1-git-send-email-pclouds@gmail.com","threadId":"25661","inReplyTo":"loom.20101107T172926-284@post.gmane.org","subject":"[PATCH] dir.c: fix EXC_FLAG_MUSTBEDIR match in sparse checkout","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-11-07T18:04:58Z","receivedAt":"2010-11-07T18:04:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Commit c84de70 (excluded_1(): support exclude files in index -\n2009-08-20) tries to work around the fact that there is no\ndirectory/file information in index entries, therefore\nEXC_FLAG_MUSTBEDIR match would fail.\n\nUnfortunately the workaround is flawed. This fixes it.\n\nReported-by: Thomas Rinderknecht \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n dir.c | 3 ++-\n t/t1011-read-tree-sparse-checkout.sh | 10 +++++++---\n 2 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex d1e5e5e..b2dfb69 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -360,7 +360,8 @@ int excluded_from_list(const char *pathname,\n \n \t\t\tif (x->flags & EXC_FLAG_MUSTBEDIR) {\n \t\t\t\tif (!dtype) {\n-\t\t\t\t\tif (!prefixcmp(pathname, exclude))\n+\t\t\t\t\tif (!prefixcmp(pathname, exclude) &&\n+\t\t\t\t\t pathname[x->patternlen] == '/')\n \t\t\t\t\t\treturn to_exclude;\n \t\t\t\t\telse\n \t\t\t\t\t\tcontinue;\ndiff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh\nindex 9a07de1..8008fa2 100755\n--- a/t/t1011-read-tree-sparse-checkout.sh\n+++ b/t/t1011-read-tree-sparse-checkout.sh\n@@ -17,17 +17,19 @@ test_expect_success 'setup' '\n \tcat >expected <<-\\EOF &&\n \t100644 77f0ba1734ed79d12881f81b36ee134de6a3327b 0\tinit.t\n \t100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tsub/added\n+\t100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tsubsub/added\n \tEOF\n \tcat >expected.swt <<-\\EOF &&\n \tH init.t\n \tH sub/added\n+\tH subsub/added\n \tEOF\n \n \ttest_commit init &&\n \techo modified >>init.t &&\n-\tmkdir sub &&\n-\ttouch sub/added &&\n-\tgit add init.t sub/added &&\n+\tmkdir sub subsub &&\n+\ttouch sub/added subsub/added &&\n+\tgit add init.t sub/added subsub/added &&\n \tgit commit -m \"modified and added\" &&\n \tgit tag top &&\n \tgit rm sub/added &&\n@@ -81,6 +83,7 @@ test_expect_success 'match directories with trailing slash' '\n \tcat >expected.swt-noinit <<-\\EOF &&\n \tS init.t\n \tH sub/added\n+\tS subsub/added\n \tEOF\n \n \techo sub/ > .git/info/sparse-checkout &&\n@@ -105,6 +108,7 @@ test_expect_success 'checkout area changes' '\n \tcat >expected.swt-nosub <<-\\EOF &&\n \tH init.t\n \tS sub/added\n+\tS subsub/added\n \tEOF\n \n \techo init.t >.git/info/sparse-checkout &&\n-- \n1.7.3.2.210.g045198\n"},{"id":"155347","messageId":"loom.20101107T224233-408@post.gmane.org","threadId":"25661","inReplyTo":"1289153098-15684-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] dir.c: fix EXC_FLAG_MUSTBEDIR match in sparse checkout","fromName":"Thomas Rinderknecht","fromEmail":"thomasr@sailguy.org","sentAt":"2010-11-07T21:44:11Z","receivedAt":"2010-11-07T21:44:11Z","isPatch":true,"sender":{"key":"thomasr@sailguy.org","avatar":null},"body":"\nThanks for the quick fix! I was initially worried about using sparse-checkout\nsince it is a fairly new feature, but it has been working well for us. I am\ntruly impressed with the quality of the git code, and the dedication of the\ndevelopers who are enhancing and maintaining it.\n"},{"id":"155423","messageId":"7vfwvbpo9c.fsf@alter.siamese.dyndns.org","threadId":"25661","inReplyTo":"1289153098-15684-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] dir.c: fix EXC_FLAG_MUSTBEDIR match in sparse checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-11-08T20:13:35Z","receivedAt":"2010-11-08T20:13:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Commit c84de70 (excluded_1(): support exclude files in index -\n> 2009-08-20) tries to work around the fact that there is no\n> directory/file information in index entries, therefore\n> EXC_FLAG_MUSTBEDIR match would fail.\n>\n> Unfortunately the workaround is flawed. This fixes it.\n>\n> Reported-by: Thomas Rinderknecht \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n\nHmmm...\n\n> diff --git a/dir.c b/dir.c\n> index d1e5e5e..b2dfb69 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -360,7 +360,8 @@ int excluded_from_list(const char *pathname,\n> \n> \t\t\tif (x->flags & EXC_FLAG_MUSTBEDIR) {\n> \t\t\t\tif (!dtype) {\n> -\t\t\t\t\tif (!prefixcmp(pathname, exclude))\n> +\t\t\t\t\tif (!prefixcmp(pathname, exclude) &&\n> +\t\t\t\t\t pathname[x->patternlen] == '/')\n\n- Can pathname be much shorter than x->patternlen (it doesn't matter as\n prefixcmp will return false in that case)?\n\n- Can pathname be equal to exclude (yes it can but in that case pathname\n is not a directory but is a regular file, symlink or a submodule)?\n\nSo it may be a tricky code, but I do not think it is a \"workaround\" in any\nway. Isn't it just a \"correct solution\"?\n\nBy the way, builtin/add.c calls excluded() with DT_UNKNOWN and relies on\nthe fact that the macro is accidentally defined as 0. 108da0d (git add:\nAdd the \"--ignore-missing\" option for the dry run, 2010-07-10). If it\nmeans NULL, it should spell it out as such.\n\nThanks.\n"},{"id":"155455","messageId":"AANLkTin7Rg19=vxO3PtcxEEXH2XeB92_VHxXPKgMbEqK@mail.gmail.com","threadId":"25661","inReplyTo":"7vfwvbpo9c.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] dir.c: fix EXC_FLAG_MUSTBEDIR match in sparse checkout","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-11-09T02:47:01Z","receivedAt":"2010-11-09T02:47:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2010/11/9 Junio C Hamano :\n>> diff --git a/dir.c b/dir.c\n>> index d1e5e5e..b2dfb69 100644\n>> --- a/dir.c\n>> +++ b/dir.c\n>> @@ -360,7 +360,8 @@ int excluded_from_list(const char *pathname,\n>>\n>>                       if (x->flags & EXC_FLAG_MUSTBEDIR) {\n>>                               if (!dtype) {\n>> -                                     if (!prefixcmp(pathname, exclude))\n>> +                                     if (!prefixcmp(pathname, exclude) &&\n>> +                                         pathname[x->patternlen] == '/')\n>\n> - Can pathname be much shorter than x->patternlen (it doesn't matter as\n>  prefixcmp will return false in that case)?\n\nprefixcmp will return non-zero in that case.\n\n> - Can pathname be equal to exclude (yes it can but in that case pathname\n>  is not a directory but is a regular file, symlink or a submodule)?\n\nIn index context, regular files, symlinks and submodules are the same\nand should not match here even if they're equal to exclude (the\noriginal exclude is \"foo/\". The trailing slash was converted to\nEXC_FLAG_MUSTBEDIR).\n\n> So it may be a tricky code, but I do not think it is a \"workaround\" in any\n> way.  Isn't it just a \"correct solution\"?\n\nHmm.. when I added it, unpack-trees.c was the only callsite and it was\na workaround (i.e. foo/ match foo directory, but foo alone does not).\n\n> By the way, builtin/add.c calls excluded() with DT_UNKNOWN and relies on\n> the fact that the macro is accidentally defined as 0.  108da0d (git add:\n> Add the \"--ignore-missing\" option for the dry run, 2010-07-10).  If it\n> means NULL, it should spell it out as such.\n\nIt does mean NULL. Should builtin/add.c pass a proper pointer that\ncontains DT_UNKNOWN instead? DT_UNKNOWN has special treatment (right\nafter that \"if\").\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex eed37bf..b104851 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -446,7 +446,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\tif (!seen[i] && pathspec[i][0]\n \t\t\t && !file_exists(pathspec[i])) {\n \t\t\t\tif (ignore_missing) {\n-\t\t\t\t\tif (excluded(&dir, pathspec[i], DT_UNKNOWN))\n+\t\t\t\t\tint dtype = DT_UNKNOWN;\n+\t\t\t\t\tif (excluded(&dir, pathspec[i], &dtype))\n \t\t\t\t\t\tdir_add_ignored(&dir, pathspec[i], strlen(pathspec[i]));\n \t\t\t\t} else\n \t\t\t\t\tdie(_(\"pathspec '%s' did not match any files\"),\n-- \nDuy\n"}]}