{"thread":{"id":"16953","subject":"unpack-trees: fix D/F conflict bugs in verify_absent","startedAt":"2009-01-01T20:54:30Z","lastAt":"2011-01-13T20:20:23Z","messageCount":20,"participants":["Clemens Buchacher","Junio C Hamano","Johannes Schindelin","Miklos Vajna","Jonathan Nieder","Miles Bader"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"99103","messageId":"1230843273-11056-1-git-send-email-drizzd@aon.at","threadId":"16953","inReplyTo":null,"subject":"unpack-trees: fix D/F conflict bugs in verify_absent","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-01-01T20:54:30Z","receivedAt":"2009-01-01T20:54:30Z","isPatch":false,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"I came across a few bugs while investigating the changes I proposed in the\nmodify/delete conflict thread. The first two are quite obvious. The third I'm\nnot so sure about. I could not find a testcase where it matters. Junio, do you\nrecall the original intention of that code?\n\n[PATCH 1/3] unpack-trees: handle failure in verify_absent\n[PATCH 2/3] unpack-trees: fix path search bug in verify_absent\n[PATCH 3/3] unpack-trees: remove redundant path search in verify_absent\n\n t/t1001-read-tree-m-2way.sh |   51 +++++++++++++++++++++++++++++++++++++++++++\n unpack-trees.c              |   37 +++++++++++++++----------------\n 2 files changed, 69 insertions(+), 19 deletions(-)\n"},{"id":"99105","messageId":"1230843273-11056-2-git-send-email-drizzd@aon.at","threadId":"16953","inReplyTo":"1230843273-11056-1-git-send-email-drizzd@aon.at","subject":"[PATCH 1/3] unpack-trees: handle failure in verify_absent","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-01-01T20:54:31Z","receivedAt":"2009-01-01T20:54:31Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Commit 203a2fe1 (Allow callers of unpack_trees() to handle failure)\nchanged the \"die on error\" behavior to \"return failure code\".\nverify_absent did not handle errors returned by\nverify_clean_subdirectory, however.\n---\n t/t1001-read-tree-m-2way.sh |   24 ++++++++++++++++++++++++\n unpack-trees.c              |    8 +++++---\n 2 files changed, 29 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh\nindex 4b44e13..7f6ab31 100755\n--- a/t/t1001-read-tree-m-2way.sh\n+++ b/t/t1001-read-tree-m-2way.sh\n@@ -341,4 +341,28 @@ test_expect_success \\\n      check_cache_at DF/DF dirty &&\n      :'\n \n+test_expect_success \\\n+    'a/b (untracked) vs a case setup.' \\\n+    'rm -f .git/index &&\n+     : >a &&\n+     git update-index --add a &&\n+     treeM=`git write-tree` &&\n+     echo treeM $treeM &&\n+     git ls-tree $treeM &&\n+     git ls-files --stage >treeM.out &&\n+\n+     rm -f a &&\n+     git update-index --remove a &&\n+     mkdir a &&\n+     : >a/b &&\n+     treeH=`git write-tree` &&\n+     echo treeH $treeH &&\n+     git ls-tree $treeH'\n+\n+test_expect_success \\\n+    'a/b (untracked) vs a, plus c/d case test.' \\\n+    '! git read-tree -u -m \"$treeH\" \"$treeM\" &&\n+     git ls-files --stage &&\n+     test -f a/b'\n+\n test_done\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 54f301d..a736947 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -588,7 +588,7 @@ static int verify_absent(struct cache_entry *ce, const char *action,\n \t\treturn 0;\n \n \tif (!lstat(ce->name, &st)) {\n-\t\tint cnt;\n+\t\tint ret;\n \t\tint dtype = ce_to_dtype(ce);\n \t\tstruct cache_entry *result;\n \n@@ -616,7 +616,9 @@ static int verify_absent(struct cache_entry *ce, const char *action,\n \t\t\t * files that are in \"foo/\" we would lose\n \t\t\t * it.\n \t\t\t */\n-\t\t\tcnt = verify_clean_subdirectory(ce, action, o);\n+\t\t\tret = verify_clean_subdirectory(ce, action, o);\n+\t\t\tif (ret < 0)\n+\t\t\t\treturn ret;\n \n \t\t\t/*\n \t\t\t * If this removed entries from the index,\n@@ -635,7 +637,7 @@ static int verify_absent(struct cache_entry *ce, const char *action,\n \t\t\t * We need to increment it by the number of\n \t\t\t * deleted entries here.\n \t\t\t */\n-\t\t\to->pos += cnt;\n+\t\t\to->pos += ret;\n \t\t\treturn 0;\n \t\t}\n \n-- \n1.6.1\n"},{"id":"99104","messageId":"1230843273-11056-3-git-send-email-drizzd@aon.at","threadId":"16953","inReplyTo":"1230843273-11056-2-git-send-email-drizzd@aon.at","subject":"[PATCH 2/3] unpack-trees: fix path search bug in verify_absent","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-01-01T20:54:32Z","receivedAt":"2009-01-01T20:54:32Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Commit 0cf73755 (unpack-trees.c: assume submodules are clean during\ncheck-out) changed an argument to verify_absent from 'path' to 'ce',\nwhich is however shadowed by a local variable of the same name.\n\nThe bug triggers if verify_absent is used on a tree entry, for which\nthe index contains one or more subsequent directories of the same\nlength. The affected subdirectories are removed from the index. The\ntestcase included in this commit bisects to 55218834 (checkout: do not\nlose staged removal), which reveals the bug in this case, but is\notherwise unrelated.\n---\n t/t1001-read-tree-m-2way.sh |   27 +++++++++++++++++++++++++++\n unpack-trees.c              |   23 ++++++++++++-----------\n 2 files changed, 39 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh\nindex 7f6ab31..271bc4e 100755\n--- a/t/t1001-read-tree-m-2way.sh\n+++ b/t/t1001-read-tree-m-2way.sh\n@@ -365,4 +365,31 @@ test_expect_success \\\n      git ls-files --stage &&\n      test -f a/b'\n \n+test_expect_success \\\n+    'a/b vs a, plus c/d case setup.' \\\n+    'rm -f .git/index &&\n+     rm -fr a &&\n+     : >a &&\n+     mkdir c &&\n+     : >c/d &&\n+     git update-index --add a c/d &&\n+     treeM=`git write-tree` &&\n+     echo treeM $treeM &&\n+     git ls-tree $treeM &&\n+     git ls-files --stage >treeM.out &&\n+\n+     rm -f a &&\n+     mkdir a\n+     : >a/b &&\n+     git update-index --add --remove a a/b &&\n+     treeH=`git write-tree` &&\n+     echo treeH $treeH &&\n+     git ls-tree $treeH'\n+\n+test_expect_success \\\n+    'a/b vs a, plus c/d case test.' \\\n+    'git read-tree -u -m \"$treeH\" \"$treeM\" &&\n+     git ls-files --stage | tee >treeMcheck.out &&\n+     test_cmp treeM.out treeMcheck.out'\n+\n test_done\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex a736947..f8e2484 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -289,7 +289,8 @@ static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmas\n \treturn 0;\n }\n \n-static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)\n+static int unpack_callback(int n, unsigned long mask, unsigned long dirmask,\n+\t\tstruct name_entry *names, struct traverse_info *info)\n {\n \tstruct cache_entry *src[5] = { NULL, };\n \tstruct unpack_trees_options *o = info->data;\n@@ -517,22 +518,22 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,\n \tnamelen = strlen(ce->name);\n \tpos = index_name_pos(o->src_index, ce->name, namelen);\n \tif (0 <= pos)\n-\t\treturn cnt; /* we have it as nondirectory */\n+\t\treturn 0; /* we have it as nondirectory */\n \tpos = -pos - 1;\n \tfor (i = pos; i < o->src_index->cache_nr; i++) {\n-\t\tstruct cache_entry *ce = o->src_index->cache[i];\n-\t\tint len = ce_namelen(ce);\n+\t\tstruct cache_entry *ce2 = o->src_index->cache[i];\n+\t\tint len = ce_namelen(ce2);\n \t\tif (len < namelen ||\n-\t\t    strncmp(ce->name, ce->name, namelen) ||\n-\t\t    ce->name[namelen] != '/')\n+\t\t    strncmp(ce->name, ce2->name, namelen) ||\n+\t\t    ce2->name[namelen] != '/')\n \t\t\tbreak;\n \t\t/*\n-\t\t * ce->name is an entry in the subdirectory.\n+\t\t * ce2->name is an entry in the subdirectory.\n \t\t */\n-\t\tif (!ce_stage(ce)) {\n-\t\t\tif (verify_uptodate(ce, o))\n+\t\tif (!ce_stage(ce2)) {\n+\t\t\tif (verify_uptodate(ce2, o))\n \t\t\t\treturn -1;\n-\t\t\tadd_entry(o, ce, CE_REMOVE, 0);\n+\t\t\tadd_entry(o, ce2, CE_REMOVE, 0);\n \t\t}\n \t\tcnt++;\n \t}\n@@ -624,7 +625,7 @@ static int verify_absent(struct cache_entry *ce, const char *action,\n \t\t\t * If this removed entries from the index,\n \t\t\t * what that means is:\n \t\t\t *\n-\t\t\t * (1) the caller unpack_trees_rec() saw path/foo\n+\t\t\t * (1) the caller unpack_callback() saw path/foo\n \t\t\t * in the index, and it has not removed it because\n \t\t\t * it thinks it is handling 'path' as blob with\n \t\t\t * D/F conflict;\n-- \n1.6.1\n"},{"id":"99115","messageId":"1230843273-11056-4-git-send-email-drizzd@aon.at","threadId":"16953","inReplyTo":"1230843273-11056-3-git-send-email-drizzd@aon.at","subject":"[PATCH 3/3] unpack-trees: remove redundant path search in verify_absent","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-01-01T20:54:33Z","receivedAt":"2009-01-01T20:54:33Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Since the only caller, verify_absent, relies on the fact that o->pos\npoints to the next index entry anyways, there is no need to recompute\nits position.\n\nFurthermore, if a nondirectory entry were found, this would return too\nearly, because there could still be an untracked directory in the way.\nThis is currently not a problem, because verify_absent is only called\nif the index does not have this entry.\n---\n unpack-trees.c |    8 ++------\n 1 files changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f8e2484..c4dc6dc 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -495,7 +495,7 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,\n \t * anything in the existing directory there.\n \t */\n \tint namelen;\n-\tint pos, i;\n+\tint i;\n \tstruct dir_struct d;\n \tchar *pathbuf;\n \tint cnt = 0;\n@@ -516,11 +516,7 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,\n \t * in that directory.\n \t */\n \tnamelen = strlen(ce->name);\n-\tpos = index_name_pos(o->src_index, ce->name, namelen);\n-\tif (0 <= pos)\n-\t\treturn 0; /* we have it as nondirectory */\n-\tpos = -pos - 1;\n-\tfor (i = pos; i < o->src_index->cache_nr; i++) {\n+\tfor (i = o->pos; i < o->src_index->cache_nr; i++) {\n \t\tstruct cache_entry *ce2 = o->src_index->cache[i];\n \t\tint len = ce_namelen(ce2);\n \t\tif (len < namelen ||\n-- \n1.6.1\n"},{"id":"99116","messageId":"7vfxk2hg21.fsf@gitster.siamese.dyndns.org","threadId":"16953","inReplyTo":"1230843273-11056-1-git-send-email-drizzd@aon.at","subject":"Re: unpack-trees: fix D/F conflict bugs in verify_absent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-01T21:28:22Z","receivedAt":"2009-01-01T21:28:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> I came across a few bugs while investigating the changes I proposed in the\n> modify/delete conflict thread. The first two are quite obvious. The third I'm\n> not so sure about. I could not find a testcase where it matters. Junio, do you\n> recall the original intention of that code?\n\nThanks, but I see only [PATCH 1/3] and [PATCH 2/3] (both of which look\nsane, by the way).\n"},{"id":"99211","messageId":"alpine.DEB.1.00.0901022244240.27818@racer","threadId":"16953","inReplyTo":"1230843273-11056-3-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 2/3] unpack-trees: fix path search bug in verify_absent","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-02T21:59:43Z","receivedAt":"2009-01-02T21:59:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\n\nOn Thu, 1 Jan 2009, Clemens Buchacher wrote:\n\n> Commit 0cf73755 (unpack-trees.c: assume submodules are clean during\n> check-out) changed an argument to verify_absent from 'path' to 'ce',\n> which is however shadowed by a local variable of the same name.\n> \n> The bug triggers if verify_absent is used on a tree entry, for which\n> the index contains one or more subsequent directories of the same\n> length. The affected subdirectories are removed from the index. The\n> testcase included in this commit bisects to 55218834 (checkout: do not\n> lose staged removal), which reveals the bug in this case, but is\n> otherwise unrelated.\n> ---\n\nSign-off?\n\nJust for the record, this patch fixes the testcase Miklos reported \nearlier.\n\nCiao,\nDscho\n"},{"id":"99213","messageId":"alpine.DEB.1.00.0901022248070.27818@racer","threadId":"16953","inReplyTo":"1230843273-11056-3-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 2/3] unpack-trees: fix path search bug in verify_absent","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-02T21:59:47Z","receivedAt":"2009-01-02T21:59:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 1 Jan 2009, Clemens Buchacher wrote:\n\n> Commit 0cf73755 (unpack-trees.c: assume submodules are clean during\n> check-out) changed an argument to verify_absent from 'path' to 'ce',\n> which is however shadowed by a local variable of the same name.\n\nThis explanation makes sense.  However, this:\n\n> @@ -289,7 +289,8 @@ static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmas\n>  \treturn 0;\n>  }\n>  \n> -static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)\n> +static int unpack_callback(int n, unsigned long mask, unsigned long dirmask,\n> +\t\tstruct name_entry *names, struct traverse_info *info)\n>  {\n>  \tstruct cache_entry *src[5] = { NULL, };\n>  \tstruct unpack_trees_options *o = info->data;\n\n... is distracting during review, and this:\n\n> @@ -517,22 +518,22 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,\n>  \tnamelen = strlen(ce->name);\n>  \tpos = index_name_pos(o->src_index, ce->name, namelen);\n>  \tif (0 <= pos)\n> -\t\treturn cnt; /* we have it as nondirectory */\n> +\t\treturn 0; /* we have it as nondirectory */\n>  \tpos = -pos - 1;\n>  \tfor (i = pos; i < o->src_index->cache_nr; i++) {\n\n... is not accounted for in the commit message.  Intended or not, that is \nthe question.\n\nCiao,\nDscho \"whether 'tis noble\"\n"},{"id":"99245","messageId":"20090103103904.GA4479@localhost","threadId":"16953","inReplyTo":"alpine.DEB.1.00.0901022248070.27818@racer","subject":"Re: [PATCH 2/3] unpack-trees: fix path search bug in verify_absent","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-01-03T10:39:04Z","receivedAt":"2009-01-03T10:39:04Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Fri, Jan 02, 2009 at 10:59:47PM +0100, Johannes Schindelin wrote:\n> This explanation makes sense.  However, this:\n> \n> > @@ -289,7 +289,8 @@ static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmas\n> >  \treturn 0;\n> >  }\n> >  \n> > -static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)\n> > +static int unpack_callback(int n, unsigned long mask, unsigned long dirmask,\n> > +\t\tstruct name_entry *names, struct traverse_info *info)\n> >  {\n> >  \tstruct cache_entry *src[5] = { NULL, };\n> >  \tstruct unpack_trees_options *o = info->data;\n> \n> ... is distracting during review, and this:\n> \n> > @@ -517,22 +518,22 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,\n> >  \tnamelen = strlen(ce->name);\n> >  \tpos = index_name_pos(o->src_index, ce->name, namelen);\n> >  \tif (0 <= pos)\n> > -\t\treturn cnt; /* we have it as nondirectory */\n> > +\t\treturn 0; /* we have it as nondirectory */\n> >  \tpos = -pos - 1;\n> >  \tfor (i = pos; i < o->src_index->cache_nr; i++) {\n> \n> ... is not accounted for in the commit message.  Intended or not, that is \n> the question.\n\nThose are trivial readability improvements in the context of the patch.\n\nOn Fri, Jan 02, 2009 at 10:59:43PM +0100, Johannes Schindelin wrote:\n> Sign-off?\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n\non all three patches.\n"},{"id":"99252","messageId":"alpine.DEB.1.00.0901031353090.30769@pacific.mpi-cbg.de","threadId":"16953","inReplyTo":"20090103103904.GA4479@localhost","subject":"Re: [PATCH 2/3] unpack-trees: fix path search bug in verify_absent","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-03T12:53:50Z","receivedAt":"2009-01-03T12:53:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 3 Jan 2009, Clemens Buchacher wrote:\n\n> On Fri, Jan 02, 2009 at 10:59:47PM +0100, Johannes Schindelin wrote:\n> > This explanation makes sense.  However, this:\n> > \n> > > @@ -289,7 +289,8 @@ static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmas\n> > >  \treturn 0;\n> > >  }\n> > >  \n> > > -static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)\n> > > +static int unpack_callback(int n, unsigned long mask, unsigned long dirmask,\n> > > +\t\tstruct name_entry *names, struct traverse_info *info)\n> > >  {\n> > >  \tstruct cache_entry *src[5] = { NULL, };\n> > >  \tstruct unpack_trees_options *o = info->data;\n> > \n> > ... is distracting during review, and this:\n> > \n> > > @@ -517,22 +518,22 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,\n> > >  \tnamelen = strlen(ce->name);\n> > >  \tpos = index_name_pos(o->src_index, ce->name, namelen);\n> > >  \tif (0 <= pos)\n> > > -\t\treturn cnt; /* we have it as nondirectory */\n> > > +\t\treturn 0; /* we have it as nondirectory */\n> > >  \tpos = -pos - 1;\n> > >  \tfor (i = pos; i < o->src_index->cache_nr; i++) {\n> > \n> > ... is not accounted for in the commit message.  Intended or not, that is \n> > the question.\n> \n> Those are trivial readability improvements in the context of the patch.\n\nThey are not trivial enough for me not to be puzzled.  Reason enough to \nexplain in the commit message?\n\nCiao,\nDscho\n"},{"id":"99254","messageId":"20090103140138.GX21154@genesis.frugalware.org","threadId":"16953","inReplyTo":"alpine.DEB.1.00.0901022244240.27818@racer","subject":"Re: [PATCH 2/3] unpack-trees: fix path search bug in verify_absent","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2009-01-03T14:01:38Z","receivedAt":"2009-01-03T14:01:38Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Fri, Jan 02, 2009 at 10:59:43PM +0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> Just for the record, this patch fixes the testcase Miklos reported \n> earlier.\n\nIndeed, thanks for the notice.\n"},{"id":"99286","messageId":"7vtz8fz8yd.fsf@gitster.siamese.dyndns.org","threadId":"16953","inReplyTo":"alpine.DEB.1.00.0901031353090.30769@pacific.mpi-cbg.de","subject":"Re: [PATCH 2/3] unpack-trees: fix path search bug in verify_absent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-04T10:01:14Z","receivedAt":"2009-01-04T10:01:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Sat, 3 Jan 2009, Clemens Buchacher wrote:\n>\n>> On Fri, Jan 02, 2009 at 10:59:47PM +0100, Johannes Schindelin wrote:\n>> > This explanation makes sense.  However, this:\n>> > \n>> > > @@ -289,7 +289,8 @@ static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmas\n>> > >  \treturn 0;\n>> > >  }\n>> > >  \n>> > > -static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)\n>> > > +static int unpack_callback(int n, unsigned long mask, unsigned long dirmask,\n>> > > +\t\tstruct name_entry *names, struct traverse_info *info)\n>> > >  {\n>> > >  \tstruct cache_entry *src[5] = { NULL, };\n>> > >  \tstruct unpack_trees_options *o = info->data;\n>> > \n>> > ... is distracting during review, and this:\n>> > \n>> > > @@ -517,22 +518,22 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,\n>> > >  \tnamelen = strlen(ce->name);\n>> > >  \tpos = index_name_pos(o->src_index, ce->name, namelen);\n>> > >  \tif (0 <= pos)\n>> > > -\t\treturn cnt; /* we have it as nondirectory */\n>> > > +\t\treturn 0; /* we have it as nondirectory */\n>> > >  \tpos = -pos - 1;\n>> > >  \tfor (i = pos; i < o->src_index->cache_nr; i++) {\n>> > \n>> > ... is not accounted for in the commit message.  Intended or not, that is \n>> > the question.\n>> \n>> Those are trivial readability improvements in the context of the patch.\n>\n> They are not trivial enough for me not to be puzzled.  Reason enough to \n> explain in the commit message?\n\nI'd say the first hunk quoted is probably on the borderline.  It is an\nunnecessary churn that won't even be commented on if it were sent alone,\nbut as a \"while we are at it\" hunk in a patch that is not too big, this is\na kind of thing that often is tolerated, because it is obvious enough not\nto hurt anything from the correctness standpoint [*1*].\n\nThe second one is moderately worse for two reasons.\n\n * I actually had to scratch my head because you need to view the change\n   in a lot wider context that covers the initializing definition of \"int\n   cnt\" near the beginning of the function down to the area affected by\n   the hunk, in order to see that the new \"return 0\" is the same as the\n   old \"return cnt\" and does not break things.  A comment to say that \"at\n   this point in the codeflow, cnt which is returned by the old code is\n   always zero\", perhaps below the three-dash marker, would have saved me\n   a minute.\n\n * The function's purpose and logic is to see if the subdirectory is\n   clean, and return how many cache entries need to be skipped if it is\n   (otherwise a negative number as an error indicator).  For that purpose,\n   the return value cnt is initialized to 0 (i.e. \"we haven't counted any\n   entry that needs to be skipped yet\"), the loop below the patched part\n   counts it up while performing the verification, and then the resulting\n   count is returned from the function.  The logic flow, at least to me,\n   is easier to follow if it returned the value in cnt, not a hardcoded 0,\n   from the place the patch tries to touch.\n\nThe latter point is with \"at least to me\", because I think an alternate\nposition is entirely valid if the author wants to justify the change by\nsaying something like:\n\n    The function's purpose is ....  Before entering the loop to count the\n    number of entries to skip, this check to detect if we do not even have\n    to count appears.  When this check triggers, we know we do not want to\n    skip anything, and returning constant 0 is much clearer than returning\n    a variable cnt that was initialized to 0 near the beginning of the\n    function; we haven't even started using it to count yet.\n\nBut the point is, if that is the reason the author thinks it is an\nimprovement, that probably needs to be stated.\n\n[Footnote]\n\n*1* I am not sure if it is obviously clear that the change improves any\nreadability.  Some people argue that splitting the function definition\nheader hurts greppability for one thing.  I personally do not find it easy\nto read when the subsequent header lines are indented without aligning\n(compare the way it is indented in the postimage of the patch with the way\nthe headers verify_absent() and show_stage_entry() are indented), either.\n"},{"id":"99324","messageId":"20090104200133.GA2202@localhost","threadId":"16953","inReplyTo":"7vtz8fz8yd.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] unpack-trees: fix path search bug in verify_absent","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-01-04T20:01:35Z","receivedAt":"2009-01-04T20:01:35Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Hi,\n\nOn Sun, Jan 04, 2009 at 02:01:14AM -0800, Junio C Hamano wrote:\n>     The function's purpose is ....  Before entering the loop to count the\n>     number of entries to skip, this check to detect if we do not even have\n>     to count appears.  When this check triggers, we know we do not want to\n>     skip anything, and returning constant 0 is much clearer than returning\n>     a variable cnt that was initialized to 0 near the beginning of the\n>     function; we haven't even started using it to count yet.\n> \n> But the point is, if that is the reason the author thinks it is an\n> improvement, that probably needs to be stated.\n\nIf you want to check the validity of the patch you have to view it in\ncontext anyways. Compared to understanding the change to the code, it takes\nmuch longer to parse and understand the above paragraph _plus_ verify its\nagreement with the code. I think you will agree that there is a limit to the\namount of documentation that's still useful.\n\nMy estimate of this limit is apparently much lower than what is expected by\nthe main contributors to this project. I respect that and I will try not to\nwaste your time any further.\n\nWhat's sad, however, is that we are now discussing style and commenting\nissues of a line of code, which, as by my analysis of [PATCH 3/3] never\nactually gets executed in the first place. I would have been much more\ncurious about your comments on that.\n\nBest regards,\nClemens\n"},{"id":"99442","messageId":"7viqoskft7.fsf@gitster.siamese.dyndns.org","threadId":"16953","inReplyTo":"1230843273-11056-4-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 3/3] unpack-trees: remove redundant path search in verify_absent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-06T08:19:00Z","receivedAt":"2009-01-06T08:19:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> Since the only caller, verify_absent, relies on the fact that o->pos\n> points to the next index entry anyways, there is no need to recompute\n> its position.\n\nI suspect that the original reasoning of this behaviour might have been in\nanticipation of other callers, but I agree with your reasoning especially\nbecause I do not think of a good reason to want to receive the number of\nentries to skip as the return value and not have o->pos pointing at the\nright place.\n\nThanks, queued.\n"},{"id":"99496","messageId":"alpine.DEB.1.00.0901062031101.30769@pacific.mpi-cbg.de","threadId":"16953","inReplyTo":"20090104200133.GA2202@localhost","subject":"Re: [PATCH 2/3] unpack-trees: fix path search bug in verify_absent","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-06T19:35:19Z","receivedAt":"2009-01-06T19:35:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 4 Jan 2009, Clemens Buchacher wrote:\n\n> On Sun, Jan 04, 2009 at 02:01:14AM -0800, Junio C Hamano wrote:\n> >     The function's purpose is ....  Before entering the loop to count \n> >     the number of entries to skip, this check to detect if we do not \n> >     even have to count appears.  When this check triggers, we know we \n> >     do not want to skip anything, and returning constant 0 is much \n> >     clearer than returning a variable cnt that was initialized to 0 \n> >     near the beginning of the function; we haven't even started using \n> >     it to count yet.\n> > \n> > But the point is, if that is the reason the author thinks it is an \n> > improvement, that probably needs to be stated.\n> \n> If you want to check the validity of the patch you have to view it in\n> context anyways.\n\nUmm.\n\nYou can make reviewing your patch attractive and easy, and you can make it \nunattractive and difficult.\n\nIf you explain in the commit message that you replaced \"cnt\" by \"0\" \nbecause it is initialized to 0 at that point anyway, it is a _much bigger_ \npleasure to review your patch.\n\nLet alone a much bigger pleasure for you, 6 months from now, when somebody \nsays \"why does this silly function return 0, when it should return cnt?\"\n\nBTW exactly for that reason, I'd like to leave it as \"cnt\".  Because code \n_will_ change, and it's quite possible that cnt will not be 0 at that \npoint in the future.\n\n> Compared to understanding the change to the code, it takes much longer \n> to parse and understand the above paragraph _plus_ verify its agreement \n> with the code. I think you will agree that there is a limit to the \n> amount of documentation that's still useful.\n\nJust look at a concrete case: me.  I saw that part of the patch, even \nbefore coming to the real meat of it.  And that head-scratching already \nremoved all the enthusiasm I had to look at unpack-trees.c again, so you \nlost a reviewer.\n\n> What's sad, however, is that we are now discussing style and commenting \n> issues of a line of code, which, as by my analysis of [PATCH 3/3] never \n> actually gets executed in the first place. I would have been much more \n> curious about your comments on that.\n\nExactly,\nDscho\n"},{"id":"159419","messageId":"20110113022415.GA8635@burratino","threadId":"16953","inReplyTo":"1230843273-11056-2-git-send-email-drizzd@aon.at","subject":"[RFC/PATCH 0/2] unpack-trees: handle lstat failures in verify_absent","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-01-13T02:24:15Z","receivedAt":"2011-01-13T02:24:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Here are two cases where we ignore the result from lstat in\nunpack_trees.  I think we rather shouldn't ignore it.  Sane?\n\nJonathan Nieder (2):\n  unpack-trees: handle lstat failure for existing directory\n  unpack-trees: handle lstat failure for existing file\n\n unpack-trees.c |    8 ++++++--\n 1 files changed, 6 insertions(+), 2 deletions(-)\n"},{"id":"159420","messageId":"20110113022636.GB8635@burratino","threadId":"16953","inReplyTo":"20110113022415.GA8635@burratino","subject":"[PATCH 1/2] unpack-trees: handle lstat failure for existing directory","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-01-13T02:26:36Z","receivedAt":"2011-01-13T02:26:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"When check_leading_path notices no file in the way of the new entry to\nbe checked out, verify_absent checks whether there is a directory\nthere or nothing at all.  If that lstat call fails (for example due to\nENOMEM), it assumes ENOENT, meaning a directory with untracked files\nwould be clobbered in that case.\n\nCheck errno after calling lstat, and for conditions other than ENOENT,\njust error out.\n\nThis is a theoretical race condition.  lstat has to succeed moments\nbefore it fails for there to be trouble.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nDoes this need an o->gently check?\n\n unpack-trees.c |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 1ca41b1..a795db5 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1382,7 +1382,9 @@ static int verify_absent_1(struct cache_entry *ce,\n \t\treturn check_ok_to_remove(ce->name, ce_namelen(ce),\n \t\t\t\tce_to_dtype(ce), ce, &st,\n \t\t\t\terror_type, o);\n-\n+\tif (errno != ENOENT)\n+\t\treturn error(\"cannot stat '%s': %s\", ce->name,\n+\t\t\t\tstrerror(errno));\n \treturn 0;\n }\n \n-- \n1.7.4.rc1\n"},{"id":"159421","messageId":"20110113022809.GC8635@burratino","threadId":"16953","inReplyTo":"20110113022415.GA8635@burratino","subject":"[PATCH 2/2] unpack-trees: handle lstat failure for existing file","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-01-13T02:28:09Z","receivedAt":"2011-01-13T02:28:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"When check_leading_path notices a file in the way of a new entry to be\nchecked out, verify_absent uses (1) the mode to determine whether it\nis a directory (2) the rest of the stat information to check if this\nis actually an old entry, disguised by a change in filename (e.g.,\nREADME -> Readme) that is significant to git but insignificant to the\nunderlying filesystem.  If lstat fails, these checks are performed\nwith an uninitialied stat structure, producing essentially random\nresults.\n\nBetter to just error out when lstat fails.\n\nThe easiest way to reproduce this is to remove a file after the\ncheck_leading_path call and before the lstat in verify_absent.  An\nlstat failure other than ENOENT in check_leading_path would also\ntrigger the same code path.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n unpack-trees.c |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex a795db5..9c3fe64 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1374,7 +1374,9 @@ static int verify_absent_1(struct cache_entry *ce,\n \t\tchar path[PATH_MAX + 1];\n \t\tmemcpy(path, ce->name, len);\n \t\tpath[len] = 0;\n-\t\tlstat(path, &st);\n+\t\tif (lstat(path, &st))\n+\t\t\treturn error(\"cannot stat '%s': %s\", path,\n+\t\t\t\t\tstrerror(errno));\n \n \t\treturn check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st,\n \t\t\t\terror_type, o);\n-- \n1.7.4.rc1\n"},{"id":"159425","messageId":"buobp3ls9l1.fsf@dhlpc061.dev.necel.com","threadId":"16953","inReplyTo":"20110113022636.GB8635@burratino","subject":"Re: [PATCH 1/2] unpack-trees: handle lstat failure for existing directory","fromName":"Miles Bader","fromEmail":"miles@gnu.org","sentAt":"2011-01-13T04:37:14Z","receivedAt":"2011-01-13T04:37:14Z","isPatch":true,"sender":{"key":"miles@gnu.org","avatar":"https://gravatar.com/avatar/01069b69593af7bff28e2f97afeb3644ae6fe2f5f56cb3a8cf34c5fb8c36efe5?d=mp&s=160"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n> @@ -1382,7 +1382,9 @@ static int verify_absent_1(struct cache_entry *ce,\n>  \t\treturn check_ok_to_remove(ce->name, ce_namelen(ce),\n>  \t\t\t\tce_to_dtype(ce), ce, &st,\n>  \t\t\t\terror_type, o);\n> -\n> +\tif (errno != ENOENT)\n> +\t\treturn error(\"cannot stat '%s': %s\", ce->name,\n> +\t\t\t\tstrerror(errno));\n>  \treturn 0;\n\nIs errno guaranteed to be set to something relevant at this point in the\ncode...?\n\n-miels\n\n-- \nReligion, n. A daughter of Hope and Fear, explaining to Ignorance the nature\nof the Unknowable.\n"},{"id":"159426","messageId":"20110113053849.GA10371@burratino","threadId":"16953","inReplyTo":"buobp3ls9l1.fsf@dhlpc061.dev.necel.com","subject":"Re: [PATCH 1/2] unpack-trees: handle lstat failure for existing directory","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-01-13T05:38:49Z","receivedAt":"2011-01-13T05:38:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Miles Bader wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> @@ -1382,7 +1382,9 @@ static int verify_absent_1(struct cache_entry *ce,\n>>  \t\treturn check_ok_to_remove(ce->name, ce_namelen(ce),\n>>  \t\t\t\tce_to_dtype(ce), ce, &st,\n>>  \t\t\t\terror_type, o);\n>> -\n>> +\tif (errno != ENOENT)\n>> +\t\treturn error(\"cannot stat '%s': %s\", ce->name,\n>> +\t\t\t\tstrerror(errno));\n>>  \treturn 0;\n>\n> Is errno guaranteed to be set to something relevant at this point in the\n> code...?\n\nYes, because lstat failed.  But perhaps that is a hint that the code\nshould be restructured as\n\n\t} else if (lstat(... )) {\n\t\tif (errno != ENOENT)\n\t\t\treturn error(...\n\t\treturn 0;\n\t} else {\n\t\treturn check_ok_to_remove(...\n\t}\n"},{"id":"159452","messageId":"20110113202023.GA15912@localhost","threadId":"16953","inReplyTo":"20110113022415.GA8635@burratino","subject":"Re: [RFC/PATCH 0/2] unpack-trees: handle lstat failures in verify_absent","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-01-13T20:20:23Z","receivedAt":"2011-01-13T20:20:23Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Wed, Jan 12, 2011 at 08:24:15PM -0600, Jonathan Nieder wrote:\n>\n> Here are two cases where we ignore the result from lstat in\n> unpack_trees.  I think we rather shouldn't ignore it.  Sane?\n\nLooks good. Thanks.\n\nBut in addition to the ones you fixed, lstat errors returned by\nlstat_cache_matchlen() in check_leading_path() are also ignored.\nI was actually hoping to restructure this into two functions.\n\n 1) check_path() to see if we need to overwrite anything (leading\n    directory _or_ file of the same name)\n 2) check_ok_to_remove() to check if we can safely overwrite that\n    directory or file\n\nAll the lstat handling would go into check_path(), and\ncheck_ok_to_remove() can reuse the stat returned by check_path().\n\nBut right now I can't say when I will find the time.\n\nClemens\n"}]}