Hi Junio On 2019-06-13 19:57 UTC Junio C Hamano <gitster@xxxxxxxxx> wrote: > > I think my earlier comments would lead to a wrong direction, i.e. to > justify the change made to rollback_single_pick(), so let's step > back a bit. Perhaps the change is unjustifiable and that is why I > had trouble reading it and trying to make sense out of it. > > Is it possible that the new callsite that passes is_skip==1 should > not be calling it (while castrating many parts of the callee) in the > first place? Perhaps it is doing something _different_ from being > called "rollback single pick" (or perhaps the name of the function > is not specific enough to describe what its existing caller, i.e. the > one that passes is_skip==0 after your patch, calls it for)? IOW, > would it lead to a better code structure if you left the original > rollback_single_pick() helper and its caller alone (perhaps rename > it to make it clearer what it does), and *add* a new helper around > the underlying reset_for_rollback() function and call it from here? That is why I added a new function, but I was not super sure if it was a good idea to begin with, since it only introduced small changes to `reset_for_rollback` (also Phillip[1] discouraged it so, I stopped thinking in that direction). > Perhaps it is not rolling back but is skipping, so the new function > needs to be called skip_single_pick() or something, and the existing > one is named correctly and there is no need for even renaming? We could have a wrapper function to `reset_for_rollback` and better rename it to `reset_merge` since we *are* resetting a merge which translates to rolling-back in reality? Thanks Rohit [1]: https://public-inbox.org/git/6d3c1c1e-6140-dd8c-c37f-8c625b04ddc9@xxxxxxxxx/
