[PATCH 0/1] [gsoc][patch] trace.c, git.c: removed unnecessary parameter to trace_repo_setup

[PATCH 0/1] [gsoc][patch] trace.c, git.c: removed unnecessary parameter to trace_repo_setup

[Idriss Fekir]

Hi, this is my first ever contribution to git.
This is about the #FIXME in trace_repo_setup, which had prefix as a parameter before prefix was added to startup_info


idriss fekir (1):
  remove parameter (prefix) from trace_repo_setup

 git.c   | 2 +-
 trace.c | 6 +++---
 trace.h | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)


base-commit: c867e4fa180bec4750e9b54eb10f459030dbebfd
-- 
2.39.1

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

[Idriss Fekir]

From: idriss fekir <mcsm224@gmail.com>

Signed-off-by: Idriss Fekir <mcsm224@gmail.com>
---
 git.c   | 2 +-
 trace.c | 6 +++---
 trace.h | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

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
-- 
2.39.1

Re:[PATCH 1/1] [gsoc][patch] trace.c, git.c: removed unnecessary parameter to trace_repo_setup

[Shuqi Liang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 766 bytes --]

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

Re: [PATCH 1/1] remove parameter (prefix) from trace_repo_setup

[Junio C Hamano]

Idriss Fekir <mcsm224@gmail.com> writes:

> From: idriss fekir <mcsm224@gmail.com>

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 1/1] trace.c, git.c: remove unnecessary parameter to trace_repo_setup

[Idriss Fekir]

From: idriss fekir <mcsm224@gmail.com>

trace_repo_setup of trace.c is called with the argument 'prefix' from
only one location, run_builtin of git.c, which sets 'prefix' to
the return value of setup_git_directory or setup_git_directory_gently
(a wrapper of the former).

Now that "prefix" is in startup_info there is no need for the parameter
of trace_repo_setup because setup_git_directory sets
"startup_info->prefix" to the same value it returns. It would be less
confusing to use "prefix" from startup_info instead of passing it as
a parameter.

Signed-off-by: Idriss Fekir <mcsm224@gmail.com>
---
Thank you very much, Shuqi Liang and Junio C Hamano for your valuable critique and suggestions.

 git.c   | 2 +-
 trace.c | 7 +++----
 trace.h | 2 +-
 3 files changed, 5 insertions(+), 6 deletions(-)

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..efa4e2d8e0 100644
--- a/trace.c
+++ b/trace.c
@@ -291,10 +291,9 @@ static const char *quote_crnl(const char *path)
 	return new_path.buf;
 }
 
