Re: [PATCH v7 3/3] replay: offer an option to linearize the commit topology

[Toon Claes <toon@iotcl.com>]

Elijah Newren <newren@gmail.com> writes:

> As I proposed last time, I'd be fine with erroring on multiple
> positive refs as an interim step (plus associated documentation and
> commit message updates) so this series lands, with per-branch
> linearization as the real fix later.

I appreciate you're open to this interim step, but I would like to
understand the end goal better before we continue.

> TL;DR version; my problems with the current implementation of
> `--linearize` are that it:
>   * Makes the rare usecase easy, and ignores the common usecase

Cannot deny that, although I'm not sure git-replay(1) is a popular
end-user command.

>   * Makes it asymmetrically difficult to recover for those that wanted
> the common usecase instead of the easy

I don't think many users would use --linearize anyway. I'm guessing
properly replaying merges would be far more useful to most people.
I'm adding it mostly to scratch my own itch: do a server-side
non-interactive rebase that's identical git-rebase(1)'s
--no-rebase-merges.

>   * Makes `--linearize` mean something other than "remove non-linearity"

It's debatable what it means, because you can think of it linearizing
all reachable commits (see also below what you said context of
gitrevisions(7)).

>   * Turns multiple branches into one, but updates several branches anyway
>   * Ignores order specified by the user on the command line
>   * Introduces an inconsistency within git-replay between `--advance`
> and `--linearize --onto`
> (The last three items being minor compared to the first three.)

I'm surprised you consider these three more minor, because I have more
issues with them personally (the ordering in particular).

I don't have a feasible example, but as I understand from your
argumentation, v7 might make commits reachable from a branch where they
weren't before:

> M1  M2  M3  M4  M5
> *---*---*---*---* <- master
>                 |
>                 |  A1  A2  A3  A4
>                 |--*---*---*---* <- branchA
>                 |      \
>                 |       -*---* <- branchC
>                 |        C1  C2
>                 |
>                 \-*---*---* <- branchB
>                   B1  B2  B3
> 
> With the current implementation of --linearize, adding that flag, i.e.
>     git replay --linearize --onto master branchA branchB branchC
> would instead give something like:
> 
> M1  M2  M3  M4  M5  B1  B2  B3  A1  A2  C1  C2  A3  A4
> *---*---*---*---*---*---*---*---*---*---*---*---*---*
>                 ^           ^               ^       ^
>                 |           |               |       |
>               master     branchB         branchC  branchA

Before the replay, branchC didn't reach any commits in branchB, while it
does now. It kind of makes sense though, because branchC is specified
after branchB. But then again, why does now branchA contain branchB and
branchC? That's the problem I have with the ordering.

> I think I know what you mean, but this isn't quite right:
> git-replay(1) only ever accepts a single revision range.  From
> gitrevisions(7) (also in git-rev-parse(1)):
> 
>        Commands that are specifically designed to take two distinct ranges
>        (e.g. "git range-diff R1 R2" to compare two ranges) do exist, but they
>        are exceptions. Unless otherwise noted, all "git" commands that operate
>        on a set of commits work on a single revision range. In other words,
>        writing two "two-dot range notation" next to each other, e.g.
> 
>            $ git log A..B C..D
> 
>        does not specify two revision ranges for most commands. Instead it will
>        name a single connected set of commits, i.e. those that are reachable
>        from either B or D but are reachable from neither A or C.

You could think v7's implementation of --linearize converts the
"distinct ranges" into a "single connected set of commits", but then the
option name isn't very good.

> The reason I am comfortable with erroring out as a stopgap: turning an
> error into working behavior later never breaks anyone, whereas letting the
> current concatenation semantics reach 'master' risks users coming to
> depend on them, which would make switching to the better behavior a
> compatibility break.

I absolutely agree with that approach.

> Erroring now keeps our options open; merging as-is
> quietly closes them.  (git-replay is still EXPERIMENTAL, so this is not
> fatal either way, but it seems better not to paint ourselves into a
> corner.)

Being EXPERIMENTAL allows us to break things if we discover we didn't
think about before, that's not the case here.

But then again, what do we do about --contained?

M1  M2  M3  M4  M5
*---*---*---*---* <- master
     \
      \  A1  A2  A3  A4  A5  A6
       \-*---*---*---*---*---* <- branchA
          \   \     /   /
           \   *---*   /  <- branchB
            \  B1  B2 /
             \---*---/  <- branchC
                 C1

This would end up into something like:

M1  M2  M3  M4  M5
*---*---*---*---* <- master
                |
                |   A1  A2  A3  B1  B2  C1 A6
                \---*---*---*---*---*---*---* <- branchA
                           branchB -^   ^- branchC

Same issue, branchC suddenly contains the commits of branchB.

The only way we can linearize (as in flatten merges) these branches is
by replaying some commits twice:

M1  M2  M3  M4  M5
*---*---*---*---* <- master
                |
                |   A1  A2  A3  B1  B2  C1 A6
                \---*---*---*---*---*---*---* <- branchA
                     \   \
                      \   \---*---*           <- branchB
                       \     B1'  B2'
                        \---*                 <- branchC
                            C1'

But is that what the user wants? They could achieve that with running
git-replay(1) once for every single branch separately (let's assume they
set COMMITTER_DATE).
Is this the end goal we want for --linearize with multiple revision
ranges? I don't think that's doable with the last_commit per branch.

But for now, I would say --contained is not allowed with --linearize as
well.

And maybe, maybe we should make --ref required when --linearize is
given. Then the user would do something like:

    $ git replay --onto master branchA branchB branchC --ref branchA

This makes the end result unambiguous: take all commits reachable from
these 3 branches, replay them linearly onto 'master' and *only* update
ref 'branchA'.


-- 
Cheers,
Toon