Re: [PATCH 1/4] stash: convert apply to builtin

[Joel Teichroeb <joel@teichroeb.net>]

On Sun, Mar 25, 2018 at 9:43 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> On 03/24, Joel Teichroeb wrote:
>> ---
>
> Missing sign-off?  I saw it's missing in the other patches as well.
>

Thanks! I always forget to add a sign-off.

>> [...]
>> +
>> +     if (info->has_u) {
>> +             struct child_process cp = CHILD_PROCESS_INIT;
>> +             struct child_process cp2 = CHILD_PROCESS_INIT;
>> +             int res;
>> +
>> +             cp.git_cmd = 1;
>> +             argv_array_push(&cp.args, "read-tree");
>> +             argv_array_push(&cp.args, sha1_to_hex(info->u_tree.hash));
>> +             argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
>> +
>> +             cp2.git_cmd = 1;
>> +             argv_array_pushl(&cp2.args, "checkout-index", "--all", NULL);
>> +             argv_array_pushf(&cp2.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
>> +
>> +             res = run_command(&cp) || run_command(&cp2);
>> +             remove_path(stash_index_path);
>> +             if (res)
>> +                     return error(_("Could not restore untracked files from stash"));
>
> A minor change in behaviour here is that we are removing the temporary
> index file unconditionally here, while we would previously only remove
> it if both 'read-tree' and 'checkout-index' would succeed.
>
> I don't think that's a bad thing, we probably don't want users to try
> and use that index file in any way, and I doubt that's part of anyones
> workflow, so I think cleaning it up makes sense.
>

I'm not sure about that. The shell script has a trap near the start in
order to clean up the temp index, unless I'm understanding the shell
script incorrectly.