RFD: should git rev-parse exit with non-zero status if ref@{n} is not valid?

RFD: should git rev-parse exit with non-zero status if ref@{n} is not valid?

[Jon Seymour]

While reworking my detached-stash topic, I noticed that git rev-parse
ref@{n} will exit with a warning message on stderr, the SHA1 of ref
and a non-zero exit code if ref exists, but ref@{n} does not.

I understand why ref@{time-spec} might behave this way, but I reckon
that if you ask for ref@{n} for n > N-1, where N is the length of the
reflog, then you should either get empty output and a non-zero status
(preferred) or a ref@{N-1} on the output with a status code of zero
and a warning message (less optimal, IMHO).

What say the list?

jon.

Re: RFD: should git rev-parse exit with non-zero status if ref@{n} is not valid?

[Jon Seymour]

| Argh! Sorry, that last message made no sense. Corrections marked with _.


On Wed, Aug 18, 2010 at 9:36 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
> While reworking my detached-stash topic, I noticed that git rev-parse
> ref@{n} will exit with a warning message on stderr, the SHA1 of ref
> and a _zero_ exit code if ref exists, but ref@{n} does not.
>
> I understand why ref@{time-spec} might behave this way, but I reckon
> that if you ask for ref@{n} for n > N-1, where N is the length of the
> reflog, then you should either get empty output and a non-zero status
> (preferred) or a ref@{N-1} on the output with a status code of zero
> and a warning message (less optimal, IMHO).
>
> What say the list?
>
> jon.
>

Re: RFD: should git rev-parse exit with non-zero status if ref@{n} is not valid?

[Junio C Hamano]

Jon Seymour <jon.seymour@gmail.com> writes:

> I understand why ref@{time-spec} might behave this way, but I reckon
> that if you ask for ref@{n} for n > N-1, where N is the length of the
> reflog, then you should either get empty output and a non-zero status
> (preferred) or a ref@{N-1} on the output with a status code of zero
> and a warning message (less optimal, IMHO).

Yeah, the behaviour of ref@{...} syntax parser is way suboptimal:

    $ git rev-parse --verify jch@{99999}; echo $?
    warning: Log for 'jch' only has 1368 entries.
    cfb88e9a8d4926b0011ae2dd67e1f57a98f4b768
    0

It even knows that it is running off the cut-off point; it should just
cause the caller to notice that fact.  I don't think changing it to error
out should cause any harm to existing callers.