-/* FIXME: move prefix to startup_info struct and get rid of this arg */
-void trace_repo_setup(const char *prefix)
+void trace_repo_setup(void)
 {
-	const char *git_work_tree;
+	const char *git_work_tree, *prefix = startup_info->prefix;
 	char *cwd;
 
 	if (!trace_want(&trace_setup_key))
@@ -305,7 +304,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..b6e35b9470 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(void);
 
 /**
  * Checks whether the trace key is enabled. Used to prevent expensive
-- 
2.39.2

Re: [PATCH 1/1] trace.c, git.c: remove unnecessary parameter to trace_repo_setup

[Christian Couder]

On Thu, Feb 16, 2023 at 12:36 AM Idriss Fekir <mcsm224@gmail.com> wrote:
>
> From: idriss fekir <mcsm224@gmail.com>

I think the subject could be improved a bit more, and it looks like
it's the second version of the patch, so something like the following
might have been better:

[PATCH v2 1/1] trace: remove unnecessary parameter to trace_repo_setup()

(Please use "v3" if you resend it with some changes.)

> trace_repo_setup of trace.c is called with the argument 'prefix' from
> only one location, run_builtin of git.c, which sets 'prefix' to
> the return value of setup_git_directory or setup_git_directory_gently
> (a wrapper of the former).

It might be a bit easier to read when functions names have "()" after
them like "trace_repo_setup()" instead of "trace_repo_setup".

Otherwise your patch looks good to me. Thanks!

[PATCH v3 1/1] trace.c, git.c: remove unnecessary parameter to trace_repo_setup()

[Idriss Fekir]

From: idriss fekir <mcsm224@gmail.com>

trace_repo_setup() of trace.c is called with the argument 'prefix' from
only one location, run_builtin of git.c, which sets 'prefix' to the return
value of setup_git_directory() or setup_git_directory_gently() (a wrapper
of the former).

Now that "prefix" is in startup_info there is no need for the parameter
of trace_repo_setup() because setup_git_directory() sets "startup_info->prefix"
to the same value it returns. It would be less confusing to use "prefix"
from startup_info instead of passing it as an argument.

Signed-off-by: Idriss Fekir <mcsm224@gmail.com>
---
Thank you very much, Christian Couder.
 git.c   | 2 +-
 trace.c | 7 +++----
 trace.h | 2 +-
 3 files changed, 5 insertions(+), 6 deletions(-)

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..efa4e2d8e0 100644
--- a/trace.c
+++ b/trace.c
@@ -291,10 +291,9 @@ static const char *quote_crnl(const char *path)
 	return new_path.buf;
 }
 
-/* FIXME: move prefix to startup_info struct and get rid of this arg */
-void trace_repo_setup(const char *prefix)
+void trace_repo_setup(void)
 {
-	const char *git_work_tree;
+	const char *git_work_tree, *prefix = startup_info->prefix;
 	char *cwd;
 
 	if (!trace_want(&trace_setup_key))
@@ -305,7 +304,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..b6e35b9470 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(void);
 
 /**
  * Checks whether the trace key is enabled. Used to prevent expensive
-- 
2.39.2

Re: [PATCH v3 1/1] trace.c, git.c: remove unnecessary parameter to trace_repo_setup()

[Junio C Hamano]

Idriss Fekir <mcsm224@gmail.com> writes:

> From: idriss fekir <mcsm224@gmail.com>
>
> trace_repo_setup() of trace.c is called with the argument 'prefix' from
> only one location, run_builtin of git.c, which sets 'prefix' to the return
> value of setup_git_directory() or setup_git_directory_gently() (a wrapper
> of the former).

The former is the wrapper of the latter, though ;-)

> Now that "prefix" is in startup_info there is no need for the parameter
> of trace_repo_setup() because setup_git_directory() sets "startup_info->prefix"
> to the same value it returns. It would be less confusing to use "prefix"
> from startup_info instead of passing it as an argument.

... but for commands with neither RUN_SETUP|RUN_SETUP_GENTLY bits
requested, the prefix is set it to NULL by run_builtin(), and
setup_git_directory.  What value does startup_info->prefix get in
that case?  If we know it will always NULL (and I suspect that is
the case, but I haven't followed the codepaths myself to find it out
lately, so you should without taking my word blindly), saying that
at the same place you mentioned the return value of setup_git_*()
functions in the first paragraph would make the reasoning perfect.

Otherwise, very nicely done.

Thanks.

Re:[PATCH v3 1/1] trace.c, git.c: remove unnecessary parameter to trace_repo_setup()

[Idriss Fekir]

On 2/21/23 21:00, Junio C Hamano wrote:
 > Idriss Fekir <mcsm224@gmail.com> writes:
 >
 >> From: idriss fekir <mcsm224@gmail.com>
 >>
 >> trace_repo_setup() of trace.c is called with the argument 'prefix' from
 >> only one location, run_builtin of git.c, which sets 'prefix' to the
return
 >> value of setup_git_directory() or setup_git_directory_gently() (a
wrapper
 >> of the former).
 >
 > The former is the wrapper of the latter, though ;-)
Oh sorry about that, that's what i meant but wrote it in reverse.

 >
 >> Now that "prefix" is in startup_info there is no need for the parameter
 >> of trace_repo_setup() because setup_git_directory() sets
"startup_info->prefix"
 >> to the same value it returns. It would be less confusing to use "prefix"
 >> from startup_info instead of passing it as an argument.
 >
 > ... but for commands with neither RUN_SETUP|RUN_SETUP_GENTLY bits
 > requested, the prefix is set it to NULL by run_builtin(), and
 > setup_git_directory.  What value does startup_info->prefix get in
 > that case?  If we know it will always NULL (and I suspect that is
 > the case, but I haven't followed the codepaths myself to find it out
 > lately, so you should without taking my word blindly), saying that
 > at the same place you mentioned the return value of setup_git_*()
 > functions in the first paragraph would make the reasoning perfect.
 >
 > Otherwise, very nicely done.
 >
 > Thanks.
 >
I did check if startup_info->prefix is *always* set, that doesn't seem
to be the case, but for trace_repo_setup() this doesn't matter as it will
never be called if neither RUN_SETUP nor RUN_SETUP_GENTLY are set
("if (run_setup &&..."). I also think there is a typo in the comment after
that if-statement, it should be "/* set_git_dir().." instead of
"/* get_git_dir()...". I have a question, why isn't startup_info initialized
with default values in setup.c?


Thanks a lot for your valuable feedback.

PS: I'm so sorry if this email has been sent multiple times, i thought it wasn't at first.

Kind regards.