Re: [PATCH v4 3/8] sha1_file: rename LOOKUP_REPLACE_OBJECT
From: Junio C Hamano <hidden>
Date: 2017-06-21 17:33:26
Jonathan Tan [off-list ref] writes:
The LOOKUP_REPLACE_OBJECT flag controls whether the
lookup_replace_object() function is invoked by
sha1_object_info_extended(), read_sha1_file_extended(), and
lookup_replace_object_extended(), but it is not immediately clear which
functions accept that flag.
Therefore restrict this flag to only sha1_object_info_extended(),
renaming it appropriately to OBJECT_INFO_LOOKUP_REPLACE and adding some
documentation. Update read_sha1_file_extended() to have a boolean
parameter instead, and delete lookup_replace_object_extended().
parse_sha1_header() also passes this flag to
parse_sha1_header_extended() since commit 46f0344 ("sha1_file: support
reading from a loose object of unknown type", 2015-05-03), but that has
had no effect since that commit. Therefore this patch also removes this
flag from that invocation.Yay, code reduction ;-).
-/* object replacement */ -#define LOOKUP_REPLACE_OBJECT 1 -extern void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size, unsigned flag); +extern void *read_sha1_file_extended(const unsigned char *sha1, + enum object_type *type, + unsigned long *size, int lookup_replace);
In general, I'd hesitate to regress the API from "unsigned flag" (that is easier to extend) to "int is_foo" (that will either have to be reverted back to "unsigned flag", or an overlong parameter list "int is_foo, int is_bar, int is_baz, ..."). But let's take this as-is and see how it evolves.
quoted hunk
@@ -3025,7 +3027,7 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep) oi.typep = &type; oi.sizep = sizep; - if (sha1_object_info_extended(sha1, &oi, LOOKUP_REPLACE_OBJECT) < 0) + if (sha1_object_info_extended(sha1, &oi, OBJECT_INFO_LOOKUP_REPLACE)) return -1; return type; }
This changes the error behaviour slightly, doesn't it? Is it guaranteed that sha1_object_info_extended() will never return positive non-zero? Right now it appears that is the case, so this probably is a justifiable simplification of a caller, but given the real consumer of the _extended() API in cat-file.c treats only negative as an error, we should be consistent. I'd prefer to see this change _not_ made as part of this patch. It may be OK to change this place and two callers in cat-file in a follow-up patch though.
quoted hunk
@@ -3107,13 +3109,14 @@ static void *read_object(const unsigned char *sha1, enum object_type *type, void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size, - unsigned flag) + int lookup_replace) { void *data; const struct packed_git *p; const char *path; struct stat st; - const unsigned char *repl = lookup_replace_object_extended(sha1, flag); + const unsigned char *repl = lookup_replace ? lookup_replace_object(sha1) + : sha1; errno = 0; data = read_object(repl, type, size);