[PATCH 0/5] New for_each_revision() helper

 Hi,

 [This' also a git-send-email test, so, if this fail by showing just
  the first e-mail in the series, do not blame me :)]

 This series introduces a helper macro to help programs to walk through
revisions (details on the first patch).

 Shawn has already alerted me that some people don't like to
'hide C constructs', but I think that in this case it's useful, as explained
in the next e-mail.

 The complete diff stat is:

 builtin-fmt-merge-msg.c |    3 +--
 builtin-log.c           |   12 ++++--------
 builtin-shortlog.c      |    3 +--
 reachable.c             |    3 +--
 revision.h              |   11 +++++++++++
 5 files changed, 18 insertions(+), 14 deletions(-)

 But if we subtract the for_each_revision() macro's code we get:

 4 files changed, 7 insertions(+), 14 deletions(-)

From: Luiz Fernando N. Capitulino <redacted>

Signed-off-by: Luiz Fernando N. Capitulino <redacted>
---
 builtin-log.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 38bf52f..705050a 100644
--- a/builtin-log.c
+++ b/builtin-log.c

@@ -79,8 +79,7 @@ static int cmd_log_walk(struct rev_info *rev)
 {
 	struct commit *commit;
 
-	prepare_revision_walk(rev);
-	while ((commit = get_revision(rev)) != NULL) {
+	for_each_revision(commit, rev) {
 		log_tree_commit(rev, commit);
 		if (!rev->reflog_info) {
 			/* we allow cycles in reflog ancestry */

@@ -390,9 +389,8 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha
 	o2->flags ^= UNINTERESTING;
 	add_pending_object(&check_rev, o1, "o1");
 	add_pending_object(&check_rev, o2, "o2");
-	prepare_revision_walk(&check_rev);
 
-	while ((commit = get_revision(&check_rev)) != NULL) {
+	for_each_revision(commit, &check_rev) {
 		/* ignore merges */
 		if (commit->parents && commit->parents->next)
 			continue;

@@ -578,8 +576,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (!use_stdout)
 		realstdout = fdopen(dup(1), "w");
 
-	prepare_revision_walk(&rev);
-	while ((commit = get_revision(&rev)) != NULL) {
+	for_each_revision(commit, &rev) {
 		/* ignore merges */
 		if (commit->parents && commit->parents->next)
 			continue;

@@ -716,8 +713,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 		die("Unknown commit %s", limit);
 
 	/* reverse the list of commits */
-	prepare_revision_walk(&revs);
-	while ((commit = get_revision(&revs)) != NULL) {
+	for_each_revision(commit, &revs) {
 		/* ignore merges */
 		if (commit->parents && commit->parents->next)
 			continue;

-- 
1.5.1.1.372.g4342

From: Luiz Fernando N. Capitulino <redacted>

Signed-off-by: Luiz Fernando N. Capitulino <redacted>
---
 builtin-shortlog.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index 3f93498..eca802d 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c

@@ -216,8 +216,7 @@ static void get_from_rev(struct rev_info *rev, struct path_list *list)
 	char scratch[1024];
 	struct commit *commit;
 
-	prepare_revision_walk(rev);
-	while ((commit = get_revision(rev)) != NULL) {
+	for_each_revision(commit, rev) {
 		const char *author = NULL, *oneline, *buffer;
 		int authorlen = authorlen, onelinelen;
 

-- 
1.5.1.1.372.g4342

From: Luiz Fernando N. Capitulino <redacted>

Signed-off-by: Luiz Fernando N. Capitulino <redacted>
---
 reachable.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/reachable.c b/reachable.c
index ff3dd34..b69edd8 100644
--- a/reachable.c
+++ b/reachable.c

@@ -79,7 +79,7 @@ static void walk_commit_list(struct rev_info *revs)
 	struct object_array objects = { 0, 0, NULL };
 
 	/* Walk all commits, process their trees */
-	while ((commit = get_revision(revs)) != NULL)
+	for_each_revision(commit, revs)
 		process_tree(commit->tree, &objects, NULL, "");
 
 	/* Then walk all the pending objects, recursively processing them too */

@@ -195,6 +195,5 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog)
 	 * Set up the revision walk - this will move all commits
 	 * from the pending list to the commit walking list.
 	 */
-	prepare_revision_walk(revs);
 	walk_commit_list(revs);
 }

-- 
1.5.1.1.372.g4342

From: Luiz Fernando N. Capitulino <redacted>

This macro may be used to iterate over revisions, so, instead of
doing:

	struct commit *commit;

	...

	prepare_revision_walk(rev);
	while ((commit = get_revision(rev)) != NULL) {

	...

 	}

New code should use:

	struct commit *commit;

	...

	for_each_revision(commit, rev) {

	...

	}

The only disadvantage is that it's something magical, and the fact that
it returns a struct commit is not obvious.

On the other hand it's documented, has the advantage of making the walking
through revisions easier and can save some lines of code.

This version was suggested by Andy Whitcroft.

Signed-off-by: Luiz Fernando N. Capitulino <redacted>
---
 revision.h |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/revision.h b/revision.h
index cdf94ad..7be3fc7 100644
--- a/revision.h
+++ b/revision.h

@@ -133,4 +133,15 @@ extern void add_object(struct object *obj,
 extern void add_pending_object(struct rev_info *revs, struct object *obj, const char *name);
 extern void add_pending_object_with_mode(struct rev_info *revs, struct object *obj, const char *name, unsigned mode);
 
+/* helpers */
+
+/**
+ * for_each_revision	- iterate over revisions
+ * @commit:	pointer to a commit object returned for each iteration
+ * @rev:	revision pointer
+ */
+#define for_each_revision(commit, rev) \
+	for (prepare_revision_walk(rev); \
+		  (commit = get_revision(rev)) != NULL; )
+
 #endif

-- 
1.5.1.1.372.g4342

From: Luiz Fernando N. Capitulino <redacted>

Signed-off-by: Luiz Fernando N. Capitulino <redacted>
---
 builtin-fmt-merge-msg.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/builtin-fmt-merge-msg.c b/builtin-fmt-merge-msg.c
index 5c145d2..8e1db1c 100644
--- a/builtin-fmt-merge-msg.c
+++ b/builtin-fmt-merge-msg.c

@@ -189,8 +189,7 @@ static void shortlog(const char *name, unsigned char *sha1,
 	add_pending_object(rev, branch, name);
 	add_pending_object(rev, &head->object, "^HEAD");
 	head->object.flags |= UNINTERESTING;
-	prepare_revision_walk(rev);
-	while ((commit = get_revision(rev)) != NULL) {
+	for_each_revision(commit, rev) {
 		char *oneline, *bol, *eol;
 
 		/* ignore merges */

-- 
1.5.1.1.372.g4342

"Luiz Fernando N. Capitulino" [off-list ref]
writes:

From: Luiz Fernando N. Capitulino <redacted>

This macro may be used to iterate over revisions, so, instead of
doing: ...

I am not a big fan of magic control-flow macros, as it makes the
code harder to grok for people new to the codebase.

Em Fri, 27 Apr 2007 12:32:11 -0700
Junio C Hamano [off-list ref] escreveu:

| "Luiz Fernando N. Capitulino" [off-list ref]
| writes:
| 
| > From: Luiz Fernando N. Capitulino [off-list ref]
| >
| > This macro may be used to iterate over revisions, so, instead of
| > doing: ...
| 
| I am not a big fan of magic control-flow macros, as it makes the
| code harder to grok for people new to the codebase.

 Yeah, I agree. But I think that any experienced programmer will
understand it.

 Anyways, I don't want to raise polemic discussions for minor
changes. Feel free to drop this one then.

-- 
Luiz Fernando N. Capitulino

Hi,

On Fri, 27 Apr 2007, Luiz Fernando N. Capitulino wrote:

I object to this, additionally to the magic argument that I agree to, on 
the grounds that it is actually wrong. The first iteration will work on an 
_uninitialized_ "commit" variable.

Furthermore, it is not like it was a huge piece of code that is being 
replaced by a shortcut. There are better places to do some libification 
than this.

Ciao,
Dscho

Johannes Schindelin, Sat, Apr 28, 2007 04:46:41 +0200:

quoted

+#define for_each_revision(commit, rev) \
+	for (prepare_revision_walk(rev); \
+		  (commit = get_revision(rev)) != NULL; )
+
 #endif

I object to this, additionally to the magic argument that I agree to, on 
the grounds that it is actually wrong. The first iteration will work on an 
_uninitialized_ "commit" variable.

No, it wont. Check it. This code is correct.

Furthermore, it is not like it was a huge piece of code that is being 
replaced by a shortcut. There are better places to do some libification 
than this.

It is not about libification. It is plain readability issue.
Look at what list_for_each_* macros did to the source of Linux kernel.

Hi,

On Sat, 28 Apr 2007, Alex Riesen wrote:

Johannes Schindelin, Sat, Apr 28, 2007 04:46:41 +0200:

quoted

quoted

+#define for_each_revision(commit, rev) \
+	for (prepare_revision_walk(rev); \
+		  (commit = get_revision(rev)) != NULL; )
+
 #endif

I object to this, additionally to the magic argument that I agree to, on 
the grounds that it is actually wrong. The first iteration will work on an 
_uninitialized_ "commit" variable.

No, it wont. Check it. This code is correct.

Yes, sorry, as I admitted in my reply to Junio, there was some serious 
mental temporary disability involved.

quoted

Furthermore, it is not like it was a huge piece of code that is being 
replaced by a shortcut. There are better places to do some 
libification than this.

It is not about libification. It is plain readability issue. Look at 
what list_for_each_* macros did to the source of Linux kernel.

Personally, I find the prepare/get_revision stuff not really too 
unreadable.

Ciao,
Dscho

Em Sat, 28 Apr 2007 13:50:59 +0200
Alex Riesen [off-list ref] escreveu:

| Johannes Schindelin, Sat, Apr 28, 2007 04:46:41 +0200:
| 
| > Furthermore, it is not like it was a huge piece of code that is being 
| > replaced by a shortcut. There are better places to do some libification 
| > than this.
| 
| It is not about libification. It is plain readability issue.

 Yes, it's just something I've thought would be worth doing, it's
not the libfication work.

| Look at what list_for_each_* macros did to the source of Linux kernel.

 BTW, I was considering using Linux kernel's linked list
implementation in git, since we have some linked lists around.

 But I think people won't like it.

Luiz Fernando N. Capitulino, Sat, Apr 28, 2007 18:02:01 +0200:

quoted

Look at what list_for_each_* macros did to the source of Linux kernel.

BTW, I was considering using Linux kernel's linked list
implementation in git, since we have some linked lists around.

Do you have some definite place in mind? It's just that the kernel
lists where intentionally kept very simple, so the list
implementations in git probably are as close to the kernel's as it
sanely possible, at which point bringing them in wont change much.

But I think people won't like it.

It depends on what you change and how you do it. Show us

Em Sat, 28 Apr 2007 18:48:36 +0200
Alex Riesen [off-list ref] escreveu:

| Luiz Fernando N. Capitulino, Sat, Apr 28, 2007 18:02:01 +0200:
| > > Look at what list_for_each_* macros did to the source of Linux kernel.
| > 
| > BTW, I was considering using Linux kernel's linked list
| > implementation in git, since we have some linked lists around.
| 
| Do you have some definite place in mind? It's just that the kernel
| lists where intentionally kept very simple, so the list
| implementations in git probably are as close to the kernel's as it
| sanely possible, at which point bringing them in wont change much.

 The ones I've looked at, looks like any other linked list I've
seen in other (not badly written) programs out there.

| > But I think people won't like it.
| 
| It depends on what you change and how you do it. Show us

 Yeah, I want to port some of them to see whether it's
worth doing.

 I've ported the list.h already:

http://repo.or.cz/w/git/libgit-gsoc.git?a=commit;h=e389611fc24843d465ef150b361f5b200068e507