Re: [PATCH v4 4/5] Add reftable library
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- Feb 7, 2020, 00:16 UTC
- Message-ID
- <20200207001612.GF6573@camp.crustytoothpaste.net>
- In-Reply-To
- <02d2ca8b878a67a3334b222725645dd035b4289c.1581029756.git.gitgitgadget@gmail.com>
On 2020-02-06 at 22:55:55, Han-Wen Nienhuys via GitGitGadget wrote:
Show 25 quoted lines
> diff --git a/reftable/README.md b/reftable/README.md > new file mode 100644 > index 0000000000..f527da0380 > --- /dev/null > +++ b/reftable/README.md > @@ -0,0 +1,19 @@ > + > +The source code in this directory comes from https://github.com/google/reftable. > + > +The VERSION file keeps track of the current version of the reftable library. > + > +To update the library, do: > + > + ((cd reftable-repo && git fetch origin && git checkout origin/master ) || > + git clone https://github.com/google/reftable reftable-repo) && \ > + cp reftable-repo/c/*.[ch] reftable/ && \ > + cp reftable-repo/LICENSE reftable/ && > + git --git-dir reftable-repo/.git show --no-patch origin/master \ > + > reftable/VERSION && \ > + echo '/* empty */' > reftable/config.h > + rm reftable/*_test.c reftable/test_framework.* > + git add reftable/*.[ch] > + > +Bugfixes should be accompanied by a test and applied to upstream project at > +https://github.com/google/reftable.
I think this particular statement may be problematic. The upstream project requires a CLA, which is a non-starter for many folks. I don't think we can expect folks who are working on Git to necessarily send patches upstream with that condition.
For example, if I end up needing to patch this series to work with SHA-256, I'm happy for Google to have my changes under the BSD License, but I won't be signing a CLA.
> +#define SHA1_SIZE 20 > +#define SHA256_SIZE 32
I'd rather we didn't hard-code these values here. If you need a size suitable for allocation, we have GIT_MAX_RAWSZ and GIT_MAX_HEXSZ. Those are guaranteed to be the right size for any future hash.
Show 7 quoted lines
> + {
> + struct merged_table m = {
> + .stack = stack,
> + .stack_len = n,
> + .min = first_min,
> + .max = last_max,
> + .hash_size = SHA1_SIZE,This is going to cause problems with SHA-256. We'd want to write this as the_hash_algo->rawsz, since it can change at runtime.
> +static byte zero[SHA256_SIZE] = {};It would be better to refer to null_oid here.
Show 6 quoted lines
> +#ifndef REFTABLE_H > +#define REFTABLE_H > + > +#include "system.h" > + > +typedef uint8_t byte;
I think we typically prefer writing "unsigned char" or "uint8_t" instead of "byte".
> +typedef byte bool;
I suspect this is going to cause a whole host of sadness if somebody ever includes stdbool.h, or if that header ever gets included by an OS-specific header, since bool is a #define constant for the built-in type _Bool.
We typically use int for booleans, but I'm not opposed to using stdbool.h for it on those systems that we know have it (which, in 2020, is probably all of them).
Show 10 quoted lines
> +struct writer *new_writer(int (*writer_func)(void *, byte *, int),
> + void *writer_arg, struct write_options *opts)
> +{
> + struct writer *wp = calloc(sizeof(struct writer), 1);
> + options_set_defaults(opts);
> + if (opts->block_size >= (1 << 24)) {
> + /* TODO - error return? */
> + abort();
> + }
> + wp->hash_size = SHA1_SIZE;Another SHA-256 problem.
Once this series has good test coverage, I'm happy to send a patch that squashes in SHA-256 support if you don't want to implement it yourself and you CC me. If you do want to implement it yourself, you can grab the transition-stage-4 branch from https://github.com/bk2204/git.git, rebase your series on top of it, and run the testsuite with GIT_TEST_DEFAULT_HASH=sha256 to see what breaks.
-- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204