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

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

[Nick Townsend]

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

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

[Nick Townsend]

From: Nick Townsend <nick.townsend@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.townsend@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.townsend@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

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

[Nick Townsend]

From: Nick Townsend <nick.townsend@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.townsend@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.townsend@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

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

[Eric Sunshine]

On Mon, Dec 2, 2013 at 7:16 PM, Nick Townsend <nick.townsend@mac.com> wrote:
> From: Nick Townsend <nick.townsend@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.townsend@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

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

[Junio C Hamano]

Nick Townsend <nick.townsend@mac.com> writes:

> From: Nick Townsend <nick.townsend@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.townsend@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

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

[Junio C Hamano]

Eric Sunshine <sunshine@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.

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

[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

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

[Junio C Hamano]

Heiko Voigt <hvoigt@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.townsend@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.s.r@web.de>
Reviewed-by: Heiko Voigt <hvoigt@hvoigt.net>
Signed-off-by: Nick Townsend <nick.townsend@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

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

[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.townsend@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.s.r@web.de>
> Reviewed-by: Heiko Voigt <hvoigt@hvoigt.net>
> Signed-off-by: Nick Townsend <nick.townsend@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
> 

[PATCH] Additional git-archive tests

[Nick Townsend]

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

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Nick Townsend <nick.townsend@mac.com>
---
 t/t5004-archive-corner-cases.sh | 71 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
index 67f3b54..a81a836 100755
--- a/t/t5004-archive-corner-cases.sh
+++ b/t/t5004-archive-corner-cases.sh
@@ -113,4 +113,75 @@ 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 a/b/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
+	) &&
+	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
+	) &&
+	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
+	) &&
+	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
+	) &&
+	make_dir extract &&
+	"$TAR" xf abxsubtree.tar -C extract &&
+	check_dir extract bf c c/cf
+'
+
 test_done
-- 
1.8.5

On 3 Dec 2013, at 09:54, Junio C Hamano <gitster@pobox.com> wrote:

> Eric Sunshine <sunshine@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.

Re: [PATCH] Additional git-archive tests

[Junio C Hamano]

Nick Townsend <nick.townsend@mac.com> writes:

> 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

In other words, are all these new tests expected to pass?

My cursory read of parse_treeish_arg() in archive.c is:

 - It reads the given object with get_sha1(), checking if it is a
   commit-ish or tree-ish to decide if it wants to add the pax
   header to record the commit object name;

 - It parses the tree object;

 - If run from a subdirectory, attempts to grab the "prefix"
   (i.e. the path to the current subdirectory---in the tests you
   added, they are all "a/") out of that tree object (it errors out
   if it can't); and then

 - It archives the tree object.

So I do not think it is expected to accept tree object names with
the HEAD:<path> style syntax, if the user expects a predictable and
consistent result.  The third step above attempts to make sure that
you name a tree-ish that corresponds to the top-level of the
project, i.e. with no <path>.

What seems to be supported are:

    cd a && git archive HEAD ;# archives HEAD:a tree
    cd a && git archive HEAD -- b ;# archives a/b/ part of HEAD:a as b/

Specifically, it appears that HEAD:./b, HEAD:b etc. are not designed
to work, at least to me.

I am not saying that these should _not_ work, but it is unclear what
it means to "work".  For example, what should this do?

    cd a && git archive HEAD:./b $pathspec

The extended SHA-1 expression HEAD:./b in the subdirectory a/ is
interpreted by get_sha1_with_context_1() to be the name of the tree
object at path "a/b" in the commit HEAD.  Further, you are giving a
pathspec while in a subdirectory a/ to the command.  What should
that pathspec be relative to?

In a normal Git command, the pathspec always is relative to the
current subdirectory, so, the way to learn about the tree object
a/b/c in the HEAD while in subdirectory a/ would be:

    cd a && git ls-tree HEAD b/c

But what should the command line for archive to grab HEAD:a/b/c be?
It feels wrong to say:

    cd a && git archive HEAD:./b b/c

and it also feels wrong to say

    cd a && git archive HEAD:./b c

No matter what we would do, we should behave consistently with this
case:

    treeish=$(git rev-parse HEAD:a/b) &&
    cd a &&
    git archive $treeish -- $pathspec

so "take the pathspec relative to the tree when the treeish was
given with '<treeish>:<path>' syntax, and otherwise treat it
relative to the cwd" is not a workable solution.

> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Nick Townsend <nick.townsend@mac.com>
> ---
>  t/t5004-archive-corner-cases.sh | 71 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
>
> diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
> index 67f3b54..a81a836 100755
> --- a/t/t5004-archive-corner-cases.sh
> +++ b/t/t5004-archive-corner-cases.sh
> @@ -113,4 +113,75 @@ 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 a/b/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
> +	) &&
> +	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
> +	) &&
> +	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
> +	) &&
> +	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
> +	) &&
> +	make_dir extract &&
> +	"$TAR" xf abxsubtree.tar -C extract &&
> +	check_dir extract bf c c/cf
> +'
> +
>  test_done

