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.

Sign up for GitHub

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

Jump to bottom

Close commit-graph before calling 'gc' #208

Closed
wants to merge 14 commits into from
Closed

Close commit-graph before calling 'gc' #208

wants to merge 14 commits into from

Conversation

derrickstolee
Copy link

In Windows, the way we rename a lock file to replace the underlying file does not work when a process holds a read handle. For this reason, we call close_all_packs() everywhere before starting a git gc --auto subprocess. We also call close_commit_graph() before renaming the commit-graph lock file. But we don't close the commit-graph handles before running gc, which can cause an issue when gc.writeCommitGraph is enabled.

This series adds close_commit_graph() to close_all_packs() and then renames close_all_packs() to close_object_store(). This name is more descriptive of its larger purpose.

This is based on ds/commit-graph-write-refactor to avoid conflicts.

Thanks,
-Stolee

Cc: johannes.schindelin@gmx.de, avarab@gmail.com

@derrickstolee @gitster
commit-graph: fix the_repository reference
c794405 
The parse_commit_buffer() method takes a repository pointer, so it
should not refer to the_repository anymore.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@derrickstolee @gitster
commit-graph: return with errors during write
efeb229 
The write_commit_graph() method uses die() to report failure and
exit when confronted with an unexpected condition. This use of
die() in a library function is incorrect and is now replaced by
error() statements and an int return type.

Now that we use 'goto cleanup' to jump to the terminal condition
on an error, we have new paths that could lead to uninitialized
values. New initializers are added to correct for this.

The builtins 'commit-graph', 'gc', and 'commit' call these methods,
so update them to check the return value.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@derrickstolee @gitster
commit-graph: collapse parameters into flags
544b58b 
The write_commit_graph() and write_commit_graph_reachable() methods
currently take two boolean parameters: 'append' and 'report_progress'.
We will soon expand the possible options to send to these methods, so
instead of complicating the parameter list, first simplify it.

Collapse these parameters into a 'flags' parameter, and adjust the
callers to provide flags as necessary.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@derrickstolee @gitster
commit-graph: remove Future Work section
6ef3487 
The commit-graph feature began with a long list of planned
benefits, most of which are now complete. The future work
section has only a few items left.

As for making more algorithms aware of generation numbers,
some are only waiting for generation number v2 to ensure the
performance matches the existing behavior using commit date.

It is unlikely that we will ever send a commit-graph file
as part of the protocol, since we would need to verify the
data, and that is expensive. If we want to start trusting
remote content, then that item can be investigated again.

While there is more work to be done on the feature, having
a section of the docs devoted to a TODO list is wasteful and
hard to keep up-to-date.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@derrickstolee @gitster
commit-graph: create write_commit_graph_context
f7a3767 
The write_commit_graph() method is too large and complex. To simplify
it, we should extract several small methods. However, we will risk
repeating a lot of declarations related to progress incidators and
object id or commit lists.

Create a new write_commit_graph_context struct that contains the
core data structures used in this process. Replace the other local
variables with the values inside the context object. Following this
change, we will start to lift code segments wholesale out of the
write_commit_graph() method and into their own methods.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@derrickstolee @gitster
commit-graph: extract fill_oids_from_packs()
d83160e 
The write_commit_graph() method is too complex, so we are
extracting methods one by one.

This extracts fill_oids_from_packs() that reads the given
pack-file list and fills the oid list in the context.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@derrickstolee @gitster
commit-graph: extract fill_oids_from_commit_hex()
63a8be6 
The write_commit_graph() method is too complex, so we are
extracting methods one by one.

Extract fill_oids_from_commit_hex() that reads the given commit
id list and fille the oid list in the context.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@derrickstolee @gitster
commit-graph: extract fill_oids_from_all_packs()
93ba186 
The write_commit_graph() method is too complex, so we are
extracting methods one by one.

Extract fill_oids_from_all_packs() that reads all pack-files
for commits and fills the oid list in the context.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@derrickstolee @gitster
commit-graph: extract count_distinct_commits()
b012f3e 
The write_commit_graph() method is too complex, so we are
extracting methods one by one.

