Re: [PATCH 4/5] rebase -i: learn to abbreviate command names

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 29, 2017, 21:40 UTC
Message-ID: <alpine.DEB.2.21.1.1711292236010.6482@virtualbox>
In-Reply-To: <b4331bb3-db5d-e4f5-54db-f04d77385ae7@gmail.com>

Hi Liam,

On Tue, 28 Nov 2017, liam Beguin wrote:

Show 18 quoted lines

> On 27/11/17 06:04 PM, Johannes Schindelin wrote:
> > 
> > On Sun, 26 Nov 2017, Liam Beguin wrote:
> > 
> >> @@ -2483,7 +2491,9 @@ int sequencer_make_script(int keep_empty, FILE *out,
> >>  		strbuf_reset(&buf);
> >>  		if (!keep_empty && is_original_commit_empty(commit))
> >>  			strbuf_addf(&buf, "%c ", comment_line_char);
> >> -		strbuf_addf(&buf, "pick %s ", oid_to_hex(&commit->object.oid));
> >> +		strbuf_addf(&buf, "%s %s ",
> >> +			    abbreviate_commands ? "p" : "pick",
> >> +			    oid_to_hex(&commit->object.oid));
> > 
> > I guess the compiler will optimize this code so that the conditional
> > is evaluated only once. Not that this is performance critical ;-)
> 
> Is your guess enough? :-) If not, how could I make sure this is
> optimized?  Should I do that check before the while() loop?

I am a fan of not relying too heavily on compiler optimization and e.g. extract code from loops when it does not need to be evaluated every single iteration. In this case:

	const char *pick = abbreviate_commands ? "p" : "pick";
	...
		strbuf_addf(&buf, "%s %s ", pick,
			    oid_to_hex(&commit->object.oid));

But given Junio's comment that the assignment of `first` was too far away from the line where it is used for his taste, I guess he will argue (once again) the exact opposite of me.

Ciao, Dscho