Re: [PATCH 03/10] revert: Eliminate global "commit" variable

From: Junio C Hamano <gitster@pobox.com>
Date: May 25, 2011, 23:10 UTC
Message-ID: <7v1uzm1im7.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1306333025-29893-4-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 9 quoted lines

> diff --git a/builtin/revert.c b/builtin/revert.c
> index 523d41a..6c485f6 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -37,7 +37,6 @@ static const char * const cherry_pick_usage[] = {
>  
>  static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
>  static enum { REVERT, CHERRY_PICK } action;
> -static struct commit *commit;

I agree that the stated goal of this change is a worthy one.

Show 8 quoted lines

> @@ -116,11 +115,12 @@ struct commit_message {
>  	const char *message;
>  };
>  
> -static int get_message(const char *raw_message, struct commit_message *out)
> +static int get_message(const char *sha1_abbrev, const char *raw_message,
> +		struct commit_message *out)
>  {

This somehow feels dubious and also goes against the "let's not rely on global variable commit", no?

This function relied on the global variable and also got information passed from the caller that this function could have derived from that global commit object (i.e. raw_message came from commit->buffer).

A logical thing to fix that situation would be to make its signature:

    static int get_message(struct commit *commit, struct commit_message *out)

no?

Especially dubious is this part:

Show 12 quoted lines

> @@ -139,17 +139,14 @@ static int get_message(const char *raw_message, struct commit_message *out)
>  	if (out->reencoded_message)
>  		out->message = out->reencoded_message;
>  
> -	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
> -	abbrev_len = strlen(abbrev);
> -
>  	subject_len = find_commit_subject(out->message, &subject);
>  
> -	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
> +	out->parent_label = xmalloc(strlen("parent of ") + DEFAULT_ABBREV +
>  			      strlen("... ") + subject_len + 1);

You moved the call to f-u-a() to the caller, and that is why you have to pass its return value as an extra parameter. If you passed commit you do not have to.

Worse yet, the above xmalloc() and memcpy below are wrong, as you are no longer taking the actual length of the abbreviated object name into account, like the old code did. f-u-a() is _about_ uniqueness, and you can get a string longer than the minimum length you gave to it.

Show 8 quoted lines

>  	q = out->parent_label;
>  	q = mempcpy(q, "parent of ", strlen("parent of "));
>  	out->label = q;
> -	q = mempcpy(q, abbrev, abbrev_len);
> +	q = mempcpy(q, sha1_abbrev, DEFAULT_ABBREV);
>  	q = mempcpy(q, "... ", strlen("... "));
>  	out->subject = q;
>  	q = mempcpy(q, subject, subject_len);

Show 7 quoted lines

> @@ -168,8 +165,7 @@ static char *get_encoding(const char *message)
>  	const char *p = message, *eol;
>  
>  	if (!p)
> -		die (_("Could not read commit message of %s"),
> -				sha1_to_hex(commit->object.sha1));
> +		die (_("BUG: get_encoding() called with NULL"));

Hmm, do we really want to translate this error message?

As the function is file-scope static, I suspect that calling it with NULL is a programmer error, not data error (i.e. it is not like we might get an object with NULL here in some repository created by a different version or implementation of git). Once you got confident (and I hope you will be before this series hits our tree) that no caller calls this function with NULL, we might want to demote this to assert(p) or even remove that if statement altogether.

Show 15 quoted lines

> @@ -185,25 +181,26 @@ 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(const char *fallback, struct strbuf *msgbuf,
> +			const char *message)
>  {
>  	const char *p = message;
>  	while (*p && (*p != '\n' || p[1] != '\n'))
>  		p++;
>  
>  	if (!*p)
> -		strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));
> +		strbuf_addstr(msgbuf, fallback);

Can you explain what the above loop and if statements are doing for me? It seems to be doing something a lot more than what the name of the function suggests, which is a bad sign that the helper is not designed properly with clear semantics in mind. I would probably prefer to see this function removed, and do this kind of thing in the only caller of this confused helper function. You could make an even smaller helper function that implements the logic to determine if there are two consecutive LFs in a string (seen above), but I suspect that would be just the matter of strstr(message, "\n\n")

Show 25 quoted lines

> -static void write_cherry_pick_head(void)
> +static void write_cherry_pick_head(const char *commit_sha1_hex)
>  {
>  	int fd;
>  	struct strbuf buf = STRBUF_INIT;
>  
> -	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
> +	strbuf_addf(&buf, "%s\n", commit_sha1_hex);
>  
>  	fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
>  	if (fd < 0)
> @@ -370,7 +367,7 @@ static int run_git_commit(const char *defmsg)
>  	return run_command_v_opt(args, RUN_GIT_CMD);
>  }
>  
> -static int do_pick_commit(void)
> +static int do_pick_commit(struct commit *commit)
>  {
>  	unsigned char head[20];
>  	struct commit *base, *next, *parent;
> @@ -378,6 +375,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 *sha1_abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);

Given the lifetime rule of f-u-a return value, I think it is a bad design to call it and pass it as an extra parameter to get_message() much later.