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

[Rohit Ashiwal <rohit.ashiwal265@gmail.com>]

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.

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

> > [...]
> > +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.
 
> Best Wishes
> 
> Phillip

Thanks
Rohit