[PATCH 1/3] get_sha1_oneline: do not leak or double free

[PATCH 1/3] get_sha1_oneline: do not leak or double free

[Nguyễn Thái Ngọc Duy]

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 <pclouds@gmail.com>
---
 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

[PATCH 2/3] get_sha1_oneline: let callers initialize the commit tips for traverse

[Nguyễn Thái Ngọc Duy]

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 <pclouds@gmail.com>
---
 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(&regex, 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

[PATCH 3/3] get_sha1: support ref^{/regex} syntax

[Nguyễn Thái Ngọc Duy]

This works like :/ syntax, but only limited to one ref.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

Re: [PATCH 3/3] get_sha1: support ref^{/regex} syntax

[Nguyen Thai Ngoc Duy]

2010/12/13 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> 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

Re: [PATCH 3/3] get_sha1: support ref^{/regex} syntax

[Jonathan Nieder]

Nguyen Thai Ngoc Duy wrote:
> 2010/12/13 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:

>> 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. :)

Re: [PATCH 3/3] get_sha1: support ref^{/regex} syntax

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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 ;-).

Re: [PATCH 3/3] get_sha1: support ref^{/regex} syntax

[Nguyen Thai Ngoc Duy]

2010/12/15 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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

Re: [PATCH 3/3] get_sha1: support ref^{/regex} syntax

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> 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?

Re: [PATCH 3/3] get_sha1: support ref^{/regex} syntax

[Nguyen Thai Ngoc Duy]

On Wed, Dec 15, 2010 at 9:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> 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

[PATCH] get_sha1: handle special case $commit^{/}

[Nguyễn Thái Ngọc Duy]

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 <pclouds@gmail.com>
---
 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

Re: [PATCH] get_sha1: handle special case $commit^{/}

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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 <pclouds@gmail.com>
> ---
>  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

Re: [PATCH] get_sha1: handle special case $commit^{/}

[Nguyen Thai Ngoc Duy]

2010/12/16 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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

Re: [PATCH 1/3] get_sha1_oneline: do not leak or double free

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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 <pclouds@gmail.com>
> ---
>  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(&regex, p + 2, 0, NULL, 0)) {
+
+		p = strstr(p, "\n\n");
+		matches = p && !regexec(&regex, p + 2, 0, NULL, 0);
+		free(to_free);
+
+		if (matches) {
 			hashcpy(sha1, commit->object.sha1);
-			retval = 0;
+			found = 1;
 			break;
 		}
 	}
 	regfree(&regex);
-	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 {

Re: [PATCH 1/3] get_sha1_oneline: do not leak or double free

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> 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 <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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(&regex, p + 2, 0, NULL, 0)) {
+
+		p = strstr(p, "\n\n");
+		matches = p && !regexec(&regex, p + 2, 0, NULL, 0);
+		free(to_free);
+
+		if (matches) {
 			hashcpy(sha1, commit->object.sha1);
-			retval = 0;
+			found = 1;
 			break;
 		}
 	}
 	regfree(&regex);
-	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

Re: [PATCH 1/3] get_sha1_oneline: do not leak or double free

[Nguyen Thai Ngoc Duy]

2010/12/13 Junio C Hamano <gitster@pobox.com>:
> Junio C Hamano <gitster@pobox.com> 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

Re: [PATCH 1/3] get_sha1_oneline: do not leak or double free

[Junio C Hamano]

... 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 <pclouds@gmail.com>
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 <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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(&regex, 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])

Re: [PATCH 1/3] get_sha1_oneline: do not leak or double free

[Junio C Hamano]

... and this is [3/3], whose sha1_name.c part needed some adjustment.

-- >8 --
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
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 <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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