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