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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Johan Herland <johan@xxxxxxxxxxx> writes:

> 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.

>> 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).

>  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.

> 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).
--
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


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]