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

DORMANTno replies

Revision v1 of 2 in this series.

8 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Luiz Fernando N Capitulino <hidden>
Date: 2016-06-15 22:43:07

 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 2/5] builtin-fmt-merge-msg.c: Use for_each_revision() helper

From: Luiz Fernando N Capitulino <hidden>
Date: 2016-06-15 22:43:07

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.320.g1cf2

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

From: Luiz Fernando N Capitulino <hidden>
Date: 2016-06-15 22:43:07

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.320.g1cf2

[PATCH 1/5] Introduces for_each_revision() helper

From: Luiz Fernando N Capitulino <hidden>
Date: 2016-06-15 22:43:07

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

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

From: Luiz Fernando N Capitulino <hidden>
Date: 2016-06-15 22:43:07

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.320.g1cf2

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

From: Luiz Fernando N Capitulino <hidden>
Date: 2016-06-15 22:43:07

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.320.g1cf2

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

From: Andy Whitcroft <hidden>
Date: 2016-06-15 22:43:07

Luiz Fernando N Capitulino wrote:

quoted hunk

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

From: Luiz Fernando N. Capitulino <hidden>
Date: 2016-06-15 22:43:07

Em Thu, 26 Apr 2007 20:59:01 +0100
Andy Whitcroft [off-list ref] 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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help