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

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

[Elijah Newren]

On Mon, Dec 10, 2018 at 1:18 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:

> 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.
> 
> So let's do the next best thing: implement a command-line option and a
> config setting to make it so.

I was a bit worried that the optics weren't good enough for recovering from
a typoed exec command (or one that otherwise wouldn't be in a state the
user could make pass but they wanted the rebase to continue).  However,
after trying it out, I found:

$ git rebase --exec /bin/false --reschedule-failed-exec HEAD~1
Executing: /bin/false
warning: execution failed: /bin/false
You can fix the problem, and then run

  git rebase --continue


hint: Could not execute the todo command
hint: 
hint:     exec /bin/false
hint: 
hint: It has been rescheduled; To edit the command before continuing, please
hint: edit the todo list first:
hint: 
hint:     git rebase --edit-todo
hint:     git rebase --continue

and if the user just runs "git rebase --continue" without looking at that big
hint, they'll get the hint again.  When they run "git rebase --edit-todo", they
see the failed command listed first and can remove that line.

So, I think my initial worry was unfounded.

 
> The obvious intent behind that config setting is to not only give users a
> way to opt into that behavior already, but also to make it possible to flip
> the default at a later date, after the feature has been battle-tested and
> after deprecating the non-rescheduling behavior for a couple of Git
> versions.
> 
> If the team agrees with my reasoning, then the 3rd patch (introducing -y
> <cmd> as a shortcut for --reschedule-failed-exec -x <cmd>) might not make
> much sense, as it would introduce a short option that would become obsolete
> soon.

This series is awesome; thanks much to Paul for suggesting this idea.
And yeah, I agree and hope the third patch won't be necessary.  :-)

> Johannes Schindelin (3):
>   rebase: introduce --reschedule-failed-exec
>   rebase: add a config option to default to --reschedule-failed-exec
>   rebase: introduce a shortcut for --reschedule-failed-exec
> 
>  Documentation/config/rebase.txt |  5 ++++
>  Documentation/git-rebase.txt    | 11 +++++++++
>  builtin/rebase--interactive.c   |  2 ++
>  builtin/rebase.c                | 42 ++++++++++++++++++++++++++++++++-
>  git-legacy-rebase.sh            | 24 ++++++++++++++++++-
>  git-rebase--common.sh           |  1 +
>  sequencer.c                     | 13 +++++++---
>  sequencer.h                     |  1 +
>  t/t3418-rebase-continue.sh      | 14 +++++++++++
>  9 files changed, 108 insertions(+), 5 deletions(-)
> 
> 
> base-commit: a1598010f775d82b5adf12c29d0f5bc9b41434c6
> Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-90%2Fdscho%2Freschedule-failed-exec-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-90/dscho/reschedule-failed-exec-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/90
> -- 
> gitgitgadget

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

[Stefan Beller]

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

I see. Without the -y patch, this series looks good to me.

> 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

Me, too.

Thanks,
Stefan

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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
> ...
> By having two classes, I would anticipate fewer compatibility
> issues ('"Exec" behaves differently, and I forgot I had turned
> on the rescheduling').

It takes us back to the original proposal that started this whole
thing.

    cf. <3fb5a7ff-a63a-6fac-1456-4dbc9135d088@gmail.com>

After re-reading the thread, I still do not quite follow why it was
considered better to change the way "exec" is run with the command
line option than to implement this as a new insn [*1*], but that is
where this series fit in the larger picture, I think.


[Footnote]

*1* The original proposal called it "test" which I do not think was
    a great name.

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

[Elijah Newren]

On Mon, Dec 10, 2018 at 3:13 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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.
>
> So let's do the next best thing: implement a command-line option and a
> config setting to make it so.
>
> The obvious intent behind that config setting is to not only give users a
> way to opt into that behavior already, but also to make it possible to flip
> the default at a later date, after the feature has been battle-tested and
> after deprecating the non-rescheduling behavior for a couple of Git
> versions.
>
> If the team agrees with my reasoning, then the 3rd patch (introducing -y
> <cmd> as a shortcut for --reschedule-failed-exec -x <cmd>) might not make
> much sense, as it would introduce a short option that would become obsolete
> soon.
>

Complete side-track: This email showed up for me just five minutes
ago, whereas the rest of the series showed up four hours ago, making
me think this email had disappeared and trying to figure out how to
respond when I didn't have the original.  Any ideas why there might be
that level of lag?

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

[Stefan Beller]

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.

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

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.

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.

Thanks,
Stefan

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

[Johannes Sixt]

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

-- Hannes

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

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

So let's do the next best thing: implement a command-line option and a
config setting to make it so.

The obvious intent behind that config setting is to not only give users a
way to opt into that behavior already, but also to make it possible to flip
the default at a later date, after the feature has been battle-tested and
after deprecating the non-rescheduling behavior for a couple of Git
versions.

If the team agrees with my reasoning, then the 3rd patch (introducing -y
<cmd> as a shortcut for --reschedule-failed-exec -x <cmd>) might not make
much sense, as it would introduce a short option that would become obsolete
soon.

Johannes Schindelin (3):
  rebase: introduce --reschedule-failed-exec
  rebase: add a config option to default to --reschedule-failed-exec
  rebase: introduce a shortcut for --reschedule-failed-exec

 Documentation/config/rebase.txt |  5 ++++
 Documentation/git-rebase.txt    | 11 +++++++++
 builtin/rebase--interactive.c   |  2 ++
 builtin/rebase.c                | 42 ++++++++++++++++++++++++++++++++-
 git-legacy-rebase.sh            | 24 ++++++++++++++++++-
 git-rebase--common.sh           |  1 +
 sequencer.c                     | 13 +++++++---
 sequencer.h                     |  1 +
 t/t3418-rebase-continue.sh      | 14 +++++++++++
 9 files changed, 108 insertions(+), 5 deletions(-)


base-commit: a1598010f775d82b5adf12c29d0f5bc9b41434c6
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-90%2Fdscho%2Freschedule-failed-exec-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-90/dscho/reschedule-failed-exec-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/90
-- 
gitgitgadget