{"thread":{"id":"34827","subject":"[PATCH v2] peel_onion(): add support for ^{tag}","startedAt":"2013-09-02T05:42:31Z","lastAt":"2013-09-03T20:07:37Z","messageCount":4,"participants":["Richard Hansen","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"226494","messageId":"1378100551-892-1-git-send-email-rhansen@bbn.com","threadId":"34827","inReplyTo":null,"subject":"[PATCH v2] peel_onion(): add support for ^{tag}","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2013-09-02T05:42:31Z","receivedAt":"2013-09-02T05:42:31Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"Complete the ^{} family of object specifiers by having\n^{tag} dereference until a tag object is found (or fail if\nunable).\n\nAt first glance this may not seem very useful, as commits, trees, and\nblobs cannot be peeled to a tag, and a tag would just peel to itself.\nHowever, this can be used to ensure that names a tag object:\n\n $ git rev-parse --verify v1.8.4^{tag}\n 04f013dc38d7512eadb915eba22efc414f18b869\n $ git rev-parse --verify master^{tag}\n error: master^{tag}: expected tag type, but the object dereferences to tree type\n fatal: Needed a single revision\n\nSigned-off-by: Richard Hansen \n---\nChanges since v1 (2013-06-18, see\n):\n * improved commit message\n * added usage note to gitrevisions[7]\n\n Documentation/revisions.txt | 3 +++\n sha1_name.c | 2 ++\n 2 files changed, 5 insertions(+)\n\ndiff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\nindex d477b3f..b3322ad 100644\n--- a/Documentation/revisions.txt\n+++ b/Documentation/revisions.txt\n@@ -121,6 +121,9 @@ some output processing may assume ref names in UTF-8.\n object that exists, without requiring 'rev' to be a tag, and\n without dereferencing 'rev'; because a tag is already an object,\n it does not have to be dereferenced even once to get to an object.\n++\n+'rev{caret}\\{tag\\}' can be used to ensure that 'rev' identifies an\n+existing tag object.\n \n '{caret}\\{\\}', e.g. 'v0.99.8{caret}\\{\\}'::\n A suffix '{caret}' followed by an empty brace pair\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 65ad066..6dc496d 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -679,6 +679,8 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)\n \tsp++; /* beginning of type name, or closing brace for empty */\n \tif (!strncmp(commit_type, sp, 6) && sp[6] == '}')\n \t\texpected_type = OBJ_COMMIT;\n+\telse if (!strncmp(tag_type, sp, 3) && sp[3] == '}')\n+\t\texpected_type = OBJ_TAG;\n \telse if (!strncmp(tree_type, sp, 4) && sp[4] == '}')\n \t\texpected_type = OBJ_TREE;\n \telse if (!strncmp(blob_type, sp, 4) && sp[4] == '}')\n-- \n1.8.4\n"},{"id":"226582","messageId":"20130903070546.GC3608@sigill.intra.peff.net","threadId":"34827","inReplyTo":"1378100551-892-1-git-send-email-rhansen@bbn.com","subject":"Re: [PATCH v2] peel_onion(): add support for ^{tag}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-03T07:05:46Z","receivedAt":"2013-09-03T07:05:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 02, 2013 at 01:42:31AM -0400, Richard Hansen wrote:\n\n> Complete the ^{} family of object specifiers by having\n> ^{tag} dereference until a tag object is found (or fail if\n> unable).\n> \n> At first glance this may not seem very useful, as commits, trees, and\n> blobs cannot be peeled to a tag, and a tag would just peel to itself.\n> However, this can be used to ensure that names a tag object:\n> \n> $ git rev-parse --verify v1.8.4^{tag}\n> 04f013dc38d7512eadb915eba22efc414f18b869\n> $ git rev-parse --verify master^{tag}\n> error: master^{tag}: expected tag type, but the object dereferences to tree type\n> fatal: Needed a single revision\n> \n> Signed-off-by: Richard Hansen \n> ---\n\nFWIW, this makes sense to me. You can already accomplish the same thing\nby checking the output of $(git cat-file -t $name), but this is a\nnatural extension of the other ^{} rules, and I can see making some\ncallers more natural.\n\n> Documentation/revisions.txt | 3 +++\n> sha1_name.c | 2 ++\n\nCan you please add a test (probably in t1511) that checks the behavior,\nsimilar to what you wrote in the commit message?\n\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 65ad066..6dc496d 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -679,6 +679,8 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)\n> \tsp++; /* beginning of type name, or closing brace for empty */\n> \tif (!strncmp(commit_type, sp, 6) && sp[6] == '}')\n> \t\texpected_type = OBJ_COMMIT;\n> +\telse if (!strncmp(tag_type, sp, 3) && sp[3] == '}')\n> +\t\texpected_type = OBJ_TAG;\n\nThis is not a problem you are introducing to this code, but the use of\nopaque constants like commit_type along with the magic number \"6\"\nassuming that it contains \"commit\" seems like a maintenance nightmare\n(the only thing saving us is that it will almost certainly never change\nfrom \"commit\"; but then why do we have the opaque type in the first\nplace?).\n\nI wonder if we could do better with:\n\n #define COMMIT_TYPE \"commit\"\n ...\n if (!strncmp(COMMIT_TYPE, sp, strlen(COMMIT_TYPE))\n && sp[strlen(COMMIT_TYPE)] == '}')\n\nAny compiler worth its salt will optimize the strlen on a string\nconstant into a constant itself. The length makes it a bit less\nreadable, though.\n\nI wonder if we could do even better with:\n\n const char *x;\n ...\n if ((x = skip_prefix(sp, commit_type)) && *x == '}')\n\nwhich avoids the magic lengths altogether (though the compiler cannot\noptimize out the strlen call inside skip_prefix, because we declare\ncommit_type and friends as an extern. It probably doesn't matter in\npeel_onion, though, which should not generally be performance critical\nanyway).\n\n-Peff\n"},{"id":"226651","messageId":"52262C37.3030406@bbn.com","threadId":"34827","inReplyTo":"20130903070546.GC3608@sigill.intra.peff.net","subject":"Re: [PATCH v2] peel_onion(): add support for ^{tag}","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2013-09-03T18:36:39Z","receivedAt":"2013-09-03T18:36:39Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"On 2013-09-03 03:05, Jeff King wrote:\n> FWIW, this makes sense to me.\n\nThank you for the feedback. I posted a reroll of the patch that you've\nalready replied to, but for the benefit of others searching the mailing\nlist archive, v3 can be found at\n.\n\nI have a patch submission question: Is it OK that I didn't use the\n'--in-reply-to' argument to 'git send-email' when I sent the v3 reroll?\n Should I have marked it as a reply to the v2 email? Or should I have\nmarked it as a reply to your review of the v2 email?\n\n> You can already accomplish the same thing\n> by checking the output of $(git cat-file -t $name), but this is a\n> natural extension of the other ^{} rules, and I can see making some\n> callers more natural.\n\nExactly. I updated the commit message to explain this so that other\npeople know why it might be a good feature to have.\n\n> Can you please add a test (probably in t1511) that checks the behavior,\n> similar to what you wrote in the commit message?\n\nDone. I see by your reply that my fear of going a bit overboard in the\ntest was justified. :) I don't mind rerolling if you'd prefer a\nsimpler test.\n\nFor future reference, is there a preference for putting tests of a new\nfeature in a separate commit? In the same commit? Doesn't really matter?\n\n>> diff --git a/sha1_name.c b/sha1_name.c\n>> index 65ad066..6dc496d 100644\n>> --- a/sha1_name.c\n>> +++ b/sha1_name.c\n>> @@ -679,6 +679,8 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)\n>> \tsp++; /* beginning of type name, or closing brace for empty */\n>> \tif (!strncmp(commit_type, sp, 6) && sp[6] == '}')\n>> \t\texpected_type = OBJ_COMMIT;\n>> +\telse if (!strncmp(tag_type, sp, 3) && sp[3] == '}')\n>> +\t\texpected_type = OBJ_TAG;\n> \n> This is not a problem you are introducing to this code, but the use of\n> opaque constants like commit_type along with the magic number \"6\"\n> assuming that it contains \"commit\" seems like a maintenance nightmare\n> (the only thing saving us is that it will almost certainly never change\n> from \"commit\"; but then why do we have the opaque type in the first\n> place?).\n\nI agree. I didn't address this in the reroll.\n\n> \n> I wonder if we could do better with:\n> \n> #define COMMIT_TYPE \"commit\"\n> ...\n> if (!strncmp(COMMIT_TYPE, sp, strlen(COMMIT_TYPE))\n> && sp[strlen(COMMIT_TYPE)] == '}')\n> \n> Any compiler worth its salt will optimize the strlen on a string\n> constant into a constant itself. The length makes it a bit less\n> readable, though.\n\nTrue, and I'm not a huge fan of macros.\n\n> \n> I wonder if we could do even better with:\n> \n> const char *x;\n> ...\n> if ((x = skip_prefix(sp, commit_type)) && *x == '}')\n> \n> which avoids the magic lengths altogether\n\nNot bad, especially since skip_prefix() already exists.\n\n> (though the compiler cannot\n> optimize out the strlen call inside skip_prefix, because we declare\n> commit_type and friends as an extern. It probably doesn't matter in\n> peel_onion, though, which should not generally be performance critical\n> anyway).\n\nYeah, I can't see performance being a problem there.\n\nThere's also this awkward approach, which would avoid strlen() altogether:\n\ncommit.h:\n\n extern const char *commit_type;\n extern const size_t commit_type_len;\n\ncommit.c:\n\n const char commit_type_array[] = \"commit\";\n const char *commit_type = &commit_type_array[0];\n const size_t commit_type_len = sizeof(commit_type_array) - 1;\n\nsha1_name.c peel_onion():\n\n if (!strncmp(commit_type, sp, commit_type_len)\n && sp[commit_type_len] == '}')\n\nbut I prefer your skip_prefix() suggestion.\n\n-Richard\n"},{"id":"226667","messageId":"20130903200737.GA18928@sigill.intra.peff.net","threadId":"34827","inReplyTo":"52262C37.3030406@bbn.com","subject":"Re: [PATCH v2] peel_onion(): add support for ^{tag}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-03T20:07:37Z","receivedAt":"2013-09-03T20:07:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 03, 2013 at 02:36:39PM -0400, Richard Hansen wrote:\n\n> I have a patch submission question: Is it OK that I didn't use the\n> '--in-reply-to' argument to 'git send-email' when I sent the v3 reroll?\n> Should I have marked it as a reply to the v2 email? Or should I have\n> marked it as a reply to your review of the v2 email?\n\nI generally prefer if they are kept in the same thread, for two reasons:\n\n 1. People reading v3 may want to refer back to v2. You can provide a\n link to the previous discussion in the archive, but in-reply-to is\n a machine-readable way of doing the same thing (so it also works in\n people's MUAs without having to jump out to the archive).\n\n 2. People reading v2 may be doing so after v3 has been published, and\n of course v2 cannot have linked to v3 at the time it was written.\n If the reader has a threaded MUA (or uses gmane), then the two are\n grouped together, and they can easily see that reading v2 carefully\n may be a waste of time, as it is already obsolete.\n\nBetween replying to the review versus the original patch, I don't have a\npreference (any decent reader should group the whole thread, which is\nenough to accomplish either of the above two).\n\n> Done. I see by your reply that my fear of going a bit overboard in the\n> test was justified. :) I don't mind rerolling if you'd prefer a\n> simpler test.\n\nI think what you have is fine (modulo dropping the \"&& true\", which I\nsomehow failed to notice on first read). It is more thorough, and I do\nnot think it hurts the readability of the end result (i.e., I can still\ntell what the point of the test is). But I am happy either way (I only\nmentioned the simpler version because you asked).\n\n> For future reference, is there a preference for putting tests of a new\n> feature in a separate commit? In the same commit? Doesn't really matter?\n\nI usually put them in the same commit for a small enhancement or fix\nlike this. Seeing the test along with the change often helps the reader\nunderstand what the patch is doing by providing a concrete example.\n\nSometimes for a set of changes that needs a large set of refactoring\npatches, I'll introduce a group of related failing tests at the\nbeginning (using test_expect_failure), and then mark them as\nexpect_success as they are fixed by individual commits.\n\nSo you can use your judgement about how the split will communicate to\nthe reviewer (and later readers of the commit history). But the one\nthing you _can't_ do is introduce a failing test and mark it as\nexpect_success. We try to keep \"make test\" successful in each commit,\nwhich lets people bisect the history more easily.\n\n> > I wonder if we could do even better with:\n> > \n> > const char *x;\n> > ...\n> > if ((x = skip_prefix(sp, commit_type)) && *x == '}')\n> > \n> > which avoids the magic lengths altogether\n> \n> Not bad, especially since skip_prefix() already exists.\n\nI'll post a follow-up patch in a second.\n\n> There's also this awkward approach, which would avoid strlen() altogether:\n> \n> commit.h:\n> \n> extern const char *commit_type;\n> extern const size_t commit_type_len;\n> \n> commit.c:\n> \n> const char commit_type_array[] = \"commit\";\n> const char *commit_type = &commit_type_array[0];\n> const size_t commit_type_len = sizeof(commit_type_array) - 1;\n\nThat is a little manual and awkward for my tastes. skip_prefix() is\nan inline specifically so that the compiler _can_ optimize out the\nstrlen in such cases, without us having to resort to writing the code\nourselves. So if we cared about the strlen here (and I don't think we\ndo), it would be cleaner to simply convert commit_type to a macro,\nstatic string literal, or static constant array.\n\n-Peff\n"}]}