On Fri, Jul 14, 2017 at 7:44 AM, Johannes Schindelin
[off-list ref] wrote:

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

Thanks for working on this

+int sequencer_make_script(int keep_empty, FILE *out,
+               int argc, const char **argv)
+{

+       init_revisions(&revs, 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", &format);
+       if (!format || !*format) {
+               free(format);
+               format = xstrdup("%s");
+       }

https://public-inbox.org/git/xmqqvapqo4i8.fsf@gitster.mtv.corp.google.com/

So this is the core part that you and Junio have differing opinions on.

All of the above feels like inviting unnecessary future breakages by
knowing too much about the implementation the current version of
revision.c happens to use.  A more careful implementation would be
to allocate our own av[] and prepare "--reverse", "--left-right",
"--cherry-pick", etc. to be parsed by setup_revisions() call we see
below.  The parsing is not an expensive part of the operation
anyway, and that way we do not have to worry about one less thing.

Allow me go through each of the options which may help
us finding a consensus (at least it helps me having a more
informed opinion).
List of options used outside of revision.c, which in the ideal
world of Git are parsed in e.g. handle_revision_opt called
from setup_revisions:

.verbose_header
  bisect.c:       opt.verbose_header = 1;
  builtin/commit.c:       rev.verbose_header = 1;
  builtin/log.c:  rev->verbose_header = 1;
  builtin/log.c:  rev.verbose_header = 1;
  builtin/log.c:  rev.verbose_header = 1;

.max_parents
  builtin/log.c:  check_rev.max_parents = 1;
  builtin/log.c:  revs.max_parents = 1;
  builtin/log.c:  rev.max_parents = 1;
  builtin/log.c:  revs.max_parents = 1;

.cherry_pick
  is clean!

.limited
  ref-filter.c:   revs.limited = 1;

.reverse
  seems clean.

.right_only:
.sort_order:
  is clean!

.topo_order:
  builtin/fast-export.c:  revs.topo_order = 1;
  builtin/log.c:  revs.topo_order = 1;

.pretty_given
  builtin/log.c:  if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
  builtin/log.c:  if (rev->pretty_given && rev->commit_format == CMIT_FMT_RAW) {

There are two conflicting messages I get:
* only a few fields seem to be polluted (verbose_header,
  max_parents), much fewer than I thought
* we do use these undocumented ways already,
  but not at the scale that DScho is trying to here.

In the reply to the cover letter I outlined that we may have
a problem with integrating the repository struct when using
string arrays only.

Thanks,
Stefan