[PATCH] log: decorate grafted commits with "grafted"

[PATCH] log: decorate grafted commits with "grafted"

[Nguyễn Thái Ngọc Duy]

This would help decide whether to deepen some more on shallow
repositories, or the branch really has really ends there.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I briefly considered wrapping _() around "grafted" text, but
 decided that making it a keyword might help searching

 log-tree.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index e945701..c469341 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -18,6 +18,7 @@ enum decoration_type {
 	DECORATION_REF_TAG,
 	DECORATION_REF_STASH,
 	DECORATION_REF_HEAD,
+	DECORATION_GRAFTED
 };
 
 static char decoration_colors[][COLOR_MAXLEN] = {
@@ -27,6 +28,7 @@ static char decoration_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_BOLD_YELLOW,	/* REF_TAG */
 	GIT_COLOR_BOLD_MAGENTA,	/* REF_STASH */
 	GIT_COLOR_BOLD_CYAN,	/* REF_HEAD */
+	GIT_COLOR_BOLD_BLUE,	/* GRAFTED */
 };
 
 static const char *decorate_get_color(int decorate_use_color, enum decoration_type ix)
@@ -638,6 +640,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 	log.parent = NULL;
 	opt->loginfo = &log;
 
+	if (!commit->parents && lookup_commit_graft(commit->object.sha1))
+		add_name_decoration(DECORATION_GRAFTED, "grafted",
+				    &commit->object);
 	shown = log_tree_diff(opt, commit, &log);
 	if (!shown && opt->loginfo && opt->always_show_header) {
 		log.parent = NULL;
-- 
1.7.4.74.g639db

Re: [PATCH] log: decorate grafted commits with "grafted"

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> @@ -638,6 +640,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
>  	log.parent = NULL;
>  	opt->loginfo = &log;
>  
> +	if (!commit->parents && lookup_commit_graft(commit->object.sha1))
> +		add_name_decoration(DECORATION_GRAFTED, "grafted",
> +				    &commit->object);

I am not very enthused about this change.

We have already looked up the commit when we parsed it, and then we again
have to call lookup_commit_graft() which would yield false for most of the
commits?

Does this work with replacements and shallows, by the way?

Re: [PATCH] log: decorate grafted commits with "grafted"

[Nguyen Thai Ngoc Duy]

2011/8/18 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> @@ -638,6 +640,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
>>       log.parent = NULL;
>>       opt->loginfo = &log;
>>
>> +     if (!commit->parents && lookup_commit_graft(commit->object.sha1))
>> +             add_name_decoration(DECORATION_GRAFTED, "grafted",
>> +                                 &commit->object);
>
> I am not very enthused about this change.
>
> We have already looked up the commit when we parsed it, and then we again
> have to call lookup_commit_graft() which would yield false for most of the
> commits?

which is why there's "!commit->parents" check. I made this patch with
shallow clone in mind. You probably have seen that if grafts are used
to extend history instead of cutting it, it won't show.

> Does this work with replacements and shallows, by the way?

Shallows, yes (that was my aim). Replacements, no. A better way would
be go over commit_graft[] and replace_object[] arrays, decorate all
grafted/replaced commits, instead of checking in log_tree_commit().
-- 
Duy

[PATCH v2 0/5] Decorate grafts and replaces

[Nguyễn Thái Ngọc Duy]

I originally wanted to decorate shallow cut points. Now it decorates
all grafts (normal as well as shallow cuts) and replaced commits/tags.

The first patch is an independent fix. The last one, decorating
replaces, can also be considered a fix (decorating "refs/replace/SHA1"
on random commits does not sound right, but maybe it's just me).

Nguyễn Thái Ngọc Duy (5):
  decoration: do not mis-decorate refs with same prefix
  Add for_each_commit_graft() to iterate all grafts
  Move write_shallow_commits to fetch-pack.c
  log: decorate grafted commits with "grafted"
  log: decorate "replaced" on to replaced commits

 builtin/fetch-pack.c |   30 ++++++++++++++++++++++++++++++
 commit.c             |   20 +++++---------------
 commit.h             |    3 ++-
 log-tree.c           |   34 ++++++++++++++++++++++++++++++----
 4 files changed, 67 insertions(+), 20 deletions(-)

-- 
1.7.4.74.g639db

[PATCH 1/5] decoration: do not mis-decorate refs with same prefix

[Nguyễn Thái Ngọc Duy]

We definitely do not want to decorate refs/headsandtails the same as
refs/heads/*, for example.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 log-tree.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index e945701..344f734 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -95,11 +95,11 @@ static int add_ref_decoration(const char *refname, const unsigned char *sha1, in
 	if (!obj)
 		return 0;
 
-	if (!prefixcmp(refname, "refs/heads"))
+	if (!prefixcmp(refname, "refs/heads/"))
 		type = DECORATION_REF_LOCAL;
-	else if (!prefixcmp(refname, "refs/remotes"))
+	else if (!prefixcmp(refname, "refs/remotes/"))
 		type = DECORATION_REF_REMOTE;
-	else if (!prefixcmp(refname, "refs/tags"))
+	else if (!prefixcmp(refname, "refs/tags/"))
 		type = DECORATION_REF_TAG;
 	else if (!prefixcmp(refname, "refs/stash"))
 		type = DECORATION_REF_STASH;
-- 
1.7.4.74.g639db

Re: [PATCH 1/5] decoration: do not mis-decorate refs with same prefix

[Junio C Hamano]

Good eyes; thanks.

[PATCH 2/5] Add for_each_commit_graft() to iterate all grafts

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 commit.c |    8 ++++++++
 commit.h |    2 ++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/commit.c b/commit.c
index dc22695..efd647d 100644
--- a/commit.c
+++ b/commit.c
@@ -226,6 +226,14 @@ struct commit_graft *lookup_commit_graft(const unsigned char *sha1)
 	return commit_graft[pos];
 }
 
+int for_each_commit_graft(each_commit_graft_fn fn, void *cb_data)
+{
+	int i, ret;
+	for (i = ret = 0; i < commit_graft_nr && !ret; i++)
+		ret = fn(commit_graft[i], cb_data);
+	return ret;
+}
+
 int write_shallow_commits(struct strbuf *out, int use_pack_protocol)
 {
 	int i, count = 0;
diff --git a/commit.h b/commit.h
index 0e36fd0..9030d42 100644
--- a/commit.h
+++ b/commit.h
@@ -143,6 +143,7 @@ struct commit_graft {
 	int nr_parent; /* < 0 if shallow commit */
 	unsigned char parent[FLEX_ARRAY][20]; /* more */
 };
+typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);
 
 struct commit_graft *read_graft_line(char *buf, int len);
 int register_commit_graft(struct commit_graft *, int);
