Re: [PATCH v2 00/19] Index-v5
Junio C Hamano gits...@pobox.com writes: Thomas Gummerer t.gumme...@gmail.com writes: Ah ok, I understand. I think it's best to add a GIT_INDEX_VERSION=x config option to config.mak, where x is the index version that should be tested. Whatever you do, please do not call it GIT_INDEX_VERSION _if_ it is only to be used while testing. Have string TEST somewhere in the name, and make t/test-lib-functions.sh take notice. Currently, the way for the user to show the preference as to which index version to use is to explicitly set the version once, and then we will (try to) propagate it inside the repository. I would not mind if we add a mechanism to make write_index() notice the environment variable GIT_INDEX_VERSION and write the index in that version. But that is conceptually very different from whatever you give make VARIABLE=blah from the command line when building Git (or set in config.mak which amounts to the same thing). What I currently did is add a environment variable GIT_INDEX_VERSION that is used only if there is no index yet, to make sure existing repositories aren't affected and still have to be converted explicitly by using git update-index. For the tests I simply did export GIT_INDEX_VERSION in test-lib.sh to allow the addition of GIT_INDEX_VERSION in config.mak. Should I rename that to GIT_INDEX_VERSION_TEST and do something like set_index_version() { export GIT_INDEX_VERSION=$GIT_INDEX_VERSION_TEST } in test-lib-functions.sh instead, does that make sense? -- 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 v2 00/19] Index-v5
Thomas Gummerer t.gumme...@gmail.com writes: What I currently did is add a environment variable GIT_INDEX_VERSION that is used only if there is no index yet, to make sure existing repositories aren't affected and still have to be converted explicitly by using git update-index. For the tests I simply did export GIT_INDEX_VERSION in test-lib.sh to allow the addition of GIT_INDEX_VERSION in config.mak. Should I rename that to GIT_INDEX_VERSION_TEST and do something like set_index_version() { export GIT_INDEX_VERSION=$GIT_INDEX_VERSION_TEST } in test-lib-functions.sh instead, does that make sense? Mostly Yes ;-). You have to write it in a valid POSIX shell, i.e. set_index_version () { GIT_INDEX_VERSION=$TEST_GIT_INDEX_VERSION export GIT_INDEX_VERSION } -- 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 v2 00/19] Index-v5
Ramsay Jones ram...@ramsay1.demon.co.uk writes: Thomas Gummerer wrote: Hi, previous rounds (without api) are at $gmane/202752, $gmane/202923, $gmane/203088 and $gmane/203517, the previous round with api was at $gmane/229732. Thanks to Junio, Duy and Eric for their comments on the previous round. If I remember correctly, the original version of this series had the same problem as Michael's Fix some reference-related races series (now in master). In particular, you had introduced an 'index_changed()' function which does essentially the same job as 'stat_validity_check()' in the new reference handling API. I seem to remember advising you not to compare st_uid, st_gid and st_ino on __CYGWIN__. Yes, you provided a patch that simply wrapped those checks in a #if !defined (__CYGWIN__), which is included in the new series too. I haven't had time to look at this version of your series yet, but it may be worth taking a look at stat_validity_check(). (although that is causing failures on cygwin at the moment! ;-) I took a quick look, that function makes sense I think. I'll use it in the re-roll. It makes probably sense to wrap the uid, gid and ino fields as in the index_changed function. Also, I can't recall if I mentioned it to you at the time, but your index reading code was (unnecessarily) calling munmap() twice on the same buffer (without an intervening mmap()). This causes problems for systems that have the NO_MMAP build variable set. In particular, the compat/mmap.c code will attempt to free() the allocated memory block twice, with unpredictable results. I wrote a patch to address this at the time (Hmm, seems to be built on v1.8.1), but didn't submit it since your patch didn't progress. :-D I have included the patch below. I can't recall this either. From a quick check I don't call munmap() on a already unmapped mmap, so I think this is fine as it is and your patch is independent from it. Not sure if it makes sense as safeguard for future changes. -- 8 -- From: Ramsay Jones ram...@ramsay1.demon.co.uk Date: Sun, 9 Sep 2012 20:50:32 +0100 Subject: [PATCH] mmap.c: Keep log of mmap() blocks to avoid double-delete bug When compiling with the NO_MMAP build variable set, the built-in 'git_mmap()' and 'git_munmap()' compatibility routines use simple memory allocation and file I/O to emulate the required behaviour. The current implementation is vulnerable to the double-delete bug (where the pointer returned by malloc() is passed to free() two or more times), should the mapped memory block address be passed to munmap() multiple times. In order to guard the implementation from such a calling sequence, we keep a list of mmap-block descriptors, which we then consult to determine the validity of the input pointer to munmap(). This then allows 'git_munmap()' to return -1 on error, as required, with errno set to EINVAL. Using a list in the log of mmap-ed blocks, along with the resulting linear search, means that the performance of the code is directly proportional to the number of concurrently active memory mapped file regions. The number of such regions is not expected to be excessive. Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk --- compat/mmap.c | 57 - 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/compat/mmap.c b/compat/mmap.c index c9d46d1..400e034 100644 --- a/compat/mmap.c +++ b/compat/mmap.c @@ -1,14 +1,61 @@ #include ../git-compat-util.h +struct mmbd { /* memory mapped block descriptor */ + struct mmbd *next; /* next in list */ + void *start; /* pointer to memory mapped block */ + size_t length; /* length of memory mapped block */ +}; + +static struct mmbd *head; /* head of mmb descriptor list */ + + +static void add_desc(struct mmbd *desc, void *start, size_t length) +{ + desc-start = start; + desc-length = length; + desc-next = head; + head = desc; +} + +static void free_desc(struct mmbd *desc) +{ + if (head == desc) + head = head-next; + else { + struct mmbd *d = head; + for (; d; d = d-next) { + if (d-next == desc) { + d-next = desc-next; + break; + } + } + } + free(desc); +} + +static struct mmbd *find_desc(void *start) +{ + struct mmbd *d = head; + for (; d; d = d-next) { + if (d-start == start) + return d; + } + return NULL; +} + void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset) { size_t n = 0; + struct mmbd *desc = NULL; if (start != NULL || !(flags MAP_PRIVATE)) die(Invalid usage of mmap when built with NO_MMAP); start = xmalloc(length); - if (start == NULL) { + desc
Re: [PATCH v2 00/19] Index-v5
Duy Nguyen pclo...@gmail.com writes: On Mon, Jul 15, 2013 at 4:30 PM, Thomas Gummerer t.gumme...@gmail.com wrote: Duy Nguyen pclo...@gmail.com writes: On Sat, Jul 13, 2013 at 12:26 AM, Thomas Gummerer t.gumme...@gmail.com wrote: t/perf/p0003-index.sh| 59 + t/t2104-update-index-skip-worktree.sh|1 + For such a big code addition, the test part seems modest. Speaking from my experience, I rarely run perf tests and make test does not activate v5 code at all. A few more tests would be nice. The good news is I changed default index version to 5 and ran make test, all passed. I was using the test suite with index version 5 as default index version for testing of the new index file format. I think that's the best way to test the index, as it covers all aspects. We need other people to run the test suite with v5 to catch bugs that only appear on other platforms. Perhaps an env variable to allow the test suite to override the binary's default index version and a new make target test-v5 maybe. Ah ok, I understand. I think it's best to add a GIT_INDEX_VERSION=x config option to config.mak, where x is the index version that should be tested. This is also the way other test options are set (e.g. NO_SVN_TESTS). In addition this allows testing other versions of the index file too. Maybe we should add a test that covers the basic functionality, just to make sure nothing obvious is broken when running the test suite with index-v2? Something like this maybe: Yep. v5 specfic test cases can cover some corner cases that the rest of the test suite does not care. Haven't read your t1600 though. Ok. I can't think of any corner cases right now that would be in v5, but not in other versions, but if I catch some I'll add tests. -- 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 v2 00/19] Index-v5
Thomas Gummerer t.gumme...@gmail.com writes: Ah ok, I understand. I think it's best to add a GIT_INDEX_VERSION=x config option to config.mak, where x is the index version that should be tested. Whatever you do, please do not call it GIT_INDEX_VERSION _if_ it is only to be used while testing. Have string TEST somewhere in the name, and make t/test-lib-functions.sh take notice. Currently, the way for the user to show the preference as to which index version to use is to explicitly set the version once, and then we will (try to) propagate it inside the repository. I would not mind if we add a mechanism to make write_index() notice the environment variable GIT_INDEX_VERSION and write the index in that version. But that is conceptually very different from whatever you give make VARIABLE=blah from the command line when building Git (or set in config.mak which amounts to the same thing). -- 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 v2 00/19] Index-v5
Thomas Gummerer wrote: Hi, previous rounds (without api) are at $gmane/202752, $gmane/202923, $gmane/203088 and $gmane/203517, the previous round with api was at $gmane/229732. Thanks to Junio, Duy and Eric for their comments on the previous round. If I remember correctly, the original version of this series had the same problem as Michael's Fix some reference-related races series (now in master). In particular, you had introduced an 'index_changed()' function which does essentially the same job as 'stat_validity_check()' in the new reference handling API. I seem to remember advising you not to compare st_uid, st_gid and st_ino on __CYGWIN__. I haven't had time to look at this version of your series yet, but it may be worth taking a look at stat_validity_check(). (although that is causing failures on cygwin at the moment! ;-) Also, I can't recall if I mentioned it to you at the time, but your index reading code was (unnecessarily) calling munmap() twice on the same buffer (without an intervening mmap()). This causes problems for systems that have the NO_MMAP build variable set. In particular, the compat/mmap.c code will attempt to free() the allocated memory block twice, with unpredictable results. I wrote a patch to address this at the time (Hmm, seems to be built on v1.8.1), but didn't submit it since your patch didn't progress. :-D I have included the patch below. ATB, Ramsay Jones -- 8 -- From: Ramsay Jones ram...@ramsay1.demon.co.uk Date: Sun, 9 Sep 2012 20:50:32 +0100 Subject: [PATCH] mmap.c: Keep log of mmap() blocks to avoid double-delete bug When compiling with the NO_MMAP build variable set, the built-in 'git_mmap()' and 'git_munmap()' compatibility routines use simple memory allocation and file I/O to emulate the required behaviour. The current implementation is vulnerable to the double-delete bug (where the pointer returned by malloc() is passed to free() two or more times), should the mapped memory block address be passed to munmap() multiple times. In order to guard the implementation from such a calling sequence, we keep a list of mmap-block descriptors, which we then consult to determine the validity of the input pointer to munmap(). This then allows 'git_munmap()' to return -1 on error, as required, with errno set to EINVAL. Using a list in the log of mmap-ed blocks, along with the resulting linear search, means that the performance of the code is directly proportional to the number of concurrently active memory mapped file regions. The number of such regions is not expected to be excessive. Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk --- compat/mmap.c | 57 - 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/compat/mmap.c b/compat/mmap.c index c9d46d1..400e034 100644 --- a/compat/mmap.c +++ b/compat/mmap.c @@ -1,14 +1,61 @@ #include ../git-compat-util.h +struct mmbd { /* memory mapped block descriptor */ + struct mmbd *next; /* next in list */ + void *start; /* pointer to memory mapped block */ + size_t length; /* length of memory mapped block */ +}; + +static struct mmbd *head; /* head of mmb descriptor list */ + + +static void add_desc(struct mmbd *desc, void *start, size_t length) +{ + desc-start = start; + desc-length = length; + desc-next = head; + head = desc; +} + +static void free_desc(struct mmbd *desc) +{ + if (head == desc) + head = head-next; + else { + struct mmbd *d = head; + for (; d; d = d-next) { + if (d-next == desc) { + d-next = desc-next; + break; + } + } + } + free(desc); +} + +static struct mmbd *find_desc(void *start) +{ + struct mmbd *d = head; + for (; d; d = d-next) { + if (d-start == start) + return d; + } + return NULL; +} + void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset) { size_t n = 0; + struct mmbd *desc = NULL; if (start != NULL || !(flags MAP_PRIVATE)) die(Invalid usage of mmap when built with NO_MMAP); start = xmalloc(length); - if (start == NULL) { + desc = xmalloc(sizeof(*desc)); + if (!start || !desc) { + free(start); + free(desc); errno = ENOMEM; return MAP_FAILED; } @@ -25,18 +72,26 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of if (errno == EAGAIN || errno == EINTR) continue; free(start); + free(desc); errno = EACCES; return MAP_FAILED; } n += count; } +
Re: [PATCH v2 00/19] Index-v5
Duy Nguyen pclo...@gmail.com writes: On Sat, Jul 13, 2013 at 12:26 AM, Thomas Gummerer t.gumme...@gmail.com wrote: t/perf/p0003-index.sh| 59 + t/t2104-update-index-skip-worktree.sh|1 + For such a big code addition, the test part seems modest. Speaking from my experience, I rarely run perf tests and make test does not activate v5 code at all. A few more tests would be nice. The good news is I changed default index version to 5 and ran make test, all passed. I was using the test suite with index version 5 as default index version for testing of the new index file format. I think that's the best way to test the index, as it covers all aspects. Maybe we should add a test that covers the basic functionality, just to make sure nothing obvious is broken when running the test suite with index-v2? Something like this maybe: ---8--- From c476b521c94f1a9b0b4fcfe92d63321442d79c9a Mon Sep 17 00:00:00 2001 From: Thomas Gummerer t.gumme...@gmail.com Date: Mon, 15 Jul 2013 11:21:06 +0200 Subject: [PATCH] t1600: add basic test for index-v5 Add a test that checks the index-v5 file format when running the test-suite with index-v2 as default index format. When making changes to the index, the test suite still should be run with both index v2 and index v5 as default index format, for better coverage of all aspects of the index. Signed-off-by: Thomas Gummerer t.gumme...@gmail.com --- t/t1600-index-v5.sh | 133 1 file changed, 133 insertions(+) create mode 100755 t/t1600-index-v5.sh diff --git a/t/t1600-index-v5.sh b/t/t1600-index-v5.sh new file mode 100755 index 000..528c17e --- /dev/null +++ b/t/t1600-index-v5.sh @@ -0,0 +1,133 @@ +#!/bin/sh +# +# Copyright (c) 2013 Thomas Gummerer + +test_description='Test basic functionaltiy of index-v5. + +This test just covers the basics, to make sure normal runs of the test +suite cover this version of the index file format too. For better +testing of the index-v5 format, the default index version should be +changed to 5 and the test suite should be re-run' + +. ./test-lib.sh + +check_resolve_undo () { + msg=$1 + shift + while case $# in + 0) break ;; + 1|2|3) die Bug in check-resolve-undo test ;; + esac + do + path=$1 + shift + for stage in 1 2 3 + do + sha1=$1 + shift + case $sha1 in + '') continue ;; + esac + sha1=$(git rev-parse --verify $sha1) + printf 100644 %s %s\t%s\n $sha1 $stage $path + done + done $msg.expect + git ls-files --resolve-undo $msg.actual + test_cmp $msg.expect $msg.actual +} + +prime_resolve_undo () { + git reset --hard + git checkout second^0 + test_tick + test_must_fail git merge third^0 + echo merge does not leave anything + check_resolve_undo empty + echo different fi/le + git add fi/le + echo resolving records + check_resolve_undo recorded fi/le initial:fi/le second:fi/le third:fi/le +} + +test_expect_success 'setup' ' + git update-index --index-version=5 + echo file1 file1 + echo file2 file2 + mkdir -p top/sub + echo x top/x + echo xy top/xy + echo y top/y + echo yx top/yx + echo sub1 top/sub/sub1 + git add . + git commit -m initial import +' + +test_expect_success 'ls-files shows all files' ' + cat expected -EOF + 100644 e2129701f1a4d54dc44f03c93bca0a2aec7c5449 0 file1 + 100644 6c493ff740f9380390d5c9ddef4af18697ac9375 0 file2 + 100644 48df0cb83fee5d667537343f60a6057a63dd3c9b 0 top/sub/sub1 + 100644 587be6b4c3f93f93c489c0111bba5596147a26cb 0 top/x + 100644 5aad9376af82d7b98a34f95fb0f298a162f52656 0 top/xy + 100644 975fbec8256d3e8a3797e7a3611380f27c49f4ac 0 top/y + 100644 ba1575927fa5b1f4bce72ad0c349566f1b02508e 0 top/yx + EOF + git ls-files --stage actual + test_cmp expected actual +' + +test_expect_success 'ls-files with pathspec in subdir' ' + cd top/sub + cat expected -EOF + ../x + ../xy + EOF + git ls-files --error-unmatch ../x* actual + test_cmp expected actual + cd ../.. +' + +test_expect_success 'read-tree HEAD establishes cache-tree' ' + git read-tree HEAD + cat expected -EOF + 84e73410ea7864ccada24d897462e8ce1e1b872b (7 entries, 1 subtrees) + 602482536bd3852e8ac2977ed1a9913a8c244aa0 top/ (5 entries, 1 subtrees) + 20bb0109200f37a7e19283b4abc4a672be3f0adb top/sub/ (1 entries, 0 subtrees) + EOF + test-dump-cache-tree actual + test_cmp expected actual +' + +test_expect_success 'setup
Re: [PATCH v2 00/19] Index-v5
On Mon, Jul 15, 2013 at 4:30 PM, Thomas Gummerer t.gumme...@gmail.com wrote: Duy Nguyen pclo...@gmail.com writes: On Sat, Jul 13, 2013 at 12:26 AM, Thomas Gummerer t.gumme...@gmail.com wrote: t/perf/p0003-index.sh| 59 + t/t2104-update-index-skip-worktree.sh|1 + For such a big code addition, the test part seems modest. Speaking from my experience, I rarely run perf tests and make test does not activate v5 code at all. A few more tests would be nice. The good news is I changed default index version to 5 and ran make test, all passed. I was using the test suite with index version 5 as default index version for testing of the new index file format. I think that's the best way to test the index, as it covers all aspects. We need other people to run the test suite with v5 to catch bugs that only appear on other platforms. Perhaps an env variable to allow the test suite to override the binary's default index version and a new make target test-v5 maybe. Maybe we should add a test that covers the basic functionality, just to make sure nothing obvious is broken when running the test suite with index-v2? Something like this maybe: Yep. v5 specfic test cases can cover some corner cases that the rest of the test suite does not care. Haven't read your t1600 though. -- Duy -- 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 v2 00/19] Index-v5
On Sat, Jul 13, 2013 at 12:26 AM, Thomas Gummerer t.gumme...@gmail.com wrote: t/perf/p0003-index.sh| 59 + t/t2104-update-index-skip-worktree.sh|1 + For such a big code addition, the test part seems modest. Speaking from my experience, I rarely run perf tests and make test does not activate v5 code at all. A few more tests would be nice. The good news is I changed default index version to 5 and ran make test, all passed. -- Duy -- 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