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

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

[Liam Beguin]

Add the 'rebase.abbreviateCommands' configuration option to allow
`git rebase -i` to default to the single-letter command-names in
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 from v1 to v2:
 - Improve Documentation and commit message

Changes from v2 to v3:
 - Transform a single patch into a series
 - change option name from 'rebase.abbrevCmd' to 'rebase.abbreviateCommands'
 - abbreviate all commands (not just pick)
 - teach `git rebase -i --autosquash` to recognise single-letter command-names
 - move rebase configuration documentation to Documentation/rebase-config.txt
 - update Documentation to use the preferred naming for the todo list
 - update Documentation and commit messages according to feedback

Liam Beguin (6):
  rebase -i: add abbreviated command-names handling
  rebase -i: add abbreviate_commands function
  rebase -i: add short command-name in --autosquash
  Documentation: move rebase.* config variables to a separate
    rebase-config.txt
  Documentation: use prefered name for the 'todo list' script
  Documentation: document the rebase.abbreviateCommands option

 Documentation/config.txt        | 31 +-----------------------
 Documentation/git-rebase.txt    | 21 +++-------------
 Documentation/rebase-config.txt | 53 +++++++++++++++++++++++++++++++++++++++++
 git-rebase--interactive.sh      | 24 ++++++++++++++++++++----
 4 files changed, 78 insertions(+), 52 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

-- 
2.9.3

[PATCH v3 1/6] rebase -i: add abbreviated command-names handling

[Liam Beguin]

make sure 'add_exec_commands' and 'transform_todo_ids' also understand
the abbreviated versions of the command-names.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 git-rebase--interactive.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2c9c0165b5ab..9b8a030ff045 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -754,7 +754,7 @@ transform_todo_ids () {
 	while read -r command rest
 	do
 		case "$command" in
-		"$comment_char"* | exec)
+		"$comment_char"* |x|exec)
 			# Be careful for oddball commands like 'exec'
 			# that do not have a SHA-1 at the beginning of $rest.
 			;;
@@ -871,7 +871,7 @@ add_exec_commands () {
 		while read -r insn rest
 		do
 			case $insn in
-			pick)
+			p|pick)
 				test -n "$first" ||
 				printf "%s" "$cmd"
 				;;
-- 
2.9.3

[PATCH v3 2/6] rebase -i: add abbreviate_commands function

[Liam Beguin]

Once the rest of the processing is done, the `abbreviate_commands`
function is called. If the 'rebase.abbreviateCommands' option is set to
true, the function will replace each command-name by its abbreviated
form.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 git-rebase--interactive.sh | 16 ++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9b8a030ff045..4fa621062cdf 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -884,6 +884,20 @@ add_exec_commands () {
 	mv "$1.new" "$1"
 }
 
+abbreviate_commands () {
+	test "$(git config --bool rebase.abbreviateCommands)" = true || return
+
+	while read -r command rest
+	do
+		case $command in
+		x|exec) command=x ;;
+		*)      command=${command%${command#?}} ;;
+		esac
+		printf "%s\n" "$command $rest"
+	done <"$1" >"$1.new" &&
+	mv -f "$1.new" "$1"
+}
+
 # Check if the SHA-1 passed as an argument is a
 # correct one, if not then print $2 in "$todo".badsha
 # $1: the SHA-1 to test
@@ -1143,6 +1158,7 @@ edit-todo)
 	git stripspace --strip-comments <"$todo" >"$todo".new
 	mv -f "$todo".new "$todo"
 	collapse_todo_ids
+	abbreviate_commands "$todo"
 	append_todo_help
 	gettext "
 You are editing the todo file of an ongoing interactive rebase.
@@ -1281,6 +1297,7 @@ fi
 test -s "$todo" || echo noop >> "$todo"
 test -n "$autosquash" && rearrange_squash "$todo"
 test -n "$cmd" && add_exec_commands "$todo"
