[PATCH 0/5] rebase -i: add config to abbreviate command names

[PATCH 0/5] rebase -i: add config to abbreviate command names

[Liam Beguin]

Hi everyone,

This series is a respin of something [1] I sent a few months ago. This
time, instead of shell, It's based on top of the C implementation of the
interactive rebase. I've also tried to address the comments that were
left in the last thread.

This series will add the 'rebase.abbreviateCommands' configuration
option to allow `git rebase -i` to default to the single-letter command
names when generating the todo list.

Using single-letter command names can present two benefits. First, it
makes it easier to change the action since you only need to replace a
single character (i.e.: in vim "r<character>" instead of
"ciw<character>").  Second, using this with a large enough value of
'core.abbrev' enables the lines of the todo list to remain aligned
making the files easier to read.

Changes since last time:
- Implement abbreviateCommands in rebase--helper
- Add note on the --[no-]autosquash option in rebase.autoSquash
- Add exec commands via the rebase--helper
- Add test case for rebase.abbreviateCommands

Liam Beguin (5):
  Documentation: move rebase.* configs to new file
  Documentation: use preferred name for the 'todo list' script
  rebase -i: add exec commands via the rebase--helper
  rebase -i: learn to abbreviate command names
  t3404: add test case for abbreviated commands

 Documentation/config.txt        |  31 +------------
 Documentation/git-rebase.txt    |  19 +-------
 Documentation/rebase-config.txt |  51 ++++++++++++++++++++
 builtin/rebase--helper.c        |  17 +++++--
 git-rebase--interactive.sh      |  23 +--------
 sequencer.c                     | 100 ++++++++++++++++++++++++++++++++++------
 sequencer.h                     |   5 +-
 t/t3404-rebase-interactive.sh   |  32 +++++++++++++
 8 files changed, 186 insertions(+), 92 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

[1] https://public-inbox.org/git/20170502040048.9065-1-liambeguin@gmail.com/
--
2.15.0.321.g19bf2bb99cee.dirty

[PATCH 1/5] Documentation: move rebase.* configs to new file

[Liam Beguin]

Move all rebase.* configuration variables to a separate file in order to
remove duplicates, and include it in config.txt and git-rebase.txt.  The
new descriptions are mostly taken from config.txt as they are more
verbose.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 Documentation/config.txt        | 31 +------------------------------
 Documentation/git-rebase.txt    | 19 +------------------
 Documentation/rebase-config.txt | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 531649cb40ea..e424b7de90b5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2691,36 +2691,7 @@ push.recurseSubmodules::
 	is retained. You may override this configuration at time of push by
 	specifying '--recurse-submodules=check|on-demand|no'.
 
-rebase.stat::
-	Whether to show a diffstat of what changed upstream since the last
-	rebase. False by default.
-
-rebase.autoSquash::
-	If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-	When set to true, automatically create a temporary stash entry
-	before the operation begins, and apply it after the operation
-	ends.  This means that you can run rebase on a dirty worktree.
-	However, use with care: the final stash application after a
-	successful rebase might result in non-trivial conflicts.
-	Defaults to false.
-
-rebase.missingCommitsCheck::
-	If set to "warn", git rebase -i will print a warning if some
-	commits are removed (e.g. a line was deleted), however the
-	rebase will still proceed. If set to "error", it will print
-	the previous warning and stop the rebase, 'git rebase
-	--edit-todo' can then be used to correct the error. If set to
-	"ignore", no checking is done.
-	To drop a commit without warning or error, use the `drop`
-	command in the todo-list.
-	Defaults to "ignore".
-
-rebase.instructionFormat::
-	A format string, as specified in linkgit:git-log[1], to be used for
-	the instruction list during an interactive rebase.  The format will automatically
-	have the long commit hash prepended to the format.
+include::rebase-config.txt[]
 
 receive.advertiseAtomic::
 	By default, git-receive-pack will advertise the atomic push
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3cedfb0fd22b..8a861c1e0d69 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -203,24 +203,7 @@ Alternatively, you can undo the 'git rebase' with
 CONFIGURATION
 -------------
 
-rebase.stat::
-	Whether to show a diffstat of what changed upstream since the last
-	rebase. False by default.
-
-rebase.autoSquash::
-	If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-	If set to true enable `--autostash` option by default.
-
-rebase.missingCommitsCheck::
-	If set to "warn", print warnings about removed commits in
-	interactive mode. If set to "error", print the warnings and
-	stop the rebase. If set to "ignore", no checking is
-	done. "ignore" by default.
-
-rebase.instructionFormat::
-	Custom commit list format to use during an `--interactive` rebase.
+include::rebase-config.txt[]
 
 OPTIONS
 -------
diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
new file mode 100644
index 000000000000..dba088d7c68f
--- /dev/null
+++ b/Documentation/rebase-config.txt
@@ -0,0 +1,32 @@
+rebase.stat::
+	Whether to show a diffstat of what changed upstream since the last
+	rebase. False by default.
+
+rebase.autoSquash::
+	If set to true enable `--autosquash` option by default.
+
+rebase.autoStash::
+	When set to true, automatically create a temporary stash entry
+	before the operation begins, and apply it after the operation
+	ends.  This means that you can run rebase on a dirty worktree.
+	However, use with care: the final stash application after a
+	successful rebase might result in non-trivial conflicts.
+	This option can be overridden by the `--no-autostash` and
+	`--autostash` options of linkgit:git-rebase[1].
+	Defaults to false.
+
+rebase.missingCommitsCheck::
+	If set to "warn", git rebase -i will print a warning if some
+	commits are removed (e.g. a line was deleted), however the
+	rebase will still proceed. If set to "error", it will print
+	the previous warning and stop the rebase, 'git rebase
+	--edit-todo' can then be used to correct the error. If set to
+	"ignore", no checking is done.
+	To drop a commit without warning or error, use the `drop`
+	command in the todo-list.
+	Defaults to "ignore".
+
+rebase.instructionFormat::
+	A format string, as specified in linkgit:git-log[1], to be used for the
+	instruction list during an interactive rebase.  The format will
+	automatically have the long commit hash prepended to the format.
-- 
2.15.0.321.g19bf2bb99cee.dirty

[PATCH 2/5] Documentation: use preferred name for the 'todo list' script

[Liam Beguin]

Use "todo list" instead of "instruction list" or "todo-list" to
reduce further confusion regarding the name of this script.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 Documentation/rebase-config.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index dba088d7c68f..30ae08cb5a4b 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -23,10 +23,10 @@ rebase.missingCommitsCheck::
 	--edit-todo' can then be used to correct the error. If set to
 	"ignore", no checking is done.
 	To drop a commit without warning or error, use the `drop`
-	command in the todo-list.
+	command in the todo list.
 	Defaults to "ignore".
 
 rebase.instructionFormat::
 	A format string, as specified in linkgit:git-log[1], to be used for the
-	instruction list during an interactive rebase.  The format will
+	todo list during an interactive rebase.  The format will
 	automatically have the long commit hash prepended to the format.
-- 
2.15.0.321.g19bf2bb99cee.dirty

[PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[Liam Beguin]

Recent work on `git-rebase--interactive` aim to convert shell code to C.
Even if this is most likely not a big performance enhacement, let's
convert it too since a comming change to abbreviate command names requires
it to be updated.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 builtin/rebase--helper.c   |  7 ++++++-
 git-rebase--interactive.sh | 23 +----------------------
 sequencer.c                | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 sequencer.h                |  1 +
 4 files changed, 54 insertions(+), 23 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f8519363a393..9d94c874c5bb 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -15,7 +15,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	int keep_empty = 0;
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
-		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
+		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
+		ADD_EXEC
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -36,6 +37,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 			N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
 		OPT_CMDMODE(0, "rearrange-squash", &command,
 			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
+		OPT_CMDMODE(0, "add-exec", &command,
+			N_("insert exec commands in todo list"), ADD_EXEC),
 		OPT_END()
 	};
 
@@ -64,5 +67,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!skip_unnecessary_picks();
 	if (command == REARRANGE_SQUASH && argc == 1)
 		return !!rearrange_squash();
+	if (command == ADD_EXEC && argc == 2)
+		return !!add_exec_commands(argv[1]);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 437815669f00..760334d3a8b3 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -722,27 +722,6 @@ collapse_todo_ids() {
 	git rebase--helper --shorten-ids
 }
 
-# Add commands after a pick or after a squash/fixup series
-# in the todo list.
-add_exec_commands () {
-	{
-		first=t
-		while read -r insn rest
-		do
-			case $insn in
-			pick)
-				test -n "$first" ||
-				printf "%s" "$cmd"
-				;;
-			esac
-			printf "%s %s\n" "$insn" "$rest"
-			first=
-		done
-		printf "%s" "$cmd"
-	} <"$1" >"$1.new" &&
-	mv "$1.new" "$1"
-}
-
 # Switch to the branch in $into and notify it in the reflog
 checkout_onto () {
 	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
@@ -982,7 +961,7 @@ fi
 
 test -s "$todo" || echo noop >> "$todo"
 test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
-test -n "$cmd" && add_exec_commands "$todo"
+test -n "$cmd" && git rebase--helper --add-exec "$cmd"
 
 todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
 todocount=${todocount##* }
diff --git a/sequencer.c b/sequencer.c
index fa94ed652d2c..810b7850748e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2492,6 +2492,52 @@ int sequencer_make_script(int keep_empty, FILE *out,
 	return 0;
 }
 
+int add_exec_commands(const char *command)
+{
+	const char *todo_file = rebase_path_todo();
+	struct todo_list todo_list = TODO_LIST_INIT;
+	int fd, res, i, first = 1;
+	FILE *out;
+
+	strbuf_reset(&todo_list.buf);
+	fd = open(todo_file, O_RDONLY);
+	if (fd < 0)
+		return error_errno(_("could not open '%s'"), todo_file);
+	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
+		close(fd);
+		return error(_("could not read '%s'."), todo_file);
+	}
+	close(fd);
+
+	res = parse_insn_buffer(todo_list.buf.buf, &todo_list);
+	if (res) {
+		todo_list_release(&todo_list);
+		return error(_("unusable todo list: '%s'"), todo_file);
+	}
+
+	out = fopen(todo_file, "w");
+	if (!out) {
+		todo_list_release(&todo_list);
+		return error(_("unable to open '%s' for writing"), todo_file);
+	}
+	for (i = 0; i < todo_list.nr; i++) {
+		struct todo_item *item = todo_list.items + i;
+		int bol = item->offset_in_buf;
+		const char *p = todo_list.buf.buf + bol;
+		int eol = i + 1 < todo_list.nr ?
+			todo_list.items[i + 1].offset_in_buf :
+			todo_list.buf.len;
+
+		if (item->command == TODO_PICK && !first)
+			fputs(command, out);
+		fwrite(p, eol - bol, 1, out);
+		first = 0;
+	}
+	fputs(command, out);
+	fclose(out);
+	todo_list_release(&todo_list);
+	return 0;
+}
 
 int transform_todo_ids(int shorten_ids)
 {
diff --git a/sequencer.h b/sequencer.h
index 6f3d3df82c0a..a2715e6c7589 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -48,6 +48,7 @@ int sequencer_remove_state(struct replay_opts *opts);
 int sequencer_make_script(int keep_empty, FILE *out,
 		int argc, const char **argv);
 
+int add_exec_commands(const char *command);
 int transform_todo_ids(int shorten_ids);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
-- 
2.15.0.321.g19bf2bb99cee.dirty

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

[Liam Beguin]

`git rebase -i` already know how to interpret single-letter command
names. Teach it to generate the todo list with these same abbreviated
names.

Based-on-patch-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 Documentation/rebase-config.txt | 19 +++++++++++++++
 builtin/rebase--helper.c        | 10 +++++---
 sequencer.c                     | 54 +++++++++++++++++++++++++++++------------
 sequencer.h                     |  4 +--
 4 files changed, 66 insertions(+), 21 deletions(-)

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
+	...
+-------------------------------------------
+
+	instead of:
+
+-------------------------------------------
+	pick deadbee The oneline of the commit
+	pick fa1afe1 The oneline of the next commit
+	...
+-------------------------------------------
+	Defaults to false.
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 9d94c874c5bb..7b1fe825a877 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,7 +12,7 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts = REPLAY_OPTS_INIT;
-	int keep_empty = 0;
+	int keep_empty = 0, abbreviate_commands = 0;
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
@@ -43,6 +43,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_default_config, NULL);
+	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
 
 	opts.action = REPLAY_INTERACTIVE_REBASE;
 	opts.allow_ff = 1;
@@ -56,11 +57,12 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	if (command == ABORT && argc == 1)
 		return !!sequencer_remove_state(&opts);
 	if (command == MAKE_SCRIPT && argc > 1)
-		return !!sequencer_make_script(keep_empty, stdout, argc, argv);
+		return !!sequencer_make_script(keep_empty, abbreviate_commands,
+					       stdout, argc, argv);
 	if (command == SHORTEN_SHA1S && argc == 1)
-		return !!transform_todo_ids(1);
+		return !!transform_todo_ids(1, abbreviate_commands);
 	if (command == EXPAND_SHA1S && argc == 1)
-		return !!transform_todo_ids(0);
+		return !!transform_todo_ids(0, abbreviate_commands);
 	if (command == CHECK_TODO_LIST && argc == 1)
 		return !!check_todo_list();
 	if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
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;
+}
+
 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;
 
@@ -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));
 		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);
+			}
 		}
 	}
 	fclose(out);
