git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 1/2] dir.c: fix bug in 'nd/exclusion-regression-fix' topic

From
Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Date
Mar 17, 2016, 12:45 UTC
Message-ID
<1458218744-15810-1-git-send-email-pclouds@gmail.com>
In-Reply-To
<56E9F5B3.6030903@fb.com>

The topic in question introduces "sticky" path list for this purpose: given a path 'abc/def', if 'abc' already matches a pattern X, it's added to X's sticky path list. When we need to check if 'abc/def' matches pattern X and see that 'abc' is already in the list, we conclude right away that 'abc/def' also matches X.

The short reason (*) for sticky path list is to workaround limitations of matching code that will return "not match" when we compare 'abc/def' and pattern X.

The bug is in this code. Not only it does "when we need to check if 'abc/def' matches...", it does an extra thing: if 'foo/bar' is _not_ in the list, return 'not matched' by bypassing all matching code with the "continue;" statement. It should let the remaining code decide match status instead.

This bug affects both .gitignore and sparse checkout, but it's reported as a sparse checkout bug, so let's examine how it happens. The sparse-checkout pattern has two rules

    /*
    !one/hideme

and the worktree has three tracked files, one/hideme, one/showme and two/showme. What happens is this

* check "one", it matches the first pattern -> positive -> keep
  examining.
*1* "thanks" to 'nd/exclusion-regression-fix' we detect this pair of
  patterns, so we put "one" in the sticky list of pattern "/*"
* enter "one", check "one/hideme", it matches the second pattern
  first (we search from bottom up) -> negative -> excluded
* check "one/showme", it does not match the second pattern.
*2* We then check it against the first pattern and notice the sticky list
  that includes "one", so we decide right away that "one/showme" is
  included.
* leave "one", check "two" which does not match the second pattern.
*3* then we check "two" against the first pattern and notice that this
   pattern has a non-empty sticky list, which contains "one", not "two".
   This bug kicks in and bypasses the true matching logic for pattern
   "/*". As a result, we exclude "two/showme".

One may notice that the order of these steps matter. If *3* occurs before *1*, then the sticky list at that moment is empty and the bug does not kick in. Sparse checkout always examines entries in alphabetical order, so "abc/showme" would be examined before "one" and not hit this bug!

The last remark is important when we move to .gitignore. We receive the list of entries with readdir() and cannot control the order of entries. Which means we can't write a test for .gitignore that will reliably fail without this fix. Which is why this patch only adds a test for sparse checkout, even though the same above steps happen to .gitignore.

(*) The problem is known and will be fixed later and described in detail then. For this commit, it's sufficient to see the following link because the long reason is really long:

http://article.gmane.org/gmane.comp.version-control.git/288479
Reported-by: Durham Goode <durham@fb.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 dir.c                                |  1 -
 t/t1011-read-tree-sparse-checkout.sh | 20 ++++++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/dir.c b/dir.c
index 69e0be6..77f38a5 100644
--- a/dir.c
+++ b/dir.c
@@ -1027,7 +1027,6 @@ static struct exclude *last_exclude_matching_from_list(const char *pathname,
 				exc = x;
 				break;
 			}
-			continue;
 		}
 
 		if (x->flags & EXC_FLAG_MUSTBEDIR) {
diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 0c74bee..ecc5e93 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -274,4 +274,24 @@ test_expect_success 'checkout with --ignore-skip-worktree-bits' '
 	git diff --exit-code HEAD
 '
 
+test_expect_success 'sparse checkout and dir.c sticky bits' '
+	git init sticky &&
+	(
+		cd sticky &&
+		mkdir one two &&
+		touch one/hideme one/showme two/showme &&
+		git add . &&
+		git commit -m initial &&
+		cat >.git/info/sparse-checkout <<-\EOF &&
+		/*
+		!one/hideme
+		EOF
+		git config core.sparsecheckout true &&
+		git checkout &&
+		test_path_is_missing one/hideme &&
+		test_path_is_file    one/showme &&
+		test_path_is_file    two/showme
+	)
+'
+
 test_done
-- 
2.8.0.rc0.210.gd302cd2
Previous: Durham GoodeNext: Nguyễn Thái Ngọc Duy
Message 13 of 30 in “bug: sparse config interpretation incorrectness in 2.8.0-rc2”
  1. Durham GoodeMar 17, 2016
  2. Duy NguyenMar 17, 2016
  3. Durham GoodeMar 17, 2016
  4. Junio C HamanoMar 17, 2016
  5. Duy NguyenMar 17, 2016
  6. Johannes SchindelinMar 17, 2016
  7. Duy NguyenMar 17, 2016
  8. Johannes SchindelinMar 17, 2016
  9. Duy NguyenMar 17, 2016
  10. Johannes SchindelinMar 18, 2016
  11. Junio C HamanoMar 17, 2016
  12. Durham GoodeMar 17, 2016
  13. 1/2 dir.c: fix bug in 'nd/exclusion-regression-fix' topicNguyễn Thái Ngọc Duy, Mar 17, 2016
  14. 1/2 dir.c: fix bug in 'nd/exclusion-regression-fix' topicNguyễn Thái Ngọc Duy, Mar 17, 2016
  15. 3/2 dir.c: fix dir re-inclusion rules with "NODIR" and "MUSTBEDIR"Nguyễn Thái Ngọc Duy, Mar 17, 2016
  16. Junio C HamanoMar 17, 2016
  17. Duy NguyenMar 18, 2016
  18. Junio C HamanoMar 18, 2016
  19. Duy NguyenMar 18, 2016
  20. Eric SunshineMar 18, 2016
  21. Durham GoodeMar 18, 2016
  22. Duy NguyenMar 18, 2016
  23. Durham GoodeMar 18, 2016
  24. Duy NguyenMar 18, 2016
  25. Junio C HamanoMar 18, 2016
  26. Extending this cycle by a week and reverting !reinclusion topicJunio C Hamano, Mar 18, 2016
  27. Duy NguyenMar 19, 2016
  28. 2/2 dir.c: correct "stuck" logging logicNguyễn Thái Ngọc Duy, Mar 17, 2016
  29. 2/2 dir.c: correct "stuck" logging logicNguyễn Thái Ngọc Duy, Mar 17, 2016
  30. Junio C HamanoMar 18, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.