[PATCH 0/5] Updates to replace-object API

[PATCH 0/5] Updates to replace-object API

[Junio C Hamano]

While I was looking at gaining a low-level access to read from the object
store, I found a handful poorly-designed part in the internal API to deal
with object replacement.

Junio C Hamano (5):
  Declare lookup_replace_object() in cache.h, not in commit.h
  t6050: make sure we test not just commit replacement
  read_sha1_file(): get rid of read_sha1_file_repl() madness
  inline lookup_replace_object() calls
  read_sha1_file(): allow selective bypassing of replacement mechanism

 builtin/mktag.c    |    4 ++--
 cache.h            |   18 ++++++++++++++----
 commit.h           |    2 --
 environment.c      |    2 +-
 object.c           |    4 ++--
 replace_object.c   |    4 +++-
 sha1_file.c        |   16 +++++++---------
 t/t6050-replace.sh |   18 ++++++++++++++++--
 8 files changed, 45 insertions(+), 23 deletions(-)

-- 
1.7.5.1.334.gdfd07

[PATCH 1/5] Declare lookup_replace_object() in cache.h, not in commit.h

[Junio C Hamano]

The declaration is misplaced as the replace API is supposed to affect
not just commits, but all types of objects.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h  |    1 +
 commit.h |    2 --
 2 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index 2b34116..e09cf75 100644
--- a/cache.h
+++ b/cache.h
@@ -763,6 +763,7 @@ static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *
 {
 	return read_sha1_file_repl(sha1, type, size, NULL);
 }
+extern const unsigned char *lookup_replace_object(const unsigned char *sha1);
 extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);
 extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);
 extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
diff --git a/commit.h b/commit.h
index b3c3bb7..f251e75 100644
--- a/commit.h
+++ b/commit.h
@@ -145,8 +145,6 @@ struct commit_graft *read_graft_line(char *buf, int len);
 int register_commit_graft(struct commit_graft *, int);
 struct commit_graft *lookup_commit_graft(const unsigned char *sha1);
 
-const unsigned char *lookup_replace_object(const unsigned char *sha1);
-
 extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2, int cleanup);
 extern struct commit_list *get_merge_bases_many(struct commit *one, int n, struct commit **twos, int cleanup);
 extern struct commit_list *get_octopus_merge_bases(struct commit_list *in);
-- 
1.7.5.1.334.gdfd07

[PATCH 2/5] t6050: make sure we test not just commit replacement

[Junio C Hamano]

The replacement mechanism should affect all types of objects not
just commits, so make sure it deals with at least a blob.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t6050-replace.sh |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index ae2194e..5c87f28 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -236,6 +236,20 @@ test_expect_success 'index-pack and replacements' '
 	git index-pack test-*.pack
 '
 
-#
-#
+test_expect_success 'not just commits' '
+	echo replaced >file &&
+	git add file &&
+	REPLACED=$(git rev-parse :file) &&
+	mv file file.replaced &&
+
+	echo original >file &&
+	git add file &&
+	ORIGINAL=$(git rev-parse :file) &&
+	git update-ref refs/replace/$ORIGINAL $REPLACED &&
+	mv file file.original &&
+
+	git checkout file &&
+	test_cmp file.replaced file
+'
+
 test_done
-- 
1.7.5.1.334.gdfd07

[PATCH 3/5] read_sha1_file(): get rid of read_sha1_file_repl() madness

[Junio C Hamano]

Most callers want to silently get a replacement object, and they do not
care what the real name of the replacement object is.  Worse yet, no sane
interface to return the underlying object without replacement is provided.

Remove the function and make only the few callers that want the name of
the replacement object find it themselves.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/mktag.c |    4 ++--
 cache.h         |    6 +-----
 object.c        |    4 ++--
 sha1_file.c     |   12 ++++--------
 4 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/builtin/mktag.c b/builtin/mktag.c
index 324a267..640ab64 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c
@@ -23,8 +23,8 @@ static int verify_object(const unsigned char *sha1, const char *expected_type)
 	int ret = -1;
 	enum object_type type;
 	unsigned long size;
