From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Sun, 06 Dec 2009 18:28:08 GMT
Subject: Re: [PATCH v2] Detailed diagnosis when parsing an object name fails.
Message-ID: <vpqhbs4dkjr.fsf@bauges.imag.fr>
In-Reply-To: <7vljhj4wv0.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> y@imag.fr writes:

(sorry for the bad email, my fingers tried to anwser "yes" when
git-send-email asked me to confirm my address)

>> diff --git a/cache.h b/cache.h
>> index 0e69384..5c8cb5f 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -708,7 +708,11 @@ static inline unsigned int hexval(unsigned char c)
>>  #define DEFAULT_ABBREV 7
>>  
>>  extern int get_sha1(const char *str, unsigned char *sha1);
>> -extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);
>> +static inline get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
>> +{
>> +	return get_sha1_with_mode_1(str, sha1, mode, 0, NULL);
>> +}
>> +extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int fatal, const char *prefix);
>
> Do I understand correctly that "fatal" here is the same as "!gently"
> elsewhere in the API?

It seems it is. I renamed it.

>> +	if (errno == ENOENT || errno == ENOTDIR) {
>> +		char *fullname = malloc(strlen(filename)
>> +					     + strlen(prefix) + 1);
>> +		strcpy(fullname, prefix);
>> +		strcat(fullname, filename);
>
> What if malloc fails here (and elsewhere in your patch)?

Should have been an xmalloc. Fixed.

>> +static void diagnose_invalid_index_path(int stage,
>> +					const char *prefix,
>> +					const char *filename)
>> +{
>> +	struct stat st;
>> +
>> +	if (!prefix)
>> +		prefix = "";
>> +
>> +	if (!lstat(filename, &st))
>> +		die("Path '%s' exists on disk, but not in the index.", filename);
>> +	if (errno == ENOENT || errno == ENOTDIR) {
>> +		struct cache_entry *ce;
>> +		int pos;
>> +		int namelen = strlen(filename) + strlen(prefix);
>> +		char *fullname = malloc(namelen + 1);
>> +		strcpy(fullname, prefix);
>> +		strcat(fullname, filename);
>> +		pos = cache_name_pos(fullname, namelen);
>> +		if (pos < 0)
>> +			pos = -pos - 1;
>> +		ce = active_cache[pos];
>> +		if (ce_namelen(ce) == namelen &&
>> +		    !memcmp(ce->name, fullname, namelen))
>> +			die("Path '%s' is in the index, but not '%s'.\n"
>> +			    "Did you mean ':%d:%s'?",
>> +			    fullname, filename,
>> +			    stage, fullname);
                            ^^^^^
This one should have been ce_stage(ce), otherwise, we
suggest :0:t/Makefile as you probably guessed:

> What happens if the user asked for ":2:Makefile" while in directory "t/",
> and there is ":1:t/Makefile" but not ":2:t/Makefile" in the index?
>
> What should happen if the user asked for ":2:t/Makefile" in such a
> case?

I originally didn't handle the case "it's in the index, but not at the
stage you requested" (since most users asking :N:blah will be fluent
enough in Git not to need a special case of error messages). But I
finally implemented this test.

>> @@ -862,9 +934,24 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
>>  	}
>>  	if (*cp == ':') {
>>  		unsigned char tree_sha1[20];
>> -		if (!get_sha1_1(name, cp-name, tree_sha1))
>> -			return get_tree_entry(tree_sha1, cp+1, sha1,
>> -					      mode);
>> +		char *object_name;
>> +		if (fatal) {
>> +			object_name = malloc(cp-name+1);
>
> Where is this freed?

Nowhere. In the present state, it's not serious since all codepath
going through this malloc reach a die() right after. But a free()
doesn't harm, I've added it.

> Instead of doing a leaky allocation, it may make sense to pass the tree
> object name as <const char *, size_t> pair, and print it with "%.*s" in
> the error reporting codepath.  After all, object_name is used only for
> that purpose in diagnose_invalid_sha1_path(), no?

matter of taste, but I prefer doing one "complicated" thing (copying
the string) once, and avoid having to deal with two variables instead
of one later.

>> +test_expect_success 'correct file objects' '
>> +	HASH_file=$(git rev-parse HEAD:file.txt) &&
>> +	git rev-parse HEAD:subdir/file.txt &&
>> +	git rev-parse :index-only.txt &&
>> +	cd subdir &&
>> +	git rev-parse HEAD:file.txt &&
>> +	git rev-parse HEAD:subdir/file2.txt &&
>> +	test $HASH_file = $(git rev-parse HEAD:file.txt) &&
>> +	test $HASH_file = $(git rev-parse :file.txt) &&
>> +	test $HASH_file = $(git rev-parse :0:file.txt) &&
>> +	cd ..
>> +'
>
> Please make it a habit of not doing "cd" without forcing a subprocess
> using ().

Fixed.

>> +	test_must_fail git rev-parse foobar:file.txt 2>&1 |
>> +		grep "Invalid object name '"'"'foobar'"'"'." &&
>
> It always is better to write this in separate steps, because exit status
> of the upstream of pipe is discarded by the shell.

Fixed.

I'll resubmit soon.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/