Skip to content

Avoid the_repository in merge-ort and replay

Elijah Newren via GitGitGadget git 3 versions – In review

Get series with b4
b4 am [email protected]

Versions newest first · linked by author and title

  1. v3 Latest5 of 6 patches 4 repliesIn review
  2. v26 patches 2 repliesSuperseded
  3. v15 patches 9 repliesSuperseded

Changes in v3 from the cover letter

* In first patch, actually avoid the_repository when attempting to remove check against the_repository
* Fix commit message of patch 3 due to the new patch 1.
* Slight tweak to commit message of patch 6.

5 of 6 patches are in this archive. The cover letter announces 6.

Patches in v3 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/6merge,diff: remove the_repository check before prefetching blobsdiff.c diffcore-break.c diffcore-rename.c +1 more+5 -50 replies
2/6merge-ort: pass repository to write_tree()merge-ort.c+8 -80 replies
3/6merge-ort: replace the_repository with opt->repomerge-ort.c+21 -210 replies
4/6merge-ort: replace the_hash_algo with opt->repo->hash_algomerge-ort.c+13 -130 replies
5/6merge-ort: prevent the_repository from coming backmerge-ort.c+8 -00 replies

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v3 yet.

Read the v3 thread

Cover letter [PATCH v3 0/6] Avoid the_repository in merge-ort and replay

Changes since v2:

* In first patch, actually avoid the_repository when attempting to remove check against the_repository
* Fix commit message of patch 3 due to the new patch 1.
* Slight tweak to commit message of patch 6.

Changes since v1:

* Add a preparatory patch removing the_repository check from blob prefetching in both merge-ort and diff*; it's no longer necessary
* Fix casing mismatch
* Simplify the hammer a bit based on the new first patch, but add some simple comments explaining it

Remove explicit uses of the_repository and the_hash_algo from merge-ort, and since this has now been done multiple times for both merge-ort and replay, implement a small measure to prevent them from returning to either merge-ort or replay.

See
https://lore.kernel.org/git/CABPp-BH7E1Bh2g0vR3T4NEsv34DvFQPzMuJSsqtOAaWY-fFCxg@mail.gmail.com/
and
https://lore.kernel.org/git/CABPp-BFuwvqiCTCCpoyT6em9_1-qrgPWHWhrufQ3UuZ+Kfkb6A@mail.gmail.com/
for recent discussions on these.

As noted in the comments on v1, I actually do not know why prefetch_for_content_merges() needs to use the_repository. When I introduced it back in 2bff554b23e8 (merge-ort: add prefetching for content merges, 2021-06-22), I was just looking at diffcore_std() and trying to mimic how it did the prefetch, and it has such a comparison. If anyone knows why diffcore_std() needs to compare against the_repository, I'd love to hear...

Series overview: Patches 1-3: Mostly mechanical removal of existing uses Patches 4-5: Simple hammer to prevent the problem from returning

Elijah Newren (6):
merge,diff: remove the_repository check before prefetching blobs
merge-ort: pass repository to write_tree()
merge-ort: replace the_repository with opt->repo
merge-ort: replace the_hash_algo with opt->repo->hash_algo
merge-ort: prevent the_repository from coming back
replay: prevent the_repository from coming back

 diff.c            |  2 +-
 diffcore-break.c  |  2 +-
 diffcore-rename.c |  4 +-
 merge-ort.c       | 94 +++++++++++++++++++++++++----------------------
 replay.c          |  6 +++
 5 files changed, 61 insertions(+), 47 deletions(-)

base-commit: 73fd77805fc6406f31c36212846d9e2541d19321
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2048%2Fnewren%2Favoid_the_repository-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2048/newren/avoid_the_repository-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2048

Range-diff vs v2:

