Re: [PATCH] rev-parse: respect push.autosetupremote when evaluating @{push}
- From
Tao Klerks <tao@klerks.biz>
- Date
- Jul 16, 2022, 17:40 UTC
- Message-ID
- <CAPMMpoirWDBJn45dkWo8CRDU23G1mnyPDHcKq1eH1AtACEW9Rg@mail.gmail.com>
- In-Reply-To
- <xmqq5yk4r96l.fsf@gitster.g>
On Sun, Jul 10, 2022 at 10:42 PM Junio C Hamano <gitster@pobox.com> wrote:
> > "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
Thanks so much for looking at this, my apologies for the delayed response.
Show 13 quoted lines
>
> > + if (branch && (!branch->merge || !branch->merge[0])) {
> > + repo_config_get_bool(the_repository,
> > + "push.autosetupremote",
> > + &autosetupremote);
> > + if (autosetupremote)
> > + return tracking_for_push_dest(remote, branch->refname, err);
>
> Before the first push of the branch X where we are asking for
> X@{push}, i.e. there is not the corresponding branch over there yet
> and we do not have the remote-tracking branch for it yet, what does
> this function return? If it continues to error out, then I think
> this patch may make sense, but ...It does indeed still error out, because even though "git push" may succeed (eg if "push.autosetupremote=true"), there simply isn't a remote-tracking branch for it yet.
The only "point" of the patch is to address the edge-case where the remote-tracking branch *does* exist, and git push *would* push to it, on the basis of the new "push.autosetupremote=true" behavior.
In that very specific case, it's "wrong" to return an error just because the tracking relationship doesn't exist yet.
Show 9 quoted lines
>
> > + {
> > + const char *up;
> > + up = default_missing_upstream(remote, branch, err);
> > + if (up)
> > + return up;
> > + return branch_get_upstream(branch, err);
>
> ... shouldn't the precedence order the other way around here ...This is a bit convoluted, but I don't see how to make it more obvious.
The new "default_missing_upstream" function checks the *same conditions* - it only "kicks in" if "branch_get_upstream()" would return an error.
The reasons I can't add the new logic *after* or *in* "branch_get_upstream()", and avoid this non-obviousness, are that: * "branch_get_upstream()" raises an error, so recovering afterwards seems non-trivial (to me at least - I haven't got my head around the "idiom" yet) * "branch_get_upstream()" is used in a a few other places for other purposes, so I'm not comfortable modifying it for my purposes * "branch_get_upstream()" is non-trivial, so I'm not comfortable duplicating it / creating a new variant
I think I see one other approach that I could try and haven't attempted yet: * Adding an extra parameter to "branch_get_upstream()" for new behavior in these contexts
This might be easier to follow, even if it makes "branch_get_upstream()" itself more complex.
There's probably some clean refactor that I'm failing to see.
Show 20 quoted lines
>
> > + }
> > case PUSH_DEFAULT_UNSPECIFIED:
> > case PUSH_DEFAULT_SIMPLE:
> > {
> > const char *up, *cur;
> >
> > + up = default_missing_upstream(remote, branch, err);
> > + if (up)
> > + return up;
> > +
> > up = branch_get_upstream(branch, err);
> > if (!up)
> > return NULL;
>
> ... and here? That is, if branch_get_upstream() finds an explicitly
> configured one, shouldn't we use that and fall back to the new
> "missing" code path only when there isn't an explicitly configured
> one?
>Yep, same deal - by the time "branch_get_upstream()" has *not* found one, it's returned an error, and I haven't understood the side-effects of that in this project, if any.
Any advice on whether an already-prepared error can simply be discarded would be welcome!