Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks

From: Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr>
Date: Jun 30, 2015, 17:02 UTC
Message-ID: <70428267.93232.1435683724659.JavaMail.zimbra@ensimag.grenoble-inp.fr>
In-Reply-To: <xmqq7fql8949.fsf@gitster.dls.corp.google.com>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Show 14 quoted lines

> Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
> 
> > Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
> >> Shouldn't all the checking also be called in a 'rebase --continue',
> >> considering that it can be called after a 'rebase --edit-todo' ?
> >> (Right now it is only called after closing the editor in 'rebase -i')
> >
> > What's your opinion on it?
> 
> It would probably be better to run the check when running "git rebase
> --continue". This way, we would have a guarantee that the todo-list is
> syntactically correct when going through it.
> 
> Just checking after --edit-todo would not guarantee that:

That's actually what I had in mind, my message wasn't clear enough, my bad.

> But in any case, the most important is the initial edition. It covers
> 99.9% of use-cases. So, I'd say: if you have time to add the checks at
> the other relevant places, good, but not doing it shouldn't block the
> inclusion of the series.

Agreed, that's something that can be done in a future patch without interfering with this one.

Junio C Hamano <gitster@pobox.com> writes:

Show 5 quoted lines

> The place where an error can be introduced is (assuming that what
> "rebase -i" writes out itself is perfect ;-) where we allow the user
> to edit, so instead of checking before "--continue", I would expect
> a sane design would check immediately after the editor we spawned
> returns.

Makes sense but we would have the problem mentioned by Matthieu:

> Warning: the command isn't recognized ...
>   
> # Hmm, let's ignore that warning
> $ git rebase --continue

While in most cases it would be irrelevant (it would be the user that 
ignored the error on purpose), I can imagine the following situation:
 - 'git rebase --edit-todo'
 - get an error
 - go do something else, forgot where I was when I come back
 - 'git status', "you are in the middle of a rebasing"
 - 'git rebase --continue'

Show 11 quoted lines

> The codepath that allows the insn sheet getting edited and the
> codepath that handles --edit-todo have to do many things the same
> way (i.e. use collapse_todo_ids to prepare the file for editing,
> spawn the editor, receive the result and use expand_todo_ids to
> prepare the file to be used by us), and I would have expected for
> these two to be using a single helper---and a sanity check like the
> above can and should be done when we receive the result from the
> editor, immediately before running expand_todo_ids perhaps.
> 
> If they are not using such a single helper right now, perhaps that
> is the preliminary restructuring of the code that is needed?

Good point, maybe I'll try doing that at a later date, however as Matthieu said, I think it doesn't hurt to add this patch (if there is no further corrections to do) and do additional things in a later patch.

Thanks, Rémi