threads / patch / 35518

v3, 10 partsteach replace objects to sha1_object_info_extended()

Subject: [PATCH v3 00/10] teach replace objects to sha1_object_info_extended()

## tl;dr

20 messages between Dec 11, 2013 and Dec 28, 2013. Diffs are folded; open one to read it.

replies: 19people: 3as markdown or json

Christian Couder· Dec 11, 2013, 07:46 UTC · lore

Here is version 3 of a patch series to improve the way sha1_object_info_extended() behaves when it is passed a replaced object. The idea is to add a flags argument to it in the same way as what has been done to read_sha1_file().

This patch series was inspired by a sub thread in this discussion:

http://thread.gmane.org/gmane.comp.version-control.git/238118

The only change compared to version 2 is that a few more tests have been added to patch 8/10, as suggested by Eric Sunchine.

As in the previous version, patches 7/10, 8/10, 9/10 and 10/10 add a new --format option to list replace refs. 'short' (which is the default), 'medium' and 'full' formats are supported. This could be considered another patch series, but it is also related, because it uses sha1_object_info() and it fixes its use in builtin/replace.c by unsetting the global variable read_replace_refs in cmd_replace().

Christian Couder (10):
  Rename READ_SHA1_FILE_REPLACE flag to LOOKUP_REPLACE_OBJECT
  replace_object: don't check read_replace_refs twice
  Introduce lookup_replace_object_extended() to pass flags
  Add an "unsigned flags" parameter to sha1_object_info_extended()
  t6050: show that git cat-file --batch fails with replace objects
  sha1_file: perform object replacement in sha1_object_info_extended()
  builtin/replace: teach listing using short, medium or full formats
  t6050: add tests for listing with --format
  builtin/replace: unset read_replace_refs
  Documentation/git-replace: describe --format option
 Documentation/git-replace.txt | 19 ++++++++++++-
 builtin/cat-file.c            |  2 +-
 builtin/replace.c             | 63 ++++++++++++++++++++++++++++++++++++++-----
 cache.h                       | 12 ++++++---
 replace_object.c              |  3 ---
 sha1_file.c                   | 20 +++++++-------
 streaming.c                   |  2 +-
 t/t6050-replace.sh            | 42 +++++++++++++++++++++++++++++
 8 files changed, 137 insertions(+), 26 deletions(-)
-- 
1.8.5.1.102.g090758b
Christian Couder· Dec 11, 2013, 07:46 UTC · re: Christian Couder · lore

[PATCH v3 01/10] Rename READ_SHA1_FILE_REPLACE flag to LOOKUP_REPLACE_OBJECT

The READ_SHA1_FILE_REPLACE flag is more related to using the lookup_replace_object() function rather than the read_sha1_file() function.

We also need such a flag to be used with sha1_object_info() instead of read_sha1_file().

The name LOOKUP_REPLACE_OBJECT is therefore better for this flag.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 cache.h     | 4 ++--
 sha1_file.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
Show changes to 2 files +3 −3

cache.h, sha1_file.c

diff --git a/cache.h b/cache.h
index ce377e1..873a6b5 100644
--- a/cache.h
+++ b/cache.h
@@ -760,11 +760,11 @@ int daemon_avoid_alias(const char *path);
 int offset_1st_component(const char *path);
 
 /* object replacement */
