Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Alban Gruin <alban.gruin@gmail.com>
Cc: git@vger.kernel.org,
	Christian Couder <christian.couder@gmail.com>,
	Thomas Gummerer <t.gummerer@gmail.com>,
	Son Luong Ngoc <sluongng@gmail.com>
Subject: Re: [PATCH v3 2/6] stash: remove the second index in stash_working_tree()
Date: Fri, 31 Jul 2020 11:26:41 -0700
Message-ID: <xmqqy2mz35i6.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200731165140.29197-3-alban.gruin@gmail.com> (Alban Gruin's message of "Fri, 31 Jul 2020 18:51:36 +0200")

Alban Gruin <alban.gruin@gmail.com> writes:

> This removes the second index used in stash_working_tree() to simplify
> the code.

Continuing what I said for [0/6], say "the second index file" to
clarify the distinction between the in-core index and on-disk index
files to avoid confusion.

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

Hmph.  So at this point i_tree is what is in the index, we reset the
working tree to it with reset_tree(), and instead of writing to
$TMPindex, we let reset_tree() clobber the main on-disk index but we
do not care because the result is supposed to be the same as what
was originally in the index anyway?

> @@ -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)) {

And then the new code now lets "update-index" work directly on the
main index (which does make an observable difference to the outside
world, but we are not letting any hook to look at this intermediate
state, so it might be OK---I cannot tell at this point in the code).

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

We used to read from $TMPindex, which has been updated with the
contents of files modified in the working tree, and write its
content out as a tree object, grabbing its object name into w_tree.

The new code instead writes out the same tree from the main index,
which has been clobbered with the working tree changes that the user
hasn't added to the index.  Because of that, we need to discard
these changes and re-read the in-core index out of i_tree with
reset_tree().

So, this change makes one new call to reset_tree() that scans and
updates working tree files that are modified?  How expensive is
that?  

It's not like this change trades cost to write out a temporary index
file with the cost of having to call reset_tree() one more time---as
far as I can see, the new code writes to on-disk index file the same
time as the original code.

How is the use of second on-disk index hurting us?  I must be
missing something obvious, but at this point, it is not clear what
we are gaining---I only see downsides.


  reply index

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-05 10:48 [RFC PATCH v1 0/6] stash: drop usage of a second index Alban Gruin
2020-05-05 10:48 ` [RFC PATCH v1 1/6] stash: mark `i_tree' in reset_tree() const Alban Gruin
2020-06-13  8:09   ` Christian Couder
2020-05-05 10:48 ` [RFC PATCH v1 2/6] stash: remove the second index in stash_working_tree() Alban Gruin
2020-06-13  8:52   ` Christian Couder
2020-06-13 18:00     ` Alban Gruin
2020-06-15 12:02       ` Christian Couder
2020-05-05 10:48 ` [RFC PATCH v1 3/6] stash: remove the second index in stash_patch() Alban Gruin
2020-06-13  9:38   ` Christian Couder
2020-06-13 10:04     ` Christian Couder
2020-05-05 10:48 ` [RFC PATCH v1 4/6] stash: remove the second index in save_untracked_files() Alban Gruin
2020-06-13 18:51   ` Christian Couder
2020-05-05 10:48 ` [RFC PATCH v1 5/6] stash: remove the second index in restore_untracked() Alban Gruin
2020-06-13 19:41   ` Christian Couder
2020-05-05 10:48 ` [RFC PATCH v1 6/6] stash: remove `stash_index_path' Alban Gruin
2020-06-04 12:07 ` [RFC PATCH v1 0/6] stash: drop usage of a second index Alban Gruin
2020-06-13  7:52 ` Christian Couder
2020-06-25 12:35   ` Alban Gruin
2020-06-15 15:27 ` SZEDER Gábor
2020-06-15 21:50   ` SZEDER Gábor
2020-06-16  7:06     ` SZEDER Gábor
2020-06-17 20:04       ` Junio C Hamano
2020-06-17 21:31         ` Alban Gruin
2020-06-30 15:15 ` [PATCH v2 " Alban Gruin
2020-06-30 15:15   ` [PATCH v2 1/6] stash: mark `i_tree' in reset_tree() const Alban Gruin
2020-06-30 15:15   ` [PATCH v2 2/6] stash: remove the second index in stash_working_tree() Alban Gruin
2020-06-30 15:15   ` [PATCH v2 3/6] stash: remove the second index in stash_patch() Alban Gruin
2020-06-30 15:15   ` [PATCH v2 4/6] stash: remove the second index in save_untracked_files() Alban Gruin
2020-06-30 15:15   ` [PATCH v2 5/6] stash: remove the second index in restore_untracked() Alban Gruin
2020-07-31 13:45     ` Christian Couder
2020-07-31 16:16       ` Alban Gruin
2020-06-30 15:15   ` [PATCH v2 6/6] stash: remove `stash_index_path' Alban Gruin
2020-07-31 13:53   ` [PATCH v2 0/6] stash: drop usage of a second index Christian Couder
2020-07-31 16:51   ` [PATCH v3 " Alban Gruin
2020-07-31 16:51     ` [PATCH v3 1/6] stash: mark `i_tree' in reset_tree() const Alban Gruin
2020-07-31 18:28       ` Junio C Hamano
2020-07-31 16:51     ` [PATCH v3 2/6] stash: remove the second index in stash_working_tree() Alban Gruin
2020-07-31 18:26       ` Junio C Hamano [this message]
2020-08-02  2:20         ` Junio C Hamano
2020-07-31 16:51     ` [PATCH v3 3/6] stash: remove the second index in stash_patch() Alban Gruin
2020-07-31 16:51     ` [PATCH v3 4/6] stash: remove the second index in save_untracked_files() Alban Gruin
2020-07-31 16:51     ` [PATCH v3 5/6] stash: remove the second index in restore_untracked() Alban Gruin
2020-07-31 16:51     ` [PATCH v3 6/6] stash: remove `stash_index_path' Alban Gruin
2020-07-31 17:48     ` [PATCH v3 0/6] stash: drop usage of a second index Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqy2mz35i6.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=alban.gruin@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=sluongng@gmail.com \
    --cc=t.gummerer@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git