[PATCH v4 0/8] Improvements to sha1_file

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jun 20, 2017, 01:03 UTC
Message-ID: <cover.1497920092.git.jonathantanmy@google.com>
In-Reply-To: <cover.1497035376.git.jonathantanmy@google.com>

Thanks, Peff and Junio for your comments. Here's an updated version and some replies to comments.

Show 10 quoted lines

> I also found this quite subtle. However, I don't think that
> has_sha1_file() actually freshens. It's a bit confusing because
> has_loose_object() reuses the check_and_freshen() function to do the
> lookup, but it actually sets the "freshen" flag to false.
> 
> That's why in 33d4221c7 (write_sha1_file: freshen existing objects,
> 2014-10-15), which introduced the freshening functions and converted
> has_loose_object(), the actual write_sha1_file() function switched to
> using the freshening functions directly (and obviously sets the freshen
> parameter to true).

Good catch.

Show 11 quoted lines

> I actually think all of that infrastructure could become part of
> Jonathan's consolidated lookup, too. We would just need:
> 
>   1. A QUICK flag to avoid re-reading objects/pack when we don't find
>      anything (which it looks like he already has).
> 
>   2. A FRESHEN flag to update the mtime of any item that we do find.
> 
> I suspect we may also need something like ONLY_LOOSE and ONLY_NONLOCAL
> to meet all the callers (e.g., has_loose_object_nonlocal). Those should
> be easy to implement, I'd think.

For things like FRESHEN, ONLY_LOOSE, and ONLY_NONLOCAL, I was thinking that I would like to restrict these patches to only handle the cases that are agnostic to the type of storage (in preparation for missing blob handling patches).

> I had the same thoughts (both on the name and the "vocabularies"). IMHO
> we should consider allocating the bits from the same set. There's only
> one HAS_SHA1 flag, and it has an exact match in OBJECT_INFO_QUICK.

Agreed - in this patch set, I have also consolidated the relevant flags, including LOOKUP_REPLACE_OBJECT and LOOKUP_UNKNOWN_OBJECT.

In addition, Junio has mentioned the potential confusion in behavior between a NULL and an empty struct passed to sha1_object_info_extended(). In this patch set, I require non-NULL, and have added an optimization that avoids accessing the pack in certain situations, but this optimization requires checking a lot of fields. Let me know what you think.

Jonathan Tan (8):
  sha1_file: teach packed_object_info about typename
  sha1_file: rename LOOKUP_UNKNOWN_OBJECT
  sha1_file: rename LOOKUP_REPLACE_OBJECT
  sha1_file: move delta base cache code up
  sha1_file: refactor read_object
  sha1_file: improve sha1_object_info_extended
  sha1_file: do not access pack if unneeded
  sha1_file: refactor has_sha1_file_with_flags

 builtin/cat-file.c   |   7 +-
 builtin/fetch.c      |  10 +-
 builtin/index-pack.c |   3 +-
 cache.h              |  37 +++--
 sha1_file.c          | 391 ++++++++++++++++++++++++++-------------------------
 streaming.c          |   1 +
 6 files changed, 228 insertions(+), 221 deletions(-)

-- 
2.13.1.611.g7e3b11ae1-goog