-#define READ_SHA1_FILE_REPLACE 1
+#define LOOKUP_REPLACE_OBJECT 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);
+	return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
 }
 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 daacc0c..76e9f32 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2591,7 +2591,7 @@ void *read_sha1_file_extended(const unsigned char *sha1,
 	void *data;
 	char *path;
 	const struct packed_git *p;
-	const unsigned char *repl = (flag & READ_SHA1_FILE_REPLACE)
+	const unsigned char *repl = (flag & LOOKUP_REPLACE_OBJECT)
 		? lookup_replace_object(sha1) : sha1;
 
 	errno = 0;
-- 
1.8.5.1.102.g090758b
Christian Couder· Dec 11, 2013, 07:46 UTC · re: Christian Couder · lore

[PATCH v3 02/10] replace_object: don't check read_replace_refs twice

Since e1111cef (inline lookup_replace_object() calls, May 15 2011) the read_replace_refs global variable is checked twice, once in lookup_replace_object() and once again in do_lookup_replace_object().

As do_lookup_replace_object() is called only from lookup_replace_object(), we can remove the check in do_lookup_replace_object().

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 replace_object.c | 3 ---
 1 file changed, 3 deletions(-)
Show changes to replace_object.c +0 −3
diff --git a/replace_object.c b/replace_object.c
index d0b1548..cdcaf8c 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -97,9 +97,6 @@ const unsigned char *do_lookup_replace_object(const unsigned char *sha1)
 	int pos, depth = MAXREPLACEDEPTH;
 	const unsigned char *cur = sha1;
 
-	if (!read_replace_refs)
-		return sha1;
-
 	prepare_replace_object();
 
 	/* Try to recursively replace the object */
-- 
1.8.5.1.102.g090758b
Christian Couder· Dec 11, 2013, 07:46 UTC · re: Christian Couder · lore

[PATCH v3 03/10] Introduce lookup_replace_object_extended() to pass flags

Currently, there is only one caller to lookup_replace_object() that can benefit from passing it some flags, but we expect that there could be more.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 cache.h     | 6 ++++++
 sha1_file.c | 3 +--
 2 files changed, 7 insertions(+), 2 deletions(-)
Show changes to 2 files +7 −2

cache.h, sha1_file.c

diff --git a/cache.h b/cache.h
index 873a6b5..563f85f 100644
--- a/cache.h
+++ b/cache.h
@@ -773,6 +773,12 @@ static inline const unsigned char *lookup_replace_object(const unsigned char *sh
 		return sha1;
 	return do_lookup_replace_object(sha1);
 }
+static inline const unsigned char *lookup_replace_object_extended(const unsigned char *sha1, unsigned flag)
+{
+	if (! (flag & LOOKUP_REPLACE_OBJECT))
+		return sha1;
+	return 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 *);
diff --git a/sha1_file.c b/sha1_file.c
index 76e9f32..4fb2f17 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2591,8 +2591,7 @@ void *read_sha1_file_extended(const unsigned char *sha1,
 	void *data;
 	char *path;
 	const struct packed_git *p;
-	const unsigned char *repl = (flag & LOOKUP_REPLACE_OBJECT)
-		? lookup_replace_object(sha1) : sha1;
+	const unsigned char *repl = lookup_replace_object_extended(sha1, flag);
 
 	errno = 0;
 	data = read_object(repl, type, size);
-- 
1.8.5.1.102.g090758b
Christian Couder· Dec 11, 2013, 07:46 UTC · re: Christian Couder · lore

[PATCH v3 04/10] Add an "unsigned flags" parameter to sha1_object_info_extended()

This parameter is not used yet, but it will be used to tell sha1_object_info_extended() if it should perform object replacement or not.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/cat-file.c | 2 +-
 cache.h            | 2 +-
 sha1_file.c        | 6 +++---
 streaming.c        | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)
Show changes to 4 files +6 −6

builtin/cat-file.c, cache.h, sha1_file.c, streaming.c

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b2ca775..b15c064 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -238,7 +238,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,
 		return 0;
 	}
 
-	if (sha1_object_info_extended(data->sha1, &data->info) < 0) {
+	if (sha1_object_info_extended(data->sha1, &data->info, LOOKUP_REPLACE_OBJECT) < 0) {
 		printf("%s missing\n", obj_name);
 		fflush(stdout);
 		return 0;
diff --git a/cache.h b/cache.h
index 563f85f..701e42c 100644
--- a/cache.h
+++ b/cache.h
@@ -1104,7 +1104,7 @@ struct object_info {
 		} packed;
 	} u;
 };
-extern int sha1_object_info_extended(const unsigned char *, struct object_info *);
+extern int sha1_object_info_extended(const unsigned char *, struct object_info *, unsigned flags);
 
 /* Dumb servers support */
 extern int update_server_info(int);
diff --git a/sha1_file.c b/sha1_file.c
index 4fb2f17..482037e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2443,7 +2443,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,
 	return 0;
 }
 
-int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)
+int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi, unsigned flags)
 {
 	struct cached_object *co;
 	struct pack_entry e;
@@ -2477,7 +2477,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)
 	rtype = packed_object_info(e.p, e.offset, oi);
 	if (rtype < 0) {
 		mark_bad_packed_object(e.p, sha1);
-		return sha1_object_info_extended(sha1, oi);
+		return sha1_object_info_extended(sha1, oi, 0);
 	} else if (in_delta_base_cache(e.p, e.offset)) {
 		oi->whence = OI_DBCACHED;
 	} else {
@@ -2499,7 +2499,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) < 0)
+	if (sha1_object_info_extended(sha1, &oi, LOOKUP_REPLACE_OBJECT) < 0)
 		return -1;
 	return type;
 }
