[PATCH] Do check_repository_format() early

[PATCH] Do check_repository_format() early

[Nguyễn Thái Ngọc Duy]

Repository version check is only performed when
setup_git_directory() is called. This makes sure
setup_git_directory_gently() does the check too.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 My worktree work still not done yet, so push this first.

 setup.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/setup.c b/setup.c
index faf4137..19a8a77 100644
--- a/setup.c
+++ b/setup.c
@@ -246,8 +246,13 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			static char buffer[1024 + 1];
 			const char *retval;
 
-			if (!work_tree_env)
-				return set_work_tree(gitdirenv);
+			if (!work_tree_env) {
+				retval = set_work_tree(gitdirenv);
+				/* config may override worktree */
+				check_repository_format();
+				return retval;
+			}
+			check_repository_format();
 			retval = get_relative_cwd(buffer, sizeof(buffer) - 1,
 					get_git_work_tree());
 			if (!retval || !*retval)
@@ -287,6 +292,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			if (!work_tree_env)
 				inside_work_tree = 0;
 			setenv(GIT_DIR_ENVIRONMENT, ".", 1);
+			check_repository_format();
 			return NULL;
 		}
 		chdir("..");
@@ -307,6 +313,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	if (!work_tree_env)
 		inside_work_tree = 1;
 	git_work_tree_cfg = xstrndup(cwd, offset);
+	check_repository_format();
 	if (offset == len)
 		return NULL;
 
