Re: [PATCH v2 5/8] sequencer: run post-rewrite hook

[Felipe Contreras <felipe.contreras@gmail.com>]

On Mon, Jun 3, 2013 at 1:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> As we should.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  sequencer.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index c217716..3aa480e 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -127,6 +127,37 @@ static void add_rewritten(unsigned char *from, unsigned char *to)
>>       rewritten.nr++;
>>  }
>>
>> +static void run_rewrite_hook(void)
>> +{
>> +     struct strbuf buf = STRBUF_INIT;
>> +     struct child_process proc;
>> +     const char *argv[3];
>> +     int code, i;
>> +
>> +     argv[0] = find_hook("post-rewrite");
>> +     if (!argv[0])
>> +             return;
>> +
>> +     argv[1] = "rebase";
>> +     argv[2] = NULL;
>
> When the end-user action is "git cherry-pick A..B", shouldn't
> the rewrite hook be called with "cherry-pick" not "rebase" as its
> first argument?

Indeed.

> More importantly, doesn't "git revert A..B" also trigger the
> codepath for [4/8] and hence this function?

True.

> I think [3/8] --skip-empty makes sense also for revert, but I do not
> think this one does as-is.  As what [4/8] introduces is a record of
> "rewrite", the patch does not make sense either.  These two steps
> would want to limit themselves to cherry-pick only, no?

Probably.

-- 
Felipe Contreras