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

[Johannes Schindelin]

Hi Liam,

On Thu, 25 May 2017, Liam Beguin wrote:

> Johannes Schindelin  writes:
> 
> > 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
> > @@ -24,6 +24,10 @@ int cmd_rebase__helper(int argc, const char **argv, 
> > const char *prefix)
> > ABORT),
> > OPT_CMDMODE(0, "make-script", ,
> > N_("make rebase script"), MAKE_SCRIPT),
> > +   OPT_CMDMODE(0, "shorten-sha1s", ,
> > +   N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
> > +   OPT_CMDMODE(0, "expand-sha1s", ,
> > +   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

Good point. You suggest the use of "ids" later, and I think that is an
even better name: what we try to do here is to expand/reduce the commit
*identifiers*. The fact that they are hexadecimal representations of
hashes is an implementation detail.

> > 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();
> > 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(_list.buf);
> > +   fd = open(todo_file, O_RDONLY);
> > +   if (fd < 0)
> > +   return error_errno(_("could not open '%s'"), todo_file);
> > +   if (strbuf_read(_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, _list);
> > +   if (res) {
> > +   todo_list_release(_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.

True. This was simply a copy-edited part, and I should have caught that.

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

Yep, my next iteration will use that term.

> > +   }
> > +
> > +   out = fopen(todo_file, "w");
> > +   if (!out) {
> > +   todo_list_release(_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(>commit->object.oid);
> 
> We could also use 'hash' or 'ids' here instead of 'sha1'.

Absolutely!

Thank you,
Johannes

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

[Liam Beguin]

Hi Johannes,

Johannes Schindelin  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 
> ---
>  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", _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", ,
>   N_("make rebase script"), MAKE_SCRIPT),
> + OPT_CMDMODE(0, "shorten-sha1s", ,
> + N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
> + OPT_CMDMODE(0, "expand-sha1s", ,
> + 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();
>   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();
>   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(_list.buf);
> + fd = open(todo_file, O_RDONLY);
> + if (fd < 0)
> + return error_errno(_("could not open '%s'"), todo_file);
> + if (strbuf_read(_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, _list);
> + if (res) {
> + todo_list_release(_list);
> + return error(_("unusable instruction 

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

[Johannes Schindelin]

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 
---
 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", _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", ,
N_("make rebase script"), MAKE_SCRIPT),
+   OPT_CMDMODE(0, "shorten-sha1s", ,
+   N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
+   OPT_CMDMODE(0, "expand-sha1s", ,
+   N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
OPT_END()
};
 
@@ -42,5 +46,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!sequencer_remove_state();
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();
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(_list.buf);
+   fd = open(todo_file, O_RDONLY);
+   if (fd < 0)
+   return error_errno(_("could not open '%s'"), todo_file);
+   if (strbuf_read(_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, _list);
+   if (res) {
+   todo_list_release(_list);
+   return error(_("unusable instruction sheet: '%s'"), todo_file);
+   }
+
+   out = fopen(todo_file, "w");
+   if (!out) {
+   todo_list_release(_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