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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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?

> +}
> +
>  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.

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

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

Ciao,
Dscho