threads / patch / 7876

patch, 5 partsNew for_each_revision() helper

Subject: [PATCH 0/5] New for_each_revision() helper

## tl;dr

17 messages between Apr 27, 2007 and Apr 30, 2007. Diffs are folded; open one to read it.

replies: 16people: 5as markdown or json

Luiz Fernando N. Capitulino· Apr 27, 2007, 17:00 UTC · lore
 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(-)
Luiz Fernando N. Capitulino· Apr 27, 2007, 17:00 UTC · re: Luiz Fernando N. Capitulino · lore

[PATCH 1/5] Introduces for_each_revision() helper

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(-)
Show changes to revision.h +11 −0
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
Junio C Hamano· Apr 27, 2007, 19:32 UTC · re: Luiz Fernando N. Capitulino · lore

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

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

Luiz Fernando N. Capitulino· Apr 27, 2007, 21:13 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Apr 29, 2007, 06:59 UTC · re: Luiz Fernando N. Capitulino · lore

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

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

Show 19 quoted lines
> 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.

Shawn O. Pearce· Apr 29, 2007, 07:06 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <junkio@cox.net> wrote:
Show 7 quoted lines
> 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.
Junio C Hamano· Apr 30, 2007, 23:19 UTC · re: Shawn O. Pearce · lore

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

"Shawn O. Pearce" <spearce@spearce.org> writes:
Show 5 quoted lines
> 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.

Johannes Schindelin· Apr 28, 2007, 02:46 UTC · re: Luiz Fernando N. Capitulino · lore

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

Hi,
On Fri, 27 Apr 2007, Luiz Fernando N. Capitulino wrote:
Show 20 quoted lines
> 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

Alex Riesen· Apr 28, 2007, 11:50 UTC · re: Johannes Schindelin · lore

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

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

Johannes Schindelin· Apr 28, 2007, 12:52 UTC · re: Alex Riesen · lore

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

Hi,
On Sat, 28 Apr 2007, Alex Riesen wrote:
Show 12 quoted lines
> 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.

Show 6 quoted lines
> > 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

Luiz Fernando N. Capitulino· Apr 28, 2007, 16:02 UTC · re: Alex Riesen · lore

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

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.
Alex Riesen· Apr 28, 2007, 16:48 UTC · re: Luiz Fernando N. Capitulino · lore

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

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
Luiz Fernando N. Capitulino· Apr 29, 2007, 13:04 UTC · re: Alex Riesen · lore

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

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
Luiz Fernando N. Capitulino· Apr 27, 2007, 17:00 UTC · re: Luiz Fernando N. Capitulino · lore

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

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(-)
Show changes to builtin-fmt-merge-msg.c +1 −2
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· Apr 27, 2007, 17:00 UTC · re: Luiz Fernando N. Capitulino · lore

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

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(-)
Show changes to reachable.c +1 −2
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
Luiz Fernando N. Capitulino· Apr 27, 2007, 17:00 UTC · re: Luiz Fernando N. Capitulino · lore

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

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(-)
Show changes to builtin-shortlog.c +1 −2
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
Luiz Fernando N. Capitulino· Apr 27, 2007, 17:00 UTC · re: Luiz Fernando N. Capitulino · lore

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

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(-)
Show changes to builtin-log.c +4 −8
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

← back to recent threads