+abbreviate_commands "$todo"
 
 todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
 todocount=${todocount##* }
-- 
2.9.3

[PATCH v3 3/6] rebase -i: add short command-name in --autosquash

[Liam Beguin]

teach `git rebase -i` to recognise short command-names when using the
'--autosquash' option. This allows commit with titles beginning with
"s! ..." and "f! ..." to be treated the same way as "squash! ..." and
"fixup! ..." respectively.

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

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 53f4e144444a..3e49d8b046ca 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -437,6 +437,8 @@ without an explicit `--interactive`.
 	commit from `pick` to `squash` (or `fixup`).  Ignores subsequent
 	"fixup! " or "squash! " after the first, in case you referred to an
 	earlier fixup/squash with `git commit --fixup/--squash`.
+	Note that their short counterparts, namely "s! ..." and "f! ..."
+	behave the same way.
 +
 This option is only valid when the `--interactive` option is used.
 +
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 4fa621062cdf..61450064c5c4 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -790,7 +790,7 @@ rearrange_squash () {
 	do
 		test -z "${format}" || message=$(git log -n 1 --format="%s" ${sha1})
 		case "$message" in
-		"squash! "*|"fixup! "*)
+		"squash! "*|"s! "*|"fixup! "*|"f! "*)
 			action="${message%%!*}"
 			rest=$message
 			prefix=
@@ -798,7 +798,7 @@ rearrange_squash () {
 			while :
 			do
 				case "$rest" in
-				"squash! "*|"fixup! "*)
+				"squash! "*|"s! "*|"fixup! "*|"f! "*)
 					prefix="$prefix${rest%%!*},"
 					rest="${rest#*! }"
 					;;
-- 
2.9.3

[PATCH v3 4/6] Documentation: move rebase.* config variables to a separate rebase-config.txt

[Liam Beguin]

Move 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 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 | 30 ++++++++++++++++++++++++++++++
 3 files changed, 32 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/rebase-config.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d5155..6b647c504e8f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2583,36 +2583,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
-	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 3e49d8b046ca..702a46adfa18 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..718721000031
--- /dev/null
+++ b/Documentation/rebase-config.txt
@@ -0,0 +1,30 @@
+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
+	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.
-- 
2.9.3

[PATCH v3 5/6] 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 718721000031..a9b1d496e63a 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -21,10 +21,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 automatically
+	the todo list during an interactive rebase.  The format will automatically
 	have the long commit hash prepended to the format.
-- 
2.9.3

[PATCH v3 6/6] Documentation: document the rebase.abbreviateCommands option

[Liam Beguin]

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

diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
index a9b1d496e63a..0f29b7d0b89a 100644
--- a/Documentation/rebase-config.txt
+++ b/Documentation/rebase-config.txt
@@ -28,3 +28,26 @@ 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 -i` will abbreviate all command-names in the
+	todo list resulting in something like this:
+-------------------------------------------
+	p ae6c43a first commit title
+	f 0694310 fixup! first commit title
+	p bf25ea8 second commit title
+	s e8fbbfd squash! second commit title
+	...
+-------------------------------------------
+	instead of:
+-------------------------------------------
+	pick ae6c43a first commit title
+	fixup 0694310 fixup! first commit title
+	pick bf25ea8 second commit title
+	squash e8fbbfd squash! second commit title
+	...
+-------------------------------------------
+	As shown above, using single-letter command-names better aligns the
+	todo list when full names have different lengths. Additionally, combined
+	with a large enough value of 'core.abbrev' (say 12), the todo list is
+	guaranteed to be fully aligned.
-- 
2.9.3

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

[Ævar Arnfjörð Bjarmason]

On Tue, May 2, 2017 at 6:00 AM, Liam Beguin <liambeguin@gmail.com> wrote:
> Add the 'rebase.abbreviateCommands' configuration option to allow
> `git rebase -i` to default to the single-letter command-names in
> 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 from v1 to v2:
>  - Improve Documentation and commit message
>
> Changes from v2 to v3:
>  - Transform a single patch into a series
>  - change option name from 'rebase.abbrevCmd' to 'rebase.abbreviateCommands'
>  - abbreviate all commands (not just pick)
>  - teach `git rebase -i --autosquash` to recognise single-letter command-names
>  - move rebase configuration documentation to Documentation/rebase-config.txt
>  - update Documentation to use the preferred naming for the todo list
>  - update Documentation and commit messages according to feedback
>
> Liam Beguin (6):
>   rebase -i: add abbreviated command-names handling
>   rebase -i: add abbreviate_commands function
>   rebase -i: add short command-name in --autosquash
>   Documentation: move rebase.* config variables to a separate
>     rebase-config.txt
>   Documentation: use prefered name for the 'todo list' script
>   Documentation: document the rebase.abbreviateCommands option

I locally rebased this into just 3 patches, i.e. in this sequence:

- Documentation: move rebase.* config variables to a separate rebase-config.txt
- Documentation: use preferred name for the 'todo list' script
- *all the rest of this squashed*

I think that's much less confusing than having 3x "rebase -i" patches.
If you look at any one of those you have very little context for
what's going on, and there seems to be no point in splitting them
since the end result is tiny (3 files changed, 45 insertions(+), 4
deletions(-)).

I think with that this looks good, but it also needs tests, if you
apply your series and then comment out the new calls to
abbreviate_commands all tests still pass, if you look at git-config(1)
and search for the other rebase.* commands & grep the test suite for
those you can see how they're tested for.

I don't think this needs a lot of testing since it's a rather trivial
feature, but just one test to make sure that the todo list ends up as
"p ..." "e  ..." instead of "pick ..." "exec ..." etc. would be good.

>  Documentation/config.txt        | 31 +-----------------------
>  Documentation/git-rebase.txt    | 21 +++-------------
>  Documentation/rebase-config.txt | 53 +++++++++++++++++++++++++++++++++++++++++
>  git-rebase--interactive.sh      | 24 ++++++++++++++++++++----
>  4 files changed, 78 insertions(+), 52 deletions(-)
>  create mode 100644 Documentation/rebase-config.txt
>
> --
> 2.9.3
>

Re: [PATCH v3 1/6] rebase -i: add abbreviated command-names handling

[Johannes Schindelin]

Hi Liam,


On Tue, 2 May 2017, Liam Beguin wrote:

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 2c9c0165b5ab..9b8a030ff045 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -754,7 +754,7 @@ transform_todo_ids () {
>  	while read -r command rest
>  	do
>  		case "$command" in
> -		"$comment_char"* | exec)
> +		"$comment_char"* |x|exec)

While the existing line has spaces around the pipe symbol, I think you can
safely remove them in this patch, as the style of the remainder of the
script seems to be disagreeing with that style. But please remove also the
space between the `*` and the `|`.

The functional part of the patch is correct, as far as I can see.

Ciao,
Johannes

Re: [PATCH v3 2/6] rebase -i: add abbreviate_commands function

[Johannes Schindelin]

Hi Liam,

On Tue, 2 May 2017, Liam Beguin wrote:

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 9b8a030ff045..4fa621062cdf 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -884,6 +884,20 @@ add_exec_commands () {
>  	mv "$1.new" "$1"
>  }
>  
> +abbreviate_commands () {
> +	test "$(git config --bool rebase.abbreviateCommands)" = true || return
> +
> +	while read -r command rest
> +	do
> +		case $command in
> +		x|exec) command=x ;;
> +		*)      command=${command%${command#?}} ;;
> +		esac
> +		printf "%s\n" "$command $rest"
> +	done <"$1" >"$1.new" &&
> +	mv -f "$1.new" "$1"
> +}

Neither this function nor its callers handle errors, but that is in line
with how the other todo list-munging functions work.

I just say this to document that I read this code, noted this shortcoming,
and think that it is okay to keep it this way for the moment.

There is one more fundamental comment I have, though: we already have a
step that converts the todo list to/from the human readable format (by
collapsing/expanding the commit hashes presented in the list). We could
easily make this conversion a part of that conversion.

Once my `rebase-i-extra` patches get accepted, we will need to rewrite
this in C, as part of sequencer.c, anyway. It will look somewhat like
this:

-- snipsnap --
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index e6591f01112..5eda71307af 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -11,7 +11,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
@@ -39,6 +39,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,11 +53,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, 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 63a588f0916..ca504063d16 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2390,7 +2390,7 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
 	strbuf_release(&sob);
 }
 
-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)
 {
 	char *format = NULL;
@@ -2430,7 +2430,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);
@@ -2440,7 +2442,7 @@ int sequencer_make_script(int keep_empty, FILE *out,
 }
 
 
-int transform_todo_ids(int shorten_sha1s)
+int transform_todo_ids(int shorten_sha1s, int abbreviate_commands)
 {
 	const char *todo_file = rebase_path_todo();
 	struct todo_list todo_list = TODO_LIST_INIT;
@@ -2476,16 +2478,31 @@ int transform_todo_ids(int shorten_sha1s)
 			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);
+		if (item->command >= TODO_EXEC && item->command != TODO_DROP) {
+			if (!abbreviate_commands)
+				fwrite(p, eol - bol, 1, out);
+			else {
+				const char *end_of_line = p + eol;
+				p += strspn(p, " \t"); /* skip whitespace */
+				p += strcspn(p, " \t"); /* skip command */
+				fprintf(out, "%c%.*s",
+					todo_command_info[item->command].c,
+					(int)(end_of_line - p), p);
+			}
+		}
 		else {
 			const char *sha1 = shorten_sha1s ?
 				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 */
+			if (abbreviate_commands) {
+				p = &todo_command_info[item->command].c;
+				len = 1;
+			} else {
+				p += strspn(p, " \t"); /* left-trim command */
+				len = strcspn(p, " \t"); /* length of command */
+			}
 
 			fprintf(out, "%.*s %s %.*s\n",
 				len, p, sha1, item->arg_len, item->arg);
diff --git a/sequencer.h b/sequencer.h
index 1c94bec7622..9a7f4333900 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -45,10 +45,10 @@ 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 transform_todo_ids(int shorten_sha1s);
+int transform_todo_ids(int shorten_sha1s, int abbreviate_commands);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);

Re: [PATCH v3 3/6] rebase -i: add short command-name in --autosquash

[Johannes Schindelin]

Hi Liam,

On Tue, 2 May 2017, Liam Beguin wrote:

> teach `git rebase -i` to recognise short command-names when using the
> '--autosquash' option. This allows commit with titles beginning with
> "s! ..." and "f! ..." to be treated the same way as "squash! ..." and
> "fixup! ..." respectively.

As the recommended way to generate those commits is by using the
--fixup/--squash options of git-commit, and as there is *a much higher*
chance of false positives when using a very short tell-tale such as `f!`
(which could be an abbreviation for an expletive, likewise `s!`), I do not
think we will want this change.

Let's keep handling just fixup!/squash!

Ciao,
Johannes

Re: [PATCH v3 4/6] Documentation: move rebase.* config variables to a separate rebase-config.txt

[Johannes Schindelin]

Hi Liam,


On Tue, 2 May 2017, Liam Beguin wrote:

> Move 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 taken from config.txt as they are more verbose.

This is a nice cleanup in its own right. My only complaint: the
rebase.autoStash documentation no longer says that it is related to the
command-line option `--autosquash`.

While the original description in `git-rebase.txt` was suboptimal, maybe
we can add back the reference by adding something like:

	This option can be overridden by the `--no-autosquash` and
	`--autosquash` options of linkgit:git-rebase[1].

Ciao,
Johannes

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 444 bytes --]

Hi,


On Tue, 2 May 2017, Ævar Arnfjörð Bjarmason wrote:

> I locally rebased this into just 3 patches, i.e. in this sequence:
> 
> - Documentation: move rebase.* config variables to a separate rebase-config.txt
> - Documentation: use preferred name for the 'todo list' script
> - *all the rest of this squashed*

I think that makes a lot of sense. (I would drop the part about f!/s!, as
I pointed out, though)

Ciao,
Johannes

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

[Johannes Schindelin]

Hi Liam,

On Tue, 2 May 2017, Liam Beguin wrote:

> Add the 'rebase.abbreviateCommands' configuration option to allow `git
> rebase -i` to default to the single-letter command-names in 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 from v1 to v2:
>  - Improve Documentation and commit message
> 
> Changes from v2 to v3:
>  - Transform a single patch into a series
>  - change option name from 'rebase.abbrevCmd' to 'rebase.abbreviateCommands'
>  - abbreviate all commands (not just pick)
>  - teach `git rebase -i --autosquash` to recognise single-letter command-names
>  - move rebase configuration documentation to Documentation/rebase-config.txt
>  - update Documentation to use the preferred naming for the todo list
>  - update Documentation and commit messages according to feedback

Thank you for this pleasant read. It is an excellent contribution, and the
way you communicate what you changed and why is very welcome.

I offered a couple of comments, my biggest one probably being that this
patch series is crossing paths with my patch series that tries to move
more functionality out of the git-rebase--interactive.sh script into the
git-rebase--helper that is written in C, closely followed by my suggestion
to fold at least part of the functionality into the SHA-1
collapsing/expanding.

If your patch series "wins", I can easily forward-port your changes to the
rebase-i-extra branch, but it may actually make sense to build on top of
the rebase-i-extra branch to begin with. If you agree: I pushed the
proposed change to the `rebase-i-extra+abbrev` branch at
https://github.com/dscho/git.

I look forward to see this story unfold!

Ciao,
Johannes

Re: [PATCH v3 3/6] rebase -i: add short command-name in --autosquash

[Liam Beguin]

Hi Johannes,

On Tue, 2017-05-02 at 17:34 +0200, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Tue, 2 May 2017, Liam Beguin wrote:
> 
> > teach `git rebase -i` to recognise short command-names when using the
> > '--autosquash' option. This allows commit with titles beginning with
> > "s! ..." and "f! ..." to be treated the same way as "squash! ..." and
> > "fixup! ..." respectively.
> 
> As the recommended way to generate those commits is by using the
> --fixup/--squash options of git-commit, and as there is *a much higher*
> chance of false positives when using a very short tell-tale such as `f!`
> (which could be an abbreviation for an expletive, likewise `s!`), I do not
> think we will want this change.
> 
> Let's keep handling just fixup!/squash!
> 
> Ciao,
> Johannes

I was not quite sure about this change. My guess was that since --autosquash
needs the whole commit title to find a match, the short version had little
probability of generating a false positive. I thought it made sense to include
the change in this series, but I understand why it's probably not a good idea
to take it. I'll remove it in the next series.

Thanks, 
Liam

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

[Liam Beguin]

Hi Johannes, 

On Tue, 2017-05-02 at 17:48 +0200, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Tue, 2 May 2017, Liam Beguin wrote:
> 
> > Add the 'rebase.abbreviateCommands' configuration option to allow `git
> > rebase -i` to default to the single-letter command-names in 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 from v1 to v2:
> >  - Improve Documentation and commit message
> > 
> > Changes from v2 to v3:
> >  - Transform a single patch into a series
> >  - change option name from 'rebase.abbrevCmd' to 'rebase.abbreviateCommands'
> >  - abbreviate all commands (not just pick)
> >  - teach `git rebase -i --autosquash` to recognise single-letter command-names
> >  - move rebase configuration documentation to Documentation/rebase-config.txt
> >  - update Documentation to use the preferred naming for the todo list
> >  - update Documentation and commit messages according to feedback
> 
> Thank you for this pleasant read. It is an excellent contribution, and the
> way you communicate what you changed and why is very welcome.
> 

Thanks! and thank you for the support and help.

> I offered a couple of comments, my biggest one probably being that this
> patch series is crossing paths with my patch series that tries to move
> more functionality out of the git-rebase--interactive.sh script into the
> git-rebase--helper that is written in C, closely followed by my suggestion
> to fold at least part of the functionality into the SHA-1
> collapsing/expanding.
> 

I've seen a few messages about this migration, but I'm not yet sure I understand
the difference between the shell and the C implementations. Is the C version going
to replace 'git-rebase--interactive.sh'?

> If your patch series "wins", I can easily forward-port your changes to the
> rebase-i-extra branch, but it may actually make sense to build on top of
> the rebase-i-extra branch to begin with. If you agree: I pushed the
> proposed change to the `rebase-i-extra+abbrev` branch at
> https://github.com/dscho/git.
> 

If 'git-rebase--interactive.sh' is bound to be replaced, I could
just shrink this to the Documentation cleanup (patches 4 and 5)
and rework the rest on top of your new implementation.

> I look forward to see this story unfold!
> 
> Ciao,
> Johannes

Thanks, 
Liam

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

[Johannes Schindelin]

Hi Liam,

On Tue, 2 May 2017, Liam Beguin wrote:

> On Tue, 2017-05-02 at 17:48 +0200, Johannes Schindelin wrote:
> 
> > I offered a couple of comments, my biggest one probably being that
> > this patch series is crossing paths with my patch series that tries to
> > move more functionality out of the git-rebase--interactive.sh script
> > into the git-rebase--helper that is written in C, closely followed by
> > my suggestion to fold at least part of the functionality into the
> > SHA-1 collapsing/expanding.
> 
> I've seen a few messages about this migration, but I'm not yet sure I
> understand the difference between the shell and the C implementations.
> Is the C version going to replace 'git-rebase--interactive.sh'?

The C version has already started to replace the shell script, yes. In
adding and using git-rebase--helper, there are already parts of the
interactive rebase functionality that are run using C code only. The idea
is to move more and more functionality over (separating out the
--preserve-merges handling into a different shell script, as I have no
plans to convert that code to C, and as far as I can see nobody else wants
to step up to that task, either). Eventually, we may be able to finish
that gigantic task of having git-rebase be all builtin C code.

> > If your patch series "wins", I can easily forward-port your changes to
> > the rebase-i-extra branch, but it may actually make sense to build on
> > top of the rebase-i-extra branch to begin with. If you agree: I pushed
> > the proposed change to the `rebase-i-extra+abbrev` branch at
> > https://github.com/dscho/git.
> 
> If 'git-rebase--interactive.sh' is bound to be replaced, I could
> just shrink this to the Documentation cleanup (patches 4 and 5)
> and rework the rest on top of your new implementation.

I kind of hoped that Junio would chime in with his verdict. That would be
the ultimate deciding factor, I think.

Ciao,
Johannes

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

[Junio C Hamano]

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

>> If 'git-rebase--interactive.sh' is bound to be replaced, I could
>> just shrink this to the Documentation cleanup (patches 4 and 5)
>> and rework the rest on top of your new implementation.
>
> I kind of hoped that Junio would chime in with his verdict. That would be
> the ultimate deciding factor, I think.

What I can predict is that within two or three release cycles
(unless you completely lose interest) the todo-list generation will
be all in C and that I anticipate that the C version may even be
capable of generating different kind of todo command (e.g. to
support tools like your Garden Shears more natively), so the
mid-term direction definitely is that any enhancement would in the
end needs to happen on top of or in coordination with the C rewrite
we've been discussing recently.

I didn't know what the comfort levels of Liam working with scripted
vs C code, and the "vertict" depends on that, I would think.  We may
want to discuss the enhancement in the original scripted form Liam
did with new tests while the C rewrite is still cooking, and either
Liam, you or somebody else can make it work with your C rewrite when
both are ready.  If Liam feels comfortable working with you and the
code after the C rewrite that is still in-flight, it is also fine if
the next iteration from Liam were on top of your series.

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

[Liam Beguin]

Hi Junio,

On Wed, 2017-05-03 at 22:04 -0700, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > > If 'git-rebase--interactive.sh' is bound to be replaced, I could
> > > just shrink this to the Documentation cleanup (patches 4 and 5)
> > > and rework the rest on top of your new implementation.
> > 
> > I kind of hoped that Junio would chime in with his verdict. That would be
> > the ultimate deciding factor, I think.
> 
> What I can predict is that within two or three release cycles
> (unless you completely lose interest) the todo-list generation will
> be all in C and that I anticipate that the C version may even be
> capable of generating different kind of todo command (e.g. to
> support tools like your Garden Shears more natively), so the
> mid-term direction definitely is that any enhancement would in the
> end needs to happen on top of or in coordination with the C rewrite
> we've been discussing recently.
> 
> I didn't know what the comfort levels of Liam working with scripted
> vs C code, and the "vertict" depends on that, I would think.  We may
> want to discuss the enhancement in the original scripted form Liam
> did with new tests while the C rewrite is still cooking, and either
> Liam, you or somebody else can make it work with your C rewrite when
> both are ready.  If Liam feels comfortable working with you and the
> code after the C rewrite that is still in-flight, it is also fine if
> the next iteration from Liam were on top of your series.
> 
> 

Sorry for the delay, I don't mind switching to C but it would probably
be easier to see if the scripted version gets approved first.
If it does, I could then get started on the C implementation.
If you prefer I could also forget about the scripted version, make a C
implementation work and see if that gets approved.

Thanks,
Liam

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

[Junio C Hamano]

Liam Beguin <liambeguin@gmail.com> writes:

> Sorry for the delay, I don't mind switching to C but it would probably
> be easier to see if the scripted version gets approved first.
> If it does, I could then get started on the C implementation.
> If you prefer I could also forget about the scripted version, make a C
> implementation work and see if that gets approved.

I am not sure what "approved" would mean in the context of this
project, though ;-) Your patch to the scripted version would
certainly not be in the upcoming release.  If you define the
"approval" as "it is queued to my tree somewhere", the patch would
start its life like everybody else by getting merged to the 'pu'
branch, where there already is a topic that removes the code you
patch your enhancement into.

The list _can_ agree that it is a good idea to have an option to
populate the todo list with shortened insn words from the beginning
(instead of merely accepting a short-hand while executing), which is
what your patch wants to do, without actually having the updated
scripted "rebase -i" merged in any of the integration branches in my
tree.  If you meant by "approval" to have such a list concensus, I
think you may already have one.  I personally do not think it is a
great idea but I do not think it is a horrible one, either.  As long
as it is an opt-in feature that many people find useful (which may
be the case already, judging from the list traffic), I do not mind
;-)

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

[Liam Beguin]

Hi,

On Mon, 2017-05-08 at 09:27 +0900, Junio C Hamano wrote:
> Liam Beguin <liambeguin@gmail.com> writes:
> 
> > Sorry for the delay, I don't mind switching to C but it would probably
> > be easier to see if the scripted version gets approved first.
> > If it does, I could then get started on the C implementation.
> > If you prefer I could also forget about the scripted version, make a C
> > implementation work and see if that gets approved.
> 
> I am not sure what "approved" would mean in the context of this
> project, though ;-) Your patch to the scripted version would
> certainly not be in the upcoming release.  If you define the
> "approval" as "it is queued to my tree somewhere", the patch would
> start its life like everybody else by getting merged to the 'pu'
> branch, where there already is a topic that removes the code you
> patch your enhancement into.
> 

By "approved", I guess I meant the list reaches an agreement. 

> The list _can_ agree that it is a good idea to have an option to
> populate the todo list with shortened insn words from the beginning
> (instead of merely accepting a short-hand while executing), which is
> what your patch wants to do, without actually having the updated
> scripted "rebase -i" merged in any of the integration branches in my
> tree.  If you meant by "approval" to have such a list concensus, I
> think you may already have one.  I personally do not think it is a
> great idea but I do not think it is a horrible one, either.  As long
> as it is an opt-in feature that many people find useful (which may
> be the case already, judging from the list traffic), I do not mind
> ;-)
> 

Ok, based on this, I'll send a new series based on the 'pu' branch.

Thanks again, 
Liam