@@ -155,6 +156,7 @@ extern struct commit_list *get_octopus_merge_bases(struct commit_list *in);
 extern int register_shallow(const unsigned char *sha1);
 extern int unregister_shallow(const unsigned char *sha1);
 extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol);
+extern int for_each_commit_graft(each_commit_graft_fn, void *);
 extern int is_repository_shallow(void);
 extern struct commit_list *get_shallow_commits(struct object_array *heads,
 		int depth, int shallow_flag, int not_shallow_flag);
-- 
1.7.4.74.g639db

[PATCH 3/5] Move write_shallow_commits to fetch-pack.c

[Nguyễn Thái Ngọc Duy]

This function produces network traffic and should be in fetch-pack. It
has been in commit.c because it needs to iterate (private) graft
list. It can now do so using for_each_commit_graft().

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/fetch-pack.c |   30 ++++++++++++++++++++++++++++++
 commit.c             |   18 ------------------
 commit.h             |    1 -
 3 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 4367984..cb5b20a 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -185,6 +185,36 @@ static void consume_shallow_list(int fd)
 	}
 }
 
+struct write_shallow_data {
+	struct strbuf *out;
+	int use_pack_protocol;
+	int count;
+};
+
+static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
+{
+	struct write_shallow_data *data = cb_data;
+	const char *hex = sha1_to_hex(graft->sha1);
+	data->count++;
+	if (data->use_pack_protocol)
+		packet_buf_write(data->out, "shallow %s", hex);
+	else {
+		strbuf_addstr(data->out, hex);
+		strbuf_addch(data->out, '\n');
+	}
+	return 0;
+}
+
+static int write_shallow_commits(struct strbuf *out, int use_pack_protocol)
+{
+	struct write_shallow_data data;
+	data.out = out;
+	data.use_pack_protocol = use_pack_protocol;
+	data.count = 0;
+	for_each_commit_graft(write_one_shallow, &data);
+	return data.count;
+}
+
 static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 {
 	static char line[1000];
diff --git a/commit.c b/commit.c
index efd647d..661ff0d 100644
--- a/commit.c
+++ b/commit.c
@@ -234,24 +234,6 @@ int for_each_commit_graft(each_commit_graft_fn fn, void *cb_data)
 	return ret;
 }
 
