Hey Phillip
On Sun, 9 Jun 2019 19:01:35 +0100 Phillip Wood <[email protected]>
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
