Re: [PATCH] sha1-file: remove OBJECT_INFO_SKIP_CACHED

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Jonathan Tan wrote:

> In a partial clone, if a user provides the hash of the empty tree ("git
> mktree </dev/null" - for SHA-1, this is 4b825d...) to a command which
> requires that that object be parsed, for example:
>
>   git diff-tree 4b825d <a non-empty tree>
>
> then Git will lazily fetch the empty tree. This fetch would merely be
> inconvenient if the promisor remote could supply that tree, but at
> $DAYJOB we discovered that some repositories do not (e.g. [1]).

Ooh, I think there's something subtle hiding in this paragraph.

When I first read it, I thought you meant that the repositories are
not self-contained --- that they contain references to the empty tree
but do not fulfill "want"s for them.  I don't believe that's what you
mean, though.

My second reading is the repository genuinely doesn't contain the
empty tree but different Git server implementations handle that
differently.  I tried to reproduce this with

	empty_tree=$(git mktree </dev/null)
	git init --bare x
	git clone --filter=blob:none file://$(pwd)/x y
	cd y
	echo hi >README
	git add README
	git commit -m 'nonempty tree'
	GIT_TRACE=1 git diff-tree "$empty_tree" HEAD

and indeed, it looks like Git serves the empty tree even from
repositories that don't contain it.  By comparison, JGit does not do
the same trick.  So we don't need to refer to a specific repository in
the wild to reproduce this.

All that said, having to fetch this object in the first place is
unexpected.  The question of the promisor remote serving it is only
relevant from the point of view of "why didn't we discover this
sooner?"

> There are 2 functions: repo_has_object_file() which does not consult
> find_cached_object() (which, among other things, knows about the empty
> tree); and repo_read_object_file() which does.

Hm, where does this dichotomy come from?  E.g. is the latter a
lower-level function used by the former?

>                                                This issue occurs
> because,

nit: on first reading I had trouble figuring out what "this issue"
refers to here.

[...]
>          as an optimization to avoid reading blobs into memory,
> parse_object() calls repo_has_object_file() before
> repo_read_object_file(). In the case of a regular repository (that is,
> not a partial clone), repo_has_object_file() will return false for the
> empty tree (thus bypassing the optimization) and repo_read_object_file()
> will nevertheless succeed, thus things coincidentally work.

This might be easier to understand if phrased in terms of the
intention behind the code instead of the specific call stacks used.
See f06ab027 for an example of this kind of thing.  For example:

  Applications within and outside of Git benefit from being able to
  assume that every repository contains the empty tree as an object
  (see, for example, commit 9abd46a347 "Make sure the empty tree
  exists when needed in merge-recursive", 2006-12-07).  To this end,
  since 346245a1bb (hard-code the empty tree object, 2008-02-13), Git
  has made the empty tree available in all repositories via
  find_cached_object, which all object access paths can use.

  Object existence checks (has_object_file), however, do not use
  find_cached_object.  <describe reason here>

>                                                             But in a
> partial clone, repo_has_object_file() triggers a lazy fetch of the
> missing empty tree.

  This particularly affects partial clones: has_object_file does not
  only report false in this case but contacts the promisor remote in
  order to obtain that answer.  The cost of these repeated negative
  lookups can add up.

  For example, in an optimization introduced in 090ea12671
  ("parse_object: avoid putting whole blob in core", 2012-03-07),
  object parsing uses has_object_file before read_object_file to check
  for a fast-path, so this negative lookup is triggered whenever we
  try to parse the absent empty tree.

When I state it this way, it's not obvious why this particular caller
of has_object_file is more relevant than others.  Did I miss some
subtlety?

[...]
>                                            This fetch would merely be
> inconvenient if the promisor remote could supply that tree, but at
> $DAYJOB we discovered that some repositories do not (e.g. [1]).

  If the promisor remote is running standard Git then it *does* have a
  copy of the empty tree, via the cached object itself.  This
  guarantee is not a documented part of the protocol, however, and
  other Git servers do not implement it.

> The best solution to the problem introduced at the start of this commit
> message seems to be to eliminate this dichotomy.

Indeed.  Can we justify the change even more simply in those terms?

  Object existence checks (has_object_file), however, do not use
  find_cached_object.  <describe reason here>

  This makes the API unnecessarily difficult to reason about.
  Instead, let's consistently view the empty tree as a genuine part of
  the repository, even in has_object_file.  As a side effect, this
  allows us to simplify the common 'has_object_file ||
  find_cached_object' pattern to a more simple existence check.

[...]
>                               A cost is that repo_has_object_file() will
> now need to oideq upon each invocation, but that is trivial compared to
> the filesystem lookup or the pack index search required anyway. (And if
> find_cached_object() needs to do more because of previous invocations to
> pretend_object_file(), all the more reason to be consistent in whether
> we present cached objects.) Therefore, remove OBJECT_INFO_SKIP_CACHED.

Thanks for discussing the possible costs, and I agree that they're
trivial relative to the I/O that these functions already incur.

[...]
>  object-store.h |  2 --
>  sha1-file.c    | 38 ++++++++++++++++++--------------------
>  2 files changed, 18 insertions(+), 22 deletions(-)

As hinted above, we should be able to simplify away has_sha1_file ||
find_cached_object checks in this change.

Thanks and hpoe that helps,
Jonathan