y@xxxxxxx writes:
> 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?
> diff --git a/sha1_name.c b/sha1_name.c
> index 44bb62d..030e2ac 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -804,7 +804,77 @@ int get_sha1(const char *name, unsigned char *sha1)
> return get_sha1_with_mode(name, sha1, &unused);
> }
>
> -int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
> +static void diagnose_invalid_sha1_path(const char *prefix,
> + const char *filename,
> + const char *tree_sha1,
> + const char *object_name)
> +{
> + struct stat st;
> + unsigned char sha1[20];
> + unsigned mode;
> +
> + if (!prefix)
> + prefix = "";
> +
> + if (!lstat(filename, &st))
> + die("Path '%s' exists on disk, but not in '%s'.",
> + filename, object_name);
> + 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)?
> + if (!get_tree_entry(tree_sha1, fullname,
> + sha1, &mode)) {
> + die("Path '%s' exists, but not '%s'.\n"
> + "Did you mean '%s:%s'?",
> + fullname,
> + filename,
> + object_name,
> + fullname);
> + }
> + die("Path '%s' does not exist in '%s'",
> + filename, object_name);
> + }
> +}
> +
> +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);
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?
> @@ -850,6 +920,8 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
> }
> pos++;
> }
> + if (fatal)
> + diagnose_invalid_index_path(stage, prefix, cp);
> return -1;
> }
> for (cp = name, bracket_depth = 0; *cp; cp++) {
> @@ -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?
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?
> + strncpy(object_name, name, cp-name);
> + object_name[cp-name] = '\0';
> + }
> + if (!get_sha1_1(name, cp-name, tree_sha1)) {
> + const char *filename = cp+1;
> + ret = get_tree_entry(tree_sha1, filename, sha1, mode);
> + if (fatal)
> + diagnose_invalid_sha1_path(prefix, filename,
> + tree_sha1, object_name);
> +
> + return ret;
> + } else {
> + if (fatal)
> + die("Invalid object name '%s'.", object_name);
> + }
> }
> return ret;
> }
> diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
> new file mode 100755
> index 0000000..8112d56
> --- /dev/null
> +++ b/t/t1506-rev-parse-diagnosis.sh
> @@ -0,0 +1,67 @@
> +#!/bin/sh
> +
> +test_description='test git rev-parse diagnosis for invalid argument'
> +
> +exec </dev/null
> +
> +. ./test-lib.sh
> +
> +HASH_file=
> +
> +test_expect_success 'set up basic repo' '
> + echo one > file.txt &&
> + mkdir subdir &&
> + echo two > subdir/file.txt &&
> + echo three > subdir/file2.txt &&
> + git add . &&
> + git commit -m init &&
> + echo four > index-only.txt &&
> + git add index-only.txt &&
> + echo five > disk-only.txt
> +'
> +
> +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 (). If 'rev-parse HEAD:file.txt' fails after "cd subdir", the next
test will start running from that directory.
> +test_expect_success 'incorrect revision id' '
> + 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.
If you expect an error exit and want to make sure a particular error
message is given, do this:
test_must_fail git rev-parse foobar:file.txt 2>error &&
grep "Invalid ..." error
If you expect an error exit and want to make sure an incorrect error
message is not produced, do this:
test_must_fail git rev-parse foobar:file.txt 2>error &&
! grep "Invalid ..." error
--
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