Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] commit-graph: fix bug around octopus merges #167

Merged

Conversation

derrickstolee
Copy link
Collaborator

The commit message below describes the bug. This has been sent upstream
hopefully in time for v2.23.0. If we are taking 2.23.0 soon, then we would not
need this fix.

The question for @jrbriggs and others is: do we rush this fix to the version
of microsoft/vfsforgit in EA? There are possible reasons that this is an
edge-case enough to ignore:

  1. It only affects the split commit-graph when a non-base file has an
    octopus merge commit. Only one octopus merge exists in the OS repo
    right now.

  2. It will not break users, but will cause "warning: commit-graph chain
    does not match" warnings and cause increasing performance issues as
    the base commit-graph files far further and further behind.

I'm open to all options here.

Thanks,
-Stolee


In 1771be9 "commit-graph: merge commit-graph chains" (2019-06-18),
the method sort_and_scan_merged_commits() was added to merge the
commit lists of two commit-graph files in the incremental format.
Unfortunately, there was an off-by-one error in that method around
incrementing num_extra_edges, which leads to an incorrect offset
for the base graph chunk.

When we store an octopus merge in the commit-graph file, we store
the first parent in the normal place, but use the second parent
position to point into the "extra edges" chunk where the remaining
parents exist. This means we should be adding "num_parents - 1"
edges to this list, not "num_parents - 2". That is the basic error.

The reason this was not caught in the test suite is more subtle.
In 5324-split-commit-graph.sh, we test creating an octopus merge
and adding it to the tip of a commit-graph chain, then verify the
result. This should have caught the problem, except that when
we load the commit-graph files we were overly careful to not fail
when the commit-graph chain does not match. This care was on
purpose to avoid race conditions as one process reads the chain
and another process modifies it. In such a case, the reading
process outputs the following message to stderr:

warning: commit-graph chain does not match

These warnings are output in the test suite, but ignored. By
checking the stderr of git commit-graph verify to include
the expected progress output, it will now catch this error.

Signed-off-by: Derrick Stolee dstolee@microsoft.com

@jrbriggs
Copy link
Member

jrbriggs commented Aug 5, 2019

Given the potential impact, let's take this.

@derrickstolee
Copy link
Collaborator Author

/azp run Microsoft.git
/azp run Microsoft.git (Windows (MSVC) Test 6)

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

In 1771be9 "commit-graph: merge commit-graph chains" (2019-06-18),
the method sort_and_scan_merged_commits() was added to merge the
commit lists of two commit-graph files in the incremental format.
Unfortunately, there was an off-by-one error in that method around
incrementing num_extra_edges, which leads to an incorrect offset
for the base graph chunk.

When we store an octopus merge in the commit-graph file, we store
the first parent in the normal place, but use the second parent
position to point into the "extra edges" chunk where the remaining
parents exist. This means we should be adding "num_parents - 1"
edges to this list, not "num_parents - 2". That is the basic error.

The reason this was not caught in the test suite is more subtle.
In 5324-split-commit-graph.sh, we test creating an octopus merge
and adding it to the tip of a commit-graph chain, then verify the
result. This _should_ have caught the problem, except that when
we load the commit-graph files we were overly careful to not fail
when the commit-graph chain does not match. This care was on
purpose to avoid race conditions as one process reads the chain
and another process modifies it. In such a case, the reading
process outputs the following message to stderr:

	warning: commit-graph chain does not match

These warnings are output in the test suite, but ignored. By
checking the stderr of `git commit-graph verify` to include
the expected progress output, it will now catch this error.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee derrickstolee merged commit 7c49a1f into microsoft:vfs-2.22.0 Aug 5, 2019
derrickstolee added a commit to derrickstolee/VFSForGit that referenced this pull request Aug 5, 2019
See microsoft/git#167 for details.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
derrickstolee added a commit to derrickstolee/VFSForGit that referenced this pull request Aug 5, 2019
See microsoft/git#167 for details.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
derrickstolee added a commit to microsoft/VFSForGit that referenced this pull request Aug 6, 2019
See microsoft/git#167 for details.

This package also includes a Linux installer, since we have updated the build definition since the last Git update.
derrickstolee added a commit to microsoft/VFSForGit that referenced this pull request Aug 6, 2019
See microsoft/git#167 for details.

This package also includes a Linux installer, since we have updated the build definition since the last Git update.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants