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

From: Stefan Beller <sbeller@google.com>
Date: Jul 14, 2017, 22:50 UTC
Message-ID: <CAGZ79kYFgR_Lo-hWq6_UanvidG6a9H54qop=sg_33FCsA9Z0pQ@mail.gmail.com>
In-Reply-To: <814284075336e715dd52859e69aeb41226624af7.1500043437.git.johannes.schindelin@gmx.de>

On Fri, Jul 14, 2017 at 7:44 AM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 19 quoted lines

> 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)
> +{

Show 16 quoted lines

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

Show 7 quoted lines

> 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