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