Alban Gruin <alban.gruin@xxxxxxxxx> writes: >> Hmph. It is easy enough to do the clean-up ourselves in this code, >> instead of asking the caller to do so. On the other hand, stashing >> of local changes is done by the caller, so it feels a bit strange >> way to divide the labor between the two parts. >> >> Other than that design choice, the patch itself looks reasonable and >> a fairly faithful reimplementation of what the scripted one did. >> > > This was the behaviour of the old complete_action(). The new one cleans > up by itself; this is the > apply_autostash()/sequencer_remove_state()/todo_list_release() dance > done at three places inside complete_action(), and has not changed since > v3[0]. > > Maybe I misunderstood what you said? I am not all that interested in comparing your previous attempts; I am more focused on the end result of applying these proposed changes. In the end result, IIUC, stashing away the local modification before rebasing happens is done in the caller (i.e. "git rebase" proper) and the caller is still prepared to do the clean-up of applying that local modification back. If "rebase -i" did the stashing away at the beginning, and cleaning up before it gives back control, that would have been one coherent division of labor. Or if "rebase -i" did neither and let "rebase" proper do both, that also would have been another coherent division of labor. Making the caller, "rebase" proper, responsible for preparing the data necessary to later come back (i.e. stashing away the local mods) while letting the callee, "rebase -i", consume that data by applying the stash, felt a strange way to divide the labor between the two parts. That is what I meant. If we are planning to make all the backend responsible for stashing away before they run and applying the stash after they are done, then it might make sense to have the application side on the backend as the first step. But if what you need to do to stash away local changes and what you need to do to reapply the stash is the same for all backends, wouldn't it make more sense to have the logic in the caller side?
