Hi again, Jonathan Nieder writes: > Ramkumar Ramachandra wrote: >> Since we want to develop the functionality to either pick or revert >> individual commits atomically later in the series, make "commit" a >> variable to be passed around explicitly as an argument for clarity. > > The above explanation is not so clear to me, but the patch looks good. > Isn't the idea something like > > commit = grab_a_nice_commit(); > res = do_pick_commit(); > > being just an unpleasant API relative to > > res = do_pick_commit(grab_a_nice_commit()); > > because in the latter it is more obvious which commit is being > cherry-picked? Likewise with the functions it calls. > > Or perhaps the idea is that eventually we will want to expose something > like do_pick_commit to other translation units, but a static variable > like "commit" would not be appropriate for exposing. Or that we save > a word of global memory. Or that this way if do_pick_commit or a > function it calls ever ends up recursing by mistake it won't get > broken. Or that we can use multiple threads some day. Or... > > Oh, the uncertainty! :) It is not clear to me what any of the above > have to do with wanting the functionality to replay an individual > commit atomically. By the way, what does pickiing or reverting a > commit atomically mean, and how is it different from ordinary > cherry-picks? Right. How about this? revert: Eliminate global "commit" variable Functions which act on commits currently rely on a file-scope static variable to be set before they're called. Consequently, the API and corresponding callsites are ugly and unclear. Remove this variable and change their API to accept the commit to act on as additional argument so that the callsites change from looking like commit = prepare_a_commit(); act_on_commit(); to looking like commit = prepare_a_commit(); act_on_commit(commit); This change is also in line with our long-term goal of exposing some of these functions through a public API. -- Ram -- 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