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

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Use create_index_from_tree() instead of reset_tree() when we create a
> temporary index file to stash the working tree changes. This matches
> what we do for "git stash push -p" and means we can avoid messing with
> set_alternate_index_output(). Add a new parameter to control whether
> the index is reset, as we need to merge the index rather than reset
> it so that we still detect intent-to-add entries. We do not alter the
> in-memory index so we can remove the call to repo_read_index_preload()
> which saves one lstat() call per changed path.

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.



> 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);