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

> 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.

>>> +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.

>>> +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.