Re: [PATCH v3 2/4] remote: use remote_state parameter internally

From: Glen Choo <chooglen@google.com>
Date: Oct 25, 2021, 18:09 UTC
Message-ID: <kl6l35opasd1.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqtuhbo2tn.fsf@gitster.g>

Sorry I wasn't able to get to this sooner, this will be my first priority.

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

>> This made my "git push" to k.org and other places over ssh segfault
>> when their tip and what I am attempting to push are identical.  I
>> haven't spent more time than just to bisect the history down to
>> identify this commit as the possible culprit.

Does this fail iff the tip and the attempted push are identical, or does it also fail for others sorts of pushes?

Show 36 quoted lines

> (gdb) bt
> #0  0x000055555579a785 in pushremote_for_branch (branch=0x0, explicit=0x7fffffffcf84)
>     at remote.c:564
> #1  0x000055555579a5c2 in remotes_remote_get_1 (remote_state=0x5555559782a0, name=0x0,
>     get_default=0x55555579a742 <pushremote_for_branch>) at remote.c:518
> #2  0x000055555579a6d0 in remotes_pushremote_get (remote_state=0x5555559782a0, name=0x0)
>     at remote.c:542
> #3  0x000055555579a740 in repo_pushremote_get (repo=0x555555974b80 <the_repo>, name=0x0)
>     at remote.c:554
> #4  0x000055555560aa9d in pushremote_get (name=0x0) at ./remote.h:135
> #5  0x000055555560c5ce in cmd_push (argc=0, argv=0x7fffffffdc70, prefix=0x0)
>     at builtin/push.c:611
> #6  0x000055555557396a in run_builtin (p=0x555555941f78 <commands+2136>, argc=3,
>     argv=0x7fffffffdc70) at git.c:461
> #7  0x0000555555573d79 in handle_builtin (argc=3, argv=0x7fffffffdc70) at git.c:713
> #8  0x0000555555573fe6 in run_argv (argcp=0x7fffffffdafc, argv=0x7fffffffdaf0) at git.c:780
> #9  0x000055555557448f in cmd_main (argc=3, argv=0x7fffffffdc70) at git.c:911
> #10 0x000055555565b2ae in main (argc=6, argv=0x7fffffffdc58) at common-main.c:52
>
> The direct culprit is this part:
>
>     const char *pushremote_for_branch(struct branch *branch, int *explicit)
>     {
>             if (branch && branch->pushremote_name) {
>                     if (explicit)
>                             *explicit = 1;
>                     return branch->pushremote_name;
>             }
>             if (branch->remote_state->pushremote_name) {
>
> where the second if() statement used to check "pushremote_name", but
> now unconditionally dereferences "branch".
>
> The caller is remote_get_1(); this funciton was called with
> "current_branch", which can be NULL until you have a repository and
> you've called read_config(), but otherwise shouldn't be.

Thanks for helping to narrow the scope of the search :)

Show 5 quoted lines

> I think somebody is not setting up the remote_state correctly?  When
> the user wants to just use the repository-wide pushremote, not
> having the current_branch would not matter, but if the pushremote
> for the current branch is different from the repository-wide one,
> the code would silently push to a wrong remote.

For the_repository, remote_state should be initialized via initialize_the_repository(), which is called in common-main.c. I assumed that this would set up remote_state correctly. I will confirm whether or not this is the cause.

Show 7 quoted lines

> In any case, any public facing entry point, like pushremote_get()
> that is directly called from cmd_push() with just a name, should
> auto vivify an instance of struct remote_state and populate its
> members as needed, I think, and in this particular case, I suspect
> that it forgets to initialize the current_branch and other members
> by calling read_config(), just like other entry points like
> repo_remote_get() do.

I'm fairly certain that pushremote_get() calls read_config() correctly via repo_pushremote_get():

  struct remote *repo_pushremote_get(struct repository *repo, const char *name)
  {
    read_config(repo);
    return remotes_pushremote_get(repo->remote_state, name);
  }

Perhaps the problem lies elsewhere, let me look into it further.