git-rebase --undo-skip proposal

git-rebase --undo-skip proposal

[Psidium Guajava]

Hello git community,

I'd like to add a new feature in git-rebase: --undo-skip.
But first, I'd like to consult with the experts if it would be
beneficial for the project and if my line of tought is correct.

Imagine that you are working on a feature for a long time, but there
are multiple bug fixes happening at `master` branch at the same time.
After lots of commits on both ends, you decide to rebase your changes
on top of the current `master` branch.
After lots of conflict resolution steps, you mistakenly call
`git-rebase --skip` instead of `git-rebase --continue`, thus losing a
commit of your work, and possibly inserting bugs in your project.
The only solution for this right now would be to abort the current
rebase and start over.

It seems that a feature like this have been requested once on the mail list [1].

I propose the existence of --undo-skip on git-rebase's `$action` domain.

How I fixed it when that happened with me was (just after running the
wrong skip):

1. I figured I was making a rebase that used `git-am` as a backend.
2. In the rebase-apply directory I searched for the patch file with
the change I just skipped.
3. Found the `rebase-apply/next` file.
4. Wrote the number of the patch I skipped - 1 in rebase-apply/next.
5. run `git rebase --skip` again on the repository.

This made the lost patch appear again and I could `--continue` it this time.

I propose the addition of an action `--undo-skip`, that could be
called only after a wrongfully called `--skip`.
`git rebase --undo-skip`.
I would implemented it to do programatically what I did by hand when
that happened with me.

Here are my questions for you:
1. Would this be beneficial for the users?
2. For `rebase--am`, I would need to change `git-rebase--am.sh` file, correct?
3. Can I assume `builtin/am.c` will always store its information on
`$state_dir/next` and `$state_dir/$patchnumbers`?
4. How hard would it be to add that logic for `rebase--interactive`
and `rebase--merge` backends?

Also, a little unrelated with this issue:
5. What happened to the rewrite of rebase in C [2]? I couldn't find
any information after 2016.

[1] https://public-inbox.org/git/201311011522.44631.thomas@koch.ro/
[2] https://public-inbox.org/git/1457779597-6918-1-git-send-email-pyokagan@gmail.com/


Best Regards,
Gabriel Borges

Re: git-rebase --undo-skip proposal

[Stefan Beller]

On Tue, Feb 13, 2018 at 12:22 PM, Psidium Guajava <psiidium@gmail.com> wrote:
> Hello git community,
>
> I'd like to add a new feature in git-rebase: --undo-skip.
> But first, I'd like to consult with the experts if it would be
> beneficial for the project and if my line of tought is correct.
>
> Imagine that you are working on a feature for a long time, but there
> are multiple bug fixes happening at `master` branch at the same time.
> After lots of commits on both ends, you decide to rebase your changes
> on top of the current `master` branch.
> After lots of conflict resolution steps, you mistakenly call
> `git-rebase --skip` instead of `git-rebase --continue`, thus losing a
> commit of your work, and possibly inserting bugs in your project.
> The only solution for this right now would be to abort the current
> rebase and start over.
>
> It seems that a feature like this have been requested once on the mail list [1].
>
> I propose the existence of --undo-skip on git-rebase's `$action` domain.
>
> How I fixed it when that happened with me was (just after running the
> wrong skip):
>
> 1. I figured I was making a rebase that used `git-am` as a backend.
> 2. In the rebase-apply directory I searched for the patch file with
> the change I just skipped.
> 3. Found the `rebase-apply/next` file.
> 4. Wrote the number of the patch I skipped - 1 in rebase-apply/next.
> 5. run `git rebase --skip` again on the repository.
>
> This made the lost patch appear again and I could `--continue` it this time.

I think this could also be done with "git rebase --edit-todo", which brings
up the right file in your editor.

> I propose the addition of an action `--undo-skip`, that could be
> called only after a wrongfully called `--skip`.
> `git rebase --undo-skip`.
> I would implemented it to do programatically what I did by hand when
> that happened with me.
>
> Here are my questions for you:
> 1. Would this be beneficial for the users?

I guess it is.

> 2. For `rebase--am`, I would need to change `git-rebase--am.sh` file, correct?
> 3. Can I assume `builtin/am.c` will always store its information on
> `$state_dir/next` and `$state_dir/$patchnumbers`?
> 4. How hard would it be to add that logic for `rebase--interactive`
> and `rebase--merge` backends?