-	const unsigned char *repl;
-	void *buffer = read_sha1_file_repl(sha1, &type, &size, &repl);
+	void *buffer = read_sha1_file(sha1, &type, &size);
+	const unsigned char *repl = lookup_replace_object(sha1);
 
 	if (buffer) {
 		if (type == type_from_string(expected_type))
diff --git a/cache.h b/cache.h
index e09cf75..a9ae100 100644
--- a/cache.h
+++ b/cache.h
@@ -758,11 +758,7 @@ int offset_1st_component(const char *path);
 
 /* Read and unpack a sha1 file into memory, write memory to a sha1 file */
 extern int sha1_object_info(const unsigned char *, unsigned long *);
-extern void *read_sha1_file_repl(const unsigned char *sha1, enum object_type *type, unsigned long *size, const unsigned char **replacement);
-static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size)
-{
-	return read_sha1_file_repl(sha1, type, size, NULL);
-}
+extern void *read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size);
 extern const unsigned char *lookup_replace_object(const unsigned char *sha1);
 extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);
 extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);
diff --git a/object.c b/object.c
index 7e1f2bb..31976b5 100644
--- a/object.c
+++ b/object.c
@@ -188,8 +188,8 @@ struct object *parse_object(const unsigned char *sha1)
 	unsigned long size;
 	enum object_type type;
 	int eaten;
-	const unsigned char *repl;
-	void *buffer = read_sha1_file_repl(sha1, &type, &size, &repl);
+	const unsigned char *repl = lookup_replace_object(sha1);
+	void *buffer = read_sha1_file(sha1, &type, &size);
 
 	if (buffer) {
 		struct object *obj;
diff --git a/sha1_file.c b/sha1_file.c
index 889fe71..5d80feb 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2206,10 +2206,9 @@ static void *read_object(const unsigned char *sha1, enum object_type *type,
  * deal with them should arrange to call read_object() and give error
  * messages themselves.
  */
-void *read_sha1_file_repl(const unsigned char *sha1,
-			  enum object_type *type,
-			  unsigned long *size,
-			  const unsigned char **replacement)
+void *read_sha1_file(const unsigned char *sha1,
+		     enum object_type *type,
+		     unsigned long *size)
 {
 	const unsigned char *repl = lookup_replace_object(sha1);
 	void *data;
@@ -2218,11 +2217,8 @@ void *read_sha1_file_repl(const unsigned char *sha1,
 
 	errno = 0;
 	data = read_object(repl, type, size);
-	if (data) {
-		if (replacement)
-			*replacement = repl;
+	if (data)
 		return data;
-	}
 
 	if (errno && errno != ENOENT)
 		die_errno("failed to read object %s", sha1_to_hex(sha1));
-- 
1.7.5.1.334.gdfd07

[PATCH 4/5] inline lookup_replace_object() calls

[Junio C Hamano]

In a repository without object replacement, lookup_replace_object() should
be a no-op. Check the flag "read_replace_refs" on the side of the caller,
and bypess a function call when we know we are not dealing with replacement.

Also, even when we are set up to replace objects, if we do not find any
replacement defined, flip that flag off to avoid function call overhead
for all the later object accesses.

As this change the semantics of the flag from "do we need read the
replacement definition?" to "do we need to check with the lookup table?"
the flag needs to be renamed later to something saner, e.g. "use_replace",
when the codebase is calmer, but not now.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h          |   12 ++++++++++--
 environment.c    |    2 +-
 replace_object.c |    4 +++-
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index a9ae100..c10a91d 100644
--- a/cache.h
+++ b/cache.h
@@ -756,10 +756,18 @@ char *strip_path_suffix(const char *path, const char *suffix);
 int daemon_avoid_alias(const char *path);
 int offset_1st_component(const char *path);
 
+/* object replacement */
+extern void *read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size);
+extern const unsigned char *do_lookup_replace_object(const unsigned char *sha1);
+static inline const unsigned char *lookup_replace_object(const unsigned char *sha1)
+{
+	if (!read_replace_refs)
+		return sha1;
+	return do_lookup_replace_object(sha1);
+}
+
 /* Read and unpack a sha1 file into memory, write memory to a sha1 file */
 extern int sha1_object_info(const unsigned char *, unsigned long *);
-extern void *read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size);
-extern const unsigned char *lookup_replace_object(const unsigned char *sha1);
 extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);
 extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);
 extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
