Re: [RFC] Rebasing merges: a jorney to the ultimate solution(RoadClear)

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

 



Hi Sergey,

On Wed, 14 Mar 2018, Sergey Organov wrote:

> Igor Djordjevic <igor.d.djordjevic@xxxxxxxxx> writes:
> 
> > On 07/03/2018 08:26, Johannes Schindelin wrote:
> 
> [...]
> 
> >> Second side note: if we can fast-forward, currently we prefer that,
> >> and I think we should keep that behavior with -R, too.
> >
> > I agree.
> 
> I'm admittedly somewhat lost in the discussion,

... and cutting so much context does not help...

The "fast-forward" here refers to the thing `git rebase` does unless you
call it with `--force-rebase`: if we are about to `pick` a commit, and
HEAD already points to its parent commit, we do not evey cherry-pick: we
just fast-forward to the original commit.

Likewise, `merge -C <commit>` will simply fast-forward to the specified
commit if HEAD is pointing to its first parent and the specified merge
heads are also identical to the respective original parent commits.

What I said with my comment was that `merge -R -C <commit>` will behave in
exactly the same way.

> but are you talking fast-forward on _rebasing_ existing merge? Where
> would it go in any of the suggested algorithms of rebasing and why?

And this is something I did not talk about yet, but it does come up in
practice: the main use case of rebasing branches is to follow an
"upstream", of course. And guess what? From time to time, some of the
branches get merged upstream (or, in Git's own source code, applied). In
this case, the Git garden shears *do* skip the merge (as the new merge
head is already an ancestor of HEAD). In --recreate-merges, it *will*
create a new merge commit, though, which is a bit annoying.

The best way to handle this would *probably* look similar to how "empty"
commits (i.e. commits whose patch is empty) are handled by rebase. But it
is not yet clear to me how that would look in practice, as `pick <commit>`
is a single operation that can be easily commented out in the todo list
depending on `--allow-empty`, while the `merge -C <commit>` command is not
the entire operation, as there may be `pick` commands in the merge head
that could potentially be skipped due to merge conflicts with
already-applied versions of the same patches.

If this sounds unclear to you, please do ask for clarification. Although
this is currently not my highest priority in the `sequencer-shears` branch
thicket (where `--recreate-merges` is the first part).

> I readily see how it can break merges. E.g., any "git merge --ff-only
> --no-ff" merge will magically disappear.

Did you mean `git merge --no-ff`? Combining `--ff-only` with `--no-ff`
does not make sense.

> So, even if somehow supported, fast-forward should not be performed by
> default during _rebasing_ of a merge.

That statement is too general to be correct.

*If* we detect that the original merge could have been fast-forwarded
instead (and it is very easy to detect that in --make-list), we would have
to handle that similar to afore-mentioned empty commits in conjunction
with `--allow-empty`.

If the original merge could not have been fast-forwarded, but during the
rebase it *can*, we should skip it, as the reason for the merge commit
is clearly no longer there.

This, by the way, is an insight you can really only win by using
--recreate-merges (or the Git garden shears). And using it a *lot*.
Because otherwise, you will not even guess correctly what will, and what
won't, come up in practice.

> >> If the user wants to force a new merge, they simply remove that -R
> >> flag.
> 
> Alternatively, they'd replace 'pick' with 'merge', as they already do
> for other actions. "A plurality is not to be posited without necessity".

No, no, no and no again!

We learned how *wrong* the `pick` approach was with --preserve-merges!
Have you *ever* used --preserve-merges in any real way? If so, you *have*
encountered the problems, and probably directed several lengthy curses my
way for that lousy design.

A pick is a pick is a pick. Of a single patch with metadata such as the
author, commit message and the date.

A merge is not a patch. While a pick introduces a specific change on top
of a single revision, the changes introduced by a merge are ideally
already there. It is conceptually *very* different.

Sure, a merge commit sometimes needs to introduce extra changes on top of
the merged changes, sure, that happens (and is called "evil merge" in Git
parlance). Those *additional* changes should be kept as minimal as
possible, in particular there should only be changes *necessitated* by the
reconciled changes.

So no, a `pick` is not a `merge`. Not at all.

> Please, _please_, don't use 'merge' command to 'pick' merge commits!
> It's utterly confusing!

Please, please, please *work* with branch thickets for a while. And you
will see that it makes a *huge* difference!

For example, within a block of `pick` lines, it is relatively safe to
reorder. You see more or less what changes interact with one another.

Be *very* careful when reordering `merge` lines, in particular when you
have a real-world branch thicket, not just a toy example.

Do *not* use `pick` for merges. Not. Ever.

By the way, let this here discussion serve as Yet Another Example why it
is important to not only consider new features theoretically. Only in
practice do you find out where your theory sounded too good to be actually
true.

> Thinking about it I've got an idea that what we actually need is
> --no-flatten flag that, when used alone, will just tell "git rebase" to
> stop flattening history, and which will be implicitly imposed by
> --recreate-merges (and --preserve-merges).

... and this flag would only make sense if every `git rebase` involved a
todo list.

Which it does not.

Ciao,
Johannes



[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]

  Powered by Linux