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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 9, 2024, 09:59 UTC
Message-ID: <CAOLa=ZT6kmP=z1FG_aoZfEnZdqtW3Hmy8D9WsJrM55CUNioCoQ@mail.gmail.com>
In-Reply-To: <xmqqsewjelrh.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 8 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Since 9badf97c4 (remote: allow resetting url list),
>
> Please do not be original in places where it shouldn't matter.  Use
> "git show -s --format=reference" that includes the datestamp to help
> readers judge how old the problem is.
>

Will do.

Show 16 quoted lines

>> we reset the remote
>> URL if the provided URL is empty. This means any caller of
>> `remotes_remote_get()` would now get a NULL remote.
>
> "NULL remote" meaning?
>
> If you have this:
>
>  [remote "multi"]
> 	url = wrong-one
> 	url = wrong-two
> 	url =
>
> and ask "remotes_remote_get()" to give you the remote "multi", you'd
> get a remote whose URL array has no elements.  Is that what you are
> referring to?

I was referring to the 'struct remote *' being 'NULL'. I think Jeff explained this in his email reply to this.

Show 16 quoted lines

>> The 'builtin/push.c' code, calls 'set_refspecs' before validating the
>> remote.
>
> There is a comment about "lazily grab remote", so it is very
> understandable.
>
>> This worked earlier since we would get a remote, albeit with an
>> empty URL. With the new changes, we get a NULL remote and this crashes.
>
> You'd really really need to clarify what you mean by "a NULL remote"
> if you want the proposed log message and the change to be
> understood.  The change made by 9badf97c (remote: allow resetting
> url list, 2024-06-14), as far as I can tell, can make the strvecs
> that hold URL and pushURL in a remote structure empty, but it does
> not otherwise destroy the remote structure, or nullify a pointer
> that points at the remote structure.  So I am completely lost here.

I will amend the commit to the following:

Since 9badf97c42 (remote: allow resetting url list, 2024-06-14), we reset the remote URL if the provided URL is empty. When a user of 'remotes_remote_get' tries to fetch a remote with an empty repo name, the function initializes the remote via 'make_remote'. But the remote is still not a valid remote, since the URL is empty, so it tries to add the URL alias using 'add_url_alias'. This in-turn will call 'add_url', but since the URL is empty we call 'strvec_clear' on the `remote->url`. Back in 'remotes_remote_get', we again check if the remote is valid, which fails, so we return 'NULL' for the 'struct remote *' value.

The 'builtin/push.c' code, calls 'set_refspecs' before validating the remote. This worked earlier since we would get a remote, albeit with an empty URL. With the new changes, we get a NULL remote this causes the check for remote to fail and raises the BUG.

---

I hope this makes it clearer