From patchwork Fri Jun 14 10:25:25 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13698570 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 40926198A1B for ; Fri, 14 Jun 2024 10:25:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718360729; cv=none; b=OxRrQvK0F48fn6Yr2b/3/zJZrkgmGmv+lsceDdi0S3d3htDhG9fJQX1rmqSNwAWw4eIZm0pN3yxw8gG8zYS4Cq3+oG4pea8vJUk2r6Sc6F9tFw909zJS3ciThmakawVL/vLniXRt9VuRc1mnkcxS/cwpLKzz8K6qOZ+j3vr3CaA= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718360729; c=relaxed/simple; bh=WxqY6klkHDteK4kYJO4ehL4L/uKPjzoGnzFXrKKj6So=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=AvrJKnXEN70qsIdTeVfc6TqjHh4k8DgeAC0Euc2+YP0Gr2f9aHT35YjupH0psDEoTj0028UcHDbCShOfD3BPlY/7zCJ63P5i8m6XqhjtvK9u7WDp2887igHZK//Ok+oD5g8OdR/xv8rLsoFbjEM0MwQMZuUdHPLe+Xt9ymvHths= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 16458 invoked by uid 109); 14 Jun 2024 10:25:26 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Jun 2024 10:25:26 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 27084 invoked by uid 111); 14 Jun 2024 10:25:23 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Jun 2024 06:25:23 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Jun 2024 06:25:25 -0400 From: Jeff King To: Junio C Hamano Cc: Mathew George , git@vger.kernel.org Subject: [PATCH 01/11] archive: fix check for missing url Message-ID: <20240614102525.GA222445@coredump.intra.peff.net> References: <20240614102439.GA222287@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240614102439.GA222287@coredump.intra.peff.net> Running "git archive --remote" checks that we have at least one url for the remote. It does so by looking at remote.url[0], but that won't work; if we have no url at all, then remote.url will be NULL, and we'll segfault. Check url_nr instead, which is a more direct way of asking what we want. You can trigger the segfault like this: git -c remote.foo.vcs=bar archive --remote=foo but I didn't bother adding a test. This is the tip of the iceberg for no-url remotes, and a later patch will improve that situation. I just wanted to clean up this bug so it didn't make further refactoring of this code more confusing. Signed-off-by: Jeff King --- This code actually goes away in patch 10, but it's possible we'd want to take a different approach there. So I preferred to fix this up front anyway. builtin/archive.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/archive.c b/builtin/archive.c index 15ee1ec7bb..f35560042e 100644 --- a/builtin/archive.c +++ b/builtin/archive.c @@ -31,7 +31,7 @@ static int run_remote_archiver(int argc, const char **argv, struct packet_reader reader; _remote = remote_get(remote); - if (!_remote->url[0]) + if (!_remote->url_nr) die(_("git archive: Remote with no URL")); transport = transport_get(_remote, _remote->url[0]); transport_connect(transport, "git-upload-archive", exec, fd); From patchwork Fri Jun 14 10:26:16 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13698571 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B5C24195969 for ; Fri, 14 Jun 2024 10:26:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718360780; cv=none; b=d3cunU+RFxjSobnXJSvopdhjCNf2V0OHA/FRpH7BNZKutAIFrVkJiCVgQhTvQ16OyIowt+VR0+B553esZvBNJPyRvr3ifW6hKL3EHAKw2efGGLYDHdmeWZkFckg2ubhyb5n4i3y0ULafXXkZYNsBKELnsiGrKCayIfgVGnJLnUI= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718360780; c=relaxed/simple; bh=u8n72KsHQdIMm4ikNwrcpbM97zk6qjYPyo6lBYCF5qc=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=RmhB75D9xq5OrQQrUxtdNBVNSe5pVNEXVx5a0vQ7wFBBdmLQ0DfvN+PfrXN+vGDBK14uT0FFnN2c9BCnhEioAM+QofufYXfVYZoDZ/GeRomcQP4OuF+VUOMZIDN8Q2nCx1fge/Q+RLQ7al4dn2XNZK9Y5mzufnEPlMyGEMzQOyk= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 16478 invoked by uid 109); 14 Jun 2024 10:26:17 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Jun 2024 10:26:17 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 27094 invoked by uid 111); 14 Jun 2024 10:26:15 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Jun 2024 06:26:15 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Jun 2024 06:26:16 -0400 From: Jeff King To: Junio C Hamano Cc: Mathew George , git@vger.kernel.org Subject: [PATCH 02/11] remote: refactor alias_url() memory ownership Message-ID: <20240614102616.GB222445@coredump.intra.peff.net> References: <20240614102439.GA222287@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240614102439.GA222287@coredump.intra.peff.net> The alias_url() function may return either a newly allocated string (which the caller must take ownership of), or the original const "url" parameter that was passed in. This often works OK because callers are generally passing in a "url" that they expect to retain ownership of anyway. So whether we got back the original or a new string, we're always interested in storing it forever. But I suspect there are some possible leaks here (e.g., add_url_alias() may end up discarding the original "url"). Whether there are active leaks or not, this is a confusing setup that makes further refactoring of memory ownership harder. So instead of returning the original string, return NULL, forcing callers to decide what to do with it explicitly. We can then build further cleanups on top of that. Signed-off-by: Jeff King --- Just to be clear, I think there's probably still a leak in add_url_alias() even after this patch. It's just a step on the way to fixing. remote.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/remote.c b/remote.c index dcb5492c85..fd9d58f820 100644 --- a/remote.c +++ b/remote.c @@ -35,7 +35,7 @@ static int valid_remote(const struct remote *remote) return (!!remote->url) || (!!remote->foreign_vcs); } -static const char *alias_url(const char *url, struct rewrites *r) +static char *alias_url(const char *url, struct rewrites *r) { int i, j; struct counted_string *longest; @@ -56,7 +56,7 @@ static const char *alias_url(const char *url, struct rewrites *r) } } if (!longest) - return url; + return NULL; return xstrfmt("%s%s", r->rewrite[longest_i]->base, url + longest->len); } @@ -76,15 +76,16 @@ static void add_pushurl(struct remote *remote, const char *pushurl) static void add_pushurl_alias(struct remote_state *remote_state, struct remote *remote, const char *url) { - const char *pushurl = alias_url(url, &remote_state->rewrites_push); - if (pushurl != url) - add_pushurl(remote, pushurl); + char *alias = alias_url(url, &remote_state->rewrites_push); + if (alias) + add_pushurl(remote, alias); } static void add_url_alias(struct remote_state *remote_state, struct remote *remote, const char *url) { - add_url(remote, alias_url(url, &remote_state->rewrites)); + char *alias = alias_url(url, &remote_state->rewrites); + add_url(remote, alias ? alias : url); add_pushurl_alias(remote_state, remote, url); } @@ -492,19 +493,22 @@ static void alias_all_urls(struct remote_state *remote_state) if (!remote_state->remotes[i]) continue; for (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) { - remote_state->remotes[i]->pushurl[j] = - alias_url(remote_state->remotes[i]->pushurl[j], - &remote_state->rewrites); + char *alias = alias_url(remote_state->remotes[i]->pushurl[j], + &remote_state->rewrites); + if (alias) + remote_state->remotes[i]->pushurl[j] = alias; } add_pushurl_aliases = remote_state->remotes[i]->pushurl_nr == 0; for (j = 0; j < remote_state->remotes[i]->url_nr; j++) { + char *alias; if (add_pushurl_aliases) add_pushurl_alias( remote_state, remote_state->remotes[i], remote_state->remotes[i]->url[j]); - remote_state->remotes[i]->url[j] = - alias_url(remote_state->remotes[i]->url[j], + alias = alias_url(remote_state->remotes[i]->url[j], &remote_state->rewrites); + if (alias) + remote_state->remotes[i]->url[j] = alias; } } } From patchwork Fri Jun 14 10:27:22 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13698591 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E258313B5AC for ; Fri, 14 Jun 2024 10:27:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718360845; cv=none; b=plJ88mzmVdYfUThvOi26UWDlR2DPRRcgSiE1RYv6UGE2vzsjqoUsb9DXhE6t279uBWnkg5NpK2GE2I7lwePGSIrM8ZNNnPqWISF3w6aBtCnxww7dycIR080mFc86FxLU12CFRs9cPWn9JhNyN4TuT+oIeTRug2DllJOxEvgiENc= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718360845; c=relaxed/simple; bh=B5ApuymeAWg2YTrRRY/jHotPLBdCqsfcrPTjh4+btcg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=EojJrUhR/QycJEmDiBA0Wk3xWnr9g4RLGPOCFhMMbc2WS/6BjTm3H2qza5fV2daFypyICU5iFXH/Ha3UgH6H+eeEVqnuTfpd3tGN9fepJ3FeNME6kV8hZXJEeZ54MnXq2aO/mhM2TGngPUg1s7rtlXYEmBmwlk9Tl0m1N0NQuno= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 16500 invoked by uid 109); 14 Jun 2024 10:27:23 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Jun 2024 10:27:23 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 27103 invoked by uid 111); 14 Jun 2024 10:27:20 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Jun 2024 06:27:20 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Jun 2024 06:27:22 -0400 From: Jeff King To: Junio C Hamano Cc: Mathew George , git@vger.kernel.org Subject: [PATCH 03/11] remote: transfer ownership of memory in add_url(), etc Message-ID: <20240614102722.GC222445@coredump.intra.peff.net> References: <20240614102439.GA222287@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240614102439.GA222287@coredump.intra.peff.net> Many of the internal functions in remote.c take const strings and store them forever in instances of "struct remote". Since the functions are internal and callers are aware of the convention, this seems to mostly work and not cause leaks. But there are some issues: - it's impossible to clear any of the arrays, because the data dependencies between them are too muddled (if you free() a string, it might also be referenced from another array, causing a user-after-free; but if you don't, that might be the last reference, causing a leak). This is mostly of interest for further refactoring and features, but there's at least one spot that's already a problem. In alias_all_urls(), we replace elements of remote->url and remote->pushurl with their aliased forms, dropping references to the original. - sometimes strings from outside callers make their way in. For example, calling remote_get("foo") when there is no configured "foo" remote will create a remote struct with the single url "foo". But we'll do so by holding on to the string passed to remote_get() forever. In practice I think this works out because we'd usually pass in a string that lasts the length of the program (a string literal, or argv reference, or other data structure allocated in the main function). But it's a rather subtle requirement. Instead, let's have remote->url and remote->pushurl own their string memory. They'll copy the const strings that are passed in, and callers can stop making their own copies. Likewise, when we overwrite an entry, we can free the memory it points to, fixing the leak mentioned above. We'll leave the struct members as "const" since they are visible to the outside world, and shouldn't usually be touched. This requires casting on free() for now, but we'll clean that further in a future patch. Signed-off-by: Jeff King --- Since now we'll always allocate, I don't think it's possible for this to introduce any memory corruption issues. It _might_ introduce a leak, but I think I fixed all of the callers. remote.c | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/remote.c b/remote.c index fd9d58f820..f7c846865f 100644 --- a/remote.c +++ b/remote.c @@ -64,13 +64,13 @@ static char *alias_url(const char *url, struct rewrites *r) static void add_url(struct remote *remote, const char *url) { ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc); - remote->url[remote->url_nr++] = url; + remote->url[remote->url_nr++] = xstrdup(url); } static void add_pushurl(struct remote *remote, const char *pushurl) { ALLOC_GROW(remote->pushurl, remote->pushurl_nr + 1, remote->pushurl_alloc); - remote->pushurl[remote->pushurl_nr++] = pushurl; + remote->pushurl[remote->pushurl_nr++] = xstrdup(pushurl); } static void add_pushurl_alias(struct remote_state *remote_state, @@ -79,6 +79,7 @@ static void add_pushurl_alias(struct remote_state *remote_state, char *alias = alias_url(url, &remote_state->rewrites_push); if (alias) add_pushurl(remote, alias); + free(alias); } static void add_url_alias(struct remote_state *remote_state, @@ -87,6 +88,7 @@ static void add_url_alias(struct remote_state *remote_state, char *alias = alias_url(url, &remote_state->rewrites); add_url(remote, alias ? alias : url); add_pushurl_alias(remote_state, remote, url); + free(alias); } struct remotes_hash_key { @@ -293,7 +295,7 @@ static void read_remotes_file(struct remote_state *remote_state, if (skip_prefix(buf.buf, "URL:", &v)) add_url_alias(remote_state, remote, - xstrdup(skip_spaces(v))); + skip_spaces(v)); else if (skip_prefix(buf.buf, "Push:", &v)) refspec_append(&remote->push, skip_spaces(v)); else if (skip_prefix(buf.buf, "Pull:", &v)) @@ -336,7 +338,7 @@ static void read_branches_file(struct remote_state *remote_state, else frag = to_free = repo_default_branch_name(the_repository, 0); - add_url_alias(remote_state, remote, strbuf_detach(&buf, NULL)); + add_url_alias(remote_state, remote, buf.buf); refspec_appendf(&remote->fetch, "refs/heads/%s:refs/heads/%s", frag, remote->name); @@ -347,6 +349,7 @@ static void read_branches_file(struct remote_state *remote_state, refspec_appendf(&remote->push, "HEAD:refs/heads/%s", frag); remote->fetch_tags = 1; /* always auto-follow */ + strbuf_release(&buf); free(to_free); } @@ -431,15 +434,13 @@ static int handle_config(const char *key, const char *value, else if (!strcmp(subkey, "prunetags")) remote->prune_tags = git_config_bool(key, value); else if (!strcmp(subkey, "url")) { - char *v; - if (git_config_string(&v, key, value)) - return -1; - add_url(remote, v); + if (!value) + return config_error_nonbool(key); + add_url(remote, value); } else if (!strcmp(subkey, "pushurl")) { - char *v; - if (git_config_string(&v, key, value)) - return -1; - add_pushurl(remote, v); + if (!value) + return config_error_nonbool(key); + add_pushurl(remote, value); } else if (!strcmp(subkey, "push")) { char *v; if (git_config_string(&v, key, value)) @@ -495,8 +496,10 @@ static void alias_all_urls(struct remote_state *remote_state) for (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) { char *alias = alias_url(remote_state->remotes[i]->pushurl[j], &remote_state->rewrites); - if (alias) + if (alias) { + free((char *)remote_state->remotes[i]->pushurl[j]); remote_state->remotes[i]->pushurl[j] = alias; + } } add_pushurl_aliases = remote_state->remotes[i]->pushurl_nr == 0; for (j = 0; j < remote_state->remotes[i]->url_nr; j++) { @@ -507,8 +510,10 @@ static void alias_all_urls(struct remote_state *remote_state) remote_state->remotes[i]->url[j]); alias = alias_url(remote_state->remotes[i]->url[j], &remote_state->rewrites); - if (alias) + if (alias) { + free((char *)remote_state->remotes[i]->url[j]); remote_state->remotes[i]->url[j] = alias; + } } } } From patchwork Fri Jun 14 10:28:01 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13698592 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 22CF91922DF for ; Fri, 14 Jun 2024 10:28:02 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718360885; cv=none; b=DZSFrYMmUXlXGTjiLZ6r2i83xh6FXGTEa/zAIcanobnEIMJYURyWY/a90XfpCQfxkYJSug797PWb1wuNAk7cNMb5OyA/NDA3pv5eaNIjNhQLAEXTLAcKdXh86EfgqHSiPWXcUHXyr6wWW5GCI6Bork4pAkxBWTSyRLVwOujpyhE= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718360885; c=relaxed/simple; bh=Ur/39kT1TUKer8VHmcRQ/UfzdpU4FZxvtPscDFFGCDM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=PS3zDV9PY4qMNbzrSbIDeccMqkyp1Lu1vLpkG4VEf6zmPc9Dlx0Po4+uqCHclhQRVBaQpMaHmolSLqcFGJl2H1H/LkJOkV7mT4Qniq93ss5+hs5kLCfegMDRkfbnPcf2qdh6HaovK51BRi1QpmnRETSbD9ar6rBUh0ClyY8plvs= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 16513 invoked by uid 109); 14 Jun 2024 10:28:02 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Jun 2024 10:28:02 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 27108 invoked by uid 111); 14 Jun 2024 10:27:59 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Jun 2024 06:27:59 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Jun 2024 06:28:01 -0400 From: Jeff King To: Junio C Hamano Cc: Mathew George , git@vger.kernel.org Subject: [PATCH 04/11] remote: use strvecs to store remote url/pushurl Message-ID: <20240614102801.GD222445@coredump.intra.peff.net> References: <20240614102439.GA222287@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240614102439.GA222287@coredump.intra.peff.net> Now that the url/pushurl fields of "struct remote" own their strings, we can switch from bare arrays to strvecs. This has a few advantages: - push/clear are now one-liners - likewise the free+assigns in alias_all_urls() can use strvec_replace() - we now use size_t for storage, avoiding possible overflow - this will enable some further cleanups in future patches There's quite a bit of fallout in the code that reads these fields, as it tends to access these arrays directly. But it's mostly a mechanical replacement of "url_nr" with "url.nr", and "url[i]" with "url.v[i]", with a few variations (e.g. "*url" could become "*url.v", but I used "url.v[0]" for consistency). Signed-off-by: Jeff King --- builtin/archive.c | 4 +-- builtin/clone.c | 4 +-- builtin/ls-remote.c | 6 ++-- builtin/push.c | 10 +++---- builtin/remote.c | 56 +++++++++++++++++++------------------- remote-curl.c | 2 +- remote.c | 52 ++++++++++++++--------------------- remote.h | 12 ++------ t/helper/test-bundle-uri.c | 2 +- transport.c | 4 +-- 10 files changed, 68 insertions(+), 84 deletions(-) diff --git a/builtin/archive.c b/builtin/archive.c index f35560042e..0d9aff7e6f 100644 --- a/builtin/archive.c +++ b/builtin/archive.c @@ -31,9 +31,9 @@ static int run_remote_archiver(int argc, const char **argv, struct packet_reader reader; _remote = remote_get(remote); - if (!_remote->url_nr) + if (!_remote->url.nr) die(_("git archive: Remote with no URL")); - transport = transport_get(_remote, _remote->url[0]); + transport = transport_get(_remote, _remote->url.v[0]); transport_connect(transport, "git-upload-archive", exec, fd); /* diff --git a/builtin/clone.c b/builtin/clone.c index 730b3efae6..d3b70b49b0 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1290,7 +1290,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) refspec_appendf(&remote->fetch, "+%s*:%s*", src_ref_prefix, branch_top.buf); - path = get_repo_path(remote->url[0], &is_bundle); + path = get_repo_path(remote->url.v[0], &is_bundle); is_local = option_local != 0 && path && !is_bundle; if (is_local) { if (option_depth) @@ -1312,7 +1312,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) if (option_local > 0 && !is_local) warning(_("--local is ignored")); - transport = transport_get(remote, path ? path : remote->url[0]); + transport = transport_get(remote, path ? path : remote->url.v[0]); transport_set_verbosity(transport, option_verbosity, option_progress); transport->family = family; transport->cloning = 1; diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c index e8d65ebbdc..4c04dbbf19 100644 --- a/builtin/ls-remote.c +++ b/builtin/ls-remote.c @@ -109,11 +109,11 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) die("bad repository '%s'", dest); die("No remote configured to list refs from."); } - if (!remote->url_nr) + if (!remote->url.nr) die("remote %s has no configured URL", dest); if (get_url) { - printf("%s\n", *remote->url); + printf("%s\n", remote->url.v[0]); return 0; } @@ -130,7 +130,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) } if (!dest && !quiet) - fprintf(stderr, "From %s\n", *remote->url); + fprintf(stderr, "From %s\n", remote->url.v[0]); for ( ; ref; ref = ref->next) { struct ref_array_item *item; if (!check_ref_type(ref, flags)) diff --git a/builtin/push.c b/builtin/push.c index 2fbb31c3ad..61b5d3afdd 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -143,12 +143,12 @@ static void set_refspecs(const char **refs, int nr, const char *repo) static int push_url_of_remote(struct remote *remote, const char ***url_p) { - if (remote->pushurl_nr) { - *url_p = remote->pushurl; - return remote->pushurl_nr; + if (remote->pushurl.nr) { + *url_p = remote->pushurl.v; + return remote->pushurl.nr; } - *url_p = remote->url; - return remote->url_nr; + *url_p = remote->url.v; + return remote->url.nr; } static NORETURN void die_push_simple(struct branch *branch, diff --git a/builtin/remote.c b/builtin/remote.c index 447ef1d3c9..ee6a33ff11 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -619,8 +619,8 @@ static int migrate_file(struct remote *remote) int i; strbuf_addf(&buf, "remote.%s.url", remote->name); - for (i = 0; i < remote->url_nr; i++) - git_config_set_multivar(buf.buf, remote->url[i], "^$", 0); + for (i = 0; i < remote->url.nr; i++) + git_config_set_multivar(buf.buf, remote->url.v[i], "^$", 0); strbuf_reset(&buf); strbuf_addf(&buf, "remote.%s.push", remote->name); for (i = 0; i < remote->push.raw_nr; i++) @@ -1002,8 +1002,8 @@ static int get_remote_ref_states(const char *name, struct transport *transport; const struct ref *remote_refs; - transport = transport_get(states->remote, states->remote->url_nr > 0 ? - states->remote->url[0] : NULL); + transport = transport_get(states->remote, states->remote->url.nr > 0 ? + states->remote->url.v[0] : NULL); remote_refs = transport_get_remote_refs(transport, NULL); states->queried = 1; @@ -1216,12 +1216,12 @@ static int get_one_entry(struct remote *remote, void *priv) const char **url; int i, url_nr; - if (remote->url_nr > 0) { + if (remote->url.nr > 0) { struct strbuf promisor_config = STRBUF_INIT; const char *partial_clone_filter = NULL; strbuf_addf(&promisor_config, "remote.%s.partialclonefilter", remote->name); - strbuf_addf(&remote_info_buf, "%s (fetch)", remote->url[0]); + strbuf_addf(&remote_info_buf, "%s (fetch)", remote->url.v[0]); if (!git_config_get_string_tmp(promisor_config.buf, &partial_clone_filter)) strbuf_addf(&remote_info_buf, " [%s]", partial_clone_filter); @@ -1230,12 +1230,12 @@ static int get_one_entry(struct remote *remote, void *priv) strbuf_detach(&remote_info_buf, NULL); } else string_list_append(list, remote->name)->util = NULL; - if (remote->pushurl_nr) { - url = remote->pushurl; - url_nr = remote->pushurl_nr; + if (remote->pushurl.nr) { + url = remote->pushurl.v; + url_nr = remote->pushurl.nr; } else { - url = remote->url; - url_nr = remote->url_nr; + url = remote->url.v; + url_nr = remote->url.nr; } for (i = 0; i < url_nr; i++) { @@ -1301,14 +1301,14 @@ static int show(int argc, const char **argv, const char *prefix) get_remote_ref_states(*argv, &info.states, query_flag); printf_ln(_("* remote %s"), *argv); - printf_ln(_(" Fetch URL: %s"), info.states.remote->url_nr > 0 ? - info.states.remote->url[0] : _("(no URL)")); - if (info.states.remote->pushurl_nr) { - url = info.states.remote->pushurl; - url_nr = info.states.remote->pushurl_nr; + printf_ln(_(" Fetch URL: %s"), info.states.remote->url.nr > 0 ? + info.states.remote->url.v[0] : _("(no URL)")); + if (info.states.remote->pushurl.nr) { + url = info.states.remote->pushurl.v; + url_nr = info.states.remote->pushurl.nr; } else { - url = info.states.remote->url; - url_nr = info.states.remote->url_nr; + url = info.states.remote->url.v; + url_nr = info.states.remote->url.nr; } for (i = 0; i < url_nr; i++) /* @@ -1454,8 +1454,8 @@ static int prune_remote(const char *remote, int dry_run) printf_ln(_("Pruning %s"), remote); printf_ln(_("URL: %s"), - states.remote->url_nr - ? states.remote->url[0] + states.remote->url.nr + ? states.remote->url.v[0] : _("(no URL)")); for_each_string_list_item(item, &states.stale) @@ -1647,15 +1647,15 @@ static int get_url(int argc, const char **argv, const char *prefix) url_nr = 0; if (push_mode) { - url = remote->pushurl; - url_nr = remote->pushurl_nr; + url = remote->pushurl.v; + url_nr = remote->pushurl.nr; } /* else fetch mode */ /* Use the fetch URL when no push URLs were found or requested. */ if (!url_nr) { - url = remote->url; - url_nr = remote->url_nr; + url = remote->url.v; + url_nr = remote->url.nr; } if (!url_nr) @@ -1718,12 +1718,12 @@ static int set_url(int argc, const char **argv, const char *prefix) if (push_mode) { strbuf_addf(&name_buf, "remote.%s.pushurl", remotename); - urlset = remote->pushurl; - urlset_nr = remote->pushurl_nr; + urlset = remote->pushurl.v; + urlset_nr = remote->pushurl.nr; } else { strbuf_addf(&name_buf, "remote.%s.url", remotename); - urlset = remote->url; - urlset_nr = remote->url_nr; + urlset = remote->url.v; + urlset_nr = remote->url.nr; } /* Special cases that add new entry. */ diff --git a/remote-curl.c b/remote-curl.c index 6008d7e87c..22957c16db 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -1574,7 +1574,7 @@ int cmd_main(int argc, const char **argv) if (argc > 2) { end_url_with_slash(&url, argv[2]); } else { - end_url_with_slash(&url, remote->url[0]); + end_url_with_slash(&url, remote->url.v[0]); } http_init(remote, url.buf, 0); diff --git a/remote.c b/remote.c index f7c846865f..76a3e41c73 100644 --- a/remote.c +++ b/remote.c @@ -32,7 +32,7 @@ struct counted_string { static int valid_remote(const struct remote *remote) { - return (!!remote->url) || (!!remote->foreign_vcs); + return (!!remote->url.nr) || (!!remote->foreign_vcs); } static char *alias_url(const char *url, struct rewrites *r) @@ -63,14 +63,12 @@ static char *alias_url(const char *url, struct rewrites *r) static void add_url(struct remote *remote, const char *url) { - ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc); - remote->url[remote->url_nr++] = xstrdup(url); + strvec_push(&remote->url, url); } static void add_pushurl(struct remote *remote, const char *pushurl) { - ALLOC_GROW(remote->pushurl, remote->pushurl_nr + 1, remote->pushurl_alloc); - remote->pushurl[remote->pushurl_nr++] = xstrdup(pushurl); + strvec_push(&remote->pushurl, pushurl); } static void add_pushurl_alias(struct remote_state *remote_state, @@ -150,18 +148,12 @@ static struct remote *make_remote(struct remote_state *remote_state, static void remote_clear(struct remote *remote) { - int i; - free((char *)remote->name); free((char *)remote->foreign_vcs); - for (i = 0; i < remote->url_nr; i++) - free((char *)remote->url[i]); - FREE_AND_NULL(remote->url); + strvec_clear(&remote->url); + strvec_clear(&remote->pushurl); - for (i = 0; i < remote->pushurl_nr; i++) - free((char *)remote->pushurl[i]); - FREE_AND_NULL(remote->pushurl); free((char *)remote->receivepack); free((char *)remote->uploadpack); FREE_AND_NULL(remote->http_proxy); @@ -493,27 +485,25 @@ static void alias_all_urls(struct remote_state *remote_state) int add_pushurl_aliases; if (!remote_state->remotes[i]) continue; - for (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) { - char *alias = alias_url(remote_state->remotes[i]->pushurl[j], + for (j = 0; j < remote_state->remotes[i]->pushurl.nr; j++) { + char *alias = alias_url(remote_state->remotes[i]->pushurl.v[j], &remote_state->rewrites); - if (alias) { - free((char *)remote_state->remotes[i]->pushurl[j]); - remote_state->remotes[i]->pushurl[j] = alias; - } + if (alias) + strvec_replace(&remote_state->remotes[i]->pushurl, + j, alias); } - add_pushurl_aliases = remote_state->remotes[i]->pushurl_nr == 0; - for (j = 0; j < remote_state->remotes[i]->url_nr; j++) { + add_pushurl_aliases = remote_state->remotes[i]->pushurl.nr == 0; + for (j = 0; j < remote_state->remotes[i]->url.nr; j++) { char *alias; if (add_pushurl_aliases) add_pushurl_alias( remote_state, remote_state->remotes[i], - remote_state->remotes[i]->url[j]); - alias = alias_url(remote_state->remotes[i]->url[j], + remote_state->remotes[i]->url.v[j]); + alias = alias_url(remote_state->remotes[i]->url.v[j], &remote_state->rewrites); - if (alias) { - free((char *)remote_state->remotes[i]->url[j]); - remote_state->remotes[i]->url[j] = alias; - } + if (alias) + strvec_replace(&remote_state->remotes[i]->url, + j, alias); } } } @@ -653,10 +643,10 @@ static void validate_remote_url(struct remote *remote) else die(_("unrecognized value transfer.credentialsInUrl: '%s'"), value); - for (i = 0; i < remote->url_nr; i++) { + for (i = 0; i < remote->url.nr; i++) { struct url_info url_info = { 0 }; - if (!url_normalize(remote->url[i], &url_info) || + if (!url_normalize(remote->url.v[i], &url_info) || !url_info.passwd_off) goto loop_cleanup; @@ -830,8 +820,8 @@ struct ref *ref_remove_duplicates(struct ref *ref_map) int remote_has_url(struct remote *remote, const char *url) { int i; - for (i = 0; i < remote->url_nr; i++) { - if (!strcmp(remote->url[i], url)) + for (i = 0; i < remote->url.nr; i++) { + if (!strcmp(remote->url.v[i], url)) return 1; } return 0; diff --git a/remote.h b/remote.h index e8c6655e42..84dc91cca0 100644 --- a/remote.h +++ b/remote.h @@ -4,6 +4,7 @@ #include "hash-ll.h" #include "hashmap.h" #include "refspec.h" +#include "strvec.h" struct option; struct transport_ls_refs_options; @@ -68,16 +69,9 @@ struct remote { char *foreign_vcs; /* An array of all of the url_nr URLs configured for the remote */ - const char **url; - - int url_nr; - int url_alloc; - + struct strvec url; /* An array of all of the pushurl_nr push URLs configured for the remote */ - const char **pushurl; - - int pushurl_nr; - int pushurl_alloc; + struct strvec pushurl; struct refspec push; diff --git a/t/helper/test-bundle-uri.c b/t/helper/test-bundle-uri.c index 09dc78733c..3285dd962e 100644 --- a/t/helper/test-bundle-uri.c +++ b/t/helper/test-bundle-uri.c @@ -88,7 +88,7 @@ static int cmd_ls_remote(int argc, const char **argv) die(_("bad repository '%s'"), dest); die(_("no remote configured to get bundle URIs from")); } - if (!remote->url_nr) + if (!remote->url.nr) die(_("remote '%s' has no configured URL"), dest); transport = transport_get(remote, NULL); diff --git a/transport.c b/transport.c index 83ddea8fbc..eba92eb7e0 100644 --- a/transport.c +++ b/transport.c @@ -1127,8 +1127,8 @@ struct transport *transport_get(struct remote *remote, const char *url) ret->remote = remote; helper = remote->foreign_vcs; - if (!url && remote->url) - url = remote->url[0]; + if (!url && remote->url.nr) + url = remote->url.v[0]; ret->url = url; /* maybe it is a foreign URL? */ From patchwork Fri Jun 14 10:29:09 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13698593 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9BE1819414D for ; Fri, 14 Jun 2024 10:29:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718360953; cv=none; b=DxeapnlyR6B9j+LLZloFPmhCKOvwrthgu0uow2kwnn59Tl7E1Kqy23vCyPXKBE0E5wR+ULPWcottHWOZwBhTZNshNiA4fkJXoF0wY0mx3nj94apOZZ9DM5eQPGVzJHuM1KAGQW5u9AvqRe7yZSQeYqOkleeSSEZWgPfdIFLtHHE= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718360953; c=relaxed/simple; bh=z41xdbzz69RRqt219UK4TBopdnj0ry7B/Fd2ZGO9sM4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=P0/w7hNOyvdvzpA6Ejt3H1BuUcKi2FY4mUedV5/RmSK+6ZfJWAQuOZW9+SszjHpcaQ3J3fZHiCPx4J1olQkpgnYx2rs/ZmU/p9l1e/9L9JYlkJV0xDvYqMnaltvQkf942YPCgf4+mxL22IN+6/6GZwm2M+QyPMxNnjxz8uTidV4= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 16538 invoked by uid 109); 14 Jun 2024 10:29:10 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Jun 2024 10:29:10 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 27114 invoked by uid 111); 14 Jun 2024 10:29:07 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Jun 2024 06:29:07 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Jun 2024 06:29:09 -0400 From: Jeff King To: Junio C Hamano Cc: Mathew George , git@vger.kernel.org Subject: [PATCH 05/11] remote: simplify url/pushurl selection Message-ID: <20240614102909.GE222445@coredump.intra.peff.net> References: <20240614102439.GA222287@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240614102439.GA222287@coredump.intra.peff.net> When we want to know the push urls for a remote, there is some simple logic: - if the user configured any remote.*.pushurl keys, then those make the complete set of push urls - otherwise we push to all urls in remote.*.url Many spots implement this with a level of indirection, assigning to a local url/url_nr pair. But since both arrays are now strvecs, we can just use a pointer to select the appropriate strvec, shortening the code a bit. Even though this is now a one-liner, since it is application logic that is present in so many places, it's worth abstracting a helper function. In fact, we already have such a function, but it's local to builtin/push.c. So we'll just make it available everywhere via remote.h. There are two spots to pay special attention to here: 1. in builtin/remote.c's get_url(), we are selecting first based on push_mode and then falling back to "url" when we're in push_mode but no pushurl is defined. The updated code makes that much more clear, compared to the original which had an "else" fall-through. 2. likewise in that file's set_url(), we _only_ respect push_mode, sine the point is that we are adding to pushurl in that case (whether it is empty or not). And thus it does not use our helper function. Signed-off-by: Jeff King --- builtin/push.c | 21 ++++----------- builtin/remote.c | 69 ++++++++++++++---------------------------------- remote.c | 5 ++++ remote.h | 1 + 4 files changed, 31 insertions(+), 65 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index 61b5d3afdd..00d99af1a8 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -141,16 +141,6 @@ static void set_refspecs(const char **refs, int nr, const char *repo) free_refs(local_refs); } -static int push_url_of_remote(struct remote *remote, const char ***url_p) -{ - if (remote->pushurl.nr) { - *url_p = remote->pushurl.v; - return remote->pushurl.nr; - } - *url_p = remote->url.v; - return remote->url.nr; -} - static NORETURN void die_push_simple(struct branch *branch, struct remote *remote) { @@ -434,8 +424,7 @@ static int do_push(int flags, struct remote *remote) { int i, errs; - const char **url; - int url_nr; + struct strvec *url; struct refspec *push_refspec = &rs; if (push_options->nr) @@ -448,11 +437,11 @@ static int do_push(int flags, setup_default_push_refspecs(&flags, remote); } errs = 0; - url_nr = push_url_of_remote(remote, &url); - if (url_nr) { - for (i = 0; i < url_nr; i++) { + url = push_url_of_remote(remote); + if (url->nr) { + for (i = 0; i < url->nr; i++) { struct transport *transport = - transport_get(remote, url[i]); + transport_get(remote, url->v[i]); if (flags & TRANSPORT_PUSH_OPTIONS) transport->push_options = push_options; if (push_with_options(transport, push_refspec, flags)) diff --git a/builtin/remote.c b/builtin/remote.c index ee6a33ff11..06c09ed060 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1213,8 +1213,8 @@ static int get_one_entry(struct remote *remote, void *priv) { struct string_list *list = priv; struct strbuf remote_info_buf = STRBUF_INIT; - const char **url; - int i, url_nr; + struct strvec *url; + int i; if (remote->url.nr > 0) { struct strbuf promisor_config = STRBUF_INIT; @@ -1230,16 +1230,10 @@ static int get_one_entry(struct remote *remote, void *priv) strbuf_detach(&remote_info_buf, NULL); } else string_list_append(list, remote->name)->util = NULL; - if (remote->pushurl.nr) { - url = remote->pushurl.v; - url_nr = remote->pushurl.nr; - } else { - url = remote->url.v; - url_nr = remote->url.nr; - } - for (i = 0; i < url_nr; i++) + url = push_url_of_remote(remote); + for (i = 0; i < url->nr; i++) { - strbuf_addf(&remote_info_buf, "%s (push)", url[i]); + strbuf_addf(&remote_info_buf, "%s (push)", url->v[i]); string_list_append(list, remote->name)->util = strbuf_detach(&remote_info_buf, NULL); } @@ -1295,28 +1289,21 @@ static int show(int argc, const char **argv, const char *prefix) for (; argc; argc--, argv++) { int i; - const char **url; - int url_nr; + struct strvec *url; get_remote_ref_states(*argv, &info.states, query_flag); printf_ln(_("* remote %s"), *argv); printf_ln(_(" Fetch URL: %s"), info.states.remote->url.nr > 0 ? info.states.remote->url.v[0] : _("(no URL)")); - if (info.states.remote->pushurl.nr) { - url = info.states.remote->pushurl.v; - url_nr = info.states.remote->pushurl.nr; - } else { - url = info.states.remote->url.v; - url_nr = info.states.remote->url.nr; - } - for (i = 0; i < url_nr; i++) + url = push_url_of_remote(info.states.remote); + for (i = 0; i < url->nr; i++) /* * TRANSLATORS: the colon ':' should align * with the one in " Fetch URL: %s" * translation. */ - printf_ln(_(" Push URL: %s"), url[i]); + printf_ln(_(" Push URL: %s"), url->v[i]); if (!i) printf_ln(_(" Push URL: %s"), _("(no URL)")); if (no_query) @@ -1622,8 +1609,7 @@ static int get_url(int argc, const char **argv, const char *prefix) int i, push_mode = 0, all_mode = 0; const char *remotename = NULL; struct remote *remote; - const char **url; - int url_nr; + struct strvec *url; struct option options[] = { OPT_BOOL('\0', "push", &push_mode, N_("query push URLs rather than fetch URLs")), @@ -1645,27 +1631,15 @@ static int get_url(int argc, const char **argv, const char *prefix) exit(2); } - url_nr = 0; - if (push_mode) { - url = remote->pushurl.v; - url_nr = remote->pushurl.nr; - } - /* else fetch mode */ - - /* Use the fetch URL when no push URLs were found or requested. */ - if (!url_nr) { - url = remote->url.v; - url_nr = remote->url.nr; - } - - if (!url_nr) + url = push_mode ? push_url_of_remote(remote) : &remote->url; + if (!url->nr) die(_("no URLs configured for remote '%s'"), remotename); if (all_mode) { - for (i = 0; i < url_nr; i++) - printf_ln("%s", url[i]); + for (i = 0; i < url->nr; i++) + printf_ln("%s", url->v[i]); } else { - printf_ln("%s", *url); + printf_ln("%s", url->v[0]); } return 0; @@ -1680,8 +1654,7 @@ static int set_url(int argc, const char **argv, const char *prefix) const char *oldurl = NULL; struct remote *remote; regex_t old_regex; - const char **urlset; - int urlset_nr; + struct strvec *urlset; struct strbuf name_buf = STRBUF_INIT; struct option options[] = { OPT_BOOL('\0', "push", &push_mode, @@ -1718,12 +1691,10 @@ static int set_url(int argc, const char **argv, const char *prefix) if (push_mode) { strbuf_addf(&name_buf, "remote.%s.pushurl", remotename); - urlset = remote->pushurl.v; - urlset_nr = remote->pushurl.nr; + urlset = &remote->pushurl; } else { strbuf_addf(&name_buf, "remote.%s.url", remotename); - urlset = remote->url.v; - urlset_nr = remote->url.nr; + urlset = &remote->url; } /* Special cases that add new entry. */ @@ -1740,8 +1711,8 @@ static int set_url(int argc, const char **argv, const char *prefix) if (regcomp(&old_regex, oldurl, REG_EXTENDED)) die(_("Invalid old URL pattern: %s"), oldurl); - for (i = 0; i < urlset_nr; i++) - if (!regexec(&old_regex, urlset[i], 0, NULL, 0)) + for (i = 0; i < urlset->nr; i++) + if (!regexec(&old_regex, urlset->v[i], 0, NULL, 0)) matches++; else negative_matches++; diff --git a/remote.c b/remote.c index 76a3e41c73..9417d83e51 100644 --- a/remote.c +++ b/remote.c @@ -827,6 +827,11 @@ int remote_has_url(struct remote *remote, const char *url) return 0; } +struct strvec *push_url_of_remote(struct remote *remote) +{ + return remote->pushurl.nr ? &remote->pushurl : &remote->url; +} + static int match_name_with_pattern(const char *key, const char *name, const char *value, char **result) { diff --git a/remote.h b/remote.h index 84dc91cca0..034f9d6660 100644 --- a/remote.h +++ b/remote.h @@ -123,6 +123,7 @@ typedef int each_remote_fn(struct remote *remote, void *priv); int for_each_remote(each_remote_fn fn, void *priv); int remote_has_url(struct remote *remote, const char *url); +struct strvec *push_url_of_remote(struct remote *remote); struct ref_push_report { const char *ref_name; From patchwork Fri Jun 14 10:30:05 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13698594 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B28931946CF for ; Fri, 14 Jun 2024 10:30:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718361008; cv=none; b=b1z0PGQm0meblmfjdZ/4+dwsayRqSYH8XrqD5FEE+VDwhu3ty3sHbHgdWUMsxV+E3TYP7qvvHPMgn0vQcNbKPJDZoa92wt9QF7IJHzyzD5xnBL3x5HmgJFc+LnYtS/cP3U1lhbrnQKMB5PMjCIBIOp8jk2afTrPFxMZ498iJstg= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718361008; c=relaxed/simple; bh=Tseg8e4OiM5F6mcjcdWNDXpH1MaNsbBV1dEB0JOw3vQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ZtQi4HwzGWmwJOzTGTS7hdFGBjkn6Uq452emaG7X06MVfYYY1tBMafwSPuW/ydLsHgeP5yfCOWt4+GzTZlgAVNL4MzrOleJndIRGqHLYIHJmNRF83E+2E2Owx53vcGuwqEjw4n4uLUDu2qkiAJv7W96B6HHOiz12J5SrEk/KaHQ= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 16556 invoked by uid 109); 14 Jun 2024 10:30:05 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Jun 2024 10:30:05 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 27218 invoked by uid 111); 14 Jun 2024 10:30:03 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Jun 2024 06:30:03 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Jun 2024 06:30:05 -0400 From: Jeff King To: Junio C Hamano Cc: Mathew George , git@vger.kernel.org Subject: [PATCH 06/11] config: document remote.*.url/pushurl interaction Message-ID: <20240614103005.GF222445@coredump.intra.peff.net> References: <20240614102439.GA222287@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240614102439.GA222287@coredump.intra.peff.net> The documentation for these keys gives a very terse definition and points you to the fetch/push manpages. But from reading those pages it was not at all obvious to me that: - these are keys that can be defined multiple times with meaningful behavior (especially remote.*.url) - the way that pushurl overrides url (the git-push page does mention that "pushurl defaults to url", but it is not immediately clear what a multi-valued url would do in that situation). Let's try to summarize the current behavior. Signed-off-by: Jeff King --- Documentation/config/remote.txt | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt index 0678b4bcfe..eef0bf4f62 100644 --- a/Documentation/config/remote.txt +++ b/Documentation/config/remote.txt @@ -5,10 +5,16 @@ remote.pushDefault:: remote..url:: The URL of a remote repository. See linkgit:git-fetch[1] or - linkgit:git-push[1]. + linkgit:git-push[1]. A configured remote can have multiple URLs; + in this case the first is used for fetching, and all are used + for pushing (assuming no `remote..pushurl` is defined). remote..pushurl:: The push URL of a remote repository. See linkgit:git-push[1]. + If a `pushurl` option is present in a configured remote, it + is used for pushing instead of `remote..url`. A configured + remote can have multiple push URLs; in this case a push goes to + all of them. remote..proxy:: For remotes that require curl (http, https and ftp), the URL to From patchwork Fri Jun 14 10:31:22 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13698603 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2443D146582 for ; Fri, 14 Jun 2024 10:31:24 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718361085; cv=none; b=iclu4Dg5zgqD+ttVir4kvfkHGqjBsGaPORUi/odzKIvYpWnoti5w/v77FNUg08jJJa42sehZzxSjUgciFuP3bRvhzw9uVoy7GjO6aZxFCDQERBNkDqD9bFz3MaIq0JXaA+6P3pCU8QpRfH3yd1msmHVHjb+Rt0OQpkufxSBZHwE= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718361085; c=relaxed/simple; bh=p6rNIeEUpu2AJd/5HO07ON18CQmztacn1QULlWQdmJ4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=O/YcyQda87cI8ivZr9r9A4+LMS8yUicBBm3NDixUPeKtTKnjUBohs+48e1sNoqLPO/w1SJM8XG5oW0L3nGM8tOiymN5nXzrvmGQgmpCGqkDICvOxMJG1dkIuBqLX3LUZmaxybf8lHortUzelDpLKdAAHHEhnNJJoNkQQ8OPKxtk= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 16575 invoked by uid 109); 14 Jun 2024 10:31:23 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Jun 2024 10:31:23 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 27223 invoked by uid 111); 14 Jun 2024 10:31:20 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Jun 2024 06:31:20 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Jun 2024 06:31:22 -0400 From: Jeff King To: Junio C Hamano Cc: Mathew George , git@vger.kernel.org Subject: [PATCH 07/11] remote: allow resetting url list Message-ID: <20240614103122.GG222445@coredump.intra.peff.net> References: <20240614102439.GA222287@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240614102439.GA222287@coredump.intra.peff.net> Because remote.*.url is treated as a multi-valued key, there is no way to override previous config. So for example if you have remote.origin.url set to some wrong value, doing: git -c remote.origin.url=right fetch would not work. It would append "right" to the list, which means we'd still fetch from "wrong" (since subsequent values are used only as push urls). Let's provide a mechanism to reset the list, like we do for other multi-valued keys (e.g., credential.helper, http.extraheaders, and merge.suppressDest all use this "empty string means reset" pattern). Reported-by: Mathew George Signed-off-by: Jeff King --- By the way, I think the nearby remote.*.fetch and remote.*.push could learn the same trick. I left that out of this series, mostly because it was getting long. But also because I had trouble imagining how a one-off refspec change would be useful. We can revisit it on top if we want. Documentation/config/remote.txt | 5 ++++- remote.c | 10 +++++++-- t/t5505-remote.sh | 36 +++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 3 deletions(-) diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt index eef0bf4f62..8efc53e836 100644 --- a/Documentation/config/remote.txt +++ b/Documentation/config/remote.txt @@ -8,13 +8,16 @@ remote..url:: linkgit:git-push[1]. A configured remote can have multiple URLs; in this case the first is used for fetching, and all are used for pushing (assuming no `remote..pushurl` is defined). + Setting this key to the empty string clears the list of urls, + allowing you to override earlier config. remote..pushurl:: The push URL of a remote repository. See linkgit:git-push[1]. If a `pushurl` option is present in a configured remote, it is used for pushing instead of `remote..url`. A configured remote can have multiple push URLs; in this case a push goes to - all of them. + all of them. Setting this key to the empty string clears the + list of urls, allowing you to override earlier config. remote..proxy:: For remotes that require curl (http, https and ftp), the URL to diff --git a/remote.c b/remote.c index 9417d83e51..b7262964fb 100644 --- a/remote.c +++ b/remote.c @@ -63,12 +63,18 @@ static char *alias_url(const char *url, struct rewrites *r) static void add_url(struct remote *remote, const char *url) { - strvec_push(&remote->url, url); + if (*url) + strvec_push(&remote->url, url); + else + strvec_clear(&remote->url); } static void add_pushurl(struct remote *remote, const char *pushurl) { - strvec_push(&remote->pushurl, pushurl); + if (*pushurl) + strvec_push(&remote->pushurl, pushurl); + else + strvec_clear(&remote->pushurl); } static void add_pushurl_alias(struct remote_state *remote_state, diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 7789ff12c4..08424e878e 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1492,4 +1492,40 @@ test_expect_success 'refs/remotes/* refspec and unqualified DWIM and ) ' +test_expect_success 'empty config clears remote.*.url list' ' + test_when_finished "git config --remove-section remote.multi" && + git config --add remote.multi.url wrong-one && + git config --add remote.multi.url wrong-two && + git -c remote.multi.url= \ + -c remote.multi.url=right-one \ + -c remote.multi.url=right-two \ + remote show -n multi >actual.raw && + grep URL actual.raw >actual && + cat >expect <<-\EOF && + Fetch URL: right-one + Push URL: right-one + Push URL: right-two + EOF + test_cmp expect actual +' + +test_expect_success 'empty config clears remote.*.pushurl list' ' + test_when_finished "git config --remove-section remote.multi" && + git config --add remote.multi.url right && + git config --add remote.multi.url will-be-ignored && + git config --add remote.multi.pushurl wrong-push-one && + git config --add remote.multi.pushurl wrong-push-two && + git -c remote.multi.pushurl= \ + -c remote.multi.pushurl=right-push-one \ + -c remote.multi.pushurl=right-push-two \ + remote show -n multi >actual.raw && + grep URL actual.raw >actual && + cat >expect <<-\EOF && + Fetch URL: right + Push URL: right-push-one + Push URL: right-push-two + EOF + test_cmp expect actual +' + test_done From patchwork Fri Jun 14 10:31:31 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13698604 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 65D50195399 for ; Fri, 14 Jun 2024 10:31:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718361094; cv=none; b=XI55581ODXRav2KOSemfY5/esJFJknNaPuaAYcN1ceYoL+8uIvDmKG9s1S/FRF+UcM6DuwuyNAaXoaBxjmBNq3GGTjkLvCqiKTyG4HKGONI36T1z8XMeeNUc8/ic66tI8+GmqiS6Bzgyh2u8u4bZ+E8z3MMj8nF8H40CWreEu4I= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718361094; c=relaxed/simple; bh=Fc46l5RtGhnOdo1bPp1UbriDiVFHQpqpMpT4I9ZJL2k=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=AMWyR69VefMLSA6WdK9JHsvjfmwwwz1F8lXORy8MumWxVCnTy1iOocvT01LQ1SZZUY6SCBtpdspIxrAfp1AMEdV41nJbgr3tBMMfA0flmrDrwbYzdnfhaX+yF/epD9Js78hMauiWUpio1RrrjX8ebg+7Hrkub4cK2Dg6lpsBu7Y= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 16585 invoked by uid 109); 14 Jun 2024 10:31:32 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Jun 2024 10:31:32 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 27228 invoked by uid 111); 14 Jun 2024 10:31:29 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Jun 2024 06:31:29 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Jun 2024 06:31:31 -0400 From: Jeff King To: Junio C Hamano Cc: Mathew George , git@vger.kernel.org Subject: [PATCH 08/11] t5801: make remote-testgit GIT_DIR setup more robust Message-ID: <20240614103131.GH222445@coredump.intra.peff.net> References: <20240614102439.GA222287@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240614102439.GA222287@coredump.intra.peff.net> Our tests use a fake helper that just imports from an existing Git repository. We're fed the path to that repo on the command line, and derive the GIT_DIR by tacking on "/.git". This is wrong if the path is a bare repository, but that's OK since this is just a limited test. But it's also wrong if the transport code feeds us the actual .git directory itself (i.e., we expect "/path/to/repo" but it gives us "/path/to/repo/.git"). None of the current tests do that, but let's future-proof ourselves against adding a test that does. We can instead ask "rev-parse" to set our GIT_DIR. Note that we have to first unset other git variables from our environment. Coming into this script, we'll have GIT_DIR set to the fetching repository, and we need to "switch" to the remote one. Signed-off-by: Jeff King --- t/t5801/git-remote-testgit | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit index c5b10f5775..f8b476499f 100755 --- a/t/t5801/git-remote-testgit +++ b/t/t5801/git-remote-testgit @@ -26,7 +26,8 @@ then t_refspec="" fi -GIT_DIR="$url/.git" +unset $(git rev-parse --local-env-vars) +GIT_DIR=$(git -C "$url" rev-parse --absolute-git-dir) export GIT_DIR force= From patchwork Fri Jun 14 10:34:54 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13698605 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3F38213BAC8 for ; Fri, 14 Jun 2024 10:34:55 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718361297; cv=none; b=mGmc6EHV+dIFFr9BKgNVfkfw6G1IfNKVLeqbsOwxWQVzmT9SQ1Z8aPWYjjls6Rf1icj2UBhyyh0PnLeFSAujkS16fpOBzoOMR66BkGNktgqDzrz9KxI8YbTCje71DGxBCxNQI7OgaANvlqINVVavSOmSRcNJ/cZiA7GYlJYuyyc= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718361297; c=relaxed/simple; bh=HtGbiCKU5FTDxmClSGcnC5s821kXlBUU6/kECUQobTc=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ZbrlPb1GYp9E0LqyEd7mxraD+SaZJdBo4fld8+7CfpZiLBQHtArdkor2aW1conZTn336miRdv3T/gXNEQF9b9wMDMfRlhtdGmkIhXLagUdtIiolEJpPo6NDOIFgMfm/UGjn9WSxIDRw549YqYmjb7RE/3Uf41j760gsJex8UcuA= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 16616 invoked by uid 109); 14 Jun 2024 10:34:55 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Jun 2024 10:34:55 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 27343 invoked by uid 111); 14 Jun 2024 10:34:54 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Jun 2024 06:34:54 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Jun 2024 06:34:54 -0400 From: Jeff King To: Junio C Hamano Cc: Mathew George , git@vger.kernel.org Subject: [PATCH 09/11] t5801: test remote.*.vcs config Message-ID: <20240614103454.GI222445@coredump.intra.peff.net> References: <20240614102439.GA222287@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240614102439.GA222287@coredump.intra.peff.net> The usual way to trigger a remote helper is to use the "::" syntax from: 87422439d1 (Allow specifying the remote helper in the url, 2009-11-18). Doing: git config remote.origin.url hg::https://example.com/repo will run "git-remote-hg origin https://example.com/repo". Or you can use the fallback handling from 25d5cc488a (Pass unknown protocols to external protocol handlers, 2009-12-09): git config remote.origin.url "foo://bar" which will run "git-remote-foo origin foo://bar". But there's a third way, from c578f51d52 (Add a config option for remotes to specify a foreign vcs, 2009-11-18): git config remote.origin.vcs foo git config remote.origin.url bar which will run "git-remote-foo origin bar". This is mostly redundant with the other methods, except that it is supposed to allow you to run without a URL at all. So: git config remote.origin.vcs foo would run "git-remote-foo origin" with no extra URL parameter (under the assumption that the helper somehow knows how to access the remote repo). However, this mode has been broken since 25d5cc488a, shortly after it was added! That commit taught the transport code to always look at the URL string to parse off the "foo::" bits, meaning it would always segfault in the no-url case. You can see that with: git -c remote.foo.vcs=bar fetch foo Nobody seems to have noticed in the almost 15 years since, so presumably it's not a well-used feature. And without that, arguably the whole remote.*.vcs feature could be removed entirely, as it isn't offering anything you couldn't do with the "helper::" syntax. But it _does_ work if you have a URL, and it has been advertised in the documentation for all that time. So we shouldn't just remove it without warning. Likewise, even if we were going to deprecate it, we should avoid breaking it in the meantime. Since there are no tests for it at all, let's add a few basic ones: - this syntax doesn't work well with "git clone" (another point against it versus "helper::"). But we can use "clone -c" to set up the config manually, passing the URL as usual to clone. This does work, though note that I had to use --no-local in the test to avoid broken interactions between the local code and the helper. In the real world this would be a non-issue, since the remote URL would generally not also be a local Git repo! - likewise, we should be able to set up the config manually and fetch into a repository. This also works. - we can simulate a vcs that has no URL support by stuffing the remote path into another environment variable. This should work, but doesn't (it hits the segfault mentioned above). In the first two cases, I took the extra step of checking GIT_TRACE output to confirm that we actually ran the helper (since the URL is a valid Git repo, the clone/fetch would appear to work even if we didn't use the helper at all!). Signed-off-by: Jeff King --- I have no real problem with deprecating this, and it might be a nice thing to clean up in the long run. But it seemed like less work to just fix it in the next patch, so I did that. ;) t/t5801-remote-helpers.sh | 23 +++++++++++++++++++++++ t/t5801/git-remote-nourl | 3 +++ 2 files changed, 26 insertions(+) create mode 100755 t/t5801/git-remote-nourl diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index 4e0a77f985..7c8c4359aa 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -38,6 +38,29 @@ test_expect_success 'cloning from local repo' ' test_cmp server/file local/file ' +test_expect_success 'clone with remote.*.vcs config' ' + GIT_TRACE=$PWD/vcs-clone.trace \ + git clone --no-local -c remote.origin.vcs=testgit "$PWD/server" vcs-clone && + test_grep remote-testgit vcs-clone.trace +' + +test_expect_success 'fetch with configured remote.*.vcs' ' + git init vcs-fetch && + git -C vcs-fetch config remote.origin.vcs testgit && + git -C vcs-fetch config remote.origin.url "$PWD/server" && + GIT_TRACE=$PWD/vcs-fetch.trace \ + git -C vcs-fetch fetch origin && + test_grep remote-testgit vcs-fetch.trace +' + +test_expect_failure 'vcs remote with no url' ' + NOURL_UPSTREAM=$PWD/server && + export NOURL_UPSTREAM && + git init vcs-nourl && + git -C vcs-nourl config remote.origin.vcs nourl && + git -C vcs-nourl fetch origin +' + test_expect_success 'create new commit on remote' ' (cd server && echo content >>file && diff --git a/t/t5801/git-remote-nourl b/t/t5801/git-remote-nourl new file mode 100755 index 0000000000..09be6013c5 --- /dev/null +++ b/t/t5801/git-remote-nourl @@ -0,0 +1,3 @@ +#!/bin/sh + +exec git-remote-testgit "$1" "$NOURL_UPSTREAM" From patchwork Fri Jun 14 10:37:11 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13698608 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CD448148319 for ; Fri, 14 Jun 2024 10:37:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718361434; cv=none; b=PDTcGIVl9vr3UWTBYFMTJM8pME4N8ZkuoOryuBYwyWV0GLu4wGN+LRG64S+DPeyt6dnlmLw9O/XN/mdQttAUYP0wmCcKjYfih4oHJ6iMc7mgbEhXcXdX1BuI/D5knqKgHEqLSseS5b+0xDWfX6SJKVio42nooABoZ4Z1vfSSE1I= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718361434; c=relaxed/simple; bh=oX5bdMPhS1HR3SjNhmUlUjfJMe6ykBhyDE0KbmNJ53E=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=HK6sCk3JjU0ltJSZM9iiobAnLl5PpDcA9GNP9VDqm3oCQAc52K+q5WnqWxTBIcyA95Nb2OeamNDqhNvDtmWNpZ6YX8eDn60jVXo7GAtcKZfAWWKlU/6Gg5MmZFs13LdZajubMVYDYtYrUKbACxB1V0f9d02QiA8pvHpYwuPxqaA= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 16632 invoked by uid 109); 14 Jun 2024 10:37:12 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Jun 2024 10:37:12 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 27384 invoked by uid 111); 14 Jun 2024 10:37:11 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Jun 2024 06:37:11 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Jun 2024 06:37:11 -0400 From: Jeff King To: Junio C Hamano Cc: Mathew George , git@vger.kernel.org Subject: [PATCH 10/11] remote: always require at least one url in a remote Message-ID: <20240614103711.GJ222445@coredump.intra.peff.net> References: <20240614102439.GA222287@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240614102439.GA222287@coredump.intra.peff.net> When we return a struct from remote_get(), the result _almost_ always has at least one url. In remotes_remote_get_1(), we do this: if (name_given && !valid_remote(ret)) add_url_alias(remote_state, ret, name); if (!valid_remote(ret)) return NULL; So if the remote doesn't have a url, we give it one based on the name (this is how unconfigured urls are used as remotes). And if that doesn't work, we return NULL. But there's a catch: valid_remote() checks that we have at least one url _unless_ the remote.*.vcs field is set. This comes from c578f51d52 (Add a config option for remotes to specify a foreign vcs, 2009-11-18), and the whole idea was to support remote helpers that don't have their own url. However, that mode has been broken since 25d5cc488a (Pass unknown protocols to external protocol handlers, 2009-12-09)! That commit unconditionally looks at the url in get_helper(), causing a segfault with something like: git -c remote.foo.vcs=bar fetch foo We could fix that now, of course. But given that it has been broken for almost 15 years and nobody noticed, there's a better option. This weird "there might not be a url" special case requires checks all over the code base, and it's not clear if there are other similar segfaults lurking. It would be nice if we could drop that special case. So instead, let's let the "the remote name is the url" code kick in. If you have "remote.foo.vcs", then your url (unless otherwise configured) is "foo". This does have a visible effect compared to what 25d5cc488a was trying to do. The idea back then is that for a remote without a url, we'd run: # only one command-line option! git-remote-bar foo whereas with our default url, now we'll run: git-remote-bar foo foo Again, in practice nobody can be relying on this because it has been segfaulting for 15 years. We should consider just removing this "vcs" config option entirely, but that would be a user-visible breakage. So by fixing it this way, we can keep things working that have been working, and simplify away one special case inside our code. This fixes the segfault from 25d5cc488a (demonstrated by the test), and we can build further cleanups on top. Signed-off-by: Jeff King --- remote.c | 2 +- t/t5801-remote-helpers.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/remote.c b/remote.c index b7262964fb..5fa046c8f8 100644 --- a/remote.c +++ b/remote.c @@ -32,7 +32,7 @@ struct counted_string { static int valid_remote(const struct remote *remote) { - return (!!remote->url.nr) || (!!remote->foreign_vcs); + return !!remote->url.nr; } static char *alias_url(const char *url, struct rewrites *r) diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index 7c8c4359aa..20f43f7b7d 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -53,7 +53,7 @@ test_expect_success 'fetch with configured remote.*.vcs' ' test_grep remote-testgit vcs-fetch.trace ' -test_expect_failure 'vcs remote with no url' ' +test_expect_success 'vcs remote with no url' ' NOURL_UPSTREAM=$PWD/server && export NOURL_UPSTREAM && git init vcs-nourl && From patchwork Fri Jun 14 10:42:03 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13698612 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C78ED13BAC8 for ; Fri, 14 Jun 2024 10:42:04 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718361726; cv=none; b=C6zqY3JQmqs8QnhYhXGFMaCMJAuoZWIU099fyOgoh3SoanJS3s9QwDE0jMIvcsa4jBi7lV6uxRFCh+ZymR2B/FLQqaUDHJjVraDrDiGO3g1Y/PBSTKLsEvk21L/5t3A8XFBlHdByP0mpBg2cbkaK0OADvsJx0P9UFgtlOyd1t7w= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1718361726; c=relaxed/simple; bh=dkSZC39SCnAjEk5DVcrbXGSFMQodQvnILwNYFOAKvIc=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ffVIIY1xUNVmbyUeXbvczjpNHocx4s/D5a9onw5S2TjEUhPsnLZdg8/J6okFCoABhrlcPuq34vZrGxB9uQI/lppw45LP/WAN5daKFJXBw6pxTPkeuWzyNmRPAvgeJ/8MY1c0rw8ytZYrUGoCiUhQ+YUPUF1f9Pc5q4yl3A4OMrI= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 16646 invoked by uid 109); 14 Jun 2024 10:42:04 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 14 Jun 2024 10:42:04 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 27425 invoked by uid 111); 14 Jun 2024 10:42:03 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Fri, 14 Jun 2024 06:42:03 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 14 Jun 2024 06:42:03 -0400 From: Jeff King To: Junio C Hamano Cc: Mathew George , git@vger.kernel.org Subject: [PATCH 11/11] remote: drop checks for zero-url case Message-ID: <20240614104203.GK222445@coredump.intra.peff.net> References: <20240614102439.GA222287@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240614102439.GA222287@coredump.intra.peff.net> Now that the previous commit removed the possibility that a "struct remote" will ever have zero url fields, we can drop a number of redundant checks and untriggerable code paths. Signed-off-by: Jeff King --- Note for reviewers: the hunk in builtin/push.c is funny. The original code was: if (url->nr) { for (i = 0; i < url->nr; i++) { do this } } else { do that } And I removed the "do that" part along with the if/else to become: for (i = 0; i < url->nr; i++) { do this } But because "this" and "that" were so similar, and because the indentation of "this" in the loop was now the same of the old "that", the diff makes it look like I dropped the first half of the conditional. builtin/archive.c | 2 -- builtin/ls-remote.c | 2 -- builtin/push.c | 13 ++----------- builtin/remote.c | 13 +++---------- t/helper/test-bundle-uri.c | 2 -- transport.c | 17 +++++++---------- 6 files changed, 12 insertions(+), 37 deletions(-) diff --git a/builtin/archive.c b/builtin/archive.c index 0d9aff7e6f..7234607aaa 100644 --- a/builtin/archive.c +++ b/builtin/archive.c @@ -31,8 +31,6 @@ static int run_remote_archiver(int argc, const char **argv, struct packet_reader reader; _remote = remote_get(remote); - if (!_remote->url.nr) - die(_("git archive: Remote with no URL")); transport = transport_get(_remote, _remote->url.v[0]); transport_connect(transport, "git-upload-archive", exec, fd); diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c index 4c04dbbf19..8f3a64d838 100644 --- a/builtin/ls-remote.c +++ b/builtin/ls-remote.c @@ -109,8 +109,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) die("bad repository '%s'", dest); die("No remote configured to list refs from."); } - if (!remote->url.nr) - die("remote %s has no configured URL", dest); if (get_url) { printf("%s\n", remote->url.v[0]); diff --git a/builtin/push.c b/builtin/push.c index 00d99af1a8..8260c6e46a 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -438,18 +438,9 @@ static int do_push(int flags, } errs = 0; url = push_url_of_remote(remote); - if (url->nr) { - for (i = 0; i < url->nr; i++) { - struct transport *transport = - transport_get(remote, url->v[i]); - if (flags & TRANSPORT_PUSH_OPTIONS) - transport->push_options = push_options; - if (push_with_options(transport, push_refspec, flags)) - errs++; - } - } else { + for (i = 0; i < url->nr; i++) { struct transport *transport = - transport_get(remote, NULL); + transport_get(remote, url->v[i]); if (flags & TRANSPORT_PUSH_OPTIONS) transport->push_options = push_options; if (push_with_options(transport, push_refspec, flags)) diff --git a/builtin/remote.c b/builtin/remote.c index 06c09ed060..c5c94d4dbd 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1002,8 +1002,7 @@ static int get_remote_ref_states(const char *name, struct transport *transport; const struct ref *remote_refs; - transport = transport_get(states->remote, states->remote->url.nr > 0 ? - states->remote->url.v[0] : NULL); + transport = transport_get(states->remote, states->remote->url.v[0]); remote_refs = transport_get_remote_refs(transport, NULL); states->queried = 1; @@ -1294,8 +1293,7 @@ static int show(int argc, const char **argv, const char *prefix) get_remote_ref_states(*argv, &info.states, query_flag); printf_ln(_("* remote %s"), *argv); - printf_ln(_(" Fetch URL: %s"), info.states.remote->url.nr > 0 ? - info.states.remote->url.v[0] : _("(no URL)")); + printf_ln(_(" Fetch URL: %s"), info.states.remote->url.v[0]); url = push_url_of_remote(info.states.remote); for (i = 0; i < url->nr; i++) /* @@ -1440,10 +1438,7 @@ static int prune_remote(const char *remote, int dry_run) } printf_ln(_("Pruning %s"), remote); - printf_ln(_("URL: %s"), - states.remote->url.nr - ? states.remote->url.v[0] - : _("(no URL)")); + printf_ln(_("URL: %s"), states.remote->url.v[0]); for_each_string_list_item(item, &states.stale) string_list_append(&refs_to_prune, item->util); @@ -1632,8 +1627,6 @@ static int get_url(int argc, const char **argv, const char *prefix) } url = push_mode ? push_url_of_remote(remote) : &remote->url; - if (!url->nr) - die(_("no URLs configured for remote '%s'"), remotename); if (all_mode) { for (i = 0; i < url->nr; i++) diff --git a/t/helper/test-bundle-uri.c b/t/helper/test-bundle-uri.c index 3285dd962e..0c5fa723d8 100644 --- a/t/helper/test-bundle-uri.c +++ b/t/helper/test-bundle-uri.c @@ -88,8 +88,6 @@ static int cmd_ls_remote(int argc, const char **argv) die(_("bad repository '%s'"), dest); die(_("no remote configured to get bundle URIs from")); } - if (!remote->url.nr) - die(_("remote '%s' has no configured URL"), dest); transport = transport_get(remote, NULL); if (transport_get_remote_bundle_uri(transport) < 0) { diff --git a/transport.c b/transport.c index eba92eb7e0..a324045240 100644 --- a/transport.c +++ b/transport.c @@ -1112,6 +1112,7 @@ static struct transport_vtable builtin_smart_vtable = { struct transport *transport_get(struct remote *remote, const char *url) { const char *helper; + const char *p; struct transport *ret = xcalloc(1, sizeof(*ret)); ret->progress = isatty(2); @@ -1127,19 +1128,15 @@ struct transport *transport_get(struct remote *remote, const char *url) ret->remote = remote; helper = remote->foreign_vcs; - if (!url && remote->url.nr) + if (!url) url = remote->url.v[0]; ret->url = url; - /* maybe it is a foreign URL? */ - if (url) { - const char *p = url; - - while (is_urlschemechar(p == url, *p)) - p++; - if (starts_with(p, "::")) - helper = xstrndup(url, p - url); - } + p = url; + while (is_urlschemechar(p == url, *p)) + p++; + if (starts_with(p, "::")) + helper = xstrndup(url, p - url); if (helper) { transport_helper_init(ret, helper);