[RFC PATCH 0/3] pretty, builtin/mailinfo: don't rely on the_repository

[RFC PATCH 0/3] pretty, builtin/mailinfo: don't rely on the_repository

[Kousik Sanagavarapu]

Hi,
Just a brief summary -

1/3 - the main changes are in environment.[ch] and repository.[ch], all
      the others are just changes due to this change.

2/3 - the main changes are in pretty.[ch], all the other changes are due
      to this change.

3/3 - This is pretty straight-forward.

One may notice that there are more "the_repository" occurences now than
before this change - which is good since it means that we have now made
the respective dependencies explicit (these were previously implicit).

The change in 1/3 is marked RFC since I was kind of skeptical about the
"repo" check in the repo_*() functions being done at _that_ level.
Since every other change in this series depends on this, I've marked all
the other RFC as well.

Thanks

Kousik Sanagavarapu (3):
  repository: move git_*_encoding configs to repo scope
  pretty: don't rely on "the_repository"
  builtin/mailinfo: don't rely on "the_repository"

 builtin/am.c          |  6 +++--
 builtin/blame.c       |  2 +-
 builtin/checkout.c    |  6 +++--
 builtin/commit.c      |  8 +++---
 builtin/log.c         |  7 ++---
 builtin/mailinfo.c    |  5 ++--
 builtin/merge.c       |  2 +-
 builtin/replay.c      |  3 ++-
 builtin/reset.c       |  2 +-
 builtin/rev-list.c    |  4 +--
 builtin/shortlog.c    |  5 ++--
 builtin/show-branch.c |  3 ++-
 builtin/stash.c       |  2 +-
 bundle.c              |  4 +--
 commit.c              |  9 ++++---
 config.c              | 10 ++++---
 environment.c         | 13 ---------
 environment.h         |  6 -----
 log-tree.c            |  6 ++---
 pretty.c              | 63 ++++++++++++++++++++++++-------------------
 pretty.h              | 14 +++++-----
 range-diff.c          |  2 +-
 remote-curl.c         |  4 ++-
 repository.c          | 13 +++++++++
 repository.h          |  6 +++++
 revision.c            | 16 +++++------
 sequencer.c           | 28 ++++++++++---------
 submodule.c           |  2 +-
 28 files changed, 139 insertions(+), 112 deletions(-)

-- 
2.47.0.73.g7a80afd5fd.dirty

[PATCH 1/3] repository: move git_*_encoding configs to repo scope

[Kousik Sanagavarapu]

Move "git_commit_encoding" and "git_log_output_encoding" to "struct
repository" and amend the functions associated with peeking at these
values so that now they take a "struct repository *" argument
accordingly.  While at it, rename the functions to repo_*() following
our usual convention.

Doing so removes the implicit dependency of these variables on
"the_repository", which is better because we now populate these
variables per repository.

Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
 builtin/am.c       |  6 ++++--
 builtin/blame.c    |  2 +-
 builtin/commit.c   |  8 +++++---
 builtin/log.c      |  3 ++-
 builtin/mailinfo.c |  3 ++-
 builtin/replay.c   |  3 ++-
 builtin/rev-list.c |  2 +-
 builtin/shortlog.c |  2 +-
 bundle.c           |  2 +-
 commit.c           |  9 ++++++---
 config.c           | 10 ++++++----
 environment.c      | 13 -------------
 environment.h      |  6 ------
 log-tree.c         |  4 ++--
 pretty.c           |  2 +-
 remote-curl.c      |  4 +++-
 repository.c       | 13 +++++++++++++
 repository.h       |  6 ++++++
 revision.c         | 10 +++++-----
 sequencer.c        | 17 +++++++++--------
 submodule.c        |  2 +-
 21 files changed, 71 insertions(+), 56 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index bfa95147cf..1b6c9e3889 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -11,6 +11,7 @@
 #include "config.h"
 #include "editor.h"
 #include "environment.h"
+#include "repository.h"
 #include "gettext.h"
 #include "hex.h"
 #include "parse-options.h"
@@ -1213,7 +1214,8 @@ static int parse_mail(struct am_state *state, const char *mail)
 	setup_mailinfo(&mi);
 
 	if (state->utf8)
-		mi.metainfo_charset = get_commit_output_encoding();
+		mi.metainfo_charset =
+			repo_get_commit_output_encoding(the_repository);
 	else
 		mi.metainfo_charset = NULL;
 
@@ -1352,7 +1354,7 @@ static void get_commit_info(struct am_state *state, struct commit *commit)
 	struct ident_split id;
 
 	buffer = repo_logmsg_reencode(the_repository, commit, NULL,
-				      get_commit_output_encoding());
+				      repo_get_commit_output_encoding(the_repository));
 
 	ident_line = find_commit_header(buffer, "author", &ident_len);
 	if (!ident_line)
diff --git a/builtin/blame.c b/builtin/blame.c
index e407a22da3..ffbcedb2bf 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -203,7 +203,7 @@ static void get_commit_info(struct commit *commit,
 	const char *subject, *encoding;
 	const char *message;
 
-	encoding = get_log_output_encoding();
+	encoding = repo_get_log_output_encoding(the_repository);
 	message = repo_logmsg_reencode(the_repository, commit, NULL, encoding);
 	get_ac_line(message, "\nauthor ",
 		    &ret->author, &ret->author_mail,
diff --git a/builtin/commit.c b/builtin/commit.c
index 8db4e9df0c..4eac20ce86 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -763,7 +763,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			c = lookup_commit_reference_by_name(squash_message);
 			if (!c)
 				die(_("could not lookup commit '%s'"), squash_message);
-			ctx.output_encoding = get_commit_output_encoding();
+			ctx.output_encoding =
+				repo_get_commit_output_encoding(the_repository);
 			repo_format_commit_message(the_repository, c,
 						   "squash! %s\n\n", &sb,
 						   &ctx);
@@ -798,7 +799,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		commit = lookup_commit_reference_by_name(fixup_commit);
 		if (!commit)
 			die(_("could not lookup commit '%s'"), fixup_commit);
-		ctx.output_encoding = get_commit_output_encoding();
+		ctx.output_encoding =
+			repo_get_commit_output_encoding(the_repository);
 		fmt = xstrfmt("%s! %%s\n\n", fixup_prefix);
 		repo_format_commit_message(the_repository, commit, fmt, &sb,
 					   &ctx);
@@ -1202,7 +1204,7 @@ static const char *read_commit_message(const char *name)
 	commit = lookup_commit_reference_by_name(name);
 	if (!commit)
 		die(_("could not lookup commit '%s'"), name);
-	out_enc = get_commit_output_encoding();
+	out_enc = repo_get_commit_output_encoding(the_repository);
 	return repo_logmsg_reencode(the_repository, commit, NULL, out_enc);
 }
 
diff --git a/builtin/log.c b/builtin/log.c
index 368f6580a6..c28064c6f3 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -668,7 +668,8 @@ static void show_tagger(const char *buf, struct rev_info *rev)
 
 	pp.fmt = rev->commit_format;
 	pp.date_mode = rev->date_mode;
-	pp_user_info(&pp, "Tagger", &out, buf, get_log_output_encoding());
+	pp_user_info(&pp, "Tagger", &out, buf,
+		     repo_get_log_output_encoding(the_repository));
 	fprintf(rev->diffopt.file, "%s", out.buf);
 	strbuf_release(&out);
 }
diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index e17dec27b1..828b2b5845 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -93,7 +93,8 @@ int cmd_mailinfo(int argc,
 
 	switch (meta_charset.policy) {
 	case CHARSET_DEFAULT:
-		mi.metainfo_charset = get_commit_output_encoding();
+		mi.metainfo_charset =
+			repo_get_commit_output_encoding(the_repository);
 		break;
 	case CHARSET_NO_REENCODE:
 		mi.metainfo_charset = NULL;
diff --git a/builtin/replay.c b/builtin/replay.c
index 2d12a4e403..c23972f044 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -59,7 +59,8 @@ static struct commit *create_commit(struct tree *tree,
 	char *sign_commit = NULL; /* FIXME: cli users might want to sign again */
 	struct commit_extra_header *extra = NULL;
 	struct strbuf msg = STRBUF_INIT;
-	const char *out_enc = get_commit_output_encoding();
+	const char *out_enc =
+		repo_get_commit_output_encoding(the_repository);
 	const char *message = repo_logmsg_reencode(the_repository, based_on,
 						   NULL, out_enc);
 	const char *orig_message = NULL;
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index f62bcbf2b1..e8427a7c0c 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -218,7 +218,7 @@ static void show_commit(struct commit *commit, void *data)
 		ctx.date_mode = revs->date_mode;
 		ctx.date_mode_explicit = revs->date_mode_explicit;
 		ctx.fmt = revs->commit_format;
-		ctx.output_encoding = get_log_output_encoding();
+		ctx.output_encoding = repo_get_log_output_encoding(the_repository);
 		ctx.color = revs->diffopt.use_color;
 		ctx.rev = revs;
 		pretty_print_commit(&ctx, commit, &buf);
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 3ed5c46078..81b8baf44a 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -246,7 +246,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
 	ctx.fmt = CMIT_FMT_USERFORMAT;
 	ctx.abbrev = log->abbrev;
 	ctx.date_mode = log->date_mode;
-	ctx.output_encoding = get_log_output_encoding();
+	ctx.output_encoding = repo_get_log_output_encoding(the_repository);
 
 	if (!log->summary) {
 		if (log->user_format)
diff --git a/bundle.c b/bundle.c
index 4773b51eb1..aad00836b9 100644
--- a/bundle.c
+++ b/bundle.c
@@ -483,7 +483,7 @@ static void write_bundle_prerequisites(struct commit *commit, void *data)
 	write_or_die(bpi->fd, buf.buf, buf.len);
 
 	ctx.fmt = CMIT_FMT_ONELINE;
-	ctx.output_encoding = get_log_output_encoding();
+	ctx.output_encoding = repo_get_log_output_encoding(the_repository);
 	strbuf_reset(&buf);
 	pretty_print_commit(&ctx, commit, &buf);
 	strbuf_trim(&buf);
diff --git a/commit.c b/commit.c
index cc03a93036..a3a0f458ae 100644
--- a/commit.c
+++ b/commit.c
@@ -1655,7 +1655,8 @@ static void write_commit_tree(struct strbuf *buffer, const char *msg, size_t msg
 	size_t i;
 
 	/* Not having i18n.commitencoding is the same as having utf-8 */
-	encoding_is_utf8 = is_encoding_utf8(git_commit_encoding);
+	encoding_is_utf8 =
+		is_encoding_utf8(the_repository->git_commit_encoding);
 
 	strbuf_grow(buffer, 8192); /* should avoid reallocs for the headers */
 	strbuf_addf(buffer, "tree %s\n", oid_to_hex(tree));
@@ -1676,7 +1677,8 @@ static void write_commit_tree(struct strbuf *buffer, const char *msg, size_t msg
 		committer = git_committer_info(IDENT_STRICT);
 	strbuf_addf(buffer, "committer %s\n", committer);
 	if (!encoding_is_utf8)
-		strbuf_addf(buffer, "encoding %s\n", git_commit_encoding);
+		strbuf_addf(buffer, "encoding %s\n",
+			    the_repository->git_commit_encoding);
 
 	while (extra) {
 		add_extra_header(buffer, extra);
@@ -1705,7 +1707,8 @@ int commit_tree_extended(const char *msg, size_t msg_len,
 	size_t i, nparents;
 
 	/* Not having i18n.commitencoding is the same as having utf-8 */
-	encoding_is_utf8 = is_encoding_utf8(git_commit_encoding);
+	encoding_is_utf8 =
+		is_encoding_utf8(the_repository->git_commit_encoding);
 
 	assert_oid_type(tree, OBJ_TREE);
 
diff --git a/config.c b/config.c
index a11bb85da3..656748692d 100644
--- a/config.c
+++ b/config.c
@@ -1690,13 +1690,15 @@ static int git_default_sparse_config(const char *var, const char *value)
 static int git_default_i18n_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "i18n.commitencoding")) {
-		FREE_AND_NULL(git_commit_encoding);
-		return git_config_string(&git_commit_encoding, var, value);
+		FREE_AND_NULL(the_repository->git_commit_encoding);
+		return git_config_string(&the_repository->git_commit_encoding,
+					 var, value);
 	}
 
 	if (!strcmp(var, "i18n.logoutputencoding")) {
-		FREE_AND_NULL(git_log_output_encoding);
-		return git_config_string(&git_log_output_encoding, var, value);
+		FREE_AND_NULL(the_repository->git_log_output_encoding);
+		return git_config_string(&the_repository->git_log_output_encoding,
+					 var, value);
 	}
 
 	/* Add other config variables here and to Documentation/config.txt. */
diff --git a/environment.c b/environment.c
index a2ce998081..288fccb9af 100644
--- a/environment.c
+++ b/environment.c
@@ -37,8 +37,6 @@ int assume_unchanged;
 int is_bare_repository_cfg = -1; /* unspecified */
 int warn_on_object_refname_ambiguity = 1;
 int repository_format_precious_objects;
-char *git_commit_encoding;
-char *git_log_output_encoding;
 char *apply_default_whitespace;
 char *apply_default_ignorewhitespace;
 char *git_attributes_file;
@@ -199,17 +197,6 @@ const char *strip_namespace(const char *namespaced_ref)
 	return NULL;
 }
 
-const char *get_log_output_encoding(void)
-{
-	return git_log_output_encoding ? git_log_output_encoding
-		: get_commit_output_encoding();
-}
-
-const char *get_commit_output_encoding(void)
-{
-	return git_commit_encoding ? git_commit_encoding : "UTF-8";
-}
-
 static int the_shared_repository = PERM_UMASK;
 static int need_shared_repository_from_config = 1;
 
diff --git a/environment.h b/environment.h
index 923e12661e..5e380901cf 100644
--- a/environment.h
+++ b/environment.h
@@ -207,12 +207,6 @@ extern int grafts_keep_true_parents;
 
 extern int repository_format_precious_objects;
 
-const char *get_log_output_encoding(void);
-const char *get_commit_output_encoding(void);
-
-extern char *git_commit_encoding;
-extern char *git_log_output_encoding;
-
 extern char *editor_program;
 extern char *askpass_program;
 extern char *excludes_file;
diff --git a/log-tree.c b/log-tree.c
index ba5632805e..6bbddcf85a 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -832,7 +832,7 @@ void show_log(struct rev_info *opt)
 
 		raw = (opt->commit_format == CMIT_FMT_USERFORMAT);
 		format_display_notes(&commit->object.oid, &notebuf,
-				     get_log_output_encoding(), raw);
+				     repo_get_log_output_encoding(the_repository), raw);
 		ctx.notes_message = strbuf_detach(&notebuf, NULL);
 	}
 
@@ -852,7 +852,7 @@ void show_log(struct rev_info *opt)
 	ctx.mailmap = opt->mailmap;
 	ctx.color = opt->diffopt.use_color;
 	ctx.expand_tabs_in_log = opt->expand_tabs_in_log;
-	ctx.output_encoding = get_log_output_encoding();
+	ctx.output_encoding = repo_get_log_output_encoding(the_repository);
 	ctx.rev = opt;
 	if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
 		ctx.from_ident = &opt->from_ident;
diff --git a/pretty.c b/pretty.c
index 6403e26890..e7b069a48c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -2296,7 +2296,7 @@ void pretty_print_commit(struct pretty_print_context *pp,
 		return;
 	}
 
-	encoding = get_log_output_encoding();
+	encoding = repo_get_log_output_encoding(the_repository);
 	msg = reencoded = repo_logmsg_reencode(the_repository, commit, NULL,
 					       encoding);
 
diff --git a/remote-curl.c b/remote-curl.c
index 9a71e04301..47c31e7def 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -4,6 +4,7 @@
 #include "git-curl-compat.h"
 #include "config.h"
 #include "environment.h"
+#include "repository.h"
 #include "gettext.h"
 #include "hex.h"
 #include "remote.h"
@@ -378,7 +379,8 @@ static int show_http_message(struct strbuf *type, struct strbuf *charset,
 	if (strcmp(type->buf, "text/plain"))
 		return -1;
 	if (charset->len)
-		strbuf_reencode(msg, charset->buf, get_log_output_encoding());
+		strbuf_reencode(msg, charset->buf,
+				repo_get_log_output_encoding(the_repository));
 
 	strbuf_trim(msg);
 	if (!msg->len)
diff --git a/repository.c b/repository.c
index f988b8ae68..2b999c4bdc 100644
--- a/repository.c
+++ b/repository.c
@@ -131,6 +131,19 @@ const char *repo_get_work_tree(struct repository *repo)
 	return repo->worktree;
 }
 
+const char *repo_get_log_output_encoding(struct repository *repo)
+{
+	return (repo && repo->git_log_output_encoding) ?
+	       repo->git_log_output_encoding :
+	       repo_get_commit_output_encoding(repo);
+}
+
+const char *repo_get_commit_output_encoding(struct repository *repo)
+{
+	return (repo && repo->git_commit_encoding) ?
+	       repo->git_commit_encoding : "UTF-8";
+}
+
 static void repo_set_commondir(struct repository *repo,
 			       const char *commondir)
 {
diff --git a/repository.h b/repository.h
index 24a66a496a..0b71b55853 100644
--- a/repository.h
+++ b/repository.h
@@ -150,6 +150,8 @@ struct repository {
 
 	/* Configurations */
 	int repository_format_worktree_config;
+	char *git_commit_encoding;
+	char *git_log_output_encoding;
 
 	/* Indicate if a repository has a different 'commondir' from 'gitdir' */
 	unsigned different_commondir:1;
@@ -179,6 +181,10 @@ struct set_gitdir_args {
 	int disable_ref_updates;
 };
 
+/* Get configurations */
+const char *repo_get_log_output_encoding(struct repository *repo);
+const char *repo_get_commit_output_encoding(struct repository *repo);
+
 void repo_set_gitdir(struct repository *repo, const char *root,
 		     const struct set_gitdir_args *extra_args);
 void repo_set_worktree(struct repository *repo, const char *path);
diff --git a/revision.c b/revision.c
index f5f5b84f2b..c3a54577b0 100644
--- a/revision.c
+++ b/revision.c
@@ -2688,11 +2688,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--invert-grep")) {
 		revs->grep_filter.no_body_match = 1;
 	} else if ((argcount = parse_long_opt("encoding", argv, &optarg))) {
-		free(git_log_output_encoding);
+		free(the_repository->git_log_output_encoding);
 		if (strcmp(optarg, "none"))
-			git_log_output_encoding = xstrdup(optarg);
+			the_repository->git_log_output_encoding = xstrdup(optarg);
 		else
-			git_log_output_encoding = xstrdup("");
+			the_repository->git_log_output_encoding = xstrdup("");
 		return argcount;
 	} else if (!strcmp(arg, "--reverse")) {
 		revs->reverse ^= 1;
@@ -3142,7 +3142,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 
 	diff_setup_done(&revs->diffopt);
 
-	if (!is_encoding_utf8(get_log_output_encoding()))
+	if (!is_encoding_utf8(repo_get_log_output_encoding(the_repository)))
 		revs->grep_filter.ignore_locale = 1;
 	compile_grep_patterns(&revs->grep_filter);
 
@@ -4041,7 +4041,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
 	 * so we will not end up with a buffer that has two different encodings
 	 * in it.
 	 */
-	encoding = get_log_output_encoding();
+	encoding = repo_get_log_output_encoding(the_repository);
 	message = repo_logmsg_reencode(the_repository, commit, NULL, encoding);
 
 	/* Copy the commit to temporary if we are using "fake" headers */
diff --git a/sequencer.c b/sequencer.c
index 8d01cd50ac..25783d3b8b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -490,7 +490,7 @@ static int get_message(struct commit *commit, struct commit_message *out)
 	int subject_len;
 
 	out->message = repo_logmsg_reencode(the_repository, commit, NULL,
-					    get_commit_output_encoding());
+			repo_get_commit_output_encoding(the_repository));
 	abbrev = short_commit_name(the_repository, commit);
 
 	subject_len = find_commit_subject(out->message, &subject);
@@ -1561,7 +1561,7 @@ static int try_to_commit(struct repository *r,
 
 	if (flags & AMEND_MSG) {
 		const char *exclude_gpgsig[] = { "gpgsig", "gpgsig-sha256", NULL };
-		const char *out_enc = get_commit_output_encoding();
+		const char *out_enc = repo_get_commit_output_encoding(r);
 		const char *message = repo_logmsg_reencode(r, current_head,
 							   NULL, out_enc);
 
@@ -2073,7 +2073,7 @@ static int update_squash_messages(struct repository *r,
 	struct strbuf buf = STRBUF_INIT;
 	int res = 0;
 	const char *message, *body;
-	const char *encoding = get_commit_output_encoding();
+	const char *encoding = repo_get_commit_output_encoding(r);
 
 	if (ctx->current_fixup_count > 0) {
 		struct strbuf header = STRBUF_INIT;
@@ -3311,7 +3311,7 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
 	if (prepare_revs(opts))
 		return -1;
 
-	encoding = get_log_output_encoding();
+	encoding = repo_get_log_output_encoding(the_repository);
 
 	while ((commit = get_revision(opts->revs))) {
 		struct todo_item *item = append_new_todo(todo_list);
@@ -3697,7 +3697,7 @@ static int make_patch(struct repository *r,
 	}
 
 	if (!file_exists(rebase_path_message())) {
-		const char *encoding = get_commit_output_encoding();
+		const char *encoding = repo_get_commit_output_encoding(r);
 		const char *commit_buffer = repo_logmsg_reencode(r,
 								 commit, NULL,
 								 encoding);
@@ -4186,7 +4186,7 @@ static int do_merge(struct repository *r,
 	}
 
 	if (commit) {
-		const char *encoding = get_commit_output_encoding();
+		const char *encoding = repo_get_commit_output_encoding(r);
 		const char *message = repo_logmsg_reencode(r, commit, NULL,
 							   encoding);
 		const char *body;
@@ -5320,7 +5320,8 @@ static int commit_staged_changes(struct repository *r,
 				struct commit *commit;
 				const char *msg;
 				const char *path = rebase_path_squash_msg();
-				const char *encoding = get_commit_output_encoding();
+				const char *encoding =
+					repo_get_commit_output_encoding(r);
 
 				if (parse_head(r, &commit)) {
 					ret = error(_("could not parse HEAD"));
@@ -6090,7 +6091,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 	get_commit_format(format, &revs);
 	free(format);
 	pp.fmt = revs.commit_format;
-	pp.output_encoding = get_log_output_encoding();
+	pp.output_encoding = repo_get_log_output_encoding(r);
 
 	if (setup_revisions(argc, argv, &revs, NULL) > 1) {
 		ret = error(_("make_script: unhandled options"));
diff --git a/submodule.c b/submodule.c
index 74d5766f07..9cfc7ca23c 100644
--- a/submodule.c
+++ b/submodule.c
@@ -501,7 +501,7 @@ static void print_submodule_diff_summary(struct repository *r, struct rev_info *
 	while ((commit = get_revision(rev))) {
 		struct pretty_print_context ctx = {0};
 		ctx.date_mode = rev->date_mode;
-		ctx.output_encoding = get_log_output_encoding();
+		ctx.output_encoding = repo_get_log_output_encoding(r);
 		strbuf_setlen(&sb, 0);
 		repo_format_commit_message(r, commit, format, &sb,
 				      &ctx);
-- 
2.47.0.73.g7a80afd5fd.dirty

[PATCH 2/3] pretty: don't rely on "the_repository"

[Kousik Sanagavarapu]

Change pretty so that it doesn't have to implicitly rely on being given
"the_repository" anymore and can handle any arbitrary repository -
hence also allowing us to remove the USE_THE_REPOSITORY_VARIABLE guard.

Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
 builtin/checkout.c    |  6 +++--
 builtin/log.c         |  4 +--
 builtin/merge.c       |  2 +-
 builtin/reset.c       |  2 +-
 builtin/rev-list.c    |  2 +-
 builtin/shortlog.c    |  3 ++-
 builtin/show-branch.c |  3 ++-
 builtin/stash.c       |  2 +-
 bundle.c              |  2 +-
 log-tree.c            |  2 +-
 pretty.c              | 63 ++++++++++++++++++++++++-------------------
 pretty.h              | 14 +++++-----
 range-diff.c          |  2 +-
 revision.c            |  6 ++---
 sequencer.c           | 11 ++++----
 15 files changed, 69 insertions(+), 55 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9c30000d3a..e90354c893 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -675,7 +675,8 @@ static void describe_detached_head(const char *msg, struct commit *commit)
 	struct strbuf sb = STRBUF_INIT;
 
 	if (!repo_parse_commit(the_repository, commit))
-		pp_commit_easy(CMIT_FMT_ONELINE, commit, &sb);
+		pp_commit_easy(the_repository, CMIT_FMT_ONELINE,
+			       commit, &sb);
 	if (print_sha1_ellipsis()) {
 		fprintf(stderr, "%s %s... %s\n", msg,
 			repo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV),
@@ -1063,7 +1064,8 @@ static void describe_one_orphan(struct strbuf *sb, struct commit *commit)
 	strbuf_add_unique_abbrev(sb, &commit->object.oid, DEFAULT_ABBREV);
 	strbuf_addch(sb, ' ');
 	if (!repo_parse_commit(the_repository, commit))
-		pp_commit_easy(CMIT_FMT_ONELINE, commit, sb);
+		pp_commit_easy(the_repository, CMIT_FMT_ONELINE,
+			       commit, sb);
 	strbuf_addch(sb, '\n');
 }
 
diff --git a/builtin/log.c b/builtin/log.c
index c28064c6f3..4e107bf775 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -186,7 +186,7 @@ static void cmd_log_init_defaults(struct rev_info *rev,
 				  struct log_config *cfg)
 {
 	if (cfg->fmt_pretty)
-		get_commit_format(cfg->fmt_pretty, rev);
+		get_commit_format(the_repository, cfg->fmt_pretty, rev);
 	if (cfg->default_follow)
 		rev->diffopt.flags.default_follow_renames = 1;
 	rev->verbose_header = 1;
@@ -2618,7 +2618,7 @@ static void print_commit(char sign, struct commit *commit, int verbose,
 		       repo_find_unique_abbrev(the_repository, &commit->object.oid, abbrev));
 	} else {
 		struct strbuf buf = STRBUF_INIT;
-		pp_commit_easy(CMIT_FMT_ONELINE, commit, &buf);
+		pp_commit_easy(the_repository, CMIT_FMT_ONELINE, commit, &buf);
 		fprintf(file, "%c %s %s\n", sign,
 		       repo_find_unique_abbrev(the_repository, &commit->object.oid, abbrev),
 		       buf.buf);
diff --git a/builtin/merge.c b/builtin/merge.c
index 84d0f3604b..7d5fda8acf 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -427,7 +427,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead
 		strbuf_addch(&out, '\n');
 		strbuf_addf(&out, "commit %s\n",
 			oid_to_hex(&commit->object.oid));
-		pretty_print_commit(&ctx, commit, &out);
+		pretty_print_commit(the_repository, &ctx, commit, &out);
 	}
 	write_file_buf(git_path_squash_msg(the_repository), out.buf, out.len);
 	strbuf_release(&out);
diff --git a/builtin/reset.c b/builtin/reset.c
index 7154f88826..6395b65584 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -139,7 +139,7 @@ static void print_new_head_line(struct commit *commit)
 	printf(_("HEAD is now at %s"),
 		repo_find_unique_abbrev(the_repository, &commit->object.oid, DEFAULT_ABBREV));
 
-	pp_commit_easy(CMIT_FMT_ONELINE, commit, &buf);
+	pp_commit_easy(the_repository, CMIT_FMT_ONELINE, commit, &buf);
 	if (buf.len > 0)
 		printf(" %s", buf.buf);
 	putchar('\n');
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index e8427a7c0c..1b564dea59 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -221,7 +221,7 @@ static void show_commit(struct commit *commit, void *data)
 		ctx.output_encoding = repo_get_log_output_encoding(the_repository);
 		ctx.color = revs->diffopt.use_color;
 		ctx.rev = revs;
-		pretty_print_commit(&ctx, commit, &buf);
+		pretty_print_commit(the_repository, &ctx, commit, &buf);
 		if (buf.len) {
 			if (revs->commit_format != CMIT_FMT_ONELINE)
 				graph_show_oneline(revs->graph);
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 81b8baf44a..426d86cc65 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -250,7 +250,8 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
 
 	if (!log->summary) {
 		if (log->user_format)
-			pretty_print_commit(&ctx, commit, &oneline);
+			pretty_print_commit(the_repository, &ctx,
+					    commit, &oneline);
 		else
 			repo_format_commit_message(the_repository, commit,
 						   "%s", &oneline, &ctx);
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index cd6bdf63bc..769e952175 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -301,7 +301,8 @@ static void show_one_commit(struct commit *commit, int no_name)
 	struct commit_name *name = commit_to_name(commit);
 
 	if (commit->object.parsed) {
-		pp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty);
+		pp_commit_easy(the_repository, CMIT_FMT_ONELINE,
+			       commit, &pretty);
 		pretty_str = pretty.buf;
 	}
 	skip_prefix(pretty_str, "[PATCH] ", &pretty_str);
diff --git a/builtin/stash.c b/builtin/stash.c
index 1399a1bbe2..2ffe5f7736 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1403,7 +1403,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
 						  &head_commit->object.oid,
 						  DEFAULT_ABBREV);
 	strbuf_addf(&msg, "%s: %s ", branch_name, head_short_sha1);
-	pp_commit_easy(CMIT_FMT_ONELINE, head_commit, &msg);
+	pp_commit_easy(the_repository, CMIT_FMT_ONELINE, head_commit, &msg);
 
 	strbuf_addf(&commit_tree_label, "index on %s\n", msg.buf);
 	commit_list_insert(head_commit, &parents);
diff --git a/bundle.c b/bundle.c
index aad00836b9..27a1cad633 100644
--- a/bundle.c
+++ b/bundle.c
@@ -485,7 +485,7 @@ static void write_bundle_prerequisites(struct commit *commit, void *data)
 	ctx.fmt = CMIT_FMT_ONELINE;
 	ctx.output_encoding = repo_get_log_output_encoding(the_repository);
 	strbuf_reset(&buf);
-	pretty_print_commit(&ctx, commit, &buf);
+	pretty_print_commit(the_repository, &ctx, commit, &buf);
 	strbuf_trim(&buf);
 
 	object = (struct object *)commit;
diff --git a/log-tree.c b/log-tree.c
index 6bbddcf85a..37ea632230 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -858,7 +858,7 @@ void show_log(struct rev_info *opt)
 		ctx.from_ident = &opt->from_ident;
 	if (opt->graph)
 		ctx.graph_width = graph_width(opt->graph);
-	pretty_print_commit(&ctx, commit, &msgbuf);
+	pretty_print_commit(the_repository, &ctx, commit, &msgbuf);
 
 	if (opt->add_signoff)
 		append_signoff(&msgbuf, 0, APPEND_SIGNOFF_DEDUP);
diff --git a/pretty.c b/pretty.c
index e7b069a48c..cc093c3787 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
 #include "git-compat-util.h"
 #include "config.h"
 #include "commit.h"
@@ -42,7 +40,8 @@ static struct cmt_fmt_map {
 static size_t builtin_formats_len;
 static size_t commit_formats_len;
 static size_t commit_formats_alloc;
-static struct cmt_fmt_map *find_commit_format(const char *sought);
+static struct cmt_fmt_map *find_commit_format(struct repository *r,
+					      const char *sought);
 
 int commit_format_is_empty(enum cmit_fmt fmt)
 {
@@ -116,7 +115,7 @@ static int git_pretty_formats_config(const char *var, const char *value,
 	return 0;
 }
 
-static void setup_commit_formats(void)
+static void setup_commit_formats(struct repository *r)
 {
 	struct cmt_fmt_map builtin_formats[] = {
 		{ "raw",	CMIT_FMT_RAW,		0,	0 },
@@ -140,7 +139,7 @@ static void setup_commit_formats(void)
 	COPY_ARRAY(commit_formats, builtin_formats,
 		   ARRAY_SIZE(builtin_formats));
 
-	git_config(git_pretty_formats_config, NULL);
+	repo_config(r, git_pretty_formats_config, NULL);
 }
 
 static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,
@@ -178,15 +177,17 @@ static struct cmt_fmt_map *find_commit_format_recursive(const char *sought,
 	return found;
 }
 
-static struct cmt_fmt_map *find_commit_format(const char *sought)
+static struct cmt_fmt_map *find_commit_format(struct repository *r,
+					      const char *sought)
 {
 	if (!commit_formats)
-		setup_commit_formats();
+		setup_commit_formats(r);
 
 	return find_commit_format_recursive(sought, sought, 0);
 }
 
-void get_commit_format(const char *arg, struct rev_info *rev)
+void get_commit_format(struct repository *r, const char *arg,
+		       struct rev_info *rev)
 {
 	struct cmt_fmt_map *commit_format;
 
@@ -205,7 +206,7 @@ void get_commit_format(const char *arg, struct rev_info *rev)
 		return;
 	}
 
-	commit_format = find_commit_format(arg);
+	commit_format = find_commit_format(r, arg);
 	if (!commit_format)
 		die("invalid --pretty format: %s", arg);
 
@@ -1434,7 +1435,8 @@ static void free_decoration_options(const struct decoration_options *opts)
 	free(opts->tag);
 }
 
-static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
+static size_t format_commit_one(struct repository *r,
+				struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
 {
@@ -1546,7 +1548,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 
 	/* these depend on the commit */
 	if (!commit->object.parsed)
-		parse_object(the_repository, &commit->object.oid);
+		parse_object(r, &commit->object.oid);
 
 	switch (placeholder[0]) {
 	case 'H':		/* commit hash */
@@ -1784,7 +1786,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	return 0;	/* unknown placeholder */
 }
 
-static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
+static size_t format_and_pad_commit(struct repository *r,
+				    struct strbuf *sb, /* in UTF-8 */
 				    const char *placeholder,
 				    struct format_commit_context *c)
 {
@@ -1803,7 +1806,8 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
 	}
 	while (1) {
 		int modifier = *placeholder == 'C';
-		size_t consumed = format_commit_one(&local_sb, placeholder, c);
+		size_t consumed = format_commit_one(r, &local_sb,
+						    placeholder, c);
 		total_consumed += consumed;
 
 		if (!modifier)
@@ -1888,7 +1892,8 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
 	return total_consumed;
 }
 
-static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
+static size_t format_commit_item(struct repository *r,
+				 struct strbuf *sb, /* in UTF-8 */
 				 const char *placeholder,
 				 struct format_commit_context *context)
 {
@@ -1930,9 +1935,9 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
 
 	orig_len = sb->len;
 	if (context->flush_type == no_flush)
-		consumed = format_commit_one(sb, placeholder, context);
+		consumed = format_commit_one(r, sb, placeholder, context);
 	else
-		consumed = format_and_pad_commit(sb, placeholder, context);
+		consumed = format_and_pad_commit(r, sb, placeholder, context);
 	if (magic == NO_MAGIC)
 		return consumed;
 
@@ -2001,7 +2006,7 @@ void repo_format_commit_message(struct repository *r,
 
 		if (skip_prefix(format, "%", &format))
 			strbuf_addch(sb, '%');
-		else if ((len = format_commit_item(sb, format, &context)))
+		else if ((len = format_commit_item(r, sb, format, &context)))
 			format += len;
 		else
 			strbuf_addch(sb, '%');
@@ -2034,7 +2039,8 @@ void repo_format_commit_message(struct repository *r,
 	repo_unuse_commit_buffer(r, commit, context.message);
 }
 
-static void pp_header(struct pretty_print_context *pp,
+static void pp_header(struct repository *r,
+		      struct pretty_print_context *pp,
 		      const char *encoding,
 		      const struct commit *commit,
 		      const char **msg_p,
@@ -2060,7 +2066,7 @@ static void pp_header(struct pretty_print_context *pp,
 		}
 
 		if (starts_with(line, "parent ")) {
-			if (linelen != the_hash_algo->hexsz + 8)
+			if (linelen != r->hash_algo->hexsz + 8)
 				die("bad parent line in commit");
 			continue;
 		}
@@ -2279,7 +2285,8 @@ void pp_remainder(struct pretty_print_context *pp,
 	}
 }
 
-void pretty_print_commit(struct pretty_print_context *pp,
+void pretty_print_commit(struct repository *r,
+			 struct pretty_print_context *pp,
 			 const struct commit *commit,
 			 struct strbuf *sb)
 {
@@ -2291,14 +2298,13 @@ void pretty_print_commit(struct pretty_print_context *pp,
 	int need_8bit_cte = pp->need_8bit_cte;
 
 	if (pp->fmt == CMIT_FMT_USERFORMAT) {
-		repo_format_commit_message(the_repository, commit,
+		repo_format_commit_message(r, commit,
 					   user_format, sb, pp);
 		return;
 	}
 
-	encoding = repo_get_log_output_encoding(the_repository);
-	msg = reencoded = repo_logmsg_reencode(the_repository, commit, NULL,
-					       encoding);
+	encoding = repo_get_log_output_encoding(r);
+	msg = reencoded = repo_logmsg_reencode(r, commit, NULL, encoding);
 
 	if (pp->fmt == CMIT_FMT_ONELINE || cmit_fmt_is_mail(pp->fmt))
 		indent = 0;
@@ -2326,7 +2332,7 @@ void pretty_print_commit(struct pretty_print_context *pp,
 		}
 	}
 
-	pp_header(pp, encoding, commit, &msg, sb);
+	pp_header(r, pp, encoding, commit, &msg, sb);
 	if (pp->fmt != CMIT_FMT_ONELINE && !cmit_fmt_is_mail(pp->fmt)) {
 		strbuf_addch(sb, '\n');
 	}
@@ -2358,13 +2364,14 @@ void pretty_print_commit(struct pretty_print_context *pp,
 	if (cmit_fmt_is_mail(pp->fmt) && sb->len <= beginning_of_body)
 		strbuf_addch(sb, '\n');
 
-	repo_unuse_commit_buffer(the_repository, commit, reencoded);
+	repo_unuse_commit_buffer(r, commit, reencoded);
 }
 
-void pp_commit_easy(enum cmit_fmt fmt, const struct commit *commit,
+void pp_commit_easy(struct repository *r, enum cmit_fmt fmt,
+		    const struct commit *commit,
 		    struct strbuf *sb)
 {
 	struct pretty_print_context pp = {0};
 	pp.fmt = fmt;
-	pretty_print_commit(&pp, commit, sb);
+	pretty_print_commit(r, &pp, commit, sb);
 }
diff --git a/pretty.h b/pretty.h
index df267afe4a..530782337a 100644
--- a/pretty.h
+++ b/pretty.h
@@ -81,8 +81,8 @@ void userformat_find_requirements(const char *fmt, struct userformat_want *w);
  * Shortcut for invoking pretty_print_commit if we do not have any context.
  * Context would be set empty except "fmt".
  */
-void pp_commit_easy(enum cmit_fmt fmt, const struct commit *commit,
-			struct strbuf *sb);
+void pp_commit_easy(struct repository *r, enum cmit_fmt fmt,
+		    const struct commit *commit, struct strbuf *sb);
 
 /*
  * Get information about user and date from "line", format it and
@@ -124,16 +124,18 @@ void repo_format_commit_message(struct repository *r,
  * Parse given arguments from "arg", check it for correctness and
  * fill struct rev_info.
  */
-void get_commit_format(const char *arg, struct rev_info *);
+void get_commit_format(struct repository *r, const char *arg,
+		       struct rev_info *);
 
 /*
  * Make a commit message with all rules from given "pp"
  * and put it into "sb".
  * Please use this function if you have a context (candidate for "pp").
  */
-void pretty_print_commit(struct pretty_print_context *pp,
-			const struct commit *commit,
-			struct strbuf *sb);
+void pretty_print_commit(struct repository *r,
+			 struct pretty_print_context *pp,
+			 const struct commit *commit,
+			 struct strbuf *sb);
 
 /*
  * Change line breaks in "msg" to "line_separator" and put it into "sb".
diff --git a/range-diff.c b/range-diff.c
index 10885ba301..e5fb27a65b 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -442,7 +442,7 @@ static void output_pair_header(struct diff_options *diffopt,
 			strbuf_addf(buf, "%s%s", color_reset, color);
 
 		strbuf_addch(buf, ' ');
-		pp_commit_easy(CMIT_FMT_ONELINE, commit, buf);
+		pp_commit_easy(the_repository, CMIT_FMT_ONELINE, commit, buf);
 	}
 	strbuf_addf(buf, "%s\n", color_reset);
 
diff --git a/revision.c b/revision.c
index c3a54577b0..1dfb8a52cf 100644
--- a/revision.c
+++ b/revision.c
@@ -2553,7 +2553,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--pretty")) {
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
-		get_commit_format(NULL, revs);
+		get_commit_format(the_repository, NULL, revs);
 	} else if (skip_prefix(arg, "--pretty=", &optarg) ||
 		   skip_prefix(arg, "--format=", &optarg)) {
 		/*
@@ -2562,7 +2562,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		 */
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
-		get_commit_format(optarg, revs);
+		get_commit_format(the_repository, optarg, revs);
 	} else if (!strcmp(arg, "--expand-tabs")) {
 		revs->expand_tabs_in_log = 8;
 	} else if (!strcmp(arg, "--no-expand-tabs")) {
@@ -2606,7 +2606,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->notes_opt.use_default_notes = 0;
 	} else if (!strcmp(arg, "--oneline")) {
 		revs->verbose_header = 1;
-		get_commit_format("oneline", revs);
+		get_commit_format(the_repository, "oneline", revs);
 		revs->pretty_given = 1;
 		revs->abbrev_commit = 1;
 	} else if (!strcmp(arg, "--graph")) {
diff --git a/sequencer.c b/sequencer.c
index 25783d3b8b..a51feb0242 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1477,7 +1477,7 @@ void print_commit_summary(struct repository *r,
 
 	rev.verbose_header = 1;
 	rev.show_root_diff = 1;
-	get_commit_format(format.buf, &rev);
+	get_commit_format(the_repository, format.buf, &rev);
 	rev.always_show_header = 0;
 	rev.diffopt.detect_rename = DIFF_DETECT_RENAME;
 	diff_setup_done(&rev.diffopt);
@@ -5883,7 +5883,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 			continue;
 
 		strbuf_reset(&oneline);
-		pretty_print_commit(pp, commit, &oneline);
+		pretty_print_commit(the_repository, pp, commit, &oneline);
 
 		to_merge = commit->parents ? commit->parents->next : NULL;
 		if (!to_merge) {
@@ -6017,7 +6017,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 				strbuf_addf(out, "%s onto\n", cmd_reset);
 			else {
 				strbuf_reset(&oneline);
-				pretty_print_commit(pp, commit, &oneline);
+				pretty_print_commit(the_repository, pp,
+						    commit, &oneline);
 				strbuf_addf(out, "%s %s # %s\n",
 					    cmd_reset, to, oneline.buf);
 			}
@@ -6088,7 +6089,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 		free(format);
 		format = xstrdup("%s");
 	}
-	get_commit_format(format, &revs);
+	get_commit_format(the_repository, format, &revs);
 	free(format);
 	pp.fmt = revs.commit_format;
 	pp.output_encoding = repo_get_log_output_encoding(r);
@@ -6122,7 +6123,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 			continue;
 		strbuf_addf(out, "%s %s ", insn,
 			    oid_to_hex(&commit->object.oid));
-		pretty_print_commit(&pp, commit, out);
+		pretty_print_commit(the_repository, &pp, commit, out);
 		if (is_empty)
 			strbuf_addf(out, " %s empty", comment_line_str);
 		strbuf_addch(out, '\n');
-- 
2.47.0.73.g7a80afd5fd.dirty

[PATCH 3/3] builtin/mailinfo: don't rely on "the_repository"

[Kousik Sanagavarapu]

Change builtin/mailinfo.c so that it doesn't have to rely on
"the_repository" anymore - hence also allowing us the remove the
USE_THE_REPOSITORY_VARIABLE guard.

Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
 builtin/mailinfo.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index 828b2b5845..9463a8780a 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -2,7 +2,6 @@
  * Another stupid program, this one parsing the headers of an
  * email to figure out authorship and subject
  */
-#define USE_THE_REPOSITORY_VARIABLE
 #include "builtin.h"
 #include "abspath.h"
 #include "environment.h"
@@ -52,7 +51,7 @@ static int parse_opt_quoted_cr(const struct option *opt, const char *arg, int un
 int cmd_mailinfo(int argc,
 		 const char **argv,
 		 const char *prefix,
-		 struct repository *repo UNUSED)
+		 struct repository *repo)
 {
 	struct metainfo_charset meta_charset;
 	struct mailinfo mi;
@@ -93,8 +92,7 @@ int cmd_mailinfo(int argc,
 
 	switch (meta_charset.policy) {
 	case CHARSET_DEFAULT:
-		mi.metainfo_charset =
-			repo_get_commit_output_encoding(the_repository);
+		mi.metainfo_charset = repo_get_commit_output_encoding(repo);
 		break;
 	case CHARSET_NO_REENCODE:
 		mi.metainfo_charset = NULL;
-- 
2.47.0.73.g7a80afd5fd.dirty

Re: [PATCH 1/3] repository: move git_*_encoding configs to repo scope

[shejialuo]

On Tue, Oct 15, 2024 at 08:01:22PM +0530, Kousik Sanagavarapu wrote:
> Move "git_commit_encoding" and "git_log_output_encoding" to "struct
> repository" and amend the functions associated with peeking at these
> values so that now they take a "struct repository *" argument
> accordingly.  While at it, rename the functions to repo_*() following
> our usual convention.
> 
> Doing so removes the implicit dependency of these variables on
> "the_repository", which is better because we now populate these
> variables per repository.
> 
> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
> ---
>  builtin/am.c       |  6 ++++--
>  builtin/blame.c    |  2 +-
>  builtin/commit.c   |  8 +++++---
>  builtin/log.c      |  3 ++-
>  builtin/mailinfo.c |  3 ++-
>  builtin/replay.c   |  3 ++-
>  builtin/rev-list.c |  2 +-
>  builtin/shortlog.c |  2 +-
>  bundle.c           |  2 +-
>  commit.c           |  9 ++++++---
>  config.c           | 10 ++++++----
>  environment.c      | 13 -------------
>  environment.h      |  6 ------
>  log-tree.c         |  4 ++--
>  pretty.c           |  2 +-
>  remote-curl.c      |  4 +++-
>  repository.c       | 13 +++++++++++++
>  repository.h       |  6 ++++++
>  revision.c         | 10 +++++-----
>  sequencer.c        | 17 +++++++++--------
>  submodule.c        |  2 +-
>  21 files changed, 71 insertions(+), 56 deletions(-)

For "git-mailinfo(1)" and "git-shortlog(1)", these two commands could
run outside of the repository. If we incorporate these two configs into
the "struct repository", we will have trouble when we remove the
"the_repository" global variable.

The patch in [1] will pass a NULL pointer for builtins with
"RUN_SETUP_GENTLY" flag.

[1] <d59b85b529865793c652d983d71a9fbb7e16b3e3.1728594828.git.gitgitgadget@gmail.com>

> diff --git a/config.c b/config.c
> index a11bb85da3..656748692d 100644
> --- a/config.c
> +++ b/config.c
> @@ -1690,13 +1690,15 @@ static int git_default_sparse_config(const char *var, const char *value)
>  static int git_default_i18n_config(const char *var, const char *value)
>  {
>  	if (!strcmp(var, "i18n.commitencoding")) {
> -		FREE_AND_NULL(git_commit_encoding);
> -		return git_config_string(&git_commit_encoding, var, value);
> +		FREE_AND_NULL(the_repository->git_commit_encoding);
> +		return git_config_string(&the_repository->git_commit_encoding,
> +					 var, value);
>  	}
>  
>  	if (!strcmp(var, "i18n.logoutputencoding")) {
> -		FREE_AND_NULL(git_log_output_encoding);
> -		return git_config_string(&git_log_output_encoding, var, value);
> +		FREE_AND_NULL(the_repository->git_log_output_encoding);
> +		return git_config_string(&the_repository->git_log_output_encoding,
> +					 var, value);
>  	}

There are many builtins will execute this config setups by calling
"config.c::git_default_config" and then "git_default_i18n_config". If we
need to use "repo" pointer, we may need to wrap this pointer. (This is
not the problem and it is not hard).

But what if the "repo" pointer is NULL? We still need to set the value
of these environment variables. Because when using "git-mailinfo(1)"
outside of the repo, we still need to set "git_commit_encoding"
according to the user's config.

So, from this perspective, I don't think it's a good idea to put these
two configs into "struct repository". Because we can use these two
configs outside of the repo, if we put them into "struct repository", it
is strange.

However, I either don't know which way we would apply. So, I cannot give
accurate answer here.

---

Patrick, I wanna ask you a question here. What's your envision here in
above situation. As you can see, if we put some configs into "struct
repository" and we run the builtins outside of the repo where we need to
set up configs, the "repo" is NULL. And we will get into trouble.

My idea is that if a config could be used outside of the repo, then we
should not put it into "struct repository".

Thanks,
Jialuo

Re: [PATCH 3/3] builtin/mailinfo: don't rely on "the_repository"

[shejialuo]

On Tue, Oct 15, 2024 at 08:01:24PM +0530, Kousik Sanagavarapu wrote:
> Change builtin/mailinfo.c so that it doesn't have to rely on
> "the_repository" anymore - hence also allowing us the remove the
> USE_THE_REPOSITORY_VARIABLE guard.
> 
> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
> ---
>  builtin/mailinfo.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
> index 828b2b5845..9463a8780a 100644
> --- a/builtin/mailinfo.c
> +++ b/builtin/mailinfo.c
> @@ -2,7 +2,6 @@
>   * Another stupid program, this one parsing the headers of an
>   * email to figure out authorship and subject
>   */
> -#define USE_THE_REPOSITORY_VARIABLE
>  #include "builtin.h"
>  #include "abspath.h"
>  #include "environment.h"
> @@ -52,7 +51,7 @@ static int parse_opt_quoted_cr(const struct option *opt, const char *arg, int un
>  int cmd_mailinfo(int argc,
>  		 const char **argv,
>  		 const char *prefix,
> -		 struct repository *repo UNUSED)
> +		 struct repository *repo)
>  {
>  	struct metainfo_charset meta_charset;
>  	struct mailinfo mi;
> @@ -93,8 +92,7 @@ int cmd_mailinfo(int argc,
>  
>  	switch (meta_charset.policy) {
>  	case CHARSET_DEFAULT:
> -		mi.metainfo_charset =
> -			repo_get_commit_output_encoding(the_repository);
> +		mi.metainfo_charset = repo_get_commit_output_encoding(repo);

This is wrong. We cannot simply pass the "repo" here. As [1] shows, we
will pass "NULL" for "repo" when running "git-mailinfo(1)" outside of
the repo. There is no harm when using "repo_get_commit_output_encoding".
Because this function will check whether "repo" is NULL and it will call
"repo_get_commit_output_encoding" and then return the default value
"UTF-8".

But what if the user has set up the "i18n.logoutputencoding" in the
config file? We will ignore. There are no tests in the current codebase
for running "git-mailinfo(1)" outside of the repo. But we should care
about this.

[1] <d59b85b529865793c652d983d71a9fbb7e16b3e3.1728594828.git.gitgitgadget@gmail.com>

>  		break;
>  	case CHARSET_NO_REENCODE:
>  		mi.metainfo_charset = NULL;
> -- 
> 2.47.0.73.g7a80afd5fd.dirty
> 
> 

Re: [RFC PATCH 0/3] pretty, builtin/mailinfo: don't rely on the_repository

[Taylor Blau]

On Tue, Oct 15, 2024 at 08:01:21PM +0530, Kousik Sanagavarapu wrote:
> Hi,
> Just a brief summary -
>
> 1/3 - the main changes are in environment.[ch] and repository.[ch], all
>       the others are just changes due to this change.
>
> 2/3 - the main changes are in pretty.[ch], all the other changes are due
>       to this change.
>
> 3/3 - This is pretty straight-forward.
>
> One may notice that there are more "the_repository" occurences now than
> before this change - which is good since it means that we have now made
> the respective dependencies explicit (these were previously implicit).
>
> The change in 1/3 is marked RFC since I was kind of skeptical about the
> "repo" check in the repo_*() functions being done at _that_ level.
> Since every other change in this series depends on this, I've marked all
> the other RFC as well.

I share the concern that others have raised in this thread about not
having the_repository when one of the affected commands is ran outside
of the repository.

I'll bring these patches into my tree, but let's hold off on queueing
them into 'seen' for now.

In the meantime, as a style suggestion, it might be nice to provide a
wrapper for function foo() -> repo_foo(), where the former still exists,
but is a wrapper for repo_foo(the_repository) like we have done in
other similar transitions.

Thanks,
Taylor

Re: [PATCH 1/3] repository: move git_*_encoding configs to repo scope

[Patrick Steinhardt]

On Wed, Oct 16, 2024 at 12:05:05AM +0800, shejialuo wrote:
> There are many builtins will execute this config setups by calling
> "config.c::git_default_config" and then "git_default_i18n_config". If we
> need to use "repo" pointer, we may need to wrap this pointer. (This is
> not the problem and it is not hard).
> 
> But what if the "repo" pointer is NULL? We still need to set the value
> of these environment variables. Because when using "git-mailinfo(1)"
> outside of the repo, we still need to set "git_commit_encoding"
> according to the user's config.
> 
> So, from this perspective, I don't think it's a good idea to put these
> two configs into "struct repository". Because we can use these two
> configs outside of the repo, if we put them into "struct repository", it
> is strange.
> 
> However, I either don't know which way we would apply. So, I cannot give
> accurate answer here.
> 
> ---
> 
> Patrick, I wanna ask you a question here. What's your envision here in
> above situation. As you can see, if we put some configs into "struct
> repository" and we run the builtins outside of the repo where we need to
> set up configs, the "repo" is NULL. And we will get into trouble.
> 
> My idea is that if a config could be used outside of the repo, then we
> should not put it into "struct repository".

I guess it would be nice to have a set of convenice functions in our
config code that know to handle the case where the passed-in repository
is `NULL`. If so, they'd only parse the global- and system-level config.
If it is set, it would parse all three of them.

I also kind of agree that it should likely not be put into the `struct
repository` in that case. I think where exactly to put things will
always depend on the current usecase. I bet that in most cases, we
should be able to get away with not storing the value anywhere global at
all, which would be the best solution in my opinion:

  - It can either be stored on the stack if we don't have to pass it
    around everywhere.

  - It can be passed around in a specific structure if we pass the value
    within in a certain subsystem, only.

  - Or we can parse it on an as-needed basis if it happens deep down in
    the calling stack when it's used essentially everwhere.

Now there will be situations where we used to store things globally as a
caching mechanism, and not caching it may have performance impacts. But
I guess that most cases do not fall into this category.

Patrick

Re: [PATCH 1/3] repository: move git_*_encoding configs to repo scope

[Kousik Sanagavarapu]

On Wed, Oct 16, 2024 at 12:05:05AM +0800, shejialuo wrote:
> On Tue, Oct 15, 2024 at 08:01:22PM +0530, Kousik Sanagavarapu wrote:
> > Move "git_commit_encoding" and "git_log_output_encoding" to "struct
> > repository" and amend the functions associated with peeking at these
> > values so that now they take a "struct repository *" argument
> > accordingly.  While at it, rename the functions to repo_*() following
> > our usual convention.
> > 
> > Doing so removes the implicit dependency of these variables on
> > "the_repository", which is better because we now populate these
> > variables per repository.
> > 
> > Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
> > 
> > [...]
> > 
> > diff --git a/config.c b/config.c
> > index a11bb85da3..656748692d 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -1690,13 +1690,15 @@ static int git_default_sparse_config(const char *var, const char *value)
> >  static int git_default_i18n_config(const char *var, const char *value)
> >  {
> >  	if (!strcmp(var, "i18n.commitencoding")) {
> > -		FREE_AND_NULL(git_commit_encoding);
> > -		return git_config_string(&git_commit_encoding, var, value);
> > +		FREE_AND_NULL(the_repository->git_commit_encoding);
> > +		return git_config_string(&the_repository->git_commit_encoding,
> > +					 var, value);
> >  	}
> >  
> >  	if (!strcmp(var, "i18n.logoutputencoding")) {
> > -		FREE_AND_NULL(git_log_output_encoding);
> > -		return git_config_string(&git_log_output_encoding, var, value);
> > +		FREE_AND_NULL(the_repository->git_log_output_encoding);
> > +		return git_config_string(&the_repository->git_log_output_encoding,
> > +					 var, value);
> >  	}
> 
> There are many builtins will execute this config setups by calling
> "config.c::git_default_config" and then "git_default_i18n_config". If we
> need to use "repo" pointer, we may need to wrap this pointer. (This is
> not the problem and it is not hard).
> 
> But what if the "repo" pointer is NULL? We still need to set the value
> of these environment variables. Because when using "git-mailinfo(1)"
> outside of the repo, we still need to set "git_commit_encoding"
> according to the user's config.
> 
> So, from this perspective, I don't think it's a good idea to put these
> two configs into "struct repository". Because we can use these two
> configs outside of the repo, if we put them into "struct repository", it
> is strange.

Makes sense.  Now that I've read this, it does indeed feel strange to
put these configs in "struct repository".

It also raises the question of how correct would it then be that these
configs and the configs similar to this are guarded by
USE_THE_REPOSITORY_VARIABLE macro - since they may not necessarily
depend on "the_repository" too - ie in cases when we are running outside
of any repo.

Re: [PATCH 1/3] repository: move git_*_encoding configs to repo scope

[Kousik Sanagavarapu]

On Wed, Oct 16, 2024 at 08:15:37AM +0200, Patrick Steinhardt wrote:
> On Wed, Oct 16, 2024 at 12:05:05AM +0800, shejialuo wrote:
> > There are many builtins will execute this config setups by calling
> > "config.c::git_default_config" and then "git_default_i18n_config". If we
> > need to use "repo" pointer, we may need to wrap this pointer. (This is
> > not the problem and it is not hard).
> > 
> > But what if the "repo" pointer is NULL? We still need to set the value
> > of these environment variables. Because when using "git-mailinfo(1)"
> > outside of the repo, we still need to set "git_commit_encoding"
> > according to the user's config.
> > 
> > So, from this perspective, I don't think it's a good idea to put these
> > two configs into "struct repository". Because we can use these two
> > configs outside of the repo, if we put them into "struct repository", it
> > is strange.
> > 
> > However, I either don't know which way we would apply. So, I cannot give
> > accurate answer here.
> > 
> > ---
> > 
> > Patrick, I wanna ask you a question here. What's your envision here in
> > above situation. As you can see, if we put some configs into "struct
> > repository" and we run the builtins outside of the repo where we need to
> > set up configs, the "repo" is NULL. And we will get into trouble.
> > 
> > My idea is that if a config could be used outside of the repo, then we
> > should not put it into "struct repository".
> 
> I guess it would be nice to have a set of convenice functions in our
> config code that know to handle the case where the passed-in repository
> is `NULL`. If so, they'd only parse the global- and system-level config.
> If it is set, it would parse all three of them.
> 
> I also kind of agree that it should likely not be put into the `struct
> repository` in that case. I think where exactly to put things will
> always depend on the current usecase. I bet that in most cases, we
> should be able to get away with not storing the value anywhere global at
> all, which would be the best solution in my opinion:
> 
>   - It can either be stored on the stack if we don't have to pass it
>     around everywhere.
> 
>   - It can be passed around in a specific structure if we pass the value
>     within in a certain subsystem, only.
> 
>   - Or we can parse it on an as-needed basis if it happens deep down in
>     the calling stack when it's used essentially everwhere.
> 
> Now there will be situations where we used to store things globally as a
> caching mechanism, and not caching it may have performance impacts. But
> I guess that most cases do not fall into this category.

I like the idea of dynamically fetching the value of the config and not
caching it somewhere - although in this particular case, ie the
*_encoding configs I'm guessing it would be better that we cache these
configs.

Now the important question is where.  In point 2, you mention about
having a separate structure - do you mean for all those configs which
would not be a part of "struct repository"?  Of course in their
respective subsystems.

Coming to point 3, won't we still have to store it somewhere when we do
need it deep down the calling stack where it is used everywhere, since
we need to pass it around?

Thanks

Re: [RFC PATCH 0/3] pretty, builtin/mailinfo: don't rely on the_repository

[Kousik Sanagavarapu]

On Tue, Oct 15, 2024 at 03:51:39PM -0400, Taylor Blau wrote:
> On Tue, Oct 15, 2024 at 08:01:21PM +0530, Kousik Sanagavarapu wrote:
> > Hi,
> > Just a brief summary -
> >
> > 1/3 - the main changes are in environment.[ch] and repository.[ch], all
> >       the others are just changes due to this change.
> >
> > 2/3 - the main changes are in pretty.[ch], all the other changes are due
> >       to this change.
> >
> > 3/3 - This is pretty straight-forward.
> >
> > One may notice that there are more "the_repository" occurences now than
> > before this change - which is good since it means that we have now made
> > the respective dependencies explicit (these were previously implicit).
> >
> > The change in 1/3 is marked RFC since I was kind of skeptical about the
> > "repo" check in the repo_*() functions being done at _that_ level.
> > Since every other change in this series depends on this, I've marked all
> > the other RFC as well.
> 
> I share the concern that others have raised in this thread about not
> having the_repository when one of the affected commands is ran outside
> of the repository.
> 
> I'll bring these patches into my tree, but let's hold off on queueing
> them into 'seen' for now.
> 
> In the meantime, as a style suggestion, it might be nice to provide a
> wrapper for function foo() -> repo_foo(), where the former still exists,
> but is a wrapper for repo_foo(the_repository) like we have done in
> other similar transitions.

Noted.  I'm thinking of waiting a bit more before re-rolling though -
I'll include this change there as well.

Thanks!

Re: [PATCH 1/3] repository: move git_*_encoding configs to repo scope

[Patrick Steinhardt]

On Wed, Oct 16, 2024 at 10:54:41PM +0530, Kousik Sanagavarapu wrote:
> On Wed, Oct 16, 2024 at 08:15:37AM +0200, Patrick Steinhardt wrote:
> > I guess it would be nice to have a set of convenice functions in our
> > config code that know to handle the case where the passed-in repository
> > is `NULL`. If so, they'd only parse the global- and system-level config.
> > If it is set, it would parse all three of them.
> > 
> > I also kind of agree that it should likely not be put into the `struct
> > repository` in that case. I think where exactly to put things will
> > always depend on the current usecase. I bet that in most cases, we
> > should be able to get away with not storing the value anywhere global at
> > all, which would be the best solution in my opinion:
> > 
> >   - It can either be stored on the stack if we don't have to pass it
> >     around everywhere.
> > 
> >   - It can be passed around in a specific structure if we pass the value
> >     within in a certain subsystem, only.
> > 
> >   - Or we can parse it on an as-needed basis if it happens deep down in
> >     the calling stack when it's used essentially everwhere.
> > 
> > Now there will be situations where we used to store things globally as a
> > caching mechanism, and not caching it may have performance impacts. But
> > I guess that most cases do not fall into this category.
> 
> I like the idea of dynamically fetching the value of the config and not
> caching it somewhere - although in this particular case, ie the
> *_encoding configs I'm guessing it would be better that we cache these
> configs.
> 
> Now the important question is where.  In point 2, you mention about
> having a separate structure - do you mean for all those configs which
> would not be a part of "struct repository"?  Of course in their
> respective subsystems.

I was mostly referring to configs that apply to a specific subsystem. So
if it is used in that subsystem only, then it likely is a good idea to
put the cached value into a structure that gets passed around. Many
subsystems already do have such a structure, so it's only a matter of
reading the value once at the entry point into the subsystem.

> Coming to point 3, won't we still have to store it somewhere when we do
> need it deep down the calling stack where it is used everywhere, since
> we need to pass it around?

I was mostly referring to the case where it's only used by a single
function deep down in the calling stack. So here it could just be parsed
ad-hoc.

In the end we'll probably have to decide on a case by case basis what's
best.

Patrick

Re: [PATCH 1/3] repository: move git_*_encoding configs to repo scope

[shejialuo]

On Wed, Oct 16, 2024 at 08:15:37AM +0200, Patrick Steinhardt wrote:
> On Wed, Oct 16, 2024 at 12:05:05AM +0800, shejialuo wrote:
> > There are many builtins will execute this config setups by calling
> > "config.c::git_default_config" and then "git_default_i18n_config". If we
> > need to use "repo" pointer, we may need to wrap this pointer. (This is
> > not the problem and it is not hard).
> > 
> > But what if the "repo" pointer is NULL? We still need to set the value
> > of these environment variables. Because when using "git-mailinfo(1)"
> > outside of the repo, we still need to set "git_commit_encoding"
> > according to the user's config.
> > 
> > So, from this perspective, I don't think it's a good idea to put these
> > two configs into "struct repository". Because we can use these two
> > configs outside of the repo, if we put them into "struct repository", it
> > is strange.
> > 
> > However, I either don't know which way we would apply. So, I cannot give
> > accurate answer here.
> > 
> > ---
> > 
> > Patrick, I wanna ask you a question here. What's your envision here in
> > above situation. As you can see, if we put some configs into "struct
> > repository" and we run the builtins outside of the repo where we need to
> > set up configs, the "repo" is NULL. And we will get into trouble.
> > 
> > My idea is that if a config could be used outside of the repo, then we
> > should not put it into "struct repository".
> 
> I guess it would be nice to have a set of convenice functions in our
> config code that know to handle the case where the passed-in repository
> is `NULL`. If so, they'd only parse the global- and system-level config.
> If it is set, it would parse all three of them.
> 
> I also kind of agree that it should likely not be put into the `struct
> repository` in that case. I think where exactly to put things will
> always depend on the current usecase. I bet that in most cases, we
> should be able to get away with not storing the value anywhere global at
> all, which would be the best solution in my opinion:
> 
>   - It can either be stored on the stack if we don't have to pass it
>     around everywhere.
> 
>   - It can be passed around in a specific structure if we pass the value
>     within in a certain subsystem, only.
> 
>   - Or we can parse it on an as-needed basis if it happens deep down in
>     the calling stack when it's used essentially everwhere.
> 
> Now there will be situations where we used to store things globally as a
> caching mechanism, and not caching it may have performance impacts. But
> I guess that most cases do not fall into this category.
> 

Thanks for the direction. I will dive into code to think about how we
could do this. Actually, I have tried to refactor "git-apply(1)" to
remove "the_repository". And I found it hard to remove dependency
"environment.c". It's a long journey.

> Patrick

Thanks,
Jialuo