# [PATCH 2/5] builtin-fmt-merge-msg.c: Use for_each_revision() helper 15 messages from 2007-04-26 to 2007-04-27. Participants: Luiz Fernando N Capitulino, Andy Whitcroft, Hermes Trismegisto, Sam Ravnborg, Luiz Fernando N. Capitulino, Junio C Hamano. Thread: https://gitlist.dev/t/7857 ## Luiz Fernando N Capitulino, 2007-04-26 19:46 Subject: [PATCH 0/5] RFC: for_each_revision() helper Message-ID: <11776168001253-git-send-email-lcapitulino@mandriva.com.br> URL: https://gitlist.dev/e/11776168001253-git-send-email-lcapitulino%40mandriva.com.br ``` 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, 2007-04-26 19:46 Subject: [PATCH 1/5] Introduces for_each_revision() helper Message-ID: <11776168001048-git-send-email-lcapitulino@mandriva.com.br> URL: https://gitlist.dev/e/11776168001048-git-send-email-lcapitulino%40mandriva.com.br In-Reply-To: <11776168001253-git-send-email-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. Signed-off-by: Luiz Fernando N Capitulino --- 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 ``` ## Luiz Fernando N Capitulino, 2007-04-26 19:46 Subject: [PATCH 2/5] builtin-fmt-merge-msg.c: Use for_each_revision() helper Message-ID: <11776168001607-git-send-email-lcapitulino@mandriva.com.br> URL: https://gitlist.dev/e/11776168001607-git-send-email-lcapitulino%40mandriva.com.br In-Reply-To: <11776168001253-git-send-email-lcapitulino@mandriva.com.br> ``` Signed-off-by: Luiz Fernando N Capitulino --- 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 ``` ## Luiz Fernando N Capitulino, 2007-04-26 19:46 Subject: [PATCH 3/5] reachable.c: Use for_each_revision() helper Message-ID: <11776168002081-git-send-email-lcapitulino@mandriva.com.br> URL: https://gitlist.dev/e/11776168002081-git-send-email-lcapitulino%40mandriva.com.br In-Reply-To: <11776168001253-git-send-email-lcapitulino@mandriva.com.br> ``` Signed-off-by: Luiz Fernando N Capitulino --- 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 ``` ## Luiz Fernando N Capitulino, 2007-04-26 19:46 Subject: [PATCH 4/5] builtin-shortlog.c: Use for_each_revision() helper Message-ID: <11776168011384-git-send-email-lcapitulino@mandriva.com.br> URL: https://gitlist.dev/e/11776168011384-git-send-email-lcapitulino%40mandriva.com.br In-Reply-To: <11776168001253-git-send-email-lcapitulino@mandriva.com.br> ``` Signed-off-by: Luiz Fernando N Capitulino --- 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 ``` ## Luiz Fernando N Capitulino, 2007-04-26 19:46 Subject: [PATCH 5/5] builtin-log.c: Use for_each_revision() helper Message-ID: <11776168013249-git-send-email-lcapitulino@mandriva.com.br> URL: https://gitlist.dev/e/11776168013249-git-send-email-lcapitulino%40mandriva.com.br In-Reply-To: <11776168001253-git-send-email-lcapitulino@mandriva.com.br> ``` Signed-off-by: Luiz Fernando N Capitulino --- 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 ``` ## Andy Whitcroft, 2007-04-26 19:59 Subject: Re: [PATCH 1/5] Introduces for_each_revision() helper Message-ID: <46310485.8070605@shadowen.org> URL: https://gitlist.dev/e/46310485.8070605%40shadowen.org In-Reply-To: <11776168001048-git-send-email-lcapitulino@mandriva.com.br> ``` 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 > --- > 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 ``` ## Hermes Trismegisto, 2007-04-26 20:57 Subject: Re: [PATCH 0/5] RFC: for_each_revision() helper Message-ID: <7vr6q6svkc.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vr6q6svkc.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <11776168001253-git-send-email-lcapitulino@mandriva.com.br> ``` Luiz Fernando N Capitulino 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... ``` ## Sam Ravnborg, 2007-04-26 21:05 Subject: Re: [PATCH 0/5] RFC: for_each_revision() helper Message-ID: <20070426210551.GA25377@uranus.ravnborg.org> URL: https://gitlist.dev/e/20070426210551.GA25377%40uranus.ravnborg.org In-Reply-To: <7vr6q6svkc.fsf@assigned-by-dhcp.cox.net> ``` On Thu, Apr 26, 2007 at 01:57:23PM -0700, Hermes Trismegisto wrote: [Copied from mail header] From: Hermes Trismegisto Wonder who sent this mail??? http://en.wikipedia.org/wiki/Hermes_Trismegistus Sam ``` ## Luiz Fernando N. Capitulino, 2007-04-26 21:12 Subject: Re: [PATCH 1/5] Introduces for_each_revision() helper Message-ID: <20070426181214.36049e32@localhost> URL: https://gitlist.dev/e/20070426181214.36049e32%40localhost In-Reply-To: <46310485.8070605@shadowen.org> ``` Em Thu, 26 Apr 2007 20:59:01 +0100 Andy Whitcroft 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 ``` ## Luiz Fernando N. Capitulino, 2007-04-26 21:14 Subject: Re: [PATCH 0/5] RFC: for_each_revision() helper Message-ID: <20070426181420.4db235cc@localhost> URL: https://gitlist.dev/e/20070426181420.4db235cc%40localhost In-Reply-To: <7vr6q6svkc.fsf@assigned-by-dhcp.cox.net> ``` Em Thu, 26 Apr 2007 13:57:23 -0700 Hermes Trismegisto escreveu: | Luiz Fernando N Capitulino 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 ``` ## Luiz Fernando N. Capitulino, 2007-04-26 21:17 Subject: Re: [PATCH 0/5] RFC: for_each_revision() helper Message-ID: <20070426181733.68b87ab0@localhost> URL: https://gitlist.dev/e/20070426181733.68b87ab0%40localhost In-Reply-To: <20070426210551.GA25377@uranus.ravnborg.org> ``` Em Thu, 26 Apr 2007 23:05:51 +0200 Sam Ravnborg escreveu: | On Thu, Apr 26, 2007 at 01:57:23PM -0700, Hermes Trismegisto wrote: | | [Copied from mail header] | From: Hermes Trismegisto | | Wonder who sent this mail??? | http://en.wikipedia.org/wiki/Hermes_Trismegistus Looks like Junio has a pseudonym: """ gitster: hi, who's Hermes Trismegisto? :) * gitster leaked his pseudonym by accident. """ Or I didn't get the joke. -- Luiz Fernando N. Capitulino ``` ## Junio C Hamano, 2007-04-26 21:21 Subject: Re: [PATCH 0/5] RFC: for_each_revision() helper Message-ID: <7vk5vysufp.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vk5vysufp.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20070426181420.4db235cc@localhost> ``` "Luiz Fernando N. Capitulino" writes: > Em Thu, 26 Apr 2007 13:57:23 -0700 > Hermes Trismegisto escreveu: > > | Luiz Fernando N Capitulino 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 ``` ## Luiz Fernando N. Capitulino, 2007-04-27 13:21 Subject: Re: [PATCH 0/5] RFC: for_each_revision() helper Message-ID: <20070427102113.46869367@localhost> URL: https://gitlist.dev/e/20070427102113.46869367%40localhost In-Reply-To: <7vk5vysufp.fsf@assigned-by-dhcp.cox.net> ``` Em Thu, 26 Apr 2007 14:21:46 -0700 Junio C Hamano escreveu: | "Luiz Fernando N. Capitulino" | writes: | | > Em Thu, 26 Apr 2007 13:57:23 -0700 | > Hermes Trismegisto escreveu: | > | > | Luiz Fernando N Capitulino 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" 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" Subject: [PATCH 5/5] builtin-log.c: Use for_each_revision() helper Cc: git@vger.kernel.org, "Luiz Fernando N. Capitulino" 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 ``` ## Junio C Hamano, 2007-04-27 17:13 Subject: Re: [PATCH 0/5] RFC: for_each_revision() helper Message-ID: <7vr6q5oi4b.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vr6q5oi4b.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20070427102113.46869367@localhost> ``` "Luiz Fernando N. Capitulino" 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 ```