[PATCH] git.c: remove the_repository dependence in run_builtin()

[PATCH] git.c: remove the_repository dependence in run_builtin()

[Lidong Yan]

run_builtin() takes a repo parameter, so the use of the_repository
is no longer necessary. Removed the usage of the_repository.

The comment before trace_repo_setup() advises not to use get_git_dir(),
but this note is unrelated to trace_repo_setup() itself. Additionally,
get_git_dir() has now been renamed to repo_get_git_dir(). Remove this
comment line.

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
 git.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/git.c b/git.c
index 77c4359522..429ad1c2fb 100644
--- a/git.c
+++ b/git.c
@@ -462,12 +462,11 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 	precompose_argv_prefix(argc, argv, NULL);
 	if (use_pager == -1 && run_setup &&
 		!(p->option & DELAY_PAGER_CONFIG))
-		use_pager = check_pager_config(the_repository, p->cmd);
+		use_pager = check_pager_config(repo, p->cmd);
 	if (use_pager == -1 && p->option & USE_PAGER)
 		use_pager = 1;
 	if (run_setup && startup_info->have_repository)
-		/* get_git_dir() may set up repo, avoid that */
-		trace_repo_setup(the_repository);
+		trace_repo_setup(repo);
 	commit_pager_choice();
 
 	if (!help && p->option & NEED_WORK_TREE)
-- 
2.43.0

Re: [PATCH] git.c: remove the_repository dependence in run_builtin()

[Junio C Hamano]

Lidong Yan <yldhome2d2@gmail.com> writes:

> run_builtin() takes a repo parameter, so the use of the_repository
> is no longer necessary. Removed the usage of the_repository.

Good.  The caller always calls this function with the_repository, so
this patch does not change anything in the bigger picture.

> The comment before trace_repo_setup() advises not to use get_git_dir(),
> but this note is unrelated to trace_repo_setup() itself. Additionally,
> get_git_dir() has now been renamed to repo_get_git_dir(). Remove this
> comment line.

Isn't it still relevant to explain the reason why this codepath
avoids calling the repo_get_git_dir() function?

e5b17bda (git: ensure correct git directory setup with -h,
2021-12-06) tells us that the comment is about use of
startup_info->have_repository, which was added by a9ca8a85
(builtins: print setup info if repo is found, 2010-11-26).

> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
> ---
>  git.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/git.c b/git.c
> index 77c4359522..429ad1c2fb 100644
> --- a/git.c
> +++ b/git.c
> @@ -462,12 +462,11 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
>  	precompose_argv_prefix(argc, argv, NULL);
>  	if (use_pager == -1 && run_setup &&
>  		!(p->option & DELAY_PAGER_CONFIG))
> -		use_pager = check_pager_config(the_repository, p->cmd);
> +		use_pager = check_pager_config(repo, p->cmd);
>  	if (use_pager == -1 && p->option & USE_PAGER)
>  		use_pager = 1;
>  	if (run_setup && startup_info->have_repository)
> -		/* get_git_dir() may set up repo, avoid that */
> -		trace_repo_setup(the_repository);
> +		trace_repo_setup(repo);
>  	commit_pager_choice();
>  
>  	if (!help && p->option & NEED_WORK_TREE)

Re: [PATCH] git.c: remove the_repository dependence in run_builtin()

[lidongyan]

Junio C Hamano <gitster@pobox.com> 写道：
> 
> Lidong Yan <yldhome2d2@gmail.com> writes:
> 
>> run_builtin() takes a repo parameter, so the use of the_repository
>> is no longer necessary. Removed the usage of the_repository.
> 
> Good.  The caller always calls this function with the_repository, so
> this patch does not change anything in the bigger picture.
> 
>> The comment before trace_repo_setup() advises not to use get_git_dir(),
>> but this note is unrelated to trace_repo_setup() itself. Additionally,
>> get_git_dir() has now been renamed to repo_get_git_dir(). Remove this
>> comment line.
> 
> Isn't it still relevant to explain the reason why this codepath
> avoids calling the repo_get_git_dir() function?
> 
> e5b17bda (git: ensure correct git directory setup with -h,
> 2021-12-06) tells us that the comment is about use of
> startup_info->have_repository, which was added by a9ca8a85
> (builtins: print setup info if repo is found, 2010-11-26).

