Hi again,
Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> > Currently, "commit" and "me" are global static variables. Since we
> > want to develop the functionality to either pick/ revert individual
> > commits atomically later in the series, make them local variables.
>
> I suppose the idea is that the current commit and whether we are
> cherry-picking or reverting is not global state and should be allowed
> to differ between threads, or that for easier debugging we would like
> to narrow their scope.
>
> How does this relate to the sequencer series? Maybe the idea is that
> they are explicit parameters in the functions that will be exposed
> rather than that they are local variables?
Right. I'll attempt to reword this in the next iteration.
> >
> > Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
> > ---
> > The variable "me" is nowhere as fundamental as "commit" -- it's
> > simply a string derived from a more fundamental "action".
>
> That suggests to me that "action" should probably be made local at the
> same time. On second thought, it looks like this commit is doing two
> unrelated things ---
>
> - simplifying the state that has to be kept by computing "me"
> from "action" on the fly
>
> - narrowing the scope of "commit" and passing it around explicitly
>
> and would be clearer as two separate commits.
Good idea -- I'll split this up into two distinct commits in the next
iteration.
> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> [...]
> > @@ -51,7 +49,7 @@ static size_t xopts_nr, xopts_alloc;
> >
> > #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
> >
> > -static char *get_encoding(const char *message);
> > +static char *get_encoding(struct commit *commit, const char *message);
>
> If the die is converted to an assert or die("BUG: ...") without
> specifying which commit then this first parameter is not needed.
Agreed. It should probably be an assertion failure, since the caller
should use the get_encoding calling API responsibly.
> > @@ -187,7 +186,8 @@ static char *get_encoding(const char *message)
> > return NULL;
> > }
> >
> > -static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
> > +static void add_message_to_msg(struct commit *commit, struct strbuf *msgbuf,
> > + const char *message)
>
> Perhaps the new parameter could be "const char *fallback" and the
> caller call sha1_to_hex unconditionally? (Yes, it sounds like wasted
> computation, but it might be worth the clarity.)
and
> > @@ -200,7 +200,7 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
> > strbuf_addstr(msgbuf, p);
> > }
> >
> > -static int write_cherry_pick_head(void)
> > +static int write_cherry_pick_head(struct commit *commit)
>
> Ah, it might not be wasted computation. This could take
> commit_sha1_hex as parameter so it only needs to be computed once.
Okay.
> > @@ -319,6 +319,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
> > int clean, index_fd;
> > const char **xopt;
> > static struct lock_file index_lock;
> > + const char *me = (action == REVERT ? "revert" : "cherry-pick");
>
> Style: I find this clearer without the parentheses (but feel free to
> ignore).
>
> [...]
> > @@ -402,6 +403,7 @@ static int do_pick_commit(void)
> > struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
> > char *defmsg = NULL;
> > struct strbuf msgbuf = STRBUF_INIT;
> > + const char *me = (action == REVERT ? "revert" : "cherry-pick");
> > int res;
> >
> > if (no_commit) {
> > @@ -458,9 +460,10 @@ static int do_pick_commit(void)
> > /* TRANSLATORS: The first %s will be "revert" or
> > "cherry-pick", the second %s a SHA1 */
> > return error(_("%s: cannot parse parent commit %s"),
> > - me, sha1_to_hex(parent->object.sha1));
> > + action == REVERT ? "revert" : "cherry-pick",
> > + sha1_to_hex(parent->object.sha1));
>
> I think one of the computations of "me" is left over.
Right; leaked into another patch -- rebase fail :|
> > @@ -562,10 +565,13 @@ static int prepare_revs(struct rev_info *revs)
> > return 0;
> > }
> >
> > -static int read_and_refresh_cache(const char *me)
> > +static int read_and_refresh_cache(void)
>
> Since you seem to be moving towards having fewer statics and more
> explicit parameters, I think this part is a step backwards. Maybe it
> should take "action" as a parameter instead.
I'll think about this.
> > @@ -583,10 +589,12 @@ static int read_and_refresh_cache(const char *me)
> > static int revert_or_cherry_pick(int argc, const char **argv)
> > {
> > struct rev_info revs;
> > + struct commit *commit;
> > + const char *me;
> > int res;
> >
> > git_config(git_default_config, NULL);
> > - me = action == REVERT ? "revert" : "cherry-pick";
> > + me = (action == REVERT ? "revert" : "cherry-pick");
>
> Why?
Consistency, mainly. I can't remember operator precedence, and there
are three operators in that line. Either way, I'll lose the
paranthesis if it's clear enough otherwise.
> > setenv(GIT_REFLOG_ACTION, me, 0);
> > parse_args(argc, argv);
> >
>
> Sorry, mostly nitpicks. Still, hope that helps.
Yes. Thanks.
-- Ram
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html