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

From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
To: phillip.wood123@gmail.com
Cc: git@vger.kernel.org, newren@gmail.com,
	rohit.ashiwal265@gmail.com, t.gummerer@gmail.com
Subject: Re: [GSoC][PATCH 2/3] cherry-pick/revert: add --skip option
Date: Mon, 10 Jun 2019 19:13:21 +0530	[thread overview]
Message-ID: <20190610134321.8203-1-rohit.ashiwal265@gmail.com> (raw)
In-Reply-To: <2488ef91-b9e5-1836-aeea-2aaf11c3c383@gmail.com>

Hi Phillip

On 2019-06-10 10:40 UTC Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> [...]
> It's actually a bit more complicated as if the cherry-pick failed
> because it would have overwriten untracked files then CHERRY_PICK_HEAD
> will not exist but we want to be able to skip that pick. So it should
> not error out in that case either. (I think you may be able to use the
> abort safety file (see rollback_is_safe()) to distinguish the 'failed to
> pick case' from the 'user committed a conflict resolution' case.)

Oh! I was thinking about some other case. (spawing another cherry-pick,
which is wrong since the topic is --skip). I'm sorry. 

>> Yes, .git/{REVERT|CHERRY_PICK}_HEAD will not exist in this case, but
>> in case of cherry-picking/reverting:
>> 
>> 1. multiple commits:
>>     sequencer dir will exist which will throw out the error listed
>>     under `create_seq_dir`
>
> I don't understand. Wont it will error out here? Why would we call
> create_seq_dir() for --skip?

No, you are correct. This won't skip commit in this case. I'll change
it to do the required.

>>> If rollback_single_pick() sees that HEAD is the null oid then it gives
>>> the error "cannot abort from a branch yet to be born". We're not
>>> aborting and if we're picking a sequence of commits the skip ought
>>> succeed, but it won't at the moment. If we're picking a single commit
>>> then the skip should probably fail like abort but with an appropriate
>>> message. Admittedly that's all a bit of a corner case.
>> 
>> Yes, you are right here. We could actually modify the advice there
>> to be more like _("cannot perform the specified action, the branch
>> is yet to be born") and I think it should suffice this. What do you
>> think?
>
> I think it should allow the user to skip if there are more commits to
> pick . Just changing the error message does not fix that.

Right! I'll check what can be done here.

>> The overall test tests that only, if cherry-pick --skip "failed" then
>> we won't get 'e' inside of `foo` and `test_cmp expect foo` will also
>> fail and if it skipped wrongly then expect.log will not match the
>> actual.log and `test_cmp` will fail. Am I missing something here?
>> Please tell if so.
>
> You're right that the tests at the end would probably pick up a failure,
> but I'm concerned that there could be some obscure corner case we've not
> thought of so checking HEAD and the file contents here would be an
> additional safety measure. It also makes it easier for someone tracking
> down a test failure to see what happened. If they rely only on the test
> at the end they need to spend time to understand where the mismatched
> contents came from.

Yes, it is worth checking here if HEAD after cherry-picking is in the
correct position, same for the file foo. I'll change this test too.
Thanks for pointing out.

Thanks for the review
Rohit