-int write_shallow_commits(struct strbuf *out, int use_pack_protocol)
-{
-	int i, count = 0;
-	for (i = 0; i < commit_graft_nr; i++)
-		if (commit_graft[i]->nr_parent < 0) {
-			const char *hex =
-				sha1_to_hex(commit_graft[i]->sha1);
-			count++;
-			if (use_pack_protocol)
-				packet_buf_write(out, "shallow %s", hex);
-			else {
-				strbuf_addstr(out, hex);
-				strbuf_addch(out, '\n');
-			}
-		}
-	return count;
-}
-
 int unregister_shallow(const unsigned char *sha1)
 {
 	int pos = commit_graft_pos(sha1);
diff --git a/commit.h b/commit.h
index 9030d42..82d5aeb 100644
--- a/commit.h
+++ b/commit.h
@@ -155,7 +155,6 @@ extern struct commit_list *get_octopus_merge_bases(struct commit_list *in);
 
 extern int register_shallow(const unsigned char *sha1);
 extern int unregister_shallow(const unsigned char *sha1);
-extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol);
 extern int for_each_commit_graft(each_commit_graft_fn, void *);
 extern int is_repository_shallow(void);
 extern struct commit_list *get_shallow_commits(struct object_array *heads,
-- 
1.7.4.74.g639db

Re: [PATCH 3/5] Move write_shallow_commits to fetch-pack.c

[Junio C Hamano]

Nice; thanks.

[PATCH 4/5] log: decorate grafted commits with "grafted"

[Nguyễn Thái Ngọc Duy]

In shallow repositories, this may help detect whether a branch ends,
or it is deeper than current depth.

It also show graft points that extend a branch.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 log-tree.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 344f734..5605244 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -18,6 +18,7 @@ enum decoration_type {
 	DECORATION_REF_TAG,
 	DECORATION_REF_STASH,
 	DECORATION_REF_HEAD,
+	DECORATION_GRAFTED,
 };
 
 static char decoration_colors[][COLOR_MAXLEN] = {
@@ -27,6 +28,7 @@ static char decoration_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_BOLD_YELLOW,	/* REF_TAG */
 	GIT_COLOR_BOLD_MAGENTA,	/* REF_STASH */
 	GIT_COLOR_BOLD_CYAN,	/* REF_HEAD */
+	GIT_COLOR_BOLD_BLUE,	/* GRAFTED */
 };
 
 static const char *decorate_get_color(int decorate_use_color, enum decoration_type ix)
@@ -118,6 +120,15 @@ static int add_ref_decoration(const char *refname, const unsigned char *sha1, in
 	return 0;
 }
 
+static int add_graft_decoration(const struct commit_graft *graft, void *cb_data)
+{
+	struct commit *commit = lookup_commit(graft->sha1);
+	if (!commit)
+		return 0;
+	add_name_decoration(DECORATION_GRAFTED, "grafted", &commit->object);
+	return 0;
+}
+
 void load_ref_decorations(int flags)
 {
 	static int loaded;
@@ -125,6 +136,7 @@ void load_ref_decorations(int flags)
 		loaded = 1;
 		for_each_ref(add_ref_decoration, &flags);
 		head_ref(add_ref_decoration, &flags);
+		for_each_commit_graft(add_graft_decoration, NULL);
 	}
 }
 
-- 
1.7.4.74.g639db

Re: [PATCH 4/5] log: decorate grafted commits with "grafted"

[Junio C Hamano]

I think this makes a lot more sense than the other option of looking up
graft points every time, as we expect only a handful of grafts, if any,
exist in a repository.

