[PATCH v4 05/10] rebase -i: also expand/collapse the SHA-1s via the rebase--helper

[Liam Beguin <liambeguin@gmail.com>]

Hi Johannes,

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> This is crucial to improve performance on Windows, as the speed is now
> mostly dominated by the SHA-1 transformation (because it spawns a new
> rev-parse process for *every* line, and spawning processes is pretty
> slow from Git for Windows' MSYS2 Bash).
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/rebase--helper.c   | 10 +++++++-
>  git-rebase--interactive.sh | 27 ++--------------------
>  sequencer.c                | 57 ++++++++++++++++++++++++++++++++++++++++++++++
>  sequencer.h                |  2 ++
>  4 files changed, 70 insertions(+), 26 deletions(-)
> 
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index 821058d452d..9444c8d6c60 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -13,7 +13,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
> +		CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S
>  	} command = 0;
>  	struct option options[] = {
>  		OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
> @@ -24,6 +24,10 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  				ABORT),
>  		OPT_CMDMODE(0, "make-script", &command,
>  			N_("make rebase script"), MAKE_SCRIPT),
> +		OPT_CMDMODE(0, "shorten-sha1s", &command,
> +			N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
> +		OPT_CMDMODE(0, "expand-sha1s", &command,
> +			N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),

Since work is being done to convert to `struct object_id` would it
not be best to use a more generic name instead of 'sha1'?
maybe something like {shorten,expand}-hashs

>  		OPT_END()
>  	};
>  
> @@ -42,5 +46,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)
> +		return !!transform_todo_ids(1);
> +	if (command == EXPAND_SHA1S && argc == 1)
> +		return !!transform_todo_ids(0);
>  	usage_with_options(builtin_rebase_helper_usage, options);
>  }
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 214af0372ba..82a1941c42c 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -750,35 +750,12 @@ skip_unnecessary_picks () {
>  		die "$(gettext "Could not skip unnecessary pick commands")"
>  }
>  
> -transform_todo_ids () {
> -	while read -r command rest
> -	do
> -		case "$command" in
> -		"$comment_char"* | exec)
> -			# Be careful for oddball commands like 'exec'
> -			# that do not have a SHA-1 at the beginning of $rest.
> -			;;
> -		*)
> -			sha1=$(git rev-parse --verify --quiet "$@" ${rest%%[	 ]*}) &&
> -			if test "a$rest" = "a${rest#*[	 ]}"
> -			then
> -				rest=$sha1
> -			else
> -				rest="$sha1 ${rest#*[	 ]}"
> -			fi
> -			;;
> -		esac
> -		printf '%s\n' "$command${rest:+ }$rest"
> -	done <"$todo" >"$todo.new" &&
> -	mv -f "$todo.new" "$todo"
> -}
> -
>  expand_todo_ids() {
> -	transform_todo_ids
> +	git rebase--helper --expand-sha1s
>  }
>  
>  collapse_todo_ids() {
> -	transform_todo_ids --short
> +	git rebase--helper --shorten-sha1s
>  }
>  
>  # Rearrange the todo list that has both "pick sha1 msg" and
> diff --git a/sequencer.c b/sequencer.c
> index 88819a1a2a9..201d45b1677 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2437,3 +2437,60 @@ int sequencer_make_script(int keep_empty, FILE *out,
>  	strbuf_release(&buf);
>  	return 0;
>  }
> +
> +
> +int transform_todo_ids(int shorten_sha1s)
> +{
> +	const char *todo_file = rebase_path_todo();
> +	struct todo_list todo_list = TODO_LIST_INIT;
> +	int fd, res, i;
> +	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 instruction sheet: '%s'"), todo_file);

As you pointed out last time, the name of the "todo script" can be a
source of confusion. The migration to C could be a good opportunity to
clarify this.
I don't know which is the preferred name but we could go with
"todo list" as it is the most common across the code base.

$ git grep  'todo[ -]list' | wc -l
20
$ git grep  'rebase[ -]script' | wc -l
0
$ git grep  'instruction[ -]list' | wc -l
1
$ git grep  'instruction[ -]sheet' | wc -l
20
$ git grep  'instruction[ -]sheet' | grep -v ^po | wc -l
8

> +	}
> +
> +	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 *sha1 = shorten_sha1s ?
> +				short_commit_name(item->commit) :
> +				oid_to_hex(&item->commit->object.oid);

We could also use 'hash' or 'ids' here instead of 'sha1'.

> +			int len;
> +
> +			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);
> +		}
> +	}
> +	fclose(out);
> +	todo_list_release(&todo_list);
> +	return 0;
> +}
> diff --git a/sequencer.h b/sequencer.h
> index 83f2943b7a9..47a81034e76 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -48,6 +48,8 @@ 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_sha1s);
> +
>  extern const char sign_off_header[];
>  
>  void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
> -- 
> 2.12.2.windows.2.800.gede8f145e06

Liam