[PATCH 0/5] RFC: for_each_revision() helper

[PATCH 0/5] RFC: for_each_revision() helper

[Luiz Fernando N Capitulino]

 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(-)

[PATCH 1/5] Introduces for_each_revision() helper

[Luiz Fernando N Capitulino]

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.

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

diff --git a/revision.h b/revision.h
index cdf94ad..bb6f475 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) \
+	prepare_revision_walk(rev);    \
+	while ((commit = get_revision(rev)) != NULL)
+
 #endif
-- 
1.5.1.1.320.g1cf2

Re: [PATCH 1/5] Introduces for_each_revision() helper

[Andy Whitcroft]

Luiz Fernando N Capitulino wrote:
> 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.
> 
> Signed-off-by: Luiz Fernando N Capitulino <lcapitulino@mandriva.com.br>
> ---
>  revision.h |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/revision.h b/revision.h
> index cdf94ad..bb6f475 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) \
> +	prepare_revision_walk(rev);    \
> +	while ((commit = get_revision(rev)) != NULL)
> +
>  #endif

If this is constructed like that then I would expect the code below to
be miss-compiled:

	if (condition)
		for_each_revision(commit, rev) {
		}

As it would be effectivly be:

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

I think you'd want this to be something more like:

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

-apw

Re: [PATCH 1/5] Introduces for_each_revision() helper

[Luiz Fernando N. Capitulino]

Em Thu, 26 Apr 2007 20:59:01 +0100
Andy Whitcroft <apw@shadowen.org> escreveu:

| If this is constructed like that then I would expect the code below to
| be miss-compiled:
| 
| 	if (condition)
| 		for_each_revision(commit, rev) {
| 		}
| 
| As it would be effectivly be:
| 
| 	if (condition)
| 		prepare_revision_walk(rev);
| 	while ((commit = get_revision(rev)) != NULL) {
| 	}
| 
| I think you'd want this to be something more like:
| 
| #define for_each_revision(commit, rev) \
| 	for (prepare_revision_walk(rev); \
| 		(commit = get_revision(rev))) != NULL); ) {

 I'm *so* clueless that this mistake does not surprise me.

 Will fix, thanks for the review Andy.

-- 
Luiz Fernando N. Capitulino

[PATCH 2/5] builtin-fmt-merge-msg.c: Use for_each_revision() helper

[Luiz Fernando N Capitulino]

Signed-off-by: Luiz Fernando N Capitulino <lcapitulino@mandriva.com.br>
---
 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.320.g1cf2

[PATCH 3/5] reachable.c: Use for_each_revision() helper

[Luiz Fernando N Capitulino]

Signed-off-by: Luiz Fernando N Capitulino <lcapitulino@mandriva.com.br>
---
 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.320.g1cf2

[PATCH 4/5] builtin-shortlog.c: Use for_each_revision() helper

[Luiz Fernando N Capitulino]

Signed-off-by: Luiz Fernando N Capitulino <lcapitulino@mandriva.com.br>
---
 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.320.g1cf2

[PATCH 5/5] builtin-log.c: Use for_each_revision() helper

[Luiz Fernando N Capitulino]

Signed-off-by: Luiz Fernando N Capitulino <lcapitulino@mandriva.com.br>
---
 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.320.g1cf2

Re: [PATCH 0/5] RFC: for_each_revision() helper

[Hermes Trismegisto]

Luiz Fernando N Capitulino <lcapitulino@mandriva.com.br> writes:

>  [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 :)]

But if you changed your name to omit '.', that is not much of a
test I suspect...

Re: [PATCH 0/5] RFC: for_each_revision() helper

[Sam Ravnborg]

On Thu, Apr 26, 2007 at 01:57:23PM -0700, Hermes Trismegisto wrote:

[Copied from mail header]
From: Hermes Trismegisto <junkio@cox.net>

Wonder who sent this mail???
http://en.wikipedia.org/wiki/Hermes_Trismegistus

	Sam

Re: [PATCH 0/5] RFC: for_each_revision() helper

[Luiz Fernando N. Capitulino]

Em Thu, 26 Apr 2007 23:05:51 +0200
Sam Ravnborg <sam@ravnborg.org> escreveu:

| On Thu, Apr 26, 2007 at 01:57:23PM -0700, Hermes Trismegisto wrote:
| 
| [Copied from mail header]
| From: Hermes Trismegisto <junkio@cox.net>
| 
| Wonder who sent this mail???
| http://en.wikipedia.org/wiki/Hermes_Trismegistus

 Looks like Junio has a pseudonym:

