From: Usman Akinyemi <usmanakinyemi202@gmail.com>
Date: Mon, 09 Mar 2026 00:56:49 GMT
Subject: Re: [RFC PATCH 2/2] push: support pushing to a remote group
Message-ID: <CAPSxiM_KVU7rE49=omWUwaYS-u_J6eQPDgTRjPop1gj6BM1qKQ@mail.gmail.com>
In-Reply-To: <xmqq4imsv13x.fsf@gitster.g>

>
> The basic idea to use "remote" (the default remote cannot be multiple)
> vs "remote_group" (the command line gave which remotes to talk with)
> sounds good.
>
> But I started wondering what happens when the command line gave a
> single remote to talk with.  Probably we want a code that does
>
>         if (remote_group has only one remote)
>                 remote = take the sole remote from the remote_group;
Make sense.
>
> here before we continue.  Or the other way around and we handle the
> "default remote cannot be multiple" case as a special case, e.g.
>
>         if (remote) {
>                 create remote_group with a single member "remote";
>                 remote = NULL;
>         }
>
> and then we do not have to do ...
>
> > +     /*
> > +      * set_refspecs and mirror detection must not use `remote`
> > +      * when it may be NULL (group path). For the single-remote case,
> > +      * handle them here. For the group case they are handled
> > +      * per-remote inside the loop below.
> > +      */
>
> ... "handle them here because single-remote is special" at all, no?
>
> I would prefer to avoid "X must be done for each remote in the
> remote-group, but Y can be done only once", as future developers
> will get it wrong when they add their own Z and consider which side
> Z falls into.  The code structure that removes special case would
> help by making sure that a singleton case is special only because
> the loop over remote_group runs once, and otherwise there is nothing
> special goes on.

Yeah, that is a good design and makes sense. Thanks.

Also, in the cover letter, I asked some questions. I think you might
have missed it.

Quoting here again:
"
  - push.default = simple interacts poorly with group pushes when the
    current branch has no upstream set, since setup_default_push_refspecs()
    will die on the first remote that is not the upstream. Users should
    use push.default = current or explicit refspecs for group pushes.
    It is worth discussing whether the group push path should automatically
    imply push.default = current, or whether a clear error message
    directing the user to configure this would be sufficient.

  - force-with-lease semantics across a group push are currently
    unmodified — the same CAS constraints are forwarded to every remote
    in the group. Whether this is the right behaviour or whether
    per-remote lease tracking is needed is an open question.
"

I will want feedback on this also.

Thanks