Re: [PATCH] Additional git-archive tests

[Nick Townsend]

On 5 Dec 2013, at 11:52, Junio C Hamano <gitster@pobox.com> wrote:

> Nick Townsend <nick.townsend@mac.com> writes:
> 
>> 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
> 
> In other words, are all these new tests expected to pass?
> 
Sorry about that - the first four tests do pass with 1.8.5, the last
three tests do not currently pass. I believe they should pass and
with my reworked git-archive patch (similar to the previous one)they do.
(though I removed that update from this set pending the discussion).
Currently 5 and 6 fail with the message:
‘fatal: current working directory is untracked’
and  number 7 fails with: ‘fatal: Not a valid object name'

> My cursory read of parse_treeish_arg() in archive.c is:
> 
> - It reads the given object with get_sha1(), checking if it is a
>   commit-ish or tree-ish to decide if it wants to add the pax
>   header to record the commit object name;
> 
> - It parses the tree object;
> 
> - If run from a subdirectory, attempts to grab the "prefix"
>   (i.e. the path to the current subdirectory---in the tests you
>   added, they are all "a/") out of that tree object (it errors out
>   if it can't); and then
> 
> - It archives the tree object.
> 
> So I do not think it is expected to accept tree object names with
> the HEAD:<path> style syntax, if the user expects a predictable and
> consistent result.  The third step above attempts to make sure that
> you name a tree-ish that corresponds to the top-level of the
> project, i.e. with no <path>.
> 
That’s not quite right - paths do work from the root directory - see tests.
It was this very useful capability that I sought to add and generalized
the code to do. 
> What seems to be supported are:
> 
>    cd a && git archive HEAD ;# archives HEAD:a tree
>    cd a && git archive HEAD -- b ;# archives a/b/ part of HEAD:a as b/
> 
> Specifically, it appears that HEAD:./b, HEAD:b etc. are not designed
> to work, at least to me.
> 
Clearly when you specify them today they don’t work, but I believe they
should work.

> I am not saying that these should _not_ work, but it is unclear what
> it means to "work".  For example, what should this do?
> 
>    cd a && git archive HEAD:./b $pathspec
> 
I think we can clear this up by documenting the cases and choosing
sensible behaviour _for git-archive_ ie. what people might expect.
Here is a suggestion:

a. The pathspec is used as a selector to include things in the archive.
it is applied after the cwd and treeish selection.

b. The current working directory (if present) prefixes a path in the object
if one is present.

c. The path in the object (if present) is prefixed by the cwd (if present) and
used to select items for archiving. However the composite path so created
*is not present* in the archive - ie. the leading components are stripped.
(This is very useful behaviour for creating archives without leading paths)

d. As currently the —prefix option (not the prefix from setup_git_directory)
 is prepended to all entries in the archive.

These correspond to the use cases that I have for git archive - extracting all
or part of a multiple submodule tree into a tgz file, with or without leading
directories.

> The extended SHA-1 expression HEAD:./b in the subdirectory a/ is
> interpreted by get_sha1_with_context_1() to be the name of the tree
> object at path "a/b" in the commit HEAD.  Further, you are giving a
> pathspec while in a subdirectory a/ to the command.  What should
> that pathspec be relative to?
> 
> In a normal Git command, the pathspec always is relative to the
> current subdirectory, so, the way to learn about the tree object
> a/b/c in the HEAD while in subdirectory a/ would be:
> 
>    cd a && git ls-tree HEAD b/c
> 
> But what should the command line for archive to grab HEAD:a/b/c be?
> It feels wrong to say:
> 
>    cd a && git archive HEAD:./b b/c
It’s clear to me that if you are in a subdirectory, that is an implicit prefix on the 
./b so you retrieve HEAD:a/b  which then archives everything in b without the
leading a/b. The pathspec is wrong (including the b) and this archive is empty. 
> 
> and it also feels wrong to say
> 
>    cd a && git archive HEAD:./b c
> 
That looks fine to me given the rules suggested above. Also git-parse manages
to return the correct thing in this case, so I’d expect this to work.

> No matter what we would do, we should behave consistently with this
> case:
> 
>    treeish=$(git rev-parse HEAD:a/b) &&
>    cd a &&
>    git archive $treeish -- $pathspec
> 
Well this will fail both in the existing case (‘fatal: current working directory is  untracked’)
and with the scheme proposed above (‘fatal: invalid object name $treeish:a/‘) 

> so "take the pathspec relative to the tree when the treeish was
> given with '<treeish>:<path>' syntax, and otherwise treat it
> relative to the cwd" is not a workable solution.

I’m not seeing that - that’s not quite the same as the algorithm above is it?
> 
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Nick Townsend <nick.townsend@mac.com>
>> ---
>> t/t5004-archive-corner-cases.sh | 71 +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 71 insertions(+)
>> 
>> diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
>> index 67f3b54..a81a836 100755
>> --- a/t/t5004-archive-corner-cases.sh
>> +++ b/t/t5004-archive-corner-cases.sh
>> @@ -113,4 +113,75 @@ 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 a/b/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
>> +	) &&
>> +	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
>> +	) &&
>> +	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
>> +	) &&
>> +	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
>> +	) &&
>> +	make_dir extract &&
>> +	"$TAR" xf abxsubtree.tar -C extract &&
>> +	check_dir extract bf c c/cf
>> +'
>> +
>> test_done

