It only makes sense to call git_hash_update(), etc, on a hash context
that has been initialized but not yet finalized or discarded. This is an
unlikely error to make, but it's easy for us to catch it and complain.
It's especially important because it would quietly "work" for many hash
backends (like sha1dc, which is just manipulating some bytes) but would
cause undefined behavior with others (like OpenSSL, which puts the
context onto the heap). Checking the flag lets us catch problems
consistently on every build.
Note that we can't do the same for git_hash_init(). Even though it would
cause a leak to call it twice (without an intervening final/discard),
the point of the function is that the contents of the struct are
undefined before the call. But calling it twice is an even less likely
error to make, so not covering it is OK.
We leave git_hash_discard() alone, as its idempotent behavior is
convenient for callers. We _could_ try to do something similar for
git_hash_final(), allowing:
git_hash_final(result, &ctx);
git_hash_final(other_result, &ctx);
but it does not make much sense. After the first final() call we have
thrown away the state, so we cannot produce the same output. We could
come up with some sensible output (the null hash, or the empty hash),
but double-calls like this are more likely a bug, so our best bet is to
complain loudly (whereas the current code produces either nonsense
output or undefined behavior, depending on the backend).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
You must always either finalize or discard a hash context to release any
resources, but you must call only one such function. This creates extra
work for some callers, since their cleanup code paths need to know
whether they got there via their happy path (and the finalization
happened) or due to an error (in which case they need to discard).
Let's add an "active" flag that turns a redundant discard into a noop.
That lets you safely do this:
git_hash_init(&ctx, algo);
...
if (some_error)
goto out;
...
git_hash_final(result, &ctx);
out:
git_hash_discard(&ctx);
This should avoid future errors, and will also let us simplify a few
existing callers (in future patches).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Our git_hash_discard() is a bit hacky: it just calls git_hash_final()
into a dummy result buffer, using the side effect that each
implementation's Final() function will also free any resources.
This is probably not too terrible, since generating the final hash is
not that expensive and we'd mostly call discard on unusual or error code
paths. But we can do better by widening the platform API a bit to add an
explicit discard function.
This requires an annoying amount of boilerplate:
- Each algorithm needs a git_$ALGO_discard() wrapper that dereferences
the union'd git_hash_ctx into the type-safe field. So sha1 + sha256
+ sha1-unsafe, plus a BUG() for the unknown algo. And then these all
need to be referenced in the git_hash_algo structs.
- Platforms which don't do anything special to discard now need a
fallback function which does nothing. And we need this for each algo
(sha1, sha256, and sha1-unsafe).
- Platforms which do need to discard must define their discard
functions. This includes sha1/openssl, sha256/openssl, and
sha256/gcrypt (no sha1-unsafe here as it sits atop the sha1/openssl
functions).
- Algo selection needs to point platform_*_Discard to the appropriate
underlying macro, or indicate that the fallback should be used. We
have a similar situation for the Clone function (where a straight
memcpy() of the context struct is not enough for some platforms).
I've tied Discard to the same flag used by Clone here, since they
are basically the same problem: is the hash context a sequence of
bytes, or does it need smart copying/discarding?
It's easy to miss a case here since we don't even compile the
implementations we aren't using. I've tested with each of:
- no flags, which uses our internal sha1/sha256 implementations, both
of which exercise the noop fallback function
- OPENSSL_SHA1_UNSAFE=1, which checks that our unsafe macro
redirections work
- OPENSSL_SHA1=1, though you should not do that in real life!
- OPENSSL_SHA256=1, passes tests with GIT_TEST_DEFAULT_HASH=sha256
- GCRYPT_SHA256=1, which likewise passes
The other implementations do not set the CLONE_HELPER flag, so they
treat the context as bytes and should be fine with the fallback.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The usual life-cycle for a git_hash_ctx is calling git_hash_init(),
adding some data, and then using git_hash_final() to get the output
digest and free any resources.
Sometimes we decide to abort the operation without the final() call
(e.g., due to errors or other reasons). In that case we just abandon the
hash_ctx completely and let it go out of scope. For most hash
implementations this is fine; they were just holding values directly in
the struct.
But some implementations do allocate memory, and in these cases we leak
the memory. Notably OpenSSL >= 3.0 requires us to allocate the digest
context on the heap with EVP_MD_CTX_new().
Let's provide a git_hash_discard() function that can be used in these
code paths to free any resources. For now we'll implement it by just
calling git_hash_final() into a dummy output, relying on its side effect
of freeing the resources. Our view of the underlying hash implementation
is abstracted behind the platform_SHA_* macros, so that's the best we
can do without widening that interface.
It's a little inefficient, but probably not noticeably so in practice,
especially as we'd usually hit this on an error code path. And by
abstracting it in this function, we can later swap it out when the
platform_SHA interface lets us do so.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The function `extend_abbrev_len()` computes the length of common hex
characters between two object IDs. This is done by:
- Making the caller provide the `hex` string for the needle object ID.
- Comparing every hex position of the haystack object ID with
`get_hex_char_from_oid()`.
Turning the binary representation into hex first is roundabout though:
we can simply compare the binary representation and give some special
attention to the final nibble.
Introduce a new function `oid_common_prefix_hexlen()` that does exactly
this and refactor the code to use the new function. This allows us to
drop the `struct min_abbrev_data::hex` field. Furthermore, this function
will be used in by some other callsites in subsequent commits.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We'd like to be able to hash our data in Rust using the same contexts as
in C. However, we need our helper functions to not be inline so they
can be linked into the binary appropriately. In addition, to avoid
managing memory manually and since we don't know the size of the hash
context structure, we want to have simple alloc and free functions we
can use to make sure a context can be easily dynamically created.
Expose the helper functions and create alloc, free, and init functions
we can call.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In C, it's easy for us to look up a hash algorithm structure by its
offset by simply indexing the hash_algos array. However, in Rust, we
sometimes need a pointer to pass to a C function, but we have our own
hash algorithm abstraction.
To get one from the other, let's provide a simple function that looks up
the C structure from the offset and expose it in Rust.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We currently use an int for this value, but we'll define this structure
from Rust in a future commit and we want to ensure that our data types
are exactly identical. To make that possible, use a uint32_t for the
hash algorithm.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `null_oid()` function returns the object ID that only consists of
zeroes. Naturally, this ID also depends on the hash algorithm used, as
the number of zeroes is different between SHA1 and SHA256. Consequently,
the function returns the hash-algorithm-specific null object ID.
This is currently done by depending on `the_hash_algo`, which implicitly
makes us depend on `the_repository`. Refactor the function to instead
pass in the hash algorithm for which we want to retrieve the null object
ID. Adapt callsites accordingly by passing in `the_repository`, thus
bubbling up the dependency on that global variable by one layer.
There are a couple of trivial exceptions for subsystems that already got
rid of `the_repository`. These subsystems instead use the repository
that is available via the calling context:
- "builtin/grep.c"
- "grep.c"
- "refs/debug.c"
There are also two non-trivial exceptions:
- "diff-no-index.c": Here we know that we may not have a repository
initialized at all, so we cannot rely on `the_repository`. Instead,
we adapt `diff_no_index()` to get a `struct git_hash_algo` as
parameter. The only caller is located in "builtin/diff.c", where we
know to call `repo_set_hash_algo()` in case we're running outside of
a Git repository. Consequently, it is fine to continue passing
`the_repository->hash_algo` even in this case.
- "builtin/ls-files.c": There is an in-flight patch series that drops
`USE_THE_REPOSITORY_VARIABLE` in this file, which causes a semantic
conflict because we use `null_oid()` in `show_submodule()`. The
value is passed to `repo_submodule_init()`, which may use the object
ID to resolve a tree-ish in the superproject from which we want to
read the submodule config. As such, the object ID should refer to an
object in the superproject, and consequently we need to use its hash
algorithm.
This means that we could in theory just not bother about this edge
case at all and just use `the_repository` in "diff-no-index.c". But
doing so would feel misdesigned.
Remove the `USE_THE_REPOSITORY_VARIABLE` preprocessor define in
"hash.c".
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
There are a couple of trivial "-Wsign-compare" warnings in "hash.c". Fix
them.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
While we have a "hash.h" header, the actual implementation of the
subsystem is hosted by "object-file.c". This makes it harder than
necessary to find the actual implementation of the hash subsystem and
intermingles the different concerns with one another.
Split out the implementation of hash algorithms into a new, separate
"hash.c" file.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
For the find_exact_renames() function, this allows us to pass the
diff_options structure pointer to the low-level routines. We will use
that to distinguish between the "rename" and "copy" cases.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This is in an effort to make the source index of 'unpack_trees()' as
being const, and thus making the compiler help us verify that we only
access it for reading.
The constification also extended to some of the hashing helpers that get
called indirectly.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We were returning the _address of_ the stored item (or NULL)
instead of the item itself. While this sort of indirection
is useful for insertion (since you can lookup and then
modify), it is unnecessary for read-only lookup. Since the
hash code splits these functions between the internal
lookup_hash_entry function and the public lookup_hash
function, it makes sense for the latter to provide what
users of the library expect.
The result of this was that the index caching returned bogus
results on lookup. We unfortunately didn't catch this
because we were returning a "struct cache_entry **" as a
"void *", and accidentally assigning it to a "struct
cache_entry *".
As it happens, this actually _worked_ most of the time,
because the entries were defined as:
struct cache_entry {
struct cache_entry *next;
...
};
meaning that interpreting a "struct cache_entry **" as a
"struct cache_entry *" would yield an entry where all fields
were totally bogus _except_ for the next pointer, which
pointed to the actual cache entry. When walking the list, we
would look at the bogus "name" field, which was unlikely to
match our lookup, and then proceed to the "real" entry.
The reading of bogus data was silently ignored most of the
time, but could cause a segfault for some data (which seems
to be more common on OS X).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This implements a smarter rename detector for exact renames, which
rather than doing a pairwise comparison (time O(m*n)) will just hash the
files into a hash-table (size O(n+m)), and only do pairwise comparisons
to renames that have the same hash (time O(n+m) except for unrealistic
hash collissions, which we just cull aggressively).
Admittedly the exact rename case is not nearly as interesting as the
generic case, but it's an important case none-the-less. A similar general
approach should work for the generic case too, but even then you do need
to handle the exact renames/copies separately (to avoid the inevitable
added cost factor that comes from the _size_ of the file), so this is
worth doing.
In the expectation that we will indeed do the same hashing trick for the
general rename case, this code uses a generic hash-table implementation
that can be used for other things too. In fact, we might be able to
consolidate some of our existing hash tables with the new generic code
in hash.[ch].
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King