Re: [PATCH] Improvements to git-archive tests and add_submodule_odb()

2013-12-03 Thread Eric Sunshine 
On Mon, Dec 2, 2013 at 7:16 PM, Nick Townsend nick.towns...@mac.com wrote:
 From: Nick Townsend nick.towns...@mac.com
 Date: Sat, 30 Nov 2013 16:54:20 -0800
 Subject: [PATCH 2/2] Additional git-archive tests

 Interplay between paths specified in three ways now tested:
 * After a : in the tree-ish,
 * As a pathspec in the command,
 * By virtue of the current working directory

 Note that these tests are based on the behaviours
 as found in 1.8.5. They may not be intentional.
 They were developed to regression test enhancements
 made to parse_treeish_arg() in archive.c

 Signed-off-by: Nick Townsend nick.towns...@mac.com
 ---
  t/t5004-archive-corner-cases.sh | 67 
 +
  1 file changed, 67 insertions(+)

 diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
 index 67f3b54..8b70e4a 100755
 --- a/t/t5004-archive-corner-cases.sh
 +++ b/t/t5004-archive-corner-cases.sh
 @@ -113,4 +113,71 @@ test_expect_success 'archive empty subtree by direct 
 pathspec' '
 check_dir extract sub
  '

 +test_expect_success 'setup - repository with subdirs' '
 +   mkdir -p a/b/{c,d} 

Unportable use of {foo,bar} notation. POSIX shells [1] will just
create a directory named {c,d}. Better to spell it out:

mkdir -p a/b/c a/b/d 

[1]: 
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_13

 +   echo af a/af 
 +   echo bf a/b/bf 
 +   echo cf a/b/c/cf 
 +   git add a 
 +   git commit -m commit 1 
 +   git tag -a -m rev-1 rev-1
 +'
 +
 +test_expect_success 'archive subtree from root by treeish' '
 +   git archive --format=tar HEAD:a atreeroot.tar 
 +   make_dir extract 
 +   $TAR xf atreeroot.tar -C extract 
 +   check_dir extract af b b/bf b/c b/c/cf
 +'
 +
 +test_expect_success 'archive subtree from root with pathspec' '
 +   git archive --format=tar HEAD a atreepath.tar 
 +   make_dir extract 
 +   $TAR xf atreepath.tar -C extract 
 +   check_dir extract a a/af a/b a/b/bf a/b/c a/b/c/cf
 +'
 +
 +test_expect_success 'archive subtree from root by 2-level treeish' '
 +   git archive --format=tar HEAD:a/b abtreeroot.tar 
 +   make_dir extract 
 +   $TAR xf abtreeroot.tar -C extract 
 +   check_dir extract bf c c/cf
 +'
 +
 +test_expect_success 'archive subtree from subdir' '
 +   cd a 
 +   git archive --format=tar HEAD ../asubtree.tar 
 +   cd .. 
 +   make_dir extract 
 +   $TAR xf asubtree.tar -C extract 
 +   check_dir extract af b b/bf b/c b/c/cf
 +'

If git-archive fails, the subsequent 'cd ..' will not be invoked,
hence all tests following this one will fail since the current
directory has not been restored. If you place the 'cd a' in a
subshell, then the current directory remains unchanged for commands
outside the subshell (and you can drop the 'cd ..'):

(
cd a 
git archive ...
) 
make_dir ...
...

Ditto for the remaining tests which share this problem.

 +
 +test_expect_success 'archive subtree from subdir with treeish' '
 +   cd a 
 +   git archive --format=tar HEAD:./b ../absubtree.tar 
 +   cd .. 
 +   make_dir extract 
 +   $TAR xf absubtree.tar -C extract 
 +   check_dir extract bf c c/cf
 +'
 +
 +test_expect_success 'archive subtree from subdir with treeish and pathspec' '
 +   cd a 
 +   git archive --format=tar HEAD:./b c ../absubtree.tar 
 +   cd .. 
 +   make_dir extract 
 +   $TAR xf absubtree.tar -C extract 
 +   check_dir extract c c/cf
 +'
 +
 +test_expect_success 'archive subtree from subdir with alt treeish' '
 +   cd a 
 +   git archive --format=tar HEAD:b ../abxsubtree.tar 
 +   cd .. 
 +   make_dir extract 
 +   $TAR xf abxsubtree.tar -C extract 
 +   check_dir extract bf c c/cf
 +'
 +
  test_done
 --
 1.8.5.4.g9d8cd78.dirty