Re: [PATCH] Additional git-archive tests

[Junio C Hamano]

Nick Townsend <nick.townsend@mac.com> writes:

>> In other words, are all these new tests expected to pass?
>
> Sorry about that - the first four tests do pass with 1.8.5, the last
> three tests do not currently pass.

OK.

>> So I do not think it is expected to accept tree object names with
>> the HEAD:<path> style syntax, if the user expects a predictable and
>> consistent result.  The third step above attempts to make sure that
>> you name a tree-ish that corresponds to the top-level of the
>> project, i.e. with no <path>.
>> 
> That’s not quite right - paths do work from the root directory - see tests.

I know that they work from the very top, but they _only_ happens to
work from the very top.  As I do not see the code is designed to
compare the prefix (i.e. the subdirectory you are in) and the path
that is tucked after the name of the top-level tree object with a
colon (e.g. path in "HEAD:path") and adjust these two accordingly, I
do not think it was designed to deal with that at all.  The code
just assumes that the tree object corresponds to the top-level, and
the adjustment is done solely by adding the prefix when limiting the
output by paths.

This observation *is* very right. Not designing to deal with that
case may or may not be right, but that is a different issue.

> It was this very useful capability that I sought to add and generalized
> the code to do. 
>> What seems to be supported are:
>> 
>>    cd a && git archive HEAD ;# archives HEAD:a tree
>>    cd a && git archive HEAD -- b ;# archives a/b/ part of HEAD:a as b/
>> 
>> Specifically, it appears that HEAD:./b, HEAD:b etc. are not designed
>> to work, at least to me.
>> ...
>> I am not saying that these should _not_ work, but it is unclear what
>> it means to "work".  For example, what should this do?
>> 
>>    cd a && git archive HEAD:./b $pathspec
>> 
> I think we can clear this up by documenting the cases and choosing
> sensible behaviour _for git-archive_ ie. what people might expect.

I am afraid that this topic is not limited to "git-archive";
"sensible behaviour _for git-archive_" will end up being far from
sufficient.  I would not be surprised if "ls-tree" shares the same
issue, for example.  If you had "subdir/subsubdir/path" (and other
paths) tracked in your project,

    cd subdir
    git ls-tree HEAD:./subsubdir path

