Re: [PATCH 1/2] Add git-archive

[Rene Scharfe <rene.scharfe@lsrfire.ath.cx>]

Franck Bui-Huu schrieb:
> Junio C Hamano wrote:
>> "Franck Bui-Huu" <vagabon.xyz@gmail.com> writes:
>>
>>> git-archive is a command to make TAR and ZIP archives of a git tree.
>>> It helps prevent a proliferation of git-{format}-tree commands.
>> Thanks.  I like the overall structure, at least mostly.
>> Also dropping -tree suffix from the command name is nice, short
>> and sweet.
>>
> 
> great !
> 
>> Obviously I cannot apply this patch because it is totally
>> whitespace damaged, but here are some comments.
> 
> (sigh), sorry for that.
> 
>>> diff --git a/archive.h b/archive.h
>>> new file mode 100644
>>> index 0000000..6c69953
>>> --- /dev/null
>>> +++ b/archive.h
>>> @@ -0,0 +1,43 @@
>>> +#ifndef ARCHIVE_H
>>> +#define ARCHIVE_H
>>> +
>>> +typedef int (*write_archive_fn_t)(struct tree *tree,
>>> +				  const unsigned char *commit_sha1,
>>> +				  const char *prefix,
>>> +				  time_t time,
>>> +				  const char **pathspec);
>> The type of the first argument might have to be different,
>> depending on performance analysis by Rene on struct tree vs
>> struct tree_desc.
>>
> 
> OK. We'll wait for Rene.

The performance difference I noticed was caused by a memleak; the speed
advantage of a struct tree_desc based traverser is significant if you
look only at the traversers' performance, but it is lost in the noise
of the "real" work that the payload function is doing (see my other
mail).

>>> +static int run_remote_archiver(struct archiver_struct *ar, int argc,
>>> +			       const char **argv)
>>> +{
>>> +	char *url, buf[1024];
>>> +	pid_t pid;
>>> +	int fd[2];
>>> +	int len, rv;
>>> +
>>> +	sprintf(buf, "git-upload-%s", ar->name);
>> Are you calling git-upload-{tar,zip,rar,...} here?
>>
> 
> yes. Actually git-upload-{tar,zip,...} commands are going to be
> removed, but git-daemon know them as a daemon service. It will
> map these services to the generic "git-upload-archive" command.
> One benefit is that we could still disable TAR format and enable
> TGZ one. Please take a look to the second patch that adds
> git-upload-archive command.

I don't think git-daemon should need to care about specific
archivers.  Policy decisions, like disallowing certain archive types
or compression levels, should be made in git-upload-archive.  This
way all code regarding archive uploading is found in one place:
git-upload-archive.  We can keep git-upload-tar as a legacy
interface, but please use only git-upload-archive for the new stuff
(and not git-upload-zip etc.).

>>> +int parse_treeish_arg(const char **argv, struct tree **tree,
>>> +		      const unsigned char **commit_sha1,
>>> +		      time_t *archive_time, const char *prefix,
>>> +		      const char **reason)
>>> +{
>>> ...
>>> +	if (prefix) {
>>> +		unsigned char tree_sha1[20];
>>> +		unsigned int mode;
>>> +		int err;
>>> +
>>> +		err = get_tree_entry((*tree)->object.sha1, prefix,
>>> +				     tree_sha1, &mode);
>>> +		if (err || !S_ISDIR(mode)) {
>>> +			*reason = "current working directory is untracked";
>>> +			goto out;
>>> +		}
>>> +		free(*tree);
>>> +		*tree = parse_tree_indirect(tree_sha1);
>>> +	}
>> I like the simplicity of just optionally sending one subtree (or
>> the whole thing), but I think this part would be made more
>> efficient if we go with "struct tree_desc" based interface.
>>
>> Also I wonder how this interacts with the pathspec you take from
>> the command line.  Personally I think this single subtree
>> support is good enough and limiting with pathspec is not needed.

[Note: There's potential for confusion here because we have two
types of prefixes.  One is the present working directory inside the
git archive, the other is the one specified with --prefix=.  Here
we have the working directory kind of prefix.]

IMHO should work like in the following example, and the code above
cuts off the Documentation part:

   $ cd Documentation
   $ git-archive --format=tar --prefix=v1.0/ HEAD howto | tar tf -
   v1.0/howto/
   v1.0/howto/isolate-bugs-with-bisect.txt
   ...

I agree that simple subtree matching would be enough, at least for
now.

René