Re: [PATCH 5/6] submodule: improve submodule_has_commits

[Brandon Williams <bmwill@google.com>]

On 04/30, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> >  	oid_array_for_each_unique(commits, check_has_commit, &has_commit);
> > +
> > +	if (has_commit) {
> > +		/*
> > +		 * Even if the submodule is checked out and the commit is
> > +		 * present, make sure it is reachable from a ref.
> > +		 */
> > +		struct child_process cp = CHILD_PROCESS_INIT;
> > +		struct strbuf out = STRBUF_INIT;
> > +
> > +		argv_array_pushl(&cp.args, "rev-list", "-n", "1", NULL);
> > +		oid_array_for_each_unique(commits, append_oid_to_argv, &cp.args);
> > +		argv_array_pushl(&cp.args, "--not", "--all", NULL);
> > +
> > +		prepare_submodule_repo_env(&cp.env_array);
> > +		cp.git_cmd = 1;
> > +		cp.no_stdin = 1;
> > +		cp.dir = path;
> > +
> > +		if (capture_command(&cp, &out, 1024) || out.len)
> > +			has_commit = 0;
> > +
> > +		strbuf_release(&out);
> > +	}
> > +
> >  	return has_commit;
> >  }
> 
> The "check-has-commit" we see in the pre-context is "we contaminated
> our in-core object store by tentatively borrowing from submodule's
> object store---now do we see these commits in our in-core view?"
> Which is a wrong thing to do from two separate point of view.  Even
> though the commit in question may be visible in our contaminated
> view, there is no guarantee that the commit exists in the object
> store of the correct submodule.  And of course the commit may exist
> but may not be anchored by any ref.
> 
> This patch fixes the latter, and if we remove that check-has-commit
> call before it, we can fix the former at the same time.

I noticed this when cleaning up this code but was unsure if I should
drop the "check-has-commit" bit.

> 
> There is value in leaving the check-has-commit code if we anticipate
> that we would very often have to say "no, the submodule does not
> have these commits"---a cheap but wrong check it does can be used as
> an optimization.  If we do not have the commit object anywhere,
> there is no chance we have it in the object store of the correct
> submodule and have it reachable from a ref, so we can fail without
> spawning rev-list which is expensive.

Mostly because it gave the code a way to fail quickly, of course I'm
making the assumption that polluting the object store than then checking
it is quicker than launching a child process (though I guess most things
are cheaper than launching a process ;)

-- 
Brandon Williams