Subject: Re: [PATCH v7 0/2] diff-files: integrate with sparse index

From: Victoria Dye <[email-masked]>
Date: Thu, 23 Mar 2023 10:25:40 -0700

Shuqi Liang wrote:
> On Wed, Mar 22, 2023 at 7:36 PM Junio C Hamano <[email-masked]> wrote:
> 
>>> 3. Use `--stat` to ignore file creation time differences in unrefreshed
>>> index.
>>
>> I am curious about this one.  Why is this a preferred solution over
>> say "run 'update-index --refresh' before running diff-files"?
>>
>> Note that this is merely "I am curious", not "I think it is wrong".
> 
> Hi Junio
> 
> Thank you for your question, it has prompted me to consider the matter
> further =)  I think both solutions, using git diff-files --stat and using git
> update-index --refresh before git diff-files, can produce the same output but
> in different ways.

While they'll (ideally) give the same user-facing result, there is a
difference in how they exercise 'diff-files' because of how 'update-index
--refresh' will affect SKIP_WORKTREE and sparse directories.

Using the same scenario you've set up for your test, suppose I start with a
fresh copy of the 't1092' repo. In the 'sparse-index' repo copy, 'folder1/'
will be a sparse directory:

$ git ls-files -t --sparse folder1/
S folder1/

(note: "S" indicates that SKIP_WORKTREE is applied to the entry)

Now suppose I copy 'a' into 'folder1/' and run 'update-index --refresh'
then 'ls-files' again:

$ git update-index --refresh
$ git ls-files -t --sparse folder1/
S folder1/0/
H folder1/a

(note: "H" indicates that 'folder1/a' does not have SKIP_WORKTREE applied)

The sparse directory has been expanded and SKIP_WORKTREE has been removed
from the file that's now present on-disk. This was an intentional "safety"
measure added in [1] to address the growing volume of bugs and complexities
in scenarios where SKIP_WORKTREE files existed on disk.

Ultimately, the main difference between this test with & without
'update-index' is who applies those index corrections when initially reading
the index: 'update-index' or 'diff-files'. I lean towards the latter because
the former is tested (almost identically) in 'update-index modify outside
sparse definition' earlier in 't1092'.

[1] https://lore.kernel.org/git/[email-masked]/

> 
> When the index file is not up-to-date, git diff-files may show differences
> between the working directory and the index that are caused by file creation
> time differences, rather than actual changes to the file contents. By using git
> diff-files --stat, which ignores file creation time differences.

More or less, yes. Internally, 'diff-files' will "see" the file creation
differences, but the '--stat' format doesn't print them.

> 
> While 'git update-index --refresh' updates the index file to match the contents
> of the working tree. By running this command before git diff-files, we can
> ensure that the index file is up-to-date and that the output of git diff-files
> accurately reflects the differences between the working directory and the index.

This isn't quite true - 'update-index' only updates the *contents* of index
entries (or, colloquially, "stage them for commit") for files explicitly
provided as arguments. Separately, though, '--refresh' updates *all* index
entries' cached 'stat' information. 

Going a bit deeper: with no arguments, 'update-index' will read the index,
do nothing to it, then write it only if something has changed. In almost all
cases, reading the index doesn't cause any changes to it, making it a no-op.
However, the removal of SKIP_WORKTREE is done on read (including a refresh
of the entry's stat information), so a even plain 'update-index' *without*
'--refresh' would write a modified index to disk. In your test, that means:

	run_on_sparse mkdir -p folder1 &&
	run_on_sparse cp a folder1/a &&
	run_on_all git update-index &&
	test_all_match git diff-files

would get you the same result as:

	run_on_sparse mkdir -p folder1 &&
	run_on_sparse cp a folder1/a &&
	run_on_all git update-index --refresh &&
	test_all_match git diff-files

> 
> Maybe using git update-index --refresh would be more direct and
> straightforward solution.
> 
> (Hi Victoria, do you have any comments?  =)

I hope the above explanation is helpful. I still think '--stat' is the best
way to test this case, but I'm interested to hear your/others' thoughts on
the matter given the additional context.

> Thanks
> Shuqi