In commit a9ca8a85, the intention was to avoid calling get_git_dir() before
confirming that we are indeed inside a Git repository and determining the prefix
between the current working directory and the repository root.

However, I believe this concern is no longer relevant:
repo_get_git_dir() no longer sets up the Git repository environment as the
original comment implied. Instead, all the necessary setup is now handled
by setup_git_env(), which is invoked by setup_git_directory_gently() after
the prefix has been determined. As a result, I believe it is no longer necessary
to retain this comment message.

Re: [PATCH] git.c: remove the_repository dependence in run_builtin()

[Junio C Hamano]

lidongyan <502024330056@smail.nju.edu.cn> writes:

>>> The comment before trace_repo_setup() advises not to use get_git_dir(),
>>> but this note is unrelated to trace_repo_setup() itself. Additionally,
>>> get_git_dir() has now been renamed to repo_get_git_dir(). Remove this
>>> comment line.
>> ... 
> However, I believe this concern is no longer relevant:
> repo_get_git_dir() no longer sets up the Git repository environment as the
> original comment implied. Instead, all the necessary setup is now handled
> by setup_git_env(), which is invoked by setup_git_directory_gently() after
> the prefix has been determined. As a result, I believe it is no longer necessary
> to retain this comment message.

If so, please update the explanation.  It reads as if the only
reason for removing the comment is because of the rename of the
function.  If the reason is because the behaviour has changed and it
is not relevant anymore, the readers should be told about it.

Thanks.

[PATCH v2] git.c: remove the_repository dependence in run_builtin()

[Lidong Yan]

run_builtin() takes a repo parameter, so the use of the_repository
is no longer necessary. Removed the usage of the_repository.

The comment preceding trace_repo_setup() was originally introduced
in commit a9ca8a85. Since get_git_dir() modifies global variables
such as git_dir and git_objects_dir which only valid when inside a git
repository. The intention of the comment was to emphasize that
get_git_dir() should not be called before confirming that the current
directory is indeed part of a git repository. However, get_git_dir()
has since been renamed to repo_get_git_dir(), and repo_get_git_dir()
no longer modifies the global the_repository state. As a result,
the original comment is no longer relevant and can be safely removed.

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
 git.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/git.c b/git.c
index 77c4359522..429ad1c2fb 100644
--- a/git.c
+++ b/git.c
@@ -462,12 +462,11 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 	precompose_argv_prefix(argc, argv, NULL);
 	if (use_pager == -1 && run_setup &&
 		!(p->option & DELAY_PAGER_CONFIG))
-		use_pager = check_pager_config(the_repository, p->cmd);
+		use_pager = check_pager_config(repo, p->cmd);
 	if (use_pager == -1 && p->option & USE_PAGER)
 		use_pager = 1;
 	if (run_setup && startup_info->have_repository)
-		/* get_git_dir() may set up repo, avoid that */
-		trace_repo_setup(the_repository);
+		trace_repo_setup(repo);
 	commit_pager_choice();
 
 	if (!help && p->option & NEED_WORK_TREE)
-- 
2.50.0-rc2

Re: [PATCH v2] git.c: remove the_repository dependence in run_builtin()

[Junio C Hamano]

Lidong Yan <yldhome2d2@gmail.com> writes:

> The comment preceding trace_repo_setup() was originally introduced
> in commit a9ca8a85. Since get_git_dir() modifies global variables
> such as git_dir and git_objects_dir which only valid when inside a git
> repository. The intention of the comment was to emphasize that
> get_git_dir() should not be called before confirming that the current
> directory is indeed part of a git repository. However, get_git_dir()
> has since been renamed to repo_get_git_dir(), and repo_get_git_dir()
> no longer modifies the global the_repository state. As a result,
> the original comment is no longer relevant and can be safely removed.

Sorry, but the above makes it sound as if 246deeac (environment:
make `get_git_dir()` accept a repository, 2024-09-12) that retired
get_git_dir() and introduced repo_get_git_dir() was the culprit that
made their semantics change, but is that really true?  It appears
that in the version immediately before that commit, get_git_dir()
was also a reference to a variable, without any lazy initialization
the above message says that the code tries to avoid, so I am even
more confused after reading the above.

