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
