Re: [PATCH 1/3] introduce GIT_INDEX_VERSION environment variable

2014-02-21 Thread Thomas Gummerer
Junio C Hamano gits...@pobox.com writes:

 Thomas Gummerer t.gumme...@gmail.com writes:

 diff --git a/Documentation/git.txt b/Documentation/git.txt
 index aec3726..bc9eeea 100644
 --- a/Documentation/git.txt
 +++ b/Documentation/git.txt
 @@ -712,6 +712,11 @@ Git so take care if using Cogito etc.
  index file. If not specified, the default of `$GIT_DIR/index`
  is used.
  
 +'GIT_INDEX_VERSION'::
 +This environment variable allows the specification of an index
 +version for new repositories.  It won't affect existing index
 +files.  By default index file version 3 is used.
 +

 This is half-correct, isn't it?  In-code variable may say version 3
 but we demote it to version 2 unless we absolutely need to use the
 version 3 ugliness.

Yes, you're right, to be correct we should say [23] instead of 3 here
maybe?

 diff --git a/read-cache.c b/read-cache.c
 index 3f735f3..3993e12 100644
 --- a/read-cache.c
 +++ b/read-cache.c
 @@ -1223,6 +1223,22 @@ static struct cache_entry *refresh_cache_entry(struct 
 cache_entry *ce, int reall
  
  #define INDEX_FORMAT_DEFAULT 3
  
 +static unsigned int get_index_format_default()
 +{
 +char *envversion = getenv(GIT_INDEX_VERSION);
 +if (!envversion) {
 +return INDEX_FORMAT_DEFAULT;
 +} else {
 +unsigned int version = strtol(envversion, NULL, 10);

 If we are using strtol() to parse it carefully, we should make sure
 it parses to the end by giving a non-NULL second argument and
 checking where the parsing stopped.

Thanks, makes sense, will fix it in the re-roll.

 diff --git a/t/t1600-index.sh b/t/t1600-index.sh
 new file mode 100755
 index 000..37fd84d
 --- /dev/null
 +++ b/t/t1600-index.sh
 @@ -0,0 +1,24 @@
 +#!/bin/sh
 +
 +test_description='index file specific tests'
 +
 +. ./test-lib.sh
 +
 +test_expect_success 'setup' '
 +echo 1 a
 +'
 +
 +test_expect_success 'out of bounds GIT_INDEX_VERSION issues warning' '
 +(
 +GIT_INDEX_VERSION=1 
 +export GIT_INDEX_VERSION 
 +git add a 21 | sed s/[0-9]// actual.err 
 +sed -e s/ Z$/ / -\EOF expect.err 
 +warning: GIT_INDEX_VERSION set, but the value is 
 invalid.
 +Using version Z
 +EOF
 +test_i18ncmp expect.err actual.err
 +)
 +'

 If we already have an index in version N when this test starts, what
 should happen?

I think we shouldn't print anything, since we won't change the file
format.  I'll add another test for that.

 Will queue on 'pu', with some suggested fixups.

Thanks, I think both fixups in 'pu' make sense, so I'll include them in
the re-roll.

 Thanks.
--
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 1/3] introduce GIT_INDEX_VERSION environment variable

2014-02-18 Thread Junio C Hamano
Thomas Gummerer t.gumme...@gmail.com writes:

 diff --git a/Documentation/git.txt b/Documentation/git.txt
 index aec3726..bc9eeea 100644
 --- a/Documentation/git.txt
 +++ b/Documentation/git.txt
 @@ -712,6 +712,11 @@ Git so take care if using Cogito etc.
   index file. If not specified, the default of `$GIT_DIR/index`
   is used.
  
 +'GIT_INDEX_VERSION'::
 + This environment variable allows the specification of an index
 + version for new repositories.  It won't affect existing index
 + files.  By default index file version 3 is used.
 +

This is half-correct, isn't it?  In-code variable may say version 3
but we demote it to version 2 unless we absolutely need to use the
version 3 ugliness.

 diff --git a/read-cache.c b/read-cache.c
 index 3f735f3..3993e12 100644
 --- a/read-cache.c
 +++ b/read-cache.c
 @@ -1223,6 +1223,22 @@ static struct cache_entry *refresh_cache_entry(struct 
 cache_entry *ce, int reall
  
  #define INDEX_FORMAT_DEFAULT 3
  
 +static unsigned int get_index_format_default()
 +{
 + char *envversion = getenv(GIT_INDEX_VERSION);
 + if (!envversion) {
 + return INDEX_FORMAT_DEFAULT;
 + } else {
 + unsigned int version = strtol(envversion, NULL, 10);

If we are using strtol() to parse it carefully, we should make sure
it parses to the end by giving a non-NULL second argument and
checking where the parsing stopped.

 diff --git a/t/t1600-index.sh b/t/t1600-index.sh
 new file mode 100755
 index 000..37fd84d
 --- /dev/null
 +++ b/t/t1600-index.sh
 @@ -0,0 +1,24 @@
 +#!/bin/sh
 +
 +test_description='index file specific tests'
 +
 +. ./test-lib.sh
 +
 +test_expect_success 'setup' '
 + echo 1 a
 +'
 +
 +test_expect_success 'out of bounds GIT_INDEX_VERSION issues warning' '
 + (
 + GIT_INDEX_VERSION=1 
 + export GIT_INDEX_VERSION 
 + git add a 21 | sed s/[0-9]// actual.err 
 + sed -e s/ Z$/ / -\EOF expect.err 
 + warning: GIT_INDEX_VERSION set, but the value is 
 invalid.
 + Using version Z
 + EOF
 + test_i18ncmp expect.err actual.err
 + )
 +'

If we already have an index in version N when this test starts, what
should happen?

Will queue on 'pu', with some suggested fixups.

Thanks.
--
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