"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.
IOW, the repository we are working against is t/trash/test/.git in
this case, and not t/trash/.git. We do not actually touch the
repository because we do not know the repository format version of
it, but we do not barf when doing operations that we do not have to
touch it. And we never touch t/trash/.git. We need to make sure
of these, which means that it is not enough to make sure
non-repository operations do not barf in t/trash/test. We also need
to make sure the reason they do not barf is not because we ignored
that repository with unknown version and went one level up and
found a repository with a known version. The reason for success
must be because we correctly ignored the version mismatch because
we knew the operations do not affect the repository.
[Footnotes]
*1* Tangent. "git grep etc/gitconfig" reveals a few instances of
$(prefix)/etc/gitconfig left behind, which was corrected in
v1.5.1.3. We need documentation updates.
*2* However, "we do not have to be in git repository" mode of operations
by definition do not touch any repository data (only work tree
files), so I do not think it is justfied to barf in such a case.
-
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
