* [PATCH] Improvements to git-archive tests and add_submodule_odb() @ 2013-12-03 0:10 Nick Townsend 2013-12-03 0:14 ` Nick Townsend 2013-12-03 0:16 ` Nick Townsend 0 siblings, 2 replies; 13+ messages in thread From: Nick Townsend @ 2013-12-03 0:10 UTC (permalink / raw) To: git; +Cc: René Scharfe, Jens Lehmann, Heiko Voigt, 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Improvements to git-archive tests and add_submodule_odb() 2013-12-03 0:10 [PATCH] Improvements to git-archive tests and add_submodule_odb() Nick Townsend @ 2013-12-03 0:14 ` Nick Townsend 2013-12-03 17:52 ` Junio C Hamano 2013-12-03 18:18 ` Heiko Voigt 2013-12-03 0:16 ` Nick Townsend 1 sibling, 2 replies; 13+ messages in thread From: Nick Townsend @ 2013-12-03 0:14 UTC (permalink / raw) To: git; +Cc: René Scharfe, Jens Lehmann, Heiko Voigt 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] Improvements to git-archive tests and add_submodule_odb() 2013-12-03 0:14 ` Nick Townsend @ 2013-12-03 17:52 ` Junio C Hamano 2013-12-03 18:18 ` Heiko Voigt 1 sibling, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2013-12-03 17:52 UTC (permalink / raw) To: Nick Townsend; +Cc: git, René Scharfe, Jens Lehmann, Heiko Voigt 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Re: [PATCH] Improvements to git-archive tests and add_submodule_odb() 2013-12-03 0:14 ` Nick Townsend 2013-12-03 17:52 ` Junio C Hamano @ 2013-12-03 18:18 ` Heiko Voigt 2013-12-03 18:39 ` Re* " Junio C Hamano 1 sibling, 1 reply; 13+ messages in thread From: Heiko Voigt @ 2013-12-03 18:18 UTC (permalink / raw) To: Nick Townsend; +Cc: git, René Scharfe, Jens Lehmann 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re* [PATCH] Improvements to git-archive tests and add_submodule_odb() 2013-12-03 18:18 ` Heiko Voigt @ 2013-12-03 18:39 ` Junio C Hamano 2013-12-03 20:42 ` Heiko Voigt 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2013-12-03 18:39 UTC (permalink / raw) To: Heiko Voigt; +Cc: Nick Townsend, git, René Scharfe, Jens Lehmann 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: Re* [PATCH] Improvements to git-archive tests and add_submodule_odb() 2013-12-03 18:39 ` Re* " Junio C Hamano @ 2013-12-03 20:42 ` Heiko Voigt 0 siblings, 0 replies; 13+ messages in thread From: Heiko Voigt @ 2013-12-03 20:42 UTC (permalink / raw) To: Junio C Hamano; +Cc: Nick Townsend, git, René Scharfe, Jens Lehmann 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Improvements to git-archive tests and add_submodule_odb() 2013-12-03 0:10 [PATCH] Improvements to git-archive tests and add_submodule_odb() Nick Townsend 2013-12-03 0:14 ` Nick Townsend @ 2013-12-03 0:16 ` Nick Townsend 2013-12-03 9:26 ` Eric Sunshine 1 sibling, 1 reply; 13+ messages in thread From: Nick Townsend @ 2013-12-03 0:16 UTC (permalink / raw) To: git; +Cc: René Scharfe, Jens Lehmann, Heiko Voigt 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] Improvements to git-archive tests and add_submodule_odb() 2013-12-03 0:16 ` Nick Townsend @ 2013-12-03 9:26 ` Eric Sunshine 2013-12-03 17:54 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Eric Sunshine @ 2013-12-03 9:26 UTC (permalink / raw) To: Nick Townsend; +Cc: Git List, René Scharfe, Jens Lehmann, Heiko Voigt 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Improvements to git-archive tests and add_submodule_odb() 2013-12-03 9:26 ` Eric Sunshine @ 2013-12-03 17:54 ` Junio C Hamano 2013-12-05 2:49 ` [PATCH] Additional git-archive tests Nick Townsend 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2013-12-03 17:54 UTC (permalink / raw) To: Eric Sunshine Cc: Nick Townsend, Git List, René Scharfe, Jens Lehmann, Heiko Voigt 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] Additional git-archive tests 2013-12-03 17:54 ` Junio C Hamano @ 2013-12-05 2:49 ` Nick Townsend 2013-12-05 19:52 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Nick Townsend @ 2013-12-05 2:49 UTC (permalink / raw) To: Git List Cc: Eric Sunshine, Junio C Hamano, René Scharfe, Jens Lehmann, Heiko Voigt 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. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] Additional git-archive tests 2013-12-05 2:49 ` [PATCH] Additional git-archive tests Nick Townsend @ 2013-12-05 19:52 ` Junio C Hamano 2013-12-10 5:26 ` Nick Townsend 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2013-12-05 19:52 UTC (permalink / raw) To: Nick Townsend Cc: Git List, Eric Sunshine, René Scharfe, Jens Lehmann, Heiko Voigt 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Additional git-archive tests 2013-12-05 19:52 ` Junio C Hamano @ 2013-12-10 5:26 ` Nick Townsend 2013-12-10 18:48 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Nick Townsend @ 2013-12-10 5:26 UTC (permalink / raw) To: Junio C Hamano Cc: Git List, Eric Sunshine, René Scharfe, Jens Lehmann, Heiko Voigt 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Additional git-archive tests 2013-12-10 5:26 ` Nick Townsend @ 2013-12-10 18:48 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2013-12-10 18:48 UTC (permalink / raw) To: Nick Townsend Cc: Git List, Eric Sunshine, René Scharfe, Jens Lehmann, Heiko Voigt 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). ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-12-10 18:49 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-12-03 0:10 [PATCH] Improvements to git-archive tests and add_submodule_odb() Nick Townsend 2013-12-03 0:14 ` Nick Townsend 2013-12-03 17:52 ` Junio C Hamano 2013-12-03 18:18 ` Heiko Voigt 2013-12-03 18:39 ` Re* " Junio C Hamano 2013-12-03 20:42 ` Heiko Voigt 2013-12-03 0:16 ` Nick Townsend 2013-12-03 9:26 ` Eric Sunshine 2013-12-03 17:54 ` Junio C Hamano 2013-12-05 2:49 ` [PATCH] Additional git-archive tests Nick Townsend 2013-12-05 19:52 ` Junio C Hamano 2013-12-10 5:26 ` Nick Townsend 2013-12-10 18:48 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).