diff mbox series

[6/6] Use the right 'struct repository' instead of the_repository

Message ID 20190624095533.22162-7-pclouds@gmail.com (mailing list archive)
State New, archived
Headers show
Series Kill the_repository in tree-walk.c | expand

Commit Message

Duy Nguyen June 24, 2019, 9:55 a.m. UTC 
There are a couple of places where 'struct repository' is already passed
around, but the_repository is still used. Use the right repo.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 merge-recursive.c | 35 ++++++++++++++++++++---------------
 sequencer.c       |  4 ++--
 sha1-name.c       |  6 ++----
 shallow.c         |  3 ++-
 4 files changed, 26 insertions(+), 22 deletions(-)

Comments

Derrick Stolee June 24, 2019, 2:24 p.m. UTC | #1 
On 6/24/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote:
> There are a couple of places where 'struct repository' is already passed
> around, but the_repository is still used. Use the right repo.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

nit: the subject line doesn't use the standard "area: topic" format (including
the capitalization of the first word). Perhaps:

treewide: use the right 'struct repository' instead of the_repository

The changes here are straight-forward, but how do we check if we are done?

Thanks,
-Stolee
Duy Nguyen June 24, 2019, 2:45 p.m. UTC | #2 
On Mon, Jun 24, 2019 at 9:24 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 6/24/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote:
> > There are a couple of places where 'struct repository' is already passed
> > around, but the_repository is still used. Use the right repo.
> >
> > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> nit: the subject line doesn't use the standard "area: topic" format

because there's no specific area to this patch. I don't think sticking
to a fixed convention for the sake of it is really sensible.

> (including
> the capitalization of the first word). Perhaps:
>
> treewide: use the right 'struct repository' instead of the_repository
>
> The changes here are straight-forward, but how do we check if we are done?

At this point, you can't. At some point we should be able to
optionally disable the_repository, at least per file [2]. But even
then some function calls inside could still hide the_repository and
you would need something like [1] to reveal them.

The problem with [2] is it will cause a lot of problems when adding
new code until most of the code is converted. I will bring that up
when the number of the_repository (outside builtin/) goes down below
~50. With all my patches, I think we're at 300.

[1] https://gitlab.com/pclouds/git/commit/902a4dbdef6829ca06e12dbf74b0690456733351
[2] https://gitlab.com/pclouds/git/commit/f03f915294210baf038ee72d76ee998d9387028b
Johannes Schindelin June 27, 2019, 9:06 a.m. UTC | #3 
Hi Duy,

On Mon, 24 Jun 2019, Nguyễn Thái Ngọc Duy wrote:

> There are a couple of places where 'struct repository' is already passed
> around, but the_repository is still used. Use the right repo.

This patch series breaks t7814 on Linux, macOS and Windows, with GCC and
clang (read: _every_ single job in the CI except for documentation and
coccinelle).

For details, see
https://dev.azure.com/gitgitgadget/git/_build/results?buildId=11536&view=ms.vss-test-web.build-test-results-tab

The first test case to break is t7814.9 basic grep tree:

-- snipsnap --
expecting success:
	cat >expect <<-\EOF &&
	HEAD:a:(1|2)d(3|4)
	HEAD:b/b:(3|4)
	HEAD:submodule/a:(1|2)d(3|4)
	HEAD:submodule/sub/a:(1|2)d(3|4)
	EOF

	git grep -e "(3|4)" --recurse-submodules HEAD >actual &&
	test_cmp expect actual

++ cat
++ git grep -e '(3|4)' --recurse-submodules HEAD
fatal: unable to read tree (e6d32f554b2f8e48c3b8feece1653e933facb34a)
error: last command exited with $?=128
diff mbox series

Patch

