Re: [PATCH v2] peel_onion(): add support for rev^{tag}
On Mon, Sep 02, 2013 at 01:42:31AM -0400, Richard Hansen wrote:
Complete the rev^{type} family of object specifiers by having
rev^{tag} dereference rev until a tag object is found (or fail if
unable).
At first glance this may not seem very useful, as commits, trees, and
blobs cannot be peeled to a tag, and a tag would just peel to itself.
However, this can be used to ensure that rev names a tag object:
$ git rev-parse --verify v1.8.4^{tag}
04f013dc38d7512eadb915eba22efc414f18b869
$ git rev-parse --verify master^{tag}
error: master^{tag}: expected tag type, but the object dereferences to
tree type
fatal: Needed a single revision
Signed-off-by: Richard Hansen rhan...@bbn.com
---
FWIW, this makes sense to me. 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.
Documentation/revisions.txt | 3 +++
sha1_name.c | 2 ++
Can you please add a test (probably in t1511) that checks the behavior,
similar to what you wrote in the commit message?
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 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.
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 (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).
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] peel_onion(): add support for rev^{tag}
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] peel_onion(): add support for rev^{tag}
On Tue, Sep 03, 2013 at 02:36:39PM -0400, Richard Hansen wrote: 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? I generally prefer if they are kept in the same thread, for two reasons: 1. People reading v3 may want to refer back to v2. You can provide a link to the previous discussion in the archive, but in-reply-to is a machine-readable way of doing the same thing (so it also works in people's MUAs without having to jump out to the archive). 2. People reading v2 may be doing so after v3 has been published, and of course v2 cannot have linked to v3 at the time it was written. If the reader has a threaded MUA (or uses gmane), then the two are grouped together, and they can easily see that reading v2 carefully may be a waste of time, as it is already obsolete. Between replying to the review versus the original patch, I don't have a preference (any decent reader should group the whole thread, which is enough to accomplish either of the above two). 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. I think what you have is fine (modulo dropping the true, which I somehow failed to notice on first read). It is more thorough, and I do not think it hurts the readability of the end result (i.e., I can still tell what the point of the test is). But I am happy either way (I only mentioned the simpler version because you asked). 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? I usually put them in the same commit for a small enhancement or fix like this. Seeing the test along with the change often helps the reader understand what the patch is doing by providing a concrete example. Sometimes for a set of changes that needs a large set of refactoring patches, I'll introduce a group of related failing tests at the beginning (using test_expect_failure), and then mark them as expect_success as they are fixed by individual commits. So you can use your judgement about how the split will communicate to the reviewer (and later readers of the commit history). But the one thing you _can't_ do is introduce a failing test and mark it as expect_success. We try to keep make test successful in each commit, which lets people bisect the history more easily. 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. I'll post a follow-up patch in a second. 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; That is a little manual and awkward for my tastes. skip_prefix() is an inline specifically so that the compiler _can_ optimize out the strlen in such cases, without us having to resort to writing the code ourselves. So if we cared about the strlen here (and I don't think we do), it would be cleaner to simply convert commit_type to a macro, static string literal, or static constant array. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