diff --git a/sequencer.h b/sequencer.h
index a2715e6c7589..cee8394673de 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -45,11 +45,11 @@ int sequencer_continue(struct replay_opts *opts);
 int sequencer_rollback(struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
-int sequencer_make_script(int keep_empty, FILE *out,
+int sequencer_make_script(int keep_empty, int abbreviate_commands, FILE *out,
 		int argc, const char **argv);
 
 int add_exec_commands(const char *command);
-int transform_todo_ids(int shorten_ids);
+int transform_todo_ids(int shorten_ids, int abbreviate_commands);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
-- 
2.15.0.321.g19bf2bb99cee.dirty

[PATCH 5/5] t3404: add test case for abbreviated commands

[Liam Beguin]

Make sure the todo list ends up using single-letter command
abbreviations when the rebase.abbreviateCommands is enabled.
This configuration options should not change anything else.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 t/t3404-rebase-interactive.sh | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 6a82d1ed876d..e460ebde3393 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1260,6 +1260,38 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_success 'prepare rebase.abbreviateCommands' '
+	reset_rebase &&
+	git checkout -b abbrevcmd master &&
+	test_commit "first" file1.txt "first line" first &&
+	test_commit "second" file1.txt "another line" second &&
+	test_commit "fixup! first" file2.txt "first line again" first_fixup &&
+	test_commit "squash! second" file1.txt "another line here" second_squash
+'
+
+cat >expected <<EOF &&
+p $(git rev-list --abbrev-commit -1 first) first
+f $(git rev-list --abbrev-commit -1 first_fixup) fixup! first
+x git show HEAD
+p $(git rev-list --abbrev-commit -1 second) second
+s $(git rev-list --abbrev-commit -1 second_squash) squash! second
+x git show HEAD
+EOF
+
+test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
+	test_when_finished "
+		git checkout master &&
+		test_might_fail git branch -D abbrevcmd &&
+		test_might_fail git rebase --abort
+	" &&
+	git checkout abbrevcmd &&
+	set_cat_todo_editor &&
+	test_config rebase.abbreviateCommands true &&
+	test_must_fail git rebase -i --exec "git show HEAD" \
+		--autosquash master >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'static check of bad command' '
 	rebase_setup_and_clean bad-cmd &&
 	set_fake_editor &&
-- 
2.15.0.321.g19bf2bb99cee.dirty

Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[Junio C Hamano]

Liam Beguin <liambeguin@gmail.com> writes:

> diff --git a/sequencer.c b/sequencer.c
> index fa94ed652d2c..810b7850748e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2492,6 +2492,52 @@ int sequencer_make_script(int keep_empty, FILE *out,
>  	return 0;
>  }
>  
> +int add_exec_commands(const char *command)
> +{

As the name of a public function, it does not feel that this hints
it strongly enough that it is from and a part of sequencer.c API.

> +	const char *todo_file = rebase_path_todo();
> +	struct todo_list todo_list = TODO_LIST_INIT;
> +	int fd, res, i, first = 1;
> +	FILE *out;

Having had to scan backwards while trying to see what the loop that
uses this variable is doing and if it gets affected by things that
happened before we entered the loop, I'd rather not to see 'first'
initialized here, left unused for quite some time until the loop is
entered.  It would make it a lot easier to follow if it is declared
and left uninitilized here, and set to 1 immediately before the
for() loop that uses it.

> +
> +	strbuf_reset(&todo_list.buf);
> +	fd = open(todo_file, O_RDONLY);
> +	if (fd < 0)
> +		return error_errno(_("could not open '%s'"), todo_file);
> +	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> +		close(fd);
> +		return error(_("could not read '%s'."), todo_file);
> +	}
> +	close(fd);

Is this strbuf_read_file() written in longhand?

> +	res = parse_insn_buffer(todo_list.buf.buf, &todo_list);
> +	if (res) {
> +		todo_list_release(&todo_list);
> +		return error(_("unusable todo list: '%s'"), todo_file);
> +	}
> +
> +	out = fopen(todo_file, "w");
> +	if (!out) {
> +		todo_list_release(&todo_list);
> +		return error(_("unable to open '%s' for writing"), todo_file);
> +	}
> +	for (i = 0; i < todo_list.nr; i++) {
> +		struct todo_item *item = todo_list.items + i;
> +		int bol = item->offset_in_buf;
> +		const char *p = todo_list.buf.buf + bol;
> +		int eol = i + 1 < todo_list.nr ?
> +			todo_list.items[i + 1].offset_in_buf :
> +			todo_list.buf.len;

Should bol and eol be of type size_t instead?  The values that get
assigned to them from other structures are.

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

[Junio C Hamano]

Liam Beguin <liambeguin@gmail.com> writes:

>  	if (command == MAKE_SCRIPT && argc > 1)
> -		return !!sequencer_make_script(keep_empty, stdout, argc, argv);
> +		return !!sequencer_make_script(keep_empty, abbreviate_commands,
> +					       stdout, argc, argv);

This suggests that a preliminary clean-up to update the parameter
list of sequencer_make_script() is in order just before this step.
How about making it like so, perhaps:

    int sequencer_make_script(FILE *out, int ac, char **av, unsigned flags)

where keep_empty becomes just one bit in that flags word.  Then another
bit in the same flags word can be used for this option.

Otherwise, every time somebody comes up with a new and shiny feature
for the function, we'd end up adding more to its parameter list.

Re: [PATCH 0/5] rebase -i: add config to abbreviate command names

[Junio C Hamano]

Liam Beguin <liambeguin@gmail.com> writes:

> Liam Beguin (5):
>   Documentation: move rebase.* configs to new file
>   Documentation: use preferred name for the 'todo list' script
>   rebase -i: add exec commands via the rebase--helper
>   rebase -i: learn to abbreviate command names
>   t3404: add test case for abbreviated commands

I didn't send any comment on [1&2/5] but they both looked good.

Re: [PATCH 5/5] t3404: add test case for abbreviated commands

[Junio C Hamano]

Liam Beguin <liambeguin@gmail.com> writes:

> Make sure the todo list ends up using single-letter command
> abbreviations when the rebase.abbreviateCommands is enabled.
> This configuration options should not change anything else.
>
> Signed-off-by: Liam Beguin <liambeguin@gmail.com>
> ---
>  t/t3404-rebase-interactive.sh | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 6a82d1ed876d..e460ebde3393 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1260,6 +1260,38 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
>  	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>  '
>  
> +test_expect_success 'prepare rebase.abbreviateCommands' '
> +	reset_rebase &&
> +	git checkout -b abbrevcmd master &&
> +	test_commit "first" file1.txt "first line" first &&
> +	test_commit "second" file1.txt "another line" second &&
> +	test_commit "fixup! first" file2.txt "first line again" first_fixup &&
> +	test_commit "squash! second" file1.txt "another line here" second_squash
> +'
> +
> +cat >expected <<EOF &&
> +p $(git rev-list --abbrev-commit -1 first) first
> +f $(git rev-list --abbrev-commit -1 first_fixup) fixup! first
> +x git show HEAD
> +p $(git rev-list --abbrev-commit -1 second) second
> +s $(git rev-list --abbrev-commit -1 second_squash) squash! second
> +x git show HEAD
> +EOF

Please have this cat inside and at the beginning of the next
test_expect_success, preferably indented with HT to align with other
commands (adding '-' to the opening of your here-document makes the
shell strip all leading HTs from the here-document), like this:

	test_expect_success 'respects rebase....' '
		cat expect <<-EOF &&
		p $(git rev-list ...)
		f $(git rev-list ...)
		...
		EOF
		test_when_finished "
                	...
		" &&
		...
		test_cmp expect actual
	'

Re: [PATCH 1/5] Documentation: move rebase.* configs to new file

[Johannes Schindelin]

Hi Liam,

On Sun, 26 Nov 2017, Liam Beguin wrote:

>  3 files changed, 34 insertions(+), 48 deletions(-)

Very nice!
Johannes

Re: [PATCH 2/5] Documentation: use preferred name for the 'todo list' script

[Johannes Schindelin]

Hi Liam,

On Sun, 26 Nov 2017, Liam Beguin wrote:

> Use "todo list" instead of "instruction list" or "todo-list" to
> reduce further confusion regarding the name of this script.

Makes sense,
Johannes

Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[Johannes Schindelin]

Hi Junio,

On Mon, 27 Nov 2017, Junio C Hamano wrote:

> Liam Beguin <liambeguin@gmail.com> writes:
> 
> > diff --git a/sequencer.c b/sequencer.c
> > index fa94ed652d2c..810b7850748e 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2492,6 +2492,52 @@ int sequencer_make_script(int keep_empty, FILE *out,
> >  	return 0;
> >  }
> >  
> > +int add_exec_commands(const char *command)
> > +{
> 
> As the name of a public function, it does not feel that this hints
> it strongly enough that it is from and a part of sequencer.c API.

How about a "yes, and" instead? As in:

To further improve this patch, let's use the name
sequencer_add_exec_commands() for this function because it is defined
globally now.

> > +	const char *todo_file = rebase_path_todo();
> > +	struct todo_list todo_list = TODO_LIST_INIT;
> > +	int fd, res, i, first = 1;
> > +	FILE *out;
> 
> Having had to scan backwards while trying to see what the loop that
> uses this variable is doing and if it gets affected by things that
> happened before we entered the loop, I'd rather not to see 'first'
> initialized here, left unused for quite some time until the loop is
> entered.  It would make it a lot easier to follow if it is declared
> and left uninitilized here, and set to 1 immediately before the
> for() loop that uses it.

Funny, I would have assumed it the other way round: since "first" always
has to be initialized with 1, I would have been surprised to see an
explicit assignment much later than it is declared.

> > +	strbuf_reset(&todo_list.buf);
> > +	fd = open(todo_file, O_RDONLY);
> > +	if (fd < 0)
> > +		return error_errno(_("could not open '%s'"), todo_file);
> > +	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> > +		close(fd);
> > +		return error(_("could not read '%s'."), todo_file);
> > +	}
> > +	close(fd);
> 
> Is this strbuf_read_file() written in longhand?

Ah, this is one of the downsides of patch-based review. If it was reviewed
in context, you would have easily spotted that Liam was merely
copy-editing my code that is still around.

And indeed, I had missed that function when I started to write the
rebase--helper patches.

> > +	res = parse_insn_buffer(todo_list.buf.buf, &todo_list);
> > +	if (res) {
> > +		todo_list_release(&todo_list);
> > +		return error(_("unusable todo list: '%s'"), todo_file);
> > +	}
> > +
> > +	out = fopen(todo_file, "w");
> > +	if (!out) {
> > +		todo_list_release(&todo_list);
> > +		return error(_("unable to open '%s' for writing"), todo_file);
> > +	}
> > +	for (i = 0; i < todo_list.nr; i++) {
> > +		struct todo_item *item = todo_list.items + i;
> > +		int bol = item->offset_in_buf;
> > +		const char *p = todo_list.buf.buf + bol;
> > +		int eol = i + 1 < todo_list.nr ?
> > +			todo_list.items[i + 1].offset_in_buf :
> > +			todo_list.buf.len;
> 
> Should bol and eol be of type size_t instead?  The values that get
> assigned to them from other structures are.

While it won't matter in practice, this would be "more correct" to do,
yes.

Ciao,
Dscho

Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[Johannes Schindelin]

Hi Liam,

could I ask for a favor? I'd like the oneline to start with

	rebase -i -x: ...

(this would help future me to realize what this commit touches already
from the concise graph output I favor).

On Sun, 26 Nov 2017, Liam Beguin wrote:

> Recent work on `git-rebase--interactive` aim to convert shell code to C.
> Even if this is most likely not a big performance enhacement, let's
> convert it too since a comming change to abbreviate command names requires
> it to be updated.

Since Junio did not comment on the commit message: could you replace
`aim` by `aims`, `enhacement` by `enhancement` and `comming` by `coming`?

> @@ -36,6 +37,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  			N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
>  		OPT_CMDMODE(0, "rearrange-squash", &command,
>  			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
> +		OPT_CMDMODE(0, "add-exec", &command,
> +			N_("insert exec commands in todo list"), ADD_EXEC),

Maybe `add-exec-commands`? I know it is longer to type, but these options do
not need to be typed interactively and the longer name would be consistent
with the function name.

> diff --git a/sequencer.c b/sequencer.c
> index fa94ed652d2c..810b7850748e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2492,6 +2492,52 @@ int sequencer_make_script(int keep_empty, FILE *out,
>  	return 0;
>  }
>  

As the code in add_exec_commands() may appear convoluted (why not simply
append the command after any pick?), the original comment would be really
nice here:

	/*
	 * Add commands after pick and (series of) squash/fixup commands
	 * in the todo list.
	 */

> +int add_exec_commands(const char *command)
> +{
> +	const char *todo_file = rebase_path_todo();
> +	struct todo_list todo_list = TODO_LIST_INIT;
> +	int fd, res, i, first = 1;
> +	FILE *out;
> +
> +	strbuf_reset(&todo_list.buf);

The todo_list.buf has been initialized already (via TODO_LIST_INIT), no
need to reset it again.

> +	fd = open(todo_file, O_RDONLY);
> +	if (fd < 0)
> +		return error_errno(_("could not open '%s'"), todo_file);
> +	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> +		close(fd);
> +		return error(_("could not read '%s'."), todo_file);
> +	}
> +	close(fd);

As Junio pointed out so gently: there is a helper function that does this
all very conveniently for us:

	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
		return error_errno(_("could not read '%s'"), todo_file);

And as I realized looking at the surrounding code: you probably just
inherited my inelegant code by copy-editing from another function in
sequencer.c. Should you decide to add a preparatory patch to your patch
series that converts these other callers, or even refactors all that code
that reads the git-rebase-todo file and then parses it, I would be quite
happy... :-) (although I would understand if you deemed this outside the
purpose of your patch series).

> +	res = parse_insn_buffer(todo_list.buf.buf, &todo_list);
> +	if (res) {
> +		todo_list_release(&todo_list);
> +		return error(_("unusable todo list: '%s'"), todo_file);
> +	}

The variable `res` is not really used here. Let's just put the
parse_insn_buffer() call inside the if ().

> +	out = fopen(todo_file, "w");
> +	if (!out) {
> +		todo_list_release(&todo_list);
> +		return error(_("unable to open '%s' for writing"), todo_file);
> +	}
> +	for (i = 0; i < todo_list.nr; i++) {
> +		struct todo_item *item = todo_list.items + i;
> +		int bol = item->offset_in_buf;
> +		const char *p = todo_list.buf.buf + bol;
> +		int eol = i + 1 < todo_list.nr ?
> +			todo_list.items[i + 1].offset_in_buf :
> +			todo_list.buf.len;

This smells like another copy-edited snippet that originated from my
brain, and I am not at all proud by the complexity I used there.

The function should also check for errors during writing. So how about
something like this instead?

	struct strbuf *buf = &todo_list.buf;
	size_t offset = 0, command_len = strlen(command);
	int first = 1, i;
	struct todo_item *item;

	...

	/* insert <command> before every pick except the first one */
	for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++)
		if (item->command == TODO_PICK) {
			if (first)
				first = 0;
			else {
				strbuf_splice(buf,
					      item->offset_in_buf + offset, 0,
					      command, command_len);
				offset += command_len;
			}
		}

	/* append a final <command> */
	strbuf_complete_list(buf);
	strbuf_add(buf, command, command_len);

	i = write_message(buf->buf, buf->len, todo_file, 0);
	todo_list_release(&todo_list);
	return i;

Ciao,
Dscho

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

[Johannes Schindelin]

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

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

[Jeff King]

On Tue, Nov 28, 2017 at 12:04:45AM +0100, Johannes Schindelin wrote:

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

Good catch. Interestingly enough, my asciidoc seems to render this
as desired for the docbook/roff version, but has screwed-up indentation
for the HTML version.

Fixing it as you suggest makes it look good in both (and I think you can
never go wrong with "+"-continuation, aside from making the source a bit
uglier).

Squashable patch below for convenience, since I did try it.

-Peff

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index 0820b60f6e..42e1ba7575 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -34,18 +34,19 @@ rebase.instructionFormat::
 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
 	...
 -------------------------------------------
-
-	instead of:
-
++
+instead of:
++
 -------------------------------------------
 	pick deadbee The oneline of the commit
 	pick fa1afe1 The oneline of the next commit
 	...
 -------------------------------------------
-	Defaults to false.
++
+Defaults to false.

Re: [PATCH 5/5] t3404: add test case for abbreviated commands

[Johannes Schindelin]

Hi Liam,

On Sun, 26 Nov 2017, Liam Beguin wrote:

> Make sure the todo list ends up using single-letter command
> abbreviations when the rebase.abbreviateCommands is enabled.
> This configuration options should not change anything else.

Makes sense. As to the diff:

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 6a82d1ed876d..e460ebde3393 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1260,6 +1260,38 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
>  	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>  '
>  
> +test_expect_success 'prepare rebase.abbreviateCommands' '
> +	reset_rebase &&
> +	git checkout -b abbrevcmd master &&
> +	test_commit "first" file1.txt "first line" first &&
> +	test_commit "second" file1.txt "another line" second &&
> +	test_commit "fixup! first" file2.txt "first line again" first_fixup &&
> +	test_commit "squash! second" file1.txt "another line here" second_squash
> +'

In addition to Junio's suggestion to include the "expected" block in the
next test case, I would be in favor of combining all the new code in a
single test case.

Also, I think that the test_commit calls can be simplified to:

	test_commit first &&
	test_commit second &&
	test_commit "fixup! first" first A dummy1 &&
	test_commit "squash! second" second B dummy2 &&

> +cat >expected <<EOF &&
> +p $(git rev-list --abbrev-commit -1 first) first

Maybe $(git rev-parse --short HEAD~3)?

> +f $(git rev-list --abbrev-commit -1 first_fixup) fixup! first
> +x git show HEAD
> +p $(git rev-list --abbrev-commit -1 second) second
> +s $(git rev-list --abbrev-commit -1 second_squash) squash! second
> +x git show HEAD
> +EOF
> +
> +test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
> +	test_when_finished "
> +		git checkout master &&
> +		test_might_fail git branch -D abbrevcmd &&
> +		test_might_fail git rebase --abort
> +	" &&
> +	git checkout abbrevcmd &&
> +	set_cat_todo_editor &&
> +	test_config rebase.abbreviateCommands true &&
> +	test_must_fail git rebase -i --exec "git show HEAD" \
> +		--autosquash master >actual &&
> +	test_cmp expected actual
> +'

Otherwise, it looks good!

Thank you for staying on the ball and getting this patch series updated.

Ciao,
Dscho

Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> As the name of a public function, it does not feel that this hints
>> it strongly enough that it is from and a part of sequencer.c API.
>
> How about a "yes, and" instead? As in:
>
> To further improve this patch, let's use the name
> sequencer_add_exec_commands() for this function because it is defined
> globally now.

I would do so when I have a single "this is strictly better"
suggestion.  In this case, I didn't, but somebody who does not have
a "better suggestion" can still have a good sense of smell to tell
something is "not right".

>> > +	const char *todo_file = rebase_path_todo();
>> > +	struct todo_list todo_list = TODO_LIST_INIT;
>> > +	int fd, res, i, first = 1;
>> > +	FILE *out;
>> 
>> Having had to scan backwards while trying to see what the loop that
>> uses this variable is doing and if it gets affected by things that
>> happened before we entered the loop, I'd rather not to see 'first'
>> initialized here, left unused for quite some time until the loop is
>> entered.  It would make it a lot easier to follow if it is declared
>> and left uninitilized here, and set to 1 immediately before the
>> for() loop that uses it.
>
> Funny, I would have assumed it the other way round: since "first" always
> has to be initialized with 1, I would have been surprised to see an
> explicit assignment much later than it is declared.

Unfortunately that would force readers to see what happens before
the loop to see if there are cases where first is incremented, and
in this case there is not any.

Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> could I ask for a favor? I'd like the oneline to start with
>
> 	rebase -i -x: ...
>
> (this would help future me to realize what this commit touches already
> from the concise graph output I favor).

Excellent.

>> Recent work on `git-rebase--interactive` aim to convert shell code to C.
>> Even if this is most likely not a big performance enhacement, let's
>> convert it too since a comming change to abbreviate command names requires
>> it to be updated.
>
> Since Junio did not comment on the commit message: could you replace
> `aim` by `aims`, `enhacement` by `enhancement` and `comming` by `coming`?

Yes, I noticed them but don't mind me ;-)  The above are all good fixes.

All suggestions in the remainder looked sensible.  Thanks for a
review.

Re: [PATCH 0/5] rebase -i: add config to abbreviate command names

[liam Beguin]

Hi Junio,

On 27/11/17 12:23 AM, Junio C Hamano wrote:
> Liam Beguin <liambeguin@gmail.com> writes:
> 
>> Liam Beguin (5):
>>   Documentation: move rebase.* configs to new file
>>   Documentation: use preferred name for the 'todo list' script
>>   rebase -i: add exec commands via the rebase--helper
>>   rebase -i: learn to abbreviate command names
>>   t3404: add test case for abbreviated commands
> 
> I didn't send any comment on [1&2/5] but they both looked good.
> 

Thanks for reviewing this. I'll go through your comments and post a
v2 shortly.

Thanks,
Liam

PS: I'm very sorry if someone received a few copies of this, I'm
having issues with my MUA! Hopefully, I've got it right this time...

Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[liam Beguin]

Hi Junio,

On 27/11/17 12:14 AM, Junio C Hamano wrote:
> Liam Beguin <liambeguin@gmail.com> writes:
> 
>> diff --git a/sequencer.c b/sequencer.c
>> index fa94ed652d2c..810b7850748e 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -2492,6 +2492,52 @@ int sequencer_make_script(int keep_empty, FILE *out,
>>  	return 0;
>>  }
>>  
>> +int add_exec_commands(const char *command)
>> +{
> 
> As the name of a public function, it does not feel that this hints
> it strongly enough that it is from and a part of sequencer.c API.
> 
>> +	const char *todo_file = rebase_path_todo();
>> +	struct todo_list todo_list = TODO_LIST_INIT;
>> +	int fd, res, i, first = 1;
>> +	FILE *out;
> 
> Having had to scan backwards while trying to see what the loop that
> uses this variable is doing and if it gets affected by things that
> happened before we entered the loop, I'd rather not to see 'first'
> initialized here, left unused for quite some time until the loop is
> entered.  It would make it a lot easier to follow if it is declared
> and left uninitilized here, and set to 1 immediately before the
> for() loop that uses it.
> 

I agree that moving 'first = 1' just above the for() loop makes it
more obvious. I'm not quite fond of how this is implemented, I just
'translated' the shell code and was hoping on maybe a few comments
on how to improve it.

>> +
>> +	strbuf_reset(&todo_list.buf);
>> +	fd = open(todo_file, O_RDONLY);
>> +	if (fd < 0)
>> +		return error_errno(_("could not open '%s'"), todo_file);
>> +	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
>> +		close(fd);
>> +		return error(_("could not read '%s'."), todo_file);
>> +	}
>> +	close(fd);
> 
> Is this strbuf_read_file() written in longhand?

Thanks for pointing this out! I'll update. And as Johannes pointed out,
I've copied this from surrounding functions, I'll add a preparatory path
to update those too.

> 
>> +	res = parse_insn_buffer(todo_list.buf.buf, &todo_list);
>> +	if (res) {
>> +		todo_list_release(&todo_list);
>> +		return error(_("unusable todo list: '%s'"), todo_file);
>> +	}
>> +
>> +	out = fopen(todo_file, "w");
>> +	if (!out) {
>> +		todo_list_release(&todo_list);
>> +		return error(_("unable to open '%s' for writing"), todo_file);
>> +	}
>> +	for (i = 0; i < todo_list.nr; i++) {
>> +		struct todo_item *item = todo_list.items + i;
>> +		int bol = item->offset_in_buf;
>> +		const char *p = todo_list.buf.buf + bol;
>> +		int eol = i + 1 < todo_list.nr ?
>> +			todo_list.items[i + 1].offset_in_buf :
>> +			todo_list.buf.len;
> 
> Should bol and eol be of type size_t instead?  The values that get
> assigned to them from other structures are.
> 

Will do.
Thanks, 

Liam

Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[liam Beguin]

Hi Johannes,

Thanks for taking the time to review this.

On 27/11/17 05:42 PM, Johannes Schindelin wrote:
> Hi Liam,
> 
> could I ask for a favor? I'd like the oneline to start with
> 
> 	rebase -i -x: ...
> 
> (this would help future me to realize what this commit touches already
> from the concise graph output I favor).

Sure, I'll update the commit subject.

> 
> On Sun, 26 Nov 2017, Liam Beguin wrote:
> 
>> Recent work on `git-rebase--interactive` aim to convert shell code to C.
>> Even if this is most likely not a big performance enhacement, let's
>> convert it too since a comming change to abbreviate command names requires
>> it to be updated.
> 
> Since Junio did not comment on the commit message: could you replace
> `aim` by `aims`, `enhacement` by `enhancement` and `comming` by `coming`?

Ow.. sorry about that! I'll fix those and make sure to proofread better next time!

> 
>> @@ -36,6 +37,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>>  			N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
>>  		OPT_CMDMODE(0, "rearrange-squash", &command,
>>  			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
>> +		OPT_CMDMODE(0, "add-exec", &command,
>> +			N_("insert exec commands in todo list"), ADD_EXEC),
> 
> Maybe `add-exec-commands`? I know it is longer to type, but these options do
> not need to be typed interactively and the longer name would be consistent
> with the function name.

Makes sense. It'll also be more consistent with the rest of the commands above.

> 
>> diff --git a/sequencer.c b/sequencer.c
>> index fa94ed652d2c..810b7850748e 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -2492,6 +2492,52 @@ int sequencer_make_script(int keep_empty, FILE *out,
>>  	return 0;
>>  }
>>  
> 
> As the code in add_exec_commands() may appear convoluted (why not simply
> append the command after any pick?), the original comment would be really
> nice here:
> 
> 	/*
> 	 * Add commands after pick and (series of) squash/fixup commands
> 	 * in the todo list.
> 	 */
> 

I'll make sure to include that comment.
The code is a bit convoluted as you say... I wanted to send it "as is" first
to get comments and update based on feedback from the list.

I just realized we could maybe add exec instructions only after pick commands
if we do add-exec-commands before rearrange-squash. I'll test it out.

>> +int add_exec_commands(const char *command)
>> +{
>> +	const char *todo_file = rebase_path_todo();
>> +	struct todo_list todo_list = TODO_LIST_INIT;
>> +	int fd, res, i, first = 1;
>> +	FILE *out;
>> +
>> +	strbuf_reset(&todo_list.buf);
> 
> The todo_list.buf has been initialized already (via TODO_LIST_INIT), no
> need to reset it again.
> 
>> +	fd = open(todo_file, O_RDONLY);
>> +	if (fd < 0)
>> +		return error_errno(_("could not open '%s'"), todo_file);
>> +	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
>> +		close(fd);
>> +		return error(_("could not read '%s'."), todo_file);
>> +	}
>> +	close(fd);
> 
> As Junio pointed out so gently: there is a helper function that does this
> all very conveniently for us:
> 
> 	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
> 		return error_errno(_("could not read '%s'"), todo_file);
> 
> And as I realized looking at the surrounding code: you probably just
> inherited my inelegant code by copy-editing from another function in
> sequencer.c. Should you decide to add a preparatory patch to your patch
> series that converts these other callers, or even refactors all that code
> that reads the git-rebase-todo file and then parses it, I would be quite
> happy... :-) (although I would understand if you deemed this outside the
> purpose of your patch series).
> 

You guessed well, I mostly did copy-editing... I thought I found this code
a little confusing because I'm not used to as much pointer gymnastics but
it reassures me a bit to read this :-). I'll see if I can come up with a
better solution.

>> +	res = parse_insn_buffer(todo_list.buf.buf, &todo_list);
>> +	if (res) {
>> +		todo_list_release(&todo_list);
>> +		return error(_("unusable todo list: '%s'"), todo_file);
>> +	}
> 
> The variable `res` is not really used here. Let's just put the
> parse_insn_buffer() call inside the if ().
> 

Will do.

>> +	out = fopen(todo_file, "w");
>> +	if (!out) {
>> +		todo_list_release(&todo_list);
>> +		return error(_("unable to open '%s' for writing"), todo_file);
>> +	}
>> +	for (i = 0; i < todo_list.nr; i++) {
>> +		struct todo_item *item = todo_list.items + i;
>> +		int bol = item->offset_in_buf;
>> +		const char *p = todo_list.buf.buf + bol;
>> +		int eol = i + 1 < todo_list.nr ?
>> +			todo_list.items[i + 1].offset_in_buf :
>> +			todo_list.buf.len;
> 
> This smells like another copy-edited snippet that originated from my
> brain, and I am not at all proud by the complexity I used there.
> 
> The function should also check for errors during writing. So how about
> something like this instead?
> 
> 	struct strbuf *buf = &todo_list.buf;
> 	size_t offset = 0, command_len = strlen(command);
> 	int first = 1, i;
> 	struct todo_item *item;
> 
> 	...
> 
> 	/* insert <command> before every pick except the first one */
> 	for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++)
> 		if (item->command == TODO_PICK) {
> 			if (first)
> 				first = 0;
> 			else {
> 				strbuf_splice(buf,
> 					      item->offset_in_buf + offset, 0,
> 					      command, command_len);
> 				offset += command_len;
> 			}
> 		}
> 
> 	/* append a final <command> */
> 	strbuf_complete_list(buf);
> 	strbuf_add(buf, command, command_len);
> 
> 	i = write_message(buf->buf, buf->len, todo_file, 0);
> 	todo_list_release(&todo_list);
> 	return i;
> 

I'll see how I can include this if calling add-exec-commands before
rearrange-squash works. But it definitely is lighter to read.

> Ciao,
> Dscho
> 

Thanks again,

Liam

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

[liam Beguin]

Hi Junio,

On 27/11/17 12:19 AM, Junio C Hamano wrote:
> Liam Beguin <liambeguin@gmail.com> writes:
> 
>>  	if (command == MAKE_SCRIPT && argc > 1)
>> -		return !!sequencer_make_script(keep_empty, stdout, argc, argv);
>> +		return !!sequencer_make_script(keep_empty, abbreviate_commands,
>> +					       stdout, argc, argv);
> 
> This suggests that a preliminary clean-up to update the parameter
> list of sequencer_make_script() is in order just before this step.
> How about making it like so, perhaps:
> 
>     int sequencer_make_script(FILE *out, int ac, char **av, unsigned flags)
> 
> where keep_empty becomes just one bit in that flags word.  Then another
> bit in the same flags word can be used for this option.
> 
> Otherwise, every time somebody comes up with a new and shiny feature
> for the function, we'd end up adding more to its parameter list.
> 

Will do.
Thanks, 

Liam

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

[liam Beguin]

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

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

[liam Beguin]

Hi Peff,

Thanks for taking the time to test this, I'll squash that patch in v2.

On 27/11/17 06:11 PM, Jeff King wrote:
> On Tue, Nov 28, 2017 at 12:04:45AM +0100, Johannes Schindelin wrote:
> 
>>> +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*.
> 
> Good catch. Interestingly enough, my asciidoc seems to render this
> as desired for the docbook/roff version, but has screwed-up indentation
> for the HTML version.
> 
> Fixing it as you suggest makes it look good in both (and I think you can
> never go wrong with "+"-continuation, aside from making the source a bit
> uglier).
> 
> Squashable patch below for convenience, since I did try it.
> 
> -Peff
> 
> diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
> index 0820b60f6e..42e1ba7575 100644
> --- a/Documentation/rebase-config.txt
> +++ b/Documentation/rebase-config.txt
> @@ -34,18 +34,19 @@ rebase.instructionFormat::
>  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
>  	...
>  -------------------------------------------
> -
> -	instead of:
> -
> ++
> +instead of:
> ++
>  -------------------------------------------
>  	pick deadbee The oneline of the commit
>  	pick fa1afe1 The oneline of the next commit
>  	...
>  -------------------------------------------
> -	Defaults to false.
> ++
> +Defaults to false.
> 

Liam

Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[Johannes Schindelin]

Hi Liam,

On Tue, 28 Nov 2017, liam Beguin wrote:

> I just realized we could maybe add exec instructions only after pick
> commands if we do add-exec-commands before rearrange-squash.

That won't work, because the squash/fixup commands are pick commands
before rearrange-squash. So you'd add one unwanted exec per
squash/fixup...

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Liam,

On Tue, 28 Nov 2017, liam Beguin wrote:

> 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

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

I would have called that variable "pick_cmd", not just "pick"; this
preference is minor enough that I would probably reject a patch to
rename from one to the other if the above were already part of the
existing codebase.

I find that the code suggested above easier to follow, simply
because it expresses clearly the flow of thought and that flow of
thought matches how I personally think: we decide how this command
is spelled in the output upfront, and then use that same spelling
consistently throughout the loop.

I do not think it matters performance-wise either way, but I value
how easy it is to follow the code for humans, and it matters much
more in the longer run.  If a compiler does a poor job, we can
eventually notice and help it to produce better code that still does
what we wanted it to do (or it may not be performance critical and
we may not even notice).  If a code is hard to follow, on the other
hand, what we wanted it to do in the first place becomes harder to
figure out.

[PATCH v2 0/9] rebase -i: add config to abbreviate command names

[Liam Beguin]

Hi everyone,

This series will add the 'rebase.abbreviateCommands' configuration
option to allow `git rebase -i` to default to the single-letter command
names when generating the todo list.

Using single-letter command names can present two benefits. First, it
makes it easier to change the action since you only need to replace a
single character (i.e.: in vim "r<character>" instead of
"ciw<character>").  Second, using this with a large enough value of
'core.abbrev' enables the lines of the todo list to remain aligned
making the files easier to read.

Changes in V2:
- Refactor and rename 'transform_todo_ids'
- Replace SHA-1 by OID in rebase--helper.c
- Update todo list related functions to take a generic 'flags' parameter
- Rename 'add_exec_commands' function to 'sequencer_add_exec_commands'
- Rename 'add-exec' option to 'add-exec-commands'
- Use 'strbur_read_file' instead of rewriting it
- Make 'command_to_char' return 'comment_char_line' if no single-letter
  command name is defined
- Combine both tests into a single test case
- Update commit messages

Liam Beguin (9):
  Documentation: move rebase.* configs to new file
  Documentation: use preferred name for the 'todo list' script
  rebase -i: set commit to null in exec commands
  rebase -i: refactor transform_todo_ids
  rebase -i: replace reference to sha1 with oid
  rebase -i: update functions to use a flags parameter
  rebase -i -x: add exec commands via the rebase--helper
  rebase -i: learn to abbreviate command names
  t3404: add test case for abbreviated commands

 Documentation/config.txt        |  31 +-------
 Documentation/git-rebase.txt    |  19 +----
 Documentation/rebase-config.txt |  52 +++++++++++++
 builtin/rebase--helper.c        |  29 +++++---
 git-rebase--interactive.sh      |  23 +-----
 sequencer.c                     | 126 +++++++++++++++++++++-----------
 sequencer.h                     |  10 ++-
 t/t3404-rebase-interactive.sh   |  22 ++++++
 8 files changed, 186 insertions(+), 126 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 1/9] Documentation: move rebase.* configs to new file

[Liam Beguin]

Move all rebase.* configuration variables to a separate file in order to
remove duplicates, and include it in config.txt and git-rebase.txt.  The
new descriptions are mostly taken from config.txt as they are more
verbose.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 Documentation/config.txt        | 31 +------------------------------
 Documentation/git-rebase.txt    | 19 +------------------
 Documentation/rebase-config.txt | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 531649cb40ea..e424b7de90b5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2691,36 +2691,7 @@ push.recurseSubmodules::
 	is retained. You may override this configuration at time of push by
 	specifying '--recurse-submodules=check|on-demand|no'.
 
-rebase.stat::
-	Whether to show a diffstat of what changed upstream since the last
-	rebase. False by default.
-
-rebase.autoSquash::
-	If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-	When set to true, automatically create a temporary stash entry
-	before the operation begins, and apply it after the operation
-	ends.  This means that you can run rebase on a dirty worktree.
-	However, use with care: the final stash application after a
-	successful rebase might result in non-trivial conflicts.
-	Defaults to false.
-
-rebase.missingCommitsCheck::
-	If set to "warn", git rebase -i will print a warning if some
-	commits are removed (e.g. a line was deleted), however the
-	rebase will still proceed. If set to "error", it will print
-	the previous warning and stop the rebase, 'git rebase
-	--edit-todo' can then be used to correct the error. If set to
-	"ignore", no checking is done.
-	To drop a commit without warning or error, use the `drop`
-	command in the todo-list.
-	Defaults to "ignore".
-
-rebase.instructionFormat::
-	A format string, as specified in linkgit:git-log[1], to be used for
-	the instruction list during an interactive rebase.  The format will automatically
-	have the long commit hash prepended to the format.
+include::rebase-config.txt[]
 
 receive.advertiseAtomic::
 	By default, git-receive-pack will advertise the atomic push
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3cedfb0fd22b..8a861c1e0d69 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -203,24 +203,7 @@ Alternatively, you can undo the 'git rebase' with
 CONFIGURATION
 -------------
 
-rebase.stat::
-	Whether to show a diffstat of what changed upstream since the last
-	rebase. False by default.
-
-rebase.autoSquash::
-	If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-	If set to true enable `--autostash` option by default.
-
-rebase.missingCommitsCheck::
-	If set to "warn", print warnings about removed commits in
-	interactive mode. If set to "error", print the warnings and
-	stop the rebase. If set to "ignore", no checking is
-	done. "ignore" by default.
-
-rebase.instructionFormat::
-	Custom commit list format to use during an `--interactive` rebase.
+include::rebase-config.txt[]
 
 OPTIONS
 -------
diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
new file mode 100644
index 000000000000..dba088d7c68f
--- /dev/null
+++ b/Documentation/rebase-config.txt
@@ -0,0 +1,32 @@
+rebase.stat::
+	Whether to show a diffstat of what changed upstream since the last
+	rebase. False by default.
+
+rebase.autoSquash::
+	If set to true enable `--autosquash` option by default.
+
+rebase.autoStash::
+	When set to true, automatically create a temporary stash entry
+	before the operation begins, and apply it after the operation
+	ends.  This means that you can run rebase on a dirty worktree.
+	However, use with care: the final stash application after a
+	successful rebase might result in non-trivial conflicts.
+	This option can be overridden by the `--no-autostash` and
+	`--autostash` options of linkgit:git-rebase[1].
+	Defaults to false.
+
+rebase.missingCommitsCheck::
+	If set to "warn", git rebase -i will print a warning if some
+	commits are removed (e.g. a line was deleted), however the
+	rebase will still proceed. If set to "error", it will print
+	the previous warning and stop the rebase, 'git rebase
+	--edit-todo' can then be used to correct the error. If set to
+	"ignore", no checking is done.
+	To drop a commit without warning or error, use the `drop`
+	command in the todo-list.
+	Defaults to "ignore".
+
+rebase.instructionFormat::
+	A format string, as specified in linkgit:git-log[1], to be used for the
+	instruction list during an interactive rebase.  The format will
+	automatically have the long commit hash prepended to the format.
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 2/9] Documentation: use preferred name for the 'todo list' script

[Liam Beguin]

Use "todo list" instead of "instruction list" or "todo-list" to
reduce further confusion regarding the name of this script.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 Documentation/rebase-config.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index dba088d7c68f..30ae08cb5a4b 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -23,10 +23,10 @@ rebase.missingCommitsCheck::
 	--edit-todo' can then be used to correct the error. If set to
 	"ignore", no checking is done.
 	To drop a commit without warning or error, use the `drop`
-	command in the todo-list.
+	command in the todo list.
 	Defaults to "ignore".
 
 rebase.instructionFormat::
 	A format string, as specified in linkgit:git-log[1], to be used for the
-	instruction list during an interactive rebase.  The format will
+	todo list during an interactive rebase.  The format will
 	automatically have the long commit hash prepended to the format.
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 3/9] rebase -i: set commit to null in exec commands

[Liam Beguin]

Make sure commit is set to NULL when parsing exec instructions
from the todo list. If not, we may try to access an uninitialized
address later while updating the todo list.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index fa94ed652d2c..5033b049d995 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1268,6 +1268,7 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
 	bol += padding;
 
 	if (item->command == TODO_EXEC) {
+		item->commit = NULL;
 		item->arg = bol;
 		item->arg_len = (int)(eol - bol);
 		return 0;
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 4/9] rebase -i: refactor transform_todo_ids

[Liam Beguin]

The transform_todo_ids function is a little hard to read. Lets try
to make it easier by using more of the strbuf API. Also, since we'll
soon be adding command abbreviations, let's rename the function so
it's name reflects that change.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 builtin/rebase--helper.c |  4 +--
 sequencer.c              | 69 ++++++++++++++++++++----------------------------
 sequencer.h              |  2 +-
 3 files changed, 31 insertions(+), 44 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f8519363a393..7c06a27de821 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -55,9 +55,9 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	if (command == MAKE_SCRIPT && argc > 1)
 		return !!sequencer_make_script(keep_empty, stdout, argc, argv);
 	if (command == SHORTEN_SHA1S && argc == 1)
-		return !!transform_todo_ids(1);
+		return !!transform_todo_insn(1);
 	if (command == EXPAND_SHA1S && argc == 1)
-		return !!transform_todo_ids(0);
+		return !!transform_todo_insn(0);
 	if (command == CHECK_TODO_LIST && argc == 1)
 		return !!check_todo_list();
 	if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index 5033b049d995..0ff3c90e44bf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2494,60 +2494,47 @@ int sequencer_make_script(int keep_empty, FILE *out,
 }
 
 
-int transform_todo_ids(int shorten_ids)
+int transform_todo_insn(int shorten_ids)
 {
 	const char *todo_file = rebase_path_todo();
 	struct todo_list todo_list = TODO_LIST_INIT;
-	int fd, res, i;
-	FILE *out;
+	struct strbuf buf = STRBUF_INIT;
+	struct todo_item *item;
+	int i;
 
-	strbuf_reset(&todo_list.buf);
-	fd = open(todo_file, O_RDONLY);
-	if (fd < 0)
-		return error_errno(_("could not open '%s'"), todo_file);
-	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
-		close(fd);
+	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
 		return error(_("could not read '%s'."), todo_file);
-	}
-	close(fd);
 
-	res = parse_insn_buffer(todo_list.buf.buf, &todo_list);
-	if (res) {
+	if (parse_insn_buffer(todo_list.buf.buf, &todo_list)) {
 		todo_list_release(&todo_list);
 		return error(_("unusable todo list: '%s'"), todo_file);
 	}
 
-	out = fopen(todo_file, "w");
-	if (!out) {
-		todo_list_release(&todo_list);
-		return error(_("unable to open '%s' for writing"), todo_file);
-	}
-	for (i = 0; i < todo_list.nr; i++) {
-		struct todo_item *item = todo_list.items + i;
-		int bol = item->offset_in_buf;
-		const char *p = todo_list.buf.buf + bol;
-		int eol = i + 1 < todo_list.nr ?
-			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 {
-			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);
+	for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
+		/* if the item is not a command write it and continue */
+		if (item->command >= TODO_COMMENT) {
+			strbuf_addf(&buf, "%.*s\n", item->arg_len, item->arg);
+			continue;
+		}
+
+		/* add command to the buffer */
+		strbuf_addstr(&buf, command_to_string(item->command));
+
+		/* add commit id */
+		if (item->commit) {
+			const char *oid = shorten_ids ?
+					  short_commit_name(item->commit) :
+					  oid_to_hex(&item->commit->object.oid);
+
+			strbuf_addf(&buf, " %s", oid);
 		}
+		/* add all the rest */
+		strbuf_addf(&buf, " %.*s\n", item->arg_len, item->arg);
 	}
-	fclose(out);
+
+	i = write_message(buf.buf, buf.len, todo_file, 0);
 	todo_list_release(&todo_list);
-	return 0;
+	return i;
 }
 
 enum check_level {
diff --git a/sequencer.h b/sequencer.h
index 6f3d3df82c0a..4e444e3bf1c4 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -48,7 +48,7 @@ int sequencer_remove_state(struct replay_opts *opts);
 int sequencer_make_script(int keep_empty, FILE *out,
 		int argc, const char **argv);
 
-int transform_todo_ids(int shorten_ids);
+int transform_todo_insn(int shorten_ids);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 5/9] rebase -i: replace reference to sha1 with oid

[Liam Beguin]

Since we are trying to abstract the hash function name elsewhere in the
code base, lets use OID instead of SHA-1 in the rebase--helper too.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 builtin/rebase--helper.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 7c06a27de821..af0f91164fd0 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -14,7 +14,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	int keep_empty = 0;
 	enum {
-		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
+		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
 	} command = 0;
 	struct option options[] = {
@@ -27,9 +27,9 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		OPT_CMDMODE(0, "make-script", &command,
 			N_("make rebase script"), MAKE_SCRIPT),
 		OPT_CMDMODE(0, "shorten-ids", &command,
-			N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
+			N_("shorten commit ids in the todo list"), SHORTEN_OIDS),
 		OPT_CMDMODE(0, "expand-ids", &command,
-			N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
+			N_("expand commit ids in the todo list"), EXPAND_OIDS),
 		OPT_CMDMODE(0, "check-todo-list", &command,
 			N_("check the todo list"), CHECK_TODO_LIST),
 		OPT_CMDMODE(0, "skip-unnecessary-picks", &command,
@@ -54,9 +54,9 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!sequencer_remove_state(&opts);
 	if (command == MAKE_SCRIPT && argc > 1)
 		return !!sequencer_make_script(keep_empty, stdout, argc, argv);
-	if (command == SHORTEN_SHA1S && argc == 1)
+	if (command == SHORTEN_OIDS && argc == 1)
 		return !!transform_todo_insn(1);
-	if (command == EXPAND_SHA1S && argc == 1)
+	if (command == EXPAND_OIDS && argc == 1)
 		return !!transform_todo_insn(0);
 	if (command == CHECK_TODO_LIST && argc == 1)
 		return !!check_todo_list();
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Liam Beguin]

Update functions used in the rebase--helper so that they take a generic
'flags' parameter instead of a growing list of options.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 builtin/rebase--helper.c | 13 +++++++------
 sequencer.c              |  9 +++++----
 sequencer.h              |  8 +++++---
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index af0f91164fd0..fe814bf7229e 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,7 +12,7 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts = REPLAY_OPTS_INIT;
-	int keep_empty = 0;
+	unsigned flags = 0, keep_empty = 0;
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
@@ -48,16 +48,17 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, NULL, options,
 			builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
 
+	flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+	flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTED_IDS : 0;
+
 	if (command == CONTINUE && argc == 1)
 		return !!sequencer_continue(&opts);
 	if (command == ABORT && argc == 1)
 		return !!sequencer_remove_state(&opts);
 	if (command == MAKE_SCRIPT && argc > 1)
-		return !!sequencer_make_script(keep_empty, stdout, argc, argv);
-	if (command == SHORTEN_OIDS && argc == 1)
-		return !!transform_todo_insn(1);
-	if (command == EXPAND_OIDS && argc == 1)
-		return !!transform_todo_insn(0);
+		return !!sequencer_make_script(stdout, argc, argv, flags);
+	if ((command == SHORTEN_OIDS || command == EXPAND_OIDS) && argc == 1)
+		return !!transform_todo_insn(flags);
 	if (command == CHECK_TODO_LIST && argc == 1)
 		return !!check_todo_list();
 	if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index 0ff3c90e44bf..7d712811e9d1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2444,14 +2444,15 @@ 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(FILE *out, int argc, const char **argv,
+			  unsigned flags)
 {
 	char *format = NULL;
 	struct pretty_print_context pp = {0};
 	struct strbuf buf = STRBUF_INIT;
 	struct rev_info revs;
 	struct commit *commit;
+	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 
 	init_revisions(&revs, NULL);
 	revs.verbose_header = 1;
@@ -2494,7 +2495,7 @@ int sequencer_make_script(int keep_empty, FILE *out,
 }
 
 
-int transform_todo_insn(int shorten_ids)
+int transform_todo_insn(unsigned flags)
 {
 	const char *todo_file = rebase_path_todo();
 	struct todo_list todo_list = TODO_LIST_INIT;
@@ -2522,7 +2523,7 @@ int transform_todo_insn(int shorten_ids)
 
 		/* add commit id */
 		if (item->commit) {
-			const char *oid = shorten_ids ?
+			const char *oid = flags & TODO_LIST_SHORTED_IDS ?
 					  short_commit_name(item->commit) :
 					  oid_to_hex(&item->commit->object.oid);
 
diff --git a/sequencer.h b/sequencer.h
index 4e444e3bf1c4..3bb6b0658192 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -45,10 +45,12 @@ int sequencer_continue(struct replay_opts *opts);
 int sequencer_rollback(struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
-int sequencer_make_script(int keep_empty, FILE *out,
-		int argc, const char **argv);
+#define TODO_LIST_KEEP_EMPTY (1U << 0)
+#define TODO_LIST_SHORTED_IDS (1U << 1)
+int sequencer_make_script(FILE *out, int argc, const char **argv,
+			  unsigned flags);
 
-int transform_todo_insn(int shorten_ids);
+int transform_todo_insn(unsigned flags);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 7/9] rebase -i -x: add exec commands via the rebase--helper

[Liam Beguin]

Recent work on `git-rebase--interactive` aims to convert shell code to
C. Even if this is most likely not a big performance enhancement, let's
convert it too since a coming change to abbreviate command names
requires it to be updated.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 builtin/rebase--helper.c   |  7 ++++++-
 git-rebase--interactive.sh | 23 +----------------------
 sequencer.c                | 39 +++++++++++++++++++++++++++++++++++++++
 sequencer.h                |  1 +
 4 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index fe814bf7229e..03337e1484a2 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -15,7 +15,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	unsigned flags = 0, keep_empty = 0;
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
-		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
+		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
+		ADD_EXEC
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -36,6 +37,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 			N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
 		OPT_CMDMODE(0, "rearrange-squash", &command,
 			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
+		OPT_CMDMODE(0, "add-exec-commands", &command,
+			N_("insert exec commands in todo list"), ADD_EXEC),
 		OPT_END()
 	};
 
@@ -65,5 +68,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!skip_unnecessary_picks();
 	if (command == REARRANGE_SQUASH && argc == 1)
 		return !!rearrange_squash();
+	if (command == ADD_EXEC && argc == 2)
+		return !!sequencer_add_exec_commands(argv[1]);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 437815669f00..e3f5a0abf3c7 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -722,27 +722,6 @@ collapse_todo_ids() {
 	git rebase--helper --shorten-ids
 }
 
-# Add commands after a pick or after a squash/fixup series
-# in the todo list.
-add_exec_commands () {
-	{
-		first=t
-		while read -r insn rest
-		do
-			case $insn in
-			pick)
-				test -n "$first" ||
-				printf "%s" "$cmd"
-				;;
-			esac
-			printf "%s %s\n" "$insn" "$rest"
-			first=
-		done
-		printf "%s" "$cmd"
-	} <"$1" >"$1.new" &&
-	mv "$1.new" "$1"
-}
-
 # Switch to the branch in $into and notify it in the reflog
 checkout_onto () {
 	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
@@ -982,7 +961,7 @@ fi
 
 test -s "$todo" || echo noop >> "$todo"
 test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
-test -n "$cmd" && add_exec_commands "$todo"
+test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd"
 
 todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
 todocount=${todocount##* }
diff --git a/sequencer.c b/sequencer.c
index 7d712811e9d1..bd047737082d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2494,6 +2494,45 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 	return 0;
 }
 
+/*
+ * Add commands after pick and (series of) squash/fixup commands
+ * in the todo list.
+ */
+int sequencer_add_exec_commands(const char *commands)
+{
+	const char *todo_file = rebase_path_todo();
+	struct todo_list todo_list = TODO_LIST_INIT;
+	struct todo_item *item;
+	struct strbuf *buf = &todo_list.buf;
+	size_t offset = 0, commands_len = strlen(commands);
+	int i, first;
+
+	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
+		return error(_("could not read '%s'."), todo_file);
+
+	if (parse_insn_buffer(todo_list.buf.buf, &todo_list)) {
+		todo_list_release(&todo_list);
+		return error(_("unusable todo list: '%s'"), todo_file);
+	}
+
+	first = 1;
+	/* insert <commands> before every pick except the first one */
+	for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
+		if (item->command == TODO_PICK && !first) {
+			strbuf_insert(buf, item->offset_in_buf + offset,
+				      commands, commands_len);
+			offset += commands_len;
+		}
+		first = 0;
+	}
+
+	/* append final <commands> */
+	strbuf_add(buf, commands, commands_len);
+
+	i = write_message(buf->buf, buf->len, todo_file, 0);
+	todo_list_release(&todo_list);
+	return i;
+}
 
 int transform_todo_insn(unsigned flags)
 {
diff --git a/sequencer.h b/sequencer.h
index 3bb6b0658192..e4a9d2419883 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -50,6 +50,7 @@ int sequencer_remove_state(struct replay_opts *opts);
 int sequencer_make_script(FILE *out, int argc, const char **argv,
 			  unsigned flags);
 
+int sequencer_add_exec_commands(const char *command);
 int transform_todo_insn(unsigned flags);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 8/9] rebase -i: learn to abbreviate command names

[Liam Beguin]

`git rebase -i` already know how to interpret single-letter command
names. Teach it to generate the todo list with these same abbreviated
names.

Based-on-patch-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 Documentation/rebase-config.txt | 20 ++++++++++++++++++++
 builtin/rebase--helper.c        |  3 +++
 sequencer.c                     | 16 ++++++++++++++--
 sequencer.h                     |  1 +
 4 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index 30ae08cb5a4b..42e1ba757564 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -30,3 +30,23 @@ 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
+	...
+-------------------------------------------
++
+instead of:
++
+-------------------------------------------
+	pick deadbee The oneline of the commit
+	pick fa1afe1 The oneline of the next commit
+	...
+-------------------------------------------
++
+Defaults to false.
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 03337e1484a2..2c51ddcfd3dd 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,6 +13,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	unsigned flags = 0, keep_empty = 0;
+	int abbreviate_commands = 0;
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
@@ -43,6 +44,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_default_config, NULL);
+	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
 
 	opts.action = REPLAY_INTERACTIVE_REBASE;
 	opts.allow_ff = 1;
@@ -52,6 +54,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 			builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
 
 	flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
 	flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTED_IDS : 0;
 
 	if (command == CONTINUE && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index bd047737082d..b752dcc52982 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 comment_line_char;
+}
+
 static int is_noop(const enum todo_command command)
 {
 	return TODO_NOOP <= command;
@@ -2453,6 +2460,7 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 	struct rev_info revs;
 	struct commit *commit;
 	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
+	const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
 
 	init_revisions(&revs, NULL);
 	revs.verbose_header = 1;
@@ -2485,7 +2493,8 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 		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 ", insn,
+			    oid_to_hex(&commit->object.oid));
 		pretty_print_commit(&pp, commit, &buf);
 		strbuf_addch(&buf, '\n');
 		fputs(buf.buf, out);
@@ -2558,7 +2567,10 @@ int transform_todo_insn(unsigned flags)
 		}
 
 		/* add command to the buffer */
-		strbuf_addstr(&buf, command_to_string(item->command));
+		if (flags & TODO_LIST_ABBREVIATE_CMDS)
+			strbuf_addch(&buf, command_to_char(item->command));
+		else
+			strbuf_addstr(&buf, command_to_string(item->command));
 
 		/* add commit id */
 		if (item->commit) {
diff --git a/sequencer.h b/sequencer.h
index e4a9d2419883..468ee79fb72d 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -47,6 +47,7 @@ int sequencer_remove_state(struct replay_opts *opts);
 
 #define TODO_LIST_KEEP_EMPTY (1U << 0)
 #define TODO_LIST_SHORTED_IDS (1U << 1)
+#define TODO_LIST_ABBREVIATE_CMDS (1U << 2)
 int sequencer_make_script(FILE *out, int argc, const char **argv,
 			  unsigned flags);
 
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v2 9/9] t3404: add test case for abbreviated commands

[Liam Beguin]

Make sure the todo list ends up using single-letter command
abbreviations when the rebase.abbreviateCommands is enabled.
This configuration option should not change anything else.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 t/t3404-rebase-interactive.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 6a82d1ed876d..481a3500900d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1260,6 +1260,28 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
+	rebase_setup_and_clean abbrevcmd &&
+	test_commit "first" file1.txt "first line" first &&
+	test_commit "second" file1.txt "another line" second &&
+	test_commit "fixup! first" file2.txt "first line again" first_fixup &&
+	test_commit "squash! second" file1.txt "another line here" second_squash &&
+	cat >expected <<-EOF &&
+	p $(git rev-list --abbrev-commit -1 first) first
+	f $(git rev-list --abbrev-commit -1 first_fixup) fixup! first
+	x git show HEAD
+	p $(git rev-list --abbrev-commit -1 second) second
+	s $(git rev-list --abbrev-commit -1 second_squash) squash! second
+	x git show HEAD
+	EOF
+	git checkout abbrevcmd &&
+	set_cat_todo_editor &&
+	test_config rebase.abbreviateCommands true &&
+	test_must_fail git rebase -i --exec "git show HEAD" \
+		--autosquash master >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'static check of bad command' '
 	rebase_setup_and_clean bad-cmd &&
 	set_fake_editor &&
-- 
2.15.1.280.g10402c1f5b5c

Re: [PATCH v2 4/9] rebase -i: refactor transform_todo_ids

[Johannes Schindelin]

Hi Liam,

On Sun, 3 Dec 2017, Liam Beguin wrote:

> The transform_todo_ids function is a little hard to read. Lets try
> to make it easier by using more of the strbuf API. Also, since we'll
> soon be adding command abbreviations, let's rename the function so
> it's name reflects that change.

I am not really a fan of the new name, and would prefer the old one, but
that's only a nit, not a reason to reject the patch.

The rest of it makes the code reads a lot nicer than before. Thank you,
Johannes

Re: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Johannes Schindelin]

Hi Liam,

On Sun, 3 Dec 2017, Liam Beguin wrote:

> diff --git a/sequencer.h b/sequencer.h
> index 4e444e3bf1c4..3bb6b0658192 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -45,10 +45,12 @@ int sequencer_continue(struct replay_opts *opts);
>  int sequencer_rollback(struct replay_opts *opts);
>  int sequencer_remove_state(struct replay_opts *opts);
>  
> -int sequencer_make_script(int keep_empty, FILE *out,
> -		int argc, const char **argv);
> +#define TODO_LIST_KEEP_EMPTY (1U << 0)
> +#define TODO_LIST_SHORTED_IDS (1U << 1)

Maybe SHORTEN_IDs? And either revert back to transform_todo_ids() or use
SHORTEN_INSNS...

Maybe also TRANSFORM_TODO_LIST_* and maybe move the #define's above the
transform_todo_ids() function, i.e. one line further down?

> +int sequencer_make_script(FILE *out, int argc, const char **argv,
> +			  unsigned flags);
>  
> -int transform_todo_insn(int shorten_ids);
> +int transform_todo_insn(unsigned flags);
>  int check_todo_list(void);
>  int skip_unnecessary_picks(void);
>  int rearrange_squash(void);

Ciao,
Johannes

Re: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Johannes Schindelin]

