On Thu, Mar 10, 2016 at 10:26 AM, Duy Nguyen <pclouds@xxxxxxxxx> wrote: > On Thu, Mar 10, 2016 at 12:48 AM, Christian Couder > <christian.couder@xxxxxxxxx> 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. I am not sure what you mean by "the later phase", but I tend to agree and that's mostly what I did. Most of the code move are at the end of the series so after the global variable related changes. There are a few exceptions because in a few cases it was difficult to understand the code without a refactoring. In such cases I think the refactoring can also actually help the reviewer. > 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. About 1/48 (builtin/apply: avoid parameter shadowing 'p_value') this is discussed in its own thread. In 2/48 (builtin/apply: avoid parameter shadowing 'linenr'), I change the "good old name" only twice in a small function, and I don't think keeping the old name is a big deal there. > 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. Thanks for this information. Though I prefer to do the fancy stuff right away, that is something like: diff --git a/builtin/apply.c b/builtin/apply.c index 4f57bce..6029869 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -52,6 +52,12 @@ struct apply_state { const char *prefix; int prefix_length; + /* + * Since lockfile.c keeps a linked list of all created + * lock_file structures, it isn't safe to free(lock_file). + */ + struct lock_file *lock_file; + int apply; int allow_overlap; int apply_in_reverse; @@ -4517,8 +4523,6 @@ static int write_out_results(struct apply_state *state, struct patch *list) return errs; } -static struct lock_file lock_file; - #define INACCURATE_EOF (1<<0) #define RECOUNT (1<<1) @@ -4566,8 +4570,10 @@ static int apply_patch(struct apply_state *state, state->apply = 0; state->update_index = state->check_index && state->apply; - if (state->update_index && newfd < 0) - newfd = hold_locked_index(&lock_file, 1); + if (state->update_index && newfd < 0) { + state->lock_file = xcalloc(1, sizeof(struct lock_file)); + newfd = hold_locked_index(state->lock_file, 1); + } if (state->check_index) { if (read_cache() < 0) @@ -4780,7 +4786,7 @@ static int apply_all_patches(struct apply_state *state, } if (state->update_index) { - if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) + if (write_locked_index(&the_index, state->lock_file, COMMIT_LOCK)) die(_("Unable to write new index file")); } It makes it possible to remove the 'newfd' variable as it is only used to check if hold_locked_index() has already been called and now "state->lock_file == NULL" can be used to do that. By the way I wonder if I should also change "static struct lock_file lock" in build_fake_ancestor(). > 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. I will see if I can do that towards the end of the series, but this is not at all on my radar for now. > 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. Yeah, I think this is worth doing, so it is done in my current series: https://github.com/chriscool/git/commits/libify-apply21 > Looking forward to seeing you double, triple git-rebase's speed. Thanks for your reviews and comments, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html