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

[Shuqi Liang <cheskaqiqi@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