On Thu, Mar 10, 2016 at 12:48 AM, Christian Couder <christian.cou...@gmail.com> wrote: > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