Perhaps you have 73f192c9 (setup: don't perform lazy initialization
of repository state, 2017-06-20) in mind?  That one did stop calling
setup_git_env() and instead force a hard BUG("") when git_dir is not
set up yet.  And that BUG("") still survives in repo_get_git_dir()
we have today.

So the call to repo_get_git_dir() may still not be made from this
code path.  It may not attempt to set up, but instead it would die
if we haven't successfully set up the repository before.  The
relevance of the comment was not changed by 246deeac that moved this
code from get_git_dir() to repo_get_git_dir(), and more importantly,
it was not changed by this patch we are reviewing here.

But stepping back a bit, is it what a9ca8a85 originally wanted to
achieve with this comment to "avoid calling get_git_dir()" in the
first place?  Once the guarding condition is satisfied, it calls
trace_repo_setup(), which in turn calls get_git_dir() anyway.
Perhaps it wanted to explain why startup_info->have_repository is
checked here?

> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
> ---
>  git.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/git.c b/git.c
> index 77c4359522..429ad1c2fb 100644
> --- a/git.c
> +++ b/git.c
> @@ -462,12 +462,11 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
>  	precompose_argv_prefix(argc, argv, NULL);
>  	if (use_pager == -1 && run_setup &&
>  		!(p->option & DELAY_PAGER_CONFIG))
> -		use_pager = check_pager_config(the_repository, p->cmd);
> +		use_pager = check_pager_config(repo, p->cmd);
>  	if (use_pager == -1 && p->option & USE_PAGER)
>  		use_pager = 1;
>  	if (run_setup && startup_info->have_repository)
> -		/* get_git_dir() may set up repo, avoid that */
> -		trace_repo_setup(the_repository);
> +		trace_repo_setup(repo);
>  	commit_pager_choice();
>  
>  	if (!help && p->option & NEED_WORK_TREE)

Re: [PATCH v2] git.c: remove the_repository dependence in run_builtin()

[Lidong Yan]

Junio C Hamano <gitster@pobox.com> writes：
> Sorry, but the above makes it sound as if 246deeac (environment:
> make `get_git_dir()` accept a repository, 2024-09-12) that retired
> get_git_dir() and introduced repo_get_git_dir() was the culprit that
> made their semantics change, but is that really true?  It appears
> that in the version immediately before that commit, get_git_dir()
> was also a reference to a variable, without any lazy initialization
> the above message says that the code tries to avoid, so I am even
> more confused after reading the above.

I was reading the code in master and noticed that repo_get_git_dir()
no longer sets up the environment. I’ve learned that I should use git blame
to identify which commit changed the code, so I can make my message clearer.

> Perhaps you have 73f192c9 (setup: don't perform lazy initialization
> of repository state, 2017-06-20) in mind?  That one did stop calling
> setup_git_env() and instead force a hard BUG("") when git_dir is not
> set up yet.  And that BUG("") still survives in repo_get_git_dir()
> we have today.

Exactly

> So the call to repo_get_git_dir() may still not be made from this
> code path.  It may not attempt to set up, but instead it would die
> if we haven't successfully set up the repository before.  The
> relevance of the comment was not changed by 246deeac that moved this
> code from get_git_dir() to repo_get_git_dir(), and more importantly,
> it was not changed by this patch we are reviewing here.
> 
> But stepping back a bit, is it what a9ca8a85 originally wanted to
> achieve with this comment to "avoid calling get_git_dir()" in the
> first place?  Once the guarding condition is satisfied, it calls
> trace_repo_setup(), which in turn calls get_git_dir() anyway.
> Perhaps it wanted to explain why startup_info->have_repository is
> checked here?

Yes, I think so. Maybe updating the comment to say 
“call repo_get_git_dir() after setting up the_repository”
would be more appropriate.

Thank you for your review,
Lidong

Re: [PATCH v2] git.c: remove the_repository dependence in run_builtin()

[Junio C Hamano]

Lidong Yan <502024330056@smail.nju.edu.cn> writes:

