[RFC,v1,2/6] stash: remove the second index in stash_working_tree()
diff mbox series

Message ID 20200505104849.13602-3-alban.gruin@gmail.com
State New
Headers show
Series
  • stash: drop usage of a second index
Related show

Commit Message

Alban Gruin May 5, 2020, 10:48 a.m. UTC
This removes the second index used in stash_working_tree() to simplify
the code.  It also help to avoid issues with the split-index: when
stash_working_tree() is called, the index is at `i_tree', and this tree
is extracted in a second index for use in a subcommand.  This is not a
problem in the non-split-index case, but in the split-index case, if the
shared index file has expired and is removed by a subcommand, the main
index contains a reference to a file that no longer exists.

The calls to set_alternative_index_output() are dropped to extract
`i_tree' to the main index, and `GIT_INDEX_FILE' is no longer set before
starting `update-index'.  When it exits, the index has changed, and must
be discarded.

The call to reset_tree() becomes useless: the only caller of
stash_working_tree() is do_create_stash(), which creates `i_tree' from
its index, calls save_untracked_files() if requested (but as it also
works on a second index, it is unaffected), then calls
stash_working_tree().  But when save_untracked_files() will be modified
to stop using another index, it won't reset the tree, because
stash_patch() wants to work on a different tree (`b_tree') than
stash_working_tree().

At the end of the function, the tree is reset to `i_tree'.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/stash.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

Comments

Christian Couder June 13, 2020, 8:52 a.m. UTC | #1
On Tue, May 5, 2020 at 12:56 PM Alban Gruin <alban.gruin@gmail.com> wrote:
>
> This removes the second index used in stash_working_tree() to simplify
> the code.  It also help to avoid issues with the split-index: when

s/help/helps/

> stash_working_tree() is called, the index is at `i_tree', and this tree
> is extracted in a second index for use in a subcommand.  This is not a
> problem in the non-split-index case, but in the split-index case, if the
> shared index file has expired and is removed by a subcommand, the main
> index contains a reference to a file that no longer exists.

As this is fixing a bug and there is no test, it might help if you can
at least give an example of something that used to fail before this
patch and doesn't after it. You are talking about stash subcommands
but it is not very clear which one for example can trigger the bug.

> The calls to set_alternative_index_output() are dropped to extract
> `i_tree' to the main index, and `GIT_INDEX_FILE' is no longer set before
> starting `update-index'.  When it exits, the index has changed, and must
> be discarded.

That makes sense.

> The call to reset_tree() becomes useless:

Your patch doesn't remove any call to reset_tree(), but actually adds
one. So the above is difficult to understand.

Do you want to say that in a later patch it will be possible to remove
the call to reset_tree()? Or do you want to say that the call to
write_index_as_tree() becomes useless?

> the only caller of
> stash_working_tree() is do_create_stash(), which creates `i_tree' from
> its index, calls save_untracked_files() if requested (but as it also
> works on a second index, it is unaffected), then calls
> stash_working_tree().  But when save_untracked_files() will be modified
> to stop using another index, it won't reset the tree, because
> stash_patch() wants to work on a different tree (`b_tree') than
> stash_working_tree().
>
> At the end of the function, the tree is reset to `i_tree'.
Alban Gruin June 13, 2020, 6 p.m. UTC | #2
Hi Christian,

Le 13/06/2020 à 10:52, Christian Couder a écrit :
> On Tue, May 5, 2020 at 12:56 PM Alban Gruin <alban.gruin@gmail.com> wrote:
>>
>> This removes the second index used in stash_working_tree() to simplify
>> the code.  It also help to avoid issues with the split-index: when
> 
> s/help/helps/
> 
>> stash_working_tree() is called, the index is at `i_tree', and this tree
>> is extracted in a second index for use in a subcommand.  This is not a
>> problem in the non-split-index case, but in the split-index case, if the
>> shared index file has expired and is removed by a subcommand, the main
>> index contains a reference to a file that no longer exists.
> 
> As this is fixing a bug and there is no test, it might help if you can
> at least give an example of something that used to fail before this
> patch and doesn't after it. You are talking about stash subcommands
> but it is not very clear which one for example can trigger the bug.
> 
>> The calls to set_alternative_index_output() are dropped to extract
>> `i_tree' to the main index, and `GIT_INDEX_FILE' is no longer set before
>> starting `update-index'.  When it exits, the index has changed, and must
>> be discarded.
> 
> That makes sense.
> 
>> The call to reset_tree() becomes useless:
> 
> Your patch doesn't remove any call to reset_tree(), but actually adds
> one. So the above is difficult to understand.
> 
> Do you want to say that in a later patch it will be possible to remove
> the call to reset_tree()? Or do you want to say that the call to
> write_index_as_tree() becomes useless?
> 

No, I meant that with this commit, reset_tree() does not need to be
called at the beginning of stash_working_tree(), because it is only
called by do_create_stash(), which sets the index at `i_tree', and
save_untracked_files() does not change the main index.  But it will
become useful again down the line, when save_untracked_file() will be
rewritten to use the "main" index, so I did not remove it.

I hope it makes more sense now.

>> the only caller of
>> stash_working_tree() is do_create_stash(), which creates `i_tree' from
>> its index, calls save_untracked_files() if requested (but as it also
>> works on a second index, it is unaffected), then calls
>> stash_working_tree().  But when save_untracked_files() will be modified
>> to stop using another index, it won't reset the tree, because
>> stash_patch() wants to work on a different tree (`b_tree') than
>> stash_working_tree().
>>
>> At the end of the function, the tree is reset to `i_tree'.

Alban
Christian Couder June 15, 2020, 12:02 p.m. UTC | #3
On Sat, Jun 13, 2020 at 8:00 PM Alban Gruin <alban.gruin@gmail.com> wrote:
>
> Le 13/06/2020 à 10:52, Christian Couder a écrit :
> > On Tue, May 5, 2020 at 12:56 PM Alban Gruin <alban.gruin@gmail.com> wrote:

> >> The call to reset_tree() becomes useless:
> >
> > Your patch doesn't remove any call to reset_tree(), but actually adds
> > one. So the above is difficult to understand.
> >
> > Do you want to say that in a later patch it will be possible to remove
> > the call to reset_tree()? Or do you want to say that the call to
> > write_index_as_tree() becomes useless?
> >
>
> No, I meant that with this commit, reset_tree() does not need to be
> called at the beginning of stash_working_tree(), because it is only
> called by do_create_stash(), which sets the index at `i_tree', and
> save_untracked_files() does not change the main index.  But it will
> become useful again down the line, when save_untracked_file() will be
> rewritten to use the "main" index, so I did not remove it.
>
> I hope it makes more sense now.

Yeah, it makes more sense with an explanation like the above. Instead
of "down the line" though, you might want to say something like "in a
later commit", as I think it will make it clearer for reviewers that
the commit when it will become useful again should be part of the same
series.

Thanks,
Christian.

Patch
diff mbox series

diff --git a/builtin/stash.c b/builtin/stash.c
index 9baa8b379e..2535335275 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1059,17 +1059,14 @@  static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
 	struct rev_info rev;
 	struct child_process cp_upd_index = CHILD_PROCESS_INIT;
 	struct strbuf diff_output = STRBUF_INIT;
-	struct index_state istate = { NULL };
 
 	init_revisions(&rev, NULL);
 	copy_pathspec(&rev.prune_data, ps);
 
-	set_alternate_index_output(stash_index_path.buf);
 	if (reset_tree(&info->i_tree, 0, 0)) {
 		ret = -1;
 		goto done;
 	}
-	set_alternate_index_output(NULL);
 
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = add_diff_to_buf;
@@ -1091,8 +1088,6 @@  static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
 	argv_array_pushl(&cp_upd_index.args, "update-index",
 			 "--ignore-skip-worktree-entries",
 			 "-z", "--add", "--remove", "--stdin", NULL);
-	argv_array_pushf(&cp_upd_index.env_array, "GIT_INDEX_FILE=%s",
-			 stash_index_path.buf);
 
 	if (pipe_command(&cp_upd_index, diff_output.buf, diff_output.len,
 			 NULL, 0, NULL, 0)) {
@@ -1100,19 +1095,16 @@  static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
 		goto done;
 	}
 
-	if (write_index_as_tree(&info->w_tree, &istate, stash_index_path.buf, 0,
-				NULL)) {
+	discard_cache();
+	if (write_cache_as_tree(&info->w_tree, 0, NULL) ||
+	    reset_tree(&info->i_tree, 0, 1))
 		ret = -1;
-		goto done;
-	}
 
 done:
-	discard_index(&istate);
 	UNLEAK(rev);
 	object_array_clear(&rev.pending);
 	clear_pathspec(&rev.prune_data);
 	strbuf_release(&diff_output);
-	remove_path(stash_index_path.buf);
 	return ret;
 }