--
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] Improvements to git-archive tests and add_submodule_odb()

2013-12-03 Thread Junio C Hamano 
Nick Townsend nick.towns...@mac.com writes:

 From: Nick Townsend nick.towns...@mac.com
 Date: Mon, 25 Nov 2013 15:31:09 -0800
 Subject: [PATCH 1/2] submodule: add_submodule_odb() usability

Please do not add these; instead, drop From/Date (because we can see
them in the mail header from your MUA) and replace the mail header
Subject with this one.

The body of the patch looks OK to me (provided that we'd want to
promote use of that function).

Thanks.

 Although add_submodule_odb() is documented as being
 externally usable, it is declared static and also
 has incorrect documentation.  This commit fixes those
 and makes no changes to existing code using them.
 All tests still pass.

 Improved wording based on Rene Scharfe feedback

 Signed-off-by: Nick Townsend nick.towns...@mac.com
 ---
  Documentation/technical/api-ref-iteration.txt | 4 ++--
  submodule.c   | 2 +-
  submodule.h   | 1 +
  3 files changed, 4 insertions(+), 3 deletions(-)

 diff --git a/Documentation/technical/api-ref-iteration.txt 
 b/Documentation/technical/api-ref-iteration.txt
 index aa1c50f..02adfd4 100644
 --- a/Documentation/technical/api-ref-iteration.txt
 +++ b/Documentation/technical/api-ref-iteration.txt
 @@ -50,10 +50,10 @@ submodules object database. You can do this by a 
 code-snippet like
  this:
  
   const char *path = path/to/submodule
 - if (!add_submodule_odb(path))
 + if (add_submodule_odb(path))
   die(Error submodule '%s' not populated., path);
  
 -`add_submodule_odb()` will return an non-zero value on success. If you
 +`add_submodule_odb()` will return zero on success. If you
  do not do this you will get an error for each ref that it does not point
  to a valid object.
  
 diff --git a/submodule.c b/submodule.c
 index 1905d75..1ea46be 100644
 --- a/submodule.c
 +++ b/submodule.c
 @@ -143,7 +143,7 @@ void stage_updated_gitmodules(void)
   die(_(staging updated .gitmodules failed));
  }
  
 -static int add_submodule_odb(const char *path)
 +int add_submodule_odb(const char *path)
  {
   struct strbuf objects_directory = STRBUF_INIT;
   struct alternate_object_database *alt_odb;
 diff --git a/submodule.h b/submodule.h
 index 7beec48..3e3cdca 100644
 --- a/submodule.h
 +++ b/submodule.h
 @@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], 
 const char *remotes_nam
   struct string_list *needs_pushing);
  int push_unpushed_submodules(unsigned char new_sha1[20], const char 
 *remotes_name);
  void connect_work_tree_and_git_dir(const char *work_tree, const char 
 *git_dir);
 +int add_submodule_odb(const char *path);
  
  #endif
--
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] Improvements to git-archive tests and add_submodule_odb()

2013-12-03 Thread Junio C Hamano 
Eric Sunshine sunsh...@sunshineco.com writes:

 +test_expect_success 'archive subtree from subdir' '
 +   cd a 
 +   git archive --format=tar HEAD ../asubtree.tar 
 +   cd .. 
 +   make_dir extract 
 +   $TAR xf asubtree.tar -C extract 
 +   check_dir extract af b b/bf b/c b/c/cf
 +'

 If git-archive fails, the subsequent 'cd ..' will not be invoked,
 hence all tests following this one will fail since the current
 directory has not been restored. If you place the 'cd a' in a
 subshell, then the current directory remains unchanged for commands
 outside the subshell (and you can drop the 'cd ..'):

 (
 cd a 
 git archive ...
 ) 
 make_dir ...
 ...

