Re: [PATCH 2/2] stash push: remove duplicate changes detection

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

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

>> Before the precontext of this hunk, repo_refresh_and_write_index()
>> is called to refresh the index.  We used to leave early when
>> check_changes() saw no need to save.  We no longer do so, and
>> instead keep going.
>> 
>>> @@ -1743,8 +1737,15 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
>>>   
>>>   	if (stash_msg)
>>>   		strbuf_addstr(&stash_msg_buf, stash_msg);
>>> -	if (do_create_stash(ps, &stash_msg_buf, include_untracked, patch_mode,
>>> -			    interactive_opts, only_staged, &info, &patch, quiet)) {
>>> +	ret =  do_create_stash(ps, &stash_msg_buf, include_untracked,
>>> +			       patch_mode, interactive_opts, only_staged, &info,
>>> +			       &patch, quiet);
>> 
>> And we call do_create_stash().  The first thing it does is to call
>> repo_read_index_preload() and repo_refresh_and_write_index().
>> 
>> Are we refreshing the index twice now, even though we know nothing
>> has changed in between, when we run "git stash push"?
>
> We've always been doing that when there is something to stash (which is 
> the normal case).

So when there is nothing to stash, we only refreshed once but now
refreshing twice is not a regression?

> To avoid that we need to make the callers of do_create_stash()
> responsible for refreshing the index.  At the moment
> create_stash() calls do_create_stash() without evening reading the
> index, but do_push_stash() needs to read the index to check if it
> the pathspec contains paths that don't match the index. That would
> also avoid calling preload_index() multiple times.

Yes, that exactly is the right direction.  Then we can reduce the
number of check_changes() without increasing the number of
refreshes, right?

Thanks.