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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 10, 2019, 17:47 UTC
Message-ID: <00733b69-7c9d-2aba-42e3-3a2f85f29007@gmail.com>
In-Reply-To: <20190610134321.8203-1-rohit.ashiwal265@gmail.com>

Hi Rohit

On 10/06/2019 14:43, Rohit Ashiwal wrote:

Show 27 quoted lines

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

Shout if you get stuck. I think distinguishing the "user committed a conflict resolution and so we don't want to skip" from the "the pick would have overwritten an untracked file so we do want to skip" cases should be possible by calling rollback_is_safe(). It would be good to have a test case for at least the "pick would have overwritten an untracked file case" as that would be easy to break in the future without noticing as it's a rare situation.

Show 17 quoted lines

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

I think you can just pass a flag to rollback_single_pick() to tell it whether it is rolling back for --skip or --abort so it can do the right thing.

Best Wishes

Phillip

Show 22 quoted lines

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