git/commit-graph.c, branch v2.25.4

Merge branch 'ds/commit-graph-set-size-mult'

2020-01-06T22:17:51Z

The code to write split commit-graph file(s) upon fetching computed bogus value for the parameter used in splitting the resulting files, which has been corrected. * ds/commit-graph-set-size-mult: commit-graph: prefer default size_mult when given zero

commit-graph: prefer default size_mult when given zero

2020-01-02T21:46:34Z

In 50f26bd ("fetch: add fetch.writeCommitGraph config setting", 2019-09-02), the fetch builtin added the capability to write a commit-graph using the "--split" feature. This feature creates multiple commit-graph files, and those can merge based on a set of "split options" including a size multiple. The default size multiple is 2, which intends to provide a log_2 N depth of the commit-graph chain where N is the number of commits. However, I noticed during dogfooding that my commit-graph chains were becoming quite large when left only to builds by 'git fetch'. It turns out that in split_graph_merge_strategy(), we default the size_mult variable to 2 except we override it with the context's split_opts if they exist. In builtin/fetch.c, we create such a split_opts, but do not populate it with values. This problem is due to two failures: 1. It is unclear that we can add the flag COMMIT_GRAPH_WRITE_SPLIT with a NULL split_opts. 2. If we have a non-NULL split_opts, then we override the default values even if a zero value is given. Correct both of these issues. First, do not override size_mult when the options provide a zero value. Second, stop creating a split_opts in the fetch builtin. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano

Merge branch 'ds/commit-graph-delay-gen-progress'

2019-12-10T21:11:43Z

One kind of progress messages were always given during commit-graph generation, instead of following the "if it takes more than two seconds, show progress" pattern, which has been corrected. * ds/commit-graph-delay-gen-progress: commit-graph: use start_delayed_progress() progress: create GIT_PROGRESS_DELAY

Merge branch 'en/doc-typofix'

2019-12-01T17:04:35Z

Docfix. * en/doc-typofix: Fix spelling errors in no-longer-updated-from-upstream modules multimail: fix a few simple spelling errors sha1dc: fix trivial comment spelling error Fix spelling errors in test commands Fix spelling errors in messages shown to users Fix spelling errors in names of tests Fix spelling errors in comments of testcases Fix spelling errors in code comments Fix spelling errors in documentation outside of Documentation/ Documentation: fix a bunch of typos, both old and new

Merge branch 'jk/cleanup-object-parsing-and-fsck'

2019-12-01T17:04:28Z

Crufty code and logic accumulated over time around the object parsing and low-level object access used in "git fsck" have been cleaned up. * jk/cleanup-object-parsing-and-fsck: (23 commits) fsck: accept an oid instead of a "struct tree" for fsck_tree() fsck: accept an oid instead of a "struct commit" for fsck_commit() fsck: accept an oid instead of a "struct tag" for fsck_tag() fsck: rename vague "oid" local variables fsck: don't require an object struct in verify_headers() fsck: don't require an object struct for fsck_ident() fsck: drop blob struct from fsck_finish() fsck: accept an oid instead of a "struct blob" for fsck_blob() fsck: don't require an object struct for report() fsck: only require an oid for skiplist functions fsck: only provide oid/type in fsck_error callback fsck: don't require object structs for display functions fsck: use oids rather than objects for object_name API fsck_describe_object(): build on our get_object_name() primitive fsck: unify object-name code fsck: require an actual buffer for non-blobs fsck: stop checking tag->tagged fsck: stop checking commit->parent counts fsck: stop checking commit->tree value commit, tag: don't set parsed bit for parse failures ...

commit-graph: use start_delayed_progress()

2019-11-27T01:57:10Z

When writing a commit-graph, we show progress along several commit walks. When we use start_delayed_progress(), the progress line will only appear if that step takes a decent amount of time. However, one place was missed: computing generation numbers. This is normally a very fast operation as all commits have been parsed in a previous step. But, this is showing up for all users no matter how few commits are being added. The tests that check for the progress output have already been updated to use GIT_PROGRESS_DELAY=0 to force the expected output. Helped-by: Jeff King Reported-by: ryenus Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano

Fix spelling errors in code comments

2019-11-10T07:00:54Z

Reported-by: Jens Schleusener Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano

Merge branch 'ds/commit-graph-on-fetch'

2019-11-04T04:33:06Z

