Re: [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Stefan,

On Mon, 10 Dec 2018, Stefan Beller wrote:

> On Mon, Dec 10, 2018 at 2:08 PM Johannes Sixt <j6t@kdbg.org> wrote:
> >
> > Am 10.12.18 um 20:04 schrieb Johannes Schindelin via GitGitGadget:
> > > The idea was brought up by Paul Morelle.
> > >
> > > To be honest, this idea of rescheduling a failed exec makes so much sense
> > > that I wish we had done this from the get-go.
> >
> > The status quo was actually not that bad a decision, because it made 'x
> > false' as a substitute for 'break' very convenient.
> >
> > But now that we have a real 'break', I'm very much in favor of flipping
> > the behavior over to rescheduling. (I'm actually not a user of the
> > feature, but the proposed behavior is so compellingly logical.)
> 
> In rare cases I had commands that may be dangerous if rerun,
> but I'd just not run them with -y but with -x.

Please note that 3/3 (i.e. the `-y` option) is *really* only intended as a
"I could do this if anybody *really* wanted to" patch. I actually would
strongly prefer not to have this patch in git.git at all.

> That brings me to some confusion I had in the last patch:
> To catch a flaky test I surely would be tempted to
>     git rebase -x make -y "make test"
> but that might reschedule a compile failure as well,
> as a single -y has implications on all other -x's.
> 
> I wonder if it might be better to push this mechanism
> one layer down: Instead of having a flag that changes
> the behavior of the "exec" instructions and having a
> handy '-y' short cut for the new mode, we'd rather have
> a new type of command that executes&retries a command
> ("exnrt", 'n'), which can still get the '-y' command line flag,
> but more importantly by having 2 separate sets of
> commands we'd have one set that is a one-shot, and the
> other that is retried. Then we can teach the user which
> is safe and which isn't for rescheduling.
> 
> By having two classes, I would anticipate fewer compatibility
> issues ('"Exec" behaves differently, and I forgot I had turned
> on the rescheduling').

As Junio points out, this brings us back to the proposal to have two
different `exec` commands.

I am really unenthusiastic about this idea, as I find it "hard to sell":
what little benefit it would bring to have two commands that pretty much
do the same thing almost all the time would be outweighed *by far* by the
confusion we would sow by that.

It is amazing to me how much my perspective changed when I actually had to
teach Git to new users. Things that I live with easily all of a sudden
become these unnecessarily confusing road blocks that make it *so hard* to
actually use Git.

> Talking about rescheduling: In the above example the flaky
> test can flake more than once, so I'd be stuck with keeping
> 'git rebase --continue'ing after I see the test flaked once again.

No, you would not be stuck with that.

You would read the error message carefully (we do that all the time, yes?)
and then run `git rebase --edit-todo` to remove that line before
continuing.

:-)

If you feel very strongly about this, we could introduce a new option for
`git rebase --skip`, say, `git rebase --skip --skip-next-commands-too=1`.
(I specifically used a too-long option name, as you and I are both
Germans, known to use too-long names for everything. I do not really
intend to use such an awkward option name, if we decide that such an
option would be a good idea. Probably more like `git rebase
--skip-next[=<N>]`.)

> My workflow with interactive rebase and fixing up things as I go
> always involves a manual final "make test" to check "for real",
> which I could lose now, which is nice.

My workflow with `git rebase -r --exec "make test"` is pretty similar to
yours. With the difference that I would want those commands to be
rescheduled *even if* the command is flakey. Actually, *in particular*
when it is flakey.

I really see myself calling

	git config --global rebase.rescheduleFailedExec true

in all my setups.

Ciao,
Dscho