Re: [PATCH 82/83] environment: add set_index_file()

From: Christian Couder <christian.couder@gmail.com>
Date: May 4, 2016, 11:50 UTC
Message-ID: <CAP8UFD1kfjU58cey-T9g3Nwo1RM6DN6MrgxyJQL8Ad53a+3DmQ@mail.gmail.com>
In-Reply-To: <xmqqh9efp30n.fsf@gitster.mtv.corp.google.com>

On Tue, May 3, 2016 at 5:36 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Christian Couder <christian.couder@gmail.com> writes:
>
>> +void set_index_file(char *index_file)
>> +{
>> +     git_index_file = index_file;
>> +}
>
> What's the rationale for this change, and more importantly, the
> ownership rule for the string?  When you call this function, does
> the caller still own that piece of memory?  When you call this
> twice, where does the old value go and who is responsible for
> taking care of not leaking it?
>
> Cannot guess any of the above with no proposed log message (that
> comment applies most of your patches in this series).

Yeah, I agree that I could have been more verbose on this one, and some other ones too.

The reason for this is that run_apply() in builtin/am.c has a "const char *index_file" argument. The current version of run_apply() does:

        if (index_file)
                argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s",
index_file);

to pass that parameter to the `git apply` process that it launches.

I couldn't find a good way other than doing:

    if (index_file) {
        save_index_file = get_index_file();
        set_index_file((char *)index_file);
    }

    /* Call libified apply functions */
    ...

    if (index_file)
        set_index_file(save_index_file);

to do the equivalent of what was done previously.

So I guess I could have written something like the following in the commit message of the commit that introduces set_index_file():

    Introduce set_index_file() to be able to temporarily change the index file.

    It should be used like:

        /* Save current index file */
        old_index_file = get_index_file();
        set_index_file((char *)tmp_index_file);

        /* Do stuff that will use tmp_index_file as the index file */
        ...

        /* When finished reset the index file */
        set_index_file(old_index_file);

Maybe I could also add a comment just before the function.