From: Ramkumar Ramachandra Date: Sun, 17 Jul 2011 16:11:34 GMT Subject: Re: [PATCH 17/17] revert: Introduce --continue to continue the operation Message-ID: In-Reply-To: <20110712204650.GG14909@elie> Hi Jonathan, Jonathan Nieder writes: > Ramkumar Ramachandra wrote: >> Using the information in ".git/sequencer", it is now possible to >> continue a cherry-pick or continue after a conflict.  To do this, we >> have to parse the information in ".git/sequencer/opts" into the >> replay_opts structure and > [...] > > Might be simpler to say: > >        Introduce a new "git cherry-pick --continue" command which uses >        the information in ".git/sequencer" to continue a cherry-pick that >        stopped because of a conflict or other error.  It works by dropping >        the first instruction from .git/sequencer/todo and performing the >        remaining cherry-picks listed there, with options (think "-s" and >        "-X") from the initial command listed in .git/sequencer/opts. > >        So now you can do: > >                $ git cherry-pick -Xpatience foo..bar >                ... description conflict in commit moo ... >                $ git cherry-pick --continue >                error: 'cherry-pick' is not possible because you have unmerged files. >                fatal: failed to resume cherry-pick >                $ echo resolved >conflictingfile >                $ git add conflictingfile && git commit >                $ git cherry-pick --continue; # resumes with the commit after "moo" Yep, thanks for the pretty commit message :) >> During the "git commit" stage, CHERRY_PICK_HEAD will aid by providing >> the commit message from the conflicting "moo" commit.  Note that the >> cherry-pick mechanism has no control at this stage, so the user is >> free to violate anything that was specified during the first >> cherry-pick invocation.  For example, if "-x" was specified during the >> first cherry-pick invocation, the user is free to edit out the message >> during commit time.  One glitch to note is that the "--signoff" option >> specified at cherry-pick invocation time is not reflected in the >> commit message provided by CHERRY_PICK_HEAD; the user must take care >> to add "--signoff" during the "git commit" invocation. > > The -s thing doesn't have much to do with this change.  But is it a > bug or not?  If it's not a bug, then this is not so much a glitch to > note as an important feature to ensure people don't sign off on a > conflict resolution without thinking about it.  (I guess I think it's > a bug.  It's hard to decide.) Bug, definitely. It happens because unlike "-x" where the cherry-picking machinery appends to the commit message, "-s" is handled at commit-time (when it spawns `git commit`). Ofcourse, if I were never to write the sequencing features, this would never been seen as a bug -- hence the term "glitch"; an implementation detail that doesn't suit our future plans (namely, this series) very well. >> --- a/t/t3510-cherry-pick-sequence.sh >> +++ b/t/t3510-cherry-pick-sequence.sh >> @@ -101,4 +101,97 @@ test_expect_success '--reset cleans up sequencer state' ' > [...] >> +test_expect_success '--continue complains when no cherry-pick is in progress' ' >> +     pristine_detach initial && >> +     test_must_fail git cherry-pick --continue >actual 2>&1 && >> +     test_i18ngrep "error" actual > > This would start to fail if the message ever changed from "error" to > "fatal", right?  I don't think that's a good thing. Okay, removed this and all similar checks. >> +test_expect_success '--continue complains when there are unresolved conflicts' ' >> +     pristine_detach initial && >> +     head=$(git rev-parse HEAD) && >> +     test_must_fail git cherry-pick base..picked && >> +     test_must_fail git cherry-pick --continue && >> +     git cherry-pick --reset >> +' >> + >> +test_expect_success '--continue continues after conflicts are resolved' ' >> +     pristine_detach initial && >> +     head=$(git rev-parse HEAD) && > > How is $head used? Stray line -- copy-pasting from an older version on my own tests. Removed everywhere. >> +     test_must_fail git cherry-pick base..anotherpick && >> +     echo "c" >foo && >> +     git add foo && >> +     git commit && >> +     git cherry-pick --continue && >> +     test_path_is_missing .git/sequencer && >> +     { >> +             git rev-list HEAD | >> +             git diff-tree --root --stdin | >> +             sed "s/[0-9a-f]\{40\}/OBJID/g" >> +     } >actual && > > $_x40 is idiomatic and safer with old seds. I see. I actually thought '[0-9a-f]\{40\}' would be more traditional, and that's why I picked in the first place :p >> +test_expect_success '--continue respects opts' ' >> +     pristine_detach initial && >> +     head=$(git rev-parse HEAD) && >> +     test_must_fail git cherry-pick -s -x base..anotherpick && >> +     echo "c" >foo && >> +     git add foo && >> +     git commit -s && >> +     git cherry-pick --continue && >> +     test_path_is_missing .git/sequencer && >> +     git cat-file commit HEAD >anotherpick_msg && >> +     git cat-file commit HEAD~1 >picked_msg && >> +     git cat-file commit HEAD~2 >unrelatedpick_msg && >> +     git cat-file commit HEAD~3 >initial_msg && >> +     test_must_fail test_i18ngrep "Signed-off-by:" initial_msg && > > This will break when GETTEXT_POISON is set --- test_i18ngrep > automatically succeeds in that case. > > Is "Signed-off-by" meant to be translated anyway?  I would use > >        ! grep > > if testing that. My bad. Replaced with grep. Thanks. > By the way, that probably should go in a separate test assertion > ("-s is not automatically propagated to resolved conflict") to make > it easier to change the behavior later. Separated into another test. Thanks. -- Ram