diff --git a/streaming.c b/streaming.c
index debe904..9659f18 100644
--- a/streaming.c
+++ b/streaming.c
@@ -113,7 +113,7 @@ static enum input_source istream_source(const unsigned char *sha1,
 
 	oi->typep = type;
 	oi->sizep = &size;
-	status = sha1_object_info_extended(sha1, oi);
+	status = sha1_object_info_extended(sha1, oi, 0);
 	if (status < 0)
 		return stream_error;
 
-- 
1.8.5.1.102.g090758b
Christian Couder· Dec 11, 2013, 07:46 UTC · re: Christian Couder · lore

[PATCH v3 05/10] t6050: show that git cat-file --batch fails with replace objects

When --batch is passed to git cat-file, the sha1_object_info_extended() function is used to get information about the objects passed to git cat-file.

Unfortunately sha1_object_info_extended() doesn't take care of object replacement properly, so it will often fail with a message like this:

$ echo a3fb2e1845a1aaf129b7975048973414dc172173 | git cat-file --batch a3fb2e1845a1aaf129b7975048973414dc172173 commit 231 fatal: object a3fb2e1845a1aaf129b7975048973414dc172173 change size!?

The goal of this patch is to show this breakage.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 5 +++++
 1 file changed, 5 insertions(+)
Show changes to t/t6050-replace.sh +5 −0
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 7d47984..b90dbdc 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -276,6 +276,11 @@ test_expect_success '-f option bypasses the type check' '
 	git replace -f HEAD^ $BLOB
 '
 
+test_expect_failure 'git cat-file --batch works on replace objects' '
+	git replace | grep $PARA3 &&
+	echo $PARA3 | git cat-file --batch
+'
+
 test_expect_success 'replace ref cleanup' '
 	test -n "$(git replace)" &&
 	git replace -d $(git replace) &&
-- 
1.8.5.1.102.g090758b
Christian Couder· Dec 11, 2013, 07:46 UTC · re: Christian Couder · lore

[PATCH v3 06/10] sha1_file: perform object replacement in sha1_object_info_extended()

sha1_object_info_extended() should perform object replacement if it is needed.

The simplest way to do that is to make it call lookup_replace_object_extended().

And now its "unsigned flags" parameter is used as it is passed to lookup_replace_object_extended().

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 sha1_file.c        | 13 +++++++------
 t/t6050-replace.sh |  2 +-
 2 files changed, 8 insertions(+), 7 deletions(-)
Show changes to 2 files +8 −7

sha1_file.c, t/t6050-replace.sh

diff --git a/sha1_file.c b/sha1_file.c
index 482037e..ee224e4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2448,8 +2448,9 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
 	struct cached_object *co;
 	struct pack_entry e;
 	int rtype;
+	const unsigned char *real = lookup_replace_object_extended(sha1, flags);
 
-	co = find_cached_object(sha1);
+	co = find_cached_object(real);
 	if (co) {
 		if (oi->typep)
 			*(oi->typep) = co->type;
@@ -2461,23 +2462,23 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
 		return 0;
 	}
 
-	if (!find_pack_entry(sha1, &e)) {
+	if (!find_pack_entry(real, &e)) {
 		/* Most likely it's a loose object. */
-		if (!sha1_loose_object_info(sha1, oi)) {
+		if (!sha1_loose_object_info(real, oi)) {
 			oi->whence = OI_LOOSE;
 			return 0;
 		}
 
 		/* Not a loose object; someone else may have just packed it. */
 		reprepare_packed_git();
-		if (!find_pack_entry(sha1, &e))
+		if (!find_pack_entry(real, &e))
 			return -1;
 	}
 
 	rtype = packed_object_info(e.p, e.offset, oi);
 	if (rtype < 0) {
-		mark_bad_packed_object(e.p, sha1);
-		return sha1_object_info_extended(sha1, oi, 0);
+		mark_bad_packed_object(e.p, real);
+		return sha1_object_info_extended(real, oi, 0);
 	} else if (in_delta_base_cache(e.p, e.offset)) {
 		oi->whence = OI_DBCACHED;
 	} else {
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index b90dbdc..bb785ec 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -276,7 +276,7 @@ test_expect_success '-f option bypasses the type check' '
 	git replace -f HEAD^ $BLOB
 '
 
-test_expect_failure 'git cat-file --batch works on replace objects' '
+test_expect_success 'git cat-file --batch works on replace objects' '
 	git replace | grep $PARA3 &&
 	echo $PARA3 | git cat-file --batch
 '
-- 
1.8.5.1.102.g090758b
Christian Couder· Dec 11, 2013, 07:46 UTC · re: Christian Couder · lore

[PATCH v3 07/10] builtin/replace: teach listing using short, medium or full formats

By default when listing replace refs, only the sha1 of the replaced objects are shown.

In many cases, it is much nicer to be able to list all the sha1 of the replaced objects along with the sha1 of the replacment objects.

And in other cases it might be interesting to also show the types of the replaced and replacement objects.

This patch introduce a new --format=<fmt> option where <fmt> can be any of the following:

	'short': this is the same as when no --format
		option is used, that is only the sha1 of
		the replaced objects are shown
	'medium': this also lists the sha1 of the
		replacement objects
	'full': this shows the sha1 and the type of both
		the replaced and the replacement objects
Some documentation and some tests will follow.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/replace.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 7 deletions(-)
Show changes to builtin/replace.c +54 −7
diff --git a/builtin/replace.c b/builtin/replace.c
index b1bd3ef..9f3619a 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -16,27 +16,65 @@
 static const char * const git_replace_usage[] = {
 	N_("git replace [-f] <object> <replacement>"),
 	N_("git replace -d <object>..."),
-	N_("git replace -l [<pattern>]"),
+	N_("git replace [--format=<format>] [-l [<pattern>]]"),
 	NULL
 };
 
+enum repl_fmt { SHORT, MEDIUM, FULL };
+
+struct show_data {
+	const char *pattern;
+	enum repl_fmt fmt;
+};
+
 static int show_reference(const char *refname, const unsigned char *sha1,
 			  int flag, void *cb_data)
 {
-	const char *pattern = cb_data;
+	struct show_data *data = cb_data;
+
+	if (!fnmatch(data->pattern, refname, 0)) {
+		if (data->fmt == SHORT)
+			printf("%s\n", refname);
+		else if (data->fmt == MEDIUM)
+			printf("%s -> %s\n", refname, sha1_to_hex(sha1));
+		else { /* data->fmt == FULL */
+			unsigned char object[20];
+			enum object_type obj_type, repl_type;
 
-	if (!fnmatch(pattern, refname, 0))
-		printf("%s\n", refname);
+			if (get_sha1(refname, object))
+				return error("Failed to resolve '%s' as a valid ref.", refname);
+
+			obj_type = sha1_object_info(object, NULL);
+			repl_type = sha1_object_info(sha1, NULL);
+
+			printf("%s (%s) -> %s (%s)\n", refname, typename(obj_type),
+			       sha1_to_hex(sha1), typename(repl_type));
+		}
+	}
 
 	return 0;
 }
 
-static int list_replace_refs(const char *pattern)
+static int list_replace_refs(const char *pattern, const char *format)
 {
+	struct show_data data;
+
 	if (pattern == NULL)
 		pattern = "*";
+	data.pattern = pattern;
+
+	if (format == NULL || *format == '\0' || !strcmp(format, "short"))
+		data.fmt = SHORT;
+	else if (!strcmp(format, "medium"))
+		data.fmt = MEDIUM;
+	else if (!strcmp(format, "full"))
+		data.fmt = FULL;
+	else
+		die("invalid replace format '%s'\n"
+		    "valid formats are 'short', 'medium' and 'full'\n",
+		    format);
 
-	for_each_replace_ref(show_reference, (void *) pattern);
+	for_each_replace_ref(show_reference, (void *) &data);
 
 	return 0;
 }
@@ -127,10 +165,12 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 int cmd_replace(int argc, const char **argv, const char *prefix)
 {
 	int list = 0, delete = 0, force = 0;
+	const char *format = NULL;
 	struct option options[] = {
 		OPT_BOOL('l', "list", &list, N_("list replace refs")),
 		OPT_BOOL('d', "delete", &delete, N_("delete replace refs")),
 		OPT_BOOL('f', "force", &force, N_("replace the ref if it exists")),
+		OPT_STRING(0, "format", &format, N_("format"), N_("use this format")),
 		OPT_END()
 	};
 
@@ -140,6 +180,10 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		usage_msg_opt("-l and -d cannot be used together",
 			      git_replace_usage, options);
 
+	if (format && delete)
+		usage_msg_opt("--format and -d cannot be used together",
+			      git_replace_usage, options);
+
 	if (force && (list || delete))
 		usage_msg_opt("-f cannot be used with -d or -l",
 			      git_replace_usage, options);
@@ -157,6 +201,9 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		if (argc != 2)
 			usage_msg_opt("bad number of arguments",
 				      git_replace_usage, options);
+		if (format)
+			usage_msg_opt("--format cannot be used when not listing",
+				      git_replace_usage, options);
 		return replace_object(argv[0], argv[1], force);
 	}
 
@@ -168,5 +215,5 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		usage_msg_opt("-f needs some arguments",
 			      git_replace_usage, options);
 
-	return list_replace_refs(argv[0]);
+	return list_replace_refs(argv[0], format);
 }
-- 
1.8.5.1.102.g090758b
Karsten Blees· Dec 18, 2013, 12:37 UTC · re: Christian Couder · lore

Re: [PATCH v3 07/10] builtin/replace: teach listing using short, medium or full formats

Am 11.12.2013 08:46, schrieb Christian Couder:
> +enum repl_fmt { SHORT, MEDIUM, FULL };
SHORT is predefined on Windows, could you choose another name?
MinGW:

builtin/replace.c:23: error: 'SHORT' redeclared as different kind of symbol c:\git\msysgit\mingw\bin\../lib/gcc/mingw32/4.4.0/../../../../include/winnt.h:78: note: previous declaration of 'SHORT' was here make: *** [builtin/replace.o] Error 1

MSVC:
    CC builtin/replace.o
replace.c
builtin/replace.c(23) : error C2365: "SHORT": Erneute Definition; vorherige Definition war "Typedef".
        C:\Program Files\Microsoft SDKs\Windows\v7.1\include\winnt.h(332): Siehe Deklaration von 'SHORT'
builtin/replace.c(23) : error C2086: 'repl_fmt SHORT': Neudefinition
        builtin/replace.c(23): Siehe Deklaration von 'SHORT'
builtin/replace.c(36) : error C2275: 'SHORT': Ungültige Verwendung dieses Typs als Ausdruck
        C:\Program Files\Microsoft SDKs\Windows\v7.1\include\winnt.h(332): Siehe Deklaration von 'SHORT'
builtin/replace.c(67) : error C2275: 'SHORT': Ungültige Verwendung dieses Typs als Ausdruck
        C:\Program Files\Microsoft SDKs\Windows\v7.1\include\winnt.h(332): Siehe Deklaration von 'SHORT'
builtin/replace.c(173) : warning C4090: 'Initialisierung': Unterschiedliche 'const'-Bezeichner
make: *** [builtin/replace.o] Error 1
Christian Couder· Dec 18, 2013, 16:49 UTC · re: Karsten Blees · lore

Re: [PATCH v3 07/10] builtin/replace: teach listing using short, medium or full formats

On Wed, Dec 18, 2013 at 1:37 PM, Karsten Blees <karsten.blees@gmail.com> wrote:
> Am 11.12.2013 08:46, schrieb Christian Couder:
>> +enum repl_fmt { SHORT, MEDIUM, FULL };
>
> SHORT is predefined on Windows, could you choose another name?
Ok, I will change to:
enum repl_fmt { SHORT_FMT, MEDIUM_FMT, FULL_FMT };

Thanks, Christian.

Junio C Hamano· Dec 18, 2013, 17:37 UTC · re: Christian Couder · lore

Re: [PATCH v3 07/10] builtin/replace: teach listing using short, medium or full formats

Christian Couder <christian.couder@gmail.com> writes:
Show 9 quoted lines
> On Wed, Dec 18, 2013 at 1:37 PM, Karsten Blees <karsten.blees@gmail.com> wrote:
>> Am 11.12.2013 08:46, schrieb Christian Couder:
>>> +enum repl_fmt { SHORT, MEDIUM, FULL };
>>
>> SHORT is predefined on Windows, could you choose another name?
>
> Ok, I will change to:
>
> enum repl_fmt { SHORT_FMT, MEDIUM_FMT, FULL_FMT };

What are these for in the first place? Your "SHORT" conflicting with something totally unrelated is a sign that you should be naming them in a way that is more specific to your use. SHORT_FMT is still not specific enough to tell what they are for. With SHOW_REPLACE_ prefix, perhaps? Also perhaps give characterization better than their output lengths?

My quick read of show_reference() tells me that they are "name only", "name and value", and something else that does not seem very useful unless you are debugging.

Christian Couder· Dec 19, 2013, 16:36 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 07/10] builtin/replace: teach listing using short, medium or full formats

On Wed, Dec 18, 2013 at 6:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> Christian Couder <christian.couder@gmail.com> writes:
>
>> On Wed, Dec 18, 2013 at 1:37 PM, Karsten Blees <karsten.blees@gmail.com> wrote:
>>> Am 11.12.2013 08:46, schrieb Christian Couder:
>>>> +enum repl_fmt { SHORT, MEDIUM, FULL };
>>>
>>> SHORT is predefined on Windows, could you choose another name?
>>
>> Ok, I will change to:
>>
>> enum repl_fmt { SHORT_FMT, MEDIUM_FMT, FULL_FMT };
>
> What are these for in the first place?  Your "SHORT" conflicting
> with something totally unrelated is a sign that you should be naming
> them in a way that is more specific to your use.  SHORT_FMT is still
> not specific enough to tell what they are for.  With SHOW_REPLACE_
> prefix, perhaps?  Also perhaps give characterization better than
> their output lengths?
I am ok with SHORT_REPLACE_FMT and so on.
> My quick read of show_reference() tells me that they are "name
> only", "name and value", and something else that does not seem
> very useful unless you are debugging.
Yeah, SHORT_REPLACE_FMT is "name only" which means something like:

$ git replace --format=short 14ac020163ea60a9d683ce68e36c946f31ecc856 4b48deed3a433909bfd6b6ab3d4b91348b6af464 5c37393794868bc8e708cccd7c9d9aaa7a5e53cb a3fb2e1845a1aaf129b7975048973414dc172173 e25dc7954f0832d962347872884aab2dffb426c5

MEDIUM_REPLACE_FMT is "name and value", like this:

$ git replace --format=medium 14ac020163ea60a9d683ce68e36c946f31ecc856 -> 4b48deed3a433909bfd6b6ab3d4b91348b6af464 4b48deed3a433909bfd6b6ab3d4b91348b6af464 -> feae347d8510cfba5eb8c8ac80056777b07c2528 5c37393794868bc8e708cccd7c9d9aaa7a5e53cb -> 14ac020163ea60a9d683ce68e36c946f31ecc856 a3fb2e1845a1aaf129b7975048973414dc172173 -> 9af2a15082b7c95982473e32f3376558c149a7e7 e25dc7954f0832d962347872884aab2dffb426c5 -> 00ad688edb1a79423184992de45a5f0322c8bdf5

and FULL _REPLACE_FMT is "name with type and value with type", like this:

$ git replace --format=full 14ac020163ea60a9d683ce68e36c946f31ecc856 (commit) -> 4b48deed3a433909bfd6b6ab3d4b91348b6af464 (blob) 4b48deed3a433909bfd6b6ab3d4b91348b6af464 (blob) -> feae347d8510cfba5eb8c8ac80056777b07c2528 (blob) 5c37393794868bc8e708cccd7c9d9aaa7a5e53cb (tree) -> 14ac020163ea60a9d683ce68e36c946f31ecc856 (commit) a3fb2e1845a1aaf129b7975048973414dc172173 (commit) -> 9af2a15082b7c95982473e32f3376558c149a7e7 (commit) e25dc7954f0832d962347872884aab2dffb426c5 (tag) -> 00ad688edb1a79423184992de45a5f0322c8bdf5 (commit)

I think this last one might be useful for people replacing objects with objects that have another type. And as we let people do that using --force, it could be useful for them to have a way to easily see what they have done.

Thanks, Christian.

Junio C Hamano· Dec 19, 2013, 18:58 UTC · re: Christian Couder · lore

Re: [PATCH v3 07/10] builtin/replace: teach listing using short, medium or full formats

Christian Couder <christian.couder@gmail.com> writes:
> I think this last one might be useful for people replacing objects
> with objects that have another type.

... which IIUC is strongly discouraged---didn't you have to tighten it recently?

And that makes it "useful primarily for debugging" unusual situations.

Show 5 quoted lines
> And as we let people do that using --force, it could be useful for
> them to have a way to easily see what they have done.
>
> Thanks,
> Christian.
Christian Couder· Dec 21, 2013, 09:34 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 07/10] builtin/replace: teach listing using short, medium or full formats

On Thu, Dec 19, 2013 at 7:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> Christian Couder <christian.couder@gmail.com> writes:
>
>> I think this last one might be useful for people replacing objects
>> with objects that have another type.
>
> ... which IIUC is strongly discouraged---didn't you have to tighten
> it recently?
>
> And that makes it "useful primarily for debugging" unusual
> situations.
Ok, so would you prefer the following:
- NAME_ONLY_REPLACE_FMT and "--format=name_only" instead of
SHORT_REPLACE_FMT and "--format=short"
- NAME_AND_VALUE_REPLACE_FMT and "--format=name_and_value" instead of
MEDIUM_REPLACE_FMT and "--format=medium"
- DEBUG_REPLACE_FMT and "--format=debug" instead of FULL _REPLACE_FMT
and "--format=full"
?

Thanks, Christian.

Junio C Hamano· Dec 26, 2013, 19:10 UTC · re: Christian Couder · lore

Re: [PATCH v3 07/10] builtin/replace: teach listing using short, medium or full formats

Christian Couder <christian.couder@gmail.com> writes:
Show 22 quoted lines
> On Thu, Dec 19, 2013 at 7:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>>> I think this last one might be useful for people replacing objects
>>> with objects that have another type.
>>
>> ... which IIUC is strongly discouraged---didn't you have to tighten
>> it recently?
>>
>> And that makes it "useful primarily for debugging" unusual
>> situations.
>
> Ok, so would you prefer the following:
>
> - NAME_ONLY_REPLACE_FMT and "--format=name_only" instead of
> SHORT_REPLACE_FMT and "--format=short"
>
> - NAME_AND_VALUE_REPLACE_FMT and "--format=name_and_value" instead of
> MEDIUM_REPLACE_FMT and "--format=medium"
>
> - DEBUG_REPLACE_FMT and "--format=debug" instead of FULL _REPLACE_FMT
> and "--format=full"

The end-user facing names are probably fine with short, medium, full, as long as what they show are clearly explained in the end-user documentation (patch 10/10 covers this).

I have a hunch that we may later regret "full" when somebody wants to add even fuller information, though. It might be better spelled "long" instead;

I'd rather see REPLACE_FMT_ as a prefix, not suffix. Do we use common suffix for enum values elsewhere?

Christian Couder· Dec 28, 2013, 10:27 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 07/10] builtin/replace: teach listing using short, medium or full formats