Thanks, and please indent the commands run in the subshell for
better readability.
--
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: Re: [PATCH] Improvements to git-archive tests and add_submodule_odb()

2013-12-03 Thread Heiko Voigt 
On Mon, Dec 02, 2013 at 04:14:37PM -0800, Nick Townsend wrote:
 diff --git a/submodule.c b/submodule.c
 index 1905d75..1ea46be 100644
 --- a/submodule.c
 +++ b/submodule.c
 @@ -143,7 +143,7 @@ void stage_updated_gitmodules(void)
   die(_(staging updated .gitmodules failed));
  }
  
 -static int add_submodule_odb(const char *path)
 +int add_submodule_odb(const char *path)

I am not against making add_submodule_odb() usable from outside
submodule.c but I would prefer if this change goes along with some code
actually using it. The reason being that when refactoring or extending
you immediately know that a function is file local only with the static
keyword. Without anyone using this function from outside submodule.c
this fact is still true and so the code should say, IMO.

Its not a big deal to postpone removing this keyword in a later commit
so I would like to drop this change from the patch. The documentation
fix is fine with me.

Cheers Heiko
--
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] Improvements to git-archive tests and add_submodule_odb()

2013-12-03 Thread Junio C Hamano 
Heiko Voigt hvo...@hvoigt.net writes:

 On Mon, Dec 02, 2013 at 04:14:37PM -0800, Nick Townsend wrote:
 diff --git a/submodule.c b/submodule.c
 index 1905d75..1ea46be 100644
 --- a/submodule.c
 +++ b/submodule.c
 @@ -143,7 +143,7 @@ void stage_updated_gitmodules(void)
  die(_(staging updated .gitmodules failed));
  }
  
 -static int add_submodule_odb(const char *path)
 +int add_submodule_odb(const char *path)

 I am not against making add_submodule_odb() usable from outside
 submodule.c but I would prefer if this change goes along with some code
 actually using it. The reason being that when refactoring or extending
 you immediately know that a function is file local only with the static
 keyword. Without anyone using this function from outside submodule.c
 this fact is still true and so the code should say, IMO.

 Its not a big deal to postpone removing this keyword in a later commit
 so I would like to drop this change from the patch. The documentation
 fix is fine with me.

OK, thanks, then let's do this.

-- 8 --
From: Nick Townsend nick.towns...@mac.com
Date: Mon, 25 Nov 2013 15:31:09 -0800
Subject: [PATCH] ref-iteration doc: add_submodule_odb() returns 0 for success

The usage sample of add_submodule_odb() function in the Submodules
section expects non-zero return value for success, but the function
actually reports success with zero.

Helped-by: René Scharfe l@web.de
Reviewed-by: Heiko Voigt hvo...@hvoigt.net
Signed-off-by: Nick Townsend nick.towns...@mac.com
---
 Documentation/technical/api-ref-iteration.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/api-ref-iteration.txt 
b/Documentation/technical/api-ref-iteration.txt
index aa1c50f..02adfd4 100644
--- a/Documentation/technical/api-ref-iteration.txt
+++ b/Documentation/technical/api-ref-iteration.txt
@@ -50,10 +50,10 @@ submodules object database. You can do this by a 
code-snippet like
 this:
 
const char *path = path/to/submodule
-   if (!add_submodule_odb(path))
+   if (add_submodule_odb(path))
die(Error submodule '%s' not populated., path);
 
-`add_submodule_odb()` will return an non-zero value on success. If you
+`add_submodule_odb()` will return zero on success. If you
 do not do this you will get an error for each ref that it does not point
 to a valid object.
 
-- 
1.8.5-262-g1a2486c

--
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: Re* [PATCH] Improvements to git-archive tests and add_submodule_odb()

2013-12-03 Thread Heiko Voigt 
Hi,