Regression fix. * ds/commit-graph-on-fetch: commit-graph: fix writing first commit-graph during fetch t5510-fetch.sh: demonstrate fetch.writeCommitGraph bug

commit, tag: don't set parsed bit for parse failures

2019-10-28T05:04:49Z

If we can't parse a commit, then parse_commit() will return an error code. But it _also_ sets the "parsed" flag, which tells us not to bother trying to re-parse the object. That means that subsequent parses have no idea that the information in the struct may be bogus. I.e., doing this: parse_commit(commit); ... if (parse_commit(commit) < 0) die("commit is broken"); will never trigger the die(). The second parse_commit() will see the "parsed" flag and quietly return success. There are two obvious ways to fix this: 1. Stop setting "parsed" until we've successfully parsed. 2. Keep a second "corrupt" flag to indicate that we saw an error (and when the parsed flag is set, return 0/-1 depending on the corrupt flag). This patch does option 1. The obvious downside versus option 2 is that we might continually re-parse a broken object. But in practice, corruption like this is rare, and we typically die() or return an error in the caller. So it's OK not to worry about optimizing for corruption. And it's much simpler: we don't need to use an extra bit in the object struct, and callers which check the "parsed" flag don't need to learn about the corrupt bit, too. There's no new test here, because this case is already covered in t5318. Note that we do need to update the expected message there, because we now detect the problem in the return from "parse_commit()", and not with a separate check for a NULL tree. In fact, we can now ditch that explicit tree check entirely, as we're covered robustly by this change (and the previous recent change to treat a NULL tree as a parse error). We'll also give tags the same treatment. I don't know offhand of any cases where the problem can be triggered (it implies somebody ignoring a parse error earlier in the process), but consistently returning an error should cause the least surprise. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

commit-graph: fix writing first commit-graph during fetch

2019-10-25T02:19:16Z

The previous commit includes a failing test for an issue around fetch.writeCommitGraph and fetching in a repo with a submodule. Here, we fix that bug and set the test to "test_expect_success". The problem arises with this set of commands when the remote repo at has a submodule. Note that --recurse-submodules is not needed to demonstrate the bug. $ git clone test $ cd test $ git -c fetch.writeCommitGraph=true fetch origin Computing commit graph generation numbers: 100% (12/12), done. BUG: commit-graph.c:886: missing parent for commit Aborted (core dumped) As an initial fix, I converted the code in builtin/fetch.c that calls write_commit_graph_reachable() to instead launch a "git commit-graph write --reachable --split" process. That code worked, but is not how we want the feature to work long-term. That test did demonstrate that the issue must be something to do with internal state of the 'git fetch' process. The write_commit_graph() method in commit-graph.c ensures the commits we plan to write are "closed under reachability" using close_reachable(). This method walks from the input commits, and uses the UNINTERESTING flag to mark which commits have already been visited. This allows the walk to take O(N) time, where N is the number of commits, instead of O(P) time, where P is the number of paths. (The number of paths can be exponential in the number of commits.) However, the UNINTERESTING flag is used in lots of places in the codebase. This flag usually means some barrier to stop a commit walk, such as in revision-walking to compare histories. It is not often cleared after the walk completes because the starting points of those walks do not have the UNINTERESTING flag, and clear_commit_marks() would stop immediately. This is happening during a 'git fetch' call with a remote. The fetch negotiation is comparing the remote refs with the local refs and marking some commits as UNINTERESTING. I tested running clear_commit_marks_many() to clear the UNINTERESTING flag inside close_reachable(), but the tips did not have the flag, so that did nothing. It turns out that the calculate_changed_submodule_paths() method is at fault. Thanks, Peff, for pointing out this detail! More specifically, for each submodule, the collect_changed_submodules() runs a revision walk to essentially do file-history on the list of submodules. That revision walk marks commits UNININTERESTING if they are simplified away by not changing the submodule. Instead, I finally arrived on the conclusion that I should use a flag that is not used in any other part of the code. In commit-reach.c, a number of flags were defined for commit walk algorithms. The REACHABLE flag seemed like it made the most sense, and it seems it was not actually used in the file. The REACHABLE flag was used in early versions of commit-reach.c, but was removed by 4fbcca4 (commit-reach: make can_all_from_reach... linear, 2018-07-20). Add the REACHABLE flag to commit-graph.c and use it instead of UNINTERESTING in close_reachable(). This fixes the bug in manual testing. Reported-by: Johannes Schindelin Helped-by: Jeff King Helped-by: Szeder Gábor Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano