Re: [RFC/PATCH 00/48] Libifying git apply

From: Duy Nguyen <pclouds@gmail.com>
Date: Mar 10, 2016, 09:26 UTC
Message-ID: <CACsJy8Bn1+zbALH347SNgGqSLR0ixCL0acW+DuUZH1g2HRS0ZA@mail.gmail.com>
In-Reply-To: <1457545756-20616-1-git-send-email-chriscool@tuxfamily.org>

On Thu, Mar 10, 2016 at 12:48 AM, Christian Couder <christian.couder@gmail.com> wrote:

Show 21 quoted lines

> This is a patch series about libifying "git apply" functionality, to
> be able to use this functionality in "git am" without spawning new
> processes. This should make "git am" and "git rebase" significantly
> faster.
>
> This has been discussed in the following thread:
>
> http://thread.gmane.org/gmane.comp.version-control.git/287236/
>
> This RFC patch series for now just gets rid of the global variables
> and refactors the code around a bit.
>
> As suggested by Junio the global variables in builtin/apply.c are just
> thrown into a single "apply_state" structure that is passed around the
> callchain. A new parameter called "state" that is a pointer to the
> "apply_state" structure comes at the beginning of the helper functions
> that need it.
>
> Before I make further changes to handle erroneous input and make the
> libified functions not die() and properly clean things up, I'd be
> happy to get some feedback.

I didn't review individual patches. Instead I redid the whole thing (in a slightly different way) and compared my end result to yours. In general it looks good but..

1) I think you should focus the series on moving global vars to struct
apply_state only. You can save code move patches for the later phase.
Easier to review.

2) Given that there are only four local variables shadowing global
ones, p_value, linenr and options, I think it's ok to drop 1/48 and
2/48 and keep the good old names. You just need to mention about them
and what function they are declared in in the relevant "move global
..." patch so the reviewer is aware of it.

3) Moving lock_file to struct apply_state, then putting the whole
struct on stack, could be problematic. I believe there's a global
linked list keeping references of all lock_file variables until the
end of time, so we can't destroy lock_file/apply_state until we are
certain it's safe to do so. We could simply leave lock_file as a
global var for now (with a note in struct apply_state so we don't
forget). We can always do fancy stuff like malloc() later on if we
need to.

4) I noticed on the interdiff that there are translatable strings in
this file but not marked as such. You're going to spend lots of time
reading this file and are in a very good positioin to determine if a
string should be translated (i.e. wrap _() around it) or not. So maybe
you can have a look at this in the next series too. It's absolutely ok
if you say "no" here.

5) Minor detail, but we can rename prefix_ to prefix in
init_apply_state(). There trailing underscore was there in cmd_apply
because there's the global "prefix", but it's gone now.

Looking forward to seeing you double, triple git-rebase's speed.

-- 
Duy