Hi Liam,

On Mon, 4 Dec 2017, Johannes Schindelin wrote:

> On Sun, 3 Dec 2017, Liam Beguin wrote:
> 
> > diff --git a/sequencer.h b/sequencer.h
> > index 4e444e3bf1c4..3bb6b0658192 100644
> > --- a/sequencer.h
> > +++ b/sequencer.h
> > @@ -45,10 +45,12 @@ int sequencer_continue(struct replay_opts *opts);
> >  int sequencer_rollback(struct replay_opts *opts);
> >  int sequencer_remove_state(struct replay_opts *opts);
> >  
> > -int sequencer_make_script(int keep_empty, FILE *out,
> > -		int argc, const char **argv);
> > +#define TODO_LIST_KEEP_EMPTY (1U << 0)
> > +#define TODO_LIST_SHORTED_IDS (1U << 1)
> 
> Maybe SHORTEN_IDs? And either revert back to transform_todo_ids() or use
> SHORTEN_INSNS...
> 
> Maybe also TRANSFORM_TODO_LIST_* and maybe move the #define's above the
> transform_todo_ids() function, i.e. one line further down?
> 
> > +int sequencer_make_script(FILE *out, int argc, const char **argv,
> > +			  unsigned flags);

Ah, I just realized that make_script() already takes `flags`. Sorry for
the noise,
Johannes

