Re: [PATCH v2] builtin/push: call set_refspecs after validating remote

From

Jeff King <peff@peff.net>

Date

Jul 10, 2024, 15:46 UTC

Message-ID

<20240710154620.GA3097664@coredump.intra.peff.net>

In-Reply-To

<CAOLa=ZQXZ6DyE3YjuVU48nQcj0xuW7uPoPvg2yqktk+S6gXwsg@mail.gmail.com>

On Wed, Jul 10, 2024 at 06:12:21AM -0700, Karthik Nayak wrote:

> > In any case, an obvious additional fix on top of your change might
> > be to do something like this:
> >
> >         diff --git i/remote.c w/remote.c
> >         index 5fa046c8f8..d7f9ba3571 100644
> >         --- i/remote.c
> >         +++ w/remote.c
> >         @@ -682,7 +682,7 @@ remotes_remote_get_1(
> >                 struct remote *ret;
> >                 int name_given = 0;
> >
> >         -	if (name)
> >         +	if (name && *name)
> >                         name_given = 1;
> >                 else
> >                         name = get_default(remote_state, remote_state->current_branch,
> >
> > which would give us the default remote name, and we would not call
> > add_url_alias() with a bogus empty string to nuke the list.
> >
> 
> I'm a bit skeptical of making this change. Mostly from the user's
> perspective.
> 
> With my patch currently:
> 
>     $ git push "" refs/heads/master
>     fatal: bad repository ''
> 
> But with this added, we'd be doing
> 
>     $ git push "" refs/heads/master
>     Everything up-to-date
> 
> This is because we actually obtained the default remote here. Isn't this
> confusing from a user's perspective? I mean I agree that an empty repo
> name is something we should support, but it also shouldn't be something
> we simply ignore?

diff --git a/remote.c b/remote.c
index 7f6406aaa2..883cf6086e 100644
--- a/remote.c
+++ b/remote.c
@@ -703,7 +703,7 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,
 		if (!valid_remote(ret))
 			read_branches_file(remote_state, ret);
 	}
-	if (name_given && !valid_remote(ret))
+	if (name_given && *name && !valid_remote(ret))
 		add_url_alias(remote_state, ret, name);
 	if (!valid_remote(ret))
 		return NULL;

was more what I was thinking. That is, inhibit the empty string
explicitly rather than letting the emergent behavior of add_url_alias()
do it for us. Or maybe even just:

diff --git a/remote.c b/remote.c
index 7f6406aaa2..a0b166131f 100644
--- a/remote.c
+++ b/remote.c
@@ -690,9 +690,11 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,
 	struct remote *ret;
 	int name_given = 0;
 
-	if (name)
+	if (name) {
+		if (!name)
+			return NULL;
 		name_given = 1;
-	else
+	} else
 		name = get_default(remote_state, remote_state->current_branch,
 				   &name_given);
 

to bail immediately.

But all of that would be internal refactoring / cleanup on top of your
patch. The user-facing behavior would be the same.

-Peff