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

[Junio C Hamano <junkio@cox.net>]

Franck Bui-Huu <vagabon.xyz@gmail.com> writes:

>>> +typedef int (*parse_extra_args_fn_t)(int argc,
>>> +				     const char **argv,
>>> +				     const char **reason);
>>> +
>> 
>> I do not see a way for parse_extra to record the parameter it
>> successfully parsed, other than in a source-file-global, static
>> variable.  Not a very nice design for a library, if we are
>> building one from scratch.
>
> Interesting, could you explain why static variables are not nice ?

Mostly taste and a little bit of re-entrancy worries.

> You might have missed my second patch:
>
> 		"[PATCH 2/2] Add git-upload-archive"
>
> Basically the server can also use 'reason' to report a failure
> description during NACK. I find it more useful than the simple
> "server sent EOF" error message.

That's a good intention, but we would also need to convey the
"server side found problem and died with these error() output"
anyway, so it would be covered either way (see how error()/die()
messages from git-upload-pack are given to git-fetch-pack over
the wire).

> 'remote' case is not a generic argument that can be passed to
> archiver backends. Remember, the archiver backends only do local
> operation. They do not know about remote protocol which is part
> of git-archive command. That's the reason why I think we shouldn't
> make this field part of arguments structure.

Ok.  Passing that as a separate paramter would make sense.

>> After parse_archive_args finds the archiver specified with
>> --format=*, it can call its parse_extra to retrieve a suitable
>> struct that has struct archive_args embedded at the beginning,
>> and then set remote and prefix on the returned structure.
>
> One bad side is that we need to malloc this embedded structure.

Not at all, if you read the example I did you would notice that
I changed parse_extra for each backend to return this structure
allocated for that particular backend.

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

That would break "git-archive --remove=ssh://site/repo treeish"
wouldn't it?