Do check_repository_format() early

Hi,

Quoted 4 lines · Nguyen Thai Ngoc Duy

setup.c +0 -0

@@ -246,8 +246,13 @@ const char *setup_git_directory_gently(int *nongit_ok)
                      static char buffer[1024 + 1];
                      const char *retval;
 

setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 255

Quoted 9 lines

setup.c +7 -2

-                     if (!work_tree_env)
-                             return set_work_tree(gitdirenv);
+                     if (!work_tree_env) {
+                             retval = set_work_tree(gitdirenv);
+                             /* config may override worktree */
+                             check_repository_format();
+                             return retval;
+                     }
+                     check_repository_format();

Why not move this check before the if? Other than that, ACK.

Ciao,
Dscho

Quoted 7 lines · Johannes Schindelin

Quoted 4 lines · Nguyen Thai Ngoc Duy

setup.c +0 -0

@@ -246,8 +246,13 @@ const char *setup_git_directory_gently(int *nongit_ok)
                      static char buffer[1024 + 1];
                      const char *retval;
 

Quoted 11 lines

setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 255

Quoted 9 lines

setup.c +7 -2

-                     if (!work_tree_env)
-                             return set_work_tree(gitdirenv);
+                     if (!work_tree_env) {
+                             retval = set_work_tree(gitdirenv);
+                             /* config may override worktree */
+                             check_repository_format();
+                             return retval;
+                     }
+                     check_repository_format();

Why not move this check before the if? Other than that, ACK.

If so it would be called twice if work_tree_env is not set.

Hi,

Quoted 21 lines · Nguyen Thai Ngoc Duy

Quoted 7 lines · Johannes Schindelin

Quoted 4 lines · Nguyen Thai Ngoc Duy

setup.c +0 -0

@@ -246,8 +246,13 @@ const char *setup_git_directory_gently(int *nongit_ok)
                      static char buffer[1024 + 1];
                      const char *retval;
 

Quoted 11 lines

setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 255

Quoted 9 lines

setup.c +7 -2

-                     if (!work_tree_env)
-                             return set_work_tree(gitdirenv);
+                     if (!work_tree_env) {
+                             retval = set_work_tree(gitdirenv);
+                             /* config may override worktree */
+                             check_repository_format();
+                             return retval;
+                     }
+                     check_repository_format();

Why not move this check before the if? Other than that, ACK.

Quoted 1 line

If so it would be called twice if work_tree_env is not set.

Well, I would have left the original

                        if (!work_tree_env)
                                return set_work_tree(gitdirenv);

alone...

If that is not possible, it might be good to add a comment as to why.

Ciao,
Dscho

setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 254

Quoted 9 lines · Nguyen Thai Ngoc Duy

The comment is clearly not clear enough. Maybe this?

setup.c +3 -0

+                     if (!work_tree_env) {
+                             retval = set_work_tree(gitdirenv);
+                             /* config may override worktree (see

set_work_tree comment) */

setup.c +3 -0

+                             check_repository_format();
+                             return retval;
+                     }

Perfect. Please make it so, and add my ACK.

Quoted 12 lines · Johannes Schindelin

Quoted 9 lines · Nguyen Thai Ngoc Duy

The comment is clearly not clear enough. Maybe this?

(quoted diff) +3 -0

+                     if (!work_tree_env) {
+                             retval = set_work_tree(gitdirenv);
+                             /* config may override worktree (see

set_work_tree comment) */

(quoted diff) +3 -0

+                             check_repository_format();
+                             return retval;
+                     }

Quoted 1 line

Perfect. Please make it so, and add my ACK.

Looks sensible, but can this be accompanied with a trivial test to demonstrate the existing breakage?

Quoted 3 lines · Junio C Hamano

Looks sensible, but can this be accompanied with a trivial test to demonstrate the existing breakage?

How can I reliably check setup_git_directory_gently()? I can pick one command that uses setup_git_directory_gently(). But commands change. Once they turn to setup_git_directory(), the test will no longer be valid.

Quoted 9 lines · Nguyen Thai Ngoc Duy

Quoted 3 lines · Junio C Hamano

Looks sensible, but can this be accompanied with a trivial test to demonstrate the existing breakage?

How can I reliably check setup_git_directory_gently()? I can pick one command that uses setup_git_directory_gently(). But commands change. Once they turn to setup_git_directory(), the test will no longer be valid.

The commands' implementation may change but what I meant was to test the intent.

What's the difference between commands that call "gently" kind and non-gently kind? The former is "I do not _have_ to be in a git repository but if I am then I want to know about it and use some information from that repository", as opposed to "I need to be in a git repository and will find one otherwise I barf" which is the latter kind.

The intent of the change, from reading your patch, is that currently the former kind of commands that take "an optional git repository" are happy if they find a directory that looks like a git repository and go ahead with their operation without checking the repository format version, and your patch addresses this issue by making sure that the git repository found via "gently" are also checked for the format version.