From: Junio C Hamano <gitster@pobox.com>
Show 17 quoted lines
>
> Christian Couder <christian.couder@gmail.com> writes: 
>>
>> Ok, so would you prefer the following:
>>
>> - NAME_ONLY_REPLACE_FMT and "--format=name_only" instead of
>> SHORT_REPLACE_FMT and "--format=short"
>>
>> - NAME_AND_VALUE_REPLACE_FMT and "--format=name_and_value" instead of
>> MEDIUM_REPLACE_FMT and "--format=medium"
>>
>> - DEBUG_REPLACE_FMT and "--format=debug" instead of FULL _REPLACE_FMT
>> and "--format=full"
> 
> The end-user facing names are probably fine with short, medium,
> full, as long as what they show are clearly explained in the
> end-user documentation (patch 10/10 covers this).
Ok, I will try to improve on that.
> I have a hunch that we may later regret "full" when somebody wants
> to add even fuller information, though. It might be better spelled
> "long" instead;
Ok, I will use "long" instead.
> I'd rather see REPLACE_FMT_ as a prefix, not suffix.  Do we use
> common suffix for enum values elsewhere?

I don't see common suffix, but we have the following enums about formats:

* in builtin/commit.c:
static enum status_format {
        STATUS_FORMAT_NONE = 0,
        STATUS_FORMAT_LONG,
        STATUS_FORMAT_SHORT,
        STATUS_FORMAT_PORCELAIN,
        STATUS_FORMAT_UNSPECIFIED
} status_format = STATUS_FORMAT_UNSPECIFIED;
* in builtin/help.c:
enum help_format {
        HELP_FORMAT_NONE,
        HELP_FORMAT_MAN,
        HELP_FORMAT_INFO,
        HELP_FORMAT_WEB
};
* in commit.h
enum cmit_fmt {
        CMIT_FMT_RAW,
        CMIT_FMT_MEDIUM,
        CMIT_FMT_DEFAULT = CMIT_FMT_MEDIUM,
        CMIT_FMT_SHORT,
        CMIT_FMT_FULL,
        CMIT_FMT_FULLER,
        CMIT_FMT_ONELINE,
        CMIT_FMT_EMAIL,
        CMIT_FMT_USERFORMAT,
        CMIT_FMT_UNSPECIFIED
};

