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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Rohit

On 10/06/2019 14:43, Rohit Ashiwal wrote:
> 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.

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

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