> Junio C Hamano <gitster@pobox.com> writes：
>> Sorry, but the above makes it sound as if 246deeac (environment:
>> make `get_git_dir()` accept a repository, 2024-09-12) that retired
>> get_git_dir() and introduced repo_get_git_dir() was the culprit that
>> made their semantics change, but is that really true?  It appears
>> that in the version immediately before that commit, get_git_dir()
>> was also a reference to a variable, without any lazy initialization
>> the above message says that the code tries to avoid, so I am even
>> more confused after reading the above.
>
> I was reading the code in master and noticed that repo_get_git_dir()
> no longer sets up the environment. I’ve learned that I should use git blame
> to identify which commit changed the code, so I can make my message clearer.
>
>> Perhaps you have 73f192c9 (setup: don't perform lazy initialization
>> of repository state, 2017-06-20) in mind?  That one did stop calling
>> setup_git_env() and instead force a hard BUG("") when git_dir is not
>> set up yet.  And that BUG("") still survives in repo_get_git_dir()
>> we have today.
>
> Exactly
>
>> So the call to repo_get_git_dir() may still not be made from this
>> code path.  It may not attempt to set up, but instead it would die
>> if we haven't successfully set up the repository before.  The
>> relevance of the comment was not changed by 246deeac that moved this
>> code from get_git_dir() to repo_get_git_dir(), and more importantly,
>> it was not changed by this patch we are reviewing here.
>> 
>> But stepping back a bit, is it what a9ca8a85 originally wanted to
>> achieve with this comment to "avoid calling get_git_dir()" in the
>> first place?  Once the guarding condition is satisfied, it calls
>> trace_repo_setup(), which in turn calls get_git_dir() anyway.
>> Perhaps it wanted to explain why startup_info->have_repository is
>> checked here?
>
> Yes, I think so. Maybe updating the comment to say 
> “call repo_get_git_dir() after setting up the_repository”
> would be more appropriate.

I do not think so.

e5b17bda (git: ensure correct git directory setup with -h,
2021-12-06) unfortunately moved lines around and made it look like
the comment is about what happens when the if() condition holds, but
if we look at the way how a9ca8a85 (builtins: print setup info if
repo is found, 2010-11-26) initially placed this comment, we can see
that this comment was to only explain the reason why we look at
startup_info->have_repository there.  "Only if we know we have
repository, do the trace_repo_setup() thing because that one calls
get_git_dir() that would die otherwise" is what the comment wants to
say, and if we revert the moving-line-around done by e5b17bda to
recover the original layout in a9ca8a85, I think it is clear enough.

But the theme of this patch is not about improving the comment
anyway, so it should probably be done as a separate patch (if we
really cared, that is).

Thanks.

Re: [PATCH v2] git.c: remove the_repository dependence in run_builtin()

[Lidong Yan]

Junio C Hamano <gitster@pobox.com> writes：
> e5b17bda (git: ensure correct git directory setup with -h,
> 2021-12-06) unfortunately moved lines around and made it look like
> the comment is about what happens when the if() condition holds, but
> if we look at the way how a9ca8a85 (builtins: print setup info if
> repo is found, 2010-11-26) initially placed this comment, we can see
> that this comment was to only explain the reason why we look at
> startup_info->have_repository there.  "Only if we know we have
> repository, do the trace_repo_setup() thing because that one calls
> get_git_dir() that would die otherwise" is what the comment wants to
> say, and if we revert the moving-line-around done by e5b17bda to
> recover the original layout in a9ca8a85, I think it is clear enough.