Re: [PATCH v2 0/9] rebase -i: add config to abbreviate command names

[Johannes Schindelin]

Hi Liam,

On Sun, 3 Dec 2017, Liam Beguin wrote:

> This series will add the 'rebase.abbreviateCommands' configuration
> option to allow `git rebase -i` to default to the single-letter command
> names when generating the todo list.
> 
> Using single-letter command names can present two benefits. First, it
> makes it easier to change the action since you only need to replace a
> single character (i.e.: in vim "r<character>" instead of
> "ciw<character>").  Second, using this with a large enough value of
> 'core.abbrev' enables the lines of the todo list to remain aligned
> making the files easier to read.
> 
> Changes in V2:
> - Refactor and rename 'transform_todo_ids'
> - Replace SHA-1 by OID in rebase--helper.c
> - Update todo list related functions to take a generic 'flags' parameter
> - Rename 'add_exec_commands' function to 'sequencer_add_exec_commands'
> - Rename 'add-exec' option to 'add-exec-commands'
> - Use 'strbur_read_file' instead of rewriting it
> - Make 'command_to_char' return 'comment_char_line' if no single-letter
>   command name is defined
> - Combine both tests into a single test case
> - Update commit messages

Looks very nice already! I offered a couple of comments/suggestions, but
nothing major.

