Re: revision API design, was Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Junio C Hamano]

Junio C Hamano  writes:

> The current API to hide such an embarrasing but necessary
> implementation details from the code that does not need to know
> them, i.e. the consumer of rev-info structure with various settings,
> is to invoke the command line parser.  Bypassing and going directly
> down to the internal implementation detail of rev_info _is_ being
> sloppy.  I would strongly prefer to see that the current series
> written for the API to allow us get it over with the "rebase -i"
> thing, and think revision setup API separately and later.

An updated API that hides the implementation details may look like
a vast enhancement of this patch.

I say "vast" because we need to do this for _all_ of the "else if"
cascade you see in revision.c, and probably for fields set by other
helper functions in the same file.  Otherwise, it doesn't have much
value.

Anybody who is tempted to say "We need to do these only for the
complex ones, like the one that needs to set revs->pretty_given
while setting something else" hasn't learned from the example of
66b2ed09.  Interactions between options start happening when new
options that are added, and that is when handling of a seemingly
unrelated old option that used to be very simple needs to do more in
the new world order.  And that is why this illustration has a
wrapper for "--first-parent-only", which happens to be very simple
today.

We do not want revision traversal API's customers to write

revs.first_parent_only = 1;

because that's mucking with the implementation detail.  The current
API to hide that detail is:

memset(, 0, sizeof(revs);
argv_pushl(, "--first-parent-only", NULL);
... may be more options ...
setup_revisions(args.argc, args.argv, , ...);

and

memset(, 0, sizeof(revs);
rev_option_first_parent_only();
... may be more options ...
setup_revisions(0, NULL, , ...);

would be a more statically-checked rewrite, if such an API was
available.

 revision-internal.h | 18 ++
 revision.c  |  9 +++--
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/revision-internal.h b/revision-internal.h
new file mode 100644
index 00..dea4c48412
--- /dev/null
+++ b/revision-internal.h
@@ -0,0 +1,18 @@
+#ifndef REVISION_INTERNAL_H
+#define REVISION_INTERNAL_H 1
+
+static inline void rev_option_first_parent_only(struct rev_info *revs)
+{
+   revs->first_parent_only = 1;
+}
+
+static inline void rev_option_simplify_merges(struct rev_info *revs)
+{
+   revs->simplify_merges = 1;
+   revs->topo_order = 1;
+   revs->rewrite_parents = 1;
+   revs->simplify_history = 0;
+   revs->limited = 1;
+}
+
+#endif
diff --git a/revision.c b/revision.c
index 265611e01f..9418676264 100644
--- a/revision.c
+++ b/revision.c
@@ -20,6 +20,7 @@
 #include "cache-tree.h"
 #include "bisect.h"
 #include "worktree.h"
+#include "revision-internal.h"
 
 volatile show_early_output_fn_t show_early_output;
 
@@ -1807,7 +1808,7 @@ static int handle_revision_opt(struct rev_info *revs, int 
argc, const char **arg
revs->min_age = approxidate(optarg);
return argcount;
} else if (!strcmp(arg, "--first-parent")) {
-   revs->first_parent_only = 1;
+   rev_option_first_parent_only();
} else if (!strcmp(arg, "--ancestry-path")) {
revs->ancestry_path = 1;
revs->simplify_history = 0;
@@ -1825,11 +1826,7 @@ static int handle_revision_opt(struct rev_info *revs, 
int argc, const char **arg
revs->sort_order = REV_SORT_IN_GRAPH_ORDER;
revs->topo_order = 1;
} else if (!strcmp(arg, "--simplify-merges")) {
-   revs->simplify_merges = 1;
-   revs->topo_order = 1;
-   revs->rewrite_parents = 1;
-   revs->simplify_history = 0;
-   revs->limited = 1;
+   rev_option_simplify_merges();
} else if (!strcmp(arg, "--simplify-by-decoration")) {
revs->simplify_merges = 1;
revs->topo_order = 1;

Re: revision API design, was Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Junio C Hamano]

Johannes Schindelin  writes:

>> See 66b2ed09 ("Fix "log" family not to be too agressive about
>> ...
>> ask the existing command line parser to set them for you.
>
> This is a very eloquent description of a problem with the API.

Yes, but ...

> The correct suggestion would be to fix the API, of course. Not to declare
> an interface intended for command-line parsing the internal API.
>
> Maybe the introduction of `pretty_given` was a strong hint at a design
> flaw to begin with, pointing to the fact that user_format is a singleton
> (yes, really, you can't have two different user_formats in the same Git
> process, what were we thinking)?

... this tells me that you do not understand the issue.  The
embarrasing but necessary pretty-given field was not about
user_format (let alone singleton-ness of it) at all.  It was about
the show_notes feature that wants to be on by default most of the
time, but needs to be defeated when the end user asked for certain
formats.

Quite frankly, I am not interested in listening to a proposal to
update API by a person who does not understand the issue in the API,
but that is a separate issue.  A more important thing is that the
update to "rebase -i" is important enough and we do not want to
delay it by introducing further dependency.  IOW, I do not want to
spend an extra development cycle or two to update the revision setup
API and have you rebase the series on top after the API update is
done.

The current API to hide such an embarrasing but necessary
implementation details from the code that does not need to know
them, i.e. the consumer of rev-info structure with various settings,
is to invoke the command line parser.  Bypassing and going directly
down to the internal implementation detail of rev_info _is_ being
sloppy.  I would strongly prefer to see that the current series
written for the API to allow us get it over with the "rebase -i"
thing, and think revision setup API separately and later.

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[liam Beguin]

Hi Johannes,

Johannes Schindelin  writes:
> The first step of an interactive rebase is to generate the so-called "todo
> script", to be stored in the state directory as "git-rebase-todo" and to
> be edited by the user.
>
> Originally, we adjusted the output of `git log ` using a simple
> sed script. Over the course of the years, the code became more
> complicated. We now use shell scripting to edit the output of `git log`
> conditionally, depending whether to keep "empty" commits (i.e. commits
> that do not change any files).
>
> On platforms where shell scripting is not native, this can be a serious
> drag. And it opens the door for incompatibilities between platforms when
> it comes to shell scripting or to Unix-y commands.
>
> Let's just re-implement the todo script generation in plain C, using the
> revision machinery directly.
>
> This is substantially faster, improving the speed relative to the
> shell script version of the interactive rebase from 2x to 3x on Windows.
>
> Note that the rearrange_squash() function in git-rebase--interactive
> relied on the fact that we set the "format" variable to the config setting
> rebase.instructionFormat. Relying on a side effect like this is no good,
> hence we explicitly perform that assignment (possibly again) in
> rearrange_squash().
>
> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/rebase--helper.c   |  8 +++-
>  git-rebase--interactive.sh | 44 +
>  sequencer.c| 49 
> ++
>  sequencer.h|  3 +++
>  4 files changed, 82 insertions(+), 22 deletions(-)
>
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index ca1ebb2fa18..821058d452d 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -11,15 +11,19 @@ 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;
>   enum {
> - CONTINUE = 1, ABORT
> + CONTINUE = 1, ABORT, MAKE_SCRIPT
>   } command = 0;
>   struct option options[] = {
>   OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
> + OPT_BOOL(0, "keep-empty", _empty, N_("keep empty 
> commits")),
>   OPT_CMDMODE(0, "continue", , N_("continue rebase"),
>   CONTINUE),
>   OPT_CMDMODE(0, "abort", , N_("abort rebase"),
>   ABORT),
> + OPT_CMDMODE(0, "make-script", ,
> + N_("make rebase script"), MAKE_SCRIPT),
>   OPT_END()
>   };

This is probably being picky, but we could also use a different name
here instead of 'rebase script'. This would help keep the name of this
script consistent as you already pointed out.
maybe something like 'make-todo-list' or just 'make-list'?

>  
> @@ -36,5 +40,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   return !!sequencer_continue();
>   if (command == ABORT && argc == 1)
>   return !!sequencer_remove_state();
> + if (command == MAKE_SCRIPT && argc > 1)
> + return !!sequencer_make_script(keep_empty, stdout, argc, argv);

same here.

>   usage_with_options(builtin_rebase_helper_usage, options);
>  }
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 2c9c0165b5a..609e150d38f 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -785,6 +785,7 @@ collapse_todo_ids() {
>  # each log message will be re-retrieved in order to normalize the
>  # autosquash arrangement
>  rearrange_squash () {
> + format=$(git config --get rebase.instructionFormat)
>   # extract fixup!/squash! lines and resolve any referenced sha1's
>   while read -r pick sha1 message
>   do
> @@ -1210,26 +1211,27 @@ else
>   revisions=$onto...$orig_head
>   shortrevisions=$shorthead
>  fi
> -format=$(git config --get rebase.instructionFormat)
> -# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H 
> to parse
> -git rev-list $merges_option --format="%m%H ${format:-%s}" \
> - --reverse --left-right --topo-order \
> - $revisions ${restrict_revision+^$restrict_revision} | \
> - sed -n "s/^>//p" |
> -while read -r sha1 rest
> -do
> -
> - if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit 
> $sha1
> - then
> - comment_out="$comment_char "
> - else
> - comment_out=
> - fi
> +if test t != "$preserve_merges"
> +then
> + git rebase--helper --make-script ${keep_empty:+--keep-empty} \
> + $revisions ${restrict_revision+^$restrict_revision} >"$todo"
> +else
> + format=$(git config --get rebase.instructionFormat)
> + # the 'rev-list .. | 

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[liam Beguin]

Hi Johannes,

On 29/05/17 06:59 AM, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Thu, 25 May 2017, Liam Beguin wrote:
> 
>> Johannes Schindelin  writes:
>>
>>> diff --git a/sequencer.c b/sequencer.c
>>> index 130cc868e51..88819a1a2a9 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -2388,3 +2388,52 @@ void append_signoff(struct strbuf *msgbuf, int 
>>> ignore_footer, unsigned flag)
>>>  
>>> strbuf_release();
>>>  }
>>> +
>>> +int sequencer_make_script(int keep_empty, FILE *out,
>>> +   int argc, const char **argv)
>>> +{
>>> +   char *format = NULL;
>>> +   struct pretty_print_context pp = {0};
>>> +   struct strbuf buf = STRBUF_INIT;
>>> +   struct rev_info revs;
>>> +   struct commit *commit;
>>> +
>>> +   init_revisions(, NULL);
>>> +   revs.verbose_header = 1;
>>> +   revs.max_parents = 1;
>>> +   revs.cherry_pick = 1;
>>> +   revs.limited = 1;
>>> +   revs.reverse = 1;
>>> +   revs.right_only = 1;
>>> +   revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
>>> +   revs.topo_order = 1;
>>> +
>>> +   revs.pretty_given = 1;
>>> +   git_config_get_string("rebase.instructionFormat", );
>>> +   if (!format || !*format) {
>>> +   free(format);
>>> +   format = xstrdup("%s");
>>> +   }
>>> +   get_commit_format(format, );
>>> +   free(format);
>>> +   pp.fmt = revs.commit_format;
>>> +   pp.output_encoding = get_log_output_encoding();
>>> +
>>> +   if (setup_revisions(argc, argv, , NULL) > 1)
>>> +   return error(_("make_script: unhandled options"));
>>> +
>>> +   if (prepare_revision_walk() < 0)
>>> +   return error(_("make_script: error preparing revisions"));
>>> +
>>> +   while ((commit = get_revision())) {
>>> +   strbuf_reset();
>>> +   if (!keep_empty && is_original_commit_empty(commit))
>>> +   strbuf_addf(, "%c ", comment_line_char);
>>
>> I've never had to use empty commits before, but while testing this, I
>> noticed that `git rebase -i --keep-empty` behaves differently if using
>> the --root option instead of a branch or something like 'HEAD~10'.
>> I also tested this on v2.13.0 and the behavior is the same.
> 
> FWIW the terminology "empty commit" is a pretty poor naming choice. This
> is totally not your fault at all. I just wish we had a much more intuitive
> name to describe a commit that does not introduce any changes to the tree.
> 
> And yes, doing this with --root is a bit of a hack. That's because --root
> is a bit of a hack.
> 
> I am curious, though, as to the exact differences you experienced. I mean,
> it could be buggy behavior that needs to be fixed (independently of this
> patch series, of course).
> 

Sorry, reading this I realized that I didn't give any details!
When using --root, --keep-empty has no effect. The empty commits
do not appear in the todo list and they are removed.
Also, when using --root without --keep-empty, the empty commits
do not show up as comments in the list.

> Ciao,
> Johannes
> 

Liam 

revision API design, was Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Johannes Schindelin]

Hi Junio,

On Tue, 30 May 2017, Junio C Hamano wrote:

> We do not want these [revision API] implementation details to code that
> does not implement command line parsing.  This one is not parsing
> anybody's set of options and should not be mucking with the low level
> implementation details.

Just to make sure we are on the same level: you want the argc & argv to be
the free-form API of setup_revisions(), rather than code setting fields
in struct rev_info whose names can be verified at compile time, and whose
names also suggest their intended use.

I am still flabberghasted that any seasoned software developer sincerely
would suggest this.

> See 66b2ed09 ("Fix "log" family not to be too agressive about
> showing notes", 2010-01-20) and poinder.  Back then, nobody outside
> builtin/log.c and revision.c (these are the two primary things that
> implement command line parsing; "log.c" needs access to the low
> level details because it wants to establish custom default that is
> applied before it reads options given by the end-user) mucked
> directly with verbose_header, which allowed the addition of
> "pretty_given" to be limited only to these places that actually do
> the parsing.  If the above patch to sequencer.c existed before
> 66b2ed09, it would have required unnecessary change to tweak
> "pretty_given" in there too when 66b2ed09 was done.  That is forcing
> a totally unnecesary work.  And there is no reason to expect that
> the kind of change 66b2ed09 made to the general command line parsing
> will not happen in the future.  Adding more code like the above that
> knows the implementation detail is unwarranted, when you can just
> ask the existing command line parser to set them for you.

This is a very eloquent description of a problem with the API.

The correct suggestion would be to fix the API, of course. Not to declare
an interface intended for command-line parsing the internal API.

Maybe the introduction of `pretty_given` was a strong hint at a design
flaw to begin with, pointing to the fact that user_format is a singleton
(yes, really, you can't have two different user_formats in the same Git
process, what were we thinking)?

Ciao,
Dscho

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Johannes Schindelin]

Hi Junio,

On Tue, 30 May 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > You still ask me to pass options in plain text that has to be parsed at
> > run-time, rather than compile-time-verifiable flags.
> 
> Absolutely.  

In other words, you want me to put my name to sloppy code.

Sorry, not interested,
Dscho

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Junio,
>
> On Mon, 29 May 2017, Junio C Hamano wrote:
>
>> Johannes Schindelin  writes:
>> 
>> > diff --git a/sequencer.c b/sequencer.c
>> > index 130cc868e51..88819a1a2a9 100644
>> > --- a/sequencer.c
>> > +++ b/sequencer.c
>> > @@ -2388,3 +2388,52 @@ void append_signoff(struct strbuf *msgbuf, int 
>> > ignore_footer, unsigned flag)
>> >  
>> >strbuf_release();
>> >  }
>> > +
>> > +int sequencer_make_script(int keep_empty, FILE *out,
>> > +  int argc, const char **argv)
>> > +{
>> > +  char *format = NULL;
>> > +  struct pretty_print_context pp = {0};
>> > +  struct strbuf buf = STRBUF_INIT;
>> > +  struct rev_info revs;
>> > +  struct commit *commit;
>> > +
>> > +  init_revisions(, NULL);
>> > +  revs.verbose_header = 1;
>> > +  revs.max_parents = 1;
>> > +  revs.cherry_pick = 1;
>> > +  revs.limited = 1;
>> > +  revs.reverse = 1;
>> > +  revs.right_only = 1;
>> > +  revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
>> > +  revs.topo_order = 1;
>> 
>> cf. 
>> 
>> This is still futzing below the API implementation detail
>> unnecessarily.
>
> You still ask me to pass options in plain text that has to be parsed at
> run-time, rather than compile-time-verifiable flags.

Absolutely.  

We do not want these implementation details to code that does not
implement command line parsing.  This one is not parsing anybody's
set of options and should not be mucking with the low level
implementation details.

See 66b2ed09 ("Fix "log" family not to be too agressive about
showing notes", 2010-01-20) and poinder.  Back then, nobody outside
builtin/log.c and revision.c (these are the two primary things that
implement command line parsing; "log.c" needs access to the low
level details because it wants to establish custom default that is
applied before it reads options given by the end-user) mucked
directly with verbose_header, which allowed the addition of
"pretty_given" to be limited only to these places that actually do
the parsing.  If the above patch to sequencer.c existed before
66b2ed09, it would have required unnecessary change to tweak
"pretty_given" in there too when 66b2ed09 was done.  That is forcing
a totally unnecesary work.  And there is no reason to expect that
the kind of change 66b2ed09 made to the general command line parsing
will not happen in the future.  Adding more code like the above that
knows the implementation detail is unwarranted, when you can just
ask the existing command line parser to set them for you.

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Johannes Schindelin]

Hi Liam,

On Thu, 25 May 2017, Liam Beguin wrote:

> Johannes Schindelin  writes:
>
> > diff --git a/sequencer.c b/sequencer.c
> > index 130cc868e51..88819a1a2a9 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2388,3 +2388,52 @@ void append_signoff(struct strbuf *msgbuf, int 
> > ignore_footer, unsigned flag)
> >  
> > strbuf_release();
> >  }
> > +
> > +int sequencer_make_script(int keep_empty, FILE *out,
> > +   int argc, const char **argv)
> > +{
> > +   char *format = NULL;
> > +   struct pretty_print_context pp = {0};
> > +   struct strbuf buf = STRBUF_INIT;
> > +   struct rev_info revs;
> > +   struct commit *commit;
> > +
> > +   init_revisions(, NULL);
> > +   revs.verbose_header = 1;
> > +   revs.max_parents = 1;
> > +   revs.cherry_pick = 1;
> > +   revs.limited = 1;
> > +   revs.reverse = 1;
> > +   revs.right_only = 1;
> > +   revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> > +   revs.topo_order = 1;
> > +
> > +   revs.pretty_given = 1;
> > +   git_config_get_string("rebase.instructionFormat", );
> > +   if (!format || !*format) {
> > +   free(format);
> > +   format = xstrdup("%s");
> > +   }
> > +   get_commit_format(format, );
> > +   free(format);
> > +   pp.fmt = revs.commit_format;
> > +   pp.output_encoding = get_log_output_encoding();
> > +
> > +   if (setup_revisions(argc, argv, , NULL) > 1)
> > +   return error(_("make_script: unhandled options"));
> > +
> > +   if (prepare_revision_walk() < 0)
> > +   return error(_("make_script: error preparing revisions"));
> > +
> > +   while ((commit = get_revision())) {
> > +   strbuf_reset();
> > +   if (!keep_empty && is_original_commit_empty(commit))
> > +   strbuf_addf(, "%c ", comment_line_char);
> 
> I've never had to use empty commits before, but while testing this, I
> noticed that `git rebase -i --keep-empty` behaves differently if using
> the --root option instead of a branch or something like 'HEAD~10'.
> I also tested this on v2.13.0 and the behavior is the same.

FWIW the terminology "empty commit" is a pretty poor naming choice. This
is totally not your fault at all. I just wish we had a much more intuitive
name to describe a commit that does not introduce any changes to the tree.

And yes, doing this with --root is a bit of a hack. That's because --root
is a bit of a hack.

I am curious, though, as to the exact differences you experienced. I mean,
it could be buggy behavior that needs to be fixed (independently of this
patch series, of course).

Ciao,
Johannes

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Johannes Schindelin]

Hi Junio,

On Mon, 29 May 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > diff --git a/sequencer.c b/sequencer.c
> > index 130cc868e51..88819a1a2a9 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2388,3 +2388,52 @@ void append_signoff(struct strbuf *msgbuf, int 
> > ignore_footer, unsigned flag)
> >  
> > strbuf_release();
> >  }
> > +
> > +int sequencer_make_script(int keep_empty, FILE *out,
> > +   int argc, const char **argv)
> > +{
> > +   char *format = NULL;
> > +   struct pretty_print_context pp = {0};
> > +   struct strbuf buf = STRBUF_INIT;
> > +   struct rev_info revs;
> > +   struct commit *commit;
> > +
> > +   init_revisions(, NULL);
> > +   revs.verbose_header = 1;
> > +   revs.max_parents = 1;
> > +   revs.cherry_pick = 1;
> > +   revs.limited = 1;
> > +   revs.reverse = 1;
> > +   revs.right_only = 1;
> > +   revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> > +   revs.topo_order = 1;
> 
> cf. 
> 
> This is still futzing below the API implementation detail
> unnecessarily.

You still ask me to pass options in plain text that has to be parsed at
run-time, rather than compile-time-verifiable flags.

I am quite puzzled that you keep asking me to make my code sloppy.

Ciao,
Dscho

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Junio C Hamano]

Johannes Schindelin  writes:

> diff --git a/sequencer.c b/sequencer.c
> index 130cc868e51..88819a1a2a9 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2388,3 +2388,52 @@ void append_signoff(struct strbuf *msgbuf, int 
> ignore_footer, unsigned flag)
>  
>   strbuf_release();
>  }
> +
> +int sequencer_make_script(int keep_empty, FILE *out,
> + int argc, const char **argv)
> +{
> + char *format = NULL;
> + struct pretty_print_context pp = {0};
> + struct strbuf buf = STRBUF_INIT;
> + struct rev_info revs;
> + struct commit *commit;
> +
> + init_revisions(, NULL);
> + revs.verbose_header = 1;
> + revs.max_parents = 1;
> + revs.cherry_pick = 1;
> + revs.limited = 1;
> + revs.reverse = 1;
> + revs.right_only = 1;
> + revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> + revs.topo_order = 1;

cf. 

This is still futzing below the API implementation detail
unnecessarily.

[PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Liam Beguin]

Hi Johannes,

Johannes Schindelin  writes:
> The first step of an interactive rebase is to generate the so-called "todo
> script", to be stored in the state directory as "git-rebase-todo" and to
> be edited by the user.
> 
> Originally, we adjusted the output of `git log ` using a simple
> sed script. Over the course of the years, the code became more
> complicated. We now use shell scripting to edit the output of `git log`
> conditionally, depending whether to keep "empty" commits (i.e. commits
> that do not change any files).
> 
> On platforms where shell scripting is not native, this can be a serious
> drag. And it opens the door for incompatibilities between platforms when
> it comes to shell scripting or to Unix-y commands.
> 
> Let's just re-implement the todo script generation in plain C, using the
> revision machinery directly.
> 
> This is substantially faster, improving the speed relative to the
> shell script version of the interactive rebase from 2x to 3x on Windows.
> 
> Note that the rearrange_squash() function in git-rebase--interactive
> relied on the fact that we set the "format" variable to the config setting
> rebase.instructionFormat. Relying on a side effect like this is no good,
> hence we explicitly perform that assignment (possibly again) in
> rearrange_squash().
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/rebase--helper.c   |  8 +++-
>  git-rebase--interactive.sh | 44 +
>  sequencer.c| 49 
> ++
>  sequencer.h|  3 +++
>  4 files changed, 82 insertions(+), 22 deletions(-)
> 
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index ca1ebb2fa18..821058d452d 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -11,15 +11,19 @@ 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;
>   enum {
> - CONTINUE = 1, ABORT
> + CONTINUE = 1, ABORT, MAKE_SCRIPT
>   } command = 0;
>   struct option options[] = {
>   OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
> + OPT_BOOL(0, "keep-empty", _empty, N_("keep empty 
> commits")),
>   OPT_CMDMODE(0, "continue", , N_("continue rebase"),
>   CONTINUE),
>   OPT_CMDMODE(0, "abort", , N_("abort rebase"),
>   ABORT),
> + OPT_CMDMODE(0, "make-script", ,
> + N_("make rebase script"), MAKE_SCRIPT),
>   OPT_END()
>   };
>  
> @@ -36,5 +40,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   return !!sequencer_continue();
>   if (command == ABORT && argc == 1)
>   return !!sequencer_remove_state();
> + if (command == MAKE_SCRIPT && argc > 1)
> + return !!sequencer_make_script(keep_empty, stdout, argc, argv);
>   usage_with_options(builtin_rebase_helper_usage, options);
>  }
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 2c9c0165b5a..609e150d38f 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -785,6 +785,7 @@ collapse_todo_ids() {
>  # each log message will be re-retrieved in order to normalize the
>  # autosquash arrangement
>  rearrange_squash () {
> + format=$(git config --get rebase.instructionFormat)
>   # extract fixup!/squash! lines and resolve any referenced sha1's
>   while read -r pick sha1 message
>   do
> @@ -1210,26 +1211,27 @@ else
>   revisions=$onto...$orig_head
>   shortrevisions=$shorthead
>  fi
> -format=$(git config --get rebase.instructionFormat)
> -# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H 
> to parse
> -git rev-list $merges_option --format="%m%H ${format:-%s}" \
> - --reverse --left-right --topo-order \
> - $revisions ${restrict_revision+^$restrict_revision} | \
> - sed -n "s/^>//p" |
> -while read -r sha1 rest
> -do
> -
> - if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit 
> $sha1
> - then
> - comment_out="$comment_char "
> - else
> - comment_out=
> - fi
> +if test t != "$preserve_merges"
> +then
> + git rebase--helper --make-script ${keep_empty:+--keep-empty} \
> + $revisions ${restrict_revision+^$restrict_revision} >"$todo"
> +else
> + format=$(git config --get rebase.instructionFormat)
> + # the 'rev-list .. | sed' requires %m to parse; the instruction 
> requires %H to parse
> + git rev-list $merges_option --format="%m%H ${format:-%s}" \
> + --reverse --left-right --topo-order \
> + $revisions 

[PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Johannes Schindelin]

The first step of an interactive rebase is to generate the so-called "todo
script", to be stored in the state directory as "git-rebase-todo" and to
be edited by the user.

Originally, we adjusted the output of `git log ` using a simple
sed script. Over the course of the years, the code became more
complicated. We now use shell scripting to edit the output of `git log`
conditionally, depending whether to keep "empty" commits (i.e. commits
that do not change any files).

On platforms where shell scripting is not native, this can be a serious
drag. And it opens the door for incompatibilities between platforms when
it comes to shell scripting or to Unix-y commands.

Let's just re-implement the todo script generation in plain C, using the
revision machinery directly.

This is substantially faster, improving the speed relative to the
shell script version of the interactive rebase from 2x to 3x on Windows.

Note that the rearrange_squash() function in git-rebase--interactive
relied on the fact that we set the "format" variable to the config setting
rebase.instructionFormat. Relying on a side effect like this is no good,
hence we explicitly perform that assignment (possibly again) in
rearrange_squash().

Signed-off-by: Johannes Schindelin 
---
 builtin/rebase--helper.c   |  8 +++-
 git-rebase--interactive.sh | 44 +
 sequencer.c| 49 ++
 sequencer.h|  3 +++
 4 files changed, 82 insertions(+), 22 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index ca1ebb2fa18..821058d452d 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -11,15 +11,19 @@ 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;
enum {
-   CONTINUE = 1, ABORT
+   CONTINUE = 1, ABORT, MAKE_SCRIPT
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
+   OPT_BOOL(0, "keep-empty", _empty, N_("keep empty 
commits")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
ABORT),
+   OPT_CMDMODE(0, "make-script", ,
+   N_("make rebase script"), MAKE_SCRIPT),
OPT_END()
};
 
@@ -36,5 +40,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!sequencer_continue();
if (command == ABORT && argc == 1)
return !!sequencer_remove_state();
+   if (command == MAKE_SCRIPT && argc > 1)
+   return !!sequencer_make_script(keep_empty, stdout, argc, argv);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2c9c0165b5a..609e150d38f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -785,6 +785,7 @@ collapse_todo_ids() {
 # each log message will be re-retrieved in order to normalize the
 # autosquash arrangement
 rearrange_squash () {
+   format=$(git config --get rebase.instructionFormat)
# extract fixup!/squash! lines and resolve any referenced sha1's
while read -r pick sha1 message
do
@@ -1210,26 +1211,27 @@ else
revisions=$onto...$orig_head
shortrevisions=$shorthead
 fi
-format=$(git config --get rebase.instructionFormat)
-# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to 
parse
-git rev-list $merges_option --format="%m%H ${format:-%s}" \
-   --reverse --left-right --topo-order \
-   $revisions ${restrict_revision+^$restrict_revision} | \
-   sed -n "s/^>//p" |
-while read -r sha1 rest
-do
-
-   if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit 
$sha1
-   then
-   comment_out="$comment_char "
-   else
-   comment_out=
-   fi
+if test t != "$preserve_merges"
+then
+   git rebase--helper --make-script ${keep_empty:+--keep-empty} \
+   $revisions ${restrict_revision+^$restrict_revision} >"$todo"
+else
+   format=$(git config --get rebase.instructionFormat)
+   # the 'rev-list .. | sed' requires %m to parse; the instruction 
requires %H to parse
+   git rev-list $merges_option --format="%m%H ${format:-%s}" \
+   --reverse --left-right --topo-order \
+   $revisions ${restrict_revision+^$restrict_revision} | \
+   sed -n "s/^>//p" |
+   while read -r sha1 rest
+   do
+
+   if test -z "$keep_empty" && is_empty_commit $sha1 && ! 
is_merge_commit $sha1
+   then
+