From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Sun, 14 Aug 2011 16:37:31 GMT
Subject: Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit
Message-ID: <CALkWK0m0KyZkOqVWJET=_Xy5GxwVfpPkB0OO1b2hJ8aN_x7hGA@mail.gmail.com>
In-Reply-To: <20110814160440.GK18466@elie.gateway.2wire.net>

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