Thank you,
Johannes

Re: [PATCH v2 4/9] rebase -i: refactor transform_todo_ids

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sun, 3 Dec 2017, Liam Beguin wrote:
>
>> The transform_todo_ids function is a little hard to read. Lets try
>> to make it easier by using more of the strbuf API. Also, since we'll
>> soon be adding command abbreviations, let's rename the function so
>> it's name reflects that change.
>
> I am not really a fan of the new name, and would prefer the old one, but
> that's only a nit, not a reason to reject the patch.

FWIW, I do think the new name goes backwards.  The command uses to
remember what operations are to be carried out in which order using
a thing, and the name of that thing "todo list".  We also called it
the "instruction sheet", and "insn" was a good term to call one item
on that sheet among other items.

But recent commits in the area are pushing us to call it "todo list"
consistently.  An element in that list should be called "todo".

A "todo" consists of two parts, "what operation is done" part and
"using what commit object" part.  The original implementation of
this function affected only the latter part, and in that light, the
original name transform_todo_ids() is understandable.  Now you are
planning to make it modify both parts, not just "ids", so it is
understandable that you would want to rename it.  But I do not think
"insn" matches the recent trend.  It even risks misunderstanding
(i.e. xfrm_todo_ids() is about modifying "using what commit" part,
so perhaps xfrm_todo_insns() is about modifying "what operation is
done" part---but that is different from what you want to do, which
is to update _both_ halves).

