| Message ID | 20181026230741.23321-8-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Add DWYM support for pushing a ref in refs/remotes/* when the <dst> I think most people call it do-what-*I*-mean, not do-what-you-mean. > ref is unqualified. Now instead of erroring out we support e.g.: > > $ ./git-push avar refs/remotes/origin/master:upstream-master -n > To github.com:avar/git.git > * [new branch] origin/master -> upstream-master > > Before this we wouldn't know what do do with > refs/remotes/origin/master, now we'll look it up and discover that > it's a commit (or tag) and add a refs/{heads,tags}/ prefix to <dst> as > appropriate. I am not sure if this is a good change, as I already hinted earlier. If I were doing "git push" to any of my publishing places, I would be irritated if "refs/remotes/ko/master:something" created a local "something" branch at the desitnation, instead of erroring out. 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).
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > This is the first use of the %N$<fmt> style of printf format in > the *.[ch] files in our codebase. It's supported by POSIX[2] and > there's existing uses for it in po/*.po files,... For now, I'll eject this from 'pu', as I had spent way too much time trying to make it and other topics work there. CC remote.o remote.c: In function 'show_push_unqualified_ref_name_error': remote.c:1035:2: error: $ operand number used after format without operand number [-Werror=format=] error(_("The destination you provided is not a full refname (i.e.,\n" ^~~~~ cc1: all warnings being treated as errors Makefile:2323: recipe for target 'remote.o' failed make: *** [remote.o] Error 1
Junio C Hamano <gitster@pobox.com> writes: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> This is the first use of the %N$<fmt> style of printf format in >> the *.[ch] files in our codebase. It's supported by POSIX[2] and >> there's existing uses for it in po/*.po files,... > > For now, I'll eject this from 'pu', as I had spent way too much time > trying to make it and other topics work there. > > CC remote.o > remote.c: In function 'show_push_unqualified_ref_name_error': > remote.c:1035:2: error: $ operand number used after format without operand number [-Werror=format=] > error(_("The destination you provided is not a full refname (i.e.,\n" > ^~~~~ > cc1: all warnings being treated as errors > Makefile:2323: recipe for target 'remote.o' failed > make: *** [remote.o] Error 1 I'll redo 'pu' with the following fix-up on top. remote.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/remote.c b/remote.c index c243e3d89e..1927c4b376 100644 --- a/remote.c +++ b/remote.c @@ -1035,8 +1035,8 @@ static void show_push_unqualified_ref_name_error(const char *dst_value, error(_("The destination you provided is not a full refname (i.e.,\n" "starting with \"refs/\"). We tried to guess what you meant by:\n" "\n" - "- Looking for a ref that matches '%s' on the remote side.\n" - "- Checking if the <src> being pushed ('%s')\n" + "- Looking for a ref that matches '%1$s' on the remote side.\n" + "- Checking if the <src> being pushed ('%2$s')\n" " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" " refs/{heads,tags}/ prefix on the remote side.\n" "- Checking if the <src> being pushed ('%2$s')\n"
On Mon, Oct 29 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> This is the first use of the %N$<fmt> style of printf format in >> the *.[ch] files in our codebase. It's supported by POSIX[2] and >> there's existing uses for it in po/*.po files,... > > For now, I'll eject this from 'pu', as I had spent way too much time > trying to make it and other topics work there. I was compiling with DEVELOPER=1 but as it turns out: CFLAGS="-O0" DEVELOPER=1 Wasn't doing what I thought, i.e. we just take 'CFLAGS' from the command-line and don't add any of the DEVELOPER #leftoverbits to it. Will fix this and other issues raised. > CC remote.o > remote.c: In function 'show_push_unqualified_ref_name_error': > remote.c:1035:2: error: $ operand number used after format without operand number [-Werror=format=] > error(_("The destination you provided is not a full refname (i.e.,\n" > ^~~~~ > cc1: all warnings being treated as errors > Makefile:2323: recipe for target 'remote.o' failed > make: *** [remote.o] Error 1 Will fix this and other issues raised. FWIW clang gives a much better error about the actual issue: remote.c:1042:46: error: cannot mix positional and non-positional arguments in format string [-Werror,-Wformat] "- Checking if the <src> being pushed ('%2$s')\n" I.e. this on top fixes it: - "- Looking for a ref that matches '%s' on the remote side.\n" - "- Checking if the <src> being pushed ('%s')\n" + "- Looking for a ref that matches '%1$s' on the remote side.\n" + "- Checking if the <src> being pushed ('%2$s')\n" Maybe this whole thing isn't worth it and I should just do: @@ -1042 +1042 @@ static void show_push_unqualified_ref_name_error(const char *dst_value, - "- Checking if the <src> being pushed ('%2$s')\n" + "- Checking if the <src> being pushed ('%s')\n" @@ -1047 +1047 @@ static void show_push_unqualified_ref_name_error(const char *dst_value, - dst_value, matched_src_name); + dst_value, matched_src_name, matched_src_name); But I'm leaning on the side of keeping it for the self-documentation aspect of "this is a repeated parameter". Your objections to this whole thing being a stupid idea non-withstanding.
On Mon, Oct 29 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Add DWYM support for pushing a ref in refs/remotes/* when the <dst> > > I think most people call it do-what-*I*-mean, not do-what-you-mean. FWIW I picked this up from the perl list where both are used depending on context. I.e. DW[IY]M depending on if the sentence is one where you'd describe things from a first or second-person perspective "I implemented this to DWIM, and it'll DWYM if you invoke it as...". Also "dwimerry"[1]. >> ref is unqualified. Now instead of erroring out we support e.g.: >> >> $ ./git-push avar refs/remotes/origin/master:upstream-master -n >> To github.com:avar/git.git >> * [new branch] origin/master -> upstream-master >> >> Before this we wouldn't know what do do with >> refs/remotes/origin/master, now we'll look it up and discover that >> it's a commit (or tag) and add a refs/{heads,tags}/ prefix to <dst> as >> appropriate. > > I am not sure if this is a good change, as I already hinted earlier. > If I were doing "git push" to any of my publishing places, I would > be irritated if "refs/remotes/ko/master:something" created a local > "something" branch at the desitnation, instead of erroring out. > > 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. Of course it isn't actually a "branch", but just a commit object (depending on the refspec configuration), so we can't quite make that hard assumption. But I figured screwing with how refs/remotes/* works by manually adding new refspecs is such an advanced feature that the people doing it are probably all here on-list, and would be the sort of users advanced enough to either know the semantics or try this with -n. Whereas the vast majority of users won't ever screw with it, and if they ever push from remotes/* to another remote almost definitely want a new branch under the new name. 1. https://www.urbandictionary.com/define.php?term=DWYM
Æ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). 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.
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): > Æ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. > 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.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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. I do not think there is *any* disagreement between us that refs/remotes/* is where somebody else's branches are stored. The fundamental difference between us is what this "push" is doing. You are limiting yourself to a single use case where you push to publish to the remote repository, as if the work on somebody else's commit were exactly like your own, hence you believe the push should go to refs/heads/* of the receiving repository. I am also allowing an additional use case I often have to use, where I emulate a fetch done at the receiving repository by pushing into it. I have refs/remotes/* by fetching somebody else's branches locally, and instead of doing the same fetch over there by logging into it and doing "git fetch" interactively, I push refs/remotes/* I happen to have locally into refs/remotes/* there. And I happen to think both are equally valid workflows, and there is no strong reason to think everybody would intuitively expect a push of my local refs/remotes/* will go to refs/heads/* of the receiving repository like you do. I'd rather want to make sure we do not make wrong guesses and force the user to "recover". > 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. That is insufficient as long as you do not consider refs/remotes/* as one of the equally valid possibilities that sits next to refs/heads and res/tags/.
diff --git a/remote.c b/remote.c index 93f802509d..c243e3d89e 100644 --- a/remote.c +++ b/remote.c @@ -973,6 +973,21 @@ static char *guess_ref(const char *name, struct ref *peer) strbuf_addstr(&buf, "refs/heads/"); } else if (starts_with(r, "refs/tags/")) { strbuf_addstr(&buf, "refs/tags/"); + } else if (starts_with(r, "refs/remotes/")) { + struct object_id oid; + enum object_type type; + + if (get_oid(peer->name, &oid)) + BUG("'%s' is not a valid object, " + "match_explicit_lhs() should catch this!", + peer->name); + type = oid_object_info(the_repository, &oid, NULL); + if (type == OBJ_COMMIT) + strbuf_addstr(&buf, "refs/heads/"); + else if (type == OBJ_TAG) + strbuf_addstr(&buf, "refs/tags/"); + else + return NULL; } else { return NULL; } @@ -1024,8 +1039,11 @@ static void show_push_unqualified_ref_name_error(const char *dst_value, "- Checking if the <src> being pushed ('%s')\n" " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" " refs/{heads,tags}/ prefix on the remote side.\n" + "- Checking if the <src> being pushed ('%2$s')\n" + " is a commit or tag in \"refs/remotes/*\". Then we infer a\n" + " corresponding refs/{heads,tags} on the remote side.\n" "\n" - "Neither worked, so we gave up. You must fully-qualify the ref."), + "None of those worked, so we gave up. You must fully-qualify the ref."), dst_value, matched_src_name); if (!advice_push_unqualified_ref_name) diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 979a13b415..a6337b50e4 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1260,11 +1260,15 @@ test_expect_success 'refs/remotes/* <src> refspec and unqualified <dst> DWIM and git config --add remote.two.fetch "+refs/blobs/*:refs/remotes/two-blobs/*" && git fetch --no-tags two && - test_must_fail git push origin refs/remotes/two/another:dst 2>err && - test_i18ngrep "error: The destination you" err && - - test_must_fail git push origin refs/remotes/two-tags/some-tag:dst-tag 2>err && - test_i18ngrep "error: The destination you" err && + echo commit >expected && + git push origin refs/remotes/two/another:dst && + git -C ../one cat-file -t refs/heads/dst >actual && + test_cmp expected actual && + + echo tag >expected && + git push origin refs/remotes/two-tags/some-tag:dst-tag && + git -C ../one cat-file -t refs/tags/dst-tag >actual && + test_cmp expected actual && test_must_fail git push origin refs/remotes/two-trees/my-head-tree:dst-tree 2>err && test_i18ngrep "error: The destination you" err &&
Add DWYM support for pushing a ref in refs/remotes/* when the <dst> ref is unqualified. Now instead of erroring out we support e.g.: $ ./git-push avar refs/remotes/origin/master:upstream-master -n To github.com:avar/git.git * [new branch] origin/master -> upstream-master Before this we wouldn't know what do do with refs/remotes/origin/master, now we'll look it up and discover that it's a commit (or tag) and add a refs/{heads,tags}/ prefix to <dst> as appropriate. The error message emitted when we still don't know what to do has been amended to note that this is something we tried: $ ./git-push avar v2.19.0^{commit}:newbranch -n error: The destination you provided is not a full refname (i.e., starting with "refs/"). We tried to guess what you meant by: - Looking for a ref that matches 'newbranch' on the remote side. - Checking if the <src> being pushed ('v2.19.0^{commit}') is a ref in "refs/{heads,tags}/". If so we add a corresponding refs/{heads,tags}/ prefix on the remote side. - Checking if the <src> being pushed ('v2.19.0^{commit}') is a commit or tag in "refs/remotes/*". Then we infer a corresponding refs/{heads,tags} on the remote side. None of those worked, so we gave up. You must fully-qualify the ref. hint: The <src> part of the refspec is a commit object. hint: Did you mean to create a new branch by pushing to hint: 'v2.19.0^{commit}:refs/heads/newbranch'? I'm bending over backwards and assuming that someone might have hacked in remote tracking tags (see [1] for a discussion of how that can be done), but punting on any tree or blob objects found under refs/remotes/*. This is the first use of the %N$<fmt> style of printf format in the *.[ch] files in our codebase. It's supported by POSIX[2] and there's existing uses for it in po/*.po files, so hopefully it won't cause any trouble. It's more obvious for translators than to have a 3rd argument to the function identical to the 2nd, by re-using the 2nd it's clear that we're continuing to talk about the <src> part of the refspec. 1. https://public-inbox.org/git/87zi1jxjqn.fsf@evledraar.gmail.com/ 2. http://pubs.opengroup.org/onlinepubs/7908799/xsh/fprintf.html Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 20 +++++++++++++++++++- t/t5505-remote.sh | 14 +++++++++----- 2 files changed, 28 insertions(+), 6 deletions(-)