On Dec 2, 2007 1:58 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> "Nguyen Thai Ngoc Duy" <pclouds@xxxxxxxxx> writes:
>
> > On Dec 1, 2007 9:36 AM, Junio C Hamano <gitster@xxxxxxxxx> 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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