It may just be the matter of doing something like this (totally
untested)...

 sha1_name.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 4af94fa..c1e51c9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -342,7 +342,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1);
 
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 {
-	static const char *warning = "warning: refname '%.*s' is ambiguous.\n";
+	static const char *warn_msg = "warning: refname '%.*s' is ambiguous.\n";
 	char *real_ref = NULL;
 	int refs_found = 0;
 	int at, reflog_len;
@@ -390,7 +390,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		return -1;
 
 	if (warn_ambiguous_refs && refs_found > 1)
-		fprintf(stderr, warning, len, str);
+		fprintf(stderr, warn_msg, len, str);
 
 	if (reflog_len) {
 		int nth, i;
@@ -426,14 +426,12 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		if (read_ref_at(real_ref, at_time, nth, sha1, NULL,
 				&co_time, &co_tz, &co_cnt)) {
 			if (at_time)
-				fprintf(stderr,
-					"warning: Log for '%.*s' only goes "
+				warning("Log for '%.*s' only goes "
 					"back to %s.\n", len, str,
 					show_date(co_time, co_tz, DATE_RFC2822));
 			else
-				fprintf(stderr,
-					"warning: Log for '%.*s' only has "
-					"%d entries.\n", len, str, co_cnt);
+				return error("Log for '%.*s' only has "
+					     "%d entries.\n", len, str, co_cnt);
 		}
 	}
 

Re: RFD: should git rev-parse exit with non-zero status if ref@{n} is not valid?

[Jon Seymour]

Thanks for that. Not quite sure why yet, but the error message is
duplicated. I wonder also if the free() call at the end of the
function needs to be duplicated before the early return?

Anyway, I am happy to look at this. Would you prefer this to be
delivered as a separate patch or can I re-roll it at the base of
detached-stash? (I realise I have to re-roll v5 anyway, because some
of the later tests don't quite reflect the intent (e.g. ! git stash
stash@{0} should be ! git stash drop stash@{0}).

jon.

On Thu, Aug 19, 2010 at 6:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jon Seymour <jon.seymour@gmail.com> writes:
>
>> I understand why ref@{time-spec} might behave this way, but I reckon
>> that if you ask for ref@{n} for n > N-1, where N is the length of the
>> reflog, then you should either get empty output and a non-zero status
>> (preferred) or a ref@{N-1} on the output with a status code of zero
>> and a warning message (less optimal, IMHO).
>
> Yeah, the behaviour of ref@{...} syntax parser is way suboptimal:
>
>    $ git rev-parse --verify jch@{99999}; echo $?
>    warning: Log for 'jch' only has 1368 entries.
>    cfb88e9a8d4926b0011ae2dd67e1f57a98f4b768
>    0
>
> It even knows that it is running off the cut-off point; it should just
> cause the caller to notice that fact.  I don't think changing it to error
> out should cause any harm to existing callers.
>
> It may just be the matter of doing something like this (totally
> untested)...
>
>  sha1_name.c |   12 +++++-------
>  1 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 4af94fa..c1e51c9 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -342,7 +342,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1);
>
>  static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>  {
> -       static const char *warning = "warning: refname '%.*s' is ambiguous.\n";
> +       static const char *warn_msg = "warning: refname '%.*s' is ambiguous.\n";
>        char *real_ref = NULL;
>        int refs_found = 0;
>        int at, reflog_len;
> @@ -390,7 +390,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>                return -1;
>
>        if (warn_ambiguous_refs && refs_found > 1)
> -               fprintf(stderr, warning, len, str);
> +               fprintf(stderr, warn_msg, len, str);
>
>        if (reflog_len) {
>                int nth, i;
> @@ -426,14 +426,12 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>                if (read_ref_at(real_ref, at_time, nth, sha1, NULL,
>                                &co_time, &co_tz, &co_cnt)) {
>                        if (at_time)
> -                               fprintf(stderr,
> -                                       "warning: Log for '%.*s' only goes "
> +                               warning("Log for '%.*s' only goes "
>                                        "back to %s.\n", len, str,
>                                        show_date(co_time, co_tz, DATE_RFC2822));
>                        else
> -                               fprintf(stderr,
> -                                       "warning: Log for '%.*s' only has "
> -                                       "%d entries.\n", len, str, co_cnt);
> +                               return error("Log for '%.*s' only has "
> +                                            "%d entries.\n", len, str, co_cnt);
>                }
>        }
>
>

[PATCH 0/4] rev-parse: improve reporting of invalid log references

[Jon Seymour]

This series fixes git rev-parse so that references of the form ref@{n}
cause rev-parse to fail with non-zero status codes when n >= N, where
N is the size of the reflog for ref.

The first commit causes git rev-parse to exit with a non-zero 
status code but has the flaw that the error message is duplicated 
if --verify is not specified.

The second commit fixes the duplicate error messages produced by
the first commit, but prevents the diagnostic being displayed
if the --verify option is specified.

The third commit restores the diagnostic if the --verify
option is specified.

The fourth commit adds tests for the changed behaviour.

Jon Seymour (4):
  rev-parse: exit with non-zero status if ref@{n} is not valid.
  rev-parse: suppress duplicate log limit exceeded message.
  rev-parse: introduce get_sha1_with_gently
  rev-parse: tests that git rev-parse --verify master@{n}

 builtin/rev-parse.c            |    2 +-
 cache.h                        |    1 +
 sha1_name.c                    |   52 ++++++++++++++++++++++++---------------
 t/t1503-rev-parse-verify.sh    |   11 ++++++++
 t/t1506-rev-parse-diagnosis.sh |    9 +++++++
 5 files changed, 54 insertions(+), 21 deletions(-)

-- 
1.7.2.1.152.g499e.dirty

[PATCH 1/4] rev-parse: exit with non-zero status if ref@{n} is not valid.

[Jon Seymour]

>From Junio's e-mail:

"The current behaviour of ref@{...} syntax parser is suboptimal:

    $ git rev-parse --verify jch@{99999}; echo $?
    warning: Log for 'jch' only has 1368 entries.
    cfb88e9a8d4926b0011ae2dd67e1f57a98f4b768
    0

It even knows that it is running off the cut-off point; it should just
cause the caller to notice that fact.  I don't think changing it to error
out should cause any harm to existing callers."

This commit is based on a patch suggested by Junio.

Note that the error message reporting the bad log reference
is printed twice if --verify is not specified because
the function is called from two different paths. This is
fixed in subsequent commits.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 sha1_name.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 4af94fa..82ad0f9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -342,7 +342,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1);
 
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 {
-	static const char *warning = "warning: refname '%.*s' is ambiguous.\n";
+	static const char *warn_msg = "warning: refname '%.*s' is ambiguous.";
 	char *real_ref = NULL;
 	int refs_found = 0;
 	int at, reflog_len;
@@ -390,7 +390,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		return -1;
 
 	if (warn_ambiguous_refs && refs_found > 1)
-		fprintf(stderr, warning, len, str);
+		fprintf(stderr, warn_msg, len, str);
 
 	if (reflog_len) {
 		int nth, i;
@@ -426,14 +426,17 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		if (read_ref_at(real_ref, at_time, nth, sha1, NULL,
 				&co_time, &co_tz, &co_cnt)) {
 			if (at_time)
-				fprintf(stderr,
-					"warning: Log for '%.*s' only goes "
-					"back to %s.\n", len, str,
+				warning("Log for '%.*s' only goes "
+					"back to %s.", len, str,
 					show_date(co_time, co_tz, DATE_RFC2822));
-			else
-				fprintf(stderr,
-					"warning: Log for '%.*s' only has "
-					"%d entries.\n", len, str, co_cnt);
+			else {
+				error("Log for '%.*s' only has %d entries.",
+					len,
+					str,
+					co_cnt);
+				free(real_ref);
+				return -1;
+			}
 		}
 	}
 
-- 
1.7.2.1.156.gf148c

[PATCH 2/4] rev-parse: suppress duplicate log limit exceeded message.

[Jon Seymour]

Modifies get_sha1_basic to accept a gently parameter to indicate
whether error should be used to report the log limit exceeded
condition.

Prior to this change, git rev-parse master@{99999} reported:

  error: Log for 'master' only has 166 entries.
  master@{99999}
  error: Log for 'master' only has 166 entries.
  fatal: ambiguous argument 'master@{99999}': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions

With this change, git rev-parse master@{99999} reports:

  master@{99999}
  error: Log for 'master' only has 166 entries.
  fatal: ambiguous argument 'master@{99999}': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions

Note: git rev-parse --verify master@{99999} now does not report
the error message although it does still exit with a non-zero
status code. This is fixed by the next commit.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 sha1_name.c |   31 ++++++++++++++++---------------
 1 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 82ad0f9..6e706eb 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -338,9 +338,9 @@ static inline int upstream_mark(const char *string, int len)
 	return 0;
 }
 
-static int get_sha1_1(const char *name, int len, unsigned char *sha1);
+static int get_sha1_1(const char *name, int len, unsigned char *sha1, int gently);
 
-static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
+static int get_sha1_basic(const char *str, int len, unsigned char *sha1, int gently)
 {
 	static const char *warn_msg = "warning: refname '%.*s' is ambiguous.";
 	char *real_ref = NULL;
@@ -375,7 +375,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		ret = interpret_branch_name(str+at, &buf);
 		if (ret > 0) {
 			/* substitute this branch name and restart */
-			return get_sha1_1(buf.buf, buf.len, sha1);
+			return get_sha1_1(buf.buf, buf.len, sha1, 1);
 		} else if (ret == 0) {
 			return -1;
 		}
@@ -430,10 +430,11 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 					"back to %s.", len, str,
 					show_date(co_time, co_tz, DATE_RFC2822));
 			else {
-				error("Log for '%.*s' only has %d entries.",
-					len,
-					str,
-					co_cnt);
+				if (!gently)
+					error("Log for '%.*s' only has %d entries.",
+						len,
+						str,
+						co_cnt);
 				free(real_ref);
 				return -1;
 			}
@@ -448,7 +449,7 @@ static int get_parent(const char *name, int len,
 		      unsigned char *result, int idx)
 {
 	unsigned char sha1[20];
-	int ret = get_sha1_1(name, len, sha1);
+	int ret = get_sha1_1(name, len, sha1, 1);
 	struct commit *commit;
 	struct commit_list *p;
 
@@ -481,7 +482,7 @@ static int get_nth_ancestor(const char *name, int len,
 	struct commit *commit;
 	int ret;
 
-	ret = get_sha1_1(name, len, sha1);
+	ret = get_sha1_1(name, len, sha1, 1);
 	if (ret)
 		return ret;
 	commit = lookup_commit_reference(sha1);
@@ -504,7 +505,7 @@ struct object *peel_to_type(const char *name, int namelen,
 		namelen = strlen(name);
 	if (!o) {
 		unsigned char sha1[20];
-		if (get_sha1_1(name, namelen, sha1))
+		if (get_sha1_1(name, namelen, sha1, 1))
 			return NULL;
 		o = parse_object(sha1);
 	}
@@ -566,7 +567,7 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 	else
 		return -1;
 
-	if (get_sha1_1(name, sp - name - 2, outer))
+	if (get_sha1_1(name, sp - name - 2, outer, 1))
 		return -1;
 
 	o = parse_object(outer);
@@ -614,7 +615,7 @@ static int get_describe_name(const char *name, int len, unsigned char *sha1)
 	return -1;
 }
 
-static int get_sha1_1(const char *name, int len, unsigned char *sha1)
+static int get_sha1_1(const char *name, int len, unsigned char *sha1, int gently)
 {
 	int ret, has_suffix;
 	const char *cp;
@@ -650,7 +651,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1)
 	if (!ret)
 		return 0;
 
-	ret = get_sha1_basic(name, len, sha1);
+	ret = get_sha1_basic(name, len, sha1, gently);
 	if (!ret)
 		return 0;
 
@@ -1059,7 +1060,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 
 	memset(oc, 0, sizeof(*oc));
 	oc->mode = S_IFINVALID;
-	ret = get_sha1_1(name, namelen, sha1);
+	ret = get_sha1_1(name, namelen, sha1, gently);
 	if (!ret)
 		return ret;
 	/* sha1:path --> object name of path in ent sha1
@@ -1122,7 +1123,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 			strncpy(object_name, name, cp-name);
 			object_name[cp-name] = '\0';
 		}
-		if (!get_sha1_1(name, cp-name, tree_sha1)) {
+		if (!get_sha1_1(name, cp-name, tree_sha1, 1)) {
 			const char *filename = cp+1;
 			ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);
 			if (!gently) {
-- 
1.7.2.1.156.gf148c

[PATCH 3/4] rev-parse: introduce get_sha1_gently

[Jon Seymour]

Use of this function allows git rev-parse --verify ref@{n}
to report an error message if n >= the size of the
reference log for ref.

However, if the user has not specified --verify, get_sha1_gently
will be called again via verify_filename and the message will
be reported then, if required.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 builtin/rev-parse.c |    2 +-
 cache.h             |    1 +
 sha1_name.c         |    8 ++++++++
 3 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index a5a1c86..dbc6a8c 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -707,7 +707,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			name++;
 			type = REVERSED;
 		}
-		if (!get_sha1(name, sha1)) {
+		if (!get_sha1_gently(name, sha1, !verify)) {
 			if (verify)
 				revs_count++;
 			else
diff --git a/cache.h b/cache.h
index 37ef9d8..625728c 100644
--- a/cache.h
+++ b/cache.h
@@ -767,6 +767,7 @@ static inline int get_sha1_with_context(const char *str, unsigned char *sha1, st
 {
 	return get_sha1_with_context_1(str, sha1, orc, 1, NULL);
 }
+extern inline int get_sha1_gently(const char *str, unsigned char *sha1, int gently);
 extern int get_sha1_hex(const char *hex, unsigned char *sha1);
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
 extern int read_ref(const char *filename, unsigned char *sha1);
diff --git a/sha1_name.c b/sha1_name.c
index 6e706eb..48e7aa0 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1050,6 +1050,14 @@ int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,
 	return ret;
 }
 
+int get_sha1_gently(const char *name, unsigned char *sha1, int gently)
+{
+	struct object_context unused;
+	int ret;
+	ret = get_sha1_with_context_1(name, sha1, &unused, gently, NULL);
+	return ret;
+}
+
 int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 			    struct object_context *oc,
 			    int gently, const char *prefix)
-- 
1.7.2.1.156.gf148c

[PATCH 4/4] rev-parse: tests that git rev-parse --verify master@{n}

[Jon Seymour]

This commit introduces tests that verify that rev-parse
parses master@{n} correctly for various values of n less
than, equal to and greater than the number of revisions
in the reference log.

In particular, these tests check that rev-parse sets a
non-zero status code and prints a message of the
following form to stderr.

    error: Log for [^ ]* only has [0-9][0-9]* entries.

Also tests that output for failing cases is generated if,
and only if, --verify is not specified.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 t/t1503-rev-parse-verify.sh    |   11 +++++++++++
 t/t1506-rev-parse-diagnosis.sh |    9 +++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index cc65394..61092f7 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -104,4 +104,15 @@ test_expect_success 'use --default' '
 	test_must_fail git rev-parse --verify --default bar
 '
 
+test_expect_success 'master@{n} for various n' '
+	N=$(git reflog | wc -l) &&
+	Nm1=$((N-1)) &&
+	Np1=$((N+1)) &&
+	git rev-parse --verify master@{0} &&
+	git rev-parse --verify master@{1} &&
+	git rev-parse --verify master@{$Nm1} &&
+	test_must_fail "git rev-parse --verify master@{$N}" &&
+	test_must_fail "git rev-parse --verify master@{$Np1}"
+'
+
 test_done
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index af721f9..427c67e 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -66,4 +66,13 @@ test_expect_success 'incorrect file in :path and :N:path' '
 	grep "fatal: Path '"'"'disk-only.txt'"'"' exists on disk, but not in the index." error
 '
 
+test_expect_success 'invalid @{n} reference' '
+	test_must_fail git rev-parse master@{99999} >output 2>error &&
+	test "$(cat output)" = "master@{99999}" &&
+	grep "error: Log for [^ ]* only has [0-9][0-9]* entries." error  &&
+	test_must_fail git rev-parse --verify master@{99999} >output 2>error &&
+	test -z "$(cat output)" &&
+	grep "error: Log for [^ ]* only has [0-9][0-9]* entries." error
+'
+
 test_done
-- 
1.7.2.1.156.gf148c

Re: [PATCH 2/4] rev-parse: suppress duplicate log limit exceeded message.

[Junio C Hamano]

Jon Seymour <jon.seymour@gmail.com> writes:

> With this change, git rev-parse master@{99999} reports:
>
>   master@{99999}
>   error: Log for 'master' only has 166 entries.
>   fatal: ambiguous argument 'master@{99999}': unknown revision or path not in the working tree.
>   Use '--' to separate paths from revisions

If we are going to say "fatal:" and die at the end, I think we probably do
not want to say a separate "error" message.  Instead of adding a boolean
parameter "fail silently or warn?", it may be better to make it a pointer
to a strbuf and have it filled with error details (or a machine readable
struct and make it responsibility of the caller to generate a message).

Then die_verify_filename can become, e.g.:

static void NORETURN die_verify_filename(const char *prefix, const char *arg)
{
	unsigned char sha1[20];
	unsigned mode;
        struct strbuf e;

        /* learn in what way is it bad? */
	get_sha1_with_mode_1(arg, sha1, &mode, &e, prefix);

	/* ... or fall back the most general message. */
	die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
	    "%s"
            "Use '--' to separate paths from revisions", arg, e.buf);
}

and we can later reuse the same mechanism to cover other kind of errors,
not just "the log does not have that many entries" error.  With your
"gently" approach it may not be easy to do that without adding more
parameters to all the functions in the codepath, no?

I also wonder if this can simply become part of "struct object_context",
which is to pass extra information in addition to the SHA-1 (which the
original API returned) back to the caller.

Re: [PATCH 2/4] rev-parse: suppress duplicate log limit exceeded message.

[Jon Seymour]

On Mon, Aug 23, 2010 at 7:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jon Seymour <jon.seymour@gmail.com> writes:
>
>> With this change, git rev-parse master@{99999} reports:
>>
>>   master@{99999}
>>   error: Log for 'master' only has 166 entries.
>>   fatal: ambiguous argument 'master@{99999}': unknown revision or path not in the working tree.
>>   Use '--' to separate paths from revisions
>
> If we are going to say "fatal:" and die at the end, I think we probably do
> not want to say a separate "error" message.  Instead of adding a boolean
> parameter "fail silently or warn?", it may be better to make it a pointer
> to a strbuf and have it filled with error details (or a machine readable
> struct and make it responsibility of the caller to generate a message).
>

I agree that it is not ideal that we have an error message and a fatal
message in this case.

Ideally,  I think we should simply be reporting the log limit exceeded
condition as a fatal condition on its own, and not reporting the
ambiguous ref catch all at all in this case. We have already decided
that the argument is a reference at this point and as such it is not
really ambiguous, it just happens to be out of bounds.

> Then die_verify_filename can become, e.g.:
>
> static void NORETURN die_verify_filename(const char *prefix, const char *arg)
> {
>        unsigned char sha1[20];
>        unsigned mode;
>        struct strbuf e;
>
>        /* learn in what way is it bad? */
>        get_sha1_with_mode_1(arg, sha1, &mode, &e, prefix);
>
>        /* ... or fall back the most general message. */
>        die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
>            "%s"
>            "Use '--' to separate paths from revisions", arg, e.buf);
> }
>
> and we can later reuse the same mechanism to cover other kind of errors,
> not just "the log does not have that many entries" error.  With your
> "gently" approach it may not be easy to do that without adding more
> parameters to all the functions in the codepath, no?
>

I agree that the gently approach used here doesn't generalise to other
cases. It seemed like the smallest change I could make that was
reasonably consistent with existing code. I am happy to look at a more
general solution.

> I also wonder if this can simply become part of "struct object_context",
> which is to pass extra information in addition to the SHA-1 (which the
> original API returned) back to the caller.
>

I'll have a look at this. object_context seems like a reasonably
coherent structure at the moment - do we risk diluting that coherence
by attaching error reporting state to it?

Should we consider passing an error callback function (+ struct)
around so that the calling context can decide whether to directly
report errors at the point of detection or defer them until later?
Calling contexts that don't care about reporting immediately can do
so, calling contexts that do care can choose how to record errors into
the struct, as required.

jon.

Re: [PATCH 2/4] rev-parse: suppress duplicate log limit exceeded message.

[Junio C Hamano]

Jon Seymour <jon.seymour@gmail.com> writes:

> Ideally,  I think we should simply be reporting the log limit exceeded
> condition as a fatal condition on its own, and not reporting the
> ambiguous ref catch all at all in this case. We have already decided
> that the argument is a reference at this point and as such it is not
> really ambiguous, it just happens to be out of bounds.

A very good point.  Would simply replacing that error-then-return-nonzero
with a die work?  If so we do not even need to worry about new mechanisms
to cause different soft error conditions reported back to the caller to be
printed.

[PATCH v2 0/2] rev-parse: strengthen validation of ref@{n} references

[Jon Seymour]

This series changes git rev-parse so that references of the form:

	ref@{n}

cause rev-parse to fail with a non-zero status code if n >= size of the 
reference log for ref.

The first commit implements the changed behaviour, the second commit 
updates the tests to include tests for the changed behaviour.

This is the second revision of this series. It differs from the first
in that it uses die rather than error to report the condition
and thus exits early, without generating any output on stdout
for the failing argument. It also reports just a single fatal
message instead of both an error message and a fatal message.

The original patch and this revision are both based on suggestions
from Junio.

Jon Seymour (2):
  rev-parse: exit with non-zero status if ref@{n} is not valid.
  rev-parse: tests git rev-parse --verify master@{n}, for various n

 sha1_name.c                    |   20 +++++++++++---------
 t/t1503-rev-parse-verify.sh    |   11 +++++++++++
 t/t1506-rev-parse-diagnosis.sh |    9 +++++++++
 3 files changed, 31 insertions(+), 9 deletions(-)

-- 
1.7.2.1.99.geab11

[PATCH v2 1/2] rev-parse: exit with non-zero status if ref@{n} is not valid.

[Jon Seymour]

>From Junio's e-mail:

"The current behaviour of ref@{...} syntax parser is suboptimal:

    $ git rev-parse --verify jch@{99999} && echo true
    warning: Log for 'jch' only has 1368 entries.
    cfb88e9a8d4926b0011ae2dd67e1f57a98f4b768
    true

It even knows that it is running off the cut-off point; it should just
cause the caller to notice that fact.  I don't think changing it to error
out should cause any harm to existing callers."

With this change:

   $ git rev-parse --verify jch@{99999} || echo false
   fatal: Log for 'jch' only has 1368 entries.
   false

   $ git rev-parse jch@{99999} || echo false
   fatal: Log for 'jch' only has 1368 entries.
   false

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 sha1_name.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 4af94fa..d40ae48 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -342,7 +342,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1);
 
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 {
-	static const char *warning = "warning: refname '%.*s' is ambiguous.\n";
+	static const char *warn_msg = "warning: refname '%.*s' is ambiguous.";
 	char *real_ref = NULL;
 	int refs_found = 0;
 	int at, reflog_len;
@@ -390,7 +390,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		return -1;
 
 	if (warn_ambiguous_refs && refs_found > 1)
-		fprintf(stderr, warning, len, str);
+		fprintf(stderr, warn_msg, len, str);
 
 	if (reflog_len) {
 		int nth, i;
@@ -426,14 +426,16 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		if (read_ref_at(real_ref, at_time, nth, sha1, NULL,
 				&co_time, &co_tz, &co_cnt)) {
 			if (at_time)
-				fprintf(stderr,
-					"warning: Log for '%.*s' only goes "
-					"back to %s.\n", len, str,
+				warning("Log for '%.*s' only goes "
+					"back to %s.", len, str,
 					show_date(co_time, co_tz, DATE_RFC2822));
-			else
-				fprintf(stderr,
-					"warning: Log for '%.*s' only has "
-					"%d entries.\n", len, str, co_cnt);
+			else {
+				free(real_ref);
+				die("Log for '%.*s' only has %d entries.",
+						len,
+						str,
+						co_cnt);
+			}
 		}
 	}
 
-- 
1.7.2.1.99.geab11

[PATCH v2 2/2] rev-parse: tests git rev-parse --verify master@{n}, for various n

[Jon Seymour]

This commit introduces tests that verify that rev-parse
parses master@{n} correctly for various values of n less
than, equal to and greater than the number of revisions
in the reference log.

In particular, these tests check that rev-parse exits with a
non-zero status code and prints a message of the
following form to stderr.

    fatal: Log for [^ ]* only has [0-9][0-9]* entries.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 t/t1503-rev-parse-verify.sh    |   11 +++++++++++
 t/t1506-rev-parse-diagnosis.sh |    9 +++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index cc65394..61092f7 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -104,4 +104,15 @@ test_expect_success 'use --default' '
 	test_must_fail git rev-parse --verify --default bar
 '
 
+test_expect_success 'master@{n} for various n' '
+	N=$(git reflog | wc -l) &&
+	Nm1=$((N-1)) &&
+	Np1=$((N+1)) &&
+	git rev-parse --verify master@{0} &&
+	git rev-parse --verify master@{1} &&
+	git rev-parse --verify master@{$Nm1} &&
+	test_must_fail "git rev-parse --verify master@{$N}" &&
+	test_must_fail "git rev-parse --verify master@{$Np1}"
+'
+
 test_done
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index af721f9..0eeeb0e 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -66,4 +66,13 @@ test_expect_success 'incorrect file in :path and :N:path' '
 	grep "fatal: Path '"'"'disk-only.txt'"'"' exists on disk, but not in the index." error
 '
 
+test_expect_success 'invalid @{n} reference' '
+	test_must_fail git rev-parse master@{99999} >output 2>error &&
+	test -z "$(cat output)" &&
+	grep "fatal: Log for [^ ]* only has [0-9][0-9]* entries." error  &&
+	test_must_fail git rev-parse --verify master@{99999} >output 2>error &&
+	test -z "$(cat output)" &&
+	grep "fatal: Log for [^ ]* only has [0-9][0-9]* entries." error
+'
+
 test_done
-- 
1.7.2.1.99.geab11

Re: [PATCH v2 1/2] rev-parse: exit with non-zero status if ref@{n} is not valid.

[Jonathan Nieder]

Jon Seymour wrote:

> +++ b/sha1_name.c
> @@ -342,7 +342,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1);
>  
>  static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>  {
> -	static const char *warning = "warning: refname '%.*s' is ambiguous.\n";
> +	static const char *warn_msg = "warning: refname '%.*s' is ambiguous.";

$ git tag master
$ bin-wrappers/git rev-parse master
warning: refname 'master' is ambiguous.cfee1ae8f0c56eed7d8ffa821f650789f8b11de2

Here's a fixup.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
diff --git a/sha1_name.c b/sha1_name.c
index d40ae48..823e582 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -342,7 +342,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1);
 
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 {
-	static const char *warn_msg = "warning: refname '%.*s' is ambiguous.";
+	static const char *warn_msg = "warning: refname '%.*s' is ambiguous.\n";
 	char *real_ref = NULL;
 	int refs_found = 0;
 	int at, reflog_len;
-- 

[PATCH v3 0/3] rev-parse: strengthen validation of ref@{n} references

[Jon Seymour]

This series changes git rev-parse so that references of the form:

       ref@{n}

cause rev-parse to fail with a non-zero status code if n >= size of the
reference log for ref.

The first commit implements the changed behaviour; the second commit
replaces uses of fprintf (within sha1_name.c) with warning; 
the third commit updates the tests to include tests for the changed behaviour.

This is the third revision of this series. It differs from the first
in that it uses die rather than error to report the condition
and thus exits early, without generating any output on stdout
for the failing argument. It also reports just a single fatal
message instead of both an error message and a fatal message.

The original patch and this revision are both based on suggestions
from Junio. Corrections to diagnostic output inspired by Jonathan Nieder's
review.

Jon Seymour (3):
  rev-parse: exit with non-zero status if ref@{n} is not valid.
  sha1_name.c: use warning in preference to fprintf(stderr
  rev-parse: tests git rev-parse --verify master@{n}, for various n

 sha1_name.c                    |   20 +++++++++++---------
 t/t1503-rev-parse-verify.sh    |   11 +++++++++++
 t/t1506-rev-parse-diagnosis.sh |    9 +++++++++
 3 files changed, 31 insertions(+), 9 deletions(-)

-- 
1.7.2.1.100.gceecd

[PATCH v3 1/3] rev-parse: exit with non-zero status if ref@{n} is not valid.

[Jon Seymour]

>From Junio's e-mail:

"The current behaviour of ref@{...} syntax parser is suboptimal:

    $ git rev-parse --verify jch@{99999} && echo true
    warning: Log for 'jch' only has 1368 entries.
    cfb88e9a8d4926b0011ae2dd67e1f57a98f4b768
    true

It even knows that it is running off the cut-off point; it should just
cause the caller to notice that fact.  I don't think changing it to error
out should cause any harm to existing callers."

With this change:

   $ git rev-parse --verify jch@{99999} || echo false
   fatal: Log for 'jch' only has 1368 entries.
   false

   $ git rev-parse jch@{99999} || echo false
   fatal: Log for 'jch' only has 1368 entries.
   false

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 sha1_name.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 4af94fa..10a4efe 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -430,10 +430,13 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 					"warning: Log for '%.*s' only goes "
 					"back to %s.\n", len, str,
 					show_date(co_time, co_tz, DATE_RFC2822));
-			else
-				fprintf(stderr,
-					"warning: Log for '%.*s' only has "
-					"%d entries.\n", len, str, co_cnt);
+			else {
+				free(real_ref);
+				die("Log for '%.*s' only has %d entries.",
+						len,
+						str,
+						co_cnt);
+			}
 		}
 	}
 
-- 
1.7.2.1.100.gceecd

[PATCH v3 2/3] sha1_name.c: use warning in preference to fprintf(stderr

[Jon Seymour]

This commit changes sha1_name.c to use warning instead of
fprintf(stderr).

Trailing newlines from message formats have been removed
since warning adds one itself.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 sha1_name.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 10a4efe..3b8aea2 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -342,7 +342,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1);
 
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 {
-	static const char *warning = "warning: refname '%.*s' is ambiguous.\n";
+	static const char *warn_msg = "refname '%.*s' is ambiguous.";
 	char *real_ref = NULL;
 	int refs_found = 0;
 	int at, reflog_len;
@@ -390,7 +390,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		return -1;
 
 	if (warn_ambiguous_refs && refs_found > 1)
-		fprintf(stderr, warning, len, str);
+		warning(warn_msg, len, str);
 
 	if (reflog_len) {
 		int nth, i;
@@ -426,9 +426,8 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		if (read_ref_at(real_ref, at_time, nth, sha1, NULL,
 				&co_time, &co_tz, &co_cnt)) {
 			if (at_time)
-				fprintf(stderr,
-					"warning: Log for '%.*s' only goes "
-					"back to %s.\n", len, str,
+				warning("Log for '%.*s' only goes "
+					"back to %s.", len, str,
 					show_date(co_time, co_tz, DATE_RFC2822));
 			else {
 				free(real_ref);
-- 
1.7.2.1.100.gceecd

[PATCH v3 3/3] rev-parse: tests git rev-parse --verify master@{n}, for various n

[Jon Seymour]

This commit introduces tests that verify that rev-parse
parses master@{n} correctly for various values of n less
than, equal to and greater than the number of revisions
in the reference log.

In particular, these tests check that rev-parse exits with a
non-zero status code and prints a message of the
following form to stderr.

    fatal: Log for [^ ]* only has [0-9][0-9]* entries.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 t/t1503-rev-parse-verify.sh    |   11 +++++++++++
 t/t1506-rev-parse-diagnosis.sh |    9 +++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index cc65394..61092f7 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -104,4 +104,15 @@ test_expect_success 'use --default' '
 	test_must_fail git rev-parse --verify --default bar
 '
 
+test_expect_success 'master@{n} for various n' '
+	N=$(git reflog | wc -l) &&
+	Nm1=$((N-1)) &&
+	Np1=$((N+1)) &&
+	git rev-parse --verify master@{0} &&
+	git rev-parse --verify master@{1} &&
+	git rev-parse --verify master@{$Nm1} &&
+	test_must_fail "git rev-parse --verify master@{$N}" &&
+	test_must_fail "git rev-parse --verify master@{$Np1}"
+'
+
 test_done
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index af721f9..0eeeb0e 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -66,4 +66,13 @@ test_expect_success 'incorrect file in :path and :N:path' '
 	grep "fatal: Path '"'"'disk-only.txt'"'"' exists on disk, but not in the index." error
 '
 
+test_expect_success 'invalid @{n} reference' '
+	test_must_fail git rev-parse master@{99999} >output 2>error &&
+	test -z "$(cat output)" &&
+	grep "fatal: Log for [^ ]* only has [0-9][0-9]* entries." error  &&
+	test_must_fail git rev-parse --verify master@{99999} >output 2>error &&
+	test -z "$(cat output)" &&
+	grep "fatal: Log for [^ ]* only has [0-9][0-9]* entries." error
+'
+
 test_done
-- 
1.7.2.1.100.gceecd