I’ve changed my mind. Layout like

	if (run_setup &&
		startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */
		trace_repo_setup(repo);

Looks unbalanced, and git-clang-format tries to format this code
snippet into

	if (run_setup && startup_info->have_repository) /* get_git_dir() may set
							   up repo, avoid that
							 */

Which looks even worse, I will leave this comment intact.

Thanks,
Lidong

Re: [PATCH v2] git.c: remove the_repository dependence in run_builtin()

[Junio C Hamano]

Lidong Yan <502024330056@smail.nju.edu.cn> writes:

> Which looks even worse, I will leave this comment intact.

That is perfectly fine.  After all, this is not about comment style.
It is about the_repository.  Let's stay focused on that.

[RFC PATCH v3 0/2] small fixes for git.c and setup.c

[Lidong Yan]

I've been reading through the git code from the beginning. This
patch series fixes some NEEDSWORKs and cleans up some unnecessary
uses of the_repository that I came across.

The first commit replace the use of the_repository to run_builtin()'s
argument repo. Since each caller pass the_repository to run_builtin(),
this replacement is safe. This commit also rewrite a comment before
trace_repo_setup(), I am not sure my version of rewrite match the code
semantics. So I am reaching out for help and hoping to get some feedback
and discussion.

The second commit takes care of a NEEDSWORK in setup_git_directory_gently()
we now properly error out if we hit a .git that is not a file or directory
when looking for the .git.

Lidong Yan (2):
  git.c: remove the_repository dependence in run_builtin()
  setup: fix NEEDSWORK in setup_git_directory_gently()

 git.c      |  6 +++---
 setup.c    | 11 +++++++----
 setup.h    | 18 +++++++++++-------
 worktree.c |  4 ++--
 4 files changed, 23 insertions(+), 16 deletions(-)

-- 
2.50.0-rc1

[PATCH v3 1/2] git.c: remove the_repository dependence in run_builtin()

[Lidong Yan]

run_builtin() takes a repo parameter, so the use of the_repository
is no longer necessary. Removed the usage of the_repository.

The comment preceding trace_repo_setup() was originally introduced
in commit a9ca8a85. Since get_git_dir() modifies global variables
such as git_dir and git_objects_dir which only valid when inside a git
repository. The intention of the comment was to emphasize that
get_git_dir() should not be called before confirming that the current
directory is indeed part of a git repository. However, get_git_dir()
has been renamed to repo_get_git_dir() in commit 246deeac. And later
in commit 73f192c9, repo_get_git_dir() stoped calling setup_get_env()
anymore. Rewrite origin comment message.

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
 git.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/git.c b/git.c
index 77c4359522..7fa81dde18 100644
--- a/git.c
+++ b/git.c
@@ -462,12 +462,12 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 	precompose_argv_prefix(argc, argv, NULL);
 	if (use_pager == -1 && run_setup &&
 		!(p->option & DELAY_PAGER_CONFIG))
-		use_pager = check_pager_config(the_repository, p->cmd);
+		use_pager = check_pager_config(repo, p->cmd);
 	if (use_pager == -1 && p->option & USE_PAGER)
 		use_pager = 1;
 	if (run_setup && startup_info->have_repository)
-		/* get_git_dir() may set up repo, avoid that */
-		trace_repo_setup(the_repository);
+		/* avoid repo_get_git_dir(), repo must be set up */
+		trace_repo_setup(repo);
 	commit_pager_choice();
 
 	if (!help && p->option & NEED_WORK_TREE)
-- 
2.50.0-rc1

[PATCH v3 2/2] setup: fix NEEDSWORK in setup_git_directory_gently()

[Lidong Yan]

In setup.c:setup_git_directory_gently(), the case that fail when dir->buf
is neither a file nor a directory is currently marked as NEEDSWORK. Add
two new READ_GITFILE_ERR types to handle this case more explicitly:

  * READ_GITFILE_ERR_NOT_A_FILE_OR_DIR:
    path exist, but path is neither a file nor directory
  * READ_GITFILE_ERR_IS_DIR:
    path exist, and it is a directory.

If dir->buf is neither a file nor a directory, then depending on
die_or_error, either read_gitfile_gently() will die, or this
function will return GIT_DIR_INVALID_GIT_FILE.

To make old use of READ_GITFILE_ERR_NOT_A_FILE still works,
Add READ_GITFILE_ERR_NOT_A_FILE(err) macro, which has the same
effect as `err == READ_GITFILE_ERR_NOT_A_FILE` in the origin code.

Also add die message for READ_GITFILE_ERR_NOT_A_FILE_OR_DIR in
read_gitfile_error_die().

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
 setup.c    | 11 +++++++----
 setup.h    | 18 +++++++++++-------
 worktree.c |  4 ++--
 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/setup.c b/setup.c
index f93bd6a24a..b18c43d93b 100644
--- a/setup.c
+++ b/setup.c
@@ -893,9 +893,11 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
 {
 	switch (error_code) {
 	case READ_GITFILE_ERR_STAT_FAILED:
-	case READ_GITFILE_ERR_NOT_A_FILE:
+	case READ_GITFILE_ERR_IS_DIR:
 		/* non-fatal; follow return path */
 		break;
+	case READ_GITFILE_ERR_NOT_A_FILE_OR_DIR:
+		die(_("'%s' is not a file or directory"), path);
 	case READ_GITFILE_ERR_OPEN_FAILED:
 		die_errno(_("error opening '%s'"), path);
 	case READ_GITFILE_ERR_TOO_LARGE:
@@ -941,7 +943,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 		goto cleanup_return;
 	}
 	if (!S_ISREG(st.st_mode)) {
-		error_code = READ_GITFILE_ERR_NOT_A_FILE;
+		error_code = S_ISDIR(st.st_mode) ?
+				     READ_GITFILE_ERR_IS_DIR :
+				     READ_GITFILE_ERR_NOT_A_FILE_OR_DIR;
 		goto cleanup_return;
 	}
 	if (st.st_size > max_file_size) {
@@ -1499,8 +1503,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 						NULL : &error_code);
 		if (!gitdirenv) {
 			if (die_on_error ||
-			    error_code == READ_GITFILE_ERR_NOT_A_FILE) {
-				/* NEEDSWORK: fail if .git is not file nor dir */
+			    error_code == READ_GITFILE_ERR_IS_DIR) {
 				if (is_git_directory(dir->buf)) {
 					gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
 					gitdir_path = xstrdup(dir->buf);
diff --git a/setup.h b/setup.h
index 18dc3b7368..7f246ad248 100644
--- a/setup.h
+++ b/setup.h
@@ -29,13 +29,17 @@ int is_git_directory(const char *path);
 int is_nonbare_repository_dir(struct strbuf *path);
 
 #define READ_GITFILE_ERR_STAT_FAILED 1
-#define READ_GITFILE_ERR_NOT_A_FILE 2
-#define READ_GITFILE_ERR_OPEN_FAILED 3
-#define READ_GITFILE_ERR_READ_FAILED 4
-#define READ_GITFILE_ERR_INVALID_FORMAT 5
-#define READ_GITFILE_ERR_NO_PATH 6
-#define READ_GITFILE_ERR_NOT_A_REPO 7
-#define READ_GITFILE_ERR_TOO_LARGE 8
+#define READ_GITFILE_ERR_IS_DIR		   2
+#define READ_GITFILE_ERR_NOT_A_FILE_OR_DIR 3
+#define READ_GITFILE_ERR_OPEN_FAILED	   4
+#define READ_GITFILE_ERR_READ_FAILED	   5
+#define READ_GITFILE_ERR_INVALID_FORMAT	   6
+#define READ_GITFILE_ERR_NO_PATH	   7
+#define READ_GITFILE_ERR_NOT_A_REPO	   8
+#define READ_GITFILE_ERR_TOO_LARGE	   9
+#define READ_GITFILE_ERR_NOT_A_FILE(x)                \
+	((x) == READ_GITFILE_ERR_NOT_A_FILE_OR_DIR || \
+	 (x) == READ_GITFILE_ERR_IS_DIR)
 void read_gitfile_error_die(int error_code, const char *path, const char *dir);
 const char *read_gitfile_gently(const char *path, int *return_error_code);
 #define read_gitfile(path) read_gitfile_gently((path), NULL)
diff --git a/worktree.c b/worktree.c
index c34b9eb74e..3f0e9748ec 100644
--- a/worktree.c
+++ b/worktree.c
@@ -646,7 +646,7 @@ static void repair_gitfile(struct worktree *wt,
 		}
 	}
 
-	if (err == READ_GITFILE_ERR_NOT_A_FILE)
+	if (READ_GITFILE_ERR_NOT_A_FILE(err))
 		fn(1, wt->path, _(".git is not a file"), cb_data);
 	else if (err)
 		repair = _(".git file broken");
@@ -826,7 +826,7 @@ void repair_worktree_at_path(const char *path,
 			strbuf_addstr(&backlink, dotgit_contents);
 			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
 		}
-	} else if (err == READ_GITFILE_ERR_NOT_A_FILE) {
+	} else if (READ_GITFILE_ERR_NOT_A_FILE(err)) {
 		fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
 		goto done;
 	} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
-- 
2.50.0-rc1

Re: [RFC PATCH v3 0/2] small fixes for git.c and setup.c

[Junio C Hamano]

Lidong Yan <yldhome2d2@gmail.com> writes:

> I've been reading through the git code from the beginning. This
> patch series fixes some NEEDSWORKs and cleans up some unnecessary
> uses of the_repository that I came across.

FYI, when we have "NEEDSWORK: do X", the intention is often "we
haven't spent enough brain cycles when we wrote this comment, so the
first step is to evaluate if doing X is a sensible thing in the
first place, and only if that is the case, do X".

> The first commit replace the use of the_repository to run_builtin()'s
> argument repo. Since each caller pass the_repository to run_builtin(),
> this replacement is safe.

That change is safe.  I'd rather see the comment left intact or
reverted to the original shape to clarify what code the comment
applies to (see the other message).

> The second commit takes care of a NEEDSWORK in setup_git_directory_gently()
> we now properly error out if we hit a .git that is not a file or directory
> when looking for the .git.

We used to just ignore and keep going to check the parent directory,
right?  Now we would error out when .git is a FIFO or device or any
other random things.  Is a bit of behaviour change, but I am not
sure if it is worth doing.  As finding these weird non-file things
in your working tree and naming them ".git" is extremely rare and
useless (from Git's point of view), I suspect that the user is
deliberately doing so for whatever reason they have, so it smells
like this change has very little chance to detect a real problem
with a larger chance to break a set-up that was deliberately done by
the end-user.  I dunno.

Thanks.

Re: [RFC PATCH v3 0/2] small fixes for git.c and setup.c

[Lidong Yan]

Junio C Hamano <gitster@pobox.com> writes：
> 
> Lidong Yan <yldhome2d2@gmail.com> writes:
> 
>> I've been reading through the git code from the beginning. This
>> patch series fixes some NEEDSWORKs and cleans up some unnecessary
>> uses of the_repository that I came across.
> 
> FYI, when we have "NEEDSWORK: do X", the intention is often "we
> haven't spent enough brain cycles when we wrote this comment, so the
> first step is to evaluate if doing X is a sensible thing in the
> first place, and only if that is the case, do X".

Understood. I initially thought that NEEDSWORK indicated work that was
guaranteed to be implemented later.

> 
>> The first commit replace the use of the_repository to run_builtin()'s
>> argument repo. Since each caller pass the_repository to run_builtin(),
>> this replacement is safe.
> 
> That change is safe.  I'd rather see the comment left intact or
> reverted to the original shape to clarify what code the comment
> applies to (see the other message).

I’d like to revert the comment to original shape and split it into separate commit.
The reason I changed this comment was simply because I found it a bit confusing
when reading through the code.

> 
>> The second commit takes care of a NEEDSWORK in setup_git_directory_gently()
>> we now properly error out if we hit a .git that is not a file or directory
>> when looking for the .git.
> 
> We used to just ignore and keep going to check the parent directory,
> right?  Now we would error out when .git is a FIFO or device or any
> other random things.  Is a bit of behaviour change, but I am not
> sure if it is worth doing.  As finding these weird non-file things
> in your working tree and naming them ".git" is extremely rare and
> useless (from Git's point of view), I suspect that the user is
> deliberately doing so for whatever reason they have, so it smells
> like this change has very little chance to detect a real problem
> with a larger chance to break a set-up that was deliberately done by
> the end-user.  I dunno.
> 
> Thanks.

I would just discard the commit about NEEDSWORK.

Thanks for your review,
Lidong

[PATCH v4] git.c: remove the_repository dependence in run_builtin()

[Lidong Yan]

run_builtin() takes a repo parameter, so the use of the_repository
is no longer necessary. Removed the usage of the_repository.

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
 git.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git.c b/git.c
index 77c4359522..8525ede550 100644
--- a/git.c
+++ b/git.c
@@ -462,12 +462,12 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 	precompose_argv_prefix(argc, argv, NULL);
 	if (use_pager == -1 && run_setup &&
 		!(p->option & DELAY_PAGER_CONFIG))
-		use_pager = check_pager_config(the_repository, p->cmd);
+		use_pager = check_pager_config(repo, p->cmd);
 	if (use_pager == -1 && p->option & USE_PAGER)
 		use_pager = 1;
 	if (run_setup && startup_info->have_repository)
 		/* get_git_dir() may set up repo, avoid that */
-		trace_repo_setup(the_repository);
+		trace_repo_setup(repo);
 	commit_pager_choice();
 
 	if (!help && p->option & NEED_WORK_TREE)
-- 
2.50.0-rc1