To conform to the above and what you suggest, I will send a new series using the following:

enum replace_format {
      REPLACE_FORMAT_SHORT,
      REPLACE_FORMAT_MEDIUM,
      REPLACE_FORMAT_LONG
};

Thanks, Christian.

Christian Couder· Dec 11, 2013, 07:46 UTC · re: Christian Couder · lore

[PATCH v3 08/10] t6050: add tests for listing with --format

This patch adds tests for "git replace -l --format=<fmt>".

'short', 'medium' and 'full' are the only allowed values for <fmt>.

'short' is the same as with no --format option. Tests for 'medium' and 'full' are the most needed.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
Show changes to t/t6050-replace.sh +37 −0
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index bb785ec..e1cc3b8 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -281,6 +281,43 @@ test_expect_success 'git cat-file --batch works on replace objects' '
 	echo $PARA3 | git cat-file --batch
 '
 
+test_expect_success 'test --format bogus' '
+	test_must_fail git replace --format bogus >/dev/null 2>&1
+'
+
+test_expect_success 'test --format short' '
+	git replace --format=short >actual &&
+	git replace >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'test --format medium' '
+	H1=$(git --no-replace-objects rev-parse HEAD~1) &&
+	HT=$(git --no-replace-objects rev-parse HEAD^{tree}) &&
+	MYTAG=$(git --no-replace-objects rev-parse mytag) &&
+	{
+		echo "$H1 -> $BLOB" &&
+		echo "$BLOB -> $REPLACED" &&
+		echo "$HT -> $H1" &&
+		echo "$PARA3 -> $S" &&
+		echo "$MYTAG -> $HASH1"
+	} | sort >expected &&
+	git replace -l --format medium | sort > actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure 'test --format full' '
+	{
+		echo "$H1 (commit) -> $BLOB (blob)" &&
+		echo "$BLOB (blob) -> $REPLACED (blob)" &&
+		echo "$HT (tree) -> $H1 (commit)" &&
+		echo "$PARA3 (commit) -> $S (commit)" &&
+		echo "$MYTAG (tag) -> $HASH1 (commit)"
+	} | sort >expected &&
+	git replace --format=full | sort > actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'replace ref cleanup' '
 	test -n "$(git replace)" &&
 	git replace -d $(git replace) &&
