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

From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
To: gitster@pobox.com
Cc: git@vger.kernel.org, newren@gmail.com, phillip.wood123@gmail.com,
	rohit.ashiwal265@gmail.com, t.gummerer@gmail.com
Subject: Re: [GSoC][PATCH v3 2/3] cherry-pick/revert: add --skip option
Date: Fri, 14 Jun 2019 09:18:08 +0530	[thread overview]
Message-ID: <20190614034808.19667-1-rohit.ashiwal265@gmail.com> (raw)
In-Reply-To: <xmqqtvctuul6.fsf@gitster-ct.c.googlers.com>

Hi Junio

On 2019-06-13 19:57 UTC Junio C Hamano <gitster@pobox.com> 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@gmail.com/