Re: [PATCH 0/1] Preserve topological ordering after merges

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 7, 2020, 12:11 UTC
Message-ID: <nycvar.QRO.7.76.6.2001071308290.46@tvgsbejvaqbjf.bet>
In-Reply-To: <pull.514.git.1578393057.gitgitgadget@gmail.com>

Hi Sergey,

On Tue, 7 Jan 2020, Sergey Rudyshin via GitGitGadget wrote:

Show 42 quoted lines

> This modifies the algoritm of topopological ordering. The problem with the
> current algoritm is that it displays the graph below as following
>
> * E
> |\
> | * D
> | |\
> | |/
> |/|
> * | C
> | * B
> |/
> * A
>
> commit B is placed between A and C, which is wrong because E stays that D
> and B comes after C
>
> the only correct solution here is
>
> * E
> |\
> | * D
> | |\
> | |/
> |/|
> | * B
> * | C
> |/
> * A
>
> while it seems that it contradicts to D staiting that C shoud be between D
> and B The new algorithm solves this issue
>
> Here is an example: walk to to the root via "first" parents go E -> C -> A
> print A because it has no parents step back to C print C because it has no
> other parents then step back to E go D -> B -> A do not print A becase A is
> already printed step back to B print B so on
>
> This change makes option "--topo-order" obsolete, because for a single
> commit there is only one way to order parents. "--date-order" and
> "--author-date-order" are preserved and make sence only for the case when
> multiple commits are given to be able to sort those commits.

I have to admit that I am not a fan of this change, as I find that there are legitimate use cases where I want to order the commits by commit topology, and other use cases where I want to order them by date.

Maybe other reviewers agree with your reasoning, though, in that case you still will need to address the test failures in t4202, t4215 and t6012: https://dev.azure.com/gitgitgadget/git/_build/results?buildId=25867&view=ms.vss-test-web.build-test-results-tab

Ciao, Johannes

Show 26 quoted lines

>
> Sergey Rudyshin (1):
>   Preserve topological ordering after merges This modifies the algorithm
>     of topopological ordering. The problem with the current algoritm is
>     that it displays the graph below as follows
>
>  Documentation/git-rev-list.txt             |   4 +-
>  Documentation/rev-list-options.txt         |  41 +++---
>  commit.c                                   | 149 ++++++++++++++-------
>  commit.h                                   |   4 +-
>  t/t4202-log.sh                             |  15 +--
>  t/t4215-log-skewed-merges.sh               |  44 +++---
>  t/t6003-rev-list-topo-order.sh             |  54 ++++----
>  t/t6012-rev-list-simplify.sh               |   8 +-
>  t/t6016-rev-list-graph-simplify-history.sh |  41 +++---
>  t/t6600-test-reach.sh                      |   6 +-
>  10 files changed, 214 insertions(+), 152 deletions(-)
>
>
> base-commit: 8679ef24ed64018bb62170c43ce73e0261c0600a
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-514%2FSergeyRudyshin%2Ftopo_order_fix-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-514/SergeyRudyshin/topo_order_fix-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/514
> --
> gitgitgadget
>