Junio C Hamano <gitster@xxxxxxxxx> writes:
>> -static int rollback_single_pick(struct repository *r)
>> +static int rollback_single_pick(struct repository *r, unsigned int is_skip)
>> {
>> ...
>> + if (is_null_oid(&head_oid) && !is_skip)
>> return error(_("cannot abort from a branch yet to be born"));
>> return reset_for_rollback(&head_oid);
>> }
>
> It is unclear *why* the function (and more importantly, its callers)
> would want to omit two sanity checks when is_skip is in effect.
> ...
>> + default:
>> + BUG("the control must not reach here");
>> + }
>> +
>> + if (rollback_single_pick(r, 1))
>> + return error(_("failed to skip the commit"));
>
> And this takes us back to the previous comment. By passing '1'
> here, this caller is asking the callee to omit certain sanity check
> the original version of the callee used to do. What makes it an
> appropriate thing to do so here?
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?
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?