Extract count_distinct_commits(), which sorts the oids list, then
iterates through to find duplicates.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@derrickstolee @gitster
commit-graph: extract copy_oids_to_commits()
2a1e28b 
The write_commit_graph() method is too complex, so we are
extracting methods one by one.

Extract copy_oids_to_commits(), which fills the commits list
with the distinct commits from the oids list. During this loop,
it also counts the number of "extra" edges from octopus merges.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@derrickstolee @gitster
commit-graph: extract write_commit_graph_file()
8520d7f 
The write_commit_graph() method is too complex, so we are
extracting methods one by one.

Extract write_commit_graph_file() that takes all of the information
in the context struct and writes the data to a commit-graph file.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@derrickstolee
commit-graph: use raw_object_store when closing
9ff662e 
The close_commit_graph() method took a repository struct, but then
only uses the raw_object_store within. Change the function prototype
to make the method more flexible.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee
packfile: close commit-graph in close_all_packs
e4811af 
The close_all_packs() method is used to close all read handles to
pack-files and the multi-pack-index before running 'git gc --auto'.
This is particularly important on the Windows platform, where read
handles block any writes to those files. Replacing one of these
files with a rename() will fail in this situation.

The commit-graph also performs a rename, so is susceptable to this
problem. We are careful to close the commit-graph before writing,
but that doesn't work when a 'git fetch' (or similar) process runs
'git gc --auto' which may write a commit-graph.

Here, close the commit-graph as part of close_all_packs().

Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee
packfile: close_all_packs to close_object_store
0e948f6 
The close_all_packs() method is now responsible for more than just pack-files.
It also closes the commit-graph and the multi-pack-index. Rename the function
to be more descriptive of its larger role. The name also fits because the
input parameter is a raw_object_store.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented May 17, 2019

Submitted as pull.208.git.gitgitgadget@gmail.com

@dscho dscho mentioned this pull request May 18, 2019
Merged
dscho added a commit to dscho/git that referenced this pull request May 18, 2019
@dscho
Merge branch 'close-graph-everywhere'
441bb2d 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@gitgitgadget
Copy link

gitgitgadget bot commented May 19, 2019

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> This series adds close_commit_graph() to close_all_packs() and then renames 
> close_all_packs() to close_object_store(). This name is more descriptive of
> its larger purpose.

