Re: [PATCH] submodule recursion in git-archive

[Nick Townsend <nick.townsend@mac.com>]

On 26 Nov 2013, at 14:18, Junio C Hamano <gitster@pobox.com> wrote:

> René Scharfe <l.s.r@web.de> writes:
> 
>> Thanks for the patches!  Please send only one per message (the second
>> one as a reply to the first one, or both as replies to a cover letter),
>> though -- that makes commenting on them much easier.
>> 
>> Side note: Documentation/SubmittingPatches doesn't mention that (yet),
>> AFAICS.
> 
> OK, how about doing this then?
> 
> Documentation/SubmittingPatches | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 7055576..304b3c0 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -140,7 +140,12 @@ comment on the changes you are submitting.  It is important for
> a developer to be able to "quote" your changes, using standard
> e-mail tools, so that they may comment on specific portions of
> your code.  For this reason, all patches should be submitted
> -"inline".  If your log message (including your name on the
> +"inline".  A patch series that consists of N commits is sent as N
> +separate e-mail messages, or a cover letter message (see below) with
> +N separate e-mail messages, each being a response to the cover
> +letter.
> +
> +If your log message (including your name on the
> Signed-off-by line) is not writable in ASCII, make sure that
> you send off a message in the correct encoding.
> 
> 
>>> The feature is disabled for remote repositories as
>>> the git_work_tree fails. This is a possible future
>>> enhancement.
>> 
>> Hmm, curious.  Why does it fail?  I guess that happens with bare
>> repositories, only, right?  (Which are the most likely kind of remote
>> repos to encounter, of course.)
> 
> Yeah, I do not think of a reason why it should fail in a bare
> repository, either. "git archive" is about writing out the contents
> of an already recorded tree, so there shouldn't be a reason to even
> call get_git_work_tree() in the first place.
> 
See below for a discussion of why I use the .git file in the work tree to 
load the objects for the submodule. I also thought it should work in a
remote repository - but I ran it on a properly initialized remote repository and
it failed. Since I didn’t need it for my immediate use-case I just decided to disable 
it with an error. I can look into this further, but we must decide about the question 
below first…

> Even if the code is run inside a repository with a working tree,
> when producing a tarball out of an ancient commit that had a
> submodule not at its current location, --recurse-submodules option
> should do the right thing, so asking for working tree location of
> that submodule to find its repository is wrong, I think.  It may
> happen to find one if the archived revision is close enough to what
> is currently checked out, but that may not necessarily be the case.
> 
> At that point when the code discovers an S_ISGITLINK entry, it
> should have both a pathname to the submodule relative to the
> toplevel and the commit object name bound to that submodule
> location.  What it should do, when it does not find the repository
> at the given path (maybe because there is no working tree, or the
> sudmodule directory has moved over time) is roughly:
> 
> - Read from .gitmodules at the top-level from the tree it is
>   creating the tarball out of;
> 
> - Find "submodule.$name.path" entry that records that path to the
>   submodule; and then
> 
> - Using that $name, find the stashed-away location of the submodule
>   repository in $GIT_DIR/modules/$name.
> 
> or something like that.
> 
> This is a related tangent, but when used in a repository that people
> often use as their remote, the repository discovery may have to
> interact with the relative URL.  People often ship .gitmodules with
> 
> 	[submodule "bar"]
>        	URL = ../bar.git
> 		path = barDir
> 
> for a top-level project "foo" that can be cloned thusly:
> 
> 	git clone git://site.xz/foo.git
> 
> and host bar.git to be clonable with
> 
> 	git clone git://site.xz/bar.git barDir/
> 
> inside the working tree of the foo project.  In such a case, when
> "archive --recurse-submodules" is running, it would find the
> repository for the "bar" submodule at "../bar.git", I would think.
> 
> So this part needs a bit more thought, I am afraid.

I see that there is a lot of potential complexity around setting up a submodule:
* The .gitmodules file can be dirty (easy to flag, but should we allow archive to proceed?)
* Users can mess with settings both prior to git submodule init and before git submodule update.
* What if it’s a raw clone and the user manually changes things between init and update?
* I’m not a git-internals expert but looking through the code I see that you can add additional object
directories and change paths as you show above.

For those reasons I deliberately decided not to reproduce the above logic all by myself.
On the other hand, what it *did* seem to me is that once you have the .git file
then you know you’ve got all that covered. So I just used that. This restricts the function to
working only on a properly setup repository - but that is my use case!

If you think that doing this more extensive setup is even *viable* given the space between
init and update then I”m happy to try it. I didn’t want to start off on a fools errand.

> 
>>> 'git archive' [--format=<fmt>] [--list] [--prefix=<prefix>/] [<extra>]
>>> 	      [-o <file> | --output=<file>] [--worktree-attributes]
>>> +	      [--recursive|--recurse-submodules]
>> 
>> I'd expect git archive --recurse to add subdirectories and their
>> contents, which it does right now, and --no-recurse to only archive the
>> specified objects, which is not implemented.  IAW: I wouldn't normally
>> associate an option with that name with submodules.  Would
>> --recurse-submodules alone suffice?
> 
> Jens already commented on this, and I agree that --recursive should
> be dropped from this patch.
I only put —recursive because that is what git-clone has for it’s behaviour wrt submodules.
If that flag is deprecated then I’m fine with using only —recurse-submodules
Perhaps a deprecation flag or note in the code would help?


Overall I’m impressed by the speed and quality of the responses (and the codebase!) so am glad to
move this forward. I look forward to your feedback.

Kind Regards
Nick