[1/1] remove parameter (prefix) from trace_repo_setup

Commit Message

Idriss Fekir Feb. 15, 2023, 10:42 a.m. UTC

From: idriss fekir <[e-mail]>

Signed-off-by: Idriss Fekir <[e-mail]>
---
 git.c   | 2 +-
 trace.c | 6 +++---
 trace.h | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

Comments

Shuqi Liang Feb. 15, 2023, 5:55 p.m. UTC | #1

Hello  Idriss,

I have some suggestions：

Seen like you didn't describe the existing problem and how you fix it
in the commit message. You can start with a description of the
existing problem in the present tense in your commit message. Also,
you should use the imperative mood to describe the change you make.

Maybe you can write something like the below in your commit message
after you use git commit -s.
----------------------------------------------------------------------------------------
trace.c, git.c: removed unnecessary parameter to trace_repo_setup

your description of the existing problem.............

Fix them.

Signed-off-by: ...
-------------------------------------------------------------------------------------------





Regards,
Shuqi

Junio C Hamano Feb. 15, 2023, 5:56 p.m. UTC | #2

Idriss Fekir <[e-mail]> writes:

> From: idriss fekir <[e-mail]>

This blank space is for you to explain why this is a good thing to
do, which is missing here.

> diff --git a/trace.c b/trace.c
> index 794a087c21..316070a43e 100644
> --- a/trace.c
> +++ b/trace.c
> @@ -292,9 +292,9 @@ static const char *quote_crnl(const char *path)
>  }
>  
>  /* FIXME: move prefix to startup_info struct and get rid of this arg */

I do not think this comment was meant as an instruction to BLINDLY
remove the parameter and instead use from startup_info.  Instead,
the "FIX" in "FIXME" would involve that whoever does the fix checks
the caller that reaches this function and makes sure that "prefix"
value the caller passes exactly matches what is in the prefix member
of the startup_info.  Documenting that work should become a major
part of the proposed log message.  After doing that, this comment
is no longer valid and needs to be removed.

> -void trace_repo_setup(const char *prefix)
> +void trace_repo_setup()

"void trace_repo_setup(void)"

> -void trace_repo_setup(const char *prefix);
> +void trace_repo_setup();

"void trace_repo_setup(void);"

Thanks.

Patch

diff --git a/git.c b/git.c
index 96b0a2837d..6171fd6769 100644
--- a/git.c
+++ b/git.c
@@ -430,7 +430,7 @@  static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 		use_pager = 1;
 	if (run_setup && startup_info->have_repository)
 		/* get_git_dir() may set up repo, avoid that */
-		trace_repo_setup(prefix);
+		trace_repo_setup();
 	commit_pager_choice();
 
 	if (!help && p->option & NEED_WORK_TREE)
diff --git a/trace.c b/trace.c
index 794a087c21..316070a43e 100644
--- a/trace.c
+++ b/trace.c
@@ -292,9 +292,9 @@  static const char *quote_crnl(const char *path)
 }
 
 /* FIXME: move prefix to startup_info struct and get rid of this arg */
-void trace_repo_setup(const char *prefix)
+void trace_repo_setup()
 {
-	const char *git_work_tree;
+	const char *git_work_tree, *prefix = startup_info->prefix;
 	char *cwd;
 
 	if (!trace_want(&trace_setup_key))
@@ -305,7 +305,7 @@  void trace_repo_setup(const char *prefix)
 	if (!(git_work_tree = get_git_work_tree()))
 		git_work_tree = "(null)";
 
-	if (!prefix)
+	if (!startup_info->prefix)
 		prefix = "(null)";
 
 	trace_printf_key(&trace_setup_key, "setup: git_dir: %s\n", quote_crnl(get_git_dir()));
diff --git a/trace.h b/trace.h
index 4e771f86ac..ca943037c6 100644
--- a/trace.h
+++ b/trace.h
@@ -93,7 +93,7 @@  extern struct trace_key trace_default_key;
 extern struct trace_key trace_perf_key;
 extern struct trace_key trace_setup_key;
 
-void trace_repo_setup(const char *prefix);
+void trace_repo_setup();
 
 /**
  * Checks whether the trace key is enabled. Used to prevent expensive