Jeff King <peff@xxxxxxxx> writes:
> 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
These should work as intended on top of my patch. But I will skip doing
these changes for now. I do see the merit but I think it is also okay
the way it is now.
Attachment:
signature.asc
Description: PGP signature
