Re: Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.

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

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

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

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