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

From: Junio C Hamano <gitster@pobox.com>
To: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Cc: git@vger.kernel.org, newren@gmail.com, t.gummerer@gmail.com,
	phillip.wood123@gmail.com
Subject: Re: [GSoC][PATCH v3 2/3] cherry-pick/revert: add --skip option
Date: Thu, 13 Jun 2019 12:57:25 -0700	[thread overview]
Message-ID: <xmqqtvctuul6.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <xmqqa7elwer9.fsf@gitster-ct.c.googlers.com> (Junio C. Hamano's message of "Thu, 13 Jun 2019 10:56:26 -0700")

Junio C Hamano <gitster@pobox.com> 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?