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

> 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?
>
> Oh, I misread Junio's patch in my earlier response. I was focused on not
> setting name_given, which I thought would result in a NULL return value,
> and didn't notice that it would also mean using the default remote.
> Something like:
>
> 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