On Tue, Feb 21, 2012 at 08:44, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> 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.
But you would have to maintain rebase.$branch.bottomLimit separately
for each topic branch, updating it whenever the topic branch is merged
to 'next'. Is that practical?
> 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.
All true, but my experience is that novice users will not enjoy this
experience, and rather become frustrated with Git for "putting them in
this situation" (i.e. giving them more rope than they can handle).
Maybe I'm doing something wrong...
>>> 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.
Agreed. We should probably focus more on this when teaching novice users.
>> 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.
Yes. Maybe it should be a tri-state thing:
- "rebase-prone" will allow rewrites without asking.
- unset will warn, but still allow rewrites. The warning should
explain the potential problem that may arise from rewriting, and
should also explain how to get rid of the warning by setting this
config to either of the above/below options. Ideally the warning
should also explain how the user may undo the rewrite. Obviously, most
of this warning should be controlled by a corresponding advice.*
config.
- "cast-in-stone" will refuse rewrites.
I notice that this config is not really per-branch, but rather per
upstream branch. I wonder how to best encode that in the config...
Also, how a remote repo may best communicate which branches are
"rebase-prone"/"cast-in-stone".
> 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).
This goes against many workflows, and many users' expectations.
Although it may be a Good(tm) practice, I don't think it's universal
enough to be worth "breaking" 'push'. What about adding a new 'git
review' command which defaults to this behavior (but can be overridden
by Gerrit and other code-review systems to do what's appropriate in
their cases)?
>> ... 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.
True. Is it universally acceptable to assume that a topic branch that
has been merged to a "cast-in-stone" branch, should not be
rebased/rewritten (unless forced by the user)?
There are several aspects to this: First, any commits on the topic
branch that have been made _since_ the merge should obviously be
rewritable (since they have yet to be merged). And commits made
_before_ the merge should probably not be rewritable. I.e. in the
following figure, the 'y' commits should be rewritable, but the 'x'
commits (preceding the merge 'M') should probably not.
--o---o---o---o---o---o---M <- master (cast-in-stone)
\ /
x---x---x---x---x---x---y---y <- topicA
But what if we have two topic branches with shared history, like this:
--o---o---o---o---o---o---M <- master (cast-in-stone)
\ /
x---x---x---x---x---x---y---y <- topicA
\
z---z <- topicB
We cannot say whether the 'x' commits "belong" to topicA or topicB.
Also, we cannot determine whether the merge M happened with topicA or
topicB (unless the default merge message has been preserved). Now,
given that we try to rewrite the entire topicB branch (including the
'x' commits): Should we refuse rewriting the 'x' commits because they
are reachable from master (regardless of whether topicA or topicB was
merged), or should we allow rewriting the 'x' commits on the basis
that topicA may be the branch that was merged in 'M', and since topicB
are "unaware" that the 'x' commits have been merged, it should thus be
allowed to rewrite them?
Now, what if you want to backport 'topicA' from 'master' to 'maint'.
You could do so like this:
git checkout -b topicA_maint topicA
git rebase --onto maint master
(note that topicA_maint is a special case of topicB in the above
graph) In this case, we would expect the 'x' commits to be rewritable,
or we could not perform the backport.
The conclusion seems to be that we _cannot_ refuse rewriting commits
merely on the basis that they have been merged to a cast-in-stone
branch.
>> 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).
Ok, trying to make as few assumptions about user workflows as
possible: Assuming there were a (machine-parsable) way to mark
branches as either "rebase-prone" or "cast-in-stone". Could we then
assume that rebasing commits that exist on a cast-in-stone branch's
remote-tracking @{upstream} should be refused by default? I'm trying
(without much success, it seems) to find _something_ that will help
the 1525 users that want Git to "warn before/when rewriting published
history"[1], but won't cause any false positives (and small enough
false negatives). If there really is no way to implement this, then we
shouldn't give users false hopes by putting it in the survey...
Have fun! :)
...Johan
[1]: Question 17 in https://www.survs.com/results/Q5CA9SKQ/P7DE07F0PL
--
Johan Herland, <johan@xxxxxxxxxxx>
www.herland.net
--
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