An off-topic administrivia. Please do not try to deflect responses meant
for you by setting Mail-Followup-To.
Christopher Tiwald <christiwald@xxxxxxxxx> writes:
> Pushing a non-fast-forward update to a remote repository will result in
> an error, but the hint text doesn't provide the correct resolution in
> every case. Three scenarios may arise depending on your workflow, each
> with a different resolution:
Are we sure there are only three, or is this just "we do not say anything
concrete, but at least we know common three cases, and there may be more"?
I am mostly interested in making sure that we do not give a bad advice.
Giving an advice that is mostly accurate and relevant for 95% of the time
is perfectly fine, as long as following the advice in the remaining 5%
does not result in a disaster.
> 1) If you push a non-fast-forward update to HEAD, you should merge
> remote changes with 'git pull' before pushing again.
You said "to HEAD", but I think you meant the case you push your current
branch (i.e. HEAD) to update any ref on the other side. In other words,
the push does not have to be "*to*" HEAD over there. Am I mistaken?
> 2) If you push to a shared repository others push to, and your local
> tracking branches are not kept up to date, the 'matching refs' default
> will generate non-fast-forward errors on outdated branches. If this is
> your workflow, the 'matching refs' default is not for you. Consider
> setting the 'push.default' configuration variable to 'upstream' to
> ensure only your checked-out branch is pushed.
OK.
> 3) If you push with explicit ref matching (e.g. 'git push ... topic:topic')
> while checked out on another branch (e.g. 'master'), the correct
> resolution is checking out the local branch, issuing git pull, and
> merging remote changes before pushing again.
Or you may have misspelled the source side of the refspec and tried to
push a wrong branch.
> Make nonfastforward an enum and teach transport.c to detect the
> scenarios described above. Give situation-specific resolution advice
> when pushes are rejected due to non-fast-forward updates. Finally,
> update other instances of nonfastforward to use the proper enum option.
I think the overall direction of the implemention is good, modulo minor
design nits.
* I do not particularly find NONFASTFORWARD_NONE that is defined to be 0
a useful readability measure. Plain vanilla constant 0 says that there
is nothing magical going on to the readers clearly already.
* Also NONFASTFORWARD_FROTZ is way too long. Wouldn't NONFF_FROTZ be
sufficient and clear?
* I can see there are three kinds of advices, but I do not see why users
need to acknowledge that they understand them one by one with separate
advice configuration. Isn't it better to have only one variable, "OK,
I know how to deal with a failed push due to non-fast-forward"?
> Signed-off-by: Christopher Tiwald <christiwald@xxxxxxxxx>
> Based-on-patch-by: Junio C Hamano <gitster@xxxxxxxxx>
These two lines are chronologically swapped.
> ---
> This is a reroll of jc/advise-push-default (2011-12-18).
I lost track of this topic during the last round. Thanks for picking it
up.
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c081657..50d9249 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -158,6 +158,21 @@ advice.*::
> Advice shown when you used linkgit:git-checkout[1] to
> move to the detach HEAD state, to instruct how to create
> a local branch after the fact.
> + pullBeforePush::
> + Advice shown when you ran linkgit:git-push[1] and pushed
> + a non-fast-forward update to HEAD, instructing you to
> + linkgit:git-pull[1] before pushing again.
> + useUpstream::
> + Advice to set 'push.default' to 'upstream' when you ran
> + linkgit:git-push[1] and pushed 'matching refs' by default
> + (i.e. you did not have any explicit refspec on the command
> + line, and no 'push.default' configuration was set) and it
> + resulted in a non-fast-forward error.
> + checkoutPullPush::
> + Advice shown when you ran linkgit:git-push[1] and pushed
> + a non-fast-forward update to a non-HEAD branch, instructing
> + you to checkout the branch and run linkgit:git-pull[1]
> + before pushing again.
I would prefer to see these consolidated into a single advice.pushNonFF
variable, but I may be missing why it could be a good idea to allow them
turned off selectively.
> diff --git a/builtin/push.c b/builtin/push.c
> index d315475..0fecf06 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> }
> }
>
> +static const char *message_advice_pull_before_push[] = {
> + "To prevent you from losing history, non-fast-forward updates to HEAD",
> + "were rejected. Merge the remote changes (e.g. 'git pull') before",
> + "pushing again. See the 'Note about fast-forwards' section of",
> + "'git push --help' for details."
> +};
This again says "*to* HEAD". If this should be "a non-fast-forward update
to send the current branch was rejected" as I suspected above, the message
needs to be rephrased accordingly.
> +static const char *message_advice_use_upstream[] = {
> + "By default, git pushes all branches that have a matching counterpart",
> + "on the remote. In this case, some of your local branches were stale",
> + "with respect to their remote counterparts. If you did not intend to",
> + "push these branches, you may want to set the 'push.default'",
> + "configuration variable to 'upstream' to push only the current branch."
> +};
If you drop everything up to and including "In this case, ", the advice
message still teaches exactly what the user needs to learn.
> +static const char *message_advice_checkout_pull_push[] = {
> + "To prevent you from losing history, your non-fast-forward branch",
> + "updates were rejected. Checkout the branch and merge the remote",
> + "changes (e.g. 'git pull') before pushing again. See the",
> + "'Note about fast-forwards' section of 'git push --help' for",
> + "details."
> +};
OK.
> @@ -136,15 +193,18 @@ static int push_with_options(struct transport *transport, int flags)
>
> err |= transport_disconnect(transport);
>
> + if (nonfastforward == NONFASTFORWARD_HEAD) {
> + advise_pull_before_push();
> + } else if (nonfastforward == NONFASTFORWARD_OTHER) {
> + if (default_matching_used)
> + advise_use_upstream();
> + else
> + advise_checkout_pull_push();
> + }
I suspect that we may find more cases not just three, so
switch (nonfastforward) {
default:
break;
case NONFF_HEAD:
advice_pull_before_push();
break;
case NONFF_OTHER:
...
}
would be a more forward-looking way to write it.
Also, shouldn't we be doing this only when err is true, or is it too
defensive?
> if (!err)
> return 0;
>
> - if (nonfastforward && advice_push_nonfastforward) {
> - fprintf(stderr, _("To prevent you from losing history,...
That is, I am wondering why your "more detailed diag & advice" code is not
here, i.e. after "if (!err) return 0".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html