Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit
- From
Ramkumar Ramachandra <artagnon@gmail.com>
- Date
- Aug 14, 2011, 16:37 UTC
- Message-ID
- <CALkWK0m0KyZkOqVWJET=_Xy5GxwVfpPkB0OO1b2hJ8aN_x7hGA@mail.gmail.com>
- In-Reply-To
- <20110814160440.GK18466@elie.gateway.2wire.net>
Hi again,
Jonathan Nieder writes:
Show 6 quoted lines
> Ramkumar Ramachandra wrote: >> [...] > > The function name (..._notify_commit()) does not seem very intuitive. > Based on the name, I expect it to use the sequencer to print a message > to the user about the commit in progress.
For the record, I'm not too happy with the name either. I was hoping that someone would suggest a better name during the review :P
> .git/index > - afterward, because when writing the index fails, I (the user) > might want to react by running "git cherry-pick --abort".
Very subtle point: will fix. I'd originally put it along with the other state-removing functions.
> Not about this patch: I keep on forgetting what the argument to > remove_sequencer_state means. Would it be possible to make it > a flag, which would both make the meaning more obvious and mean > it is easy to support additional flags in the future?
I don't want to create a struct and populate it because I highly doubt remove_sequencer_state will take any more arguments in the future. Putting an "int aggressive = 1" and passing the variable instead of the literal is a little inelegant. Actually making any change at this stage is inelegant because of the other remove_sequencer_state() calls :| My call: we can fix it if and when the function needs more arguments.
Show 17 quoted lines
>> --- a/t/t3032-merge-recursive-options.sh >> +++ b/t/t3032-merge-recursive-options.sh >> @@ -114,8 +114,10 @@ test_expect_success 'naive cherry-pick fails' ' >> git read-tree --reset -u HEAD && >> test_must_fail git cherry-pick --no-commit remote && >> git read-tree --reset -u HEAD && >> + git cherry-pick --reset && >> test_must_fail git cherry-pick remote && >> test_must_fail git update-index --refresh && >> + git cherry-pick --reset && >> grep "<<<<<<" text.txt >> ' > > What is this about? Maybe it would be clearer to change the "git > read-tree ..." to "git reset --hard", so the test assertion would not > rely on new cherry-pick features (and to mention the change in the > commit message!).
Okay, I can do this instead (don't check whitespace! I just typed out the patch):
@@ -114,8 +114,10 @@ test_expect_success 'naive cherry-pick fails' ' git read-tree --reset -u HEAD && test_must_fail git cherry-pick --no-commit remote && - git read-tree --reset -u HEAD && + git reset --hard && test_must_fail git cherry-pick remote && test_must_fail git update-index --refresh && + git reset --hard && grep "<<<<<<" text.txt ' > Doesn't this point to a risk in the patch? Do you think there might > be scripts out there relying on being able to use "git read-tree > --reset -u HEAD" to clear away a failed cherry-pick before trying > again, and if so, can we do something about it? I'm not sure we can do anything about it -- we should probably put some kind of warning in the commit message? >> --- a/t/t3510-cherry-pick-sequence.sh >> +++ b/t/t3510-cherry-pick-sequence.sh >> @@ -82,13 +82,13 @@ test_expect_success '--reset cleans up sequencer state' ' >> test_path_is_missing .git/sequencer >> ' >> >> -test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' ' >> +test_expect_success 'final commit cleans up sequencer state' ' >> pristine_detach initial && >> test_must_fail git cherry-pick base..picked && >> - test_path_is_missing .git/sequencer && >> echo "resolved" >foo && >> git add foo && >> git commit && >> + test_path_is_missing .git/sequencer && >> { > > It would also be nice to check "test_path_is_dir" before the final > commit, so people working on this code in the future know both aspects > of the patch are intentional. Good point. Fixed. Thanks. -- Ram