1: 7155a0da6f ! 1: e75334a5cf merge,diff: remove the_repository check before prefetching blobs

     @@ diff.c: void diffcore_std(struct diff_options *options)
       	 * decides that it needs inexact rename detection.
       	 */
      -	if (options->repo == the_repository && repo_has_promisor_remote(the_repository) &&
     -+	if (repo_has_promisor_remote(the_repository) &&
     ++	if (repo_has_promisor_remote(options->repo) &&
       	    (options->output_format & output_formats_to_prefetch ||
       	     options->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK))
       		diff_queued_diff_prefetch(options->repo);
     @@ diffcore-break.c: static int should_break(struct repository *r,
       		return 0; /* they are the same */
       
      -	if (r == the_repository && repo_has_promisor_remote(the_repository)) {
     -+	if (repo_has_promisor_remote(the_repository)) {
     ++	if (repo_has_promisor_remote(r)) {
       		options.missing_object_cb = diff_queued_diff_prefetch;
       		options.missing_object_data = r;
       	}
     @@ diffcore-rename.c: static int find_basename_matches(struct diff_options *options
       	}
       
      -	if (options->repo == the_repository && repo_has_promisor_remote(the_repository)) {
     -+	if (repo_has_promisor_remote(the_repository)) {
     ++	if (repo_has_promisor_remote(options->repo)) {
       		dpf_options.missing_object_cb = basename_prefetch;
       		dpf_options.missing_object_data = &prefetch_options;
       	}
     @@ diffcore-rename.c: void diffcore_rename_extended(struct diff_options *options,
       	/* Finish setting up dpf_options */
       	prefetch_options.skip_unmodified = skip_unmodified;
      -	if (options->repo == the_repository && repo_has_promisor_remote(the_repository)) {
     -+	if (repo_has_promisor_remote(the_repository)) {
     ++	if (repo_has_promisor_remote(options->repo)) {
       		dpf_options.missing_object_cb = inexact_prefetch;
       		dpf_options.missing_object_data = &prefetch_options;
       	}
     @@ merge-ort.c: static void prefetch_for_content_merges(struct merge_options *opt,
       	struct oid_array to_fetch = OID_ARRAY_INIT;
       
      -	if (opt->repo != the_repository || !repo_has_promisor_remote(the_repository))
     -+	if (!repo_has_promisor_remote(the_repository))
     ++	if (!repo_has_promisor_remote(opt->repo))
       		return;
       
       	for (e = &plist->items[plist->nr-1]; e >= plist->items; --e) {
 2:  911cba991b = 2:  a9a9d422a3 merge-ort: pass repository to write_tree()
 3:  68af47ed18 ! 3:  4ebfcb08a5 merge-ort: replace the_repository with opt->repo

@@ Commit message
merge-ort: replace the_repository with opt->repo

We have a perfectly valid repository available and do not need to use
- the_repository, except for one location in
- prefetch_for_content_merges().
+ the_repository.

@@ merge-ort.c: static int read_oid_strbuf(struct merge_options *opt,
if (!buf) {
path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,
path, NULL, NULL, NULL,
-@@ merge-ort.c: static void prefetch_for_content_merges(struct merge_options *opt,
- struct string_list_item *e;
- struct oid_array to_fetch = OID_ARRAY_INIT;
-
-- if (!repo_has_promisor_remote(the_repository))
-+ if (!repo_has_promisor_remote(opt->repo))
- return;
-
- for (e = &plist->items[plist->nr-1]; e >= plist->items; --e) {
@@ merge-ort.c: static int checkout(struct merge_options *opt,
unpack_opts.verbose_update = (opt->verbosity > 2);
unpack_opts.fn = twoway_merge;

 4:  bfa68716af = 4:  09076d81b6 merge-ort: replace the_hash_algo with opt->repo->hash_algo
 5:  932d945c9b = 5:  42a2576878 merge-ort: prevent the_repository from coming back
 6:  67db46f34f ! 6:  0654d04584 replay: prevent the_repository from coming back

@@ Commit message
coming back.

Define the_repository to make it a compilation error so that they don't
- come back any more.
+ come back any more; the repo parameter plumbed through the various
+ functions can be used instead.

Signature · 1 line
gitgitgadget