Avoid the_repository in merge-ort and replay
cover Avoid the_repository in merge-ort and replay
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.
Series overview: Patches 1-3: Mostly mechanical removal of existing uses Patches 4-5: Simple hammer to prevent the problem from returning
Elijah Newren (5):
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
merge-ort.c | 92 ++++++++++++++++++++++++++++------------------------- replay.c | 2 ++ 2 files changed, 51 insertions(+), 43 deletions(-)
base-commit: 73fd77805fc6406f31c36212846d9e2541d19321
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2048%2Fnewren%2Favoid_the_repository-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2048/newren/avoid_the_repository-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2048
Signature · 1 line
gitgitgadget
PATCH 1/5 merge-ort: pass repository to write_tree()
From: Elijah Newren <[email protected]>
In order to get rid of a usage of the_repository, we need to know the value of opt->repo; pass it along to write_tree(). Once we have the repository, though, we no longer need to pass opt->repo->hash_algo->rawsz, we can have write_tree() look up that value itself.
--- merge-ort.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
merge-ort.c +8 -8
@@ -3822,15 +3822,16 @@ static int tree_entry_order(const void *a_, const void *b_) b->string, strlen(b->string), bmi->result.mode); } -static int write_tree (struct object_id *result_oid, +static int write_tree (struct repository *repo, +struct object_id *result_oid,struct string_list *versions, -unsigned int offset, -size_t hash_size) +unsigned int offset) {size_t maxlen =0 , extra;unsigned int nr;struct strbuf buf =STRBUF_INIT ;int i, ret =0 ; +size_t hash_size = repo->hash_algo->rawsz;assert (offset <= versions->nr); nr = versions->nr - offset;
@@ -3856,7 +3857,7 @@ static int write_tree(struct object_id *result_oid, }/* Write this object file out, and record in result_oid */ -if (odb_write_object (the_repository->objects, buf.buf, +if (odb_write_object (repo->objects, buf.buf, buf.len,OBJ_TREE , result_oid)) ret =-1 ;strbuf_release (&buf);
@@ -4026,8 +4027,8 @@ static int write_completed_directory(struct merge_options *opt, dir_info->is_null =0 ; dir_info->result.mode =S_IFDIR ;if (record_tree && -write_tree (&dir_info->result.oid, &info->versions, offset, - opt->repo->hash_algo->rawsz) <0 ) +write_tree (opt->repo, &dir_info->result.oid, &info->versions, + offset) <0 ) ret =-1 ; }
@@ -4573,8 +4574,7 @@ static int process_entries(struct merge_options *opt,BUG ("dir_metadata accounting completely off; shouldn't happen" ); }if (record_tree && -write_tree (result_oid, &dir_metadata.versions,0 , - opt->repo->hash_algo->rawsz) <0 ) +write_tree (opt->repo, result_oid, &dir_metadata.versions,0 ) <0 ) ret =-1 ; cleanup:string_list_clear (&plist,0 );
Signature · 1 line
gitgitgadget
PATCH 2/5 merge-ort: replace the_repository with opt->repo
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
We have a perfectly valid repository available and do not need to use the_repository, except for one location in prefetch_for_content_merges().
--- merge-ort.c | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-)
merge-ort.c +22 -22
@@ -1732,9 +1732,9 @@ static int collect_merge_info(struct merge_options *opt, info.data = opt; info.show_all_errors =1 ; -if (repo_parse_tree (the_repository, merge_base) <0 || -repo_parse_tree (the_repository, side1) <0 || -repo_parse_tree (the_repository, side2) <0 ) +if (repo_parse_tree (opt->repo, merge_base) <0 || +repo_parse_tree (opt->repo, side1) <0 || +repo_parse_tree (opt->repo, side2) <0 )return -1 ;init_tree_desc (t +0 , &merge_base->object.oid, merge_base->buffer, merge_base->size);
@@ -2136,9 +2136,9 @@ static int merge_3way(struct merge_options *opt, name2 =mkpathdup ("%s:%s" , opt->branch2, pathnames[2 ]); } -read_mmblob (&orig, the_repository->objects, o); -read_mmblob (&src1, the_repository->objects, a); -read_mmblob (&src2, the_repository->objects, b); +read_mmblob (&orig, opt->repo->objects, o); +read_mmblob (&src1, opt->repo->objects, a); +read_mmblob (&src2, opt->repo->objects, b); merge_status = ll_merge(result_buf, path, &orig, base, &src1, name1, &src2, name2,
@@ -2254,7 +2254,7 @@ static int handle_content_merge(struct merge_options *opt, }if (!ret && record_object && -odb_write_object (the_repository->objects, result_buf.ptr, result_buf.size, +odb_write_object (opt->repo->objects, result_buf.ptr, result_buf.size,OBJ_BLOB , &result->oid)) { path_msg(opt, ERROR_OBJECT_WRITE_FAILED, 0, pathnames[0], pathnames[1], pathnames[2], NULL,
@@ -3713,7 +3713,7 @@ static int read_oid_strbuf(struct merge_options *opt,void *buf;enum object_type type;unsigned long size; - buf =odb_read_object (the_repository->objects, oid, &type, &size); + buf =odb_read_object (opt->repo->objects, oid, &type, &size);if (!buf) { path_msg(opt, ERROR_OBJECT_READ_FAILED, 0, path, NULL, NULL, NULL,
@@ -4439,7 +4439,7 @@ static void prefetch_for_content_merges(struct merge_options *opt,struct string_list_item *e;struct oid_array to_fetch =OID_ARRAY_INIT ; -if (opt->repo != the_repository || !repo_has_promisor_remote (the_repository)) +if (opt->repo != the_repository || !repo_has_promisor_remote (opt->repo))return ;for (e = &plist->items[plist->nr-1 ]; e >= plist->items; --e) {
@@ -4619,10 +4619,10 @@ static int checkout(struct merge_options *opt, unpack_opts.verbose_update = (opt->verbosity >2 ); unpack_opts.fn = twoway_merge; unpack_opts.preserve_ignored =0 ;/* FIXME: !opts->overwrite_ignore */ -if (repo_parse_tree (the_repository, prev) <0 ) +if (repo_parse_tree (opt->repo, prev) <0 )return -1 ;init_tree_desc (&trees[0 ], &prev->object.oid, prev->buffer, prev->size); -if (repo_parse_tree (the_repository, next) <0 ) +if (repo_parse_tree (opt->repo, next) <0 )return -1 ;init_tree_desc (&trees[1 ], &next->object.oid, next->buffer, next->size);
@@ -5280,7 +5280,7 @@ redo:if (result->clean >=0 ) {if (!opt->mergeability_only) { - result->tree =repo_parse_tree_indirect (the_repository, + result->tree =repo_parse_tree_indirect (opt->repo, &working_tree_oid);if (!result->tree) die(_("unable to read tree (%s)"),
@@ -5309,7 +5309,7 @@ static void merge_ort_internal(struct merge_options *opt,struct strbuf merge_base_abbrev =STRBUF_INIT ;if (!merge_bases) { -if (repo_get_merge_bases (the_repository, h1, h2, +if (repo_get_merge_bases (opt->repo, h1, h2, &merge_bases) <0 ) { result->clean =-1 ; goto out;
@@ -5440,20 +5440,20 @@ static void merge_recursive_config(struct merge_options *opt, int ui) {char *value =NULL ;int renormalize =0 ; -repo_config_get_int (the_repository,"merge.verbosity" , &opt->verbosity); -repo_config_get_int (the_repository,"diff.renamelimit" , &opt->rename_limit); -repo_config_get_int (the_repository,"merge.renamelimit" , &opt->rename_limit); -repo_config_get_bool (the_repository,"merge.renormalize" , &renormalize); +repo_config_get_int (opt->repo,"merge.verbosity" , &opt->verbosity); +repo_config_get_int (opt->repo,"diff.renamelimit" , &opt->rename_limit); +repo_config_get_int (opt->repo,"merge.renamelimit" , &opt->rename_limit); +repo_config_get_bool (opt->repo,"merge.renormalize" , &renormalize); opt->renormalize = renormalize; -if (!repo_config_get_string (the_repository,"diff.renames" , &value)) { +if (!repo_config_get_string (opt->repo,"diff.renames" , &value)) { opt->detect_renames =git_config_rename ("diff.renames" , value);free (value); } -if (!repo_config_get_string (the_repository,"merge.renames" , &value)) { +if (!repo_config_get_string (opt->repo,"merge.renames" , &value)) { opt->detect_renames =git_config_rename ("merge.renames" , value);free (value); } -if (!repo_config_get_string (the_repository,"merge.directoryrenames" , &value)) { +if (!repo_config_get_string (opt->repo,"merge.directoryrenames" , &value)) {int boolval =git_parse_maybe_bool (value);if (0 <= boolval) { opt->detect_directory_renames = boolval ?
@@ -5466,7 +5466,7 @@ static void merge_recursive_config(struct merge_options *opt, int ui)free (value); }if (ui) { -if (!repo_config_get_string (the_repository,"diff.algorithm" , &value)) { +if (!repo_config_get_string (opt->repo,"diff.algorithm" , &value)) {long diff_algorithm =parse_algorithm_value (value);if (diff_algorithm <0 )die (_ ("unknown value for config '%s': %s" ),"diff.algorithm" , value);
@@ -5474,7 +5474,7 @@ static void merge_recursive_config(struct merge_options *opt, int ui)free (value); } } -repo_config (the_repository, git_xmerge_config,NULL ); +repo_config (opt->repo, git_xmerge_config,NULL ); } static void init_merge_options(struct merge_options *opt,
Signature · 1 line
gitgitgadget
PATCH 3/5 merge-ort: replace the_hash_algo with opt->repo->hash_algo
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
--- merge-ort.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
merge-ort.c +13 -13
@@ -1857,7 +1857,7 @@ static int merge_submodule(struct merge_options *opt,BUG ("submodule deleted on one side; this should be handled outside of merge_submodule()" );if ((sub_not_initialized =repo_submodule_init (&subrepo, - opt->repo, path,null_oid (the_hash_algo)))) { + opt->repo, path,null_oid (opt->repo->hash_algo)))) { path_msg(opt, CONFLICT_SUBMODULE_NOT_INITIALIZED, 0, path, NULL, NULL, NULL, _("Failed to merge submodule %s (not checked out)"),
@@ -2240,7 +2240,7 @@ static int handle_content_merge(struct merge_options *opt, two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode)); merge_status =merge_3way (opt, path, - two_way ?null_oid (the_hash_algo) : &o->oid, + two_way ?null_oid (opt->repo->hash_algo) : &o->oid, &a->oid, &b->oid, pathnames, extra_marker_size, &result_buf);
@@ -2272,7 +2272,7 @@ static int handle_content_merge(struct merge_options *opt, }else if (S_ISGITLINK (a->mode)) {int two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode)); clean =merge_submodule (opt, pathnames[0 ], - two_way ?null_oid (the_hash_algo) : &o->oid, + two_way ?null_oid (opt->repo->hash_algo) : &o->oid, &a->oid, &b->oid, &result->oid);if (clean <0 )return -1 ;
@@ -2786,7 +2786,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,assert (!new_ci->match_mask); new_ci->dirmask =0 ; new_ci->stages[1 ].mode =0 ; -oidcpy (&new_ci->stages[1 ].oid,null_oid (the_hash_algo)); +oidcpy (&new_ci->stages[1 ].oid,null_oid (opt->repo->hash_algo));/* * Now that we have the file information in new_ci, make sure
@@ -2799,7 +2799,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,continue ;/* zero out any entries related to files */ ci->stages[i].mode =0 ; -oidcpy (&ci->stages[i].oid,null_oid (the_hash_algo)); +oidcpy (&ci->stages[i].oid,null_oid (opt->repo->hash_algo)); }/* Now we want to focus on new_ci, so reassign ci to it. */
@@ -3214,7 +3214,7 @@ static int process_renames(struct merge_options *opt,if (type_changed) {/* rename vs. typechange */ /* Mark the original as resolved by removal */ -memcpy (&oldinfo->stages[0 ].oid,null_oid (the_hash_algo), +memcpy (&oldinfo->stages[0 ].oid,null_oid (opt->repo->hash_algo),sizeof (oldinfo->stages[0 ].oid)); oldinfo->stages[0 ].mode =0 ; oldinfo->filemask &=0x06 ;
@@ -4102,7 +4102,7 @@ static int process_entry(struct merge_options *opt,if (ci->filemask & (1 << i))continue ; ci->stages[i].mode =0 ; -oidcpy (&ci->stages[i].oid,null_oid (the_hash_algo)); +oidcpy (&ci->stages[i].oid,null_oid (opt->repo->hash_algo)); } }else if (ci->df_conflict && ci->merged.result.mode !=0 ) {/*
@@ -4149,7 +4149,7 @@ static int process_entry(struct merge_options *opt,continue ;/* zero out any entries related to directories */ new_ci->stages[i].mode =0 ; -oidcpy (&new_ci->stages[i].oid,null_oid (the_hash_algo)); +oidcpy (&new_ci->stages[i].oid,null_oid (opt->repo->hash_algo)); }/*
@@ -4271,11 +4271,11 @@ static int process_entry(struct merge_options *opt, new_ci->merged.result.mode = ci->stages[2 ].mode;oidcpy (&new_ci->merged.result.oid, &ci->stages[2 ].oid); new_ci->stages[1 ].mode =0 ; -oidcpy (&new_ci->stages[1 ].oid,null_oid (the_hash_algo)); +oidcpy (&new_ci->stages[1 ].oid,null_oid (opt->repo->hash_algo)); new_ci->filemask =5 ;if ((S_IFMT & b_mode) != (S_IFMT & o_mode)) { new_ci->stages[0 ].mode =0 ; -oidcpy (&new_ci->stages[0 ].oid,null_oid (the_hash_algo)); +oidcpy (&new_ci->stages[0 ].oid,null_oid (opt->repo->hash_algo)); new_ci->filemask =4 ; }
@@ -4283,11 +4283,11 @@ static int process_entry(struct merge_options *opt, ci->merged.result.mode = ci->stages[1 ].mode;oidcpy (&ci->merged.result.oid, &ci->stages[1 ].oid); ci->stages[2 ].mode =0 ; -oidcpy (&ci->stages[2 ].oid,null_oid (the_hash_algo)); +oidcpy (&ci->stages[2 ].oid,null_oid (opt->repo->hash_algo)); ci->filemask =3 ;if ((S_IFMT & a_mode) != (S_IFMT & o_mode)) { ci->stages[0 ].mode =0 ; -oidcpy (&ci->stages[0 ].oid,null_oid (the_hash_algo)); +oidcpy (&ci->stages[0 ].oid,null_oid (opt->repo->hash_algo)); ci->filemask =2 ; }
@@ -4415,7 +4415,7 @@ static int process_entry(struct merge_options *opt,/* Deleted on both sides */ ci->merged.is_null =1 ; ci->merged.result.mode =0 ; -oidcpy (&ci->merged.result.oid,null_oid (the_hash_algo)); +oidcpy (&ci->merged.result.oid,null_oid (opt->repo->hash_algo));assert (!ci->df_conflict); ci->merged.clean = !ci->path_conflict; }
Signature · 1 line
gitgitgadget
Re: [PATCH 3/5] merge-ort: replace the_hash_algo with opt->repo->hash_algo
Quoted 1 line · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
Nit: might make sense to have at least a oneliner here to explain what we're doing, even if the subject already says it all.
Patrick
Re: [PATCH 3/5] merge-ort: replace the_hash_algo with opt->repo->hash_algo
Quoted 6 lines · Patrick Steinhardt
Quoted 1 line · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
Nit: might make sense to have at least a oneliner here to explain what we're doing, even if the subject already says it all.
Fair enough; will do.
PATCH 4/5 merge-ort: prevent the_repository from coming back
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
There are two things preventing us from removing our usage of USE_THE_REPOSITORY_VARIABLE: one necessary use of the_repository in prefetch_for_content_merges(), and the use of DEFAULT_ABBREV. We have removed all other uses of the_repository in merge-ort before (multiple times), but without removing that definition, they keep coming back.
Define the_repository to make it a compilation error so that they don't come back any more, with a special carve-out for prefetch_for_content_merges().
--- merge-ort.c | 6 ++++++ 1 file changed, 6 insertions(+)
merge-ort.c +6 -0
@@ -53,6 +53,8 @@#include "unpack-trees.h" #include "xdiff-interface.h" +#define the_repository DO_NOT_USE_THE_REPOSITORY +/* * We have many arrays of size 3. Whenever we have such an array, the * indices refer to one of the sides of the three-way merge. This is so
@@ -4433,6 +4435,8 @@ static int process_entry(struct merge_options *opt,return 0 ; } +#undef the_repository +static void prefetch_for_content_merges (struct merge_options *opt,struct string_list *plist) {
@@ -4481,6 +4485,8 @@ static void prefetch_for_content_merges(struct merge_options *opt,oid_array_clear (&to_fetch); } +#define the_repository DO_NOT_USE_the_repository +static int process_entries (struct merge_options *opt,struct object_id *result_oid) {
Signature · 1 line
gitgitgadget
Re: [PATCH 4/5] merge-ort: prevent the_repository from coming back
Quoted 15 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
There are two things preventing us from removing our usage of
USE_THE_REPOSITORY_VARIABLE: one necessary use ofthe_repositoryinprefetch_for_content_merges(), and the use ofDEFAULT_ABBREV. We have removed all other uses ofthe_repositoryin merge-ort before (multiple times), but without removing that definition, they keep coming back.Define
the_repositoryto make it a compilation error so that they don't come back any more, with a special carve-out forprefetch_for_content_merges().Signed-off-by Elijah Newren <[email protected]>---
[snip]
merge-ort.c
Quoted 4 lines
merge-ort.c +3
-0+#define the_repository DO_NOT_USE_THE_REPOSITORY + +#define the_repository DO_NOT_USE_the_repository
Here the casing is different?
Quoted 4 lines
merge-ort.c +1
-0+static int process_entries (struct merge_options *opt,struct object_id *result_oid) {
Re: [PATCH 4/5] merge-ort: prevent the_repository from coming back
Quoted 14 lines · Kristoffer Haugsbakk
Quoted 12 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
There are two things preventing us from removing our usage of
USE_THE_REPOSITORY_VARIABLE: one necessary use ofthe_repositoryinprefetch_for_content_merges(), and the use ofDEFAULT_ABBREV. We have removed all other uses ofthe_repositoryin merge-ort before (multiple times), but without removing that definition, they keep coming back.Define
the_repositoryto make it a compilation error so that they don't come back any more, with a special carve-out forprefetch_for_content_merges().
Quoted 9 lines
merge-ort.c
Quoted 7 lines
Signed-off-by Elijah Newren <[email protected]>---
[snip]merge-ort.c +3
-0+#define the_repository DO_NOT_USE_THE_REPOSITORY + +#define the_repository DO_NOT_USE_the_repositoryHere the casing is different?
Oops. Will fix; thanks for taking a look.
Re: [PATCH 4/5] merge-ort: prevent the_repository from coming back
In reply to Elijah Newren via GitGitGadgetQuoted 7 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
There are two things preventing us from removing our usage of
USE_THE_REPOSITORY_VARIABLE: one necessary use ofthe_repositoryinprefetch_for_content_merges(), and the use ofDEFAULT_ABBREV. We have removed all other uses ofthe_repositoryin merge-ort before (multiple times), but without removing that definition, they keep coming back.
Unfortunate, but I think it's okay to be pragmatic here.
merge-ort.c line 56
Quoted 9 lines
merge-ort.c +1
-0@@ -53,6 +53,8 @@#include "unpack-trees.h" #include "xdiff-interface.h" +#define the_repository DO_NOT_USE_THE_REPOSITORY
Do we want to have a comment here that explains why we do this dance?
Patrick
Re: [PATCH 4/5] merge-ort: prevent the_repository from coming back
Quoted 11 lines · Patrick Steinhardt
Quoted 7 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
There are two things preventing us from removing our usage of
USE_THE_REPOSITORY_VARIABLE: one necessary use ofthe_repositoryinprefetch_for_content_merges(), and the use ofDEFAULT_ABBREV. We have removed all other uses ofthe_repositoryin merge-ort before (multiple times), but without removing that definition, they keep coming back.Unfortunate, but I think it's okay to be pragmatic here.
Yeah, also full disclosure: I 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. I don't actually understand why the comparison against the_repository is there for either of these functions. Maybe someone else knows and could shed some light? (cc:
Jonathan Tan for the diffcore_std() case I was copying from...)
Quoted 11 lines
merge-ort.c line 56
Quoted 9 lines
merge-ort.c +1
-0@@ -53,6 +53,8 @@#include "unpack-trees.h" #include "xdiff-interface.h" +#define the_repository DO_NOT_USE_THE_REPOSITORYDo we want to have a comment here that explains why we do this dance?
Sure, will do.
Re: [PATCH 4/5] merge-ort: prevent the_repository from coming back
Quoted 8 lines · Elijah Newren
Yeah, also full disclosure: I do not know why
prefetch_for_content_merges()needs to usethe_repository. When I introduced it back in2bff554b23e8(merge-ort: add prefetching for content merges, 2021-06-22), I was just looking atdiffcore_std()and trying to mimic how it did the prefetch. I don't actually understand why the comparison againstthe_repositoryis there for either of these functions. Maybe someone else knows and could shed some light? (cc:
Jonathan Tan for thediffcore_std()case I was copying from...)
I did a bit of digging for you ;-)
The comparison with the_repository is from 7fbbcb21 (diff: batch fetching of missing blobs, 2019-04-05), whose original version did not have it, but was later amended with
https://lore.kernel.org/git/[email protected]/
And after that it survived across evolutions like b14ed5ad (Use promisor_remote_get_direct() and has_promisor_remote(), 2019-06-25), 95acf11a (diff: restrict when prefetching occurs, 2020-04-07), and finally a5183d76 (cocci: apply the "promisor-remote.h" part of "the_repository.pending", 2023-03-28).
Re: [PATCH 4/5] merge-ort: prevent the_repository from coming back
Quoted 12 lines · Junio C Hamano
Quoted 8 lines · Elijah Newren
Yeah, also full disclosure: I do not know why
prefetch_for_content_merges()needs to usethe_repository. When I introduced it back in2bff554b23e8(merge-ort: add prefetching for content merges, 2021-06-22), I was just looking atdiffcore_std()and trying to mimic how it did the prefetch. I don't actually understand why the comparison againstthe_repositoryis there for either of these functions. Maybe someone else knows and could shed some light? (cc:
Jonathan Tan for thediffcore_std()case I was copying from...)
Quoted 1 line
I did a bit of digging for you ;-)
Thanks!
Quoted 5 lines
The comparison with
the_repositoryis from7fbbcb21(diff: batch fetching of missing blobs, 2019-04-05), whose original version did not have it, but was later amended with
Ah, the explanation in that email doesn't exist inside the commit history, since the diff was squashed in and the relevant part of the explanation wasn't added to the commit message. I should have thought to check the mail archives too. Anyway, the important bit is:
Quoted 3 lines
Also, prefetch only if the repository being diffed
isthe_repository(because we do not support lazy fetching for any other repository anyway).
This comment came from mid-2019. I then copied the logic from diffcore_std() in 2020 (though didn't get the relevant patch upstream until 2021), and as of 2021 we have ef830cc43412 (promisor-remote:
teach lazy-fetch in any repo, 2021-06-17), which means that this check has been obsoleted by events. And it looks like the check in diffcore_std() was left in place as an oversight as well.
So, we don't need that check anymore. I'll add a commit that simply removes these checks from both merge-ort.c and diff.c and reduces two more uses of the_repository.
PATCH 5/5 replay: prevent the_repository from coming back
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
Due to the use of DEFAULT_ABBREV, we cannot get rid of our usage of USE_THE_REPOSITORY_VARIABLE. We have removed all other uses of the_repository before, but without removing that definition, they keep coming back.
Define the_repository to make it a compilation error so that they don't come back any more.
--- replay.c | 2 ++ 1 file changed, 2 insertions(+)
replay.c +2 -0
@@ -11,6 +11,8 @@#include "strmap.h" #include "tree.h" +#define the_repository DO_NOT_USE_THE_REPOSITORY +static const char *short_commit_name (struct repository *repo,struct commit *commit) {
Signature · 1 line
gitgitgadget
Re: [PATCH 5/5] replay: prevent the_repository from coming back
replay.c line 14
Quoted 9 lines · Elijah Newren via GitGitGadget
replay.c +1
-0@@ -11,6 +11,8 @@#include "strmap.h" #include "tree.h" +#define the_repository DO_NOT_USE_THE_REPOSITORY
Same remark here: might make sense to add a comment here to explain what's going on.
Other than that the series is a welcome cleanup, thanks!
Patrick
v2 cover Avoid the_repository in merge-ort and replay
In reply to Elijah Newren via GitGitGadgetChanges 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-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2048/newren/avoid_the_repository-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2048
Range-diff vs v1:
-: ---------- > 1: 7155a0da6f merge,diff: remove the_repository check before prefetching blobs
1: 620c4ea38b = 2: 911cba991b merge-ort: pass repository to write_tree()
2: abba4bd762 ! 3: 68af47ed18 merge-ort: replace the_repository with opt->repo
@@ 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 (opt->repo != the_repository || !repo_has_promisor_remote(the_repository))
-+ if (opt->repo != the_repository || !repo_has_promisor_remote(opt->repo))
+- 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) { 3: 36c2713ceb ! 4: bfa68716af merge-ort: replace the_hash_algo with opt->repo->hash_algo
@@ Metadata
## Commit message ##
merge-ort: replace the_hash_algo with opt->repo->hash_algo
+ We have a perfectly valid repository available and do not need to use
+ the_hash_algo (a shorthand for the_repository->hash_algo), so use the
+ known repository instead.
+
## merge-ort.c ##
4: 46c24e0d05 ! 5: 932d945c9b merge-ort: prevent the_repository from coming back
@@ Metadata
## Commit message ##
merge-ort: prevent the_repository from coming back
- There are two things preventing us from removing our usage of
- USE_THE_REPOSITORY_VARIABLE: one necessary use of the_repository in
- prefetch_for_content_merges(), and the use of DEFAULT_ABBREV. We have
- removed all other uses of the_repository in merge-ort before (multiple
- times), but without removing that definition, they keep coming back.
+ Due to the use of DEFAULT_ABBREV, we cannot get rid of our usage of
+ USE_THE_REPOSITORY_VARIABLE. However, we have removed all other uses of
+ the_repository in merge-ort a few times. But they keep coming back.
Define the_repository to make it a compilation error so that they don't
- come back any more, with a special carve-out for
- prefetch_for_content_merges().
+ come back any more.
@@ merge-ort.c
#include "unpack-trees.h"
#include "xdiff-interface.h"
++/*
++ * We technically need USE_THE_REPOSITORY_VARIABLE above for DEFAULT_ABBREV, ++ * but do not want more uses of the_repository. Prevent them.
++ *
++ * opt->repo is available; use it instead.
++ */
+#define the_repository DO_NOT_USE_THE_REPOSITORY
+
/*
* We have many arrays of size 3. Whenever we have such an array, the
* indices refer to one of the sides of the three-way merge. This is so
-@@ merge-ort.c: static int process_entry(struct merge_options *opt,
- return 0;
- }
-
-+#undef the_repository
-+
- static void prefetch_for_content_merges(struct merge_options *opt,
- struct string_list *plist)
- {
-@@ merge-ort.c: static void prefetch_for_content_merges(struct merge_options *opt,
- oid_array_clear(&to_fetch);
- }
-
-+#define the_repository DO_NOT_USE_the_repository
-+
- static int process_entries(struct merge_options *opt,
- struct object_id *result_oid)
- {
5: d75a71aef9 ! 6: 67db46f34f replay: prevent the_repository from coming back
@@ replay.c
#include "strmap.h"
#include "tree.h"
++/*
++ * We technically need USE_THE_REPOSITORY_VARIABLE for DEFAULT_ABBREV, but
++ * do not want to use the_repository.
++ */
+#define the_repository DO_NOT_USE_THE_REPOSITORY
+
static const char *short_commit_name(struct repository *repo,Signature · 1 line
gitgitgadget
PATCH v2 1/6 merge,diff: remove the_repository check before prefetching blobs
From: Elijah Newren <[email protected]>
Prefetching of blobs from promisor remotes was added to diff in 7fbbcb21b162 (diff: batch fetching of missing blobs, 2019-04-05). In that commit,
https://lore.kernel.org/git/[email protected]/
was squashed into
without the extra explanation about the squashed changes being added to the commit message; in particular, this explanation from that first link is absent:
Quoted 3 lines
Also, prefetch only if the repository being diffed is
the_repository(because we do not support lazy fetching for any other repository
anyway).
Then, later, this checking was spread from diff.c to diffcore-rename.c and diffcore-break.c by 95acf11a3dc3 (diff: restrict when prefetching occurs, 2020-04-07) and then further split in d331dd3b0c82 (diffcore-rename: allow different missing_object_cb functions, 2021-06-22). I also copied the logic from prefetching blobs from diff.c to merge-ort.c in 2bff554b23e8 (merge-ort: add prefetching for content merges, 2021-06-22).
The reason for all these checks was noted above -- we only supported lazy fetching for the_repository. However, that changed with ef830cc43412 (promisor-remote: teach lazy-fetch in any repo, 2021-06-17), so these checks are now unnecessary. Remove them.
--- diff.c | 2 +- diffcore-break.c | 2 +- diffcore-rename.c | 4 ++-- merge-ort.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)
diff.c +1 -1
@@ -7176,7 +7176,7 @@ void diffcore_std(struct diff_options *options)* If no prefetching occurs, diffcore_rename() will prefetch if it * 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) && (options->output_format & output_formats_to_prefetch || options->pickaxe_opts &DIFF_PICKAXE_KINDS_MASK ))diff_queued_diff_prefetch (options->repo);
diffcore-break.c +1 -1
@@ -69,7 +69,7 @@ static int should_break(struct repository *r, oideq(&src->oid, &dst->oid))return 0 ;/* they are the same */ -if (r == the_repository &&repo_has_promisor_remote (the_repository)) { +if (repo_has_promisor_remote (the_repository)) { options.missing_object_cb = diff_queued_diff_prefetch; options.missing_object_data = r; }
diffcore-rename.c +2 -2
@@ -987,7 +987,7 @@ static int find_basename_matches(struct diff_options *options,strintmap_set (&dests, base, i); } -if (options->repo == the_repository &&repo_has_promisor_remote (the_repository)) { +if (repo_has_promisor_remote (the_repository)) { dpf_options.missing_object_cb = basename_prefetch; dpf_options.missing_object_data = &prefetch_options; }
@@ -1574,7 +1574,7 @@ 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)) { dpf_options.missing_object_cb = inexact_prefetch; dpf_options.missing_object_data = &prefetch_options; }
merge-ort.c +1 -1
@@ -4438,7 +4438,7 @@ static void prefetch_for_content_merges(struct merge_options *opt,struct string_list_item *e;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))return ;for (e = &plist->items[plist->nr-1 ]; e >= plist->items; --e) {
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/6] merge,diff: remove the_repository check before prefetching blobs
diff.c void diffcore_std(struct diff_options *options) · line 7179
Quoted 10 lines · Elijah Newren via GitGitGadget
diff.c +1
-1@@ -7176,7 +7176,7 @@ void diffcore_std(struct diff_options *options)* If no prefetching occurs, diffcore_rename() will prefetch if it * 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) &&
I wonder though -- shouldn't we also pass options->repo to repo_has_promisor_remote() now? Otherwise we may support backfill fetches from arbitrary repositories, but we'll only do them in case the main repository has a promisor remote.
Patrick
Re: [PATCH v2 1/6] merge,diff: remove the_repository check before prefetching blobs
Quoted 17 lines · Patrick Steinhardt
diff.c void diffcore_std(struct diff_options *options) · line 7179
Quoted 10 lines · Elijah Newren via GitGitGadget
diff.c +1
-1@@ -7176,7 +7176,7 @@ void diffcore_std(struct diff_options *options)* If no prefetching occurs, diffcore_rename() will prefetch if it * 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) &&I wonder though -- shouldn't we also pass
options->repotorepo_has_promisor_remote()now? Otherwise we may support backfill fetches from arbitrary repositories, but we'll only do them in case the main repository has a promisor remote.
Doh. Definitely yes. Amid having this patch originally be last and rebasing and whatnot, I didn't look closely enough and missed that. Will fix all the callsites and resubmit.
PATCH v2 2/6 merge-ort: pass repository to write_tree()
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
In order to get rid of a usage of the_repository, we need to know the value of opt->repo; pass it along to write_tree(). Once we have the repository, though, we no longer need to pass opt->repo->hash_algo->rawsz, we can have write_tree() look up that value itself.
--- merge-ort.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
merge-ort.c +8 -8
@@ -3822,15 +3822,16 @@ static int tree_entry_order(const void *a_, const void *b_) b->string, strlen(b->string), bmi->result.mode); } -static int write_tree (struct object_id *result_oid, +static int write_tree (struct repository *repo, +struct object_id *result_oid,struct string_list *versions, -unsigned int offset, -size_t hash_size) +unsigned int offset) {size_t maxlen =0 , extra;unsigned int nr;struct strbuf buf =STRBUF_INIT ;int i, ret =0 ; +size_t hash_size = repo->hash_algo->rawsz;assert (offset <= versions->nr); nr = versions->nr - offset;
@@ -3856,7 +3857,7 @@ static int write_tree(struct object_id *result_oid, }/* Write this object file out, and record in result_oid */ -if (odb_write_object (the_repository->objects, buf.buf, +if (odb_write_object (repo->objects, buf.buf, buf.len,OBJ_TREE , result_oid)) ret =-1 ;strbuf_release (&buf);
@@ -4026,8 +4027,8 @@ static int write_completed_directory(struct merge_options *opt, dir_info->is_null =0 ; dir_info->result.mode =S_IFDIR ;if (record_tree && -write_tree (&dir_info->result.oid, &info->versions, offset, - opt->repo->hash_algo->rawsz) <0 ) +write_tree (opt->repo, &dir_info->result.oid, &info->versions, + offset) <0 ) ret =-1 ; }
@@ -4573,8 +4574,7 @@ static int process_entries(struct merge_options *opt,BUG ("dir_metadata accounting completely off; shouldn't happen" ); }if (record_tree && -write_tree (result_oid, &dir_metadata.versions,0 , - opt->repo->hash_algo->rawsz) <0 ) +write_tree (opt->repo, result_oid, &dir_metadata.versions,0 ) <0 ) ret =-1 ; cleanup:string_list_clear (&plist,0 );
Signature · 1 line
gitgitgadget
PATCH v2 3/6 merge-ort: replace the_repository with opt->repo
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
We have a perfectly valid repository available and do not need to use the_repository, except for one location in prefetch_for_content_merges().
--- merge-ort.c | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-)
merge-ort.c +22 -22
@@ -1732,9 +1732,9 @@ static int collect_merge_info(struct merge_options *opt, info.data = opt; info.show_all_errors =1 ; -if (repo_parse_tree (the_repository, merge_base) <0 || -repo_parse_tree (the_repository, side1) <0 || -repo_parse_tree (the_repository, side2) <0 ) +if (repo_parse_tree (opt->repo, merge_base) <0 || +repo_parse_tree (opt->repo, side1) <0 || +repo_parse_tree (opt->repo, side2) <0 )return -1 ;init_tree_desc (t +0 , &merge_base->object.oid, merge_base->buffer, merge_base->size);
@@ -2136,9 +2136,9 @@ static int merge_3way(struct merge_options *opt, name2 =mkpathdup ("%s:%s" , opt->branch2, pathnames[2 ]); } -read_mmblob (&orig, the_repository->objects, o); -read_mmblob (&src1, the_repository->objects, a); -read_mmblob (&src2, the_repository->objects, b); +read_mmblob (&orig, opt->repo->objects, o); +read_mmblob (&src1, opt->repo->objects, a); +read_mmblob (&src2, opt->repo->objects, b); merge_status = ll_merge(result_buf, path, &orig, base, &src1, name1, &src2, name2,
@@ -2254,7 +2254,7 @@ static int handle_content_merge(struct merge_options *opt, }if (!ret && record_object && -odb_write_object (the_repository->objects, result_buf.ptr, result_buf.size, +odb_write_object (opt->repo->objects, result_buf.ptr, result_buf.size,OBJ_BLOB , &result->oid)) { path_msg(opt, ERROR_OBJECT_WRITE_FAILED, 0, pathnames[0], pathnames[1], pathnames[2], NULL,
@@ -3713,7 +3713,7 @@ static int read_oid_strbuf(struct merge_options *opt,void *buf;enum object_type type;unsigned long size; - buf =odb_read_object (the_repository->objects, oid, &type, &size); + buf =odb_read_object (opt->repo->objects, oid, &type, &size);if (!buf) { path_msg(opt, ERROR_OBJECT_READ_FAILED, 0, path, NULL, NULL, NULL,
@@ -4439,7 +4439,7 @@ 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) {
@@ -4619,10 +4619,10 @@ static int checkout(struct merge_options *opt, unpack_opts.verbose_update = (opt->verbosity >2 ); unpack_opts.fn = twoway_merge; unpack_opts.preserve_ignored =0 ;/* FIXME: !opts->overwrite_ignore */ -if (repo_parse_tree (the_repository, prev) <0 ) +if (repo_parse_tree (opt->repo, prev) <0 )return -1 ;init_tree_desc (&trees[0 ], &prev->object.oid, prev->buffer, prev->size); -if (repo_parse_tree (the_repository, next) <0 ) +if (repo_parse_tree (opt->repo, next) <0 )return -1 ;init_tree_desc (&trees[1 ], &next->object.oid, next->buffer, next->size);
@@ -5280,7 +5280,7 @@ redo:if (result->clean >=0 ) {if (!opt->mergeability_only) { - result->tree =repo_parse_tree_indirect (the_repository, + result->tree =repo_parse_tree_indirect (opt->repo, &working_tree_oid);if (!result->tree) die(_("unable to read tree (%s)"),
@@ -5309,7 +5309,7 @@ static void merge_ort_internal(struct merge_options *opt,struct strbuf merge_base_abbrev =STRBUF_INIT ;if (!merge_bases) { -if (repo_get_merge_bases (the_repository, h1, h2, +if (repo_get_merge_bases (opt->repo, h1, h2, &merge_bases) <0 ) { result->clean =-1 ; goto out;
@@ -5440,20 +5440,20 @@ static void merge_recursive_config(struct merge_options *opt, int ui) {char *value =NULL ;int renormalize =0 ; -repo_config_get_int (the_repository,"merge.verbosity" , &opt->verbosity); -repo_config_get_int (the_repository,"diff.renamelimit" , &opt->rename_limit); -repo_config_get_int (the_repository,"merge.renamelimit" , &opt->rename_limit); -repo_config_get_bool (the_repository,"merge.renormalize" , &renormalize); +repo_config_get_int (opt->repo,"merge.verbosity" , &opt->verbosity); +repo_config_get_int (opt->repo,"diff.renamelimit" , &opt->rename_limit); +repo_config_get_int (opt->repo,"merge.renamelimit" , &opt->rename_limit); +repo_config_get_bool (opt->repo,"merge.renormalize" , &renormalize); opt->renormalize = renormalize; -if (!repo_config_get_string (the_repository,"diff.renames" , &value)) { +if (!repo_config_get_string (opt->repo,"diff.renames" , &value)) { opt->detect_renames =git_config_rename ("diff.renames" , value);free (value); } -if (!repo_config_get_string (the_repository,"merge.renames" , &value)) { +if (!repo_config_get_string (opt->repo,"merge.renames" , &value)) { opt->detect_renames =git_config_rename ("merge.renames" , value);free (value); } -if (!repo_config_get_string (the_repository,"merge.directoryrenames" , &value)) { +if (!repo_config_get_string (opt->repo,"merge.directoryrenames" , &value)) {int boolval =git_parse_maybe_bool (value);if (0 <= boolval) { opt->detect_directory_renames = boolval ?
@@ -5466,7 +5466,7 @@ static void merge_recursive_config(struct merge_options *opt, int ui)free (value); }if (ui) { -if (!repo_config_get_string (the_repository,"diff.algorithm" , &value)) { +if (!repo_config_get_string (opt->repo,"diff.algorithm" , &value)) {long diff_algorithm =parse_algorithm_value (value);if (diff_algorithm <0 )die (_ ("unknown value for config '%s': %s" ),"diff.algorithm" , value);
@@ -5474,7 +5474,7 @@ static void merge_recursive_config(struct merge_options *opt, int ui)free (value); } } -repo_config (the_repository, git_xmerge_config,NULL ); +repo_config (opt->repo, git_xmerge_config,NULL ); } static void init_merge_options(struct merge_options *opt,
Signature · 1 line
gitgitgadget
PATCH v2 4/6 merge-ort: replace the_hash_algo with opt->repo->hash_algo
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
We have a perfectly valid repository available and do not need to use the_hash_algo (a shorthand for the_repository->hash_algo), so use the known repository instead.
--- merge-ort.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
merge-ort.c +13 -13
@@ -1857,7 +1857,7 @@ static int merge_submodule(struct merge_options *opt,BUG ("submodule deleted on one side; this should be handled outside of merge_submodule()" );if ((sub_not_initialized =repo_submodule_init (&subrepo, - opt->repo, path,null_oid (the_hash_algo)))) { + opt->repo, path,null_oid (opt->repo->hash_algo)))) { path_msg(opt, CONFLICT_SUBMODULE_NOT_INITIALIZED, 0, path, NULL, NULL, NULL, _("Failed to merge submodule %s (not checked out)"),
@@ -2240,7 +2240,7 @@ static int handle_content_merge(struct merge_options *opt, two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode)); merge_status =merge_3way (opt, path, - two_way ?null_oid (the_hash_algo) : &o->oid, + two_way ?null_oid (opt->repo->hash_algo) : &o->oid, &a->oid, &b->oid, pathnames, extra_marker_size, &result_buf);
@@ -2272,7 +2272,7 @@ static int handle_content_merge(struct merge_options *opt, }else if (S_ISGITLINK (a->mode)) {int two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode)); clean =merge_submodule (opt, pathnames[0 ], - two_way ?null_oid (the_hash_algo) : &o->oid, + two_way ?null_oid (opt->repo->hash_algo) : &o->oid, &a->oid, &b->oid, &result->oid);if (clean <0 )return -1 ;
@@ -2786,7 +2786,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,assert (!new_ci->match_mask); new_ci->dirmask =0 ; new_ci->stages[1 ].mode =0 ; -oidcpy (&new_ci->stages[1 ].oid,null_oid (the_hash_algo)); +oidcpy (&new_ci->stages[1 ].oid,null_oid (opt->repo->hash_algo));/* * Now that we have the file information in new_ci, make sure
@@ -2799,7 +2799,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,continue ;/* zero out any entries related to files */ ci->stages[i].mode =0 ; -oidcpy (&ci->stages[i].oid,null_oid (the_hash_algo)); +oidcpy (&ci->stages[i].oid,null_oid (opt->repo->hash_algo)); }/* Now we want to focus on new_ci, so reassign ci to it. */
@@ -3214,7 +3214,7 @@ static int process_renames(struct merge_options *opt,if (type_changed) {/* rename vs. typechange */ /* Mark the original as resolved by removal */ -memcpy (&oldinfo->stages[0 ].oid,null_oid (the_hash_algo), +memcpy (&oldinfo->stages[0 ].oid,null_oid (opt->repo->hash_algo),sizeof (oldinfo->stages[0 ].oid)); oldinfo->stages[0 ].mode =0 ; oldinfo->filemask &=0x06 ;
@@ -4102,7 +4102,7 @@ static int process_entry(struct merge_options *opt,if (ci->filemask & (1 << i))continue ; ci->stages[i].mode =0 ; -oidcpy (&ci->stages[i].oid,null_oid (the_hash_algo)); +oidcpy (&ci->stages[i].oid,null_oid (opt->repo->hash_algo)); } }else if (ci->df_conflict && ci->merged.result.mode !=0 ) {/*
@@ -4149,7 +4149,7 @@ static int process_entry(struct merge_options *opt,continue ;/* zero out any entries related to directories */ new_ci->stages[i].mode =0 ; -oidcpy (&new_ci->stages[i].oid,null_oid (the_hash_algo)); +oidcpy (&new_ci->stages[i].oid,null_oid (opt->repo->hash_algo)); }/*
@@ -4271,11 +4271,11 @@ static int process_entry(struct merge_options *opt, new_ci->merged.result.mode = ci->stages[2 ].mode;oidcpy (&new_ci->merged.result.oid, &ci->stages[2 ].oid); new_ci->stages[1 ].mode =0 ; -oidcpy (&new_ci->stages[1 ].oid,null_oid (the_hash_algo)); +oidcpy (&new_ci->stages[1 ].oid,null_oid (opt->repo->hash_algo)); new_ci->filemask =5 ;if ((S_IFMT & b_mode) != (S_IFMT & o_mode)) { new_ci->stages[0 ].mode =0 ; -oidcpy (&new_ci->stages[0 ].oid,null_oid (the_hash_algo)); +oidcpy (&new_ci->stages[0 ].oid,null_oid (opt->repo->hash_algo)); new_ci->filemask =4 ; }
@@ -4283,11 +4283,11 @@ static int process_entry(struct merge_options *opt, ci->merged.result.mode = ci->stages[1 ].mode;oidcpy (&ci->merged.result.oid, &ci->stages[1 ].oid); ci->stages[2 ].mode =0 ; -oidcpy (&ci->stages[2 ].oid,null_oid (the_hash_algo)); +oidcpy (&ci->stages[2 ].oid,null_oid (opt->repo->hash_algo)); ci->filemask =3 ;if ((S_IFMT & a_mode) != (S_IFMT & o_mode)) { ci->stages[0 ].mode =0 ; -oidcpy (&ci->stages[0 ].oid,null_oid (the_hash_algo)); +oidcpy (&ci->stages[0 ].oid,null_oid (opt->repo->hash_algo)); ci->filemask =2 ; }
@@ -4415,7 +4415,7 @@ static int process_entry(struct merge_options *opt,/* Deleted on both sides */ ci->merged.is_null =1 ; ci->merged.result.mode =0 ; -oidcpy (&ci->merged.result.oid,null_oid (the_hash_algo)); +oidcpy (&ci->merged.result.oid,null_oid (opt->repo->hash_algo));assert (!ci->df_conflict); ci->merged.clean = !ci->path_conflict; }
Signature · 1 line
gitgitgadget
PATCH v2 5/6 merge-ort: prevent the_repository from coming back
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
Due to the use of DEFAULT_ABBREV, we cannot get rid of our usage of USE_THE_REPOSITORY_VARIABLE. However, we have removed all other uses of the_repository in merge-ort a few times. But they keep coming back.
Define the_repository to make it a compilation error so that they don't come back any more.
--- merge-ort.c | 8 ++++++++ 1 file changed, 8 insertions(+)
merge-ort.c +8 -0
@@ -53,6 +53,14 @@#include "unpack-trees.h" #include "xdiff-interface.h" +/* +* We technically need USE_THE_REPOSITORY_VARIABLE above for DEFAULT_ABBREV, +* but do not want more uses of the_repository. Prevent them. +* +* opt->repo is available; use it instead. +*/ +#define the_repository DO_NOT_USE_THE_REPOSITORY +/* * We have many arrays of size 3. Whenever we have such an array, the * indices refer to one of the sides of the three-way merge. This is so
Signature · 1 line
gitgitgadget
PATCH v2 6/6 replay: prevent the_repository from coming back
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
Due to the use of DEFAULT_ABBREV, we cannot get rid of our usage of USE_THE_REPOSITORY_VARIABLE. We have removed all other uses of the_repository before, but without removing that definition, they keep coming back.
Define the_repository to make it a compilation error so that they don't come back any more.
--- replay.c | 6 ++++++ 1 file changed, 6 insertions(+)
replay.c +6 -0
@@ -11,6 +11,12 @@#include "strmap.h" #include "tree.h" +/* +* We technically need USE_THE_REPOSITORY_VARIABLE for DEFAULT_ABBREV, but +* do not want to use the_repository. +*/ +#define the_repository DO_NOT_USE_THE_REPOSITORY +static const char *short_commit_name (struct repository *repo,struct commit *commit) {
Signature · 1 line
gitgitgadget
v3 cover Avoid the_repository in merge-ort and replay
In reply to Elijah Newren via GitGitGadgetChanges 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
PATCH v3 1/6 merge,diff: remove the_repository check before prefetching blobs
From: Elijah Newren <[email protected]>
Prefetching of blobs from promisor remotes was added to diff in 7fbbcb21b162 (diff: batch fetching of missing blobs, 2019-04-05). In that commit,
https://lore.kernel.org/git/[email protected]/
was squashed into
without the extra explanation about the squashed changes being added to the commit message; in particular, this explanation from that first link is absent:
Quoted 3 lines
Also, prefetch only if the repository being diffed is
the_repository(because we do not support lazy fetching for any other repository
anyway).
Then, later, this checking was spread from diff.c to diffcore-rename.c and diffcore-break.c by 95acf11a3dc3 (diff: restrict when prefetching occurs, 2020-04-07) and then further split in d331dd3b0c82 (diffcore-rename: allow different missing_object_cb functions, 2021-06-22). I also copied the logic from prefetching blobs from diff.c to merge-ort.c in 2bff554b23e8 (merge-ort: add prefetching for content merges, 2021-06-22).
The reason for all these checks was noted above -- we only supported lazy fetching for the_repository. However, that changed with ef830cc43412 (promisor-remote: teach lazy-fetch in any repo, 2021-06-17), so these checks are now unnecessary. Remove them.
--- diff.c | 2 +- diffcore-break.c | 2 +- diffcore-rename.c | 4 ++-- merge-ort.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)
diff.c +1 -1
@@ -7176,7 +7176,7 @@ void diffcore_std(struct diff_options *options)* If no prefetching occurs, diffcore_rename() will prefetch if it * decides that it needs inexact rename detection. */ -if (options->repo == the_repository &&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 +1 -1
@@ -69,7 +69,7 @@ static int should_break(struct repository *r, oideq(&src->oid, &dst->oid))return 0 ;/* they are the same */ -if (r == the_repository &&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 +2 -2
@@ -987,7 +987,7 @@ static int find_basename_matches(struct diff_options *options,strintmap_set (&dests, base, i); } -if (options->repo == the_repository &&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; }
@@ -1574,7 +1574,7 @@ 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 (options->repo)) { dpf_options.missing_object_cb = inexact_prefetch; dpf_options.missing_object_data = &prefetch_options; }
merge-ort.c +1 -1
@@ -4438,7 +4438,7 @@ static void prefetch_for_content_merges(struct merge_options *opt,struct string_list_item *e;struct oid_array to_fetch =OID_ARRAY_INIT ; -if (opt->repo != the_repository || !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) {
Signature · 1 line
gitgitgadget
PATCH v3 2/6 merge-ort: pass repository to write_tree()
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
In order to get rid of a usage of the_repository, we need to know the value of opt->repo; pass it along to write_tree(). Once we have the repository, though, we no longer need to pass opt->repo->hash_algo->rawsz, we can have write_tree() look up that value itself.
--- merge-ort.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
merge-ort.c +8 -8
@@ -3822,15 +3822,16 @@ static int tree_entry_order(const void *a_, const void *b_) b->string, strlen(b->string), bmi->result.mode); } -static int write_tree (struct object_id *result_oid, +static int write_tree (struct repository *repo, +struct object_id *result_oid,struct string_list *versions, -unsigned int offset, -size_t hash_size) +unsigned int offset) {size_t maxlen =0 , extra;unsigned int nr;struct strbuf buf =STRBUF_INIT ;int i, ret =0 ; +size_t hash_size = repo->hash_algo->rawsz;assert (offset <= versions->nr); nr = versions->nr - offset;
@@ -3856,7 +3857,7 @@ static int write_tree(struct object_id *result_oid, }/* Write this object file out, and record in result_oid */ -if (odb_write_object (the_repository->objects, buf.buf, +if (odb_write_object (repo->objects, buf.buf, buf.len,OBJ_TREE , result_oid)) ret =-1 ;strbuf_release (&buf);
@@ -4026,8 +4027,8 @@ static int write_completed_directory(struct merge_options *opt, dir_info->is_null =0 ; dir_info->result.mode =S_IFDIR ;if (record_tree && -write_tree (&dir_info->result.oid, &info->versions, offset, - opt->repo->hash_algo->rawsz) <0 ) +write_tree (opt->repo, &dir_info->result.oid, &info->versions, + offset) <0 ) ret =-1 ; }
@@ -4573,8 +4574,7 @@ static int process_entries(struct merge_options *opt,BUG ("dir_metadata accounting completely off; shouldn't happen" ); }if (record_tree && -write_tree (result_oid, &dir_metadata.versions,0 , - opt->repo->hash_algo->rawsz) <0 ) +write_tree (opt->repo, result_oid, &dir_metadata.versions,0 ) <0 ) ret =-1 ; cleanup:string_list_clear (&plist,0 );
Signature · 1 line
gitgitgadget
PATCH v3 3/6 merge-ort: replace the_repository with opt->repo
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
We have a perfectly valid repository available and do not need to use the_repository.
--- merge-ort.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-)
merge-ort.c +21 -21
@@ -1732,9 +1732,9 @@ static int collect_merge_info(struct merge_options *opt, info.data = opt; info.show_all_errors =1 ; -if (repo_parse_tree (the_repository, merge_base) <0 || -repo_parse_tree (the_repository, side1) <0 || -repo_parse_tree (the_repository, side2) <0 ) +if (repo_parse_tree (opt->repo, merge_base) <0 || +repo_parse_tree (opt->repo, side1) <0 || +repo_parse_tree (opt->repo, side2) <0 )return -1 ;init_tree_desc (t +0 , &merge_base->object.oid, merge_base->buffer, merge_base->size);
@@ -2136,9 +2136,9 @@ static int merge_3way(struct merge_options *opt, name2 =mkpathdup ("%s:%s" , opt->branch2, pathnames[2 ]); } -read_mmblob (&orig, the_repository->objects, o); -read_mmblob (&src1, the_repository->objects, a); -read_mmblob (&src2, the_repository->objects, b); +read_mmblob (&orig, opt->repo->objects, o); +read_mmblob (&src1, opt->repo->objects, a); +read_mmblob (&src2, opt->repo->objects, b); merge_status = ll_merge(result_buf, path, &orig, base, &src1, name1, &src2, name2,
@@ -2254,7 +2254,7 @@ static int handle_content_merge(struct merge_options *opt, }if (!ret && record_object && -odb_write_object (the_repository->objects, result_buf.ptr, result_buf.size, +odb_write_object (opt->repo->objects, result_buf.ptr, result_buf.size,OBJ_BLOB , &result->oid)) { path_msg(opt, ERROR_OBJECT_WRITE_FAILED, 0, pathnames[0], pathnames[1], pathnames[2], NULL,
@@ -3713,7 +3713,7 @@ static int read_oid_strbuf(struct merge_options *opt,void *buf;enum object_type type;unsigned long size; - buf =odb_read_object (the_repository->objects, oid, &type, &size); + buf =odb_read_object (opt->repo->objects, oid, &type, &size);if (!buf) { path_msg(opt, ERROR_OBJECT_READ_FAILED, 0, path, NULL, NULL, NULL,
@@ -4619,10 +4619,10 @@ static int checkout(struct merge_options *opt, unpack_opts.verbose_update = (opt->verbosity >2 ); unpack_opts.fn = twoway_merge; unpack_opts.preserve_ignored =0 ;/* FIXME: !opts->overwrite_ignore */ -if (repo_parse_tree (the_repository, prev) <0 ) +if (repo_parse_tree (opt->repo, prev) <0 )return -1 ;init_tree_desc (&trees[0 ], &prev->object.oid, prev->buffer, prev->size); -if (repo_parse_tree (the_repository, next) <0 ) +if (repo_parse_tree (opt->repo, next) <0 )return -1 ;init_tree_desc (&trees[1 ], &next->object.oid, next->buffer, next->size);
@@ -5280,7 +5280,7 @@ redo:if (result->clean >=0 ) {if (!opt->mergeability_only) { - result->tree =repo_parse_tree_indirect (the_repository, + result->tree =repo_parse_tree_indirect (opt->repo, &working_tree_oid);if (!result->tree) die(_("unable to read tree (%s)"),
@@ -5309,7 +5309,7 @@ static void merge_ort_internal(struct merge_options *opt,struct strbuf merge_base_abbrev =STRBUF_INIT ;if (!merge_bases) { -if (repo_get_merge_bases (the_repository, h1, h2, +if (repo_get_merge_bases (opt->repo, h1, h2, &merge_bases) <0 ) { result->clean =-1 ; goto out;
@@ -5440,20 +5440,20 @@ static void merge_recursive_config(struct merge_options *opt, int ui) {char *value =NULL ;int renormalize =0 ; -repo_config_get_int (the_repository,"merge.verbosity" , &opt->verbosity); -repo_config_get_int (the_repository,"diff.renamelimit" , &opt->rename_limit); -repo_config_get_int (the_repository,"merge.renamelimit" , &opt->rename_limit); -repo_config_get_bool (the_repository,"merge.renormalize" , &renormalize); +repo_config_get_int (opt->repo,"merge.verbosity" , &opt->verbosity); +repo_config_get_int (opt->repo,"diff.renamelimit" , &opt->rename_limit); +repo_config_get_int (opt->repo,"merge.renamelimit" , &opt->rename_limit); +repo_config_get_bool (opt->repo,"merge.renormalize" , &renormalize); opt->renormalize = renormalize; -if (!repo_config_get_string (the_repository,"diff.renames" , &value)) { +if (!repo_config_get_string (opt->repo,"diff.renames" , &value)) { opt->detect_renames =git_config_rename ("diff.renames" , value);free (value); } -if (!repo_config_get_string (the_repository,"merge.renames" , &value)) { +if (!repo_config_get_string (opt->repo,"merge.renames" , &value)) { opt->detect_renames =git_config_rename ("merge.renames" , value);free (value); } -if (!repo_config_get_string (the_repository,"merge.directoryrenames" , &value)) { +if (!repo_config_get_string (opt->repo,"merge.directoryrenames" , &value)) {int boolval =git_parse_maybe_bool (value);if (0 <= boolval) { opt->detect_directory_renames = boolval ?
@@ -5466,7 +5466,7 @@ static void merge_recursive_config(struct merge_options *opt, int ui)free (value); }if (ui) { -if (!repo_config_get_string (the_repository,"diff.algorithm" , &value)) { +if (!repo_config_get_string (opt->repo,"diff.algorithm" , &value)) {long diff_algorithm =parse_algorithm_value (value);if (diff_algorithm <0 )die (_ ("unknown value for config '%s': %s" ),"diff.algorithm" , value);
@@ -5474,7 +5474,7 @@ static void merge_recursive_config(struct merge_options *opt, int ui)free (value); } } -repo_config (the_repository, git_xmerge_config,NULL ); +repo_config (opt->repo, git_xmerge_config,NULL ); } static void init_merge_options(struct merge_options *opt,
Signature · 1 line
gitgitgadget
PATCH v3 4/6 merge-ort: replace the_hash_algo with opt->repo->hash_algo
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
We have a perfectly valid repository available and do not need to use the_hash_algo (a shorthand for the_repository->hash_algo), so use the known repository instead.
--- merge-ort.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
merge-ort.c +13 -13
@@ -1857,7 +1857,7 @@ static int merge_submodule(struct merge_options *opt,BUG ("submodule deleted on one side; this should be handled outside of merge_submodule()" );if ((sub_not_initialized =repo_submodule_init (&subrepo, - opt->repo, path,null_oid (the_hash_algo)))) { + opt->repo, path,null_oid (opt->repo->hash_algo)))) { path_msg(opt, CONFLICT_SUBMODULE_NOT_INITIALIZED, 0, path, NULL, NULL, NULL, _("Failed to merge submodule %s (not checked out)"),
@@ -2240,7 +2240,7 @@ static int handle_content_merge(struct merge_options *opt, two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode)); merge_status =merge_3way (opt, path, - two_way ?null_oid (the_hash_algo) : &o->oid, + two_way ?null_oid (opt->repo->hash_algo) : &o->oid, &a->oid, &b->oid, pathnames, extra_marker_size, &result_buf);
@@ -2272,7 +2272,7 @@ static int handle_content_merge(struct merge_options *opt, }else if (S_ISGITLINK (a->mode)) {int two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode)); clean =merge_submodule (opt, pathnames[0 ], - two_way ?null_oid (the_hash_algo) : &o->oid, + two_way ?null_oid (opt->repo->hash_algo) : &o->oid, &a->oid, &b->oid, &result->oid);if (clean <0 )return -1 ;
@@ -2786,7 +2786,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,assert (!new_ci->match_mask); new_ci->dirmask =0 ; new_ci->stages[1 ].mode =0 ; -oidcpy (&new_ci->stages[1 ].oid,null_oid (the_hash_algo)); +oidcpy (&new_ci->stages[1 ].oid,null_oid (opt->repo->hash_algo));/* * Now that we have the file information in new_ci, make sure
@@ -2799,7 +2799,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,continue ;/* zero out any entries related to files */ ci->stages[i].mode =0 ; -oidcpy (&ci->stages[i].oid,null_oid (the_hash_algo)); +oidcpy (&ci->stages[i].oid,null_oid (opt->repo->hash_algo)); }/* Now we want to focus on new_ci, so reassign ci to it. */
@@ -3214,7 +3214,7 @@ static int process_renames(struct merge_options *opt,if (type_changed) {/* rename vs. typechange */ /* Mark the original as resolved by removal */ -memcpy (&oldinfo->stages[0 ].oid,null_oid (the_hash_algo), +memcpy (&oldinfo->stages[0 ].oid,null_oid (opt->repo->hash_algo),sizeof (oldinfo->stages[0 ].oid)); oldinfo->stages[0 ].mode =0 ; oldinfo->filemask &=0x06 ;
@@ -4102,7 +4102,7 @@ static int process_entry(struct merge_options *opt,if (ci->filemask & (1 << i))continue ; ci->stages[i].mode =0 ; -oidcpy (&ci->stages[i].oid,null_oid (the_hash_algo)); +oidcpy (&ci->stages[i].oid,null_oid (opt->repo->hash_algo)); } }else if (ci->df_conflict && ci->merged.result.mode !=0 ) {/*
@@ -4149,7 +4149,7 @@ static int process_entry(struct merge_options *opt,continue ;/* zero out any entries related to directories */ new_ci->stages[i].mode =0 ; -oidcpy (&new_ci->stages[i].oid,null_oid (the_hash_algo)); +oidcpy (&new_ci->stages[i].oid,null_oid (opt->repo->hash_algo)); }/*
@@ -4271,11 +4271,11 @@ static int process_entry(struct merge_options *opt, new_ci->merged.result.mode = ci->stages[2 ].mode;oidcpy (&new_ci->merged.result.oid, &ci->stages[2 ].oid); new_ci->stages[1 ].mode =0 ; -oidcpy (&new_ci->stages[1 ].oid,null_oid (the_hash_algo)); +oidcpy (&new_ci->stages[1 ].oid,null_oid (opt->repo->hash_algo)); new_ci->filemask =5 ;if ((S_IFMT & b_mode) != (S_IFMT & o_mode)) { new_ci->stages[0 ].mode =0 ; -oidcpy (&new_ci->stages[0 ].oid,null_oid (the_hash_algo)); +oidcpy (&new_ci->stages[0 ].oid,null_oid (opt->repo->hash_algo)); new_ci->filemask =4 ; }
@@ -4283,11 +4283,11 @@ static int process_entry(struct merge_options *opt, ci->merged.result.mode = ci->stages[1 ].mode;oidcpy (&ci->merged.result.oid, &ci->stages[1 ].oid); ci->stages[2 ].mode =0 ; -oidcpy (&ci->stages[2 ].oid,null_oid (the_hash_algo)); +oidcpy (&ci->stages[2 ].oid,null_oid (opt->repo->hash_algo)); ci->filemask =3 ;if ((S_IFMT & a_mode) != (S_IFMT & o_mode)) { ci->stages[0 ].mode =0 ; -oidcpy (&ci->stages[0 ].oid,null_oid (the_hash_algo)); +oidcpy (&ci->stages[0 ].oid,null_oid (opt->repo->hash_algo)); ci->filemask =2 ; }
@@ -4415,7 +4415,7 @@ static int process_entry(struct merge_options *opt,/* Deleted on both sides */ ci->merged.is_null =1 ; ci->merged.result.mode =0 ; -oidcpy (&ci->merged.result.oid,null_oid (the_hash_algo)); +oidcpy (&ci->merged.result.oid,null_oid (opt->repo->hash_algo));assert (!ci->df_conflict); ci->merged.clean = !ci->path_conflict; }
Signature · 1 line
gitgitgadget
PATCH v3 5/6 merge-ort: prevent the_repository from coming back
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
Due to the use of DEFAULT_ABBREV, we cannot get rid of our usage of USE_THE_REPOSITORY_VARIABLE. However, we have removed all other uses of the_repository in merge-ort a few times. But they keep coming back.
Define the_repository to make it a compilation error so that they don't come back any more.
--- merge-ort.c | 8 ++++++++ 1 file changed, 8 insertions(+)
merge-ort.c +8 -0
@@ -53,6 +53,14 @@#include "unpack-trees.h" #include "xdiff-interface.h" +/* +* We technically need USE_THE_REPOSITORY_VARIABLE above for DEFAULT_ABBREV, +* but do not want more uses of the_repository. Prevent them. +* +* opt->repo is available; use it instead. +*/ +#define the_repository DO_NOT_USE_THE_REPOSITORY +/* * We have many arrays of size 3. Whenever we have such an array, the * indices refer to one of the sides of the three-way merge. This is so
Signature · 1 line
gitgitgadget
Quoted 13 lines · Elijah Newren via GitGitGadget
Changes since v2:
* In first patch, actually avoid
the_repositorywhen attempting to remove check againstthe_repository
* Fix commit message of patch 3 due to the new patch 1.
* Slight tweak to commit message of patch 6.
...
As noted in the comments on v1, I actually do not know whyprefetch_for_content_merges()needs to usethe_repository. When I introduced it back in2bff554b23e8(merge-ort: add prefetching for content merges, 2021-06-22), I was just looking atdiffcore_std()and trying to mimic how it did the prefetch, and it has such a comparison. If anyone knows whydiffcore_std()needs to compare againstthe_repository, I'd love to hear...
Is this comment still current?
Quoted 7 lines
Elijah Newren (6):
merge,diff: removethe_repositorycheck before prefetching blobs
merge-ort: pass repository towrite_tree()
merge-ort: replacethe_repositorywithopt->repo
merge-ort: replacethe_hash_algowithopt->repo->hash_algo
merge-ort: preventthe_repositoryfrom coming back
replay: preventthe_repositoryfrom coming back
I do not seem to see the last step on the list archive.
https://lore.kernel.org/git/[email protected]/
I'll resurrect it using the previous one and ...
Quoted 10 lines
6:
67db46f34f! 6:0654d04584replay: preventthe_repositoryfrom coming back
@@ Commit message
coming back.Define
the_repositoryto 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.Signed-off-by Elijah Newren <[email protected]>
... this piece of information.
Quoted 17 lines · Junio C Hamano
Quoted 13 lines · Elijah Newren via GitGitGadget
Changes since v2:
* In first patch, actually avoid
the_repositorywhen attempting to remove check againstthe_repository
* Fix commit message of patch 3 due to the new patch 1.
* Slight tweak to commit message of patch 6.
...
As noted in the comments on v1, I actually do not know whyprefetch_for_content_merges()needs to usethe_repository. When I introduced it back in2bff554b23e8(merge-ort: add prefetching for content merges, 2021-06-22), I was just looking atdiffcore_std()and trying to mimic how it did the prefetch, and it has such a comparison. If anyone knows whydiffcore_std()needs to compare againstthe_repository, I'd love to hear...
Quoted 1 line
Is this comment still current?
No, I should have pulled it out of the cover letter since the commit message of patch #1 answers this; sorry for the oversight.
Quoted 9 lines
Quoted 7 lines
Elijah Newren (6):
merge,diff: removethe_repositorycheck before prefetching blobs
merge-ort: pass repository towrite_tree()
merge-ort: replacethe_repositorywithopt->repo
merge-ort: replacethe_hash_algowithopt->repo->hash_algo
merge-ort: preventthe_repositoryfrom coming back
replay: preventthe_repositoryfrom coming backI do not seem to see the last step on the list archive.
Weird.
Quoted 8 lines
https://lore.kernel.org/git/[email protected]/
I'll resurrect it using the previous one and ...
Quoted 4 lines
6:
67db46f34f! 6:0654d04584replay: preventthe_repositoryfrom coming back
@@ Commit message
coming back.
Quoted 8 lines
Quoted 6 lines
Define
the_repositoryto 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.Signed-off-by Elijah Newren <[email protected]>... this piece of information.
Thanks.
Quoted 15 lines · Elijah Newren via GitGitGadget
Changes since v2:
* In first patch, actually avoid
the_repositorywhen attempting to remove check againstthe_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_repositorycheck 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
Quoted 5 lines
Remove explicit uses of
the_repositoryandthe_hash_algofrom 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.
I reviewed this version (plus patch 6 from v2) and think it is good to go as-is. I went back to check the v1 feedback and thought it all valuable and resolved by this version.
Thanks,
-Stolee
Quoted 22 lines · Derrick Stolee
Quoted 15 lines · Elijah Newren via GitGitGadget
Changes since v2:
* In first patch, actually avoid
the_repositorywhen attempting to remove check againstthe_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_repositorycheck 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 itQuoted 5 lines
Remove explicit uses of
the_repositoryandthe_hash_algofrom 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.
Quoted 3 lines
I reviewed this version (plus patch 6 from v2) and think it is good to go as-is. I went back to check the v1 feedback and thought it all valuable and resolved by this version.
Likewise. I only had a single comment on v2 that was addressed now. Thanks!
Patrick