| Message ID | 20190626235032.177551-6-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | example implementation of revwalk tutorial | expand |
On Wed, Jun 26, 2019 at 7:51 PM Emily Shaffer <emilyshaffer@google.com> wrote: > `struct rev_info` is what's used by the struct itself. What "struct itself"? Do you mean 'struct rev_info' is used by the _walk_ itself? Or something? > `repo_init_revisions()` initializes the struct; then we need to set it > up for the walk we want to perform, which is done in > `final_rev_info_setup()`. > [...] > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > diff --git a/builtin/walken.c b/builtin/walken.c > @@ -30,6 +31,40 @@ static void init_walken_defaults(void) > +/* > + * cmd_log calls a second set of init after the repo_init_revisions call. We'll > + * mirror those settings in post_repo_init_init. > + */ What is 'post_repo_init_init'? I found the reference to cmd_log() confusing because I was looking for it in this patch (as if it was being introduced here). Newcomers might be even more confused. Perhaps if you state explicitly that you're referring to existing code in an existing file, it might be clearer. Maybe: builtin/log.c:cmd_log() calls a second ... Overall, I find this entire function comment mystifying. > +static void final_rev_info_setup(int argc, const char **argv, const char *prefix, > + struct rev_info *rev) > +{ > + /* > + * Optional: > + * setup_revision_opt is used to pass options to the setup_revisions() > + * call. It's got some special items for submodules and other types of > + * optimizations, but for now, we'll just point it to HEAD and call it > + * good. First we should make sure to reset it. This is useful for more > + * complicated stuff but a decent shortcut for the first pass is > + * add_head_to_pending(). > + */ I had to pause over "call it good" for several seconds (since I couldn't understand why someone would want to write "bad" code) until I figured out you meant "do nothing else". It would be clearer simply to drop that, ending the sentence at "HEAD": ..., but for now, we'll just point it at HEAD. > + /* > + * struct setup_revision_opt opt; > + > + * memset(&opt, 0, sizeof(opt)); > + * opt.def = "HEAD"; > + * opt.revarg_opt = REVARG_COMMITTISH; > + * setup_revisions(argc, argv, rev, &opt); > + */ > + > + /* Let's force oneline format. */ > + get_commit_format("oneline", rev); > + rev->verbose_header = 1; > + > + /* add the HEAD to pending so we can start */ > + add_head_to_pending(rev); > +} It would be easier for the reader to associate the add_head_to_pending() invocation with the commented-out setting of "HEAD" via 'setup_revision_opt' if the two bits abutted one another without being separated by the "oneline" gunk.
On Thu, Jun 27, 2019 at 01:06:32AM -0400, Eric Sunshine wrote: > On Wed, Jun 26, 2019 at 7:51 PM Emily Shaffer <emilyshaffer@google.com> wrote: > > `struct rev_info` is what's used by the struct itself. > > What "struct itself"? Do you mean 'struct rev_info' is used by the > _walk_ itself? Or something? Yep, that's the one. Thanks for the fresh eyes. > > > `repo_init_revisions()` initializes the struct; then we need to set it > > up for the walk we want to perform, which is done in > > `final_rev_info_setup()`. > > [...] > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > > --- > > diff --git a/builtin/walken.c b/builtin/walken.c > > @@ -30,6 +31,40 @@ static void init_walken_defaults(void) > > +/* > > + * cmd_log calls a second set of init after the repo_init_revisions call. We'll > > + * mirror those settings in post_repo_init_init. > > + */ > > What is 'post_repo_init_init'? > > I found the reference to cmd_log() confusing because I was looking for > it in this patch (as if it was being introduced here). Newcomers might > be even more confused. Perhaps if you state explicitly that you're > referring to existing code in an existing file, it might be clearer. > Maybe: > > builtin/log.c:cmd_log() calls a second ... > > Overall, I find this entire function comment mystifying. Yeah, this is very stale and never got updated when I realized cmd_log() was calling two init functions for apparently legacy reasons, and I didn't need to mirror it here. Again a case where fresh eyes caught something that became invisible after I stared at it for weeks. I really appreciate you doing the deep review, Eric. I've replaced it: /* * Perform configuration for commit walk here. Within this function we set a * starting point, and can customize our walk in various ways. */ > > > +static void final_rev_info_setup(int argc, const char **argv, const char *prefix, > > + struct rev_info *rev) > > +{ > > + /* > > + * Optional: > > + * setup_revision_opt is used to pass options to the setup_revisions() > > + * call. It's got some special items for submodules and other types of > > + * optimizations, but for now, we'll just point it to HEAD and call it > > + * good. First we should make sure to reset it. This is useful for more > > + * complicated stuff but a decent shortcut for the first pass is > > + * add_head_to_pending(). > > + */ > > I had to pause over "call it good" for several seconds (since I > couldn't understand why someone would want to write "bad" code) until > I figured out you meant "do nothing else". It would be clearer simply > to drop that, ending the sentence at "HEAD": > > ..., but for now, we'll just point it at HEAD. Definitely. Done. > > > + /* > > + * struct setup_revision_opt opt; > > + > > + * memset(&opt, 0, sizeof(opt)); > > + * opt.def = "HEAD"; > > + * opt.revarg_opt = REVARG_COMMITTISH; > > + * setup_revisions(argc, argv, rev, &opt); > > + */ > > + > > + /* Let's force oneline format. */ > > + get_commit_format("oneline", rev); > > + rev->verbose_header = 1; > > + > > + /* add the HEAD to pending so we can start */ > > + add_head_to_pending(rev); > > +} > > It would be easier for the reader to associate the > add_head_to_pending() invocation with the commented-out setting of > "HEAD" via 'setup_revision_opt' if the two bits abutted one another > without being separated by the "oneline" gunk. Good point, done.
diff --git a/builtin/walken.c b/builtin/walken.c index 2474a0d7b2..c463eca843 100644 --- a/builtin/walken.c +++ b/builtin/walken.c @@ -5,6 +5,7 @@ */ #include "builtin.h" +#include "revision.h" #include "config.h" #include "parse-options.h" @@ -30,6 +31,40 @@ static void init_walken_defaults(void) */ } +/* + * cmd_log calls a second set of init after the repo_init_revisions call. We'll + * mirror those settings in post_repo_init_init. + */ +static void final_rev_info_setup(int argc, const char **argv, const char *prefix, + struct rev_info *rev) +{ + /* + * Optional: + * setup_revision_opt is used to pass options to the setup_revisions() + * call. It's got some special items for submodules and other types of + * optimizations, but for now, we'll just point it to HEAD and call it + * good. First we should make sure to reset it. This is useful for more + * complicated stuff but a decent shortcut for the first pass is + * add_head_to_pending(). + */ + + /* + * struct setup_revision_opt opt; + + * memset(&opt, 0, sizeof(opt)); + * opt.def = "HEAD"; + * opt.revarg_opt = REVARG_COMMITTISH; + * setup_revisions(argc, argv, rev, &opt); + */ + + /* Let's force oneline format. */ + get_commit_format("oneline", rev); + rev->verbose_header = 1; + + /* add the HEAD to pending so we can start */ + add_head_to_pending(rev); +} + /* * This method will be called back by git_config(). It is used to gather values * from the configuration files available to Git. @@ -61,6 +96,8 @@ int cmd_walken(int argc, const char **argv, const char *prefix) OPT_END() }; + struct rev_info rev; + /* * parse_options() handles showing usage if incorrect options are * provided, or if '-h' is passed. @@ -71,6 +108,19 @@ int cmd_walken(int argc, const char **argv, const char *prefix) git_config(git_walken_config, NULL); + /* + * Time to set up the walk. repo_init_revisions sets up rev_info with + * the defaults, but then you need to make some configuration settings + * to make it do what's special about your walk. + */ + repo_init_revisions(the_repository, &rev, prefix); + + /* + * Before we do the walk, we need to set a starting point by giving it + * something to go in `pending` - that happens in here + */ + final_rev_info_setup(argc, argv, prefix, &rev); + /* * This line is "human-readable" and we are writing a plumbing command, * so we localize it and use the trace library to print only when
`struct rev_info` is what's used by the struct itself. `repo_init_revisions()` initializes the struct; then we need to set it up for the walk we want to perform, which is done in `final_rev_info_setup()`. The most important step here is adding the first object we want to walk to the pending array. Here, we take the easy road and use `add_head_to_pending()`; there is also a way to do it with `setup_revision_opt()` and `setup_revisions()` which we demonstrate but do not use. If we were to forget this step, the walk would do nothing - the pending queue would be checked, determined to be empty, and the walk would terminate immediately. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Change-Id: I76754b740227cf17a449f3f536dbbe37031e6f9a --- builtin/walken.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)