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