On Tue, Dec 03, 2013 at 10:39:36AM -0800, Junio C Hamano wrote:
 OK, thanks, then let's do this.

Yes, sounds good.

Cheers Heiko

 -- 8 --
 From: Nick Townsend nick.towns...@mac.com
 Date: Mon, 25 Nov 2013 15:31:09 -0800
 Subject: [PATCH] ref-iteration doc: add_submodule_odb() returns 0 for success
 
 The usage sample of add_submodule_odb() function in the Submodules
 section expects non-zero return value for success, but the function
 actually reports success with zero.
 
 Helped-by: René Scharfe l@web.de
 Reviewed-by: Heiko Voigt hvo...@hvoigt.net
 Signed-off-by: Nick Townsend nick.towns...@mac.com
 ---
  Documentation/technical/api-ref-iteration.txt | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/Documentation/technical/api-ref-iteration.txt 
 b/Documentation/technical/api-ref-iteration.txt
 index aa1c50f..02adfd4 100644
 --- a/Documentation/technical/api-ref-iteration.txt
 +++ b/Documentation/technical/api-ref-iteration.txt
 @@ -50,10 +50,10 @@ submodules object database. You can do this by a 
 code-snippet like
  this:
  
   const char *path = path/to/submodule
 - if (!add_submodule_odb(path))
 + if (add_submodule_odb(path))
   die(Error submodule '%s' not populated., path);
  
 -`add_submodule_odb()` will return an non-zero value on success. If you
 +`add_submodule_odb()` will return zero on success. If you
  do not do this you will get an error for each ref that it does not point
  to a valid object.
  
 -- 
 1.8.5-262-g1a2486c
 
--
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] Improvements to git-archive tests and add_submodule_odb()

2013-12-02 Thread Nick Townsend 
From: Nick Townsend nick.towns...@mac.com
Date: Mon, 25 Nov 2013 15:31:09 -0800
Subject: [PATCH 1/2] submodule: add_submodule_odb() usability

Although add_submodule_odb() is documented as being
externally usable, it is declared static and also
has incorrect documentation.  This commit fixes those
and makes no changes to existing code using them.
All tests still pass.

Improved wording based on Rene Scharfe feedback

Signed-off-by: Nick Townsend nick.towns...@mac.com
---
 Documentation/technical/api-ref-iteration.txt | 4 ++--
 submodule.c   | 2 +-
 submodule.h   | 1 +
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/api-ref-iteration.txt 
b/Documentation/technical/api-ref-iteration.txt
index aa1c50f..02adfd4 100644
--- a/Documentation/technical/api-ref-iteration.txt
+++ b/Documentation/technical/api-ref-iteration.txt
@@ -50,10 +50,10 @@ submodules object database. You can do this by a 
code-snippet like
 this:
 
const char *path = path/to/submodule
-   if (!add_submodule_odb(path))
+   if (add_submodule_odb(path))
die(Error submodule '%s' not populated., path);
 
-`add_submodule_odb()` will return an non-zero value on success. If you
+`add_submodule_odb()` will return zero on success. If you
 do not do this you will get an error for each ref that it does not point
 to a valid object.
 
diff --git a/submodule.c b/submodule.c
index 1905d75..1ea46be 100644
--- a/submodule.c
+++ b/submodule.c
@@ -143,7 +143,7 @@ void stage_updated_gitmodules(void)
die(_(staging updated .gitmodules failed));
 }
 
-static int add_submodule_odb(const char *path)
+int add_submodule_odb(const char *path)
 {
struct strbuf objects_directory = STRBUF_INIT;
struct alternate_object_database *alt_odb;
diff --git a/submodule.h b/submodule.h
index 7beec48..3e3cdca 100644
--- a/submodule.h
+++ b/submodule.h
@@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], 
const char *remotes_nam
struct string_list *needs_pushing);
 int push_unpushed_submodules(unsigned char new_sha1[20], const char 
*remotes_name);
 void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
+int add_submodule_odb(const char *path);
 
 #endif
-- 
1.8.5.4.g9d8cd78.dirty

