git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 4/4] remote: use remote_is_configured() for add and rename

From
Jeff King <peff@peff.net>
Date
Feb 15, 2016, 18:33 UTC
Message-ID
<20160215183334.GH26443@sigill.intra.peff.net>
In-Reply-To
<1455558150-30267-5-git-send-email-t.gummerer@gmail.com>
On Mon, Feb 15, 2016 at 06:42:30PM +0100, Thomas Gummerer wrote:
Show 17 quoted lines
> Both remote add and remote rename use a slightly different hand-rolled
> check if the remote exits.  The hand-rolled check may have some subtle
> cases in which it might fail to detect when a remote already exists.
> One such case was fixed in fb86e32 ("git remote: allow adding remotes
> agreeing with url.<...>.insteadOf").  Another case is when a remote is
> configured as follows:
> 
>   [remote "foo"]
>     vcs = bar
> 
> If we try to run `git remote add foo bar` with the above remote
> configuration, git segfaults.  This change fixes it.
> 
> In addition, git remote rename $existing foo with the configuration for
> foo as above silently succeeds, even though foo already exists,
> modifying its configuration.  With this patch it fails with "remote foo
> already exists".
Checking is_configured() certainly sounds like a better test, but...
Show 14 quoted lines
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 981c487..bd57f1b 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -186,10 +186,7 @@ static int add(int argc, const char **argv)
>  	url = argv[1];
>  
>  	remote = remote_get(name);
> -	if (remote && (remote->url_nr > 1 ||
> -			(strcmp(name, remote->url[0]) &&
> -				strcmp(url, remote->url[0])) ||
> -			remote->fetch_refspec_nr))
> +	if (remote_is_configured(remote))
>  		die(_("remote %s already exists."), name);

This original is quite confusing. I thought at first that there was perhaps something going on with allowing repeated re-configuration of the same remote, as long as some parameters matched. I.e., I am wondering if there is a case here that does _not_ segfault, that we would be breaking.

But reading over fb86e32dcc, I think I have convinced myself that it was merely an ad-hoc check for "is_configured", and using that function is a better replacement.

-Peff
Previous: Thomas GummererNext: Thomas Gummerer
Message 14 of 18 in “git remote improvements”
  1. 0/4 git remote improvementsThomas Gummerer, Feb 15, 2016
  2. 1/4 remote: use skip_prefixThomas Gummerer, Feb 15, 2016
  3. Jeff KingFeb 15, 2016
  4. Eric SunshineFeb 15, 2016
  5. Jeff KingFeb 15, 2016
  6. Thomas GummererFeb 15, 2016
  7. 2/4 remote: simplify remote_is_configured()Thomas Gummerer, Feb 15, 2016
  8. Jeff KingFeb 15, 2016
  9. Thomas GummererFeb 15, 2016
  10. Junio C HamanoFeb 15, 2016
  11. 3/4 remote: actually check if remote exitsThomas Gummerer, Feb 15, 2016
  12. Jeff KingFeb 15, 2016
  13. 4/4 remote: use remote_is_configured() for add and renameThomas Gummerer, Feb 15, 2016
  14. Jeff KingFeb 15, 2016
  15. Thomas GummererFeb 15, 2016
  16. Johannes SchindelinFeb 17, 2016
  17. Thomas GummererFeb 17, 2016
  18. Johannes SchindelinFeb 17, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.