Re: [RFC] pre-rebase: Refuse to rewrite commits that are reachable from upstream

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 21, 2012, 07:44 UTC
Message-ID: <7vehtoiqae.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CALKQrgcDDVH8rxn80ZTMyR3y3n3Vs1LSCtH=ZiwG7it_LE2wgQ@mail.gmail.com>

Johan Herland <johan@herland.net> writes:

Show 5 quoted lines

> What  you are describing here may be a common workflow, but
> "rebase.bottomLimit" is still very specific to that kind of workflow.
> What I'm after is a much more workflow-agnostic concept of:
>
>   "If I have pushed something, I should probably not rebase it"

Your "this branch pushes directly to that remote branch, so I can check if it will result in rewrite of published commit" is even *less* generic than having a single bottomLimit in my illustration.

I may not push out my topic branches directly, only the aggregate of them in 'next', but once 'next' is pushed out, they are not eligible for rebasing. A per-branch bottom, e.g. rebase.$branch.bottomLimit, might make it more flexible to cover such a case, though.

On the other hand, without any such safety, a merge to 'next' would give many conflicts and "shortlog master..next" will show many duplicates after any topic that are already merged to it are accidentally rewritten, and it is just the matter of using reflog on topic branches to recover from such a mistake.

Show 6 quoted lines

>> I wonder if it would be a more direct solution to the issue you are
>> raising to give them a good tool to help them to be more careful with less
>> effort on their part before they publish (not before they rebase).
> 
> ..., I'm not sure how we can help the user _not_ publish the
> branch until it's ready.

I think we are in agreement that we do not think of a good solution offhand to the real cause of the issue, except by encouraging the use of throw-away review branches, perhaps.

> I think the following decribes what often happens for many users:
>
>  1. User A pushes the branch to a public repo.
>  2. User B points out a simple mistake in the branch.

That's the CVS workflow, and it is not "a" public repo but "the" public repo shared between A and B (and also with all the project participants).

Show 5 quoted lines

>  3. User A makes a fix
>  4. User A squashes the fix into the (already-published) history.
>  5. User A attempts to push the "fixed" history (but is rejected by
> the public repo because of non-fast-forward).
> At this point, the damage is already done,...

Which is probably a sufficient safety which the user can learn from. If this happens too often, that probably means we are not helping them enough to learn not to "commit --amend" or "rebase" if they are using Git as a better CVS.

> You could say that User A should be more careful and push to a "less
> public" repo in step #1 (thus allowing the fix to be squashed before
> pushing to a "more public" repo in step #5),...

That is essentially a workflow that uses throw-away review branches in a distributed environment, and at that point, we are not constrained by the limitation of the CVS workflow. While still in early review cycles (which corresponds to being in our 'pu'), "commit --amend" and "rebase" are fine tool to be used. And...

> but how "public" is
> "public" enough to have someone point out the bug, but still
> "unpublic" enough to allow rebase?

... I can imagine that currently that is determined purely by project convention. Perhaps there needs a way to mark throw-away review branches like 'pu' (or saying the same thing from the different perspective, to mark cast-in-stone integration branches like 'next') so that tools can mechanically decide what should and should not be rewritten.

To extend the idea of promoting throw-away review branches further, perhaps it might help if there is an easy way to let the users publish their "master" to a branch that is not the "master" of the central shared repository even in the CVS workflow (e.g. by default a push from user A always goes to refs/review/A/master), and to have an option to "git push" that makes it go to the "master" when the user really means the branch is ready (and it would move refs/review/A/master to attic to be later gc'ed).

> ... And I
> think that refusing rewrites of commits that are already present in
> the @{upstream} remote-tracking branch is good enough to help most
> users avoid steps #4 through #6 (in a push-based workflow[1]).

See above regarding branches that should not be rebased even if they are not directly pushed out.

Show 5 quoted lines

> In
> fact, from a pedagogical POV, I think step #4 is probably the best
> spot for novice users to learn exactly the distinction between
> acceptable and unacceptable history rewrites (instead of having it
> explained to them as a consequence of the step #5).

I doubt you have enough information at point #4, unless you restrict the workflow you allow your novice users to use fairly severely, to give appropriate advice. While I agree with you that it would be the best if we could do so at step #4 without stopping the user from doing what s/he needs to do with false positive, I think it is not pedagogical POV but dreaming if the world were ideal without knowing what it would take to make it ideal.

At least I don't know offhand what kind of changes are needed to restrict the user actions to an "approved" workflow so that step #4 can make a useful decision (that is, no false positives and small enough false negatives).