Examples of commands that do not necessarily require a valid git repository are:

* apply: when being used as a "patch that is better than GNU", that is, without --index, --cached, nor --check option.

* bundle: when verifying and listing the contained head of an existing bundle file.

* config: without being in a git repository, you can still interact with $HOME/.gitconfig and /etc/gitconfig [*1*].

* ls-remote: without being in a git repository, you can still list refs from a remote repository. If you are in a git repository, you can use nicknames you have in your repositories' remote.$nickname.url configuration.

So what I would suggest would be:

* The directory your tests run in, t/trash, is a valid git repository. Leave it as is.

* mkdir test inside t/trash, cd there, and run "git init" there to initialize t/trash/test/.git (the shell function test_create_repo can be used for this).

* corrupt this by updating the core.repositoryformatversion to a large value, by doing something like:

        V=$(git config core.repositoryformatversion)
        (
                cd test
                N=$(( ${V:-0} + 99 ))
                git config core.repositoryformatversion $N
        )

* make sure t/trash/test/.git/config file, and not t/trash/.git/config file, got that change by doing something like:

GIT_CONFIG=.git/config git config core.repositoryformatversion GIT_CONFIG=test/.git/config git config core.repositoryformatversion

The former would report the current version ($V above) while the latter should error out.

Up to this step is the "test setup". The actual tests would be done in t/trash/test directory.

* Use a few commands that have the "we can run in git repository but we do not have to" behaviour, in modes that _require_ git repository. For example, "git apply --check" wants a valid repository to check the patch against the index. They should fail because the repository format is too new for them to understand.

* Similarly, run a few commands in modes that do not require git repository. For example, "git apply --stat" of an existing patch should be viewable no matter where you are (that is just a "better diffstat" mode), so ideally it should not barf only because you happen to be in a repository that is too new for you to understand. I do not know offhand how your patch would handle this situation.

Note that making sure the latter works is tricky to do right, for a few reasons.

(1) It is not absolutely clear what the right behaviour is. It could be argued that we should just barf saying we found a repository we do not understand, refraining from doing any damange on it [*2*].

(2) If we choose not to barf on such a repository, it remains to be decided what "gently" should do --- if it should still treat t/trash/test (which has too new a version) as the found repository, or ignore it and use t/trash (which we can understand) as the found repository. I think it should do the former.

The patch's behaviour is barf if the repository version is too new. The list of files that use setup_git_directory_gently is not long. I am going to have a look over the files before amending the patch again to make it only barf if nongit_ok is NULL.

* Similarly, run a few commands in modes that do not require git repository. For example, "git apply --stat" of an existing patch should be viewable no matter where you are (that is just a "better diffstat" mode), so ideally it should not barf only because you happen to be in a repository that is too new for you to understand. I do not know offhand how your patch would handle this situation.

Note that making sure the latter works is tricky to do right, for a few reasons.

(1) It is not absolutely clear what the right behaviour is. It could be argued that we should just barf saying we found a repository we do not understand, refraining from doing any damange on it [*2*].

(2) If we choose not to barf on such a repository, it remains to be decided what "gently" should do --- if it should still treat t/trash/test (which has too new a version) as the found repository, or ignore it and use t/trash (which we can understand) as the found repository. I think it should do the former.

Quoted 14 lines · Junio C Hamano

* Similarly, run a few commands in modes that do not require git repository. For example, "git apply --stat" of an existing patch should be viewable no matter where you are (that is just a "better diffstat" mode), so ideally it should not barf only because you happen to be in a repository that is too new for you to understand. I do not know offhand how your patch would handle this situation.

Note that making sure the latter works is tricky to do right, for a few reasons.

(1) It is not absolutely clear what the right behaviour is. It could be argued that we should just barf saying we found a repository we do not understand, refraining from doing any damange on it [*2*].

Quoted 5 lines

(2) If we choose not to barf on such a repository, it remains to be decided what "gently" should do --- if it should still treat t/trash/test (which has too new a version) as the found repository, or ignore it and use t/trash (which we can understand) as the found repository. I think it should do the former.

You might have forgotten the third choice: ignore t/trash/test and stop searching, instead pretend there is no repository at all (maybe with a big warning of unsupported repository).

I agree t/trash should not be touched no matter what. I had enough "fun" with nested gitdir already. But if _gently() treats t/trash/test as a good repository, mysterious things may happen. Suppose gitdir v2 supports some crazy refspec that current installed git cannot understand. Now you run git-remote on a v2 repository, it would end up barfing "invalid refspec" or something instead of "your repository version is not supported, upgrade git now". The latter error message is much clearer IMHO.

If we are going "t/trash/test is good repo" route, we must make sure _gently() callers check repository version (and barf at proper places) before actually using it. Doing so makes repo version checking in _gently redundant, you need to check it from callers anyway as the callers will decide when to barf. Or return *nongit_ok=-1 and let the callers check return value so they do not need to run check_repository_format_version() again.

Comments?