From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:48
Chris Rorvick [off-list ref] writes:
On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano [off-list ref] wrote:
quoted
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.
From: Chris Rorvick <hidden> Date: 2016-06-15 22:55:48
On Thu, Jan 17, 2013 at 12:59 AM, Junio C Hamano [off-list ref] wrote:
Chris Rorvick [off-list ref] writes:
quoted
On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano [off-list ref] wrote:
quoted
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
From: Jeff King <hidden> Date: 2016-06-15 22:55:48
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
From: Chris Rorvick <hidden> Date: 2016-06-15 22:55:48
On Thu, Jan 17, 2013 at 7:06 PM, Jeff King [off-list ref] 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
From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:48
Jeff King [off-list ref] 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.
From: Jeff King <hidden> Date: 2016-06-15 22:55:50
On Thu, Jan 17, 2013 at 09:18:50PM -0600, Chris Rorvick wrote:
On Thu, Jan 17, 2013 at 7:06 PM, Jeff King [off-list ref] wrote:
quoted
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".
quoted
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
From: Chris Rorvick <hidden> Date: 2016-06-15 22:55:51
On Mon, Jan 21, 2013 at 5:40 PM, Jeff King [off-list ref] wrote:
On Thu, Jan 17, 2013 at 09:18:50PM -0600, Chris Rorvick wrote:
quoted
On Thu, Jan 17, 2013 at 7:06 PM, Jeff King [off-list ref] wrote:
quoted
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.
quoted
quoted
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
From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:51
The "nonfastforward" field is only used to decide what value to
assign to the "status" locally in a single function. Remove it from
the "struct ref" and make it into a local variable.
The "requires_force" field is not used to decide if the proposed
update requires a --force option to succeed, or to record such a
decision made elsewhere. It is used by status reporting code that
the particular update was "forced". Rename it to "forced_udpate",
and move the code to assign to it around to further clarify how it
is used and what it is used for.
Signed-off-by: Junio C Hamano <redacted>
---
cache.h | 3 +--
remote.c | 10 +++++-----
transport.c | 2 +-
3 files changed, 7 insertions(+), 8 deletions(-)
From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:51
This builds on Chris Rorvick's earlier effort to forbid unforced
updates to refs/tags/ hierarchy and giving sensible error and advise
messages for that case (we are not rejecting such a push due to fast
forwardness, and suggesting to fetch and integrate before pushing
again does not make sense).
The main change is in the second patch. When we
* do not have the object at the tip of the remote;
* the object at the tip of the remote is not a commit; or
* the object we are pushing is not a commit,
there is no point suggesting to fetch, integrate and push again.
If we do not have the current object at the tip of the remote, we
should tell the user to fetch first and evaluate the situation
before deciding what to do next.
Otherwise, if the current object is not a commit, or if we are
trying to push an object that is not a commit, then the user does
not have to fetch first (we already have the object), but it still
does not make sense to suggest to integrate and re-push. Just tell
them that such a push requires a force in such a case.
Junio C Hamano (3):
push: further clean up fields of "struct ref"
push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
push: further reduce "struct ref" and simplify the logic
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 25 +++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 6 +++---
remote.c | 38 ++++++++++++++++----------------------
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 14 +++++++++++++-
transport.h | 2 ++
10 files changed, 87 insertions(+), 26 deletions(-)
--
1.8.1.1.498.gfdee8be
From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:51
The "update" field in "struct ref" is only used in a very narrow
scope in a single function. Remove it.
Also simplify the code that rejects an attempted push by first
checking if the proposed update is forced (in which case we do not
need any check on our end).
Signed-off-by: Junio C Hamano <redacted>
---
cache.h | 1 -
remote.c | 42 +++++++++++++-----------------------------
2 files changed, 13 insertions(+), 30 deletions(-)
From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:51
When pushing update an existing ref, we wouldn't even know if we are
fast-forwarding the ref on the other end if:
* we do not have the object currently at the tip of remote;
* the object currently at the tip of remote is not a committish; or
* the object we are pushing is not a committish.
In such a case, the push has been rejected on the client end, but we
used the same error and advice messages as the ones used when
rejecting a non-fast-forward push, i.e. pull from there and
integrate before pushing again. This did not make much sense.
Introduce two error classes and suggest fetching from the other side
first and evaluate the situation, if we do not have the current
object, or just tell the user the update needs --force when we do
have the current object and either it or the object we are trying to
push is not a committish, in which case it can never fast-forward
and we know there is no point suggesting to merge.
Signed-off-by: Junio C Hamano <redacted>
---
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 25 +++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 2 ++
remote.c | 22 ++++++++++++++++------
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 12 ++++++++++++
transport.h | 2 ++
10 files changed, 85 insertions(+), 6 deletions(-)
@@ -224,6 +224,13 @@ static const char message_advice_ref_already_exists[] =N_("Updates were rejected because the destination reference already exists\n""in the remote.");+staticconstcharmessage_advice_ref_fetch_first[]=+N_("Updates were rejected; you need to fetch the destination reference\n"+"to decide what to do.\n");++staticconstcharmessage_advice_ref_needs_force[]=+N_("Updates were rejected; you need to force update.\n");+staticvoidadvise_pull_before_push(void){if(!advice_push_non_ff_current||!advice_push_update_rejected)
@@ -285,6 +306,10 @@ static int push_with_options(struct transport *transport, int flags)advise_checkout_pull_push();}elseif(reject_reasons&REJECT_ALREADY_EXISTS){advise_ref_already_exists();+}elseif(reject_reasons&REJECT_FETCH_FIRST){+advise_ref_fetch_first();+}elseif(reject_reasons&REJECT_NEEDS_FORCE){+advise_ref_needs_force();}return1;
@@ -1322,17 +1322,26 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,!is_null_sha1(ref->old_sha1);if(ref->update){-intnonfastforward=-!has_sha1_file(ref->old_sha1)-||!ref_newer(ref->new_sha1,ref->old_sha1);-if(!prefixcmp(ref->name,"refs/tags/")){if(!force_ref_update){ref->status=REF_STATUS_REJECT_ALREADY_EXISTS;continue;}ref->forced_update=1;-}elseif(nonfastforward){+}elseif(!has_sha1_file(ref->old_sha1)||+!lookup_commit_reference_gently(ref->old_sha1,1)){+if(!force_ref_update){+ref->status=REF_STATUS_REJECT_FETCH_FIRST;+continue;+}+ref->forced_update=1;+}elseif(!lookup_commit_reference_gently(ref->new_sha1,1)){+if(!force_ref_update){+ref->status=REF_STATUS_REJECT_NEEDS_FORCE;+continue;+}+ref->forced_update=1;+}elseif(!ref_newer(ref->new_sha1,ref->old_sha1)){if(!force_ref_update){ref->status=REF_STATUS_REJECT_NONFASTFORWARD;continue;
@@ -1521,7 +1530,8 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)structcommit_list*list,*used;intfound=0;-/* Both new and old must be commit-ish and new is descendant of+/*+*Bothnewandoldmustbecommit-ishandnewisdescendantof*old.Otherwisewerequire--force.*/o=deref_tag(parse_object(old_sha1),NULL,0);
From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:51
This builds on Chris Rorvick's earlier effort to forbid unforced
updates to refs/tags/ hierarchy and giving sensible error and advise
messages for that case (we are not rejecting such a push due to fast
forwardness, and suggesting to fetch and integrate before pushing
again does not make sense).
The series applies on top of 256b9d7 (push: fix "refs/tags/
hierarchy cannot be updated without --force", 2013-01-16).
The main change is in the second patch. When we
* do not have the object at the tip of the remote;
* the object at the tip of the remote is not a commit; or
* the object we are pushing is not a commit,
there is no point suggesting to fetch, integrate and push again.
If we do not have the current object at the tip of the remote, we
should tell the user to fetch first and evaluate the situation
before deciding what to do next.
Otherwise, if the current object is not a commit, or if we are
trying to push an object that is not a commit, then the user does
not have to fetch first (we already have the object), but it still
does not make sense to suggest to integrate and re-push. Just tell
them that such a push requires a force in such a case.
Junio C Hamano (3):
push: further clean up fields of "struct ref"
push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
push: further simplify the logic to assign rejection status
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 25 +++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 6 +++---
remote.c | 42 +++++++++++++++++++-----------------------
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 14 +++++++++++++-
transport.h | 2 ++
10 files changed, 90 insertions(+), 27 deletions(-)
--
1.8.1.1.498.gfdee8be
From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:51
The "nonfastforward" and "update" fields are only used while
deciding what value to assign to the "status" locally in a single
function. Remove them from the "struct ref".
The "requires_force" field is not used to decide if the proposed
update requires a --force option to succeed, or to record such a
decision made elsewhere. It is used by status reporting code that
the particular update was "forced". Rename it to "forced_udpate",
and move the code to assign to it around to further clarify how it
is used and what it is used for.
Signed-off-by: Junio C Hamano <redacted>
---
* The "update" removal in v1 has been moved to this.
cache.h | 4 +---
remote.c | 16 ++++++----------
transport.c | 2 +-
3 files changed, 8 insertions(+), 14 deletions(-)
From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:51
When we push to update an existing ref, if:
* we do not have the object at the tip of the remote; or
* the object at the tip of the remote is not a commit; or
* the object we are pushing is not a commit,
there is no point suggesting to fetch, integrate and push again.
If we do not have the current object at the tip of the remote, we
should tell the user to fetch first and evaluate the situation
before deciding what to do next.
Otherwise, if the current object is not a commit, or if we are
trying to push an object that is not a commit, then the user does
not have to fetch first (we already have the object), but it still
does not make sense to suggest to integrate and re-push. Just tell
them that such a push requires a force in such a case.
In these cases, the push was locally rejected on the client end, but
we used the same error and advice messages as the ones used when
rejecting a non-fast-forward push, i.e. pull from there and
integrate before pushing again. This did not make much sense.
Signed-off-by: Junio C Hamano <redacted>
---
* Updated log message and fixed the logic to decide "fetch first";
we should say "fetch first" only when we do not have the current
tip of the remote end.
send-pack.c has style violation that "else" is not on the same
line as closing brace of its corresponding "if", but I followed
the existing style of surrounding code. Cleaning them up is for
a separate topic.
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 25 +++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 2 ++
remote.c | 22 ++++++++++++++++------
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 12 ++++++++++++
transport.h | 2 ++
10 files changed, 85 insertions(+), 6 deletions(-)
@@ -224,6 +224,13 @@ static const char message_advice_ref_already_exists[] =N_("Updates were rejected because the destination reference already exists\n""in the remote.");+staticconstcharmessage_advice_ref_fetch_first[]=+N_("Updates were rejected; you need to fetch the destination reference\n"+"to decide what to do.\n");++staticconstcharmessage_advice_ref_needs_force[]=+N_("Updates were rejected; you need to force update.\n");+staticvoidadvise_pull_before_push(void){if(!advice_push_non_ff_current||!advice_push_update_rejected)
@@ -285,6 +306,10 @@ static int push_with_options(struct transport *transport, int flags)advise_checkout_pull_push();}elseif(reject_reasons&REJECT_ALREADY_EXISTS){advise_ref_already_exists();+}elseif(reject_reasons&REJECT_FETCH_FIRST){+advise_ref_fetch_first();+}elseif(reject_reasons&REJECT_NEEDS_FORCE){+advise_ref_needs_force();}return1;
@@ -1318,17 +1318,26 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,*/if(!ref->deletion&&!is_null_sha1(ref->old_sha1)){-intnonfastforward=-!has_sha1_file(ref->old_sha1)-||!ref_newer(ref->new_sha1,ref->old_sha1);-if(!prefixcmp(ref->name,"refs/tags/")){if(!force_ref_update){ref->status=REF_STATUS_REJECT_ALREADY_EXISTS;continue;}ref->forced_update=1;-}elseif(nonfastforward){+}elseif(!has_sha1_file(ref->old_sha1)){+if(!force_ref_update){+ref->status=REF_STATUS_REJECT_FETCH_FIRST;+continue;+}+ref->forced_update=1;+}elseif(!lookup_commit_reference_gently(ref->old_sha1,1)||+!lookup_commit_reference_gently(ref->new_sha1,1)){+if(!force_ref_update){+ref->status=REF_STATUS_REJECT_NEEDS_FORCE;+continue;+}+ref->forced_update=1;+}elseif(!ref_newer(ref->new_sha1,ref->old_sha1)){if(!force_ref_update){ref->status=REF_STATUS_REJECT_NONFASTFORWARD;continue;
@@ -1517,7 +1526,8 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)structcommit_list*list,*used;intfound=0;-/* Both new and old must be commit-ish and new is descendant of+/*+*Bothnewandoldmustbecommit-ishandnewisdescendantof*old.Otherwisewerequire--force.*/o=deref_tag(parse_object(old_sha1),NULL,0);
From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:51
Instead of using deeply nested if/else statements, first decide what
rejection status we would get if this push weren't forced, and then
assign the rejection reason to the ref->status field and flip the
ref->forced_update field when we forced a push for a ref that indeed
required forcing.
Signed-off-by: Junio C Hamano <redacted>
---
* The first one mistakenly changed the semantics and reported a
forced push even when the push was done with useless and
unnecessary --force option (e.g. the update was properly
fast-forwarding but --force was given from the command line).
This fixes it.
remote.c | 40 +++++++++++++++-------------------------
1 file changed, 15 insertions(+), 25 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:55:51
On Mon, Jan 21, 2013 at 10:30:28PM -0800, Junio C Hamano wrote:
The "nonfastforward" and "update" fields are only used while
deciding what value to assign to the "status" locally in a single
function. Remove them from the "struct ref".
The "requires_force" field is not used to decide if the proposed
update requires a --force option to succeed, or to record such a
decision made elsewhere. It is used by status reporting code that
the particular update was "forced". Rename it to "forced_udpate",
Typo.
and move the code to assign to it around to further clarify how it
is used and what it is used for.
Signed-off-by: Junio C Hamano <redacted>
---
* The "update" removal in v1 has been moved to this.
cache.h | 4 +---
remote.c | 16 ++++++----------
transport.c | 2 +-
3 files changed, 8 insertions(+), 14 deletions(-)
Looks much better.
I wondered briefly why nonfastforward was even there, as I recall that I
was the one who added it many years ago. It turns out that it used to
serve the purpose of the new forced_update, but Chris's series from a
few months ago split it out to "nonfastforward" and "not_forwardable",
and then added "requires_force" to give a single flag that is set in
either case.
So I think your simplification is correct; the first two can be local
variables, and the only thing that matters to carry forward is
requires_force (and I agree that forced_update is a better name).
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:55:51
On Mon, Jan 21, 2013 at 10:30:29PM -0800, Junio C Hamano wrote:
When we push to update an existing ref, if:
* we do not have the object at the tip of the remote; or
* the object at the tip of the remote is not a commit; or
* the object we are pushing is not a commit,
there is no point suggesting to fetch, integrate and push again.
If we do not have the current object at the tip of the remote, we
should tell the user to fetch first and evaluate the situation
before deciding what to do next.
Should we? I know that it is more correct to do so, because we do not
even know for sure that the remote object is a commit, and fetching
_might_ lead to us saying "hey, this is not something that can be
fast-forwarded".
But by far the common case will be that it _is_ a commit, and the right
thing is going to be to pull. Adding in the extra steps makes the
workflow longer and more complicated, and most of the time doesn't
matter. For example, imagine that Alice is working on "master", and when
she goes to push, she finds that Bob has already pushed his work. With
the current code, she sees:
$ git push
To ...
! [rejected] HEAD -> master (non-fast-forward)
error: failed to push some refs to '...'
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.
and she presumably pulls, and all is well with the follow-up push.
With your patch, she sees:
$ git push
To ...
! [rejected] HEAD -> master (fetch first)
error: failed to push some refs to '...'
hint: Updates were rejected; you need to fetch the destination reference
hint: to decide what to do.
$ git fetch
...
$ git push
To ...
! [rejected] HEAD -> master (non-fast-forward)
error: failed to push some refs to '...'
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.
which is technically more correct (it's possible that in the second
step, she would find that Bob pushed a tree or something). But in the
common case that it is a commit, we've needlessly added two extra steps
(a fetch and another failed push), both of which involve network access
(so they are slow, and may involve Alice having to type her credentials).
Is the extra hassle in the common case worth it for the off chance that
we might give a more accurate message? Should the "fetch first" message
be some hybrid that covers both cases accurately, but still points the
user towards "git pull" (which will fail anyway if the remote ref is not
a commit)?
-Peff
From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:52
This builds on Chris Rorvick's earlier effort to forbid unforced
updates to refs/tags/ hierarchy and giving sensible error and advise
messages for that case (we are not rejecting such a push due to fast
forwardness, and suggesting to fetch and integrate before pushing
again does not make sense).
The series applies on top of 256b9d7 (push: fix "refs/tags/
hierarchy cannot be updated without --force", 2013-01-16).
This fourth round swaps the order of clean-up patches and now the
bottom two are clean-up patches. The main change is in the third
one.
When the object at the tip of the remote is not a committish, or the
object we are pushing is not a committish, the existing code already
rejects such a push on the client end, but we used the same error
and advice messages as the ones used when rejecting a push that does
not fast-forward, i.e. pull from there and integrate before pushing
again. Introduce a new rejection reason NEEDS_FORCE and explain why
the push was rejected, stressing the fact that --force is required
when non committish objects are involved, so that the user can (1)
notice a possibly mistyped source object name or destination ref
name, when the user is trying to push an ordinary commit, or (2)
learn that "--force" is an appropriate thing to use when the user is
sure that s/he wants to push a non-committish (which is unusual).
Unlike the third round, we do not say "fetch first, inspect the
situation to decide what to do", when we do not have the object
sitting at the tip of the remote. Most likely, it is a commit
somebody who has been working on the same branch pushed that we
haven't fetched yet, so suggesting to pull is often sufficient and
appropriate, and in a more uncommon case in which the unknown object
is not a committish, the suggested pull will fail without making
permanent damage anywhere. Next atttempt to push without changing
anything (e.g. "reset --hard") will then trigger the NEEDS_FORCE
"Your push involves non-commit objects" case.
Junio C Hamano (3):
push: further clean up fields of "struct ref"
push: further simplify the logic to assign rejection reason
push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
Documentation/config.txt | 12 +++++++++++-
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 29 +++++++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 6 +++---
remote.c | 42 +++++++++++++++++++-----------------------
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 14 +++++++++++++-
transport.h | 2 ++
11 files changed, 105 insertions(+), 28 deletions(-)
--
1.8.1.1.517.g0318d2b
From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:52
The "nonfastforward" and "update" fields are only used while
deciding what value to assign to the "status" locally in a single
function. Remove them from the "struct ref".
The "requires_force" field is not used to decide if the proposed
update requires a --force option to succeed, or to record such a
decision made elsewhere. It is used by status reporting code that
the particular update was "forced". Rename it to "forced_udpate",
and move the code to assign to it around to further clarify how it
is used and what it is used for.
Signed-off-by: Junio C Hamano <redacted>
---
cache.h | 4 +---
remote.c | 16 ++++++----------
transport.c | 2 +-
3 files changed, 8 insertions(+), 14 deletions(-)
From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:52
First compute the reason why this push would fail if done without
"--force", and then fail it by assigning that reason when the push
was not forced (or if there is no reason to require force, allow it
to succeed).
Record the fact that the push was forced in the forced_update field
only when the push would have failed without the option.
The code becomes shorter, less repetitive and easier to read this
way, especially given that the set of rejection reasons will be
extended in a later patch.
Signed-off-by: Junio C Hamano <redacted>
---
remote.c | 27 +++++++++++----------------
1 file changed, 11 insertions(+), 16 deletions(-)
From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:52
When we push to update an existing ref, if:
* the object at the tip of the remote is not a commit; or
* the object we are pushing is not a commit,
it won't be correct to suggest to fetch, integrate and push again,
as the old and new objects will not "merge".
If we do not have the current object at the tip of the remote, we do
not even know that object, when fetched, is something that can be
merged. In such a case, suggesting to pull first just like
non-fast-forward case may not be technically correct, but in
practice, most such failures are seen when you try to push your work
to a branch without knowing that somebody else already pushed to
update the same branch since you forked, so "pull first" would work
as a suggestion most of the time.
In these cases, the current code already rejects such a push on the
client end, but we used the same error and advice messages as the
ones used when rejecting a non-fast-forward push, i.e. pull from
there and integrate before pushing again. Introduce new
rejection reasons and reword the messages appropriately.
Signed-off-by: Junio C Hamano <redacted>
---
Documentation/config.txt | 12 +++++++++++-
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 29 +++++++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 2 ++
remote.c | 11 ++++++++---
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 12 ++++++++++++
transport.h | 2 ++
11 files changed, 92 insertions(+), 4 deletions(-)
@@ -143,7 +143,8 @@ advice.*:: pushUpdateRejected:: Set this variable to 'false' if you want to disable 'pushNonFFCurrent', 'pushNonFFDefault',- 'pushNonFFMatching', and 'pushAlreadyExists'+ 'pushNonFFMatching', 'pushAlreadyExists',+ 'pushFetchFirst', and 'pushNeedsForce' simultaneously. pushNonFFCurrent:: Advice shown when linkgit:git-push[1] fails due to a
@@ -162,6 +163,15 @@ advice.*:: pushAlreadyExists:: Shown when linkgit:git-push[1] rejects an update that does not qualify for fast-forwarding (e.g., a tag.)+ pushFetchFirst::+ Shown when linkgit:git-push[1] rejects an update that+ tries to overwrite a remote ref that points at an+ object we do not have.+ pushNeedsForce::+ Shown when linkgit:git-push[1] rejects an update that+ tries to overwrite a remote ref that points at an+ object that is not a committish, or make the remote+ ref point at an object that is not a committish. statusHints:: Show directions on how to proceed from the current state in the output of linkgit:git-status[1] and in
@@ -220,10 +220,21 @@ static const char message_advice_checkout_pull_push[] ="(e.g. 'git pull') before pushing again.\n""See the 'Note about fast-forwards' in 'git push --help' for details.");+staticconstcharmessage_advice_ref_fetch_first[]=+N_("Updates were rejected because you do not have the object at the tip\n"+"of the remote. You may want to first merge the remote changes (e.g.\n"+" 'git pull') before pushing again.\n"+"See the 'Note about fast-forwards' in 'git push --help' for details.");+staticconstcharmessage_advice_ref_already_exists[]=N_("Updates were rejected because the destination reference already exists\n""in the remote.");+staticconstcharmessage_advice_ref_needs_force[]=+N_("You cannot update a remote ref that points at a non-commit object,\n"+"or update a remote ref to make it point at a non-commit object,\n"+"without using the '--force' option.\n");+staticvoidadvise_pull_before_push(void){if(!advice_push_non_ff_current||!advice_push_update_rejected)
@@ -285,6 +310,10 @@ static int push_with_options(struct transport *transport, int flags)advise_checkout_pull_push();}elseif(reject_reasons&REJECT_ALREADY_EXISTS){advise_ref_already_exists();+}elseif(reject_reasons&REJECT_FETCH_FIRST){+advise_ref_fetch_first();+}elseif(reject_reasons&REJECT_NEEDS_FORCE){+advise_ref_needs_force();}return1;
@@ -1322,8 +1322,12 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,if(!prefixcmp(ref->name,"refs/tags/"))why=REF_STATUS_REJECT_ALREADY_EXISTS;-elseif(!has_sha1_file(ref->old_sha1)-||!ref_newer(ref->new_sha1,ref->old_sha1))+elseif(!has_sha1_file(ref->old_sha1))+why=REF_STATUS_REJECT_FETCH_FIRST;+elseif(!lookup_commit_reference_gently(ref->old_sha1,1)||+!lookup_commit_reference_gently(ref->new_sha1,1))+why=REF_STATUS_REJECT_NEEDS_FORCE;+elseif(!ref_newer(ref->new_sha1,ref->old_sha1))why=REF_STATUS_REJECT_NONFASTFORWARD;if(!force_ref_update)
@@ -1512,7 +1516,8 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)structcommit_list*list,*used;intfound=0;-/* Both new and old must be commit-ish and new is descendant of+/*+*Bothnewandoldmustbecommit-ishandnewisdescendantof*old.Otherwisewerequire--force.*/o=deref_tag(parse_object(old_sha1),NULL,0);
From: Jeff King <hidden> Date: 2016-06-15 22:55:52
On Wed, Jan 23, 2013 at 01:55:30PM -0800, Junio C Hamano wrote:
If we do not have the current object at the tip of the remote, we do
not even know that object, when fetched, is something that can be
merged. In such a case, suggesting to pull first just like
non-fast-forward case may not be technically correct, but in
practice, most such failures are seen when you try to push your work
to a branch without knowing that somebody else already pushed to
update the same branch since you forked, so "pull first" would work
as a suggestion most of the time.
In these cases, the current code already rejects such a push on the
client end, but we used the same error and advice messages as the
ones used when rejecting a non-fast-forward push, i.e. pull from
there and integrate before pushing again. Introduce new
rejection reasons and reword the messages appropriately.
So obviously from our previous discussion, I agree with the general
behavior of this patch. Let me get nit-picky on the message itself,
though:
+static const char message_advice_ref_fetch_first[] =
+ N_("Updates were rejected because you do not have the object at the tip\n"
+ "of the remote. You may want to first merge the remote changes (e.g.\n"
+ " 'git pull') before pushing again.\n"
+ "See the 'Note about fast-forwards' in 'git push --help' for details.");
+
The condition that triggers this message is going to come up fairly
often for new git users (e.g., anyone using a central repo model), which
I think is why the original message_advice_pull_before_push has gotten
so much attention. And in most cases, users will be seeing this message
now instead of "pull before push", because the common triggering cause
is somebody else pushing unrelated work.
The existing message says:
Updates were rejected because a pushed branch tip is behind its remote
counterpart. Check out this branch and merge the remote changes
(e.g. 'git pull') before pushing again.
I wonder: will the new message be as comprehensible to a new user as the
old?
They are quite similar, but something about the presence of the word
"behind" in the latter makes me think it helps explain what is going on
a bit more. When I read the new one, my first question is "why don't I
have that object?". Of course, saying "behind" in this case would not be
strictly accurate, because we do not even know the remote has a commit.
I wonder if we can reword it to explain more about why we do not have
the object, without getting too inaccurate. Something like:
Updates were rejected because the remote contains objects that you do
not have locally. This is usually caused by another repository pushing
to the same ref. You may want to first merge the remote changes (e.g.,
'git pull') before pushing again.
I was also tempted to s/objects/work/, which is more vague, but is less
jargon-y for new users who do not know how git works.
Also, how should this interact with the checkout-then-pull-then-push
advice? We make a distinction for the non-fastforward case between HEAD
and other refs. Should we be making the same distinction here?
-Peff
From: Eric Sunshine <hidden> Date: 2016-06-15 22:55:53
On Wed, Jan 23, 2013 at 4:55 PM, Junio C Hamano [off-list ref] wrote:
The "nonfastforward" and "update" fields are only used while
deciding what value to assign to the "status" locally in a single
function. Remove them from the "struct ref".
The "requires_force" field is not used to decide if the proposed
update requires a --force option to succeed, or to record such a
decision made elsewhere. It is used by status reporting code that
the particular update was "forced". Rename it to "forced_udpate",
From: Chris Rorvick <hidden> Date: 2016-06-15 22:55:53
On Wed, Jan 23, 2013 at 3:55 PM, Junio C Hamano [off-list ref] wrote:
This builds on Chris Rorvick's earlier effort to forbid unforced
updates to refs/tags/ hierarchy and giving sensible error and advise
messages for that case (we are not rejecting such a push due to fast
forwardness, and suggesting to fetch and integrate before pushing
again does not make sense).
FWIW, these changes look good to me. The logic in
set_ref_status_for_push() is easier to follow and the additional error
statuses (and associated advice) make things much clearer.
Had I written the the "already exists" advice in the context of these
additional statuses I would have said "the destination *tag* reference
already exists", or maybe even just "the destination *tag* already
exists". It's probably fine the way it is, but I only avoided using
"tag" in the advice because I was abusing it.
Thanks,
Chris