Re: [PATCH 2/2] dir: test_one_path: fix inconsistent behavior due to missing '/'
Eric Sunshine wrote: Although undocumented, directory_exists_in_index_icase(dirname,len) unconditionally assumes the presence of a '/' at dirname[len] (despite being past the end-of-string). Callers are expected to respect [...] Fix this problem. So, does this fix the problem by changing directory_exists_in_index_icase() to be more liberal in what it accepts, or callers to be more conservative in what they pass in? Please forgive my laziness. I ask in order to save future readers the time of digging into the code. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] dir: test_one_path: fix inconsistent behavior due to missing '/'
On Sun, Aug 25, 2013 at 2:00 AM, Jonathan Nieder jrnie...@gmail.com wrote: Eric Sunshine wrote: Although undocumented, directory_exists_in_index_icase(dirname,len) unconditionally assumes the presence of a '/' at dirname[len] (despite being past the end-of-string). Callers are expected to respect [...] Fix this problem. So, does this fix the problem by changing directory_exists_in_index_icase() to be more liberal in what it accepts, or callers to be more conservative in what they pass in? It places the onus upon the caller. As mentioned in the cover letter [1], I was not happy with this solution. Junio felt likewise. A follow-up series [2] fixes both directory_exists_in_index() and directory_exists_in_index_icase() to be more liberal in what they accept, relieving the caller of the burden. By the time that series was posted, however, Junio and Peff had decided that a fix at a more fundamental level would be better (a conclusion with which I agree, but for which I do not yet have sufficient knowledge about git internals to implement). In the meantime, as an interim bug fix, Junio decided [3] to apply the patch to which you responded (but with an updated commit message). [1]: http://thread.gmane.org/gmane.comp.version-control.git/232796 [2]: http://thread.gmane.org/gmane.comp.version-control.git/232833 [3]: http://thread.gmane.org/gmane.comp.version-control.git/232833/focus=232837 Please forgive my laziness. I ask in order to save future readers the time of digging into the code. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] dir: test_one_path: fix inconsistent behavior due to missing '/'
Eric Sunshine sunsh...@sunshineco.com writes: ... A follow-up series [2] fixes both directory_exists_in_index() and directory_exists_in_index_icase() to be more liberal in what they accept, relieving the caller of the burden. By the time that series was posted, however, Junio and Peff had decided that a fix at a more fundamental level would be better (a conclusion with which I agree, Thanks for a summary; the last would be better is a qualified one, though. My understanding is that we agreed that it would be better *if* we can fix it at a more fundamental level. I looked at the codepaths involved just enough to make that off the cuff suggestion and no deeper than that, and I suspect neither did Peff. If any of us looked at the problem deep enough, we may realize that it would affect too many things, and $gmane/232833/focus=232835 (your second round) may turn out to be a better solution. I think none of us know yet. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] dir: test_one_path: fix inconsistent behavior due to missing '/'
Although undocumented, directory_exists_in_index_icase(dirname,len)
unconditionally assumes the presence of a '/' at dirname[len] (despite
being past the end-of-string). Callers are expected to respect this
assumption by ensuring that a '/' is present beyond the last character
of the passed path. directory_exists_in_index(), on the other hand,
does not assume nor care about a trailing '/' beyond end-of-string.
2eac2a4cc4bdc8d7 (ls-files -k: a directory only can be killed if the
index has a non-directory; 2013-08-15) adds a caller which forgets to
ensure the trailing '/', thus leading to inconsistent behavior between
directory_exists_in_index() and directory_exists_in_index_icase()
depending upon the setting of core.ignorecase. Fix this problem.
This also fixes an initially-unnoticed failure in a t3010 test added by
3c56875176390eee (t3010: update to demonstrate ls-files -k
optimization pitfalls; 2013-08-15) when core.ignorecase is true.
Signed-off-by: Eric Sunshine sunsh...@sunshineco.com
---
dir.c | 12 +---
t/t3103-ls-tree-misc.sh | 2 +-
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/dir.c b/dir.c
index edd666a..a52c6f9 100644
--- a/dir.c
+++ b/dir.c
@@ -1160,9 +1160,15 @@ static enum path_treatment treat_one_path(struct
dir_struct *dir,
*/
if ((dir-flags DIR_COLLECT_KILLED_ONLY)
(dtype == DT_DIR)
- !has_path_in_index
- (directory_exists_in_index(path-buf, path-len) ==
index_nonexistent))
- return path_none;
+ !has_path_in_index) {
+ strbuf_addch(path, '/');
+ if (directory_exists_in_index(path-buf, path-len - 1) ==
+ index_nonexistent) {
+ strbuf_setlen(path, path-len - 1);
+ return path_none;
+ }
+ strbuf_setlen(path, path-len - 1);
+ }
exclude = is_excluded(dir, path-buf, dtype);
diff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh
index fd95991..9fb1706 100755
--- a/t/t3103-ls-tree-misc.sh
+++ b/t/t3103-ls-tree-misc.sh
@@ -22,7 +22,7 @@ test_expect_success 'ls-tree fails with non-zero exit code on
broken tree' '
test_must_fail git ls-tree -r HEAD
'
-test_expect_failure 'ls-tree directory core.ignorecase' '
+test_expect_success 'ls-tree directory core.ignorecase' '
cat expect -\EOF
d/e/f
EOF
--
1.8.4.rc4.529.g78818d7
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