cc'd Johannes who is currently working on revamping rebase.

>
> Also, a little unrelated with this issue:
> 5. What happened to the rewrite of rebase in C [2]? I couldn't find
> any information after 2016.
>
> [1] https://public-inbox.org/git/201311011522.44631.thomas@koch.ro/
> [2] https://public-inbox.org/git/1457779597-6918-1-git-send-email-pyokagan@gmail.com/

cc'd Paul Tan, maybe he recalls the situation.

Re: git-rebase --undo-skip proposal

[Psidium Guajava]

On 2018-02-13 18:42 GMT-02:00 Stefan Beller <sbeller@google.com> wrote:
> On Tue, Feb 13, 2018 at 12:22 PM, Psidium Guajava <psiidium@gmail.com> wrote:
> I think this could also be done with "git rebase --edit-todo", which brings
> up the right file in your editor.

Yeah that'd would only work if one started a rebase as a interactive
one, not am or merge.

> cc'd Paul Tan, maybe he recalls the situation.

From what I've found on the archive, he didn't recently answer mails
that come from the mail list, I could be wrong tho.

Re: git-rebase --undo-skip proposal

[Johannes Schindelin]

Hi,

On Wed, 14 Feb 2018, Psidium Guajava wrote:

> On 2018-02-13 18:42 GMT-02:00 Stefan Beller <sbeller@google.com> wrote:
> > On Tue, Feb 13, 2018 at 12:22 PM, Psidium Guajava <psiidium@gmail.com> wrote:
> > I think this could also be done with "git rebase --edit-todo", which brings
> > up the right file in your editor.
> 
> Yeah that'd would only work if one started a rebase as a interactive
> one, not am or merge.

I agree that the original proposal was clearly for the non-interactive
rebase (it talked about .git/rebase-apply/).

The biggest problem with the feature request is not how useful it would
be: I agree it would be useful. The biggest problem is that it is a little
bit of an ill-defined problem.

Imagine that you are rebasing 30 patches. Now, let's assume that patch #7
causes a merge conflict, and you mistakenly call `git rebase --skip`.

Now, when is the next possible time you can call `git rebase --undo-skip`?
It could be after a merge conflict in #8. Or in interactive rebase, after
a `pick` that was turned into an `edit`. Or, and this is also entirely
possible, after the rebase finished with #30 without problems and the
rebase is actually no longer in progress.

So I do not think that there is a way, in general, to implement this
feature. Even if you try to remember the state where a `--skip` was
called, you would remember it in the .git/rebase-apply/ (or
.git/rebase-merge/) directory, which is cleaned up after the rebase
concluded successfully. So even then the information required to implement
the feature would not necessarily be there, still, when it would be needed.

Ciao,
Johannes

Re: git-rebase --undo-skip proposal

[Jacob Keller]

On Wed, Feb 14, 2018 at 4:53 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Wed, 14 Feb 2018, Psidium Guajava wrote:
>
>> On 2018-02-13 18:42 GMT-02:00 Stefan Beller <sbeller@google.com> wrote:
>> > On Tue, Feb 13, 2018 at 12:22 PM, Psidium Guajava <psiidium@gmail.com> wrote:
>> > I think this could also be done with "git rebase --edit-todo", which brings
>> > up the right file in your editor.
>>
>> Yeah that'd would only work if one started a rebase as a interactive
>> one, not am or merge.
>
> I agree that the original proposal was clearly for the non-interactive
> rebase (it talked about .git/rebase-apply/).
>
> The biggest problem with the feature request is not how useful it would
> be: I agree it would be useful. The biggest problem is that it is a little
> bit of an ill-defined problem.
>
> Imagine that you are rebasing 30 patches. Now, let's assume that patch #7
> causes a merge conflict, and you mistakenly call `git rebase --skip`.
>
> Now, when is the next possible time you can call `git rebase --undo-skip`?
> It could be after a merge conflict in #8. Or in interactive rebase, after
> a `pick` that was turned into an `edit`. Or, and this is also entirely
> possible, after the rebase finished with #30 without problems and the
> rebase is actually no longer in progress.
>
> So I do not think that there is a way, in general, to implement this
> feature. Even if you try to remember the state where a `--skip` was
> called, you would remember it in the .git/rebase-apply/ (or
> .git/rebase-merge/) directory, which is cleaned up after the rebase
> concluded successfully. So even then the information required to implement
> the feature would not necessarily be there, still, when it would be needed.
>
> Ciao,
> Johannes