@@ -367,7 +374,6 @@ int check_repository_format(void)
 const char *setup_git_directory(void)
 {
 	const char *retval = setup_git_directory_gently(NULL);
-	check_repository_format();
 
 	/* If the work tree is not the default one, recompute prefix */
 	if (inside_work_tree < 0) {
-- 
1.5.3.6.2041.g106f-dirty

Re: [PATCH] Do check_repository_format() early

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 555 bytes --]

Hi,

On Wed, 28 Nov 2007, Nguyễn Thái Ngọc Duy wrote:

> @@ -246,8 +246,13 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  			static char buffer[1024 + 1];
>  			const char *retval;
>  
> -			if (!work_tree_env)
> -				return set_work_tree(gitdirenv);
> +			if (!work_tree_env) {
> +				retval = set_work_tree(gitdirenv);
> +				/* config may override worktree */
> +				check_repository_format();
> +				return retval;
> +			}
> +			check_repository_format();

Why not move this check before the if?  Other than that, ACK.

Ciao,
Dscho

Re: [PATCH] Do check_repository_format() early

[Nguyen Thai Ngoc Duy]

On Nov 29, 2007 12:05 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Wed, 28 Nov 2007, Nguyen Thai Ngoc Duy wrote:
>
> > @@ -246,8 +246,13 @@ const char *setup_git_directory_gently(int *nongit_ok)
> >                       static char buffer[1024 + 1];
> >                       const char *retval;
> >
> > -                     if (!work_tree_env)
> > -                             return set_work_tree(gitdirenv);
> > +                     if (!work_tree_env) {
> > +                             retval = set_work_tree(gitdirenv);
> > +                             /* config may override worktree */
> > +                             check_repository_format();
> > +                             return retval;
> > +                     }
> > +                     check_repository_format();
>
> Why not move this check before the if?  Other than that, ACK.

If so it would be called twice if work_tree_env is not set.

> Ciao,
> Dscho
>



-- 
Duy

Re: [PATCH] Do check_repository_format() early

[Johannes Schindelin]

Hi,

On Thu, 29 Nov 2007, Nguyen Thai Ngoc Duy wrote:

> On Nov 29, 2007 12:05 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Wed, 28 Nov 2007, Nguyen Thai Ngoc Duy wrote:
> >
> > > @@ -246,8 +246,13 @@ const char *setup_git_directory_gently(int *nongit_ok)
> > >                       static char buffer[1024 + 1];
> > >                       const char *retval;
> > >
> > > -                     if (!work_tree_env)
> > > -                             return set_work_tree(gitdirenv);
> > > +                     if (!work_tree_env) {
> > > +                             retval = set_work_tree(gitdirenv);
> > > +                             /* config may override worktree */
> > > +                             check_repository_format();
> > > +                             return retval;
> > > +                     }
> > > +                     check_repository_format();
> >
> > Why not move this check before the if?  Other than that, ACK.
> 
> If so it would be called twice if work_tree_env is not set.

Well, I would have left the original

			if (!work_tree_env)
				return set_work_tree(gitdirenv);

alone...

If that is not possible, it might be good to add a comment as to why.

Ciao,
Dscho

Re: [PATCH] Do check_repository_format() early

[Nguyen Thai Ngoc Duy]

On Nov 29, 2007 12:18 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Thu, 29 Nov 2007, Nguyen Thai Ngoc Duy wrote:
>
> > On Nov 29, 2007 12:05 AM, Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > >
> > > On Wed, 28 Nov 2007, Nguyen Thai Ngoc Duy wrote:
> > >
> > > > @@ -246,8 +246,13 @@ const char *setup_git_directory_gently(int *nongit_ok)
> > > >                       static char buffer[1024 + 1];
> > > >                       const char *retval;
> > > >
> > > > -                     if (!work_tree_env)
> > > > -                             return set_work_tree(gitdirenv);
> > > > +                     if (!work_tree_env) {
> > > > +                             retval = set_work_tree(gitdirenv);
> > > > +                             /* config may override worktree */
> > > > +                             check_repository_format();
> > > > +                             return retval;
> > > > +                     }
> > > > +                     check_repository_format();
> > >
> > > Why not move this check before the if?  Other than that, ACK.
> >
> > If so it would be called twice if work_tree_env is not set.
>
> Well, I would have left the original
>
>                         if (!work_tree_env)
>                                 return set_work_tree(gitdirenv);
>
> alone...
>
> If that is not possible, it might be good to add a comment as to why.

I did, and the tests failed. I also added a comment "config may
override worktree". set_work_tree() will reset git_work_tree_cfg but
the correct behaviour is config takes precedence (from comment of
set_work_tree). The comment is clearly not clear enough. Maybe this?

+                     if (!work_tree_env) {
+                             retval = set_work_tree(gitdirenv);
+                             /* config may override worktree (see
set_work_tree comment) */
+                             check_repository_format();
+                             return retval;
+                     }

> Ciao,
> Dscho
>
>



-- 
Duy

Re: [PATCH] Do check_repository_format() early

[Johannes Schindelin]

Hi,

On Thu, 29 Nov 2007, Nguyen Thai Ngoc Duy wrote:

> The comment is clearly not clear enough. Maybe this?
> 
> +                     if (!work_tree_env) {
> +                             retval = set_work_tree(gitdirenv);
> +                             /* config may override worktree (see
> set_work_tree comment) */
> +                             check_repository_format();
> +                             return retval;
> +                     }

Perfect.  Please make it so, and add my ACK.

Thanks,
Dscho

Re: [PATCH] Do check_repository_format() early

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Thu, 29 Nov 2007, Nguyen Thai Ngoc Duy wrote:
>
>> The comment is clearly not clear enough. Maybe this?
>> 
>> +                     if (!work_tree_env) {
>> +                             retval = set_work_tree(gitdirenv);
>> +                             /* config may override worktree (see
>> set_work_tree comment) */
>> +                             check_repository_format();
>> +                             return retval;
>> +                     }
>
> Perfect.  Please make it so, and add my ACK.

Looks sensible, but can this be accompanied with a trivial test to
demonstrate the existing breakage?

Re: [PATCH] Do check_repository_format() early

[Nguyen Thai Ngoc Duy]

On Dec 1, 2007 9:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Thu, 29 Nov 2007, Nguyen Thai Ngoc Duy wrote:
> >
> >> The comment is clearly not clear enough. Maybe this?
> >>
> >> +                     if (!work_tree_env) {
> >> +                             retval = set_work_tree(gitdirenv);
> >> +                             /* config may override worktree (see
> >> set_work_tree comment) */
> >> +                             check_repository_format();
> >> +                             return retval;
> >> +                     }
> >
> > Perfect.  Please make it so, and add my ACK.
>
> 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.

-- 
Duy

Re: [PATCH] Do check_repository_format() early

[Junio C Hamano]

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

     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.

Re: [PATCH] Do check_repository_format() early

[Nguyen Thai Ngoc Duy]

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

Re: [PATCH] Do check_repository_format() early

[Johannes Schindelin]

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
@@ -0,0 +1,1 @@
+narf
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

Re: [PATCH] Do check_repository_format() early

[Nguyen Thai Ngoc Duy]

On Dec 2, 2007 1:58 AM, Junio C Hamano <gitster@pobox.com> 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

Re: [PATCH] Do check_repository_format() early

[Junio C Hamano]

"Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:

> On Dec 2, 2007 1:58 AM, Junio C Hamano <gitster@pobox.com> 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?

I think I phrased the above (2) not brilliantly.  I meant your "third
choice" is the sane approach.  Treat t/trash/test as a found place that
we do not understand, perhaps issue a warning saying that we will
operate in there but the repository version is too new for us to
understand, but still go ahead and operate in there without doing any
repository operation (so the plain git-apply without --index will act as
if it is a GNU patch called to modify files in that directory).

[PATCH] Do check_repository_format() early

[Nguyễn Thái Ngọc Duy]

This pushes check_repository_format() (actually _gently() version)
to setup_git_directory_gently() in order to prevent from
using unsupported repositories.

New setup_git_directory_gently()'s behaviour is stop searching
for a valid gitdir and return as if there is no gitdir if a
unsupported repository is found. Warning will be thrown in these
cases.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Another round. Test added. Behaviour changed to make it tolerate
 unsupported repos as much as possible.

 setup.c                 |   37 ++++++++++++++++++++++++++++++-------
 t/t1302-repo-version.sh |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 7 deletions(-)
 create mode 100755 t/t1302-repo-version.sh

diff --git a/setup.c b/setup.c
index faf4137..434a7a6 100644
--- a/setup.c
+++ b/setup.c
@@ -222,6 +222,22 @@ void setup_work_tree(void)
 	initialized = 1;
 }
 