"""
<lcapitulino>   gitster: hi, who's Hermes Trismegisto? :)
* gitster leaked his pseudonym by accident.
"""

 Or I didn't get the joke.

-- 
Luiz Fernando N. Capitulino

Re: [PATCH 0/5] RFC: for_each_revision() helper

[Luiz Fernando N. Capitulino]

Em Thu, 26 Apr 2007 13:57:23 -0700
Hermes Trismegisto <junkio@cox.net> escreveu:

| Luiz Fernando N Capitulino <lcapitulino@mandriva.com.br> writes:
| 
| >  [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 :)]
| 
| But if you changed your name to omit '.', that is not much of a
| test I suspect...

 Yes, I did. But git-send-email is taking my name from the patches,
so the same problem happened.

 I had to change my name in the patches to make it to work.

-- 
Luiz Fernando N. Capitulino

Re: [PATCH 0/5] RFC: for_each_revision() helper

[Junio C Hamano]

"Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br>
writes:

> Em Thu, 26 Apr 2007 13:57:23 -0700
> Hermes Trismegisto <junkio@cox.net> escreveu:
>
> | Luiz Fernando N Capitulino <lcapitulino@mandriva.com.br> writes:
> | 
> | >  [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 :)]
> | 
> | But if you changed your name to omit '.', that is not much of a
> | test I suspect...
>
>  Yes, I did. But git-send-email is taking my name from the patches,
> so the same problem happened.
>
>  I had to change my name in the patches to make it to work.

I know.  But my point of "changing your name is not much of a
test" is that that was exactly what Robin Johnson's patches to
quote CC: addresses that were taken from the sign-off lines in
the proposed commit log message were meant to fix.

Specifically:

http://repo.or.cz/w/alt-git.git?a=commitdiff;h=732263d411fe2e3e29ee9fa1c2ad1a20bdea062c

Re: [PATCH 0/5] RFC: for_each_revision() helper

[Luiz Fernando N. Capitulino]

Em Thu, 26 Apr 2007 14:21:46 -0700
Junio C Hamano <junkio@cox.net> escreveu:

| "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br>
| writes:
| 
| > Em Thu, 26 Apr 2007 13:57:23 -0700
| > Hermes Trismegisto <junkio@cox.net> escreveu:
| >
| > | Luiz Fernando N Capitulino <lcapitulino@mandriva.com.br> writes:
| > | 
| > | >  [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 :)]
| > | 
| > | But if you changed your name to omit '.', that is not much of a
| > | test I suspect...
| >
| >  Yes, I did. But git-send-email is taking my name from the patches,
| > so the same problem happened.
| >
| >  I had to change my name in the patches to make it to work.
| 
| I know.  But my point of "changing your name is not much of a
| test" is that that was exactly what Robin Johnson's patches to
| quote CC: addresses that were taken from the sign-off lines in
| the proposed commit log message were meant to fix.
| 
| Specifically:
| 
| http://repo.or.cz/w/alt-git.git?a=commitdiff;h=732263d411fe2e3e29ee9fa1c2ad1a20bdea062c

 Okay.

 With the --dry-run option it became very easy to run tests,
so, I've changed everything back and tried to reproduce it
again:

-> First e-mail:

"""
Dry-OK. Log says:
Date: Fri, 27 Apr 2007 10:04:48 -0300
Sendmail: /usr/sbin/sendmail -i junkio@cox.net git@vger.kernel.org
From: "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br>
Subject: [PATCH 0/5] RFC: for_each_revision() helper
Cc: git@vger.kernel.org
To: junkio@cox.net
"""

-> Last one (others are the same)

"""
Dry-OK. Log says:
Date: Fri, 27 Apr 2007 10:04:53 -0300
Sendmail: /usr/sbin/sendmail -i junkio@cox.net git@vger.kernel.org lcapitulino@mandriva.com.br
From: "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br>
Subject: [PATCH 5/5] builtin-log.c: Use for_each_revision() helper
Cc: git@vger.kernel.org, "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br>
To: junkio@cox.net
"""

 Looks like it's fixed, I'll submit this patch series again
shortly.

 BTW, Robin, can we have an option to read the introductory e-mail
from a file? It could read a Subject line from it too.

-- 
Luiz Fernando N. Capitulino