Calling it just transform_todo() would probably be more in line with
the reason why you wanted to rename it in the first place.

Re: [PATCH v2 4/9] rebase -i: refactor transform_todo_ids

[liam Beguin]

Hi Junio,

On 04/12/17 11:09 AM, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>> On Sun, 3 Dec 2017, Liam Beguin wrote:
>>
>>> The transform_todo_ids function is a little hard to read. Lets try
>>> to make it easier by using more of the strbuf API. Also, since we'll
>>> soon be adding command abbreviations, let's rename the function so
>>> it's name reflects that change.
>>
>> I am not really a fan of the new name, and would prefer the old one, but
>> that's only a nit, not a reason to reject the patch.
> 
> FWIW, I do think the new name goes backwards.  The command uses to
> remember what operations are to be carried out in which order using
> a thing, and the name of that thing "todo list".  We also called it
> the "instruction sheet", and "insn" was a good term to call one item
> on that sheet among other items.
> 

Good point on saying this name change is going backwards.

> But recent commits in the area are pushing us to call it "todo list"
> consistently.  An element in that list should be called "todo".
> 
> A "todo" consists of two parts, "what operation is done" part and
> "using what commit object" part.  The original implementation of
> this function affected only the latter part, and in that light, the
> original name transform_todo_ids() is understandable.  Now you are
> planning to make it modify both parts, not just "ids", so it is
> understandable that you would want to rename it.  But I do not think
> "insn" matches the recent trend.  It even risks misunderstanding
> (i.e. xfrm_todo_ids() is about modifying "using what commit" part,
> so perhaps xfrm_todo_insns() is about modifying "what operation is
> done" part---but that is different from what you want to do, which
> is to update _both_ halves).
> 

You're right! We do want the name to reflect that we intend to change
both halves and not only the command.

> Calling it just transform_todo() would probably be more in line with
> the reason why you wanted to rename it in the first place.
> 

Good suggestion. Would transform_todos() work too? I'll send an update
tomorrow.
Thanks, 

Liam

Re: [PATCH v2 4/9] rebase -i: refactor transform_todo_ids

[liam Beguin]

Hi Johannes,

On 04/12/17 09:42 AM, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Sun, 3 Dec 2017, Liam Beguin wrote:
> 
>> The transform_todo_ids function is a little hard to read. Lets try
>> to make it easier by using more of the strbuf API. Also, since we'll
>> soon be adding command abbreviations, let's rename the function so
>> it's name reflects that change.
> 
> I am not really a fan of the new name, and would prefer the old one, but
> that's only a nit, not a reason to reject the patch.
> 

You're right, it's probably not the best name. I'll change it to
transform_todos() as we want the function name to reflect that it changes
both parts of the todo.

> The rest of it makes the code reads a lot nicer than before. Thank you,
> Johannes
> 

Thanks,
Liam

Re: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[liam Beguin]

Hi Johannes,

On 04/12/17 10:46 AM, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Sun, 3 Dec 2017, Liam Beguin wrote:
> 
>> diff --git a/sequencer.h b/sequencer.h
>> index 4e444e3bf1c4..3bb6b0658192 100644
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -45,10 +45,12 @@ int sequencer_continue(struct replay_opts *opts);
>>  int sequencer_rollback(struct replay_opts *opts);
>>  int sequencer_remove_state(struct replay_opts *opts);
>>  
>> -int sequencer_make_script(int keep_empty, FILE *out,
>> -		int argc, const char **argv);
>> +#define TODO_LIST_KEEP_EMPTY (1U << 0)
>> +#define TODO_LIST_SHORTED_IDS (1U << 1)
> 
> Maybe SHORTEN_IDs? And either revert back to transform_todo_ids() or use
> SHORTEN_INSNS...
> 

I'll change it to TODO_LIST_SHORTED_IDs. TODO_LIST_SHORTED_INSNS would
suggest the flag changes both parts of the todo.

> Maybe also TRANSFORM_TODO_LIST_* and maybe move the #define's above the
> transform_todo_ids() function, i.e. one line further down?
> 
>> +int sequencer_make_script(FILE *out, int argc, const char **argv,
>> +			  unsigned flags);
>>  
>> -int transform_todo_insn(int shorten_ids);
>> +int transform_todo_insn(unsigned flags);
>>  int check_todo_list(void);
>>  int skip_unnecessary_picks(void);
>>  int rearrange_squash(void);
> 
> Ciao,
> Johannes
> 

Thanks, 
Liam

Re: [PATCH v2 4/9] rebase -i: refactor transform_todo_ids

[Junio C Hamano]

liam Beguin <liambeguin@gmail.com> writes:

> Good suggestion. Would transform_todos() work too?

If the function is about munging multiple of them, then todo"s"
would work well; I wasn't focusing on singular vs plural, as I
thought the choice between them needs much less thought to make
correctly.

Re: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Junio C Hamano]

liam Beguin <liambeguin@gmail.com> writes:

> I'll change it to TODO_LIST_SHORTED_IDs. TODO_LIST_SHORTED_INSNS would
> suggest the flag changes both parts of the todo.

I am not a native speaker, but SHORTED does not sound like a right
phrase.  When you make something shorter, that thing is "shortened",
not "shorted".

RE: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Kerry, Richard]

"Shorted" is what happens when you put a piece of wire across the terminals of a battery ... (bang, smoke, etc).
It's short for "short-circuited".
Yes, I think you mean "shortened" in this case.

Regards,
Richard.



Richard Kerry
BNCS Engineer, SI SOL Telco & Media Vertical Practice

T: +44 (0)20 3618 2669
M: +44 (0)7812 325518
Lync: +44 (0) 20 3618 0778
Room G300, Stadium House, Wood Lane, London, W12 7TA
richard.kerry@atos.net




> -----Original Message-----
> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On
> Behalf Of Junio C Hamano
> Sent: Tuesday, December 05, 2017 12:37 PM
> To: liam Beguin <liambeguin@gmail.com>
> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>;
> git@vger.kernel.org; peff@peff.net
> Subject: Re: [PATCH v2 6/9] rebase -i: update functions to use a flags
> parameter
>
> liam Beguin <liambeguin@gmail.com> writes:
>
> > I'll change it to TODO_LIST_SHORTED_IDs. TODO_LIST_SHORTED_INSNS
> would
> > suggest the flag changes both parts of the todo.
>
> I am not a native speaker, but SHORTED does not sound like a right phrase.
> When you make something shorter, that thing is "shortened", not "shorted".

Atos, Atos Consulting, Worldline and Canopy The Open Cloud Company are trading names used by the Atos group. The following trading entities are registered in England and Wales: Atos IT Services UK Limited (registered number 01245534), Atos Consulting Limited (registered number 04312380), Atos Worldline UK Limited (registered number 08514184) and Canopy The Open Cloud Company Limited (registration number 08011902). The registered office for each is at 4 Triton Square, Regent’s Place, London, NW1 3HG.The VAT No. for each is: GB232327983.

This e-mail and the documents attached are confidential and intended solely for the addressee, and may contain confidential or privileged information. If you receive this e-mail in error, you are not authorised to copy, disclose, use or retain it. Please notify the sender immediately and delete this email from your systems. As emails may be intercepted, amended or lost, they are not secure. Atos therefore can accept no liability for any errors or their content. Although Atos endeavours to maintain a virus-free network, we do not warrant that this transmission is virus-free and can accept no liability for any damages resulting from any virus transmitted. The risks are deemed to be accepted by everyone who communicates with Atos by email.

Re: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[liam Beguin]

Hi,

On 05/12/17 07:41 AM, Kerry, Richard wrote:
> 
> "Shorted" is what happens when you put a piece of wire across the terminals of a battery ... (bang, smoke, etc).
> It's short for "short-circuited".
> Yes, I think you mean "shortened" in this case.
> 

Thanks for the explanation.
Sorry, my eyes stopped at the lowercase 's' in Johannes message.
Will fix.

> Regards,
> Richard.
> 
> 
> 
> Richard Kerry
> BNCS Engineer, SI SOL Telco & Media Vertical Practice
> 
> T: +44 (0)20 3618 2669
> M: +44 (0)7812 325518
> Lync: +44 (0) 20 3618 0778
> Room G300, Stadium House, Wood Lane, London, W12 7TA
> richard.kerry@atos.net
> 
> 

[...]

Thanks,
Liam

Re: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Junio C Hamano]

"Kerry, Richard" <richard.kerry@atos.net> writes:

> "Shorted" is what happens when you put a piece of wire across the terminals of a battery ... (bang, smoke, etc).
> It's short for "short-circuited".

Or it is what you do to something that you sell and that you yet do
not own, expecting that you can later buy it cheaper, allowing you
to pocket the difference ;-).

RE: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Kerry, Richard]

Indeed so.
In which case it is short for "selling short", or possibly "short selling".

Of course a little searching shows that "shorted" could mean some other things, including possibly the meaning originally suggested.
Nevertheless it seems to me that "shortened" is the most appropriate word in modern English.

R.


Richard Kerry
BNCS Engineer, SI SOL Telco & Media Vertical Practice

T: +44 (0)20 3618 2669
M: +44 (0)7812 325518
Lync: +44 (0) 20 3618 0778
Room G300, Stadium House, Wood Lane, London, W12 7TA
richard.kerry@atos.net




> -----Original Message-----
> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Tuesday, December 05, 2017 4:06 PM
> To: Kerry, Richard <richard.kerry@atos.net>
> Cc: git@vger.kernel.org; Johannes Schindelin
> <Johannes.Schindelin@gmx.de>; peff@peff.net; liam Beguin
> <liambeguin@gmail.com>
> Subject: Re: [PATCH v2 6/9] rebase -i: update functions to use a flags
> parameter
>
> "Kerry, Richard" <richard.kerry@atos.net> writes:
>
> > "Shorted" is what happens when you put a piece of wire across the
> terminals of a battery ... (bang, smoke, etc).
> > It's short for "short-circuited".
>
> Or it is what you do to something that you sell and that you yet do not own,
> expecting that you can later buy it cheaper, allowing you to pocket the
> difference ;-).
>

Atos, Atos Consulting, Worldline and Canopy The Open Cloud Company are trading names used by the Atos group. The following trading entities are registered in England and Wales: Atos IT Services UK Limited (registered number 01245534), Atos Consulting Limited (registered number 04312380), Atos Worldline UK Limited (registered number 08514184) and Canopy The Open Cloud Company Limited (registration number 08011902). The registered office for each is at 4 Triton Square, Regent’s Place, London, NW1 3HG.The VAT No. for each is: GB232327983.

This e-mail and the documents attached are confidential and intended solely for the addressee, and may contain confidential or privileged information. If you receive this e-mail in error, you are not authorised to copy, disclose, use or retain it. Please notify the sender immediately and delete this email from your systems. As emails may be intercepted, amended or lost, they are not secure. Atos therefore can accept no liability for any errors or their content. Although Atos endeavours to maintain a virus-free network, we do not warrant that this transmission is virus-free and can accept no liability for any damages resulting from any virus transmitted. The risks are deemed to be accepted by everyone who communicates with Atos by email.

[PATCH v2 0/9] rebase -i: add config to abbreviate command names

[Liam Beguin]

Hi everyone,

This series will add the 'rebase.abbreviateCommands' configuration
option to allow `git rebase -i` to default to the single-letter command
names when generating the todo list.

Using single-letter command names can present two benefits. First, it
makes it easier to change the action since you only need to replace a
single character (i.e.: in vim "r<character>" instead of
"ciw<character>").  Second, using this with a large enough value of
'core.abbrev' enables the lines of the todo list to remain aligned
making the files easier to read.

Changes in V2:
- Refactor and rename 'transform_todo_ids'
- Replace SHA-1 by OID in rebase--helper.c
- Update todo list related functions to take a generic 'flags' parameter
- Rename 'add_exec_commands' function to 'sequencer_add_exec_commands'
- Rename 'add-exec' option to 'add-exec-commands'
- Use 'strbur_read_file' instead of rewriting it
- Make 'command_to_char' return 'comment_char_line' if no single-letter
  command name is defined
- Combine both tests into a single test case
- Update commit messages

Changes in V2:
- Rename 'transform_todo_insn' to 'transform_todos'
- Fix flag name TODO_LIST_SHORTE{D,N}_IDS

Liam Beguin (9):
  Documentation: move rebase.* configs to new file
  Documentation: use preferred name for the 'todo list' script
  rebase -i: set commit to null in exec commands
  rebase -i: refactor transform_todo_ids
  rebase -i: replace reference to sha1 with oid
  rebase -i: update functions to use a flags parameter
  rebase -i -x: add exec commands via the rebase--helper
  rebase -i: learn to abbreviate command names
  t3404: add test case for abbreviated commands

 Documentation/config.txt        |  31 +-------
 Documentation/git-rebase.txt    |  19 +----
 Documentation/rebase-config.txt |  52 +++++++++++++
 builtin/rebase--helper.c        |  29 +++++---
 git-rebase--interactive.sh      |  23 +-----
 sequencer.c                     | 126 +++++++++++++++++++++-----------
 sequencer.h                     |  10 ++-
 t/t3404-rebase-interactive.sh   |  22 ++++++
 8 files changed, 186 insertions(+), 126 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

-- 
2.15.1.280.g10402c1f5b5c

[PATCH v3 1/9] Documentation: move rebase.* configs to new file

[Liam Beguin]

Move all rebase.* configuration variables to a separate file in order to
remove duplicates, and include it in config.txt and git-rebase.txt.  The
new descriptions are mostly taken from config.txt as they are more
verbose.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 Documentation/config.txt        | 31 +------------------------------
 Documentation/git-rebase.txt    | 19 +------------------
 Documentation/rebase-config.txt | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+), 48 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 531649cb40ea..e424b7de90b5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2691,36 +2691,7 @@ push.recurseSubmodules::
 	is retained. You may override this configuration at time of push by
 	specifying '--recurse-submodules=check|on-demand|no'.
 
