Re: [GSOC][PATCH v2] diff-index: enable sparse index

From: Shuqi Liang <cheskaqiqi@gmail.com>
Date: Apr 22, 2023, 21:25 UTC
Message-ID: <20230422212500.476955-1-cheskaqiqi@gmail.com>
In-Reply-To: <CAPnUp-=3aoG9WwCcLnMZ4UL90j+snL8qUePPmm02WQK9tUkCzw@mail.gmail.com>

>> Re: my last review [2] - did you look into the behavior of 'diff' with
>> pathspecs and whether this 'pathspec_needs_expanded_index()' could be
>> centralized (in e.g. 'run_diff_index()')? What did you find?

>I hadn't understood the review properly. I just thought you wanted to
>make sure the function was added to diiff-index itself. I have read
>through some of it, but I am still not 100% sure of the behaviour.
>Will run through it more to get more definitive answers

Hello Raghul!

I hope this email finds you well. I recently came across your patch and noticed that you might be facing some difficulties with a specific issue. I've reviewed your patch and thought I'd share a few suggestions that might help you overcome the issue.The code below I've already test it. But there must have many detail I did not handle.

In the builtin/diff.c file, the cmd_diff() function can call either 'run_diff_files()' or 'run_diff_index()' depending on the situation. when you run 'git diff',run_diff_files() is called to find differences between the working directory and the index. when you run 'git diff --cached'. 'run_diff_index()' is called to find difference between the indexand the commit.

Both the "diff-index" and "diff" commands share the "run_diff_index" function. So, we can handling of pathspecs in one place(run_diff_index). Doing this we can simplify the codebase and make it easier to maintain.

1.add test for diff in t1092. We will find the test will fail.

test_expect_success 'git diff with pathspec expands index when necessary' '
	init_repos &&

	echo "new" >>sparse-index/deep/a &&
	git -C sparse-index add deep/a &&

	# pathspec that should expand index
	ensure_expanded diff --cached "*/a" &&

	write_script edit-conflict <<-\EOF &&
	echo test >>"$1"
	EOF

	run_on_all ../edit-contents deep/a &&
	ensure_expanded diff HEAD "*/a"
'

2."run_diff_index" is in 'diff-lib.c'.We can add 'pathspec_needs_expanded_index' in front of 'do_diff_cache()'(process the index before the start of the diff process).

int run_diff_index(struct rev_info *revs, unsigned int option)
{
	......
	......
	if (merge_base) {
		diff_get_merge_base(revs, &oid);
		name = oid_to_hex_r(merge_base_hex, &oid);
	} else {
		oidcpy(&oid, &ent->item->oid);
		name = ent->name;
	}

	if (pathspec_needs_expanded_index(revs->diffopt.repo->index, &revs->diffopt.pathspec))
		ensure_full_index(revs->diffopt.repo->index);

	if (diff_cache(revs, &oid, name, cached))
		exit(128);

	.......
	......
	.......
}

3.Delete 'the pathspec_needs_expanded_index' function you have in your 'builtin/diff-index.c' in last patch.

4.Run the test again, then the test for 'git diff' and your test for 'git diff-index'will all pass!

I hope these suggestions prove helpful to you. If you have any questions or would like to discuss further, please don't hesitate to reach out.

----------------------------------------------------------------------- Best, Shuqi