Re: [PATCH 10/14] pull: set reflog message

From: Junio C Hamano <gitster@pobox.com>
Date: May 18, 2015, 21:53 UTC
Message-ID: <xmqqmw11a6yk.fsf@gitster.dls.corp.google.com>
In-Reply-To: <390e789e8f84122d52185f22e6e798f2@www.dscho.org>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 21 quoted lines

>>  /**
>> + * Sets the GIT_REFLOG_ACTION environment variable to the concatenation of argv
>> + */
>> +static void set_reflog_message(int argc, const char **argv)
>> +{
>> +	int i;
>> +	struct strbuf msg = STRBUF_INIT;
>> +
>> +	for (i = 0; i < argc; i++) {
>> +		strbuf_addstr(&msg, argv[i]);
>> +		strbuf_addch(&msg, ' ');
>> +	}
>> +
>> +	strbuf_rtrim(&msg);
>
> Since this is not performance critical code, I would be slightly in
> favor of simplifying thusly:
>
> /* Join the argument list, separated by spaces */
> for (i = 0; i < argc; i++)
> 	strbuf_addf(&msg, "%s%s", i ? " " : "", argv[i]);

Yeah, either that, or "insert separator only before adding to something else" pattern, i.e.

	for (i = 0; i < argc; i++) {
        	if (i)
                	addch(&msg, ' ');
		addstr(&msg, argv[i]);
	}

Both are easier to read than "always append SP and trim at the end". Besides, trimming at the end makes readers wonder if there are cases where argv[argc-1] ends with whitespaces and the code on purpose removes them.