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

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

Hi Rohit

On 10/06/2019 06:57, Rohit Ashiwal wrote:
> Hey Phillip
> 
> On Sun, 9 Jun 2019 19:01:35 +0100 Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Rohit
>>
>> --skip is definitely a useful addition to cherry-pick/revert
>>
>> On 08/06/2019 20:19, Rohit Ashiwal wrote:
>>>
>>> [...]
>>> @@ -2784,6 +2784,29 @@ int sequencer_rollback(struct repository *r, struct replay_opts *opts)
>>>  	return -1;
>>>  }
>>>  
>>> +int sequencer_skip(struct repository *r, struct replay_opts *opts)
>>> +{
>>> +	switch (opts->action) {
>>> +	case REPLAY_REVERT:
>>> +		if (!file_exists(git_path_revert_head(r)))
>>> +			return error(_("no revert in progress"));
>>
>> This error message is slightly misleading. If the user has already
>> committed a conflict resolution then REVERT_HEAD/CHERRY_PICK_HEAD will
>> not exist but it is possible that a cherry-pick/revert is in progress if
>> the user was picking a sequence of commits. It would be nice to give a
>> different error message or maybe just a warning in that case.
>> sequencer_get_last_command() should help with that.

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

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

> 2. single commit:
>     Then ACTION is already over and there is nothing to do and the
>     error is correct.
> 
>>> +		break;
>>> +	case REPLAY_PICK:
>>> +		if (!file_exists(git_path_cherry_pick_head(r)))
>>> +			return error(_("no cherry-pick in progress"));
>>> +		break;
>>> +	default:
>>> +		BUG("the control must not reach here.");
>>> +	}
>>> +
>>> +	if (rollback_single_pick(r))
>>> +		return error(_("failed to skip the commit"));
>>
>> 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.

> 
>>> [...]
>>> +test_expect_success 'skip a commit and check if rest of sequence is correct' '
>>> +	pristine_detach initial &&
>>> +	echo e >expect &&
>>> +	cat >expect.log <<-EOF &&
>>> +	OBJID
>>> +	:100644 100644 OBJID OBJID M	foo
>>> +	OBJID
>>> +	:100644 100644 OBJID OBJID M	foo
>>> +	OBJID
>>> +	:100644 100644 OBJID OBJID M	unrelated
>>> +	OBJID
>>> +	:000000 100644 OBJID OBJID A	foo
>>> +	:000000 100644 OBJID OBJID A	unrelated
>>> +	EOF
>>> +	test_must_fail git cherry-pick base..yetanotherpick &&
>>> +	test_must_fail git cherry-pick --skip &&
>>
>> It would be good to check that the cherry-pick has progressed since
>> --skip and didn't just fail without trying to pick another commit.
> 
> 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.

Best Wishes

Phillip


>  
>> Best Wishes
>>
>> Phillip
> 
> Thanks
> Rohit
>