Re: [PATCH 2/8] revert: Make "commit" and "me" local variables

Hi again,

Jonathan Nieder writes:

Ramkumar Ramachandra wrote:

quoted

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.

quoted

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 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.

quoted

--- a/builtin/revert.c
+++ b/builtin/revert.c

[...]

quoted

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

quoted

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

quoted

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

quoted

@@ -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).

[...]

quoted

@@ -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 :|

quoted

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

quoted

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

quoted

 	setenv(GIT_REFLOG_ACTION, me, 0);
 	parse_args(argc, argv);
 

Sorry, mostly nitpicks.  Still, hope that helps.

Yes.  Thanks.

-- Ram