[PATCH 0/5] New for_each_revision() helper

[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

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

[Luiz Fernando N. Capitulino]

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

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.372.g4342

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

[Luiz Fernando N. Capitulino]

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

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.372.g4342

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

[Luiz Fernando N. Capitulino]

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

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.372.g4342

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

[Luiz Fernando N. Capitulino]

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

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.372.g4342

[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