OK.  Somebody needs to go though all the existing callers of
close_all_packs() to see if the above claim is true (unless you
haven't done so, that is), but I really like the general direction.

Thanks.

@gitgitgadget
Copy link

gitgitgadget bot commented May 19, 2019

This branch is now known as ds/close-object-store.

@gitgitgadget
Copy link

gitgitgadget bot commented May 19, 2019

This patch series was integrated into pu via git@aa1e0f0.

@gitgitgadget gitgitgadget bot added the pu label May 19, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented May 19, 2019

This patch series was integrated into pu via git@9d9077a.

@weekly-digest weekly-digest bot mentioned this pull request May 19, 2019
Closed
dscho added a commit to dscho/git that referenced this pull request May 19, 2019
@dscho
Merge branch 'close-graph-everywhere'
a28b2de 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/git that referenced this pull request May 19, 2019
@dscho
Merge branch 'close-graph-everywhere'
202835b 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to git-for-windows/git that referenced this pull request May 19, 2019
@dscho
Merge branch 'close-graph-everywhere'
89f435f 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@weekly-digest weekly-digest bot mentioned this pull request Jun 2, 2019
Closed
git-for-windows-ci pushed a commit to git-for-windows/git that referenced this pull request Jun 3, 2019
@dscho
Merge branch 'close-graph-everywhere'
dc0f139 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to git-for-windows/git that referenced this pull request Jun 3, 2019
@dscho
Merge branch 'close-graph-everywhere'
653897e 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to git-for-windows/git that referenced this pull request Jun 3, 2019
@dscho
Merge branch 'close-graph-everywhere'
4154a0e 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to git-for-windows/git that referenced this pull request Jun 3, 2019
@dscho
Merge branch 'close-graph-everywhere'
ac57618 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to git-for-windows/git that referenced this pull request Jun 4, 2019
@dscho
Merge branch 'close-graph-everywhere'
f11d6e8 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
git-for-windows-ci pushed a commit to git-for-windows/git that referenced this pull request Jun 7, 2019
@dscho
Merge branch 'close-graph-everywhere'
1b95c63 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to git-for-windows/git that referenced this pull request Jun 8, 2019
@dscho
Merge branch 'close-graph-everywhere'
c422e12 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
git-for-windows-ci pushed a commit to git-for-windows/git that referenced this pull request Jun 8, 2019
@dscho
Merge branch 'close-graph-everywhere'
a192006 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to git-for-windows/git that referenced this pull request Jun 8, 2019
@dscho
Merge branch 'close-graph-everywhere'
180c4ae 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to git-for-windows/git that referenced this pull request Jun 8, 2019
@dscho
Merge branch 'close-graph-everywhere'
b63de73 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@derrickstolee derrickstolee changed the base branch from ds/commit-graph-write-refactor to master June 12, 2019 15:27
git-for-windows-ci pushed a commit to git-for-windows/git that referenced this pull request Jun 12, 2019
@dscho
Merge branch 'close-graph-everywhere'
5a07077 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
git-for-windows-ci pushed a commit to git-for-windows/git that referenced this pull request Jun 13, 2019
@dscho
Merge branch 'close-graph-everywhere'
e54e242 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
git-for-windows-ci pushed a commit to git-for-windows/git that referenced this pull request Jun 18, 2019
@dscho
Merge branch 'close-graph-everywhere'
fd66f9e 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to git-for-windows/git that referenced this pull request Jun 18, 2019
@dscho
Merge branch 'close-graph-everywhere'
aecbf3a 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to git-for-windows/git that referenced this pull request Jun 19, 2019
@dscho
Merge branch 'close-graph-everywhere'
0cae71b 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to git-for-windows/git that referenced this pull request Jun 21, 2019
@dscho
Merge branch 'close-graph-everywhere'
0dd0803 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
git-for-windows-ci pushed a commit to git-for-windows/git that referenced this pull request Jun 21, 2019
@dscho
Merge branch 'close-graph-everywhere'
287c2ea 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
git-for-windows-ci pushed a commit to git-for-windows/git that referenced this pull request Jun 21, 2019
@dscho
Merge branch 'close-graph-everywhere'
c7218da 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
git-for-windows-ci pushed a commit to git-for-windows/git that referenced this pull request Jul 8, 2019
@dscho
Merge branch 'close-graph-everywhere'
f5d2f34 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
git-for-windows-ci pushed a commit to git-for-windows/git that referenced this pull request Jul 8, 2019
@dscho
Merge branch 'close-graph-everywhere'
e96cae3 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
git-for-windows-ci pushed a commit to git-for-windows/git that referenced this pull request Jul 11, 2019
@dscho
Merge branch 'close-graph-everywhere'
27c5385 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
git-for-windows-ci pushed a commit to git-for-windows/git that referenced this pull request Jul 23, 2019
@dscho
Merge branch 'close-graph-everywhere'
ea77402 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
git-for-windows-ci pushed a commit to git-for-windows/git that referenced this pull request Jul 25, 2019
@dscho
Merge branch 'close-graph-everywhere'
3c1f09c 
This topic branch is a backport of
gitgitgadget#208, which avoids a lock
contention in `git gc --auto` where the `git gc` process holds a read
lock to the `commit-graph` file (if `core.commitgraph=true`) and the
spawned `git commit-graph write` (if `gc.writecommitgraph=true`) tries
to overwrite it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gitgitgadget gitgitgadget[bot] gitgitgadget[bot] left review comments

Assignees
No one assigned
Labels
pu
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@derrickstolee