Instead of an "--undo-skip", what if we ask the question of what the
user actually wants?

Generally I'd assume that the user wishes to go back to the rebase and
"pick" the commit back in.

So what if we just make "git rebase --skip" more verbose so that it
more clearly spells out which commit is being skipped? Possibly even
as extra lines of "the following patches were skipped during the
rebase" after it completes?

Then it's up to the user to determine what to do with those commits,
and there are many tools they could use to solve it, "git rebase -i,
git cherry-pick, git reflog to restore to a previous and re-run the
rebase, etc".

Thanks,
Jake

Re: git-rebase --undo-skip proposal

[Psidium Guajava]

2018-02-14 22:53 GMT-02:00 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Now, when is the next possible time you can call `git rebase --undo-skip`?

What if the scope were reduced from `--undo-skip` to `--undo-last-skip`?
Also, further reduce the scope to only allow `--undo-last-skip` during
a ongoing rebase, not caring about a finished one?

But, this could be so niche that I have doubts if this would ever be used;

Re: git-rebase --undo-skip proposal

[Jacob Keller]

On Wed, Feb 14, 2018 at 5:50 PM, Psidium Guajava <psiidium@gmail.com> wrote:
> 2018-02-14 22:53 GMT-02:00 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>> Now, when is the next possible time you can call `git rebase --undo-skip`?
>
> What if the scope were reduced from `--undo-skip` to `--undo-last-skip`?
> Also, further reduce the scope to only allow `--undo-last-skip` during
> a ongoing rebase, not caring about a finished one?
>
> But, this could be so niche that I have doubts if this would ever be used;