If you have, on the other hand, tons of historical graft points, and you
are viewing only near the tip (e.g. "log -20"), this implimentation still
calls lookup_commit() for all those historical graft points that are
likely to be scattered further back in the packfile, and the performance
may suffer.  But that is what it means when we say that we are optimized
for use case where only a handful of grafts exist, so it is not something
that we worry about too much.

[PATCH 5/5] log: decorate "replaced" on to replaced commits

[Nguyễn Thái Ngọc Duy]

Old code also decorates "new" commits with "refs/replace/SHA1". This
is now gone, but I guess no one will miss it.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 log-tree.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 5605244..b982b9f 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -92,8 +92,22 @@ static void add_name_decoration(enum decoration_type type, const char *name, str
 
 static int add_ref_decoration(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
 {
-	struct object *obj = parse_object(sha1);
+	struct object *obj;
 	enum decoration_type type = DECORATION_NONE;
+
+	if (!prefixcmp(refname, "refs/replace/")) {
+		unsigned char original_sha1[20];
+		if (get_sha1_hex(refname + 13, original_sha1)) {
+			warning("invalid replace ref %s", refname);
+			return 0;
+		}
+		obj = parse_object(original_sha1);
+		if (obj && (obj->type == OBJ_COMMIT || obj->type == OBJ_TAG))
+			add_name_decoration(DECORATION_GRAFTED, "replaced", obj);
+		return 0;
+	}
+
+	obj = parse_object(sha1);
 	if (!obj)
 		return 0;
 
-- 
1.7.4.74.g639db

Re: [PATCH 5/5] log: decorate "replaced" on to replaced commits

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Old code also decorates "new" commits with "refs/replace/SHA1". This
> is now gone, but I guess no one will miss it.

Makes sense but...

> +	if (!prefixcmp(refname, "refs/replace/")) {
> +		unsigned char original_sha1[20];
> +		if (get_sha1_hex(refname + 13, original_sha1)) {
> +			warning("invalid replace ref %s", refname);
> +			return 0;
> +		}
> +		obj = parse_object(original_sha1);
> +		if (obj && (obj->type == OBJ_COMMIT || obj->type == OBJ_TAG))

... is it necessary to check and limit the types like this here?

If the argument is "we know only commits and tags are listed and blobs and
trees are not shown with --decorate option" and "excluding the decoration
that we know will never be used will avoid bloating the decorate hashtable
with unused cruft", then add_name_decoration() should be doing the check
for all of its callers, not just this one, no?

> +			add_name_decoration(DECORATION_GRAFTED, "replaced", obj);
> +		return 0;
> +	}
> +
> +	obj = parse_object(sha1);
>  	if (!obj)
>  		return 0;

[PATCH] log: decorate "replaced" on to replaced commits

[Nguyễn Thái Ngọc Duy]

Old code also decorates "new" commits with "refs/replace/SHA1". This
is now gone, but I guess no one will miss it.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 2011/8/19 Junio C Hamano <gitster@pobox.com>:
 > If the argument is "we know only commits and tags are listed and blobs and
 > trees are not shown with --decorate option" and "excluding the decoration
 > that we know will never be used will avoid bloating the decorate hashtable
 > with unused cruft", then add_name_decoration() should be doing the check
 > for all of its callers, not just this one, no?

 Makes sense. Moreover replacing blobs and trees are generally not safe.
 If people do that, they may have more issues to worry about than this.
 Let's keep it simple. We can fix add_name_decoration() later if it becomes
 a real problem.

 log-tree.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index e945701..d73e69c 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -92,8 +92,22 @@ static void add_name_decoration(enum decoration_type type, const char *name, str
 
 static int add_ref_decoration(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
 {
-	struct object *obj = parse_object(sha1);
+	struct object *obj;
 	enum decoration_type type = DECORATION_NONE;
+
+	if (!prefixcmp(refname, "refs/replace/")) {
+		unsigned char original_sha1[20];
+		if (get_sha1_hex(refname + 13, original_sha1)) {
+			warning("invalid replace ref %s", refname);
+			return 0;
+		}
+		obj = parse_object(original_sha1);
+		if (obj)
+			add_name_decoration(DECORATION_GRAFTED, "replaced", obj);
+		return 0;
+	}
+
+	obj = parse_object(sha1);
 	if (!obj)
 		return 0;
 
-- 
1.7.4.74.g639db