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

[liam Beguin <liambeguin@gmail.com>]

Hi Johannes,

On 27/11/17 06:04 PM, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Sun, 26 Nov 2017, Liam Beguin wrote:
> 
>> diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
>> index 30ae08cb5a4b..0820b60f6e12 100644
>> --- a/Documentation/rebase-config.txt
>> +++ b/Documentation/rebase-config.txt
>> @@ -30,3 +30,22 @@ rebase.instructionFormat::
>>  	A format string, as specified in linkgit:git-log[1], to be used for the
>>  	todo list during an interactive rebase.  The format will
>>  	automatically have the long commit hash prepended to the format.
>> +
>> +rebase.abbreviateCommands::
>> +	If set to true, `git rebase` will use abbreviated command names in the
>> +	todo list resulting in something like this:
>> +
>> +-------------------------------------------
>> +	p deadbee The oneline of the commit
>> +	p fa1afe1 The oneline of the next commit
>> +	...
>> +-------------------------------------------
> 
> I *think* that AsciiDoc will render this in a different way from what we
> want, but I am not an AsciiDoc expert. In my hands, I always had to add a
> single + in an otherwise empty line to start a new indented paragraph *and
> then continue with non-indented lines*.
> 
>> diff --git a/sequencer.c b/sequencer.c
>> index 810b7850748e..aa01e8bd9280 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -795,6 +795,13 @@ static const char *command_to_string(const enum todo_command command)
>>  	die("Unknown command: %d", command);
>>  }
>>  
>> +static const char command_to_char(const enum todo_command command)
>> +{
>> +	if (command < TODO_COMMENT && todo_command_info[command].c)
>> +		return todo_command_info[command].c;
>> +	return -1;
> 
> My initial reaction was: should we return comment_line_char instead of -1
> here? Only after reading how this is called did I realize that the idea is
> to use full command names if there is no abbreviation. Not sure whether
> this is worth a code comment. What do you think?
> 

I guess it probably deserves a comment!

>> +}
>> +
>>  static int is_noop(const enum todo_command command)
>>  {
>>  	return TODO_NOOP <= command;
>> @@ -1242,15 +1249,16 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
>>  		return 0;
>>  	}
>>  
>> -	for (i = 0; i < TODO_COMMENT; i++)
>> +	for (i = 0; i < TODO_COMMENT; i++) {
>>  		if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
>>  			item->command = i;
>>  			break;
>> -		} else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
>> +		} else if (bol[1] == ' ' && *bol == command_to_char(i)) {
>>  			bol++;
>>  			item->command = i;
>>  			break;
>>  		}
>> +	}
>>  	if (i >= TODO_COMMENT)
>>  		return -1;
>>  
> 
> I would prefer this hunk to be skipped, it does not really do anything if
> I understand correctly.

Ok, I was not so sure about this but thought it was probably worth it.
Will remove.

> 
>> @@ -2443,8 +2451,8 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
>>  	strbuf_release(&sob);
>>  }
>>  
>> -int sequencer_make_script(int keep_empty, FILE *out,
>> -		int argc, const char **argv)
>> +int sequencer_make_script(int keep_empty, int abbreviate_commands, FILE *out,
>> +			  int argc, const char **argv)
>>  {
>>  	char *format = NULL;
>>  	struct pretty_print_context pp = {0};
>> @@ -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?

> 
>>  		pretty_print_commit(&pp, commit, &buf);
>>  		strbuf_addch(&buf, '\n');
>>  		fputs(buf.buf, out);
>> @@ -2539,7 +2549,7 @@ int add_exec_commands(const char *command)
>>  	return 0;
>>  }
>>  
>> -int transform_todo_ids(int shorten_ids)
>> +int transform_todo_ids(int shorten_ids, int abbreviate_commands)
>>  {
>>  	const char *todo_file = rebase_path_todo();
>>  	struct todo_list todo_list = TODO_LIST_INIT;
>> @@ -2575,19 +2585,33 @@ int transform_todo_ids(int shorten_ids)
>>  			todo_list.items[i + 1].offset_in_buf :
>>  			todo_list.buf.len;
>>  
>> -		if (item->command >= TODO_EXEC && item->command != TODO_DROP)
>> -			fwrite(p, eol - bol, 1, out);
>> -		else {
>> +		if (item->command >= TODO_EXEC && item->command != TODO_DROP) {
>> +			if (!abbreviate_commands || command_to_char(item->command) < 0) {
>> +				fwrite(p, eol - bol, 1, out);
>> +			} else {
>> +				const char *end_of_line = strchrnul(p, '\n');
>> +				p += strspn(p, " \t"); /* skip whitespace */
>> +				p += strcspn(p, " \t"); /* skip command */
>> +				fprintf(out, "%c%.*s\n",
>> +					command_to_char(item->command),
>> +					(int)(end_of_line - p), p);
>> +			}
>> +		} else {
>>  			const char *id = shorten_ids ?
>>  				short_commit_name(item->commit) :
>>  				oid_to_hex(&item->commit->object.oid);
>> -			int len;
>>  
>> -			p += strspn(p, " \t"); /* left-trim command */
>> -			len = strcspn(p, " \t"); /* length of command */
>> -
>> -			fprintf(out, "%.*s %s %.*s\n",
>> -				len, p, id, item->arg_len, item->arg);
>> +			if (abbreviate_commands) {
>> +				fprintf(out, "%c %s %.*s\n",
>> +					command_to_char(item->command),
>> +					id, item->arg_len, item->arg);
>> +			} else {
>> +				int len;
>> +				p += strspn(p, " \t"); /* left-trim command */
>> +				len = strcspn(p, " \t"); /* length of command */
>> +				fprintf(out, "%.*s %s %.*s\n",
>> +					len, p, id, item->arg_len, item->arg);
>> +			}
> 
> This hunk changes indentation quite a bit, therefore it is a bit harder to
> read than necessary (and the resulting code, too, as it is more smooshed
> against the 80-column boundary on the right).
> 
> How about this instead:
> 
> -		if (item->command >= TODO_EXEC && item->command != TODO_DROP)
> +		if (abbreviate_commands && command_to_char(item->command)) {
> +			const char *id = shorten_ids ?
> +				short_commit_name(item->commit) :
> +				oid_to_hex(&item->commit->object.oid);
> +			fprintf(out, "%c %s %.*s\n",
> +				command_to_char(item->command),
> +				id, item->arg_len, item->arg);
> +		} else if (item->command >= TODO_EXEC &&
> +			 item->command != TODO_DROP)
> 
> i.e. test first for the short and sweet case that we want (and can)
> abbreviate the command, otherwise keep the code as before?

That looks quite better! I'll update.

> 
> Ciao,
> Dscho
> 

Thanks,
Liam