From: Tao Klerks <tao@klerks.biz>
To: Junio C Hamano <gitster@pobox.com>
Cc: Tao Klerks via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Subject: Re: [PATCH] rev-parse: respect push.autosetupremote when evaluating @{push}
Date: Sat, 16 Jul 2022 19:40:21 +0200 [thread overview]
Message-ID: <CAPMMpoirWDBJn45dkWo8CRDU23G1mnyPDHcKq1eH1AtACEW9Rg@mail.gmail.com> (raw)
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.
>
> > + 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!
next prev parent reply other threads:[~2022-07-16 17:40 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-10 19:16 [PATCH] rev-parse: respect push.autosetupremote when evaluating @{push} Tao Klerks via GitGitGadget
2022-07-10 20:42 ` Junio C Hamano
2022-07-16 17:40 ` Tao Klerks [this message]
2023-05-28 9:58 ` [PATCH v2] " Tao Klerks via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPMMpoirWDBJn45dkWo8CRDU23G1mnyPDHcKq1eH1AtACEW9Rg@mail.gmail.com \
--to=tao@klerks.biz \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).