"Nguyen Thai Ngoc Duy" [off-list ref] writes:
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.
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.
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
Hi,
On Mon, 3 Dec 2007, Nguyen Thai Ngoc Duy wrote:
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.
In the interest of least surprise, _gently() should not ignore a too new
repository format. It is also keeping the semantics simpler, which is
good for users like me.
So I'd test it with "git apply", just because it is the first on the list.
Something like
-- snip --
cat > patch << EOF
diff a/empty-file b/empty-file
--- a/empty-file
+++ b/empty-file
EOF
test_expect_success '_gently() respects repositoryversion' '
mkdir way_too_new &&
(cd way_too_new &&
git init &&
git config core.repositoryFormatVersion 999999 &&
: > empty-file &&
! git apply < ../patch 2> error.out &&
grep "git repo version" error.out &&
! test -s empty-file
'
-- snap --
Hmm?
Ciao,
Dscho
On Dec 2, 2007 1:58 AM, Junio C Hamano [off-list ref] wrote:
* 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.
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?
--
Duy