Daniel Barkalow <barkalow@xxxxxxxxxxxx> writes:
> On Sat, 31 Oct 2009, Junio C Hamano wrote:
> ...
>> Attached is a minimum fix/work around, but this is done without being very
>> familiar with the current assumptions in the codepaths involved.
>>
>> Issues I want area experts to consider before coming up with the final fix
>> are:
>> ...
> I think there's no benefit to allowing NULL for the remote; I think you
> can always get a struct remote for what you want to access. So it's
> probably just as well to require it, particularly because, as in the case
> of cmd_ls_remote() below, you'd need a special case to not get a struct
> remote.
>
> Is there any way in which the intended semantics of "transport_get(NULL,
> url)" is not the same as "transport_get(remote_get(url), url)"?
> (And, in the extended series, I make "transport_get(remote_get(url),
> NULL)" also mean the same thing as above, while "transport_get(NULL,
> NULL)" is obviously underspecified.)
That was really my question to people who are involved in the transport
layer code. I didn't know how transport->url and transport->remote->url
are intended to relate to each other, for example, and that was why you
were on Cc list. In other words, you are the area expert, you tell me ;-)
Sverre seemed to think slightly differently; perhaps having worked on the
foreign vcs interface he has some other input.
>> - When helping to handle ls-remote request, there is no need for the
>> helper to know anything about the local state. We probably shouldn't
>> even run setup_git_directory_gently() at all in this case. But when
>> helping other kinds of request, the helper does need to know where our
>> repository is.
>>
>> In general, what should the initial environment for helpers be? Should
>> they assume that they have to figure out where the git repository is
>> themselves (in other words, should they assume they cannot rely on
>> anything the caller does before they are called? Would the caller
>> generally have done the usual repo discovery (including chdir() to the
>> toplevel), and there are some set of assumptions they can make? If so
>> what are they?
>
> Probably, the helper should be run with a predicable initial environment,
> simply because operations that use remote repositories are most often run
> from the toplevel of a repo, so people will fail to notice their bugs
> which trigger on running from subdirectories....
> Perhaps we should actively tell the helper if there is no git repository
> (or, if any git repository we happen to be in is merely coincidental and
> shouldn't affect the helper)? Helpers involving importing will probably
> want to know they don't have a private refs namespace, private state
> directory, etc. even for implementing "list" for ls-remote, and it would
> probably be best to require helper authors to report that they've
> considered this possibility before trying to use it.
I think that is a sane approach.
>> diff --git a/builtin-ls-remote.c b/builtin-ls-remote.c
>> index 78a88f7..a8d5613 100644
>> --- a/builtin-ls-remote.c
>> +++ b/builtin-ls-remote.c
>> @@ -86,7 +86,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>> pattern[j - i] = p;
>> }
>> }
>> - remote = nongit ? NULL : remote_get(dest);
>> + remote = remote_get(dest);
>> if (remote && !remote->url_nr)
>> die("remote %s has no configured URL", dest);
>> transport = transport_get(remote, remote ? remote->url[0] : dest);
>
> You can also drop the two checks for remote being non-NULL here, since
> it's now always non-NULL...
You are probably right; I didn't even look when I did the above.
>> diff --git a/remote-curl.c b/remote-curl.c
>> index ad6a163..7c83f77 100644
>> --- a/remote-curl.c
>> +++ b/remote-curl.c
>> @@ -81,8 +81,9 @@ int main(int argc, const char **argv)
>> struct strbuf buf = STRBUF_INIT;
>> const char *url;
>> struct walker *walker = NULL;
>> + int nongit = 0;
>>
>> - setup_git_directory();
>> + setup_git_directory_gently(&nongit);
>> if (argc < 2) {
>> fprintf(stderr, "Remote needed\n");
>> return 1;
>
> Do things like git_path() fail cleanly if there was no git directory? If
> not, there should probably be tests of nongit on paths that actually need
> a git directory,...
I don't know. Again, you tell me ;-)
It probably makes sesne as you outlined in the earlier part of your
response for the caller to give a bit more clue to the helper to help
making such a decision.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html