-- 
1.8.5.1.102.g090758b
Christian Couder· Dec 11, 2013, 07:46 UTC · re: Christian Couder · lore

[PATCH v3 09/10] builtin/replace: unset read_replace_refs

When checking to see if some objects are of the same type and when displaying the type of objects, git replace uses the sha1_object_info() function.

Unfortunately this function by default respects replace refs, so instead of the type of a replaced object, it gives the type of the replacement object which might be different.

To fix this bug, and because git replace should work at a level before replacement takes place, let's unset the read_replace_refs global variable at the beginning of cmd_replace().

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/replace.c  | 2 ++
 t/t6050-replace.sh | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)
Show changes to 2 files +3 −1

builtin/replace.c, t/t6050-replace.sh

diff --git a/builtin/replace.c b/builtin/replace.c
index 9f3619a..1672870 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -174,6 +174,8 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	read_replace_refs = 0;
+
 	argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);
 
 	if (list && delete)
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index e1cc3b8..d0c62f7 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -306,7 +306,7 @@ test_expect_success 'test --format medium' '
 	test_cmp expected actual
 '
 
-test_expect_failure 'test --format full' '
+test_expect_success 'test --format full' '
 	{
 		echo "$H1 (commit) -> $BLOB (blob)" &&
 		echo "$BLOB (blob) -> $REPLACED (blob)" &&
-- 
1.8.5.1.102.g090758b
Christian Couder· Dec 11, 2013, 07:46 UTC · re: Christian Couder · lore

[PATCH v3 10/10] Documentation/git-replace: describe --format option

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-replace.txt | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)
Show changes to Documentation/git-replace.txt +18 −1
diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
index f373ab4..7a07828 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git replace' [-f] <object> <replacement>
 'git replace' -d <object>...
