Re: [PATCH 1/2] clone: Fix error message for reference repository

From: Aaron Schrab <aaron@schrab.com>
Date: Apr 8, 2013, 00:06 UTC
Message-ID: <20130408000658.GG27178@pug.qqx.org>
In-Reply-To: <20130407234810.GG19857@elie.Belkin>

At 16:48 -0700 07 Apr 2013, Jonathan Nieder <jrnieder@gmail.com> wrote:

>Hi Aaron,

Thanks for the feedback.

Show 9 quoted lines

>Aaron Schrab wrote:
>
>> Do not report an argument to clone's --reference option is not a local
>> directory.  Nothing checks for the actual directory so we have no way to
>> know if whether or not exists.  Telling the user that a directory doesn't
>> exist when that isn't actually known may lead him or her on the wrong
>> path to finding the problem.
>
>I don't understand the above explanation.  Could you give an example?

I originally noticed this while trying to use a submodule as a reference repository. Since that submodule was first checked out using a recent version of git it used a .git file rather than having a .git directory. This caused the checks to fail, and the misleading error message had me checking for a typo in the path which I'd supplied.

I'll attempt to clarify that message in the next version.

Show 12 quoted lines

>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -241,7 +241,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
>>  		free(ref_git);
>>  		ref_git = ref_git_git;
>>  	} else if (!is_directory(mkpath("%s/objects", ref_git)))
>> -		die(_("reference repository '%s' is not a local directory."),
>> +		die(_("reference repository '%s' is not a local repository."),
>
>"is_directory" calls stat and checks if its target is a directory.  Is
>the problem that "/path/to/repo.git" might be a directory but
>"/path/to/repo.git/objects" may not?

In my case the issue was that /path/to/repo is a directory, but /path/to/repo/.git/objects (which is checked shortly before the above context) didn't exist since /path/to/repo/.git is a file.

>Would it make sense for the message to say something like the
>following?
>
>	fatal: alternate object store '/path/to/repo.git/objects' is not a local directory

That would also avoid lying to the user. But if combined with the second patch in this series it could cause confusion for a different reason. Once .git files are honored, the path reported there may have no relation to the path supplied by the user.