Hi again,
Jonathan Nieder writes:
> 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.
>> --- 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html