log: --remerge-diff needs to keep around commit parents #1825
+13
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dscho
force-pushed
the
log-remerge-diff-needs-commit-parents
branch
from
bd9d036 to
6823831
Compare
|
/submit |
|
Submitted as pull.1825.git.1731073435641.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Elijah Newren wrote (reply to this): On Fri, Nov 8, 2024 at 5:43 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> To show a remerge diff, the merge needs to be recreated. For that to
> work, the merge base(s) need to be found, which means that the commits'
> parents have to be traversed until common ancestors are found (if any).
>
> However, one optimization that hails all the way back to
> cb115748ec0d (Some more memory leak avoidance, 2006-06-17) is to release
> the commit's list of parents immediately after showing it. This can break
> the merge base computation.
>
> Note that it matters more clearly when traversing the commits in
> reverse: In that instance, if a parent of a merge commit has been shown
> as part of the `git log` command, by the time the merge commit's diff
> needs to be computed, that parent commit's list of parent commits will
> have been set to `NULL` and as a result no merge base will be found.
>
> Let's fix this by special-casing the `remerge_diff` mode, similar to
> what we did with reflogs in f35650dff6a4 (log: do not free parents when
> walking reflog, 2017-07-07).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> log: --remerge-diff needs to keep around commit parents
>
> This fixes a bug that is pretty much as old as the remerge-diff
> machinery itself. I noticed it while writing my reply to Hannes Sixt's
> concerns about my range-diff --diff-merges=<mode> patch
> (https://lore.kernel.org/git/af576487-5de2-fba3-b341-3c082322c9ec@gmx.de/).
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1825%2Fdscho%2Flog-remerge-diff-needs-commit-parents-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1825/dscho/log-remerge-diff-needs-commit-parents-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1825
>
> builtin/log.c | 2 +-
> t/t4069-remerge-diff.sh | 7 +++++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index c0a8bb95e98..a297c6caf59 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -522,7 +522,7 @@ static int cmd_log_walk_no_free(struct rev_info *rev)
> * but we didn't actually show the commit.
> */
> rev->max_count++;
> - if (!rev->reflog_info) {
> + if (!rev->reflog_info && !rev->remerge_diff) {
> /*
> * We may show a given commit multiple times when
> * walking the reflogs.
Should this comment be updated to reflect the expanded rationale for
this block's purpose?
> diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh
> index 07323ebafe0..a68c6bfa036 100755
> --- a/t/t4069-remerge-diff.sh
> +++ b/t/t4069-remerge-diff.sh
> @@ -317,4 +317,11 @@ test_expect_success 'remerge-diff turns off history simplification' '
> test_cmp expect actual
> '
>
> +test_expect_success 'remerge-diff with --reverse' '
> + git log -1 --remerge-diff --oneline ab_resolution^ >expect &&
> + git log -1 --remerge-diff --oneline ab_resolution >>expect &&
> + git log -2 --remerge-diff --oneline ab_resolution --reverse >actual &&
> + test_cmp expect actual
> +'
> +
> test_done
>
> base-commit: bea9ecd24b0c3bf06cab4a851694fe09e7e51408
> --
> gitgitgadget
Wow, nice find, and I appreciate the nice simple testcase
demonstrating what had been the problem. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> To show a remerge diff, the merge needs to be recreated. For that to
> work, the merge base(s) need to be found, which means that the commits'
> parents have to be traversed until common ancestors are found (if any).
>
> However, one optimization that hails all the way back to
> cb115748ec0d (Some more memory leak avoidance, 2006-06-17) is to release
> the commit's list of parents immediately after showing it. This can break
> the merge base computation.
> Note that it matters more clearly when traversing the commits in
> reverse: In that instance, if a parent of a merge commit has been shown
> as part of the `git log` command, by the time the merge commit's diff
> needs to be computed, that parent commit's list of parent commits will
> have been set to `NULL` and as a result no merge base will be found.
Ouch.
I am curious about "more clearly" in the above, though. Do we also
lose parents before a base can be computed during a forward
traversal? Unlike the reflog walk, we won't show the same commit
twice, so unless we are traversing some of the history of our
parents *and* showing these ancestors found there _before_ we need
to show the merge in question, I do not think the issue would
surface during a forward traversal. So the problematic case is when
we are not doing topological display, and traversing down to a
commit with skewed timestamp causes it to be shown (and lose its
parents due to memory optimization) because it appears much newer
than our merge, and there is an ancestry chain leading to it that
passes commits other than our merge, or something?
> Let's fix this by special-casing the `remerge_diff` mode, similar to
> what we did with reflogs in f35650dff6a4 (log: do not free parents when
> walking reflog, 2017-07-07).
OK.
> diff --git a/builtin/log.c b/builtin/log.c
> index c0a8bb95e98..a297c6caf59 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -522,7 +522,7 @@ static int cmd_log_walk_no_free(struct rev_info *rev)
> * but we didn't actually show the commit.
> */
> rev->max_count++;
> - if (!rev->reflog_info) {
> + if (!rev->reflog_info && !rev->remerge_diff) {
> /*
> * We may show a given commit multiple times when
> * walking the reflogs.
OK, that's trivial ;-)
> diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh
> index 07323ebafe0..a68c6bfa036 100755
> --- a/t/t4069-remerge-diff.sh
> +++ b/t/t4069-remerge-diff.sh
> @@ -317,4 +317,11 @@ test_expect_success 'remerge-diff turns off history simplification' '
> test_cmp expect actual
> '
>
> +test_expect_success 'remerge-diff with --reverse' '
> + git log -1 --remerge-diff --oneline ab_resolution^ >expect &&
> + git log -1 --remerge-diff --oneline ab_resolution >>expect &&
> + git log -2 --remerge-diff --oneline ab_resolution --reverse >actual &&
> + test_cmp expect actual
> +'
This is the trivially reproducible "show in reverse" case. Nice
finding.
Will queue. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Elijah Newren <newren@gmail.com> writes:
>> - if (!rev->reflog_info) {
>> + if (!rev->reflog_info && !rev->remerge_diff) {
>> /*
>> * We may show a given commit multiple times when
>> * walking the reflogs.
>
> Should this comment be updated to reflect the expanded rationale for
> this block's purpose?
Ah, we probably should. Especially if the newly discovered failure
mode is not due to having to show the same commit multiple times.
During reflog walk, we may show the same commit more than once,
so do not discard the parents. There may be a merge commit that
is descendent from the current commit, and in order to show it with
remerge-diff enabled, we need its ancestry chain, including our
parents, to compute the merge base.
or something? It is still not clear to me in what scenario a merge
commit, in a forward traversal, is shown _after_ one of its ancestor
commit is shown (and due to the memory optimization, loses its
parents).
Thanks, both.
|
|
This patch series was integrated into seen via git@cd7e6ff. |
dscho
force-pushed
the
log-remerge-diff-needs-commit-parents
branch
from
6823831 to
afff27f
Compare
|
/submit |
|
Submitted as pull.1825.v2.git.1731350009491.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Junio,
On Mon, 11 Nov 2024, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > To show a remerge diff, the merge needs to be recreated. For that to
> > work, the merge base(s) need to be found, which means that the commits'
> > parents have to be traversed until common ancestors are found (if any).
> >
> > However, one optimization that hails all the way back to
> > cb115748ec0d (Some more memory leak avoidance, 2006-06-17) is to release
> > the commit's list of parents immediately after showing it. This can break
> > the merge base computation.
>
> > Note that it matters more clearly when traversing the commits in
> > reverse: In that instance, if a parent of a merge commit has been shown
> > as part of the `git log` command, by the time the merge commit's diff
> > needs to be computed, that parent commit's list of parent commits will
> > have been set to `NULL` and as a result no merge base will be found.
>
> Ouch.
>
> I am curious about "more clearly" in the above, though.
I consider these examples less clear, but they are still affected:
git show --remerge-diff v2.45.2^0
vs
git show --remerge-diff v2.44.2^0 v2.45.2^0
git show --remerge-diff v2.45.1~1
vs
git log --topo-order --first-parent --remerge-diff v2.44.2 v2.45.1
Concretely, these diffs should be empty, but are not:
git diff --no-index \
<(git show --remerge-diff v2.45.2^0 | sed 1d) \
<(git show --remerge-diff v2.44.2^0 v2.45.2^0 | sed 1,/^commit/d)
and
git diff --no-index \
<(git show --remerge-diff v2.45.1~1 | grep -v ^commit) \
<(git log --topo-order --first-parent -6 --remerge-diff v2.44.2 v2.45.1 |
sed '1,/^commit 1c00f92eb5ee4a48ab615eefa41f2dd6024d43bc/d;/^commit/,$d')
No `--reverse` required, not even clock skew.
Ciao,
Johannes |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I consider these examples less clear, but they are still affected:
>
> git show --remerge-diff v2.45.2^0
> vs
> git show --remerge-diff v2.44.2^0 v2.45.2^0
>
> git show --remerge-diff v2.45.1~1
> vs
> git log --topo-order --first-parent --remerge-diff v2.44.2 v2.45.1
>
> Concretely, these diffs should be empty, but are not:
>
> git diff --no-index \
> <(git show --remerge-diff v2.45.2^0 | sed 1d) \
> <(git show --remerge-diff v2.44.2^0 v2.45.2^0 | sed 1,/^commit/d)
>
> and
>
> git diff --no-index \
> <(git show --remerge-diff v2.45.1~1 | grep -v ^commit) \
> <(git log --topo-order --first-parent -6 --remerge-diff v2.44.2 v2.45.1 |
> sed '1,/^commit 1c00f92eb5ee4a48ab615eefa41f2dd6024d43bc/d;/^commit/,$d')
>
> No `--reverse` required, not even clock skew.
As we often tell contributors, questions in reviewer comments should
be also answered in an updated patch, so that future readers of "git
log", who cannot ask direct questions to the author of the patch, do
not have to ask the same questions. Can we add an explanation how
this affects forward traversal in a three-line paragraph?
Thanks.
|
|
This patch series was integrated into seen via git@70e0e2f. |
|
This patch series was integrated into seen via git@0e49e1e. |
|
This patch series was integrated into seen via git@e00edba. |
|
This patch series was integrated into seen via git@ee5a48d. |
|
This patch series was integrated into seen via git@7ca4a92. |
|
This patch series was integrated into seen via git@9fb6743. |
|
This patch series was integrated into seen via git@43be7c6. |
|
This patch series was integrated into seen via git@d68487a. |
|
This patch series was integrated into seen via git@7b178ed. |
|
This patch series was integrated into seen via git@1bc8be5. |
|
This patch series was integrated into seen via git@e5877d3. |
|
This patch series was integrated into seen via git@95e3831. |
|
This patch series was integrated into seen via git@7a15d32. |
|
This patch series was integrated into seen via git@fe2d833. |
|
This patch series was integrated into seen via git@de2ef44. |
|
This patch series was integrated into seen via git@eabefc6. |
|
This patch series was integrated into seen via git@6c08f94. |
|
This patch series was integrated into seen via git@93626bc. |
|
This patch series was integrated into seen via git@c59eaa0. |
|
This patch series was integrated into seen via git@83a7ccc. |
|
This patch series was integrated into seen via git@a1392cd. |
|
This patch series was integrated into seen via git@64f05cc. |
|
This patch series was integrated into seen via git@b1bfee5. |
|
This patch series was integrated into seen via git@cdc7e12. |
|
This patch series was integrated into seen via git@abcdf51. |
|
This patch series was integrated into seen via git@7bd8332. |
To show a remerge diff, the merge needs to be recreated. For that to work, the merge base(s) need to be found, which means that the commits' parents have to be traversed until common ancestors are found (if any). However, one optimization that hails all the way back to cb11574 (Some more memory leak avoidance, 2006-06-17) is to release the commit's list of parents immediately after showing it _and to set that parent list to `NULL`_. This can break the merge base computation. This problem is most obvious when traversing the commits in reverse: In that instance, if a parent of a merge commit has been shown as part of the `git log` command, by the time the merge commit's diff needs to be computed, that parent commit's list of parent commits will have been set to `NULL` and as a result no merge base will be found (even if one should be found). Traversing commits in reverse is far from the only circumstance in which this problem occurs, though. There are many avenues to traversing at least one commit in the revision walk that will later be part of a merge base computation, for example when not even walking any revisions in `git show <merge1> <merge2>` where `<merge1>` is part of the commit graph between the parents of `<merge2>`. Another way to force a scenario where a commit is traversed before it has to be traversed again as part of a merge base computation is to start with two revisions (where the first one is reachable from the second but not in a first-parent ancestry) and show the commit log with `--topo-order` and `--first-parent`. Let's fix this by special-casing the `remerge_diff` mode, similar to what we did with reflogs in f35650d (log: do not free parents when walking reflog, 2017-07-07). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
log-remerge-diff-needs-commit-parents
branch
from
afff27f to
ab75a56
Compare
|
/submit |
|
Submitted as pull.1825.v3.git.1733999352289.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> This problem is most obvious when traversing the commits in reverse: In
> that instance, if a parent of a merge commit has been shown as part of
> the `git log` command, by the time the merge commit's diff needs to be
> computed, that parent commit's list of parent commits will have been set
> to `NULL` and as a result no merge base will be found (even if one
> should be found).
>
> Traversing commits in reverse is far from the only circumstance in which
> this problem occurs, though. There are many avenues to traversing at
> least one commit in the revision walk that will later be part of a merge
> base computation, for example when not even walking any revisions in
> `git show <merge1> <merge2>` where `<merge1>` is part of the commit
> graph between the parents of `<merge2>`.
>
> Another way to force a scenario where a commit is traversed before it
> has to be traversed again as part of a merge base computation is to
> start with two revisions (where the first one is reachable from the
> second but not in a first-parent ancestry) and show the commit log with
> `--topo-order` and `--first-parent`.
>
> Let's fix this by special-casing the `remerge_diff` mode, similar to
> what we did with reflogs in f35650dff6a4 (log: do not free parents when
> walking reflog, 2017-07-07).
Nicely described.
> This fixes a bug that is pretty much as old as the remerge-diff
> machinery itself. I noticed it while writing my reply to Hannes Sixt's
> concerns about my range-diff --diff-merges=<mode> patch
> diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh
> index 07323ebafe0..a68c6bfa036 100755
> --- a/t/t4069-remerge-diff.sh
> +++ b/t/t4069-remerge-diff.sh
> @@ -317,4 +317,11 @@ test_expect_success 'remerge-diff turns off history simplification' '
> test_cmp expect actual
> '
>
> +test_expect_success 'remerge-diff with --reverse' '
> + git log -1 --remerge-diff --oneline ab_resolution^ >expect &&
> + git log -1 --remerge-diff --oneline ab_resolution >>expect &&
> + git log -2 --remerge-diff --oneline ab_resolution --reverse >actual &&
> + test_cmp expect actual
> +'
> +
Will queue. Let me mark it for 'next'.
Thanks.
|
|
This patch series was integrated into seen via git@15a9ec3. |
|
This patch series was integrated into next via git@90156d7. |
|
This patch series was integrated into seen via git@28bfc69. |
|
This patch series was integrated into seen via git@8346224. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@a954b1c. |
|
This patch series was integrated into seen via git@5bb3d78. |
|
This patch series was integrated into seen via git@cb89eeb. |
|
This patch series was integrated into master via git@cb89eeb. |
|
This patch series was integrated into next via git@cb89eeb. |
|
Closed via cb89eeb. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a bug that is pretty much as old as the remerge-diff machinery itself. I noticed it while writing my reply to Hannes Sixt's concerns about my
range-diff --diff-merges=<mode>patch (https://lore.kernel.org/git/af576487-5de2-fba3-b341-3c082322c9ec@gmx.de/).Changes since v2:
Changes since v1:
Cc: Johannes Sixt j6t@kdbg.org
Cc: Elijah Newren newren@gmail.com