is likely to have the same issue as

    cd subdir
    git archive HEAD:./subsubdir path

I would think.

> Here is a suggestion:
>
> a. The pathspec is used as a selector to include things in the archive.
> it is applied after the cwd and treeish selection.

That would mean that doing this in the top-level:

        git archive HEAD:subdir/subsubdir path<TAB>

will not auto-complete the pathname.  In all other Git commands,
vanilla pathspecs (at least the ones that do not use the magic
escape hatch ":/path", etc.) are relative to your current directory,
and I do not think we want a single command to work in an
inconsistent way with respect to that.

> b. The current working directory (if present) prefixes a path in the object
> if one is present.

That is already done at a lower-level get_sha1_with_context(), I
think.

	cd subdir
	git archive HEAD:./subsubdir

will try to use tree subdir/subsubdir in the HEAD commit (but due to
the prefix logic that is not preprared to use such a tree object,
you will get "current working directory is untracked" error).

> c. The path in the object (if present) is prefixed by the cwd (if present) and
> used to select items for archiving. However the composite path so created
> *is not present* in the archive - ie. the leading components are stripped.
> (This is very useful behaviour for creating archives without leading paths)
>
> d. As currently the ―prefix option (not the prefix from setup_git_directory)
>  is prepended to all entries in the archive.
>
> These correspond to the use cases that I have for git archive - extracting all
> or part of a multiple submodule tree into a tgz file, with or without leading
> directories.
>
>> The extended SHA-1 expression HEAD:./b in the subdirectory a/ is
>> interpreted by get_sha1_with_context_1() to be the name of the tree
>> object at path "a/b" in the commit HEAD.  Further, you are giving a
>> pathspec while in a subdirectory a/ to the command.  What should
>> that pathspec be relative to?
>> 
>> In a normal Git command, the pathspec always is relative to the
>> current subdirectory, so, the way to learn about the tree object
>> a/b/c in the HEAD while in subdirectory a/ would be:
>> 
>>    cd a && git ls-tree HEAD b/c
>> 
>> But what should the command line for archive to grab HEAD:a/b/c be?
>> It feels wrong to say:
>> 
>>    cd a && git archive HEAD:./b b/c
> It’s clear to me that if you are in a subdirectory, that is an implicit prefix on the 
> ./b so you retrieve HEAD:a/b  which then archives everything in b without the
> leading a/b. The pathspec is wrong (including the b) and this archive is empty. 
>> 
>> and it also feels wrong to say
>> 
>>    cd a && git archive HEAD:./b c
>> 
> That looks fine to me given the rules suggested above.
>
> Also git-parse manages
> to return the correct thing in this case, so I’d expect this to work.
>
>> No matter what we would do, we should behave consistently with this
>> case:
>> 
>>    treeish=$(git rev-parse HEAD:a/b) &&
>>    cd a &&
>>    git archive $treeish -- $pathspec
>> 
> Well this will fail both in the existing case (‘fatal: current
> working directory is untracked’) and with the scheme proposed
> above (‘fatal: invalid object name $treeish:a/‘)

The thing is the "current working directory is untracked" is a
fallout from the current archive code not expecting to see a
tree-ish specifed in HEAD:<path> style to interact with $cwd (aka
prefix).  Go back to the description in how parse_treeish_arg()
works in the message you are responding to (or to archive.c).  It
says "if we have prefix, grab the tree object that corresponds to
the prefix in the given tree-ish".  When "subdir" is your current
directory, if you give HEAD as the tree-ish, this logic will give
you the subtree HEAD:subdir; if you give anything else that does not
correspond to the top-level of the project, it will not be able to
find such a subtree (by definition, any tree-ish that does not
correspond to the top-level cannot contain "subdir" tree object that
corresponds to "subdir" directory of the top-level; it may contain a
random directory that happens to be named "subdir", but that will be
a different directory from the "subdir" you see at the top-level).

I do not think a workable compromise exists, but one that is closest
to be workable, even though I do not think it is such a great idea,
would be to apply the add "prefix" logic _only_ when the given
tree-ish is known to be at the top-level (e.g. when it was
originally given as a commit).