Junio C Hamano <gitster@xxxxxxxxx> writes:
> If "--lockref" automatically implies "--allow-no-ff" (the design in
> the reposted patch), you cannot express that combination. But once
> you use "--lockref" in such a situation , for the push to succeed,
> you know that the push replaces not just _any_ ancestor of what you
> are pushing, but replaces the exact current value. So I do not think
> your implicit introduction of --allow-no-ff via redefining the
> semantics of the plus prefix is not adding much value (if any),
> while making the common case less easy to use.
>
>> No; --lockref only adds the check that the destination is at the
>> expected revision, but does *NOT* override the no-ff check.
>
> You _could_ do it in that way, but that is less useful.
Another issue I have with the proposal is that we close the door to
"force only this one" convenience we have with "+ref" vs "--force
ref". Assuming that it is useful to require lockref while still
making sure that the usual "must fast-forward" rule is followed (if
that is not the case, I do not see a reason why your proposal is any
useful---am I missing something?), I would prefer to allow users a
way to decorate this basic syntax to say:
git push --lockref master jch pu
things like
(1) pu may not fast-forward and please override that "must
fast-forward" check from it, while still keeping the lockref
safety (e.g. "+pu" that does not --force, which is your
proposal);
(2) any of them may not fast-forward and please override that "must
fast-forward" check from it, while still keeping the lockref
safety (without adding "--allow-no-ff", I do not see how it is
possible with your proposal, short of forcing user to add "+"
everywhere);
(3) I know jch does not fast-forward so please override the "must
fast-forward", but still apply the lockref safety, pu may not
even satisfy lockref safety so please force it (as the "only
force this one" semantics is removed from "+", I do not see how
it is possible with your proposal).
So I would understand if your proposal _were_ to
* add "--allow-no-ff" option;
* change the meaning of "+ref" to "--allow-no-ff for only this
ref"; and
* add a new "*ref" (or whatever new syntax) to still allow people
to say "--force only this ref".
but we still need to assume that it makes sense to ask lockref but
still want to ensure the update fast-forwards. I personally do not
think it does [*1*].
The semantics the posted patch (rerolled to allow "--force" push
anything) implements lets "--lockref" to imply "--allow-no-ff" and
that makes it much simpler; we do not have to deal with any of the
above complexity.
[Footnote]
*1* The assurance --lockref gives is a lot stronger than "must
fast-forward". You may have fetched the topic whose tip was at
commit X, and rebased it on top of X~4 to create a new history
leading to Y.
o----o----Y
/
o---o----o----o----o----X
X~4
When you "git push --lockref=topic:X Y:X", you are requiring
their tip to be still at X. Other people's change cannot be to
add something on top of X (which will be lost if we replace the
tip of the topic with Y).
If your change were not a rebase but to build one of you own:
o---o----o----o----o----X---Y
your "git push --lockref=topic:X Y:X" still requires the tip is
at X. If somebody rewound the tip to X~2 in the meantime
(because they decided the tip 2 commits were not good), your
"git push Y:X" without the "--lockref" will lose their rewind,
because Y will still be a fast-forward update of X~2.
"--lockref=topic:X" will protect you in this case as well.
So I think "--lockref" that automatically disables "must
fast-forward" check is the right thing to do, as we are
replacing the weaker "must fast-forward" with something
stronger. I do not think we are getting anything from forcing
the user to say "--allow-no-ff" with "+ref" syntax when the
user says "--lockref". It is not making it safer, and it is
making it less convenient.
--
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