Re: [GSoC][PATCH v3 2/3] cherry-pick/revert: add --skip option

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 13, 2019, 19:57 UTC
Message-ID: <xmqqtvctuul6.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <xmqqa7elwer9.fsf@gitster-ct.c.googlers.com>

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

Show 23 quoted lines

>> -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?