Re: [PATCH 0/5] RFC: for_each_revision() helper

[Junio C Hamano]

"Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br>
writes:

>  BTW, Robin, can we have an option to read the introductory e-mail
> from a file? It could read a Subject line from it too.

Without adding any new option, I think you can do that today.

	$ git format-patch -o ./+outgo -n master..jc/my-series
	$ edit ./+outgo/0000-cover.txt
        $ git send-email [options] ./+outgo

[PATCH 0/5] New for_each_revision() helper

[Luiz Fernando N. Capitulino]

 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(-)

[PATCH 1/5] Introduces for_each_revision() helper

[Luiz Fernando N. Capitulino]

From: Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>

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 <lcapitulino@mandriva.com.br>
---
 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

Re: [PATCH 1/5] Introduces for_each_revision() helper

[Junio C Hamano]

"Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br>
writes:

> From: Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>
>
> 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.

Re: [PATCH 1/5] Introduces for_each_revision() helper

[Luiz Fernando N. Capitulino]

Em Fri, 27 Apr 2007 12:32:11 -0700
Junio C Hamano <junkio@cox.net> escreveu:

| "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br>
| writes:
| 
| > From: Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>
| >
| > 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

Re: [PATCH 1/5] Introduces for_each_revision() helper

[Junio C Hamano]

"Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br>
writes:

> Em Fri, 27 Apr 2007 12:32:11 -0700
> Junio C Hamano <junkio@cox.net> escreveu:
>
> | "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br>
> | writes:
> | 
> | > From: Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>
> | >
> | > 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.

I on the other hand like the kernel style list macros.

The reason I do not like this particular one is because both
operations you are hiding are not simple operations like
"initialize a variable to list head" or "follow a single pointer
in the structure", but rather heavyweight operations with rather
complex semantics.  I would want to make sure that people
realize they are calling something heavyweight when they use the
revision traversal.

Re: [PATCH 1/5] Introduces for_each_revision() helper

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> The reason I do not like this particular one is because both
> operations you are hiding are not simple operations like
> "initialize a variable to list head" or "follow a single pointer
> in the structure", but rather heavyweight operations with rather
> complex semantics.  I would want to make sure that people
> realize they are calling something heavyweight when they use the
> revision traversal.

But in_merge_base is heavyweight if the two commits are in the
same object database, but aren't connected at all.  You'll need
to traverse both histories before aborting and saying there is
no merge base.  That ain't cheap on large trees.  But its also a
single line of code.

Anyway, my original problem with this macro was the way it was
defined.  I think Luiz was able to fix most of my issues with it
in his latest version, but I still have a personal distaste for
hiding things like a for(;;) construct in a macro, or allowing a
macro parameter to be used more than once within the definition of
the macro (unexpected side-effects of evaluating an more than once).

-- 
Shawn.

Re: [PATCH 1/5] Introduces for_each_revision() helper

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> But in_merge_base is heavyweight if the two commits are in the
> same object database, but aren't connected at all.  You'll need
> to traverse both histories before aborting and saying there is
> no merge base.  That ain't cheap on large trees.  But its also a
> single line of code.

Who said anything about "a single line of code"?  That is quite
different from heaviness hidden in a control structure
lookalike.

Re: [PATCH 1/5] Introduces for_each_revision() helper

[Johannes Schindelin]

Hi,

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

> 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

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

Re: [PATCH 1/5] Introduces for_each_revision() helper

[Alex Riesen]

Johannes Schindelin, Sat, Apr 28, 2007 04:46:41 +0200:
> > +#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.

Re: [PATCH 1/5] Introduces for_each_revision() helper

[Johannes Schindelin]

Hi,

On Sat, 28 Apr 2007, Alex Riesen wrote:

> Johannes Schindelin, Sat, Apr 28, 2007 04:46:41 +0200:
> > > +#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.

> > 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

Re: [PATCH 1/5] Introduces for_each_revision() helper

[Luiz Fernando N. Capitulino]

Em Sat, 28 Apr 2007 13:50:59 +0200
Alex Riesen <raa.lkml@gmail.com> 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.

Re: [PATCH 1/5] Introduces for_each_revision() helper

[Alex Riesen]

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.

> But I think people won't like it.

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

Re: [PATCH 1/5] Introduces for_each_revision() helper

[Luiz Fernando N. Capitulino]

Em Sat, 28 Apr 2007 18:48:36 +0200
Alex Riesen <raa.lkml@gmail.com> 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