I think this is too niche to actually be useful in practice,
especially since figuring out exactly what to replay might be tricky..
I suppose it could keep track of where in the rebase the last skip was
used, and then just go back to rebase and stop there again? That
sounds like just redoing the rebase is easier.. (ie: use the reflog to
go back to before the rebase started, and then re-run it again and
don't skip this time).

Re: git-rebase --undo-skip proposal

[Paul Tan]

Hi Gabriel,

On Wed, Feb 14, 2018 at 4:42 AM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Feb 13, 2018 at 12:22 PM, Psidium Guajava <psiidium@gmail.com> wrote:
>>
>> Also, a little unrelated with this issue:
>> 5. What happened to the rewrite of rebase in C [2]? I couldn't find
>> any information after 2016.
>>
>> [1] https://public-inbox.org/git/201311011522.44631.thomas@koch.ro/
>> [2] https://public-inbox.org/git/1457779597-6918-1-git-send-email-pyokagan@gmail.com/
>
> cc'd Paul Tan, maybe he recalls the situation.

It was discarded in favor of Johannes' rebase-helper approach, and I
think parts of it are already in master. There's probably room for
help there.

I haven't had time to keep track of Git development, hence my
inactivity. Sorry about that.

Regards,
Paul

Re: git-rebase --undo-skip proposal

[Johannes Schindelin]

Hi Jake,

On Wed, 14 Feb 2018, Jacob Keller wrote:

> On Wed, Feb 14, 2018 at 4:53 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Wed, 14 Feb 2018, Psidium Guajava wrote:
> >
> >> On 2018-02-13 18:42 GMT-02:00 Stefan Beller <sbeller@google.com> wrote:
> >> > On Tue, Feb 13, 2018 at 12:22 PM, Psidium Guajava
> >> > <psiidium@gmail.com> wrote: I think this could also be done with
> >> > "git rebase --edit-todo", which brings up the right file in your
> >> > editor.
> >>
> >> Yeah that'd would only work if one started a rebase as a interactive
> >> one, not am or merge.
> >
> > I agree that the original proposal was clearly for the non-interactive
> > rebase (it talked about .git/rebase-apply/).
> >
> > The biggest problem with the feature request is not how useful it
> > would be: I agree it would be useful. The biggest problem is that it
> > is a little bit of an ill-defined problem.
> >
> > Imagine that you are rebasing 30 patches. Now, let's assume that patch
> > #7 causes a merge conflict, and you mistakenly call `git rebase
> > --skip`.
> >
> > Now, when is the next possible time you can call `git rebase
> > --undo-skip`?  It could be after a merge conflict in #8. Or in
> > interactive rebase, after a `pick` that was turned into an `edit`. Or,
> > and this is also entirely possible, after the rebase finished with #30
> > without problems and the rebase is actually no longer in progress.
> >
> > So I do not think that there is a way, in general, to implement this
> > feature. Even if you try to remember the state where a `--skip` was
> > called, you would remember it in the .git/rebase-apply/ (or
> > .git/rebase-merge/) directory, which is cleaned up after the rebase
> > concluded successfully. So even then the information required to
> > implement the feature would not necessarily be there, still, when it
> > would be needed.
> 
> Instead of an "--undo-skip", what if we ask the question of what the
> user actually wants?

Heh, I thought in this instance, --undo-skip was what the user wanted ;-)

> Generally I'd assume that the user wishes to go back to the rebase and
> "pick" the commit back in.

Right, and then replay whatever series of commits was picked since the one
that was skipped.

What *could* be done is to save a copy of the current todo list (with the
skipped commit put back in, before the current first line) and save that
together with `git rev-parse HEAD`.

This should make it possible to implement `--undo-skip` for as long as
the rebase did not finish.

Theoretically, you could even save the commit name of the skipped commit
somewhere else than $GIT_DIR/rebase-apply/ (or $GIT_DIR/rebase-merge/),
say, as worktree-local `refs/rebase/skipped`, and then a `git rebase
--undo-skip` could detect the absence of $GIT_DIR/rebase-*/ and fall back
to `git cherry-pick refs/rebase/skipped`.

You'd have to take pains to handle that ref in gc, and to record when the
user edited the todo list via `git rebase --edit-todo` after skipping that
commit (and warn loudly upon/prevent --undo-skip) because those todo list
changes would then be lost.

That's just one way how this feature could be implemented.

It does strike me as awfully specific, though. And it would still only
extend to the latest `git rebase --skip`.

So I am not sure whether we really would want to go this direction, or
whether we can maybe come up with something (probably based on your
suggestion to give the user enough information) that would allow many more
scenarios than just --undo-skip.

> So what if we just make "git rebase --skip" more verbose so that it
> more clearly spells out which commit is being skipped? Possibly even
> as extra lines of "the following patches were skipped during the
> rebase" after it completes?
> 
> Then it's up to the user to determine what to do with those commits,
> and there are many tools they could use to solve it, "git rebase -i,
> git cherry-pick, git reflog to restore to a previous and re-run the
> rebase, etc".

I think this is a saner direction, as it will probably allow more
scenarios to be addressed than just undoing the latest `git rebase
--skip`.

Ciao,
Dscho

Re: git-rebase --undo-skip proposal

[Johannes Schindelin]

Hi Jake,

On Wed, 14 Feb 2018, Jacob Keller wrote:

> On Wed, Feb 14, 2018 at 5:50 PM, Psidium Guajava <psiidium@gmail.com> wrote:
> > 2018-02-14 22:53 GMT-02:00 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> >> Now, when is the next possible time you can call `git rebase --undo-skip`?
> >
> > What if the scope were reduced from `--undo-skip` to `--undo-last-skip`?
> > Also, further reduce the scope to only allow `--undo-last-skip` during
> > a ongoing rebase, not caring about a finished one?
> >
> > But, this could be so niche that I have doubts if this would ever be used;
> 
> I think this is too niche to actually be useful in practice,
> especially since figuring out exactly what to replay might be tricky..
> I suppose it could keep track of where in the rebase the last skip was
> used, and then just go back to rebase and stop there again? That
> sounds like just redoing the rebase is easier.. (ie: use the reflog to
> go back to before the rebase started, and then re-run it again and
> don't skip this time).

Yes, and having a record of what commits were skipped would probably be
very helpful not only in this instance.

Maybe also a record of what commits caused merge conflicts, which a
mapping between original and new commit (which does get a bit tricky with
fixup!/squash! commits, though).

Ciao,
Dscho