On 2 Dec 2013, at 16:10, Nick Townsend nick.towns...@mac.com wrote:

 As per the previous patch request, I’ve delayed the work on git-archive.
 However the following two patches (attached as replies) should still
 be considered.
 Kind Regards
 Nick

--
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] Improvements to git-archive tests and add_submodule_odb()

2013-12-02 Thread Nick Townsend 
From: Nick Townsend nick.towns...@mac.com
Date: Sat, 30 Nov 2013 16:54:20 -0800
Subject: [PATCH 2/2] Additional git-archive tests

Interplay between paths specified in three ways now tested:
* After a : in the tree-ish,
* As a pathspec in the command,
* By virtue of the current working directory

Note that these tests are based on the behaviours
as found in 1.8.5. They may not be intentional.
They were developed to regression test enhancements
made to parse_treeish_arg() in archive.c

Signed-off-by: Nick Townsend nick.towns...@mac.com
---
 t/t5004-archive-corner-cases.sh | 67 +
 1 file changed, 67 insertions(+)

diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
index 67f3b54..8b70e4a 100755
--- a/t/t5004-archive-corner-cases.sh
+++ b/t/t5004-archive-corner-cases.sh
@@ -113,4 +113,71 @@ test_expect_success 'archive empty subtree by direct 
pathspec' '
check_dir extract sub
 '
 
+test_expect_success 'setup - repository with subdirs' '
+   mkdir -p a/b/{c,d} 
+   echo af a/af 
+   echo bf a/b/bf 
+   echo cf a/b/c/cf 
+   git add a 
+   git commit -m commit 1 
+   git tag -a -m rev-1 rev-1
+'
+
+test_expect_success 'archive subtree from root by treeish' '
+   git archive --format=tar HEAD:a atreeroot.tar 
+   make_dir extract 
+   $TAR xf atreeroot.tar -C extract 
+   check_dir extract af b b/bf b/c b/c/cf
+'
+
+test_expect_success 'archive subtree from root with pathspec' '
+   git archive --format=tar HEAD a atreepath.tar 
+   make_dir extract 
+   $TAR xf atreepath.tar -C extract 
+   check_dir extract a a/af a/b a/b/bf a/b/c a/b/c/cf
+'
+
+test_expect_success 'archive subtree from root by 2-level treeish' '
+   git archive --format=tar HEAD:a/b abtreeroot.tar 
+   make_dir extract 
+   $TAR xf abtreeroot.tar -C extract 
+   check_dir extract bf c c/cf
+'
+
+test_expect_success 'archive subtree from subdir' '
+   cd a 
+   git archive --format=tar HEAD ../asubtree.tar 
+   cd .. 
+   make_dir extract 
+   $TAR xf asubtree.tar -C extract 
+   check_dir extract af b b/bf b/c b/c/cf
+'
+
+test_expect_success 'archive subtree from subdir with treeish' '
+   cd a 
+   git archive --format=tar HEAD:./b ../absubtree.tar 
+   cd .. 
+   make_dir extract 
+   $TAR xf absubtree.tar -C extract 
+   check_dir extract bf c c/cf
+'
+
+test_expect_success 'archive subtree from subdir with treeish and pathspec' '
+   cd a 
+   git archive --format=tar HEAD:./b c ../absubtree.tar 
+   cd .. 
+   make_dir extract 
+   $TAR xf absubtree.tar -C extract 
+   check_dir extract c c/cf
+'
+
+test_expect_success 'archive subtree from subdir with alt treeish' '
+   cd a 
+   git archive --format=tar HEAD:b ../abxsubtree.tar 
+   cd .. 
+   make_dir extract 
+   $TAR xf abxsubtree.tar -C extract 
+   check_dir extract bf c c/cf
+'
+
 test_done
-- 
1.8.5.4.g9d8cd78.dirty

On 2 Dec 2013, at 16:10, Nick Townsend nick.towns...@mac.com wrote:

 As per the previous patch request, I’ve delayed the work on git-archive.
 However the following two patches (attached as replies) should still
 be considered.
 Kind Regards
 Nick

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