On 2013-09-03 03:05, Jeff King wrote: > FWIW, this makes sense to me. Thank you for the feedback. I posted a reroll of the patch that you've already replied to, but for the benefit of others searching the mailing list archive, v3 can be found at <http://thread.gmane.org/gmane.comp.version-control.git/233752>. I have a patch submission question: Is it OK that I didn't use the '--in-reply-to' argument to 'git send-email' when I sent the v3 reroll? Should I have marked it as a reply to the v2 email? Or should I have marked it as a reply to your review of the v2 email? > You can already accomplish the same thing > by checking the output of $(git cat-file -t $name), but this is a > natural extension of the other ^{} rules, and I can see making some > callers more natural. Exactly. I updated the commit message to explain this so that other people know why it might be a good feature to have. > Can you please add a test (probably in t1511) that checks the behavior, > similar to what you wrote in the commit message? Done. I see by your reply that my fear of going a bit overboard in the test was justified. :) I don't mind rerolling if you'd prefer a simpler test. For future reference, is there a preference for putting tests of a new feature in a separate commit? In the same commit? Doesn't really matter? >> diff --git a/sha1_name.c b/sha1_name.c >> index 65ad066..6dc496d 100644 >> --- a/sha1_name.c >> +++ b/sha1_name.c >> @@ -679,6 +679,8 @@ static int peel_onion(const char *name, int len, unsigned char *sha1) >> sp++; /* beginning of type name, or closing brace for empty */ >> if (!strncmp(commit_type, sp, 6) && sp[6] == '}') >> expected_type = OBJ_COMMIT; >> + else if (!strncmp(tag_type, sp, 3) && sp[3] == '}') >> + expected_type = OBJ_TAG; > > This is not a problem you are introducing to this code, but the use of > opaque constants like commit_type along with the magic number "6" > assuming that it contains "commit" seems like a maintenance nightmare > (the only thing saving us is that it will almost certainly never change > from "commit"; but then why do we have the opaque type in the first > place?). I agree. I didn't address this in the reroll. > > I wonder if we could do better with: > > #define COMMIT_TYPE "commit" > ... > if (!strncmp(COMMIT_TYPE, sp, strlen(COMMIT_TYPE)) > && sp[strlen(COMMIT_TYPE)] == '}') > > Any compiler worth its salt will optimize the strlen on a string > constant into a constant itself. The length makes it a bit less > readable, though. True, and I'm not a huge fan of macros. > > I wonder if we could do even better with: > > const char *x; > ... > if ((x = skip_prefix(sp, commit_type)) && *x == '}') > > which avoids the magic lengths altogether Not bad, especially since skip_prefix() already exists. > (though the compiler cannot > optimize out the strlen call inside skip_prefix, because we declare > commit_type and friends as an extern. It probably doesn't matter in > peel_onion, though, which should not generally be performance critical > anyway). Yeah, I can't see performance being a problem there. There's also this awkward approach, which would avoid strlen() altogether: commit.h: extern const char *commit_type; extern const size_t commit_type_len; commit.c: const char commit_type_array[] = "commit"; const char *commit_type = &commit_type_array[0]; const size_t commit_type_len = sizeof(commit_type_array) - 1; sha1_name.c peel_onion(): if (!strncmp(commit_type, sp, commit_type_len) && sp[commit_type_len] == '}') but I prefer your skip_prefix() suggestion. -Richard -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html