[PATCH v6 0/8] push: update remote tags only with force
This patch series originated in response to the following thread: http://thread.gmane.org/gmane.comp.version-control.git/208354 I made some adjustments based on Junio's last round of feedback including a new patch reworking the "push rules" comment in remote.c. Also refined some of the log messages--nothing major. Finally, took a stab at putting something together for the release notes, see below. Chris Release notes: "git push" no longer updates tags (lightweight or annotated) by default. Specifically, if the destination reference already exists and is under refs/tags/ or it points to a tag object, it is not allowed to fast- forward (unless forced using +A:B notation or by passing --force.) This is consistent with how a tag is normally thought of: a reference that does not move once defined. It also ensures a push will not inadvertently clobber an already existing tag--something that can go unnoticed if fast-forwarding is allowed. Chris Rorvick (8): push: return reject reasons as a bitset push: add advice for rejected tag reference push: flag updates push: flag updates that require force push: require force for refs under refs/tags/ push: require force for annotated tags push: clarify rejection of update to non-commit-ish push: cleanup push rules comment Documentation/git-push.txt | 9 ++--- builtin/push.c | 24 +- builtin/send-pack.c| 9 +++-- cache.h| 7 +++- remote.c | 83 +++--- send-pack.c| 1 + t/t5516-fetch-push.sh | 44 +++- transport-helper.c | 6 transport.c| 25 -- transport.h| 10 +++--- 10 files changed, 167 insertions(+), 51 deletions(-) -- 1.8.0.158.g0c4328c -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
Thanks; will queue. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
Hi there, I was just working on improving git-remote-helper.txt by documenting how remote helper can signal error conditions to git. This lead me to notice a (to me) surprising change in behavior between master and next that I traced back to this patch series. Specifically: On 30.11.2012, at 02:41, Chris Rorvick wrote: > This patch series originated in response to the following thread: > > http://thread.gmane.org/gmane.comp.version-control.git/208354 > > I made some adjustments based on Junio's last round of feedback > including a new patch reworking the "push rules" comment in remote.c. > Also refined some of the log messages--nothing major. Finally, took a > stab at putting something together for the release notes, see below. >From the discussion in that gmane thread and from the commits in this series, >I had the impression that it should mostly affect pushing tags. However, this >is not the case: It also changes messages upon regular push "conflicts. >Consider this test script: #!/bin/sh -ex git init repo_orig cd repo_orig echo a > a git add a git commit -m a cd .. git clone repo_orig repo_clone cd repo_orig echo b > b git add b git commit -m b cd .. cd repo_clone echo B > b git add b git commit -m B git push With git 1.8.1, I get this message: ! [rejected]master -> master (non-fast-forward) error: failed to push some refs to '/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig' hint: Updates were rejected because the tip of your current branch is behind hint: its remote counterpart. Merge the remote changes (e.g. 'git pull') hint: before pushing again. hint: See the 'Note about fast-forwards' in 'git push --help' for details. But with next, I get this: ! [rejected]master -> master (already exists) error: failed to push some refs to '/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig' hint: Updates were rejected because the destination reference already exists hint: in the remote. This looks like a regression to me. No tags were involve, and the new message is very confusing if not outright wrong -- at least in my mind, but perhaps I am missing a way to interpret it "correctly" ? What am I missing? Cheers, Max -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
Max Horn writes: > But with next, I get this: > > > ! [rejected]master -> master (already exists) > error: failed to push some refs to '/Users/mhorn/Proje...o_orig' > hint: Updates were rejected because the destination reference already exists > hint: in the remote. > > This looks like a regression to me. It is an outright bug. The new helper function is_forwrdable() is bogus to assume that both original and updated objects can be locally inspected, but you do not necessarily have the original locally. remote.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/remote.c b/remote.c index aa6b719..4a253ef 100644 --- a/remote.c +++ b/remote.c @@ -1286,9 +1286,12 @@ static inline int is_forwardable(struct ref* ref) if (!prefixcmp(ref->name, "refs/tags/")) return 0; - /* old object must be a commit */ + /* +* old object must be a commit, but we may be forcing +* without having it in the first place! +*/ o = parse_object(ref->old_sha1); - if (!o || o->type != OBJ_COMMIT) + if (o && o->type != OBJ_COMMIT) return 0; /* new object must be commit-ish */ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
Junio C Hamano writes: > Max Horn writes: > >> But with next, I get this: >> >> ! [rejected]master -> master (already exists) >> error: failed to push some refs to '/Users/mhorn/Proje...o_orig' >> hint: Updates were rejected because the destination reference already exists >> hint: in the remote. >> >> This looks like a regression to me. > > It is an outright bug. The new helper function is_forwrdable() is > bogus to assume that both original and updated objects can be > locally inspected, but you do not necessarily have the original > locally. The way the caller uses the result of this function is equally questionable. If this function says "we do not want to let this push go through", it translates that unconditionally into "we blocked it because the destination already exists". It is fine when pushing into "refs/tags/" hierarchy. It is *NOT* OK if the type check does not satisfy this function. In that case, we do not actually see the existence of the destination as a problem, but it is reported as such. We are blocking because we do not like the type of the new object or the type of the old object. If the destination points at a commit, the push can succeed if the user changes what object to push, so saying "you cannot push because the destination already exists" is just wrong in such a case. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
On Wed, Jan 16, 2013 at 02:32:03PM +0100, Max Horn wrote: > With git 1.8.1, I get this message: > > ! [rejected]master -> master (non-fast-forward) > [...] > But with next, I get this: > > ! [rejected]master -> master (already exists) Thanks for the detailed report. I was able to reproduce easily here. The problem is the logic in is_forwardable: static inline int is_forwardable(struct ref* ref) { struct object *o; if (!prefixcmp(ref->name, "refs/tags/")) return 0; /* old object must be a commit */ o = parse_object(ref->old_sha1); if (!o || o->type != OBJ_COMMIT) return 0; /* new object must be commit-ish */ o = deref_tag(parse_object(ref->new_sha1), NULL, 0); if (!o || o->type != OBJ_COMMIT) return 0; return 1; } The intent is to allow fast-forward only between objects that both point to commits eventually. But we are doing this check on the client, which does not necessarily have the object for ref->old_sha1 at all. So it cannot know the type, and cannot enforce this condition accurately. I.e., we trigger the "!o" branch after the parse_object in your example. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
Max Horn writes: > But with next, I get this: > > ! [rejected]master -> master (already exists) > error: failed to push some refs to > '/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig' > hint: Updates were rejected because the destination reference already exists > hint: in the remote. > > This looks like a regression to me. It is in master now X-<, and this looks like a bug to me. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
Jeff King writes: > I.e., we trigger the "!o" branch after the parse_object in your example. Heh, I didn't see this message until now (gmane seems to be lagging a bit). I am very tempted to do this. * Remove unnecessary not_forwardable from "struct ref"; it is only used inside set_ref_status_for_push(); * "refs/tags/" is the only hierarchy that cannot be replaced without --force; * Remove the misguided attempt to force that everything that updates an existing ref has to be a commit outside "refs/tags/" hierarchy. This code does not know what kind of objects the user wants to place in "refs/frotz/" hierarchy it knows nothing about. I feel moderately strongly about the last point. Defining special semantics for one hierarchy (e.g. "refs/tags/") and implementing a policy for enforcement is one thing, but a random policy that depends on object type that applies globally is simply insane. The user may want to do "refs/tested/" hierarchy that is meant to hold references to commit, with one annotated tag "refs/tested/latest" that points at the "latest tested version" with some commentary, and maintain the latter by keep pushing to it. If that is the semantics the user wanted to ahve in the "refs/tested/" hierarchy, it is not reasonable to require --force for such a workflow. The user knows better than Git in such a case. cache.h | 1 - remote.c | 24 +--- t/t5516-fetch-push.sh | 21 - 3 files changed, 1 insertion(+), 45 deletions(-) diff --git a/cache.h b/cache.h index a32a0ea..a942bbd 100644 --- a/cache.h +++ b/cache.h @@ -1004,7 +1004,6 @@ struct ref { requires_force:1, merge:1, nonfastforward:1, - not_forwardable:1, update:1, deletion:1; enum { diff --git a/remote.c b/remote.c index aa6b719..2c747c4 100644 --- a/remote.c +++ b/remote.c @@ -1279,26 +1279,6 @@ int match_push_refs(struct ref *src, struct ref **dst, return 0; } -static inline int is_forwardable(struct ref* ref) -{ - struct object *o; - - if (!prefixcmp(ref->name, "refs/tags/")) - return 0; - - /* old object must be a commit */ - o = parse_object(ref->old_sha1); - if (!o || o->type != OBJ_COMMIT) - return 0; - - /* new object must be commit-ish */ - o = deref_tag(parse_object(ref->new_sha1), NULL, 0); - if (!o || o->type != OBJ_COMMIT) - return 0; - - return 1; -} - void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, int force_update) { @@ -1344,8 +1324,6 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, * passing the --force argument */ - ref->not_forwardable = !is_forwardable(ref); - ref->update = !ref->deletion && !is_null_sha1(ref->old_sha1); @@ -1355,7 +1333,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, !has_sha1_file(ref->old_sha1) || !ref_newer(ref->new_sha1, ref->old_sha1); - if (ref->not_forwardable) { + if (!prefixcmp(ref->name, "refs/tags/")) { ref->requires_force = 1; if (!force_ref_update) { ref->status = REF_STATUS_REJECT_ALREADY_EXISTS; diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 6009372..8f024a0 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -950,27 +950,6 @@ test_expect_success 'push requires --force to update lightweight tag' ' ) ' -test_expect_success 'push requires --force to update annotated tag' ' - mk_test heads/master && - mk_child child1 && - mk_child child2 && - ( - cd child1 && - git tag -a -m "message 1" Tag && - git push ../child2 Tag:refs/tmp/Tag && - git push ../child2 Tag:refs/tmp/Tag && - >file1 && - git add file1 && - git commit -m "file1" && - git tag -f -a -m "message 2" Tag && - test_must_fail git push ../child2 Tag:refs/tmp/Tag && - git push --force ../child2 Tag:refs/tmp/Tag && - git tag -f -a -m "message 3" Tag HEAD~ && - test_must_fail git push ../child2 Tag:refs/tmp/Tag && - git push --force ../child2 Tag:refs/tmp/Tag - ) -' - test_expect_success 'push --porcelain' ' mk_empty && echo >.git/foo "To testrepo" && -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
On Wed, Jan 16, 2013 at 09:10:10AM -0800, Junio C Hamano wrote: > Jeff King writes: > > > I.e., we trigger the "!o" branch after the parse_object in your example. > > Heh, I didn't see this message until now (gmane seems to be lagging > a bit). I think it is vger lagging, actually. > I am very tempted to do this. > > * Remove unnecessary not_forwardable from "struct ref"; it is only >used inside set_ref_status_for_push(); > > * "refs/tags/" is the only hierarchy that cannot be replaced >without --force; Agreed. > * Remove the misguided attempt to force that everything that >updates an existing ref has to be a commit outside "refs/tags/" >hierarchy. This code does not know what kind of objects the user >wants to place in "refs/frotz/" hierarchy it knows nothing about. I agree with what your patch does, but my thinking is a bit different. My original suggestion with respect to object types was that the rule for --force should be "do not ever lose any objects without --force". So a fast-forward is OK, as the new objects reference the old. A non-fast forward is not, because objects become unreferenced. Replacing a tag object is not OK, even if it points to the same commit, as you are losing the old tag object (replacing an object with a tag that points to the original object or its descendent is OK in theory, though I doubt it is common enough to worry about). I think that is a reasonable rule that could be applied across all parts of the namespace hierarchy. And it could be applied by the client, because all you need to know is whether ref->old_sha1 is reachable from ref->new_sha1. But it is somewhat orthogonal to the "already exists" idea, and checking refs/tags/. Those ideas are about enforcing sane rules on the tag hierarchy. My rule is a safety valve that is meant to extend the idea of "is fast-forwardable" to non-commit object types. If we do it at all, it should be part of the fast-forward check (e.g., as part of ref_newer). The current code conflates the two under the "already exists" condition, which is just wrong. I think the best thing at this point is to split the two ideas apart, keep the refs/tags check (and translate it to "already exists" in the UI, as we do), and table the safety valve. I am not even sure if it is something that is useful, and it can come later if we decide it is. > I feel moderately strongly about the last point. Defining special > semantics for one hierarchy (e.g. "refs/tags/") and implementing a > policy for enforcement is one thing, but a random policy that > depends on object type that applies globally is simply insane. The > user may want to do "refs/tested/" hierarchy that is meant to hold > references to commit, with one annotated tag "refs/tested/latest" > that points at the "latest tested version" with some commentary, and > maintain the latter by keep pushing to it. If that is the semantics > the user wanted to ahve in the "refs/tested/" hierarchy, it is not > reasonable to require --force for such a workflow. The user knows > better than Git in such a case. I see what you are saying, but I think the ship has already sailed to some degree. We already implement the non-fast-forward check everywhere, and I cannot have a "refs/tested" hierarchy that pushes arbitrary commits without regard to their history. If I have such a hierarchy, I have to use "--force" (or more likely, mark the refspec with "+"). In my mind, the object-type checking is just making that fast-forward check more thorough (i.e., extending it to non-commit objects). > cache.h | 1 - > remote.c | 24 +--- > t/t5516-fetch-push.sh | 21 - > 3 files changed, 1 insertion(+), 45 deletions(-) The patch itself looks fine to me. Whether we agree on the fast-forward object-type checking or not, it is the correct first step to take in either case. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
Jeff King writes: > I see what you are saying, but I think the ship has already sailed to > some degree. We already implement the non-fast-forward check everywhere, > and I cannot have a "refs/tested" hierarchy that pushes arbitrary > commits without regard to their history. If I have such a hierarchy, I > have to use "--force" (or more likely, mark the refspec with "+"). Yeah, actually in that example, I meant refs/tested/ would have pointers to bare tree objects. I often rebuild 'pu' and another private integration branch for testing, reordering the series that are still not in 'next' and also after rewriting log messages of some commits. It is not unusual to end up the updated 'pu' having the identical tree as 'pu' before update, and I want to skip testing the result (tree equality matters while commit equality does not in such a use case). > In my mind, the object-type checking is just making that fast-forward > check more thorough (i.e., extending it to non-commit objects). Yes, I agree with that point of view. Thanks. Here is what I am planning to queue (the patch is the same, but the message is different on the third point). -- >8 -- Subject: [PATCH] push: fix "refs/tags/ hierarchy cannot be updated without --force" When pushing to update a branch with a commit that is not a descendant of the commit at the tip, a wrong message "already exists" was given, instead of the correct "non-fast-forward", if we do not have the object sitting in the destination repository at the tip of the ref we are updating. The primary cause of the bug is that the check in a new helper function is_forwardable() assumed both old and new objects are available and can be checked, which is not always the case. The way the caller uses the result of this function is also wrong. If the helper says "we do not want to let this push go through", the caller unconditionally translates it into "we blocked it because the destination already exists", which is not true at all in this case. Fix this by doing these three things: * Remove unnecessary not_forwardable from "struct ref"; it is only used inside set_ref_status_for_push(); * Make "refs/tags/" the only hierarchy that cannot be replaced without --force; * Remove the misguided attempt to force that everything that updates an existing ref has to be a commit outside "refs/tags/" hierarchy. The policy last one tried to implement may later be resurrected and extended to ensure fast-forwardness (defined as "not losing objects", extending from the traditional "not losing commits from the resulting history") when objects that are not commit are involved (e.g. an annotated tag in hierarchies outside refs/tags), but such a logic belongs to "is this a fast-forward?" check that is done by ref_newer(); is_forwardable(), which is now removed, was not the right place to do so. Signed-off-by: Junio C Hamano --- cache.h | 1 - remote.c | 43 +++ t/t5516-fetch-push.sh | 21 - 3 files changed, 7 insertions(+), 58 deletions(-) diff --git a/cache.h b/cache.h index a32a0ea..a942bbd 100644 --- a/cache.h +++ b/cache.h @@ -1004,7 +1004,6 @@ struct ref { requires_force:1, merge:1, nonfastforward:1, - not_forwardable:1, update:1, deletion:1; enum { diff --git a/remote.c b/remote.c index aa6b719..d3a1ca2 100644 --- a/remote.c +++ b/remote.c @@ -1279,26 +1279,6 @@ int match_push_refs(struct ref *src, struct ref **dst, return 0; } -static inline int is_forwardable(struct ref* ref) -{ - struct object *o; - - if (!prefixcmp(ref->name, "refs/tags/")) - return 0; - - /* old object must be a commit */ - o = parse_object(ref->old_sha1); - if (!o || o->type != OBJ_COMMIT) - return 0; - - /* new object must be commit-ish */ - o = deref_tag(parse_object(ref->new_sha1), NULL, 0); - if (!o || o->type != OBJ_COMMIT) - return 0; - - return 1; -} - void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, int force_update) { @@ -1320,32 +1300,23 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, } /* -* The below logic determines whether an individual -* refspec A:B can be pushed. The push will succeed -* if any of the following are true: +* Decide whether an individual refspec A:B can be +* pushed. The push will succeed if any of the +* following are true: * * (1) the remote reference B does not exist * * (2) the remote reference B is being removed (i.e., * pushing :B where no source is specified) * -*
Re: [PATCH v6 0/8] push: update remote tags only with force
On Wed, Jan 16, 2013 at 11:43 AM, Jeff King wrote: > I think that is a reasonable rule that could be applied across all parts > of the namespace hierarchy. And it could be applied by the client, > because all you need to know is whether ref->old_sha1 is reachable from > ref->new_sha1. is_forwardable() did solve a UI issue. Previously all instances where old is not reachable by new were assumed to be addressable with a merge. is_forwardable() attempted to determine if the concept of forwarding made sense given the inputs. For example, if old is a blob it is useless to suggest merging it. Chris -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
On Wed, Jan 16, 2013 at 08:19:28PM -0600, Chris Rorvick wrote: > On Wed, Jan 16, 2013 at 11:43 AM, Jeff King wrote: > > I think that is a reasonable rule that could be applied across all parts > > of the namespace hierarchy. And it could be applied by the client, > > because all you need to know is whether ref->old_sha1 is reachable from > > ref->new_sha1. > > is_forwardable() did solve a UI issue. Previously all instances where > old is not reachable by new were assumed to be addressable with a > merge. is_forwardable() attempted to determine if the concept of > forwarding made sense given the inputs. For example, if old is a blob > it is useless to suggest merging it. I think it makes sense to mark such a case as different from a regular non-fast-forward (because "git pull" is not the right advice), but: 1. is_forwardable should assume a missing object is a commit not to regress the common case; otherwise we do not show the pull advice when we probably should, and most of the time it is going to be a commit 2. When we know that we are not working with commits, I am not sure that "already exists" is the right advice to give for such a case. It is neither "this tag already exists, so we do not update it", nor is it strictly "cannot fast forward this commit", but rather something else. The expanded definition of "what is a fast forward" that I suggested would let this fall naturally between the two. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
On Wed, Jan 16, 2013 at 9:11 PM, Jeff King wrote: >> is_forwardable() did solve a UI issue. Previously all instances where >> old is not reachable by new were assumed to be addressable with a >> merge. is_forwardable() attempted to determine if the concept of >> forwarding made sense given the inputs. For example, if old is a blob >> it is useless to suggest merging it. > > I think it makes sense to mark such a case as different from a regular > non-fast-forward (because "git pull" is not the right advice), but: > > 1. is_forwardable should assume a missing object is a commit not to > regress the common case; otherwise we do not show the pull advice > when we probably should, and most of the time it is going to be a > commit Yes, obviously this was a bug, thus the use of "attempted" above. It would have been better to assume a missing 'old' was potentially forwardable to present the user with the most helpful advice. > 2. When we know that we are not working with commits, I am not sure > that "already exists" is the right advice to give for such a case. > It is neither "this tag already exists, so we do not update it", > nor is it strictly "cannot fast forward this commit", but rather > something else. But the reference already existing in the remote is a substantial reason for not allowing the push in all of these cases. You can break this out further if you like to explain why the specific reference shouldn't be moved on the remote, but this is even more complicated a simple "is old reachable from new?" test. Chris -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano wrote: > It is fine when pushing into "refs/tags/" hierarchy. It is *NOT* > OK if the type check does not satisfy this function. In that case, > we do not actually see the existence of the destination as a > problem, but it is reported as such. We are blocking because we do > not like the type of the new object or the type of the old object. > If the destination points at a commit, the push can succeed if the > user changes what object to push, so saying "you cannot push because > the destination already exists" is just wrong in such a case. So the solution is to revert back to recommending a merge? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
Chris Rorvick writes: > On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano wrote: >> It is fine when pushing into "refs/tags/" hierarchy. It is *NOT* >> OK if the type check does not satisfy this function. In that case, >> we do not actually see the existence of the destination as a >> problem, but it is reported as such. We are blocking because we do >> not like the type of the new object or the type of the old object. >> If the destination points at a commit, the push can succeed if the >> user changes what object to push, so saying "you cannot push because >> the destination already exists" is just wrong in such a case. > > So the solution is to revert back to recommending a merge? Of course not, because at that point you may not even have what you were attempting to overwrite. Nobody says it is even something you could merge. The recommended solution certainly will involve a "fetch" (not "pull" or "pull --rebase"). You fetch from over there to check what you were about to overwrite, examine the situation to decide what the appropriate action is. The point is that Git in general, and the codepath that was touched by the patch in particular, does not have enough information to decide what the appropriate action is for the user, especially when the ref is outside the ones we know what the conventional uses of them are. We can make policy decisions like "tags are meant to be unmoving anchor points, so it is unusual to overwrite any old with any new", "heads are meant to be branch tips, and because rewinding them while more than one repositories are working with them will cause issues to other repositories, it is unusual to push a non-fast-forward" and enforcement mechanism for such policy decisions will help users, but that is only because we know what their uses are. The immediate action we should take is to get closer to the original behaviour of not complaining with "ref already exists", which is nonsensical. That does not mean that we will forbid improving the codepath by giving different advices depending on the case. One of the new advices could tell them to "fetch it and inspect the situation", if old is not something we do not even have (hence we cannot check its type, let alone the ancestry relationship of it with new), for example. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
On Thu, Jan 17, 2013 at 12:59 AM, Junio C Hamano wrote: > Chris Rorvick writes: > >> On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano wrote: >>> It is fine when pushing into "refs/tags/" hierarchy. It is *NOT* >>> OK if the type check does not satisfy this function. In that case, >>> we do not actually see the existence of the destination as a >>> problem, but it is reported as such. We are blocking because we do >>> not like the type of the new object or the type of the old object. >>> If the destination points at a commit, the push can succeed if the >>> user changes what object to push, so saying "you cannot push because >>> the destination already exists" is just wrong in such a case. >> >> So the solution is to revert back to recommending a merge? > > Of course not, because at that point you may not even have what you > were attempting to overwrite. Nobody says it is even something you > could merge. I was referring to your concern about rejecting based on type. A push causing a reference to move (for example) from a commit to a blob is rejected as "already exists" with this patch. You emphatically state this is not OK and your solution is to revert back to behavior that advises a merge. Clearly the bug regarding an 'old' unknown to the client should be fixed. This is a obvious test case I should have covered and it's unfortunate it made it into master. But I don't understand why is_forwardable() was misguided (maybe poorly named) nor why ref_newer() is a better place to solve the issues it was addressing. Chris -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
On Thu, Jan 17, 2013 at 07:09:16AM -0600, Chris Rorvick wrote: > I was referring to your concern about rejecting based on type. A push > causing a reference to move (for example) from a commit to a blob is > rejected as "already exists" with this patch. You emphatically state > this is not OK and your solution is to revert back to behavior that > advises a merge. > > Clearly the bug regarding an 'old' unknown to the client should be > fixed. This is a obvious test case I should have covered and it's > unfortunate it made it into master. But I don't understand why > is_forwardable() was misguided (maybe poorly named) nor why > ref_newer() is a better place to solve the issues it was addressing. I think that a type-based rule that relies on knowing the type of the other side will always have to guess in some cases, because we do not necessarily have that information. However, if instead of the rule being "blobs on the remote side cannot be replaced", if it becomes "the old value on the remote side must be referenced by what we replace it with", that _is_ something we can calculate reliably on the sending side. And that is logically an extension of the fast-forward rule, which is why I suggested placing it with ref_newer (but the latter should probably be extended to not suggest merging if we _know_ it is a non-commit object). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
On Thu, Jan 17, 2013 at 7:06 PM, Jeff King wrote: > However, if instead of the rule being > "blobs on the remote side cannot be replaced", if it becomes "the old > value on the remote side must be referenced by what we replace it with", > that _is_ something we can calculate reliably on the sending side. Interesting. I would have thought knowing reachability implied having the old object in the sending repository. > And > that is logically an extension of the fast-forward rule, which is why I > suggested placing it with ref_newer (but the latter should probably be > extended to not suggest merging if we _know_ it is a non-commit object). Sounds great, especially if it is not dependent on the sender actually having the old object. Until this is implemented, though, I don't understand what was wrong with doing the checks in the is_forwardable() helper function (of course after fixing the regression/bug.) Chris -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
Jeff King writes: > However, if instead of the rule being "blobs on the remote side > cannot be replaced", if it becomes "the old value on the remote > side must be referenced by what we replace it with", that _is_ > something we can calculate reliably on the sending side. And that > is logically an extension of the fast-forward rule,... It may be an extension of the fast-forward, but only in the graph reachability sense. I can buy that it is mathmatically consistent with the mode that has proven to be useful for commits at the branch tips, which we know why "fast-forward" rule is an appropriate default for. You haven't shown if that mathmatical consistency is useful for non-commit case. The primary reason "fast-forward" is a good default for branches is not that "we do not want to lose objects to gc" (you have reflog for that). The reason is non fast-forward is a sign of unintended rewind, and later will cause duplicated history with merge conflicts. That comes from the way objects pointed by refs/heads aka branches are used. It is not just "commit" (as object type), but how these objects are used. Think why we decided it was a good idea to do one thing in the topic that introduced the regression under discussion: "Even if the new commit is a descendant of the old commit, we do not want to fast-forward a ref if it is under refs/tags/". Type of object may be one factor, but how it is used is more important factor in deciding what kind of policy is appropriate. If users have workflows that want to have a ref hierarchy that point at a blob, there will not be any update to such a ref that will satisfy your definition of "extended" fast-forward requirement, and that requirement came solely from mathematical purity (i.e. graph reachability), not from any workflow consideration. That is very disturbing to me. A workflow that employes such a "blob at a ref" may perfectly be happy with replacing the blob as last-one-wins basis. I do not think the client side should enforce a policy to forbid such a push. I personally think the current client side that insists that updates to any ref has to have the current object and must fast-forward and requires --force otherwise was a mistake (this predates the change by Chris). The receiving end does not implement such an arbitrary restriction outside refs/heads/, and does so only for refs/heads/ only when deny-non-fast-forwards is set. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
On Thu, Jan 17, 2013 at 09:18:50PM -0600, Chris Rorvick wrote: > On Thu, Jan 17, 2013 at 7:06 PM, Jeff King wrote: > > However, if instead of the rule being > > "blobs on the remote side cannot be replaced", if it becomes "the old > > value on the remote side must be referenced by what we replace it with", > > that _is_ something we can calculate reliably on the sending side. > > Interesting. I would have thought knowing reachability implied having > the old object in the sending repository. No, because if you do not have it, then you know it is not reachable from your refs (or your repository is corrupted). If you do have it, it _might_ be reachable. For commits, checking is cheap (merge-base) and we already do it. For trees and blobs, it is much more expensive, as you have to walk the whole object graph. While it might be "more correct" in some sense to say "it's OK to replace a tree with a commit that points to it", in practice I doubt anyone cares, so you can probably just punt on those ones and say "no, it's not a fast forward". > > And > > that is logically an extension of the fast-forward rule, which is why I > > suggested placing it with ref_newer (but the latter should probably be > > extended to not suggest merging if we _know_ it is a non-commit object). > > Sounds great, especially if it is not dependent on the sender actually > having the old object. Until this is implemented, though, I don't > understand what was wrong with doing the checks in the > is_forwardable() helper function (of course after fixing the > regression/bug.) I don't think it is wrong per se; I just think that the check would go more naturally where we are checking whether the object does indeed fast-forward. Because is_forwardable in some cases must say "I don't know; I don't have the object to check its type, so maybe it is forwardable, and maybe it is not". Whereas when we do the actual reachability check, we can say definitely "this is not reachable because I don't have it, or this is not reachable because it is a commit and I checked, or this might be reachable but I don't care to check because it has a funny type". I think looking at it as the latter makes it more obvious how to handle the "maybe" situation (e.g., the bug in is_forwardable was hard to see). Anyway, I do not care that much where it goes. To me, the important thing is the error message. I do think the error "already exists" is a reasonable one for refs/tags (we do not allow non-force pushes of existing tags), but not necessarily for other cases, like trying to push a blob over a blob. The problem there is not "already exists" but rather "a blob is not something that can fast-forward". Using the existing REJECT_NONFASTFORWARD is insufficient (because later code will recommend pull-then-push, which is wrong). So I'd be in favor of creating a new error status for it. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
Jeff King writes: > ... The problem there is not "already exists" but rather > "a blob is not something that can fast-forward". Using the existing > REJECT_NONFASTFORWARD is insufficient (because later code will recommend > pull-then-push, which is wrong). So I'd be in favor of creating a new > error status for it. Very well said. Please make it so ;-) or should I? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
On Mon, Jan 21, 2013 at 5:40 PM, Jeff King wrote: > On Thu, Jan 17, 2013 at 09:18:50PM -0600, Chris Rorvick wrote: > >> On Thu, Jan 17, 2013 at 7:06 PM, Jeff King wrote: >> > However, if instead of the rule being >> > "blobs on the remote side cannot be replaced", if it becomes "the old >> > value on the remote side must be referenced by what we replace it with", >> > that _is_ something we can calculate reliably on the sending side. >> >> Interesting. I would have thought knowing reachability implied having >> the old object in the sending repository. > > No, because if you do not have it, then you know it is not reachable > from your refs (or your repository is corrupted). If you do have it, it > _might_ be reachable. For commits, checking is cheap (merge-base) and we > already do it. For trees and blobs, it is much more expensive, as you > have to walk the whole object graph. While it might be "more correct" > in some sense to say "it's OK to replace a tree with a commit that > points to it", in practice I doubt anyone cares, so you can probably > just punt on those ones and say "no, it's not a fast forward". Thanks for explaining this further. I'm not exactly sure what I was thinking when I wrote the above other than I didn't fully grasp you point and responded in a confused state. Clear on all fronts now. >> > And >> > that is logically an extension of the fast-forward rule, which is why I >> > suggested placing it with ref_newer (but the latter should probably be >> > extended to not suggest merging if we _know_ it is a non-commit object). >> >> Sounds great, especially if it is not dependent on the sender actually >> having the old object. Until this is implemented, though, I don't >> understand what was wrong with doing the checks in the >> is_forwardable() helper function (of course after fixing the >> regression/bug.) > > I don't think it is wrong per se; I just think that the check would go > more naturally where we are checking whether the object does indeed > fast-forward. Because is_forwardable in some cases must say "I don't > know; I don't have the object to check its type, so maybe it is > forwardable, and maybe it is not". Whereas when we do the actual > reachability check, we can say definitely "this is not reachable because > I don't have it, or this is not reachable because it is a commit and I > checked, or this might be reachable but I don't care to check because it > has a funny type". > > I think looking at it as the latter makes it more obvious how to handle > the "maybe" situation (e.g., the bug in is_forwardable was hard to see). > > Anyway, I do not care that much where it goes. To me, the important > thing is the error message. I do think the error "already exists" is a > reasonable one for refs/tags (we do not allow non-force pushes of > existing tags), but not necessarily for other cases, like trying to push > a blob over a blob. The problem there is not "already exists" but rather > "a blob is not something that can fast-forward". Using the existing > REJECT_NONFASTFORWARD is insufficient (because later code will recommend > pull-then-push, which is wrong). So I'd be in favor of creating a new > error status for it. I agree with everything above. I just don't understand why reverting the "already exists" behavior for non-commit-ish objects was a prerequisite to fixing this. Despite the flaws (I am not referring to the buggy behavior) you and Junio have pointed out, this still seems like an improvement over the previous (and soon-to-be current) behavior. Saying the remote reference already exists is true, and it implies that removing it might solve the problem which is also true. Adding another error status will allow the error message to be made clearer in both cases (i.e., I avoided the word "tag" specifically so that it would apply to other cases, or so I thought.) Chris -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/8] push: update remote tags only with force
Chris Rorvick writes: > I agree with everything above. I just don't understand why reverting > the "already exists" behavior for non-commit-ish objects was a > prerequisite to fixing this. Because it is a regression. People who did not force such a push did not get "already exists", but with your patch they do. By reverting the wrong message so that we get the old wrong message instead, people will only have to deal with an already known breakage; a known devil is better than an unknown new devil (or an unknown angel). When a change that brings in a regression and an improvement at the same time, it does not matter what the improvement is; we fix the regression first as soon as safely possible and we then attempt to resurrect and polish the improvement. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html