Re: [GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C

From: Alban Gruin <alban.gruin@gmail.com>
Date: Jun 18, 2018, 17:04 UTC
Message-ID: <f7320b16-a543-11c1-d511-fd95ec7a7f8d@gmail.com>
In-Reply-To: <6583e68e-3aeb-3afa-8e13-9391b77e4fe7@talktalk.net>

Hi Phillip,

Le 18/06/2018 à 17:34, Phillip Wood a écrit :

Show 10 quoted lines

> On 18/06/18 14:18, Alban Gruin wrote:
>> This rewrites setup_reflog_action() from shell to C.
>>
>> A new command is added to rebase--helper.c, “setup-reflog”, as such as a
>> new flag, “verbose”, to silence the output of the checkout operation
>> called by setup_reflog_action().
> 
> I'm having difficulty understanding what that means, surely the verbose
> flag is to stop the output from being silenced.
>

I reversed the meaning of the flag, I will correct it in a reroll.

Show 11 quoted lines

> I'm not that keen on the naming in this patch, if it's only a staging
> post and will be removed it probably doesn't matter too much. I can see
> why you've based the function and flag names on the shell version, but
> the C version is not setting up the reflog it is checking out the branch
> we're rebasing. --checkout-base or something like that would be clearer.
> 
> Also the name of the function that does the checkout is tied to checking
> out the base revision, but then reused in the next patch to checkout the
> 'onto' commit. As such I think it would be clearer if it was called
> run_git_checkout() or something like that.
>

Right.

> One further thought - how easy would it be to refactor the code in
> do_reset() to handle the checkouts in this patch series, rather than
> having to fork 'git checkout'
>

Good remark, I did not notice do_reset(). I intend to refactor and optimize after I have done (most of) the conversion, so this goes into my todo-list, but it does not really seem difficult at first sight.

Cheers, Alban