Re: [PATCH] Do check_repository_format() early

["Nguyen Thai Ngoc Duy" <pclouds@gmail.com>]

On Dec 2, 2007 1:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:
>
> > On Dec 1, 2007 9:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> 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