Re: [PATCH 06/17] revert: Eliminate global "commit" variable

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Jul 13, 2011, 08:33 UTC
Message-ID: <CALkWK0mUbEXCjUYVPi8Kvctkh0+bnnk0VoAHjUjT_8M5druzqQ@mail.gmail.com>
In-Reply-To: <20110713071029.GA31285@elie>

Hi again,

Jonathan Nieder writes:

Show 20 quoted lines

> Sounds fine to me.  I could nitpick by saying that "Functions which
> act on commits" is a little vague (I think the actual story is
> something like "The global 'commit' variable used to represent one of
> the arguments passed to 'git cherry-pick' or 'git revert' and thus was
> static along with the options.  When cherry-pick learned to replay
> multiple commits in sequence, that wasn't changed, resulting in a
> somewhat unnatural calling sequence that uses a global to pass in a
> parameters:
>
>        for each commit c {
>                commit = c;
>                res = do_pick_commit();
>                if (res)
>                        return res;
>        }
>
> Teach do_pick_commit and the functions it calls to accept the commit
> to act on as an argument, like we always should have.  Part of a
> campaign to clean up cherry-pick/revert's internal APIs before
> exposing them.")

Yes.  Thanks.  How about this?

revert: Eliminate global "commit" variable

Prior to v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than one commit, 2010-06-02), the file-scope static "commit" variable used to represent one of the arguments passed to the "git cherry-pick" or "git revert". This was not changed after cherry-pick learnt to replay multiple commits in a sequence, resulting in ugly and unclear callsites which rely on setting the global variable before calling a function to act on it:

for each c in commits {
    commit = c;
    act_on_commit();
}

Remove the global variable and modify the API of various functions to pass "commit" around as an argument, so that the callsites become clearer:

for each c in commits {
    act_on_commit(c);
}

The change is also in line with our long-term goal of exposing the cherry-picking machinery through a public API.

-- Ram