On Tue, Mar 13, 2012 at 09:27:08PM -0700, Junio C Hamano wrote: > An off-topic administrivia. Please do not try to deflect responses meant > for you by setting Mail-Followup-To. Thanks for catching this. Truth be told I downloaded a command line MUA specifically to send this patch and read in others from this list. I've been wrestling with the config and will wrestle it further. > 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? > > > 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? These notes make sense and I will reroll v2 with them in mind, as well as the other comments about the advice wording, sign-off line, and making nonfastforward a switch, not quoted here. > * 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"? > > > 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. After mulling over it, I tend to agree with this, but will address further down the thread. > Also, shouldn't we be doing this only when err is true, or is it too > defensive? This was an oversight on my part. Given that the current error message has been in place since 07436e4 in 2009 and doesn't seem to have caused trouble (other than it not being applicable in some 'git push' situations), I'll move the code in v2. Thanks for the comments. They are much appreciated. -- Christopher Tiwald -- 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