# [PATCH 1/3] get_sha1_oneline: do not leak or double free 17 messages from 2010-12-13 to 2010-12-16. Participants: Nguyễn Thái Ngọc Duy, Nguyen Thai Ngoc Duy, Jonathan Nieder, Junio C Hamano. Thread: https://gitlist.dev/t/26033 ## Nguyễn Thái Ngọc Duy, 2010-12-13 03:01 Subject: [PATCH 1/3] get_sha1_oneline: do not leak or double free Message-ID: <1292209275-17451-1-git-send-email-pclouds@gmail.com> URL: https://gitlist.dev/e/1292209275-17451-1-git-send-email-pclouds%40gmail.com ``` Double free can happen when commit->buffer == NULL in the first iteration, then != NULL in the next two iterations. Signed-off-by: Nguyễn Thái Ngọc Duy --- sha1_name.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/sha1_name.c b/sha1_name.c index 2c3a5fb..13ee6f5 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -718,13 +718,13 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1) commit = pop_most_recent_commit(&list, ONELINE_SEEN); if (!parse_object(commit->object.sha1)) continue; - free(temp_commit_buffer); if (commit->buffer) p = commit->buffer; else { p = read_sha1_file(commit->object.sha1, &type, &size); if (!p) continue; + free(temp_commit_buffer); temp_commit_buffer = p; } if (!(p = strstr(p, "\n\n"))) @@ -740,6 +740,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1) free_commit_list(list); for (l = backup; l; l = l->next) clear_commit_marks(l->item, ONELINE_SEEN); + free_commit_list(backup); return retval; } -- 1.7.3.3.476.g10a82 ``` ## Nguyễn Thái Ngọc Duy, 2010-12-13 03:01 Subject: [PATCH 2/3] get_sha1_oneline: let callers initialize the commit tips for traverse Message-ID: <1292209275-17451-2-git-send-email-pclouds@gmail.com> URL: https://gitlist.dev/e/1292209275-17451-2-git-send-email-pclouds%40gmail.com In-Reply-To: <1292209275-17451-1-git-send-email-pclouds@gmail.com> ``` This gives callers more control, i.e. which ref will be searched from. get_sha1_oneline takes care of ONELINE_SEEN marks (which is now TMP_MARK) Signed-off-by: Nguyễn Thái Ngọc Duy --- sha1_name.c | 62 +++++++++++++++++++++++++++++++--------------------------- 1 files changed, 33 insertions(+), 29 deletions(-) diff --git a/sha1_name.c b/sha1_name.c index 13ee6f5..3c2c61c 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -6,6 +6,8 @@ #include "tree-walk.h" #include "refs.h" #include "remote.h" +#include "diff.h" +#include "revision.h" static int find_short_object_filename(int len, const char *name, unsigned char *sha1) { @@ -669,30 +671,10 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1) * For future extension, ':/!' is reserved. If you want to match a message * beginning with a '!', you have to repeat the exclamation mark. */ -#define ONELINE_SEEN (1u<<20) - -static int handle_one_ref(const char *path, - const unsigned char *sha1, int flag, void *cb_data) -{ - struct commit_list **list = cb_data; - struct object *object = parse_object(sha1); - if (!object) - return 0; - if (object->type == OBJ_TAG) { - object = deref_tag(object, path, strlen(path)); - if (!object) - return 0; - } - if (object->type != OBJ_COMMIT) - return 0; - insert_by_date((struct commit *)object, list); - object->flags |= ONELINE_SEEN; - return 0; -} - -static int get_sha1_oneline(const char *prefix, unsigned char *sha1) +static int get_sha1_oneline(const char *prefix, unsigned char *sha1, + struct commit_list *list) { - struct commit_list *list = NULL, *backup = NULL, *l; + struct commit_list *backup = NULL, *l; int retval = -1; char *temp_commit_buffer = NULL; regex_t regex; @@ -706,16 +688,17 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1) if (regcomp(®ex, prefix, REG_EXTENDED)) die("Invalid search pattern: %s", prefix); - for_each_ref(handle_one_ref, &list); - for (l = list; l; l = l->next) + for (l = list; l; l = l->next) { + l->item->object.flags |= TMP_MARK; commit_list_insert(l->item, &backup); + } while (list) { char *p; struct commit *commit; enum object_type type; unsigned long size; - commit = pop_most_recent_commit(&list, ONELINE_SEEN); + commit = pop_most_recent_commit(&list, TMP_MARK); if (!parse_object(commit->object.sha1)) continue; if (commit->buffer) @@ -739,7 +722,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1) free(temp_commit_buffer); free_commit_list(list); for (l = backup; l; l = l->next) - clear_commit_marks(l->item, ONELINE_SEEN); + clear_commit_marks(l->item, TMP_MARK); free_commit_list(backup); return retval; } @@ -1067,6 +1050,24 @@ int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, return ret; } +static int handle_one_ref(const char *path, + const unsigned char *sha1, int flag, void *cb_data) +{ + struct commit_list **list = cb_data; + struct object *object = parse_object(sha1); + if (!object) + return 0; + if (object->type == OBJ_TAG) { + object = deref_tag(object, path, strlen(path)); + if (!object) + return 0; + } + if (object->type != OBJ_COMMIT) + return 0; + insert_by_date((struct commit *)object, list); + return 0; +} + int get_sha1_with_context_1(const char *name, unsigned char *sha1, struct object_context *oc, int gently, const char *prefix) @@ -1089,9 +1090,12 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1, int stage = 0; struct cache_entry *ce; int pos; - if (namelen > 2 && name[1] == '/') + if (namelen > 2 && name[1] == '/') { + struct commit_list *list = NULL; + for_each_ref(handle_one_ref, &list); /* don't need mode for commit */ - return get_sha1_oneline(name + 2, sha1); + return get_sha1_oneline(name + 2, sha1, list); + } if (namelen < 3 || name[2] != ':' || name[1] < '0' || '3' < name[1]) -- 1.7.3.3.476.g10a82 ``` ## Nguyễn Thái Ngọc Duy, 2010-12-13 03:01 Subject: [PATCH 3/3] get_sha1: support ref^{/regex} syntax Message-ID: <1292209275-17451-3-git-send-email-pclouds@gmail.com> URL: https://gitlist.dev/e/1292209275-17451-3-git-send-email-pclouds%40gmail.com In-Reply-To: <1292209275-17451-1-git-send-email-pclouds@gmail.com> ``` This works like :/ syntax, but only limited to one ref. Signed-off-by: Nguyễn Thái Ngọc Duy --- Documentation/revisions.txt | 6 +++ sha1_name.c | 34 +++++++++++++------ t/t1511-rev-parse-caret.sh | 73 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 11 deletions(-) create mode 100755 t/t1511-rev-parse-caret.sh diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index 3d4b79c..174fa8e 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -106,6 +106,12 @@ the `$GIT_DIR/refs` directory or from the `$GIT_DIR/packed-refs` file. and dereference the tag recursively until a non-tag object is found. +* A suffix '{caret}' to a revision parameter followed by a brace + pair that contains a text led by a slash (e.g. `HEAD^{/fix nasty bug}`): + this is the same as `:/fix nasty bug` syntax below except that + it returns the youngest matching commit which is reachable from + the ref before '{caret}'. + * A colon, followed by a slash, followed by a text (e.g. `:/fix nasty bug`): this names a commit whose commit message matches the specified regular expression. This name returns the youngest matching commit which is diff --git a/sha1_name.c b/sha1_name.c index 3c2c61c..a7e1bcb 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -529,6 +529,7 @@ struct object *peel_to_type(const char *name, int namelen, } } +static int get_sha1_oneline(const char *, unsigned char *, struct commit_list *); static int peel_onion(const char *name, int len, unsigned char *sha1) { unsigned char outer[20]; @@ -564,6 +565,8 @@ static int peel_onion(const char *name, int len, unsigned char *sha1) expected_type = OBJ_BLOB; else if (sp[0] == '}') expected_type = OBJ_NONE; + else if (sp[0] == '/') + expected_type = OBJ_COMMIT; else return -1; @@ -578,19 +581,28 @@ static int peel_onion(const char *name, int len, unsigned char *sha1) if (!o || (!o->parsed && !parse_object(o->sha1))) return -1; hashcpy(sha1, o->sha1); + return 0; } - else { - /* - * At this point, the syntax look correct, so - * if we do not get the needed object, we should - * barf. - */ - o = peel_to_type(name, len, o, expected_type); - if (o) { - hashcpy(sha1, o->sha1); - return 0; - } + + /* + * At this point, the syntax look correct, so if we do not get + * the needed object, we should barf. + */ + o = peel_to_type(name, len, o, expected_type); + if (!o) return -1; + + hashcpy(sha1, o->sha1); + if (sp[0] == '/') { /* ^{/foo} */ + struct commit_list *list = NULL; + char *prefix; + int ret; + + commit_list_insert((struct commit *)o, &list); + prefix = xstrndup(sp + 1, name + len - 1 - (sp + 1)); + ret = get_sha1_oneline(prefix, sha1, list); + free(prefix); + return ret; } return 0; } diff --git a/t/t1511-rev-parse-caret.sh b/t/t1511-rev-parse-caret.sh new file mode 100755 index 0000000..5c8439c --- /dev/null +++ b/t/t1511-rev-parse-caret.sh @@ -0,0 +1,73 @@ +#!/bin/sh + +test_description='tests for ref^{stuff}' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo blob >a-blob && + git tag -a -m blob blob-tag `git hash-object -w a-blob` + mkdir a-tree && + echo moreblobs >a-tree/another-blob && + git add . && + TREE_SHA1=`git write-tree` && + git tag -a -m tree tree-tag "$TREE_SHA1" && + git commit -m Initial && + git tag -a -m commit commit-tag && + git branch ref && + git checkout master && + echo modified >>a-blob && + git add -u && + git commit -m Modified +' + +test_expect_success 'ref^{non-existent}' ' + test_must_fail git rev-parse ref^{non-existent} +' + +test_expect_success 'ref^{}' ' + git rev-parse ref >expected && + git rev-parse ref^{} >actual && + test_cmp expected actual && + git rev-parse commit-tag^{} >actual && + test_cmp expected actual +' + +test_expect_success 'ref^{commit}' ' + git rev-parse ref >expected && + git rev-parse ref^{commit} >actual && + test_cmp expected actual && + git rev-parse commit-tag^{commit} >actual && + test_cmp expected actual && + test_must_fail git rev-parse tree-tag^{commit} && + test_must_fail git rev-parse blob-tag^{commit} +' + +test_expect_success 'ref^{tree}' ' + echo $TREE_SHA1 >expected && + git rev-parse ref^{tree} >actual && + test_cmp expected actual && + git rev-parse commit-tag^{tree} >actual && + test_cmp expected actual && + git rev-parse tree-tag^{tree} >actual && + test_cmp expected actual && + test_must_fail git rev-parse blob-tag^{tree} +' + +test_expect_success 'ref^{/}' ' + git rev-parse master >expected && + git rev-parse master^{/} >actual && + test_cmp expected actual +' + +test_expect_success 'ref^{/non-existent}' ' + test_must_fail git rev-parse master^{/non-existent} +' + +test_expect_success 'ref^{/Initial}' ' + git rev-parse ref >expected && + git rev-parse master^{/Initial} >actual && + test_cmp expected actual +' + +test_done -- 1.7.3.3.476.g10a82 ``` ## Nguyen Thai Ngoc Duy, 2010-12-13 03:10 Subject: Re: [PATCH 3/3] get_sha1: support ref^{/regex} syntax Message-ID: URL: https://gitlist.dev/e/AANLkTik2M%3DPPCXqtepRhntVYofrA0PUgzBLvdMNeeP5P%40mail.gmail.com In-Reply-To: <1292209275-17451-3-git-send-email-pclouds@gmail.com> ``` 2010/12/13 Nguyễn Thái Ngọc Duy : > This works like :/ syntax, but only limited to one ref. Argh.. I read Jonthan's comment too late. Maybe "limited to one commit tip (of a DAG)"? -- Duy ``` ## Jonathan Nieder, 2010-12-13 04:40 Subject: Re: [PATCH 3/3] get_sha1: support ref^{/regex} syntax Message-ID: <20101213044031.GA20812@burratino> URL: https://gitlist.dev/e/20101213044031.GA20812%40burratino In-Reply-To: ``` Nguyen Thai Ngoc Duy wrote: > 2010/12/13 Nguyễn Thái Ngọc Duy : >> This works like :/ syntax, but only limited to one ref. > > Argh.. I read Jonthan's comment too late. Maybe "limited to one commit > tip (of a DAG)"? As long as it doesn't filter into the documentation, I don't really mind. :) ``` ## Junio C Hamano, 2010-12-13 06:12 Subject: Re: [PATCH 1/3] get_sha1_oneline: do not leak or double free Message-ID: <7v1v5m6w26.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1v5m6w26.fsf%40alter.siamese.dyndns.org In-Reply-To: <1292209275-17451-1-git-send-email-pclouds@gmail.com> ``` Nguyễn Thái Ngọc Duy writes: > Double free can happen when commit->buffer == NULL in the first > iteration, then != NULL in the next two iterations. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > sha1_name.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) Thanks. First the later hunk: > diff --git a/sha1_name.c b/sha1_name.c > index 2c3a5fb..13ee6f5 100644 > --- a/sha1_name.c > +++ b/sha1_name.c > @@ -740,6 +740,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1) > free_commit_list(list); > for (l = backup; l; l = l->next) > clear_commit_marks(l->item, ONELINE_SEEN); > + free_commit_list(backup); > return retval; > } This is necessary, but is unrelated to the topic, no? > @@ -718,13 +718,13 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1) > commit = pop_most_recent_commit(&list, ONELINE_SEEN); > if (!parse_object(commit->object.sha1)) > continue; > - free(temp_commit_buffer); > if (commit->buffer) > p = commit->buffer; > else { > p = read_sha1_file(commit->object.sha1, &type, &size); > if (!p) > continue; > + free(temp_commit_buffer); > temp_commit_buffer = p; > } > if (!(p = strstr(p, "\n\n"))) This looks very convoluted. I think the "temp-commit-buffer with a lifetime one iteration more than the loop body itself" is merely a misguided attempt to avoid sprinkling many free() calls inside the loop that has irregular exit points with continue and break. If you rewrite the loop to have more regular structure, there is no reason to have such a temporary variable with tricky lifespan. I think the following is easier to read and conveys what the code is trying to do more clearly. No? sha1_name.c | 24 +++++++++++++----------- 1 files changed, 13 insertions(+), 11 deletions(-) diff --git a/sha1_name.c b/sha1_name.c index 2c3a5fb..2cc7a42 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -693,8 +693,7 @@ static int handle_one_ref(const char *path, static int get_sha1_oneline(const char *prefix, unsigned char *sha1) { struct commit_list *list = NULL, *backup = NULL, *l; - int retval = -1; - char *temp_commit_buffer = NULL; + int found = 0; regex_t regex; if (prefix[0] == '!') { @@ -710,37 +709,40 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1) for (l = list; l; l = l->next) commit_list_insert(l->item, &backup); while (list) { - char *p; + char *p, *to_free = NULL; struct commit *commit; enum object_type type; unsigned long size; + int matches; commit = pop_most_recent_commit(&list, ONELINE_SEEN); if (!parse_object(commit->object.sha1)) continue; - free(temp_commit_buffer); if (commit->buffer) p = commit->buffer; else { p = read_sha1_file(commit->object.sha1, &type, &size); if (!p) continue; - temp_commit_buffer = p; + to_free = p; } - if (!(p = strstr(p, "\n\n"))) - continue; - if (!regexec(®ex, p + 2, 0, NULL, 0)) { + + p = strstr(p, "\n\n"); + matches = p && !regexec(®ex, p + 2, 0, NULL, 0); + free(to_free); + + if (matches) { hashcpy(sha1, commit->object.sha1); - retval = 0; + found = 1; break; } } regfree(®ex); - free(temp_commit_buffer); free_commit_list(list); for (l = backup; l; l = l->next) clear_commit_marks(l->item, ONELINE_SEEN); - return retval; + free_commit_list(backup); + return found ? 0 : -1; } struct grab_nth_branch_switch_cbdata { ``` ## Junio C Hamano, 2010-12-13 06:19 Subject: Re: [PATCH 1/3] get_sha1_oneline: do not leak or double free Message-ID: <7vvd2y5h63.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vvd2y5h63.fsf%40alter.siamese.dyndns.org In-Reply-To: <7v1v5m6w26.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > I think the following is easier to read and conveys what the code is > trying to do more clearly. No? This time with a proposed log message... -- >8 -- Subject: [PATCH] get_sha1_oneline: fix lifespan rule of temp_commit_buffer variable This is trying to free only what we ourselves read (as opposed to what we borrowed from commit->buffer) but do so lazily only to work around the fact that the code has many irregular exit points, and doing it right makes it necessary to call free() from many different places in the loop. Rewrite the structure of the code inside the loop so that the variable has to live within a single iteration, ever. This should make the logic easier to follow as well. Also we didn't free a temporary commit list we kept to hold the original set of commits. Free it. Noticed-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- sha1_name.c | 24 +++++++++++++----------- 1 files changed, 13 insertions(+), 11 deletions(-) diff --git a/sha1_name.c b/sha1_name.c index 2c3a5fb..2cc7a42 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -693,8 +693,7 @@ static int handle_one_ref(const char *path, static int get_sha1_oneline(const char *prefix, unsigned char *sha1) { struct commit_list *list = NULL, *backup = NULL, *l; - int retval = -1; - char *temp_commit_buffer = NULL; + int found = 0; regex_t regex; if (prefix[0] == '!') { @@ -710,37 +709,40 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1) for (l = list; l; l = l->next) commit_list_insert(l->item, &backup); while (list) { - char *p; + char *p, *to_free = NULL; struct commit *commit; enum object_type type; unsigned long size; + int matches; commit = pop_most_recent_commit(&list, ONELINE_SEEN); if (!parse_object(commit->object.sha1)) continue; - free(temp_commit_buffer); if (commit->buffer) p = commit->buffer; else { p = read_sha1_file(commit->object.sha1, &type, &size); if (!p) continue; - temp_commit_buffer = p; + to_free = p; } - if (!(p = strstr(p, "\n\n"))) - continue; - if (!regexec(®ex, p + 2, 0, NULL, 0)) { + + p = strstr(p, "\n\n"); + matches = p && !regexec(®ex, p + 2, 0, NULL, 0); + free(to_free); + + if (matches) { hashcpy(sha1, commit->object.sha1); - retval = 0; + found = 1; break; } } regfree(®ex); - free(temp_commit_buffer); free_commit_list(list); for (l = backup; l; l = l->next) clear_commit_marks(l->item, ONELINE_SEEN); - return retval; + free_commit_list(backup); + return found ? 0 : -1; } struct grab_nth_branch_switch_cbdata { -- 1.7.3.3.763.g91c7d ``` ## Nguyen Thai Ngoc Duy, 2010-12-13 06:27 Subject: Re: [PATCH 1/3] get_sha1_oneline: do not leak or double free Message-ID: URL: https://gitlist.dev/e/AANLkTimMb2ouZQbgOFB_JTuJb99vzC%3D2CzeUH3TnFU8M%40mail.gmail.com In-Reply-To: <7vvd2y5h63.fsf@alter.siamese.dyndns.org> ``` 2010/12/13 Junio C Hamano : > Junio C Hamano writes: > >> I think the following is easier to read and conveys what the code is >> trying to do more clearly.  No? > > This time with a proposed log message... Much better. Thanks. -- Duy ``` ## Junio C Hamano, 2010-12-13 06:31 Subject: Re: [PATCH 1/3] get_sha1_oneline: do not leak or double free Message-ID: <7voc8q5gll.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7voc8q5gll.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vvd2y5h63.fsf@alter.siamese.dyndns.org> ``` ... and this is how your [2/3] would look on top of that. I didn't change the scratchpad bit assignment in this commit, as that is a logically separate change and I didn't look at all the codepaths that can call into this function to make sure they never use TMP_MARK themselves. They shouldn't, but it is easier to revert the change if there were. -- >8 -- From: Nguyễn Thái Ngọc Duy Date: Mon, 13 Dec 2010 10:01:14 +0700 Subject: [PATCH 2/3] get_sha1_oneline: make callers prepare the commit list to traverse This gives callers more control, i.e. which ref will be searched from. They must prepare the list ordered by committer date. Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- sha1_name.c | 19 +++++++++++-------- 1 files changed, 11 insertions(+), 8 deletions(-) diff --git a/sha1_name.c b/sha1_name.c index 2cc7a42..aefae1f 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -686,13 +686,13 @@ static int handle_one_ref(const char *path, if (object->type != OBJ_COMMIT) return 0; insert_by_date((struct commit *)object, list); - object->flags |= ONELINE_SEEN; return 0; } -static int get_sha1_oneline(const char *prefix, unsigned char *sha1) +static int get_sha1_oneline(const char *prefix, unsigned char *sha1, + struct commit_list *list) { - struct commit_list *list = NULL, *backup = NULL, *l; + struct commit_list *backup = NULL, *l; int found = 0; regex_t regex; @@ -705,9 +705,10 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1) if (regcomp(®ex, prefix, REG_EXTENDED)) die("Invalid search pattern: %s", prefix); - for_each_ref(handle_one_ref, &list); - for (l = list; l; l = l->next) + for (l = list; l; l = l->next) { + l->item->object.flags |= ONELINE_SEEN; commit_list_insert(l->item, &backup); + } while (list) { char *p, *to_free = NULL; struct commit *commit; @@ -1090,9 +1091,11 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1, int stage = 0; struct cache_entry *ce; int pos; - if (namelen > 2 && name[1] == '/') - /* don't need mode for commit */ - return get_sha1_oneline(name + 2, sha1); + if (namelen > 2 && name[1] == '/') { + struct commit_list *list = NULL; + for_each_ref(handle_one_ref, &list); + return get_sha1_oneline(name + 2, sha1, list); + } if (namelen < 3 || name[2] != ':' || name[1] < '0' || '3' < name[1]) ``` ## Junio C Hamano, 2010-12-13 06:43 Subject: Re: [PATCH 1/3] get_sha1_oneline: do not leak or double free Message-ID: <7vk4je5g1n.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk4je5g1n.fsf%40alter.siamese.dyndns.org In-Reply-To: <7voc8q5gll.fsf@alter.siamese.dyndns.org> ``` ... and this is [3/3], whose sha1_name.c part needed some adjustment. -- >8 -- From: Nguyễn Thái Ngọc Duy Date: Mon, 13 Dec 2010 10:01:15 +0700 Subject: [PATCH] get_sha1: support $commit^{/regex} syntax This works like ":/regex" syntax that finds a recently created commit starting from all refs, but limits the discovery to those reachable from the named commit. Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- Documentation/revisions.txt | 6 +++ sha1_name.c | 37 +++++++++++++++------ t/t1511-rev-parse-caret.sh | 73 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 11 deletions(-) create mode 100755 t/t1511-rev-parse-caret.sh diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index 3d4b79c..174fa8e 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -106,6 +106,12 @@ the `$GIT_DIR/refs` directory or from the `$GIT_DIR/packed-refs` file. and dereference the tag recursively until a non-tag object is found. +* A suffix '{caret}' to a revision parameter followed by a brace + pair that contains a text led by a slash (e.g. `HEAD^{/fix nasty bug}`): + this is the same as `:/fix nasty bug` syntax below except that + it returns the youngest matching commit which is reachable from + the ref before '{caret}'. + * A colon, followed by a slash, followed by a text (e.g. `:/fix nasty bug`): this names a commit whose commit message matches the specified regular expression. This name returns the youngest matching commit which is diff --git a/sha1_name.c b/sha1_name.c index aefae1f..1ba4bc3 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -7,6 +7,8 @@ #include "refs.h" #include "remote.h" +static int get_sha1_oneline(const char *, unsigned char *, struct commit_list *); + static int find_short_object_filename(int len, const char *name, unsigned char *sha1) { struct alternate_object_database *alt; @@ -562,6 +564,8 @@ static int peel_onion(const char *name, int len, unsigned char *sha1) expected_type = OBJ_BLOB; else if (sp[0] == '}') expected_type = OBJ_NONE; + else if (sp[0] == '/') + expected_type = OBJ_COMMIT; else return -1; @@ -576,19 +580,30 @@ static int peel_onion(const char *name, int len, unsigned char *sha1) if (!o || (!o->parsed && !parse_object(o->sha1))) return -1; hashcpy(sha1, o->sha1); + return 0; } - else { - /* - * At this point, the syntax look correct, so - * if we do not get the needed object, we should - * barf. - */ - o = peel_to_type(name, len, o, expected_type); - if (o) { - hashcpy(sha1, o->sha1); - return 0; - } + + /* + * At this point, the syntax look correct, so + * if we do not get the needed object, we should + * barf. + */ + o = peel_to_type(name, len, o, expected_type); + if (!o) return -1; + + hashcpy(sha1, o->sha1); + if (sp[0] == '/') { + /* "$commit^{/foo}" */ + char *prefix; + int ret; + struct commit_list *list = NULL; + + prefix = xstrndup(sp + 1, name + len - 1 - (sp + 1)); + commit_list_insert((struct commit *)o, &list); + ret = get_sha1_oneline(prefix, sha1, list); + free(prefix); + return ret; } return 0; } diff --git a/t/t1511-rev-parse-caret.sh b/t/t1511-rev-parse-caret.sh new file mode 100755 index 0000000..5c8439c --- /dev/null +++ b/t/t1511-rev-parse-caret.sh @@ -0,0 +1,73 @@ +#!/bin/sh + +test_description='tests for ref^{stuff}' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo blob >a-blob && + git tag -a -m blob blob-tag `git hash-object -w a-blob` + mkdir a-tree && + echo moreblobs >a-tree/another-blob && + git add . && + TREE_SHA1=`git write-tree` && + git tag -a -m tree tree-tag "$TREE_SHA1" && + git commit -m Initial && + git tag -a -m commit commit-tag && + git branch ref && + git checkout master && + echo modified >>a-blob && + git add -u && + git commit -m Modified +' + +test_expect_success 'ref^{non-existent}' ' + test_must_fail git rev-parse ref^{non-existent} +' + +test_expect_success 'ref^{}' ' + git rev-parse ref >expected && + git rev-parse ref^{} >actual && + test_cmp expected actual && + git rev-parse commit-tag^{} >actual && + test_cmp expected actual +' + +test_expect_success 'ref^{commit}' ' + git rev-parse ref >expected && + git rev-parse ref^{commit} >actual && + test_cmp expected actual && + git rev-parse commit-tag^{commit} >actual && + test_cmp expected actual && + test_must_fail git rev-parse tree-tag^{commit} && + test_must_fail git rev-parse blob-tag^{commit} +' + +test_expect_success 'ref^{tree}' ' + echo $TREE_SHA1 >expected && + git rev-parse ref^{tree} >actual && + test_cmp expected actual && + git rev-parse commit-tag^{tree} >actual && + test_cmp expected actual && + git rev-parse tree-tag^{tree} >actual && + test_cmp expected actual && + test_must_fail git rev-parse blob-tag^{tree} +' + +test_expect_success 'ref^{/}' ' + git rev-parse master >expected && + git rev-parse master^{/} >actual && + test_cmp expected actual +' + +test_expect_success 'ref^{/non-existent}' ' + test_must_fail git rev-parse master^{/non-existent} +' + +test_expect_success 'ref^{/Initial}' ' + git rev-parse ref >expected && + git rev-parse master^{/Initial} >actual && + test_cmp expected actual +' + +test_done -- 1.7.3.3.763.g91c7d ``` ## Junio C Hamano, 2010-12-15 00:50 Subject: Re: [PATCH 3/3] get_sha1: support ref^{/regex} syntax Message-ID: <7vaak7rh9k.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vaak7rh9k.fsf%40alter.siamese.dyndns.org In-Reply-To: <1292209275-17451-3-git-send-email-pclouds@gmail.com> ``` Nguyễn Thái Ngọc Duy writes: > +test_expect_success 'ref^{/}' ' > + git rev-parse master >expected && > + git rev-parse master^{/} >actual && > + test_cmp expected actual > +' This test fails on FBSD 8, which refuses to regcomp("") saying "empty (sub)expression", which is somewhat bogus. I'd change it to "/." for now ;-). ``` ## Nguyen Thai Ngoc Duy, 2010-12-15 01:58 Subject: Re: [PATCH 3/3] get_sha1: support ref^{/regex} syntax Message-ID: URL: https://gitlist.dev/e/AANLkTikgXAg9%2B0TXzpFH7LT1yD2sV5Dp%3D025GbudnY5S%40mail.gmail.com In-Reply-To: <7vaak7rh9k.fsf@alter.siamese.dyndns.org> ``` 2010/12/15 Junio C Hamano : > Nguyễn Thái Ngọc Duy   writes: > >> +test_expect_success 'ref^{/}' ' >> +     git rev-parse master >expected && >> +     git rev-parse master^{/} >actual && >> +     test_cmp expected actual >> +' > > This test fails on FBSD 8, which refuses to regcomp("") > saying "empty (sub)expression", which is somewhat bogus. > > I'd change it to "/." for now ;-). Perhaps we should just reject this case. No sane body would ever use it. -- Duy ``` ## Junio C Hamano, 2010-12-15 02:56 Subject: Re: [PATCH 3/3] get_sha1: support ref^{/regex} syntax Message-ID: <7v62uvrbfk.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v62uvrbfk.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Nguyen Thai Ngoc Duy writes: > Perhaps we should just reject this case. No sane body would ever use it. I'd have to say that dismissing before even trying is not a very disciplined attitude. Shouldn't an empty regexp simply match everything? Even on FBSD8, $ grep '' /etc/passwd seems to show everything. How hard would it be to do this in the codepath we are discussing? ``` ## Nguyen Thai Ngoc Duy, 2010-12-15 03:12 Subject: Re: [PATCH 3/3] get_sha1: support ref^{/regex} syntax Message-ID: URL: https://gitlist.dev/e/AANLkTimy5MVkxDzXqzA8T4DHJmzsSAvwqVVzRvWw5KA0%40mail.gmail.com In-Reply-To: <7v62uvrbfk.fsf@alter.siamese.dyndns.org> ``` On Wed, Dec 15, 2010 at 9:56 AM, Junio C Hamano wrote: > Nguyen Thai Ngoc Duy writes: > >> Perhaps we should just reject this case. No sane body would ever use it. > > I'd have to say that dismissing before even trying is not a very > disciplined attitude. > > Shouldn't an empty regexp simply match everything?  Even on FBSD8, > >    $ grep '' /etc/passwd > > seems to show everything.  How hard would it be to do this in the codepath > we are discussing? To make '' match everything is easy. I'll cook up something when I get home. -- Duy ``` ## Nguyễn Thái Ngọc Duy, 2010-12-15 09:02 Subject: [PATCH] get_sha1: handle special case $commit^{/} Message-ID: <1292403774-361-1-git-send-email-pclouds@gmail.com> URL: https://gitlist.dev/e/1292403774-361-1-git-send-email-pclouds%40gmail.com In-Reply-To: ``` Empty regex pattern should always match. But the exact behavior of regexec() may vary. Because it always matches anyway, we can just return 'matched' without calling regex machinery. Signed-off-by: Nguyễn Thái Ngọc Duy --- On top of nd/oneline-sha1-name-from-specific-ref sha1_name.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/sha1_name.c b/sha1_name.c index 1ba4bc3..c5c59ce 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -599,6 +599,13 @@ static int peel_onion(const char *name, int len, unsigned char *sha1) int ret; struct commit_list *list = NULL; + /* + * $commit^{/}. Some regex implementation may reject. + * We don't need regex anyway. '' pattern always matches. + */ + if (sp[1] == '}') + return 0; + prefix = xstrndup(sp + 1, name + len - 1 - (sp + 1)); commit_list_insert((struct commit *)o, &list); ret = get_sha1_oneline(prefix, sha1, list); -- 1.7.3.3.476.g10a82 ``` ## Junio C Hamano, 2010-12-15 23:44 Subject: Re: [PATCH] get_sha1: handle special case $commit^{/} Message-ID: <7v8vzqppo3.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8vzqppo3.fsf%40alter.siamese.dyndns.org In-Reply-To: <1292403774-361-1-git-send-email-pclouds@gmail.com> ``` Nguyễn Thái Ngọc Duy writes: > Empty regex pattern should always match. But the exact behavior of > regexec() may vary. Because it always matches anyway, we can just > return 'matched' without calling regex machinery. Hmm, I just noticed that "git grep -e '' Makefile" fails on FBSD8 for the same reason. I'd prefer a solution that is not about "special case $commit^{/}" but is about "work around regcomp that cannot compile an empty regexp". Wisdoms? > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > On top of nd/oneline-sha1-name-from-specific-ref > > sha1_name.c | 7 +++++++ > 1 files changed, 7 insertions(+), 0 deletions(-) > > diff --git a/sha1_name.c b/sha1_name.c > index 1ba4bc3..c5c59ce 100644 > --- a/sha1_name.c > +++ b/sha1_name.c > @@ -599,6 +599,13 @@ static int peel_onion(const char *name, int len, unsigned char *sha1) > int ret; > struct commit_list *list = NULL; > > + /* > + * $commit^{/}. Some regex implementation may reject. > + * We don't need regex anyway. '' pattern always matches. > + */ > + if (sp[1] == '}') > + return 0; > + > prefix = xstrndup(sp + 1, name + len - 1 - (sp + 1)); > commit_list_insert((struct commit *)o, &list); > ret = get_sha1_oneline(prefix, sha1, list); > -- > 1.7.3.3.476.g10a82 ``` ## Nguyen Thai Ngoc Duy, 2010-12-16 00:23 Subject: Re: [PATCH] get_sha1: handle special case $commit^{/} Message-ID: URL: https://gitlist.dev/e/AANLkTim3Y5jqwc9%3DStOLgvJYPPJS7B84Otz0%3Dv1dAbc1%40mail.gmail.com In-Reply-To: <7v8vzqppo3.fsf@alter.siamese.dyndns.org> ``` 2010/12/16 Junio C Hamano : > Nguyễn Thái Ngọc Duy writes: > >> Empty regex pattern should always match. But the exact behavior of >> regexec() may vary. Because it always matches anyway, we can just >> return 'matched' without calling regex machinery. > > Hmm, I just noticed that "git grep -e '' Makefile" fails on FBSD8 for > the same reason. > > I'd prefer a solution that is not about "special case $commit^{/}" but is > about "work around regcomp that cannot compile an empty regexp". Hmm.. if there is no compilation phase, we can redefine a macro to workaround. Perhaps prefer compat/regex to FBSD's implementation? -- Duy ```