Re: [PATCH v2 05/30] loose: add a mapping between SHA-1 and SHA-256 for loose objects

[Linus Arver <linusa@google.com>]

"Eric W. Biederman" <ebiederm@gmail.com> writes:

> From: "brian m. carlson" <sandals@crustytoothpaste.net>
>
> As part of the transition plan, we'd like to add a file in the .git
> directory that maps loose objects between SHA-1 and SHA-256.  Let's
> implement the specification in the transition plan and store this data
> on a per-repository basis in struct repository.

Could you explain a bit what the specification is, exactly? That would
save reviewers the trouble of comparing the large chunk of new code here
with the transition plan (which I assume is still
Documentation/technical/hash-function-transition.txt.

Also, are there any slight deviations from the specification for reasons
that may not be obvious to reviewers?

I would prefer if this patch is split up into smaller preparatory
patches, starting with the core essentials and then building it up
piece-by-piece to make it easier to review.

> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
>  Makefile              |   1 +
>  loose.c               | 246 ++++++++++++++++++++++++++++++++++++++++++
>  loose.h               |  22 ++++
>  object-file-convert.c |  14 ++-
>  object-store-ll.h     |   3 +
>  object.c              |   2 +
>  repository.c          |   6 ++
>  7 files changed, 293 insertions(+), 1 deletion(-)
>  create mode 100644 loose.c
>  create mode 100644 loose.h
>
> diff --git a/Makefile b/Makefile
> index f7e824f25cda..3c18664def9a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1053,6 +1053,7 @@ LIB_OBJS += list-objects-filter.o
>  LIB_OBJS += list-objects.o
>  LIB_OBJS += lockfile.o
>  LIB_OBJS += log-tree.o
> +LIB_OBJS += loose.o

The name "loose" appears to be a bit too generic for something with such
a specialized role (_mapping_ of loose objects). Would
"loose-object-map" be a better name?

>  LIB_OBJS += ls-refs.o
>  LIB_OBJS += mailinfo.o
>  LIB_OBJS += mailmap.o
> diff --git a/loose.c b/loose.c
> new file mode 100644
> index 000000000000..6ba73cc84dca
> --- /dev/null
> +++ b/loose.c
> @@ -0,0 +1,246 @@
> +#include "git-compat-util.h"
> +#include "hash.h"
> +#include "path.h"
> +#include "object-store.h"
> +#include "hex.h"
> +#include "wrapper.h"
> +#include "gettext.h"
> +#include "loose.h"
> +#include "lockfile.h"
> +
> +static const char *loose_object_header = "# loose-object-idx\n";

IDK what the "loose-object-idx" is vs the "loose-object-map", but I
guess I need to read more of the code.

But also, I am at my limits here and am unable to review this patch as
is (too big for me to chew at once, sorry).

I'll pause my review of this series here to give Eric B some time to
respond. Thanks.