diff --git a/merge-recursive.c b/merge-recursive.c
index 6d772eb0eb..12300131fc 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -465,17 +465,18 @@  static void get_files_dirs(struct merge_options *opt, struct tree *tree)
 {
 	struct pathspec match_all;
 	memset(&match_all, 0, sizeof(match_all));
-	read_tree_recursive(the_repository, tree, "", 0, 0,
+	read_tree_recursive(opt->repo, tree, "", 0, 0,
 			    &match_all, save_files_dirs, opt);
 }
 
-static int get_tree_entry_if_blob(const struct object_id *tree,
+static int get_tree_entry_if_blob(struct repository *r,
+				  const struct object_id *tree,
 				  const char *path,
 				  struct diff_filespec *dfs)
 {
 	int ret;
 
-	ret = get_tree_entry(the_repository, tree, path, &dfs->oid, &dfs->mode);
+	ret = get_tree_entry(r, tree, path, &dfs->oid, &dfs->mode);
 	if (S_ISDIR(dfs->mode)) {
 		oidcpy(&dfs->oid, &null_oid);
 		dfs->mode = 0;
@@ -487,15 +488,16 @@  static int get_tree_entry_if_blob(const struct object_id *tree,
  * Returns an index_entry instance which doesn't have to correspond to
  * a real cache entry in Git's index.
  */
-static struct stage_data *insert_stage_data(const char *path,
+static struct stage_data *insert_stage_data(struct repository *r,
+		const char *path,
 		struct tree *o, struct tree *a, struct tree *b,
 		struct string_list *entries)
 {
 	struct string_list_item *item;
 	struct stage_data *e = xcalloc(1, sizeof(struct stage_data));
-	get_tree_entry_if_blob(&o->object.oid, path, &e->stages[1]);
-	get_tree_entry_if_blob(&a->object.oid, path, &e->stages[2]);
-	get_tree_entry_if_blob(&b->object.oid, path, &e->stages[3]);
+	get_tree_entry_if_blob(r, &o->object.oid, path, &e->stages[1]);
+	get_tree_entry_if_blob(r, &a->object.oid, path, &e->stages[2]);
+	get_tree_entry_if_blob(r, &b->object.oid, path, &e->stages[3]);
 	item = string_list_insert(entries, path);
 	item->util = e;
 	return e;
@@ -1900,12 +1902,13 @@  static struct diff_queue_struct *get_diffpairs(struct merge_options *opt,
 	return ret;
 }
 
-static int tree_has_path(struct tree *tree, const char *path)
+static int tree_has_path(struct repository *r, struct tree *tree,
+			 const char *path)
 {
 	struct object_id hashy;
 	unsigned short mode_o;
 
-	return !get_tree_entry(the_repository,
+	return !get_tree_entry(r,
 			       &tree->object.oid, path,
 			       &hashy, &mode_o);
 }
@@ -2057,7 +2060,7 @@  static char *handle_path_level_conflicts(struct merge_options *opt,
 	 */
 	if (collision_ent->reported_already) {
 		clean = 0;
-	} else if (tree_has_path(tree, new_path)) {
+	} else if (tree_has_path(opt->repo, tree, new_path)) {
 		collision_ent->reported_already = 1;
 		strbuf_add_separated_string_list(&collision_paths, ", ",
 						 &collision_ent->source_files);
@@ -2135,7 +2138,7 @@  static void handle_directory_level_conflicts(struct merge_options *opt,
 			string_list_append(&remove_from_merge,
 					   merge_ent->dir)->util = merge_ent;
 			strbuf_release(&merge_ent->new_dir);
-		} else if (tree_has_path(head, head_ent->dir)) {
+		} else if (tree_has_path(opt->repo, head, head_ent->dir)) {
 			/* 2. This wasn't a directory rename after all */
 			string_list_append(&remove_from_head,
 					   head_ent->dir)->util = head_ent;
@@ -2149,7 +2152,7 @@  static void handle_directory_level_conflicts(struct merge_options *opt,
 	hashmap_iter_init(dir_re_merge, &iter);
 	while ((merge_ent = hashmap_iter_next(&iter))) {
 		head_ent = dir_rename_find_entry(dir_re_head, merge_ent->dir);
-		if (tree_has_path(merge, merge_ent->dir)) {
+		if (tree_has_path(opt->repo, merge, merge_ent->dir)) {
 			/* 2. This wasn't a directory rename after all */
 			string_list_append(&remove_from_merge,
 					   merge_ent->dir)->util = merge_ent;
@@ -2478,7 +2481,7 @@  static void apply_directory_rename_modifications(struct merge_options *opt,
 		if (pair->status == 'R')
 			re->dst_entry->processed = 1;
 
-		re->dst_entry = insert_stage_data(new_path,
+		re->dst_entry = insert_stage_data(opt->repo, new_path,
 						  o_tree, a_tree, b_tree,
 						  entries);
 		item = string_list_insert(entries, new_path);
@@ -2587,14 +2590,16 @@  static struct string_list *get_renames(struct merge_options *opt,
 		re->dir_rename_original_dest = NULL;
 		item = string_list_lookup(entries, re->pair->one->path);
 		if (!item)
-			re->src_entry = insert_stage_data(re->pair->one->path,
+			re->src_entry = insert_stage_data(opt->repo,
+					re->pair->one->path,
 					o_tree, a_tree, b_tree, entries);
 		else
 			re->src_entry = item->util;
 
 		item = string_list_lookup(entries, re->pair->two->path);
 		if (!item)
-			re->dst_entry = insert_stage_data(re->pair->two->path,
+			re->dst_entry = insert_stage_data(opt->repo,
+					re->pair->two->path,
 					o_tree, a_tree, b_tree, entries);
 		else
 			re->dst_entry = item->util;
diff --git a/sequencer.c b/sequencer.c
index d565fcf2b1..64428ac28f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3733,7 +3733,7 @@  static int pick_commits(struct repository *r,
 			unlink(rebase_path_author_script());
 			unlink(rebase_path_stopped_sha());
 			unlink(rebase_path_amend());
-			unlink(git_path_merge_head(the_repository));
+			unlink(git_path_merge_head(r));
 			delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
 
 			if (item->command == TODO_BREAK)
@@ -4107,7 +4107,7 @@  static int commit_staged_changes(struct repository *r,
 			   opts, flags))
 		return error(_("could not commit staged changes."));
 	unlink(rebase_path_amend());
-	unlink(git_path_merge_head(the_repository));
+	unlink(git_path_merge_head(r));
 	if (final_fixup) {
 		unlink(rebase_path_fixup_msg());
 		unlink(rebase_path_squash_msg());
diff --git a/sha1-name.c b/sha1-name.c
index 3c9fa10af8..6069fe006b 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -478,7 +478,7 @@  static enum get_oid_result get_short_oid(struct repository *r,
 	 * or migrated from loose to packed.
 	 */
 	if (status == MISSING_OBJECT) {
-		reprepare_packed_git(the_repository);
+		reprepare_packed_git(r);
 		find_short_object_filename(&ds);
 		find_short_packed_object(&ds);
 		status = finish_object_disambiguation(&ds, oid);
@@ -1389,9 +1389,7 @@  int repo_get_oid_mb(struct repository *r,
 	two = lookup_commit_reference_gently(r, &oid_tmp, 0);
 	if (!two)
 		return -1;
-	if (r != the_repository)
-		BUG("sorry get_merge_bases() can't take struct repository yet");
-	mbs = get_merge_bases(one, two);
+	mbs = repo_get_merge_bases(r, one, two);
 	if (!mbs || mbs->next)
 		st = -1;
 	else {
diff --git a/shallow.c b/shallow.c
index ce45297940..5fa2b15d37 100644
--- a/shallow.c
+++ b/shallow.c
@@ -248,7 +248,8 @@  static void check_shallow_file_for_update(struct repository *r)
 	if (r->parsed_objects->is_shallow == -1)
 		BUG("shallow must be initialized by now");
 
-	if (!stat_validity_check(r->parsed_objects->shallow_stat, git_path_shallow(the_repository)))
+	if (!stat_validity_check(r->parsed_objects->shallow_stat,
+				 git_path_shallow(r)))
 		die("shallow file has changed since we read it");
 }