+static int check_repository_format_gently(int *nongit_ok)
+{
+	git_config(check_repository_format_version);
+	if (GIT_REPO_VERSION < repository_format_version) {
+		if (!nongit_ok)
+			die ("Expected git repo version <= %d, found %d",
+			     GIT_REPO_VERSION, repository_format_version);
+		warning("Expected git repo version <= %d, found %d",
+			GIT_REPO_VERSION, repository_format_version);
+		warning("Please upgrade Git");
+		*nongit_ok = -1;
+		return -1;
+	}
+	return 0;
+}
+
 /*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.
@@ -246,8 +262,15 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			static char buffer[1024 + 1];
 			const char *retval;
 
-			if (!work_tree_env)
-				return set_work_tree(gitdirenv);
+			if (!work_tree_env) {
+				retval = set_work_tree(gitdirenv);
+				/* config may override worktree */
+				if (check_repository_format_gently(nongit_ok))
+					return NULL;
+				return retval;
+			}
+			if (check_repository_format_gently(nongit_ok))
+				return NULL;
 			retval = get_relative_cwd(buffer, sizeof(buffer) - 1,
 					get_git_work_tree());
 			if (!retval || !*retval)
@@ -287,6 +310,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			if (!work_tree_env)
 				inside_work_tree = 0;
 			setenv(GIT_DIR_ENVIRONMENT, ".", 1);
+			if (check_repository_format_gently(nongit_ok))
+				return NULL;
 			return NULL;
 		}
 		chdir("..");
@@ -307,6 +332,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	if (!work_tree_env)
 		inside_work_tree = 1;
 	git_work_tree_cfg = xstrndup(cwd, offset);
+	if (check_repository_format_gently(nongit_ok))
+		return NULL;
 	if (offset == len)
 		return NULL;
 
@@ -357,11 +384,7 @@ int check_repository_format_version(const char *var, const char *value)
 
 int check_repository_format(void)
 {
-	git_config(check_repository_format_version);
-	if (GIT_REPO_VERSION < repository_format_version)
-		die ("Expected git repo version <= %d, found %d",
-		     GIT_REPO_VERSION, repository_format_version);
-	return 0;
+	return check_repository_format_gently(NULL);
 }
 
 const char *setup_git_directory(void)
diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
new file mode 100755
index 0000000..37fc1c8
--- /dev/null
+++ b/t/t1302-repo-version.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Nguyễn Thái Ngọc Duy
+#
+
+test_description='Test repository version check'
+
+. ./test-lib.sh
+
+cat >test.patch <<EOF
+diff --git a/test.txt b/test.txt
+new file mode 100644
+--- /dev/null
++++ b/test.txt
+@@ -0,0 +1 @@
++123
+EOF
+
+test_create_repo "test"
+test_create_repo "test2"
+
+GIT_CONFIG=test2/.git/config git config core.repositoryformatversion 99 || exit 1
+
+test_expect_success 'gitdir selection on normal repos' '
+	(test "$(git config core.repositoryformatversion)" = 0 &&
+	cd test &&
+	test "$(git config core.repositoryformatversion)" = 0)'
+
+# Make sure it would stop at test2, not trash
+test_expect_success 'gitdir selection on unsupported repo' '
+	(cd test2 &&
+	test "$(git config core.repositoryformatversion)" = 99)'
+
+test_expect_success 'gitdir not required mode' '
+	(git apply --stat test.patch &&
+	cd test && git apply --stat ../test.patch &&
+	cd ../test2 && git apply --stat ../test.patch)'
+
+test_expect_success 'gitdir required mode on normal repos' '
+	(git apply --check --index test.patch &&
+	cd test && git apply --check --index ../test.patch)'
+
+test_expect_failure 'gitdir required mode on unsupported repo' '
+	(cd test2 && git apply --check --index ../test.patch)'
+
+test_done
-- 
1.5.3.6.2040.gcdff-dirty

Re: [PATCH] Do check_repository_format() early

[Nguyen Thai Ngoc Duy]

On Dec 5, 2007 8:33 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> @@ -287,6 +310,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
>                         if (!work_tree_env)
>                                 inside_work_tree = 0;
>                         setenv(GIT_DIR_ENVIRONMENT, ".", 1);
> +                       if (check_repository_format_gently(nongit_ok))
> +                               return NULL;
>                         return NULL;
>                 }
>                 chdir("..");

This part better be as follow (patch may be damaged as I'm editing it in gmail)

 @@ -287,6 +310,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
                         if (!work_tree_env)
                                 inside_work_tree = 0;
                         setenv(GIT_DIR_ENVIRONMENT, ".", 1);
 +                       check_repository_format_gently(nongit_ok);
                         return NULL;
                 }
                 chdir("..");
-- 
Duy

Re: [PATCH] Do check_repository_format() early

[Junio C Hamano]

Thanks, this looks very sensible to me.