Re: [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>"

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 5, 2018, 11:31 UTC
Message-ID: <87k1lr7obd.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqy3adtopw.fsf@gitster-ct.c.googlers.com>

I'll re-roll this. Hopefully sooner than later. I'll leave out the later part of this series as it's more controversial and we can discuss that later on its own. Meanwhile just some replies to this (while I remember):

Show 40 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> On the other hand, I do not think I mind all that much if a src that
>>> is a tag object to automatically go to refs/tags/ (having a tag
>>> object in refs/remotes/** is rare enough to matter in the first
>>> place).
>>
>> Yeah maybe this is going too far. I don't think so, but happy to me
>> challenged on that point :)
>>
>> I don't think so because the only reason I've ever needed this is
>> because I deleted some branch accidentally and am using a push from
>> "remotes/*" to bring it back. I.e. I'll always want branch-for-branch,
>> not to push that as a tag.
>
> Oh, I didn't consider pushing it out as a tag, but now you bring it
> up, I think that it also would make sense in a workflow to tell your
> colleages to look at (sort of like how people use pastebin---"look
> here, this commit has the kind of change I have in mind in this
> discussion") some random commit and the commit happens to be sitting
> at a tip of a remote-trackig branch.  Instead of pushing it out as a
> branch or a remote-tracking branch, which has strong connotations of
> inviting others to build on top, pushing it out as a tag would make
> more sense in that context.
>
> And as I mentioned already, I think it would equally likely, if not
> more likely, for people like me to push remotes/** out as a
> remote-tracking branch (rather than a local branch) of the
> repository I'm pushing into.
>
> So I tend to agree that this is going too far.  If the original
> motivating example was not an ingredient of everyday workflow, but
> was an one-off "recovery", I'd rather see people forced to be more
> careful by requiring "push origin/frotz:refs/heads/frotz" rather
> than incorrectly DWIDNM "push origin/frotz:frotz" and ending up with
> creating refs/tags/frotz or refs/remotes/origin/frotz, which also
> are plausible choices depending on what the user is trying to
> recover from, which the sending end would not know (the side on
> which the accidental loss of a ref happened earlier is on the remote
> repository that would be receiving this push, and it _might_ know).

Yeah this example was bad, but now since I've written it I've become more convinced that it's the right way to go, just that I didn't explain it well.

E.g. just now I did:

    git push avar avar/some-branch:some-branch-wip
    git push avar HEAD -f # I was on 'some-branch'

Because I'd regretted taking the "some-branch" name with some WIP code, but didn't want to lose the work, so I wanted to swap.

It's also arbitrary and contrary to the distributed nature of git that we're treating branches from other repos differently than the ones from ours.

After all sometimes "other" is just the repo on my laptop or server. I shouldn't need to jump through hoops to re-push stuff from my "other" repo anymore than from the local repo.

Yes refs/remotes/* isn't guaranteed to be "other repo's branches" in the same way our local refs/heads/* is, and this series bends over backwards to check if someone's (ab)used it for that, but I think for the purposes of the default UI we should treat it as "branches from other remotes" if we find commit objects there.

So I think the *only* thing we should be checking for the purposes of this DWYM feature is what local type of object (branch or tag) we find on the LHS of the refspec. And if we're sure of its type push to refs/{heads,tags}/* as appropriate.

I don't think it makes any sense as you suggest to move away from "guess based on existing type" to breaking the dwimmery because we found the branch in some other place. The user's pushing "newref" from an existing branch, let's just assume the new thing is a branch, whether the source is local or not.

Show 15 quoted lines

> As to the entirety of the series,
>
>  - I do not think this step 7, and its documentation in step 8, are
>    particularly a good idea, in their current shape.  Pushing tag
>    objects to refs/tags/ is probably a good idea, but pushing a
>    commit as local branch heads are necessarily not.
>
>  - Step 6 is probably a good documentation on the cases in which we
>    make and do not make guess on the unqualified push destination.
>
>  - Step 5 and earlier looked like good changes.
>
> If we were to salvage some parts of step 7 (and step 8), we'd
> probably need fb7c2268 ("SQUASH???", 2018-10-29) to number all the
> placeholders in the printf format string.

Thanks. As noted will try to re-roll this soon.