-rebase.stat::
-	Whether to show a diffstat of what changed upstream since the last
-	rebase. False by default.
-
-rebase.autoSquash::
-	If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-	When set to true, automatically create a temporary stash entry
-	before the operation begins, and apply it after the operation
-	ends.  This means that you can run rebase on a dirty worktree.
-	However, use with care: the final stash application after a
-	successful rebase might result in non-trivial conflicts.
-	Defaults to false.
-
-rebase.missingCommitsCheck::
-	If set to "warn", git rebase -i will print a warning if some
-	commits are removed (e.g. a line was deleted), however the
-	rebase will still proceed. If set to "error", it will print
-	the previous warning and stop the rebase, 'git rebase
-	--edit-todo' can then be used to correct the error. If set to
-	"ignore", no checking is done.
-	To drop a commit without warning or error, use the `drop`
-	command in the todo-list.
-	Defaults to "ignore".
-
-rebase.instructionFormat::
-	A format string, as specified in linkgit:git-log[1], to be used for
-	the instruction list during an interactive rebase.  The format will automatically
-	have the long commit hash prepended to the format.
+include::rebase-config.txt[]
 
 receive.advertiseAtomic::
 	By default, git-receive-pack will advertise the atomic push
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3cedfb0fd22b..8a861c1e0d69 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -203,24 +203,7 @@ Alternatively, you can undo the 'git rebase' with
 CONFIGURATION
 -------------
 
-rebase.stat::
-	Whether to show a diffstat of what changed upstream since the last
-	rebase. False by default.
-
-rebase.autoSquash::
-	If set to true enable `--autosquash` option by default.
-
-rebase.autoStash::
-	If set to true enable `--autostash` option by default.
-
-rebase.missingCommitsCheck::
-	If set to "warn", print warnings about removed commits in
-	interactive mode. If set to "error", print the warnings and
-	stop the rebase. If set to "ignore", no checking is
-	done. "ignore" by default.
-
-rebase.instructionFormat::
-	Custom commit list format to use during an `--interactive` rebase.
+include::rebase-config.txt[]
 
 OPTIONS
 -------
diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
new file mode 100644
index 000000000000..dba088d7c68f
--- /dev/null
+++ b/Documentation/rebase-config.txt
@@ -0,0 +1,32 @@
+rebase.stat::
+	Whether to show a diffstat of what changed upstream since the last
+	rebase. False by default.
+
+rebase.autoSquash::
+	If set to true enable `--autosquash` option by default.
+
+rebase.autoStash::
+	When set to true, automatically create a temporary stash entry
+	before the operation begins, and apply it after the operation
+	ends.  This means that you can run rebase on a dirty worktree.
+	However, use with care: the final stash application after a
+	successful rebase might result in non-trivial conflicts.
+	This option can be overridden by the `--no-autostash` and
+	`--autostash` options of linkgit:git-rebase[1].
+	Defaults to false.
+
+rebase.missingCommitsCheck::
+	If set to "warn", git rebase -i will print a warning if some
+	commits are removed (e.g. a line was deleted), however the
+	rebase will still proceed. If set to "error", it will print
+	the previous warning and stop the rebase, 'git rebase
+	--edit-todo' can then be used to correct the error. If set to
+	"ignore", no checking is done.
+	To drop a commit without warning or error, use the `drop`
+	command in the todo-list.
+	Defaults to "ignore".
+
+rebase.instructionFormat::
+	A format string, as specified in linkgit:git-log[1], to be used for the
+	instruction list during an interactive rebase.  The format will
+	automatically have the long commit hash prepended to the format.
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v3 2/9] Documentation: use preferred name for the 'todo list' script

[Liam Beguin]

Use "todo list" instead of "instruction list" or "todo-list" to
reduce further confusion regarding the name of this script.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 Documentation/rebase-config.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index dba088d7c68f..30ae08cb5a4b 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -23,10 +23,10 @@ rebase.missingCommitsCheck::
 	--edit-todo' can then be used to correct the error. If set to
 	"ignore", no checking is done.
 	To drop a commit without warning or error, use the `drop`
-	command in the todo-list.
+	command in the todo list.
 	Defaults to "ignore".
 
 rebase.instructionFormat::
 	A format string, as specified in linkgit:git-log[1], to be used for the
-	instruction list during an interactive rebase.  The format will
+	todo list during an interactive rebase.  The format will
 	automatically have the long commit hash prepended to the format.
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v3 3/9] rebase -i: set commit to null in exec commands

[Liam Beguin]

Make sure commit is set to NULL when parsing exec instructions
from the todo list. If not, we may try to access an uninitialized
address later while updating the todo list.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index fa94ed652d2c..5033b049d995 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1268,6 +1268,7 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
 	bol += padding;
 
 	if (item->command == TODO_EXEC) {
+		item->commit = NULL;
 		item->arg = bol;
 		item->arg_len = (int)(eol - bol);
 		return 0;
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v3 4/9] rebase -i: refactor transform_todo_ids

[Liam Beguin]

The transform_todo_ids function is a little hard to read. Lets try
to make it easier by using more of the strbuf API. Also, since we'll
soon be adding command abbreviations, let's rename the function so
it's name reflects that change.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 builtin/rebase--helper.c |  4 +--
 sequencer.c              | 69 ++++++++++++++++------------------------
 sequencer.h              |  2 +-
 3 files changed, 31 insertions(+), 44 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f8519363a393..8ad4779d1650 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -55,9 +55,9 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	if (command == MAKE_SCRIPT && argc > 1)
 		return !!sequencer_make_script(keep_empty, stdout, argc, argv);
 	if (command == SHORTEN_SHA1S && argc == 1)
-		return !!transform_todo_ids(1);
+		return !!transform_todos(1);
 	if (command == EXPAND_SHA1S && argc == 1)
-		return !!transform_todo_ids(0);
+		return !!transform_todos(0);
 	if (command == CHECK_TODO_LIST && argc == 1)
 		return !!check_todo_list();
 	if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index 5033b049d995..c9a661a8c4bd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2494,60 +2494,47 @@ int sequencer_make_script(int keep_empty, FILE *out,
 }
 
 
-int transform_todo_ids(int shorten_ids)
+int transform_todos(int shorten_ids)
 {
 	const char *todo_file = rebase_path_todo();
 	struct todo_list todo_list = TODO_LIST_INIT;
-	int fd, res, i;
-	FILE *out;
+	struct strbuf buf = STRBUF_INIT;
+	struct todo_item *item;
+	int i;
 
-	strbuf_reset(&todo_list.buf);
-	fd = open(todo_file, O_RDONLY);
-	if (fd < 0)
-		return error_errno(_("could not open '%s'"), todo_file);
-	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
-		close(fd);
+	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
 		return error(_("could not read '%s'."), todo_file);
-	}
-	close(fd);
 
-	res = parse_insn_buffer(todo_list.buf.buf, &todo_list);
-	if (res) {
+	if (parse_insn_buffer(todo_list.buf.buf, &todo_list)) {
 		todo_list_release(&todo_list);
 		return error(_("unusable todo list: '%s'"), todo_file);
 	}
 
-	out = fopen(todo_file, "w");
-	if (!out) {
-		todo_list_release(&todo_list);
-		return error(_("unable to open '%s' for writing"), todo_file);
-	}
-	for (i = 0; i < todo_list.nr; i++) {
-		struct todo_item *item = todo_list.items + i;
-		int bol = item->offset_in_buf;
-		const char *p = todo_list.buf.buf + bol;
-		int eol = i + 1 < todo_list.nr ?
-			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 {
-			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);
+	for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
+		/* if the item is not a command write it and continue */
+		if (item->command >= TODO_COMMENT) {
+			strbuf_addf(&buf, "%.*s\n", item->arg_len, item->arg);
+			continue;
+		}
+
+		/* add command to the buffer */
+		strbuf_addstr(&buf, command_to_string(item->command));
+
+		/* add commit id */
+		if (item->commit) {
+			const char *oid = shorten_ids ?
+					  short_commit_name(item->commit) :
+					  oid_to_hex(&item->commit->object.oid);
+
+			strbuf_addf(&buf, " %s", oid);
 		}
+		/* add all the rest */
+		strbuf_addf(&buf, " %.*s\n", item->arg_len, item->arg);
 	}
-	fclose(out);
+
+	i = write_message(buf.buf, buf.len, todo_file, 0);
 	todo_list_release(&todo_list);
-	return 0;
+	return i;
 }
 
 enum check_level {
diff --git a/sequencer.h b/sequencer.h
index 6f3d3df82c0a..4f7f2c93f83e 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -48,7 +48,7 @@ int sequencer_remove_state(struct replay_opts *opts);
 int sequencer_make_script(int keep_empty, FILE *out,
 		int argc, const char **argv);
 
-int transform_todo_ids(int shorten_ids);
+int transform_todos(int shorten_ids);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v3 5/9] rebase -i: replace reference to sha1 with oid

[Liam Beguin]

