On Thu, Jan 26, 2023 at 1:16 AM Glen Choo <chooglen@xxxxxxxxxx> wrote:
>
>
> Calvin Wan <calvinwan@xxxxxxxxxx> writes:
>
> > @@ -226,6 +242,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
> > newmode = ce->ce_mode;
> > } else {
> > struct stat st;
> > + unsigned ignore_untracked = 0;
> > + int defer_submodule_status = !!revs->repo;
>
> What is the reasoning behind this condition? I would expect revs->repo
> to always be set, and we would always end up deferring.
Ah looks like a vestigial sanity check. You're correct that we would
always be deferring anyways.
>
> > newmode = ce_mode_from_stat(ce, st.st_mode);
> > + if (defer_submodule_status) {
> > + struct submodule_status_util tmp = {
> > + .changed = changed,
> > + .dirty_submodule = 0,
> > + .ignore_untracked = ignore_untracked,
> > + .newmode = newmode,
> > + .ce = ce,
> > + .path = ce->name,
> > + };
> > + struct string_list_item *item;
> > +
> > + item = string_list_append(&submodules, ce->name);
> > + item->util = xmalloc(sizeof(tmp));
> > + memcpy(item->util, &tmp, sizeof(tmp));
>
> (Not a C expert) Since we don't return the string list, I wonder if we
> can avoid the memcpy() by using &tmp like so:
>
> struct string_list_item *item;
> item = string_list_append(&submodules, ce->name);
> item->util = &tmp;
>
> And then when we call string_list_clear(), we wouldn't need to free the
> util since we exit the stack frame.
Unfortunately this doesn't work because tmp is deallocated off the stack
after changing scope.
> > +test_expect_success 'diff in superproject with submodules respects parallel settings' '
> > + test_when_finished "rm -f trace.out" &&
> > + (
> > + GIT_TRACE=$(pwd)/trace.out git diff &&
> > + grep "1 tasks" trace.out &&
> > + >trace.out &&
> > +
> > + git config submodule.diffJobs 8 &&
> > + GIT_TRACE=$(pwd)/trace.out git diff &&
> > + grep "8 tasks" trace.out &&
> > + >trace.out &&
> > +
> > + GIT_TRACE=$(pwd)/trace.out git -c submodule.diffJobs=0 diff &&
> > + grep "preparing to run up to [0-9]* tasks" trace.out &&
> > + ! grep "up to 0 tasks" trace.out &&
> > + >trace.out
> > + )
> > +'
> > +
>
> Could we get tests to check that the output of git diff isn't changed by
> setting parallelism? This might not be feasible for submodule.diffJobs >
> 1 due to raciness, but it would be good to see for submodule.diffJobs =
> 1 at least.
ack.
>
> > test_expect_success 'git diff --raw HEAD' '
> > hexsz=$(test_oid hexsz) &&
> > git diff --raw --abbrev=$hexsz HEAD >actual &&
> > diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
> > index d050091345..52a82b703f 100755
> > --- a/t/t7506-status-submodule.sh
> > +++ b/t/t7506-status-submodule.sh
> > @@ -412,4 +412,23 @@ test_expect_success 'status with added file in nested submodule (short)' '
> > EOF
> > '
> >
> > +test_expect_success 'status in superproject with submodules respects parallel settings' '
> > + test_when_finished "rm -f trace.out" &&
> > + (
> > + GIT_TRACE=$(pwd)/trace.out git status &&
> > + grep "1 tasks" trace.out &&
> > + >trace.out &&
> > +
> > + git config submodule.diffJobs 8 &&
> > + GIT_TRACE=$(pwd)/trace.out git status &&
> > + grep "8 tasks" trace.out &&
> > + >trace.out &&
> > +
> > + GIT_TRACE=$(pwd)/trace.out git -c submodule.diffJobs=0 status &&
> > + grep "preparing to run up to [0-9]* tasks" trace.out &&
> > + ! grep "up to 0 tasks" trace.out &&
> > + >trace.out
> > + )
> > +'
> > +
>
> Ditto for "status".
ack.
