Re: [PATCH v2 5/7] stash create: rework temporary index creation

[Phillip Wood <phillip.wood123@gmail.com>]

On 14/10/2026 19:16, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> It is unclear what problem we are trying to solve.  Obviously we can
> enhance create_index_from_tree() and replace reset_tree() just fine,
> but the proposed log message should make it clear why we are doing so.
> 
> Please follow the usual pattern to state "observation" (e.g., "we
> use reset_tree() when we do X"), "problem" (e.g., "use of
> reset_tree() is undesirable because ..."), and "solution" (e.g, "we
> can improve by replacing reset_tree() with create_index_from_tree()
> because the latter has these desirable characteristics") in this
> order, perhaps in separate paragraphs for extra readability.

I'll reword following that pattern. The gist is that the code is easier 
to reason about if we use create_index_from_tree() because we avoid 
depending on global variables for the index path and we don't overwrite 
the in-memory index.

Thanks

Phillip

> 
> 
> 
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   builtin/stash.c | 15 ++++-----------
>>   1 file changed, 4 insertions(+), 11 deletions(-)
>>
>> diff --git a/builtin/stash.c b/builtin/stash.c
>> index b4fe7465e4..ddeb5a5d2a 100644
>> --- a/builtin/stash.c
>> +++ b/builtin/stash.c
>> @@ -374,7 +374,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
>>   }
>>   
>>   static int create_index_from_tree(const struct object_id *tree_id,
>> -				  const char *index_path)
>> +				  const char *index_path, bool reset)
>>   {
>>   	int nr_trees = 1;
>>   	int ret = 0;
>> @@ -403,7 +403,7 @@ static int create_index_from_tree(const struct object_id *tree_id,
>>   	opts.src_index = the_repository->index;
>>   	opts.dst_index = &dst_istate;
>>   	opts.merge = 1;
>> -	opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
>> +	opts.reset = reset ? UNPACK_RESET_PROTECT_UNTRACKED : 0;
>>   	opts.fn = oneway_merge;
>>   
>>   	if (unpack_trees(nr_trees, t, &opts)) {
>> @@ -1399,7 +1399,7 @@ static enum create_result stash_patch(struct stash_info *info,
>>   		goto done;
>>   	}
>>   
>> -	if (create_index_from_tree(head_tree, stash_index_path.buf)) {
>> +	if (create_index_from_tree(head_tree, stash_index_path.buf, true)) {
>>   		ret = CREATE_ERROR;
>>   		goto done;
>>   	}
>> @@ -1458,22 +1458,15 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
>>   	repo_init_revisions(the_repository, &rev, NULL);
>>   	copy_pathspec(&rev.prune_data, ps);
>>   
>> -	set_alternate_index_output(stash_index_path.buf);
>> -	if (reset_tree(&info->i_tree, 0, 0)) {
>> +	if (create_index_from_tree(&info->i_tree, stash_index_path.buf, false)) {
>>   		ret = -1;
>>   		goto done;
>>   	}
>> -	set_alternate_index_output(NULL);
>>   
>>   	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
>>   	rev.diffopt.format_callback = add_diff_to_buf;
>>   	rev.diffopt.format_callback_data = &diff_output;
>>   
>> -	if (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0) {
>> -		ret = -1;
>> -		goto done;
>> -	}
>> -
>>   	add_pending_object(&rev, parse_object(the_repository, &info->b_commit),
>>   			   "");
>>   	run_diff_index(&rev, 0);