Re: [PATCH] Do check_repository_format() early

On Dec 2, 2007 1:58 AM, Junio C Hamano [off-list ref] wrote:

"Nguyen Thai Ngoc Duy" [off-list ref] writes:

quoted

On Dec 1, 2007 9:36 AM, Junio C Hamano [off-list ref] wrote:

quoted

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.
-- 
Duy