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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 1, 2009, 04:27 UTC
Message-ID: <7v8weq50pm.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20091101.010702.527849118592864646._deepfire@feelingofgreen.ru>

Samium Gromoff <_deepfire@feelingofgreen.ru> writes:

> Attached is the SEGV bugreport I sent to debian.

Thanks for a report.

There are two issues.

 * transport-helper.c::get_helper() assumes that the transport structure
   always has its remote member filled and it can get name out of it.
   This is the segv in the report.

 * Even if we work around the above issue, the helper subprocess (in this
   case, remote-curl helper) insists on being inside a git repository,  To
   satisfy ls-remote request, you do not have to be in one.

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:

 - Should we fix get_helper() in transport-helper.c, instead of touching
   ls-remote.c like this patch does?

   This issue really boils down to this question: is it valid for a
   transport to have NULL in its remote field, and should all the code
   that touch transport be prepared to deal with such a transport
   structure?

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

 builtin-ls-remote.c |    2 +-
 remote-curl.c       |    3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

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