diff --git a/environment.c b/environment.c
index 40185bc..9182820 100644
--- a/environment.c
+++ b/environment.c
@@ -42,7 +42,7 @@ const char *editor_program;
 const char *askpass_program;
 const char *excludes_file;
 enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
-int read_replace_refs = 1;
+int read_replace_refs = 1; /* NEEDSWORK: rename to use_replace_refs */
 enum eol eol = EOL_UNSET;
 enum safe_crlf safe_crlf = SAFE_CRLF_WARN;
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
diff --git a/replace_object.c b/replace_object.c
index 7c6c754..d0b1548 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -85,12 +85,14 @@ static void prepare_replace_object(void)
 
 	for_each_replace_ref(register_replace_ref, NULL);
 	replace_object_prepared = 1;
+	if (!replace_object_nr)
+		read_replace_refs = 0;
 }
 
 /* We allow "recursive" replacement. Only within reason, though */
 #define MAXREPLACEDEPTH 5
 
-const unsigned char *lookup_replace_object(const unsigned char *sha1)
+const unsigned char *do_lookup_replace_object(const unsigned char *sha1)
 {
 	int pos, depth = MAXREPLACEDEPTH;
 	const unsigned char *cur = sha1;
-- 
1.7.5.1.334.gdfd07

[PATCH 5/5] read_sha1_file(): allow selective bypassing of replacement mechanism

[Junio C Hamano]

The way "object replacement" mechanism was tucked to the read_sha1_file()
interface was suboptimal in a couple of ways:

 - Callers that want it to die with useful diagnosis upon seeing a corrupt
   object does not have a way to say that they do not want any object
   replacement.

 - Callers who do not want it to die but want to handle the errors
   themselves are told to arrange to call read_object(), but the function
   does not use the replacement mechanism, and also it is a file scope
   static function that not many callers can call to begin with.

This adds a read_sha1_file_extended() that takes a set of flags; the
callers of read_sha1_file() passes a flag READ_SHA1_FILE_REPLACE to ask
for object replacement mechanism to kick in.

Later, we could add another flag bit to tell the function to return an
error instead of dying and then remove the misguided "call read_object()
yourself".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h     |    7 ++++++-
 sha1_file.c |   10 ++++++----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index c10a91d..5f1f5c3 100644
--- a/cache.h
+++ b/cache.h
@@ -757,7 +757,12 @@ int daemon_avoid_alias(const char *path);
 int offset_1st_component(const char *path);
 
 /* object replacement */
-extern void *read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size);
+#define READ_SHA1_FILE_REPLACE 1
+extern void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size, unsigned flag);
+static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size)
+{
+	return read_sha1_file_extended(sha1, type, size, READ_SHA1_FILE_REPLACE);
+}
 extern const unsigned char *do_lookup_replace_object(const unsigned char *sha1);
 static inline const unsigned char *lookup_replace_object(const unsigned char *sha1)
 {
diff --git a/sha1_file.c b/sha1_file.c
index 5d80feb..7e6e976 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2206,14 +2206,16 @@ static void *read_object(const unsigned char *sha1, enum object_type *type,
  * deal with them should arrange to call read_object() and give error
  * messages themselves.
  */
-void *read_sha1_file(const unsigned char *sha1,
-		     enum object_type *type,
-		     unsigned long *size)
+void *read_sha1_file_extended(const unsigned char *sha1,
+			      enum object_type *type,
+			      unsigned long *size,
+			      unsigned flag)
 {
-	const unsigned char *repl = lookup_replace_object(sha1);
 	void *data;
 	char *path;
 	const struct packed_git *p;
+	const unsigned char *repl = (flag & READ_SHA1_FILE_REPLACE)
+		? lookup_replace_object(sha1) : sha1;
 
 	errno = 0;
 	data = read_object(repl, type, size);
-- 
1.7.5.1.334.gdfd07