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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 10, 2024, 15:34 UTC
Message-ID: <xmqqle29z2eo.fsf@gitster.g>
In-Reply-To: <CAOLa=ZQXZ6DyE3YjuVU48nQcj0xuW7uPoPvg2yqktk+S6gXwsg@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 26 quoted lines

>> 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.
> ...
> This is because we actually obtained the default remote here. Isn't this
> confusing from a user's perspective?

I agree with you 100%. I haven't checked the ramifications of such a change to other code paths, and callers of remote_get() are many. With your fix, the above is not necessary and it certainly can be left well outside the scope of the current topic.

> I mean I agree that an empty repo
> name is something we should support, but it also shouldn't be something
> we simply ignore?

For the sake of simplicity of the UI design, I think an empty repo name (giving an empty string explicitly where a repository nickname or URL is expected) should be forbidden. The above change lets "" stand for the default remote (which is different from "simply" ignoring), and is confusing, because we never did that historically. Unless we advertise it as a new "feature" [*], that is (and I do not believe it is such a great feature myself).

> So if we're simply testing my patch, this is definitely enough. But I
> wanted to also keep in mind the state before this patch. Wherein the
> only way the flow would be triggered is if we provide a local_ref,
> providing ':' follows a different path in 'set_refspecs'.

OK. If so we may want to have both, so that we won't regress in the other code paths?

Thanks.

[Footnote]

 * When you want to interact with the remote you usually work with
   but want to affect a custom set of refs, with the current UI,
   you'd need to remember what you call that usual remote (typically
   "origin") and say "git [push|fetch] origin $refspec".  The above
   change may be seen as a feature that allows you to use an empty
   string in place of the remote that has to be named explicitly on
   the command line.