Since we are trying to abstract the hash function name elsewhere in the
code base, lets use OID instead of SHA-1 in the rebase--helper too.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 builtin/rebase--helper.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 8ad4779d1650..c3b8e4d401f8 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -14,7 +14,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	int keep_empty = 0;
 	enum {
-		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
+		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
 	} command = 0;
 	struct option options[] = {
@@ -27,9 +27,9 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		OPT_CMDMODE(0, "make-script", &command,
 			N_("make rebase script"), MAKE_SCRIPT),
 		OPT_CMDMODE(0, "shorten-ids", &command,
-			N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
+			N_("shorten commit ids in the todo list"), SHORTEN_OIDS),
 		OPT_CMDMODE(0, "expand-ids", &command,
-			N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
+			N_("expand commit ids in the todo list"), EXPAND_OIDS),
 		OPT_CMDMODE(0, "check-todo-list", &command,
 			N_("check the todo list"), CHECK_TODO_LIST),
 		OPT_CMDMODE(0, "skip-unnecessary-picks", &command,
@@ -54,9 +54,9 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!sequencer_remove_state(&opts);
 	if (command == MAKE_SCRIPT && argc > 1)
 		return !!sequencer_make_script(keep_empty, stdout, argc, argv);
-	if (command == SHORTEN_SHA1S && argc == 1)
+	if (command == SHORTEN_OIDS && argc == 1)
 		return !!transform_todos(1);
-	if (command == EXPAND_SHA1S && argc == 1)
+	if (command == EXPAND_OIDS && argc == 1)
 		return !!transform_todos(0);
 	if (command == CHECK_TODO_LIST && argc == 1)
 		return !!check_todo_list();
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v3 6/9] rebase -i: update functions to use a flags parameter

[Liam Beguin]

Update functions used in the rebase--helper so that they take a generic
'flags' parameter instead of a growing list of options.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 builtin/rebase--helper.c | 13 +++++++------
 sequencer.c              |  9 +++++----
 sequencer.h              |  8 +++++---
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index c3b8e4d401f8..1102ecb43b67 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,7 +12,7 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts = REPLAY_OPTS_INIT;
-	int keep_empty = 0;
+	unsigned flags = 0, keep_empty = 0;
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
@@ -48,16 +48,17 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, NULL, options,
 			builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
 
+	flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+	flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
+
 	if (command == CONTINUE && argc == 1)
 		return !!sequencer_continue(&opts);
 	if (command == ABORT && argc == 1)
 		return !!sequencer_remove_state(&opts);
 	if (command == MAKE_SCRIPT && argc > 1)
-		return !!sequencer_make_script(keep_empty, stdout, argc, argv);
-	if (command == SHORTEN_OIDS && argc == 1)
-		return !!transform_todos(1);
-	if (command == EXPAND_OIDS && argc == 1)
-		return !!transform_todos(0);
+		return !!sequencer_make_script(stdout, argc, argv, flags);
+	if ((command == SHORTEN_OIDS || command == EXPAND_OIDS) && argc == 1)
+		return !!transform_todos(flags);
 	if (command == CHECK_TODO_LIST && argc == 1)
 		return !!check_todo_list();
 	if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index c9a661a8c4bd..8b0dd610c881 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2444,14 +2444,15 @@ 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(FILE *out, int argc, const char **argv,
+			  unsigned flags)
 {
 	char *format = NULL;
 	struct pretty_print_context pp = {0};
 	struct strbuf buf = STRBUF_INIT;
 	struct rev_info revs;
 	struct commit *commit;
+	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 
 	init_revisions(&revs, NULL);
 	revs.verbose_header = 1;
@@ -2494,7 +2495,7 @@ int sequencer_make_script(int keep_empty, FILE *out,
 }
 
 
-int transform_todos(int shorten_ids)
+int transform_todos(unsigned flags)
 {
 	const char *todo_file = rebase_path_todo();
 	struct todo_list todo_list = TODO_LIST_INIT;
@@ -2522,7 +2523,7 @@ int transform_todos(int shorten_ids)
 
 		/* add commit id */
 		if (item->commit) {
-			const char *oid = shorten_ids ?
+			const char *oid = flags & TODO_LIST_SHORTEN_IDS ?
 					  short_commit_name(item->commit) :
 					  oid_to_hex(&item->commit->object.oid);
 
diff --git a/sequencer.h b/sequencer.h
index 4f7f2c93f83e..68284e9762c8 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -45,10 +45,12 @@ int sequencer_continue(struct replay_opts *opts);
 int sequencer_rollback(struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
-int sequencer_make_script(int keep_empty, FILE *out,
-		int argc, const char **argv);
+#define TODO_LIST_KEEP_EMPTY (1U << 0)
+#define TODO_LIST_SHORTEN_IDS (1U << 1)
+int sequencer_make_script(FILE *out, int argc, const char **argv,
+			  unsigned flags);
 
-int transform_todos(int shorten_ids);
+int transform_todos(unsigned flags);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v3 7/9] rebase -i -x: add exec commands via the rebase--helper

[Liam Beguin]

Recent work on `git-rebase--interactive` aims to convert shell code to
C. Even if this is most likely not a big performance enhancement, let's
convert it too since a coming change to abbreviate command names
requires it to be updated.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 builtin/rebase--helper.c   |  7 ++++++-
 git-rebase--interactive.sh | 23 +---------------------
 sequencer.c                | 39 ++++++++++++++++++++++++++++++++++++++
 sequencer.h                |  1 +
 4 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 1102ecb43b67..4229ea0dc122 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -15,7 +15,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	unsigned flags = 0, keep_empty = 0;
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
-		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
+		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
+		ADD_EXEC
 	} command = 0;
 	struct option options[] = {
 		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -36,6 +37,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 			N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
 		OPT_CMDMODE(0, "rearrange-squash", &command,
 			N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
+		OPT_CMDMODE(0, "add-exec-commands", &command,
+			N_("insert exec commands in todo list"), ADD_EXEC),
 		OPT_END()
 	};
 
@@ -65,5 +68,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		return !!skip_unnecessary_picks();
 	if (command == REARRANGE_SQUASH && argc == 1)
 		return !!rearrange_squash();
+	if (command == ADD_EXEC && argc == 2)
+		return !!sequencer_add_exec_commands(argv[1]);
 	usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 437815669f00..e3f5a0abf3c7 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -722,27 +722,6 @@ collapse_todo_ids() {
 	git rebase--helper --shorten-ids
 }
 
-# Add commands after a pick or after a squash/fixup series
-# in the todo list.
-add_exec_commands () {
-	{
-		first=t
-		while read -r insn rest
-		do
-			case $insn in
-			pick)
-				test -n "$first" ||
-				printf "%s" "$cmd"
-				;;
-			esac
-			printf "%s %s\n" "$insn" "$rest"
-			first=
-		done
-		printf "%s" "$cmd"
-	} <"$1" >"$1.new" &&
-	mv "$1.new" "$1"
-}
-
 # Switch to the branch in $into and notify it in the reflog
 checkout_onto () {
 	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
@@ -982,7 +961,7 @@ fi
 
 test -s "$todo" || echo noop >> "$todo"
 test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
-test -n "$cmd" && add_exec_commands "$todo"
+test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd"
 
 todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
 todocount=${todocount##* }
diff --git a/sequencer.c b/sequencer.c
index 8b0dd610c881..892d242f6966 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2494,6 +2494,45 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 	return 0;
 }
 
+/*
+ * Add commands after pick and (series of) squash/fixup commands
+ * in the todo list.
+ */
+int sequencer_add_exec_commands(const char *commands)
+{
+	const char *todo_file = rebase_path_todo();
+	struct todo_list todo_list = TODO_LIST_INIT;
+	struct todo_item *item;
+	struct strbuf *buf = &todo_list.buf;
+	size_t offset = 0, commands_len = strlen(commands);
+	int i, first;
+
+	if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
+		return error(_("could not read '%s'."), todo_file);
+
+	if (parse_insn_buffer(todo_list.buf.buf, &todo_list)) {
+		todo_list_release(&todo_list);
+		return error(_("unusable todo list: '%s'"), todo_file);
+	}
+
+	first = 1;
+	/* insert <commands> before every pick except the first one */
+	for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
+		if (item->command == TODO_PICK && !first) {
+			strbuf_insert(buf, item->offset_in_buf + offset,
+				      commands, commands_len);
+			offset += commands_len;
+		}
+		first = 0;
+	}
+
+	/* append final <commands> */
+	strbuf_add(buf, commands, commands_len);
+
+	i = write_message(buf->buf, buf->len, todo_file, 0);
+	todo_list_release(&todo_list);
+	return i;
+}
 
 int transform_todos(unsigned flags)
 {
diff --git a/sequencer.h b/sequencer.h
index 68284e9762c8..212426c44548 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -50,6 +50,7 @@ int sequencer_remove_state(struct replay_opts *opts);
 int sequencer_make_script(FILE *out, int argc, const char **argv,
 			  unsigned flags);
 
+int sequencer_add_exec_commands(const char *command);
 int transform_todos(unsigned flags);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v3 8/9] rebase -i: learn to abbreviate command names

[Liam Beguin]

`git rebase -i` already know how to interpret single-letter command
names. Teach it to generate the todo list with these same abbreviated
names.

Based-on-patch-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 Documentation/rebase-config.txt | 20 ++++++++++++++++++++
 builtin/rebase--helper.c        |  3 +++
 sequencer.c                     | 16 ++++++++++++++--
 sequencer.h                     |  1 +
 4 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index 30ae08cb5a4b..42e1ba757564 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -30,3 +30,23 @@ 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
+	...
+-------------------------------------------
++
+instead of:
++
+-------------------------------------------
+	pick deadbee The oneline of the commit
+	pick fa1afe1 The oneline of the next commit
+	...
+-------------------------------------------
++
+Defaults to false.
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 4229ea0dc122..7daee544b7b4 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,6 +13,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	unsigned flags = 0, keep_empty = 0;
+	int abbreviate_commands = 0;
 	enum {
 		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
 		CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
@@ -43,6 +44,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_default_config, NULL);
+	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
 
 	opts.action = REPLAY_INTERACTIVE_REBASE;
 	opts.allow_ff = 1;
@@ -52,6 +54,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 			builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
 
 	flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
 	flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
 
 	if (command == CONTINUE && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index 892d242f6966..115085d39ca8 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 comment_line_char;
+}
+
 static int is_noop(const enum todo_command command)
 {
 	return TODO_NOOP <= command;
@@ -2453,6 +2460,7 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 	struct rev_info revs;
 	struct commit *commit;
 	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
+	const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
 
 	init_revisions(&revs, NULL);
 	revs.verbose_header = 1;
@@ -2485,7 +2493,8 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 		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 ", insn,
+			    oid_to_hex(&commit->object.oid));
 		pretty_print_commit(&pp, commit, &buf);
 		strbuf_addch(&buf, '\n');
 		fputs(buf.buf, out);
@@ -2558,7 +2567,10 @@ int transform_todos(unsigned flags)
 		}
 
 		/* add command to the buffer */
-		strbuf_addstr(&buf, command_to_string(item->command));
+		if (flags & TODO_LIST_ABBREVIATE_CMDS)
+			strbuf_addch(&buf, command_to_char(item->command));
+		else
+			strbuf_addstr(&buf, command_to_string(item->command));
 
 		/* add commit id */
 		if (item->commit) {
diff --git a/sequencer.h b/sequencer.h
index 212426c44548..81f6d7d393fd 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -47,6 +47,7 @@ int sequencer_remove_state(struct replay_opts *opts);
 
 #define TODO_LIST_KEEP_EMPTY (1U << 0)
 #define TODO_LIST_SHORTEN_IDS (1U << 1)
+#define TODO_LIST_ABBREVIATE_CMDS (1U << 2)
 int sequencer_make_script(FILE *out, int argc, const char **argv,
 			  unsigned flags);
 
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v3 9/9] t3404: add test case for abbreviated commands

[Liam Beguin]

Make sure the todo list ends up using single-letter command
abbreviations when the rebase.abbreviateCommands is enabled.
This configuration option should not change anything else.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 t/t3404-rebase-interactive.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 6a82d1ed876d..481a3500900d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1260,6 +1260,28 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
+	rebase_setup_and_clean abbrevcmd &&
+	test_commit "first" file1.txt "first line" first &&
+	test_commit "second" file1.txt "another line" second &&
+	test_commit "fixup! first" file2.txt "first line again" first_fixup &&
+	test_commit "squash! second" file1.txt "another line here" second_squash &&
+	cat >expected <<-EOF &&
+	p $(git rev-list --abbrev-commit -1 first) first
+	f $(git rev-list --abbrev-commit -1 first_fixup) fixup! first
+	x git show HEAD
+	p $(git rev-list --abbrev-commit -1 second) second
+	s $(git rev-list --abbrev-commit -1 second_squash) squash! second
+	x git show HEAD
+	EOF
+	git checkout abbrevcmd &&
+	set_cat_todo_editor &&
+	test_config rebase.abbreviateCommands true &&
+	test_must_fail git rebase -i --exec "git show HEAD" \
+		--autosquash master >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'static check of bad command' '
 	rebase_setup_and_clean bad-cmd &&
 	set_fake_editor &&
-- 
2.15.1.280.g10402c1f5b5c

Re: [PATCH v2 0/9] rebase -i: add config to abbreviate command names

[Junio C Hamano]

Liam Beguin <liambeguin@gmail.com> writes:

> This series will add the 'rebase.abbreviateCommands' configuration
> option to allow `git rebase -i` to default to the single-letter command
> names when generating the todo list.
>
> Using single-letter command names can present two benefits. First, it
> makes it easier to change the action since you only need to replace a
> single character (i.e.: in vim "r<character>" instead of
> "ciw<character>").  Second, using this with a large enough value of
> 'core.abbrev' enables the lines of the todo list to remain aligned
> making the files easier to read.
>
> Changes in V2:
> - Refactor and rename 'transform_todo_ids'
> - Replace SHA-1 by OID in rebase--helper.c
> - Update todo list related functions to take a generic 'flags' parameter
> - Rename 'add_exec_commands' function to 'sequencer_add_exec_commands'
> - Rename 'add-exec' option to 'add-exec-commands'
> - Use 'strbur_read_file' instead of rewriting it
> - Make 'command_to_char' return 'comment_char_line' if no single-letter
>   command name is defined
> - Combine both tests into a single test case
> - Update commit messages
>
> Changes in V2:
> - Rename 'transform_todo_insn' to 'transform_todos'
> - Fix flag name TODO_LIST_SHORTE{D,N}_IDS

I've replaced this series and pushed out the result.

Thanks.

Re: [PATCH v2 0/9] rebase -i: add config to abbreviate command names

[liam Beguin]

On 05/12/17 05:21 PM, Junio C Hamano wrote:
> Liam Beguin <liambeguin@gmail.com> writes:
> 
>> This series will add the 'rebase.abbreviateCommands' configuration
>> option to allow `git rebase -i` to default to the single-letter command
>> names when generating the todo list.
>>
>> Using single-letter command names can present two benefits. First, it
>> makes it easier to change the action since you only need to replace a
>> single character (i.e.: in vim "r<character>" instead of
>> "ciw<character>").  Second, using this with a large enough value of
>> 'core.abbrev' enables the lines of the todo list to remain aligned
>> making the files easier to read.
>>
>> Changes in V2:
>> - Refactor and rename 'transform_todo_ids'
>> - Replace SHA-1 by OID in rebase--helper.c
>> - Update todo list related functions to take a generic 'flags' parameter
>> - Rename 'add_exec_commands' function to 'sequencer_add_exec_commands'
>> - Rename 'add-exec' option to 'add-exec-commands'
>> - Use 'strbur_read_file' instead of rewriting it
>> - Make 'command_to_char' return 'comment_char_line' if no single-letter
>>   command name is defined
>> - Combine both tests into a single test case
>> - Update commit messages
>>
>> Changes in V2:
>> - Rename 'transform_todo_insn' to 'transform_todos'
>> - Fix flag name TODO_LIST_SHORTE{D,N}_IDS
> 
> I've replaced this series and pushed out the result.

Great! Thanks again,

> 
> Thanks.
> 

Liam

Re: [PATCH v2 8/9] rebase -i: learn to abbreviate command names

[Duy Nguyen]

On Mon, Dec 4, 2017 at 5:17 AM, Liam Beguin <liambeguin@gmail.com> wrote:
> +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 comment_line_char;
> +}

    CC sequencer.o
sequencer.c:798:19: error: type qualifiers ignored on function return
type [-Werror=ignored-qualifiers]
 static const char command_to_char(const enum todo_command command)
                   ^

Maybe drop the first const.
-- 
Duy

Re: [PATCH v2 8/9] rebase -i: learn to abbreviate command names

[Liam Beguin]

Hi Duy,

On Mon, 25 Dec 2017 at 07:48 Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Mon, Dec 4, 2017 at 5:17 AM, Liam Beguin <liambeguin@gmail.com> wrote:
> > +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 comment_line_char;
> > +}
>
>     CC sequencer.o
> sequencer.c:798:19: error: type qualifiers ignored on function return
> type [-Werror=ignored-qualifiers]
>  static const char command_to_char(const enum todo_command command)
>                    ^
>
> Maybe drop the first const.

Sorry, that's another copy-edit error I made that slipped through...
I'm curious, how did you build to get this error to show?
I tried with the DEVELOPER 'flag' but nothing showed and -Wextra gave
way too much messages...
Did you just add -Wignored-qualifiers to CFLAGS?

> --
> Duy

Thanks,
Liam

Re: [PATCH v2 8/9] rebase -i: learn to abbreviate command names

[Duy Nguyen]

On Mon, Dec 25, 2017 at 10:39 PM, Liam Beguin <liambeguin@gmail.com> wrote:
> I'm curious, how did you build to get this error to show?
> I tried with the DEVELOPER 'flag' but nothing showed and -Wextra gave
> way too much messages...
> Did you just add -Wignored-qualifiers to CFLAGS?

I have a custom CFLAGS, created before DEVELOPER flag was added, which
is -Wextra -Werror plus about 5  -Wno-xxx to shut gcc up.
-- 
Duy

Re: [PATCH v2 8/9] rebase -i: learn to abbreviate command names

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Mon, Dec 4, 2017 at 5:17 AM, Liam Beguin <liambeguin@gmail.com> wrote:
>> +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 comment_line_char;
>> +}
>
>     CC sequencer.o
> sequencer.c:798:19: error: type qualifiers ignored on function return
> type [-Werror=ignored-qualifiers]
>  static const char command_to_char(const enum todo_command command)
>                    ^
>
> Maybe drop the first const.

Thanks.  This topic has been in 'next' for quite some time and I
wanted to merge it down to 'master' soonish, so I've added the
following before doing so.

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 27 Dec 2017 11:12:45 -0800
Subject: [PATCH] sequencer.c: drop 'const' from function return type

With -Werror=ignored-qualifiers, a function that claims to return
"const char" gets this error:

    CC sequencer.o
sequencer.c:798:19: error: type qualifiers ignored on function return
type [-Werror=ignored-qualifiers]
 static const char command_to_char(const enum todo_command command)
                   ^

Reported-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 115085d39c..2a407cbe54 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -795,7 +795,7 @@ 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)
+static char command_to_char(const enum todo_command command)
 {
 	if (command < TODO_COMMENT && todo_command_info[command].c)
 		return todo_command_info[command].c;
-- 
2.15.1-597-g62d91a8972

Re: [PATCH v2 8/9] rebase -i: learn to abbreviate command names

[Liam Beguin]

Hi Junio,


On 27 December 2017 at 20:15, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Mon, Dec 4, 2017 at 5:17 AM, Liam Beguin <liambeguin@gmail.com> wrote:
>>> +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 comment_line_char;
>>> +}
>>
>>     CC sequencer.o
>> sequencer.c:798:19: error: type qualifiers ignored on function return
>> type [-Werror=ignored-qualifiers]
>>  static const char command_to_char(const enum todo_command command)
>>                    ^
>>
>> Maybe drop the first const.
>
> Thanks.  This topic has been in 'next' for quite some time and I
> wanted to merge it down to 'master' soonish, so I've added the
> following before doing so.

Thanks for taking the time. I had prepared the patch but was waiting to get
home to send it.
Only comment I have, maybe s/sequencer.c/rebase -i/ in the subject line
so it matches with the rest.

Since this came up, would it be a good thing to add -Wignored-qualifiers
to the DEVELOPER flags?

>
> -- >8 --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Wed, 27 Dec 2017 11:12:45 -0800
> Subject: [PATCH] sequencer.c: drop 'const' from function return type
>
> With -Werror=ignored-qualifiers, a function that claims to return
> "const char" gets this error:
>
>     CC sequencer.o
> sequencer.c:798:19: error: type qualifiers ignored on function return
> type [-Werror=ignored-qualifiers]
>  static const char command_to_char(const enum todo_command command)
>                    ^
>
> Reported-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 115085d39c..2a407cbe54 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -795,7 +795,7 @@ 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)
> +static char command_to_char(const enum todo_command command)
>  {
>         if (command < TODO_COMMENT && todo_command_info[command].c)
>                 return todo_command_info[command].c;
> --
> 2.15.1-597-g62d91a8972
>

Thanks,
Liam

Re: [PATCH v2 8/9] rebase -i: learn to abbreviate command names

[Junio C Hamano]

Liam Beguin <liambeguin@gmail.com> writes:

> Since this came up, would it be a good thing to add -Wignored-qualifiers
> to the DEVELOPER flags?

Quite frankly, I am not sure if catching that particular warning
violation buys us much. As a return value from a function is never
an lvalue, what triggers the warning may certainly be an indication
of a sloppy coding, but otherwise I do not see it as diagnosing a
potential error.  "The programmer thought that the returned value
will only be assigned to a const variable and will never be
modified, but the language does not guarantee such a behaviour out
of the caller"---does such an incorrect expectation lead to an error
in the codepath that involves such a function?