Re: Re* [PATCH] Improvements to git-archive tests and add_submodule_odb()
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 > 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 > Reviewed-by: Heiko Voigt > Signed-off-by: Nick Townsend > --- > 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()
Heiko Voigt 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 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 Reviewed-by: Heiko Voigt Signed-off-by: Nick Townsend --- 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()
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()
Eric Sunshine 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: [PATCH] Improvements to git-archive tests and add_submodule_odb()
Nick Townsend writes: > From: Nick Townsend > 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 > --- > 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()
On Mon, Dec 2, 2013 at 7:16 PM, Nick Townsend wrote: > From: Nick Townsend > 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 > --- > 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()
From: Nick Townsend 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 --- 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 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()
From: Nick Townsend 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 --- 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 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
[PATCH] Improvements to git-archive tests and add_submodule_odb()
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