Re: [PATCH v4 5/5] stash: implement builtin stash

[Joel Teichroeb <joel@teichroeb.net>]

On Fri, Jun 16, 2017 at 3:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Joel Teichroeb <joel@teichroeb.net> writes:
>> +/*
>> + * Untracked files are stored by themselves in a parentless commit, for
>> + * ease of unpacking later.
>> + */
>> +static int save_untracked(struct stash_info *info, const char *message,
>> +             int include_untracked, int include_ignored, const char **argv)
>> +{
>> +     struct child_process cp = CHILD_PROCESS_INIT;
>> +     struct strbuf out = STRBUF_INIT;
>> +     struct object_id orig_tree;
>> +     int ret;
>> +     const char *index_file = get_index_file();
>> +
>> +     set_alternate_index_output(stash_index_path);
>> +     untracked_files(&out, include_untracked, include_ignored, argv);
>> +
>> +     cp.git_cmd = 1;
>> +     argv_array_pushl(&cp.args, "update-index", "-z", "--add", "--remove",
>> +             "--stdin", NULL);
>> +     argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
>> +
>> +     if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) {
>> +             strbuf_release(&out);
>> +             return 1;
>> +     }
>> +
>
> OK, that's a very straight-forward way of doing this, and as we do
> not care too much about performance in this initial conversion to C,
> it is even sensible.  In a later update after this patch lands, you
> may want to use dir.c's fill_directory() API to find the untracked
> files and add them yourself internally, without running ls-files (in
> untracked_files()) or update-index (here) as subprocesses, but that
> is in the future.  Let's get this round finished.
>
>> +     strbuf_reset(&out);
>> +
>> +     discard_cache();
>> +     read_cache_from(stash_index_path);
>> +
>> +     write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0,NULL);
>
> SP before "NULL".
>
>> +     discard_cache();
>> +
>> +     read_cache_from(stash_index_path);
>
> Hmph, what did anybody change in the on-disk stash_index (or
> contents in the_index) since you read_cache_from()?
>
>> +     write_cache_as_tree(info->u_tree.hash, 0, NULL);
>
> Then you write exactly the same index contents again, this time to
> info->u_tree here.  I am not sure why you need to do this twice, and
> I do not see how orig_tree.hash you wrote earlier is used?
>

I'm not sure I understand what's happening here either. When I was
writing this, it was essentially a lot of trial and error in order to
get the index handling correct. Getting rid of any single one of these
lines makes the test fail. At some point I'd like to redo all the
index handling parts here, as I think I can do without an additional
index, but I'd need to make sure the error handling is perfect first.