subject:"\[PATCH v2 00\/19\] Index\-v5"

Re: [PATCH v2 00/19] Index-v5

2013-07-19 Thread Thomas Gummerer

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

2013-07-19 Thread Junio C Hamano

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

2013-07-17 Thread Thomas Gummerer

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

2013-07-17 Thread Thomas Gummerer

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

2013-07-17 Thread Junio C Hamano

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

2013-07-16 Thread Ramsay Jones

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

2013-07-15 Thread Thomas Gummerer

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

2013-07-15 Thread Duy Nguyen

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

2013-07-13 Thread Duy Nguyen

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

Re: [PATCH v2 00/19] Index-v5

Re: [PATCH v2 00/19] Index-v5

Re: [PATCH v2 00/19] Index-v5

Re: [PATCH v2 00/19] Index-v5

Re: [PATCH v2 00/19] Index-v5

Re: [PATCH v2 00/19] Index-v5

Re: [PATCH v2 00/19] Index-v5

Re: [PATCH v2 00/19] Index-v5

Re: [PATCH v2 00/19] Index-v5

9 matches

Site Navigation

Mail list logo

Footer information