-'git replace' -l [<pattern>]
+'git replace' [--format=<format>] [-l [<pattern>]]
 
 DESCRIPTION
 -----------
@@ -70,6 +70,23 @@ OPTIONS
 	Typing "git replace" without arguments, also lists all replace
 	refs.
 
+--format=<format>::
+	When listing, use the specified <format>, which can be one of
+	'short', 'medium' and 'full'. When omitted, the format
+	defaults to 'short'.
+
+FORMATS
+-------
+
+The following format are available:
+
+* 'short':
+	<replaced sha1>
+* 'medium':
+	<replaced sha1> -> <replacement sha1>
+* 'full'
+	<replaced sha1> (<replaced type>) -> <replacement sha1> (<replacement type>)
+
 CREATING REPLACEMENT OBJECTS
 ----------------------------
 
-- 
1.8.5.1.102.g090758b
Junio C Hamano· Dec 12, 2013, 20:05 UTC · re: Christian Couder · lore

Re: [PATCH v3 00/10] teach replace objects to sha1_object_info_extended()

Christian Couder <chriscool@tuxfamily.org> writes:
> Here is version 3 of a patch series to improve the way
> sha1_object_info_extended() behaves when it is passed a
> replaced object. The idea is to add a flags argument to it
> in the same way as what has been done to read_sha1_file().
Thanks.
Will take a look again (in the meantime, will queue on 'pu').

I do not think the new name for the bit is necessary nor it is a good change, though. Given an object name, reading the data and inspecting the metadata (i.e. type and size) should yield consistent results, so the original name is a perfectly appropriate name that means "use the replacement object when using read-sha1-file and friends to ask about an object". The use of the replacement object happens to be implmented via lookup-replace-object helper, but that is an implementation detail of _how_ it is done, not the high level descroption of _what_ the callers want to see done.

But that is just a minor nit.

← back to recent threads