Re: [PATCH v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
On Wed, Jan 23, 2013 at 08:28:49AM -0800, Junio C Hamano wrote: > How about doing this? > > For "needs force" cases, we say this instead: > > hint: you cannot update a ref that points at a non-commit object, or > hint: update a ref to point at a non-commit object, without --force. > > Being explicit about "non-commit" twice will catch user's eyes and > cause him to double check that it is not a mistyped LHS of the push > refspec (if he is sending a non-commit) or mistyped RHS (if the ref > is pointing at a non-commit). If he _is_ trying to push a blob out, > the advice makes it clear what to do next: he does want to force it. Yeah, I think that is sensible. > Note that you _could_ split the "needs force" case into two, namely, > "cannot replace a non-commit" and "cannot push a non-commit". You > could even further split them [...etc...] I do not think it is worth worrying too much about. This should really not happen very often, and the user should be able to investigate and figure out what is going on. I think making the error message extremely specific is just going to end up making it harder to understand. > If we did that, then we could loosen the "You should fetch first" > case to say something like this: > > hint: you do not have the object at the tip of the remote ref; > hint: perhaps you want to pull from there first? Yeah, better. I'll comment on the specific message you used in response to the patch itself. > This explicitly denies one of Chris's wish "we shouldn't suggest to > merge something that we may not be able to", but in the "You should > fetch first" case, we cannot fundamentally know if we can merge > until we fetch. I agree with you that the most common case is that > the unknown object is a commit, and that suggesting to pull is a > good compromise. I thought the wish was more about "we shouldn't suggest to merge something we _know_ we will not be able to", and you are still handling that (i.e., the "needs force" 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 v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
Jeff King writes: > 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 > 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)? I was actually much less happy with "needs force" than this one, as you have to assume too many things for the message to be a useful and a safe advise: the user has actually examined the situation and forcing the push is the right thing to do. Both old and new objects exist, so the user _could_ have done so, but did he really check them, thought about the situation and made the right decision? Perhaps the attempted push had a typo in the object name it wanted to update the other end with, and the right thing to do is not to force but to fix the refspec instead? "You need --force to perform this push" was a very counter-productive advice in this case, but I didn't think of a better wording. The "fetch first and inspect" was an attempt to reduce the risk of that "needs force" message that could encourage brainless forced pushes. Perhaps if we reword "needs force" to something less risky, we do not have to be so explicit in "You have to fetch first and examine". How about doing this? For "needs force" cases, we say this instead: hint: you cannot update a ref that points at a non-commit object, or hint: update a ref to point at a non-commit object, without --force. Being explicit about "non-commit" twice will catch user's eyes and cause him to double check that it is not a mistyped LHS of the push refspec (if he is sending a non-commit) or mistyped RHS (if the ref is pointing at a non-commit). If he _is_ trying to push a blob out, the advice makes it clear what to do next: he does want to force it. If we did that, then we could loosen the "You should fetch first" case to say something like this: hint: you do not have the object at the tip of the remote ref; hint: perhaps you want to pull from there first? This explicitly denies one of Chris's wish "we shouldn't suggest to merge something that we may not be able to", but in the "You should fetch first" case, we cannot fundamentally know if we can merge until we fetch. I agree with you that the most common case is that the unknown object is a commit, and that suggesting to pull is a good compromise. Note that you _could_ split the "needs force" case into two, namely, "cannot replace a non-commit" and "cannot push a non-commit". You could even further split them into combinations (e.g. an attempt to replace an annotated tag with a commit and an attempt to replace a tree with a commit may be different situations), but I think the advices we can give to these cases would end up being the same, so I tend to think it is not worth it. That is what I meant by "I do not expect me doing the type-based policy myself" in the concluding message of the series. -- 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 v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
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 -- 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
[PATCH v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
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
---
* 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(-)
diff --git a/advice.c b/advice.c
index d287927..780f58d 100644
--- a/advice.c
+++ b/advice.c
@@ -5,6 +5,8 @@ int advice_push_non_ff_current = 1;
int advice_push_non_ff_default = 1;
int advice_push_non_ff_matching = 1;
int advice_push_already_exists = 1;
+int advice_push_fetch_first = 1;
+int advice_push_needs_force = 1;
int advice_status_hints = 1;
int advice_commit_before_merge = 1;
int advice_resolve_conflict = 1;
@@ -20,6 +22,8 @@ static struct {
{ "pushnonffdefault", &advice_push_non_ff_default },
{ "pushnonffmatching", &advice_push_non_ff_matching },
{ "pushalreadyexists", &advice_push_already_exists },
+ { "pushfetchfirst", &advice_push_fetch_first },
+ { "pushneedsforce", &advice_push_needs_force },
{ "statushints", &advice_status_hints },
{ "commitbeforemerge", &advice_commit_before_merge },
{ "resolveconflict", &advice_resolve_conflict },
diff --git a/advice.h b/advice.h
index 8bf6356..fad36df 100644
--- a/advice.h
+++ b/advice.h
@@ -8,6 +8,8 @@ extern int advice_push_non_ff_current;
extern int advice_push_non_ff_default;
extern int advice_push_non_ff_matching;
extern int advice_push_already_exists;
+extern int advice_push_fetch_first;
+extern int advice_push_needs_force;
extern int advice_status_hints;
extern int advice_commit_before_merge;
extern int advice_resolve_conflict;
diff --git a/builtin/push.c b/builtin/push.c
index 8491e43..da928fa 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -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.");
+static const char message_advice_ref_fetch_first[] =
+ N_("Updates were rejected; you need to fetch the destination
reference\n"
+ "to decide what to do.\n");
+
+static const char message_advice_ref_needs_force[] =
+ N_("Updates were rejected; you need to force update.\n");
+
static void advise_pull_before_push(void)
{
if (!advice_push_non_ff_current || !advice_push_update_rejected)
@@ -252,6 +259,20 @@ static void advise_ref_already_exists(void)
advise(_(message_advice_ref_already_exists));
}
+static void advise_ref_fetch_first(void)
+{
+ if (!advice_push_fetch_first || !advice_push_update_rejected)
+ return;
+ advise(_(message_advice_ref_fetch_first));
+}
+
+static void advise_ref_needs_force(void)
+{
+ if (!advice_push_needs_force || !advice_push_update_rejected)
+ return;
+ advise(_(message_advice_ref_needs_force));
+}
+
static int push_with_options(struct transport *transport, int flags)
{
int err;
@@ -285,6 +306,10 @@ static int push_with_options(struct transport *transport,
int flags)
advise_checkout_pull_push();
} else if (reject_reasons & REJECT_ALREADY_EXISTS) {
advise_ref_already_exists();
+ } else if (reject_reasons & REJECT_FETCH_FIRST) {
+ advise_ref_fetch_first();
+ } else if (reject_reasons & REJECT_NEEDS_FORCE) {
+ advise_r
