On Sun, Jul 10, 2022 at 10:42 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Tao Klerks via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
Thanks so much for looking at this, my apologies for the delayed response.
>
> > + 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.
>
> > + {
> > + 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.
>
> > + }
> > 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!
