{"thread":{"id":"24780","subject":"RFD: should git rev-parse exit with non-zero status if ref@{n} is not valid?","startedAt":"2010-08-18T11:36:11Z","lastAt":"2010-08-24T04:52:44Z","messageCount":20,"participants":["Jon Seymour","Junio C Hamano","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"148358","messageId":"AANLkTi=Ktz=ARx0qUqB0844aYuq8BbqYUM8g726r9o5t@mail.gmail.com","threadId":"24780","inReplyTo":null,"subject":"RFD: should git rev-parse exit with non-zero status if ref@{n} is not valid?","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-18T11:36:11Z","receivedAt":"2010-08-18T11:36:11Z","isPatch":false,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"While reworking my detached-stash topic, I noticed that git rev-parse\nref@{n} will exit with a warning message on stderr, the SHA1 of ref\nand a non-zero exit code if ref exists, but ref@{n} does not.\n\nI understand why ref@{time-spec} might behave this way, but I reckon\nthat if you ask for ref@{n} for n > N-1, where N is the length of the\nreflog, then you should either get empty output and a non-zero status\n(preferred) or a ref@{N-1} on the output with a status code of zero\nand a warning message (less optimal, IMHO).\n\nWhat say the list?\n\njon.\n"},{"id":"148359","messageId":"AANLkTim+A5SFekBSg9bndPzesuBrOKMQeVsfn5pidu3S@mail.gmail.com","threadId":"24780","inReplyTo":"AANLkTi=Ktz=ARx0qUqB0844aYuq8BbqYUM8g726r9o5t@mail.gmail.com","subject":"Re: RFD: should git rev-parse exit with non-zero status if ref@{n} is not valid?","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-18T11:41:49Z","receivedAt":"2010-08-18T11:41:49Z","isPatch":false,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"| Argh! Sorry, that last message made no sense. Corrections marked with _.\n\n\nOn Wed, Aug 18, 2010 at 9:36 PM, Jon Seymour <jon.seymour@gmail.com> wrote:\n> While reworking my detached-stash topic, I noticed that git rev-parse\n> ref@{n} will exit with a warning message on stderr, the SHA1 of ref\n> and a _zero_ exit code if ref exists, but ref@{n} does not.\n>\n> I understand why ref@{time-spec} might behave this way, but I reckon\n> that if you ask for ref@{n} for n > N-1, where N is the length of the\n> reflog, then you should either get empty output and a non-zero status\n> (preferred) or a ref@{N-1} on the output with a status code of zero\n> and a warning message (less optimal, IMHO).\n>\n> What say the list?\n>\n> jon.\n>\n"},{"id":"148412","messageId":"7vsk2b8x2t.fsf@alter.siamese.dyndns.org","threadId":"24780","inReplyTo":"AANLkTi=Ktz=ARx0qUqB0844aYuq8BbqYUM8g726r9o5t@mail.gmail.com","subject":"Re: RFD: should git rev-parse exit with non-zero status if ref@{n} is not valid?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-18T20:50:18Z","receivedAt":"2010-08-18T20:50:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jon Seymour <jon.seymour@gmail.com> writes:\n\n> I understand why ref@{time-spec} might behave this way, but I reckon\n> that if you ask for ref@{n} for n > N-1, where N is the length of the\n> reflog, then you should either get empty output and a non-zero status\n> (preferred) or a ref@{N-1} on the output with a status code of zero\n> and a warning message (less optimal, IMHO).\n\nYeah, the behaviour of ref@{...} syntax parser is way suboptimal:\n\n    $ git rev-parse --verify jch@{99999}; echo $?\n    warning: Log for 'jch' only has 1368 entries.\n    cfb88e9a8d4926b0011ae2dd67e1f57a98f4b768\n    0\n\nIt even knows that it is running off the cut-off point; it should just\ncause the caller to notice that fact.  I don't think changing it to error\nout should cause any harm to existing callers.\n\nIt may just be the matter of doing something like this (totally\nuntested)...\n\n sha1_name.c |   12 +++++-------\n 1 files changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 4af94fa..c1e51c9 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -342,7 +342,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1);\n \n static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n {\n-\tstatic const char *warning = \"warning: refname '%.*s' is ambiguous.\\n\";\n+\tstatic const char *warn_msg = \"warning: refname '%.*s' is ambiguous.\\n\";\n \tchar *real_ref = NULL;\n \tint refs_found = 0;\n \tint at, reflog_len;\n@@ -390,7 +390,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\treturn -1;\n \n \tif (warn_ambiguous_refs && refs_found > 1)\n-\t\tfprintf(stderr, warning, len, str);\n+\t\tfprintf(stderr, warn_msg, len, str);\n \n \tif (reflog_len) {\n \t\tint nth, i;\n@@ -426,14 +426,12 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\tif (read_ref_at(real_ref, at_time, nth, sha1, NULL,\n \t\t\t\t&co_time, &co_tz, &co_cnt)) {\n \t\t\tif (at_time)\n-\t\t\t\tfprintf(stderr,\n-\t\t\t\t\t\"warning: Log for '%.*s' only goes \"\n+\t\t\t\twarning(\"Log for '%.*s' only goes \"\n \t\t\t\t\t\"back to %s.\\n\", len, str,\n \t\t\t\t\tshow_date(co_time, co_tz, DATE_RFC2822));\n \t\t\telse\n-\t\t\t\tfprintf(stderr,\n-\t\t\t\t\t\"warning: Log for '%.*s' only has \"\n-\t\t\t\t\t\"%d entries.\\n\", len, str, co_cnt);\n+\t\t\t\treturn error(\"Log for '%.*s' only has \"\n+\t\t\t\t\t     \"%d entries.\\n\", len, str, co_cnt);\n \t\t}\n \t}\n \n"},{"id":"148420","messageId":"AANLkTi=m7+h0nVg+EAmiir-rnrjoBwbpLtztcNNBVxiz@mail.gmail.com","threadId":"24780","inReplyTo":"7vsk2b8x2t.fsf@alter.siamese.dyndns.org","subject":"Re: RFD: should git rev-parse exit with non-zero status if ref@{n} is not valid?","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-18T23:02:32Z","receivedAt":"2010-08-18T23:02:32Z","isPatch":false,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"Thanks for that. Not quite sure why yet, but the error message is\nduplicated. I wonder also if the free() call at the end of the\nfunction needs to be duplicated before the early return?\n\nAnyway, I am happy to look at this. Would you prefer this to be\ndelivered as a separate patch or can I re-roll it at the base of\ndetached-stash? (I realise I have to re-roll v5 anyway, because some\nof the later tests don't quite reflect the intent (e.g. ! git stash\nstash@{0} should be ! git stash drop stash@{0}).\n\njon.\n\nOn Thu, Aug 19, 2010 at 6:50 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jon Seymour <jon.seymour@gmail.com> writes:\n>\n>> I understand why ref@{time-spec} might behave this way, but I reckon\n>> that if you ask for ref@{n} for n > N-1, where N is the length of the\n>> reflog, then you should either get empty output and a non-zero status\n>> (preferred) or a ref@{N-1} on the output with a status code of zero\n>> and a warning message (less optimal, IMHO).\n>\n> Yeah, the behaviour of ref@{...} syntax parser is way suboptimal:\n>\n>    $ git rev-parse --verify jch@{99999}; echo $?\n>    warning: Log for 'jch' only has 1368 entries.\n>    cfb88e9a8d4926b0011ae2dd67e1f57a98f4b768\n>    0\n>\n> It even knows that it is running off the cut-off point; it should just\n> cause the caller to notice that fact.  I don't think changing it to error\n> out should cause any harm to existing callers.\n>\n> It may just be the matter of doing something like this (totally\n> untested)...\n>\n>  sha1_name.c |   12 +++++-------\n>  1 files changed, 5 insertions(+), 7 deletions(-)\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 4af94fa..c1e51c9 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -342,7 +342,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1);\n>\n>  static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n>  {\n> -       static const char *warning = \"warning: refname '%.*s' is ambiguous.\\n\";\n> +       static const char *warn_msg = \"warning: refname '%.*s' is ambiguous.\\n\";\n>        char *real_ref = NULL;\n>        int refs_found = 0;\n>        int at, reflog_len;\n> @@ -390,7 +390,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n>                return -1;\n>\n>        if (warn_ambiguous_refs && refs_found > 1)\n> -               fprintf(stderr, warning, len, str);\n> +               fprintf(stderr, warn_msg, len, str);\n>\n>        if (reflog_len) {\n>                int nth, i;\n> @@ -426,14 +426,12 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n>                if (read_ref_at(real_ref, at_time, nth, sha1, NULL,\n>                                &co_time, &co_tz, &co_cnt)) {\n>                        if (at_time)\n> -                               fprintf(stderr,\n> -                                       \"warning: Log for '%.*s' only goes \"\n> +                               warning(\"Log for '%.*s' only goes \"\n>                                        \"back to %s.\\n\", len, str,\n>                                        show_date(co_time, co_tz, DATE_RFC2822));\n>                        else\n> -                               fprintf(stderr,\n> -                                       \"warning: Log for '%.*s' only has \"\n> -                                       \"%d entries.\\n\", len, str, co_cnt);\n> +                               return error(\"Log for '%.*s' only has \"\n> +                                            \"%d entries.\\n\", len, str, co_cnt);\n>                }\n>        }\n>\n>\n"},{"id":"148615","messageId":"1282355022-17795-1-git-send-email-jon.seymour@gmail.com","threadId":"24780","inReplyTo":"AANLkTi=m7+h0nVg+EAmiir-rnrjoBwbpLtztcNNBVxiz@mail.gmail.com","subject":"[PATCH 0/4] rev-parse: improve reporting of invalid log references","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-21T01:43:38Z","receivedAt":"2010-08-21T01:43:38Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"This series fixes git rev-parse so that references of the form ref@{n}\ncause rev-parse to fail with non-zero status codes when n >= N, where\nN is the size of the reflog for ref.\n\nThe first commit causes git rev-parse to exit with a non-zero \nstatus code but has the flaw that the error message is duplicated \nif --verify is not specified.\n\nThe second commit fixes the duplicate error messages produced by\nthe first commit, but prevents the diagnostic being displayed\nif the --verify option is specified.\n\nThe third commit restores the diagnostic if the --verify\noption is specified.\n\nThe fourth commit adds tests for the changed behaviour.\n\nJon Seymour (4):\n  rev-parse: exit with non-zero status if ref@{n} is not valid.\n  rev-parse: suppress duplicate log limit exceeded message.\n  rev-parse: introduce get_sha1_with_gently\n  rev-parse: tests that git rev-parse --verify master@{n}\n\n builtin/rev-parse.c            |    2 +-\n cache.h                        |    1 +\n sha1_name.c                    |   52 ++++++++++++++++++++++++---------------\n t/t1503-rev-parse-verify.sh    |   11 ++++++++\n t/t1506-rev-parse-diagnosis.sh |    9 +++++++\n 5 files changed, 54 insertions(+), 21 deletions(-)\n\n-- \n1.7.2.1.152.g499e.dirty\n"},{"id":"148616","messageId":"1282355022-17795-2-git-send-email-jon.seymour@gmail.com","threadId":"24780","inReplyTo":"AANLkTi=m7+h0nVg+EAmiir-rnrjoBwbpLtztcNNBVxiz@mail.gmail.com","subject":"[PATCH 1/4] rev-parse: exit with non-zero status if ref@{n} is not valid.","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-21T01:43:39Z","receivedAt":"2010-08-21T01:43:39Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":">From Junio's e-mail:\n\n\"The current behaviour of ref@{...} syntax parser is suboptimal:\n\n    $ git rev-parse --verify jch@{99999}; echo $?\n    warning: Log for 'jch' only has 1368 entries.\n    cfb88e9a8d4926b0011ae2dd67e1f57a98f4b768\n    0\n\nIt even knows that it is running off the cut-off point; it should just\ncause the caller to notice that fact.  I don't think changing it to error\nout should cause any harm to existing callers.\"\n\nThis commit is based on a patch suggested by Junio.\n\nNote that the error message reporting the bad log reference\nis printed twice if --verify is not specified because\nthe function is called from two different paths. This is\nfixed in subsequent commits.\n\nSigned-off-by: Jon Seymour <jon.seymour@gmail.com>\n---\n sha1_name.c |   21 ++++++++++++---------\n 1 files changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 4af94fa..82ad0f9 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -342,7 +342,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1);\n \n static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n {\n-\tstatic const char *warning = \"warning: refname '%.*s' is ambiguous.\\n\";\n+\tstatic const char *warn_msg = \"warning: refname '%.*s' is ambiguous.\";\n \tchar *real_ref = NULL;\n \tint refs_found = 0;\n \tint at, reflog_len;\n@@ -390,7 +390,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\treturn -1;\n \n \tif (warn_ambiguous_refs && refs_found > 1)\n-\t\tfprintf(stderr, warning, len, str);\n+\t\tfprintf(stderr, warn_msg, len, str);\n \n \tif (reflog_len) {\n \t\tint nth, i;\n@@ -426,14 +426,17 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\tif (read_ref_at(real_ref, at_time, nth, sha1, NULL,\n \t\t\t\t&co_time, &co_tz, &co_cnt)) {\n \t\t\tif (at_time)\n-\t\t\t\tfprintf(stderr,\n-\t\t\t\t\t\"warning: Log for '%.*s' only goes \"\n-\t\t\t\t\t\"back to %s.\\n\", len, str,\n+\t\t\t\twarning(\"Log for '%.*s' only goes \"\n+\t\t\t\t\t\"back to %s.\", len, str,\n \t\t\t\t\tshow_date(co_time, co_tz, DATE_RFC2822));\n-\t\t\telse\n-\t\t\t\tfprintf(stderr,\n-\t\t\t\t\t\"warning: Log for '%.*s' only has \"\n-\t\t\t\t\t\"%d entries.\\n\", len, str, co_cnt);\n+\t\t\telse {\n+\t\t\t\terror(\"Log for '%.*s' only has %d entries.\",\n+\t\t\t\t\tlen,\n+\t\t\t\t\tstr,\n+\t\t\t\t\tco_cnt);\n+\t\t\t\tfree(real_ref);\n+\t\t\t\treturn -1;\n+\t\t\t}\n \t\t}\n \t}\n \n-- \n1.7.2.1.156.gf148c\n"},{"id":"148617","messageId":"1282355022-17795-3-git-send-email-jon.seymour@gmail.com","threadId":"24780","inReplyTo":"AANLkTi=m7+h0nVg+EAmiir-rnrjoBwbpLtztcNNBVxiz@mail.gmail.com","subject":"[PATCH 2/4] rev-parse: suppress duplicate log limit exceeded message.","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-21T01:43:40Z","receivedAt":"2010-08-21T01:43:40Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"Modifies get_sha1_basic to accept a gently parameter to indicate\nwhether error should be used to report the log limit exceeded\ncondition.\n\nPrior to this change, git rev-parse master@{99999} reported:\n\n  error: Log for 'master' only has 166 entries.\n  master@{99999}\n  error: Log for 'master' only has 166 entries.\n  fatal: ambiguous argument 'master@{99999}': unknown revision or path not in the working tree.\n  Use '--' to separate paths from revisions\n\nWith this change, git rev-parse master@{99999} reports:\n\n  master@{99999}\n  error: Log for 'master' only has 166 entries.\n  fatal: ambiguous argument 'master@{99999}': unknown revision or path not in the working tree.\n  Use '--' to separate paths from revisions\n\nNote: git rev-parse --verify master@{99999} now does not report\nthe error message although it does still exit with a non-zero\nstatus code. This is fixed by the next commit.\n\nSigned-off-by: Jon Seymour <jon.seymour@gmail.com>\n---\n sha1_name.c |   31 ++++++++++++++++---------------\n 1 files changed, 16 insertions(+), 15 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 82ad0f9..6e706eb 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -338,9 +338,9 @@ static inline int upstream_mark(const char *string, int len)\n \treturn 0;\n }\n \n-static int get_sha1_1(const char *name, int len, unsigned char *sha1);\n+static int get_sha1_1(const char *name, int len, unsigned char *sha1, int gently);\n \n-static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n+static int get_sha1_basic(const char *str, int len, unsigned char *sha1, int gently)\n {\n \tstatic const char *warn_msg = \"warning: refname '%.*s' is ambiguous.\";\n \tchar *real_ref = NULL;\n@@ -375,7 +375,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\tret = interpret_branch_name(str+at, &buf);\n \t\tif (ret > 0) {\n \t\t\t/* substitute this branch name and restart */\n-\t\t\treturn get_sha1_1(buf.buf, buf.len, sha1);\n+\t\t\treturn get_sha1_1(buf.buf, buf.len, sha1, 1);\n \t\t} else if (ret == 0) {\n \t\t\treturn -1;\n \t\t}\n@@ -430,10 +430,11 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\t\t\t\t\"back to %s.\", len, str,\n \t\t\t\t\tshow_date(co_time, co_tz, DATE_RFC2822));\n \t\t\telse {\n-\t\t\t\terror(\"Log for '%.*s' only has %d entries.\",\n-\t\t\t\t\tlen,\n-\t\t\t\t\tstr,\n-\t\t\t\t\tco_cnt);\n+\t\t\t\tif (!gently)\n+\t\t\t\t\terror(\"Log for '%.*s' only has %d entries.\",\n+\t\t\t\t\t\tlen,\n+\t\t\t\t\t\tstr,\n+\t\t\t\t\t\tco_cnt);\n \t\t\t\tfree(real_ref);\n \t\t\t\treturn -1;\n \t\t\t}\n@@ -448,7 +449,7 @@ static int get_parent(const char *name, int len,\n \t\t      unsigned char *result, int idx)\n {\n \tunsigned char sha1[20];\n-\tint ret = get_sha1_1(name, len, sha1);\n+\tint ret = get_sha1_1(name, len, sha1, 1);\n \tstruct commit *commit;\n \tstruct commit_list *p;\n \n@@ -481,7 +482,7 @@ static int get_nth_ancestor(const char *name, int len,\n \tstruct commit *commit;\n \tint ret;\n \n-\tret = get_sha1_1(name, len, sha1);\n+\tret = get_sha1_1(name, len, sha1, 1);\n \tif (ret)\n \t\treturn ret;\n \tcommit = lookup_commit_reference(sha1);\n@@ -504,7 +505,7 @@ struct object *peel_to_type(const char *name, int namelen,\n \t\tnamelen = strlen(name);\n \tif (!o) {\n \t\tunsigned char sha1[20];\n-\t\tif (get_sha1_1(name, namelen, sha1))\n+\t\tif (get_sha1_1(name, namelen, sha1, 1))\n \t\t\treturn NULL;\n \t\to = parse_object(sha1);\n \t}\n@@ -566,7 +567,7 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)\n \telse\n \t\treturn -1;\n \n-\tif (get_sha1_1(name, sp - name - 2, outer))\n+\tif (get_sha1_1(name, sp - name - 2, outer, 1))\n \t\treturn -1;\n \n \to = parse_object(outer);\n@@ -614,7 +615,7 @@ static int get_describe_name(const char *name, int len, unsigned char *sha1)\n \treturn -1;\n }\n \n-static int get_sha1_1(const char *name, int len, unsigned char *sha1)\n+static int get_sha1_1(const char *name, int len, unsigned char *sha1, int gently)\n {\n \tint ret, has_suffix;\n \tconst char *cp;\n@@ -650,7 +651,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1)\n \tif (!ret)\n \t\treturn 0;\n \n-\tret = get_sha1_basic(name, len, sha1);\n+\tret = get_sha1_basic(name, len, sha1, gently);\n \tif (!ret)\n \t\treturn 0;\n \n@@ -1059,7 +1060,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \n \tmemset(oc, 0, sizeof(*oc));\n \toc->mode = S_IFINVALID;\n-\tret = get_sha1_1(name, namelen, sha1);\n+\tret = get_sha1_1(name, namelen, sha1, gently);\n \tif (!ret)\n \t\treturn ret;\n \t/* sha1:path --> object name of path in ent sha1\n@@ -1122,7 +1123,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t\t\tstrncpy(object_name, name, cp-name);\n \t\t\tobject_name[cp-name] = '\\0';\n \t\t}\n-\t\tif (!get_sha1_1(name, cp-name, tree_sha1)) {\n+\t\tif (!get_sha1_1(name, cp-name, tree_sha1, 1)) {\n \t\t\tconst char *filename = cp+1;\n \t\t\tret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);\n \t\t\tif (!gently) {\n-- \n1.7.2.1.156.gf148c\n"},{"id":"148614","messageId":"1282355022-17795-4-git-send-email-jon.seymour@gmail.com","threadId":"24780","inReplyTo":"AANLkTi=m7+h0nVg+EAmiir-rnrjoBwbpLtztcNNBVxiz@mail.gmail.com","subject":"[PATCH 3/4] rev-parse: introduce get_sha1_gently","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-21T01:43:41Z","receivedAt":"2010-08-21T01:43:41Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"Use of this function allows git rev-parse --verify ref@{n}\nto report an error message if n >= the size of the\nreference log for ref.\n\nHowever, if the user has not specified --verify, get_sha1_gently\nwill be called again via verify_filename and the message will\nbe reported then, if required.\n\nSigned-off-by: Jon Seymour <jon.seymour@gmail.com>\n---\n builtin/rev-parse.c |    2 +-\n cache.h             |    1 +\n sha1_name.c         |    8 ++++++++\n 3 files changed, 10 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex a5a1c86..dbc6a8c 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -707,7 +707,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\tname++;\n \t\t\ttype = REVERSED;\n \t\t}\n-\t\tif (!get_sha1(name, sha1)) {\n+\t\tif (!get_sha1_gently(name, sha1, !verify)) {\n \t\t\tif (verify)\n \t\t\t\trevs_count++;\n \t\t\telse\ndiff --git a/cache.h b/cache.h\nindex 37ef9d8..625728c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -767,6 +767,7 @@ static inline int get_sha1_with_context(const char *str, unsigned char *sha1, st\n {\n \treturn get_sha1_with_context_1(str, sha1, orc, 1, NULL);\n }\n+extern inline int get_sha1_gently(const char *str, unsigned char *sha1, int gently);\n extern int get_sha1_hex(const char *hex, unsigned char *sha1);\n extern char *sha1_to_hex(const unsigned char *sha1);\t/* static buffer result! */\n extern int read_ref(const char *filename, unsigned char *sha1);\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 6e706eb..48e7aa0 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1050,6 +1050,14 @@ int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode,\n \treturn ret;\n }\n \n+int get_sha1_gently(const char *name, unsigned char *sha1, int gently)\n+{\n+\tstruct object_context unused;\n+\tint ret;\n+\tret = get_sha1_with_context_1(name, sha1, &unused, gently, NULL);\n+\treturn ret;\n+}\n+\n int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t\t\t    struct object_context *oc,\n \t\t\t    int gently, const char *prefix)\n-- \n1.7.2.1.156.gf148c\n"},{"id":"148618","messageId":"1282355022-17795-5-git-send-email-jon.seymour@gmail.com","threadId":"24780","inReplyTo":"AANLkTi=m7+h0nVg+EAmiir-rnrjoBwbpLtztcNNBVxiz@mail.gmail.com","subject":"[PATCH 4/4] rev-parse: tests that git rev-parse --verify master@{n}","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-21T01:43:42Z","receivedAt":"2010-08-21T01:43:42Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"This commit introduces tests that verify that rev-parse\nparses master@{n} correctly for various values of n less\nthan, equal to and greater than the number of revisions\nin the reference log.\n\nIn particular, these tests check that rev-parse sets a\nnon-zero status code and prints a message of the\nfollowing form to stderr.\n\n    error: Log for [^ ]* only has [0-9][0-9]* entries.\n\nAlso tests that output for failing cases is generated if,\nand only if, --verify is not specified.\n\nSigned-off-by: Jon Seymour <jon.seymour@gmail.com>\n---\n t/t1503-rev-parse-verify.sh    |   11 +++++++++++\n t/t1506-rev-parse-diagnosis.sh |    9 +++++++++\n 2 files changed, 20 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh\nindex cc65394..61092f7 100755\n--- a/t/t1503-rev-parse-verify.sh\n+++ b/t/t1503-rev-parse-verify.sh\n@@ -104,4 +104,15 @@ test_expect_success 'use --default' '\n \ttest_must_fail git rev-parse --verify --default bar\n '\n \n+test_expect_success 'master@{n} for various n' '\n+\tN=$(git reflog | wc -l) &&\n+\tNm1=$((N-1)) &&\n+\tNp1=$((N+1)) &&\n+\tgit rev-parse --verify master@{0} &&\n+\tgit rev-parse --verify master@{1} &&\n+\tgit rev-parse --verify master@{$Nm1} &&\n+\ttest_must_fail \"git rev-parse --verify master@{$N}\" &&\n+\ttest_must_fail \"git rev-parse --verify master@{$Np1}\"\n+'\n+\n test_done\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex af721f9..427c67e 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -66,4 +66,13 @@ test_expect_success 'incorrect file in :path and :N:path' '\n \tgrep \"fatal: Path '\"'\"'disk-only.txt'\"'\"' exists on disk, but not in the index.\" error\n '\n \n+test_expect_success 'invalid @{n} reference' '\n+\ttest_must_fail git rev-parse master@{99999} >output 2>error &&\n+\ttest \"$(cat output)\" = \"master@{99999}\" &&\n+\tgrep \"error: Log for [^ ]* only has [0-9][0-9]* entries.\" error  &&\n+\ttest_must_fail git rev-parse --verify master@{99999} >output 2>error &&\n+\ttest -z \"$(cat output)\" &&\n+\tgrep \"error: Log for [^ ]* only has [0-9][0-9]* entries.\" error\n+'\n+\n test_done\n-- \n1.7.2.1.156.gf148c\n"},{"id":"148721","messageId":"7vy6by2vlf.fsf@alter.siamese.dyndns.org","threadId":"24780","inReplyTo":"1282355022-17795-3-git-send-email-jon.seymour@gmail.com","subject":"Re: [PATCH 2/4] rev-parse: suppress duplicate log limit exceeded message.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-22T21:20:12Z","receivedAt":"2010-08-22T21:20:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jon Seymour <jon.seymour@gmail.com> writes:\n\n> With this change, git rev-parse master@{99999} reports:\n>\n>   master@{99999}\n>   error: Log for 'master' only has 166 entries.\n>   fatal: ambiguous argument 'master@{99999}': unknown revision or path not in the working tree.\n>   Use '--' to separate paths from revisions\n\nIf we are going to say \"fatal:\" and die at the end, I think we probably do\nnot want to say a separate \"error\" message.  Instead of adding a boolean\nparameter \"fail silently or warn?\", it may be better to make it a pointer\nto a strbuf and have it filled with error details (or a machine readable\nstruct and make it responsibility of the caller to generate a message).\n\nThen die_verify_filename can become, e.g.:\n\nstatic void NORETURN die_verify_filename(const char *prefix, const char *arg)\n{\n\tunsigned char sha1[20];\n\tunsigned mode;\n        struct strbuf e;\n\n        /* learn in what way is it bad? */\n\tget_sha1_with_mode_1(arg, sha1, &mode, &e, prefix);\n\n\t/* ... or fall back the most general message. */\n\tdie(\"ambiguous argument '%s': unknown revision or path not in the working tree.\\n\"\n\t    \"%s\"\n            \"Use '--' to separate paths from revisions\", arg, e.buf);\n}\n\nand we can later reuse the same mechanism to cover other kind of errors,\nnot just \"the log does not have that many entries\" error.  With your\n\"gently\" approach it may not be easy to do that without adding more\nparameters to all the functions in the codepath, no?\n\nI also wonder if this can simply become part of \"struct object_context\",\nwhich is to pass extra information in addition to the SHA-1 (which the\noriginal API returned) back to the caller.\n"},{"id":"148753","messageId":"AANLkTi=3=sumzcSFoCN6FUPQPzfQvfSim8KPRoXoT3tL@mail.gmail.com","threadId":"24780","inReplyTo":"7vy6by2vlf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/4] rev-parse: suppress duplicate log limit exceeded message.","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-23T14:59:58Z","receivedAt":"2010-08-23T14:59:58Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"On Mon, Aug 23, 2010 at 7:20 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jon Seymour <jon.seymour@gmail.com> writes:\n>\n>> With this change, git rev-parse master@{99999} reports:\n>>\n>>   master@{99999}\n>>   error: Log for 'master' only has 166 entries.\n>>   fatal: ambiguous argument 'master@{99999}': unknown revision or path not in the working tree.\n>>   Use '--' to separate paths from revisions\n>\n> If we are going to say \"fatal:\" and die at the end, I think we probably do\n> not want to say a separate \"error\" message.  Instead of adding a boolean\n> parameter \"fail silently or warn?\", it may be better to make it a pointer\n> to a strbuf and have it filled with error details (or a machine readable\n> struct and make it responsibility of the caller to generate a message).\n>\n\nI agree that it is not ideal that we have an error message and a fatal\nmessage in this case.\n\nIdeally,  I think we should simply be reporting the log limit exceeded\ncondition as a fatal condition on its own, and not reporting the\nambiguous ref catch all at all in this case. We have already decided\nthat the argument is a reference at this point and as such it is not\nreally ambiguous, it just happens to be out of bounds.\n\n> Then die_verify_filename can become, e.g.:\n>\n> static void NORETURN die_verify_filename(const char *prefix, const char *arg)\n> {\n>        unsigned char sha1[20];\n>        unsigned mode;\n>        struct strbuf e;\n>\n>        /* learn in what way is it bad? */\n>        get_sha1_with_mode_1(arg, sha1, &mode, &e, prefix);\n>\n>        /* ... or fall back the most general message. */\n>        die(\"ambiguous argument '%s': unknown revision or path not in the working tree.\\n\"\n>            \"%s\"\n>            \"Use '--' to separate paths from revisions\", arg, e.buf);\n> }\n>\n> and we can later reuse the same mechanism to cover other kind of errors,\n> not just \"the log does not have that many entries\" error.  With your\n> \"gently\" approach it may not be easy to do that without adding more\n> parameters to all the functions in the codepath, no?\n>\n\nI agree that the gently approach used here doesn't generalise to other\ncases. It seemed like the smallest change I could make that was\nreasonably consistent with existing code. I am happy to look at a more\ngeneral solution.\n\n> I also wonder if this can simply become part of \"struct object_context\",\n> which is to pass extra information in addition to the SHA-1 (which the\n> original API returned) back to the caller.\n>\n\nI'll have a look at this. object_context seems like a reasonably\ncoherent structure at the moment - do we risk diluting that coherence\nby attaching error reporting state to it?\n\nShould we consider passing an error callback function (+ struct)\naround so that the calling context can decide whether to directly\nreport errors at the point of detection or defer them until later?\nCalling contexts that don't care about reporting immediately can do\nso, calling contexts that do care can choose how to record errors into\nthe struct, as required.\n\njon.\n"},{"id":"148762","messageId":"7vr5hp1e6i.fsf@alter.siamese.dyndns.org","threadId":"24780","inReplyTo":"AANLkTi=3=sumzcSFoCN6FUPQPzfQvfSim8KPRoXoT3tL@mail.gmail.com","subject":"Re: [PATCH 2/4] rev-parse: suppress duplicate log limit exceeded message.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-23T16:33:57Z","receivedAt":"2010-08-23T16:33:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jon Seymour <jon.seymour@gmail.com> writes:\n\n> Ideally,  I think we should simply be reporting the log limit exceeded\n> condition as a fatal condition on its own, and not reporting the\n> ambiguous ref catch all at all in this case. We have already decided\n> that the argument is a reference at this point and as such it is not\n> really ambiguous, it just happens to be out of bounds.\n\nA very good point.  Would simply replacing that error-then-return-nonzero\nwith a die work?  If so we do not even need to worry about new mechanisms\nto cause different soft error conditions reported back to the caller to be\nprinted.\n"},{"id":"148797","messageId":"1282605103-8320-1-git-send-email-jon.seymour@gmail.com","threadId":"24780","inReplyTo":"7vr5hp1e6i.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2 0/2] rev-parse: strengthen validation of ref@{n} references","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-23T23:11:41Z","receivedAt":"2010-08-23T23:11:41Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"This series changes git rev-parse so that references of the form:\n\n\tref@{n}\n\ncause rev-parse to fail with a non-zero status code if n >= size of the \nreference log for ref.\n\nThe first commit implements the changed behaviour, the second commit \nupdates the tests to include tests for the changed behaviour.\n\nThis is the second revision of this series. It differs from the first\nin that it uses die rather than error to report the condition\nand thus exits early, without generating any output on stdout\nfor the failing argument. It also reports just a single fatal\nmessage instead of both an error message and a fatal message.\n\nThe original patch and this revision are both based on suggestions\nfrom Junio.\n\nJon Seymour (2):\n  rev-parse: exit with non-zero status if ref@{n} is not valid.\n  rev-parse: tests git rev-parse --verify master@{n}, for various n\n\n sha1_name.c                    |   20 +++++++++++---------\n t/t1503-rev-parse-verify.sh    |   11 +++++++++++\n t/t1506-rev-parse-diagnosis.sh |    9 +++++++++\n 3 files changed, 31 insertions(+), 9 deletions(-)\n\n-- \n1.7.2.1.99.geab11\n"},{"id":"148796","messageId":"1282605103-8320-2-git-send-email-jon.seymour@gmail.com","threadId":"24780","inReplyTo":"7vr5hp1e6i.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2 1/2] rev-parse: exit with non-zero status if ref@{n} is not valid.","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-23T23:11:42Z","receivedAt":"2010-08-23T23:11:42Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":">From Junio's e-mail:\n\n\"The current behaviour of ref@{...} syntax parser is suboptimal:\n\n    $ git rev-parse --verify jch@{99999} && echo true\n    warning: Log for 'jch' only has 1368 entries.\n    cfb88e9a8d4926b0011ae2dd67e1f57a98f4b768\n    true\n\nIt even knows that it is running off the cut-off point; it should just\ncause the caller to notice that fact.  I don't think changing it to error\nout should cause any harm to existing callers.\"\n\nWith this change:\n\n   $ git rev-parse --verify jch@{99999} || echo false\n   fatal: Log for 'jch' only has 1368 entries.\n   false\n\n   $ git rev-parse jch@{99999} || echo false\n   fatal: Log for 'jch' only has 1368 entries.\n   false\n\nSigned-off-by: Jon Seymour <jon.seymour@gmail.com>\n---\n sha1_name.c |   20 +++++++++++---------\n 1 files changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 4af94fa..d40ae48 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -342,7 +342,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1);\n \n static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n {\n-\tstatic const char *warning = \"warning: refname '%.*s' is ambiguous.\\n\";\n+\tstatic const char *warn_msg = \"warning: refname '%.*s' is ambiguous.\";\n \tchar *real_ref = NULL;\n \tint refs_found = 0;\n \tint at, reflog_len;\n@@ -390,7 +390,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\treturn -1;\n \n \tif (warn_ambiguous_refs && refs_found > 1)\n-\t\tfprintf(stderr, warning, len, str);\n+\t\tfprintf(stderr, warn_msg, len, str);\n \n \tif (reflog_len) {\n \t\tint nth, i;\n@@ -426,14 +426,16 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\tif (read_ref_at(real_ref, at_time, nth, sha1, NULL,\n \t\t\t\t&co_time, &co_tz, &co_cnt)) {\n \t\t\tif (at_time)\n-\t\t\t\tfprintf(stderr,\n-\t\t\t\t\t\"warning: Log for '%.*s' only goes \"\n-\t\t\t\t\t\"back to %s.\\n\", len, str,\n+\t\t\t\twarning(\"Log for '%.*s' only goes \"\n+\t\t\t\t\t\"back to %s.\", len, str,\n \t\t\t\t\tshow_date(co_time, co_tz, DATE_RFC2822));\n-\t\t\telse\n-\t\t\t\tfprintf(stderr,\n-\t\t\t\t\t\"warning: Log for '%.*s' only has \"\n-\t\t\t\t\t\"%d entries.\\n\", len, str, co_cnt);\n+\t\t\telse {\n+\t\t\t\tfree(real_ref);\n+\t\t\t\tdie(\"Log for '%.*s' only has %d entries.\",\n+\t\t\t\t\t\tlen,\n+\t\t\t\t\t\tstr,\n+\t\t\t\t\t\tco_cnt);\n+\t\t\t}\n \t\t}\n \t}\n \n-- \n1.7.2.1.99.geab11\n"},{"id":"148798","messageId":"1282605103-8320-3-git-send-email-jon.seymour@gmail.com","threadId":"24780","inReplyTo":"7vr5hp1e6i.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2 2/2] rev-parse: tests git rev-parse --verify master@{n}, for various n","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-23T23:11:43Z","receivedAt":"2010-08-23T23:11:43Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"This commit introduces tests that verify that rev-parse\nparses master@{n} correctly for various values of n less\nthan, equal to and greater than the number of revisions\nin the reference log.\n\nIn particular, these tests check that rev-parse exits with a\nnon-zero status code and prints a message of the\nfollowing form to stderr.\n\n    fatal: Log for [^ ]* only has [0-9][0-9]* entries.\n\nSigned-off-by: Jon Seymour <jon.seymour@gmail.com>\n---\n t/t1503-rev-parse-verify.sh    |   11 +++++++++++\n t/t1506-rev-parse-diagnosis.sh |    9 +++++++++\n 2 files changed, 20 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh\nindex cc65394..61092f7 100755\n--- a/t/t1503-rev-parse-verify.sh\n+++ b/t/t1503-rev-parse-verify.sh\n@@ -104,4 +104,15 @@ test_expect_success 'use --default' '\n \ttest_must_fail git rev-parse --verify --default bar\n '\n \n+test_expect_success 'master@{n} for various n' '\n+\tN=$(git reflog | wc -l) &&\n+\tNm1=$((N-1)) &&\n+\tNp1=$((N+1)) &&\n+\tgit rev-parse --verify master@{0} &&\n+\tgit rev-parse --verify master@{1} &&\n+\tgit rev-parse --verify master@{$Nm1} &&\n+\ttest_must_fail \"git rev-parse --verify master@{$N}\" &&\n+\ttest_must_fail \"git rev-parse --verify master@{$Np1}\"\n+'\n+\n test_done\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex af721f9..0eeeb0e 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -66,4 +66,13 @@ test_expect_success 'incorrect file in :path and :N:path' '\n \tgrep \"fatal: Path '\"'\"'disk-only.txt'\"'\"' exists on disk, but not in the index.\" error\n '\n \n+test_expect_success 'invalid @{n} reference' '\n+\ttest_must_fail git rev-parse master@{99999} >output 2>error &&\n+\ttest -z \"$(cat output)\" &&\n+\tgrep \"fatal: Log for [^ ]* only has [0-9][0-9]* entries.\" error  &&\n+\ttest_must_fail git rev-parse --verify master@{99999} >output 2>error &&\n+\ttest -z \"$(cat output)\" &&\n+\tgrep \"fatal: Log for [^ ]* only has [0-9][0-9]* entries.\" error\n+'\n+\n test_done\n-- \n1.7.2.1.99.geab11\n"},{"id":"148800","messageId":"20100824001454.GB17406@burratino","threadId":"24780","inReplyTo":"1282605103-8320-2-git-send-email-jon.seymour@gmail.com","subject":"Re: [PATCH v2 1/2] rev-parse: exit with non-zero status if ref@{n} is not valid.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-24T00:14:55Z","receivedAt":"2010-08-24T00:14:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jon Seymour wrote:\n\n> +++ b/sha1_name.c\n> @@ -342,7 +342,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1);\n>  \n>  static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n>  {\n> -\tstatic const char *warning = \"warning: refname '%.*s' is ambiguous.\\n\";\n> +\tstatic const char *warn_msg = \"warning: refname '%.*s' is ambiguous.\";\n\n$ git tag master\n$ bin-wrappers/git rev-parse master\nwarning: refname 'master' is ambiguous.cfee1ae8f0c56eed7d8ffa821f650789f8b11de2\n\nHere's a fixup.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\ndiff --git a/sha1_name.c b/sha1_name.c\nindex d40ae48..823e582 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -342,7 +342,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1);\n \n static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n {\n-\tstatic const char *warn_msg = \"warning: refname '%.*s' is ambiguous.\";\n+\tstatic const char *warn_msg = \"warning: refname '%.*s' is ambiguous.\\n\";\n \tchar *real_ref = NULL;\n \tint refs_found = 0;\n \tint at, reflog_len;\n-- \n"},{"id":"148812","messageId":"1282625564-12752-1-git-send-email-jon.seymour@gmail.com","threadId":"24780","inReplyTo":"20100824001454.GB17406@burratino","subject":"[PATCH v3 0/3] rev-parse: strengthen validation of ref@{n} references","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-24T04:52:41Z","receivedAt":"2010-08-24T04:52:41Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"This series changes git rev-parse so that references of the form:\n\n       ref@{n}\n\ncause rev-parse to fail with a non-zero status code if n >= size of the\nreference log for ref.\n\nThe first commit implements the changed behaviour; the second commit\nreplaces uses of fprintf (within sha1_name.c) with warning; \nthe third commit updates the tests to include tests for the changed behaviour.\n\nThis is the third revision of this series. It differs from the first\nin that it uses die rather than error to report the condition\nand thus exits early, without generating any output on stdout\nfor the failing argument. It also reports just a single fatal\nmessage instead of both an error message and a fatal message.\n\nThe original patch and this revision are both based on suggestions\nfrom Junio. Corrections to diagnostic output inspired by Jonathan Nieder's\nreview.\n\nJon Seymour (3):\n  rev-parse: exit with non-zero status if ref@{n} is not valid.\n  sha1_name.c: use warning in preference to fprintf(stderr\n  rev-parse: tests git rev-parse --verify master@{n}, for various n\n\n sha1_name.c                    |   20 +++++++++++---------\n t/t1503-rev-parse-verify.sh    |   11 +++++++++++\n t/t1506-rev-parse-diagnosis.sh |    9 +++++++++\n 3 files changed, 31 insertions(+), 9 deletions(-)\n\n-- \n1.7.2.1.100.gceecd\n"},{"id":"148814","messageId":"1282625564-12752-2-git-send-email-jon.seymour@gmail.com","threadId":"24780","inReplyTo":"20100824001454.GB17406@burratino","subject":"[PATCH v3 1/3] rev-parse: exit with non-zero status if ref@{n} is not valid.","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-24T04:52:42Z","receivedAt":"2010-08-24T04:52:42Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":">From Junio's e-mail:\n\n\"The current behaviour of ref@{...} syntax parser is suboptimal:\n\n    $ git rev-parse --verify jch@{99999} && echo true\n    warning: Log for 'jch' only has 1368 entries.\n    cfb88e9a8d4926b0011ae2dd67e1f57a98f4b768\n    true\n\nIt even knows that it is running off the cut-off point; it should just\ncause the caller to notice that fact.  I don't think changing it to error\nout should cause any harm to existing callers.\"\n\nWith this change:\n\n   $ git rev-parse --verify jch@{99999} || echo false\n   fatal: Log for 'jch' only has 1368 entries.\n   false\n\n   $ git rev-parse jch@{99999} || echo false\n   fatal: Log for 'jch' only has 1368 entries.\n   false\n\nSigned-off-by: Jon Seymour <jon.seymour@gmail.com>\n---\n sha1_name.c |   11 +++++++----\n 1 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 4af94fa..10a4efe 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -430,10 +430,13 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\t\t\t\t\"warning: Log for '%.*s' only goes \"\n \t\t\t\t\t\"back to %s.\\n\", len, str,\n \t\t\t\t\tshow_date(co_time, co_tz, DATE_RFC2822));\n-\t\t\telse\n-\t\t\t\tfprintf(stderr,\n-\t\t\t\t\t\"warning: Log for '%.*s' only has \"\n-\t\t\t\t\t\"%d entries.\\n\", len, str, co_cnt);\n+\t\t\telse {\n+\t\t\t\tfree(real_ref);\n+\t\t\t\tdie(\"Log for '%.*s' only has %d entries.\",\n+\t\t\t\t\t\tlen,\n+\t\t\t\t\t\tstr,\n+\t\t\t\t\t\tco_cnt);\n+\t\t\t}\n \t\t}\n \t}\n \n-- \n1.7.2.1.100.gceecd\n"},{"id":"148815","messageId":"1282625564-12752-3-git-send-email-jon.seymour@gmail.com","threadId":"24780","inReplyTo":"20100824001454.GB17406@burratino","subject":"[PATCH v3 2/3] sha1_name.c: use warning in preference to fprintf(stderr","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-24T04:52:43Z","receivedAt":"2010-08-24T04:52:43Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"This commit changes sha1_name.c to use warning instead of\nfprintf(stderr).\n\nTrailing newlines from message formats have been removed\nsince warning adds one itself.\n\nSigned-off-by: Jon Seymour <jon.seymour@gmail.com>\n---\n sha1_name.c |    9 ++++-----\n 1 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 10a4efe..3b8aea2 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -342,7 +342,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1);\n \n static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n {\n-\tstatic const char *warning = \"warning: refname '%.*s' is ambiguous.\\n\";\n+\tstatic const char *warn_msg = \"refname '%.*s' is ambiguous.\";\n \tchar *real_ref = NULL;\n \tint refs_found = 0;\n \tint at, reflog_len;\n@@ -390,7 +390,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\treturn -1;\n \n \tif (warn_ambiguous_refs && refs_found > 1)\n-\t\tfprintf(stderr, warning, len, str);\n+\t\twarning(warn_msg, len, str);\n \n \tif (reflog_len) {\n \t\tint nth, i;\n@@ -426,9 +426,8 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\tif (read_ref_at(real_ref, at_time, nth, sha1, NULL,\n \t\t\t\t&co_time, &co_tz, &co_cnt)) {\n \t\t\tif (at_time)\n-\t\t\t\tfprintf(stderr,\n-\t\t\t\t\t\"warning: Log for '%.*s' only goes \"\n-\t\t\t\t\t\"back to %s.\\n\", len, str,\n+\t\t\t\twarning(\"Log for '%.*s' only goes \"\n+\t\t\t\t\t\"back to %s.\", len, str,\n \t\t\t\t\tshow_date(co_time, co_tz, DATE_RFC2822));\n \t\t\telse {\n \t\t\t\tfree(real_ref);\n-- \n1.7.2.1.100.gceecd\n"},{"id":"148813","messageId":"1282625564-12752-4-git-send-email-jon.seymour@gmail.com","threadId":"24780","inReplyTo":"20100824001454.GB17406@burratino","subject":"[PATCH v3 3/3] rev-parse: tests git rev-parse --verify master@{n}, for various n","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-24T04:52:44Z","receivedAt":"2010-08-24T04:52:44Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"This commit introduces tests that verify that rev-parse\nparses master@{n} correctly for various values of n less\nthan, equal to and greater than the number of revisions\nin the reference log.\n\nIn particular, these tests check that rev-parse exits with a\nnon-zero status code and prints a message of the\nfollowing form to stderr.\n\n    fatal: Log for [^ ]* only has [0-9][0-9]* entries.\n\nSigned-off-by: Jon Seymour <jon.seymour@gmail.com>\n---\n t/t1503-rev-parse-verify.sh    |   11 +++++++++++\n t/t1506-rev-parse-diagnosis.sh |    9 +++++++++\n 2 files changed, 20 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh\nindex cc65394..61092f7 100755\n--- a/t/t1503-rev-parse-verify.sh\n+++ b/t/t1503-rev-parse-verify.sh\n@@ -104,4 +104,15 @@ test_expect_success 'use --default' '\n \ttest_must_fail git rev-parse --verify --default bar\n '\n \n+test_expect_success 'master@{n} for various n' '\n+\tN=$(git reflog | wc -l) &&\n+\tNm1=$((N-1)) &&\n+\tNp1=$((N+1)) &&\n+\tgit rev-parse --verify master@{0} &&\n+\tgit rev-parse --verify master@{1} &&\n+\tgit rev-parse --verify master@{$Nm1} &&\n+\ttest_must_fail \"git rev-parse --verify master@{$N}\" &&\n+\ttest_must_fail \"git rev-parse --verify master@{$Np1}\"\n+'\n+\n test_done\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex af721f9..0eeeb0e 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -66,4 +66,13 @@ test_expect_success 'incorrect file in :path and :N:path' '\n \tgrep \"fatal: Path '\"'\"'disk-only.txt'\"'\"' exists on disk, but not in the index.\" error\n '\n \n+test_expect_success 'invalid @{n} reference' '\n+\ttest_must_fail git rev-parse master@{99999} >output 2>error &&\n+\ttest -z \"$(cat output)\" &&\n+\tgrep \"fatal: Log for [^ ]* only has [0-9][0-9]* entries.\" error  &&\n+\ttest_must_fail git rev-parse --verify master@{99999} >output 2>error &&\n+\ttest -z \"$(cat output)\" &&\n+\tgrep \"fatal: Log for [^ ]* only has [0-9][0-9]* entries.\" error\n+'\n+\n test_done\n-- \n1.7.2.1.100.gceecd\n"}]}