threads / patch / 28141

patchlog: decorate grafted commits with "grafted"

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

## tl;dr

14 messages between Aug 17, 2011 and Aug 19, 2011. Diffs are folded; open one to read it.

replies: 13people: 2as markdown or json

Nguyễn Thái Ngọc Duy· Aug 17, 2011, 15:02 UTC · lore

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(-)
Show changes to log-tree.c +5 −0
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
Junio C Hamano· Aug 17, 2011, 18:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 7 quoted lines
> @@ -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?
Nguyen Thai Ngoc Duy· Aug 18, 2011, 02:02 UTC · re: Junio C Hamano · lore

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

2011/8/18 Junio C Hamano <gitster@pobox.com>:
Show 15 quoted lines
> 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
Nguyễn Thái Ngọc Duy· Aug 18, 2011, 12:29 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 0/5] Decorate grafts and replaces

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
Nguyễn Thái Ngọc Duy· Aug 18, 2011, 12:29 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to log-tree.c +3 −3
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
Nguyễn Thái Ngọc Duy· Aug 18, 2011, 12:29 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 2 files +10 −0

commit.c, commit.h

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
Nguyễn Thái Ngọc Duy· Aug 18, 2011, 12:29 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 3 files +30 −19

builtin/fetch-pack.c, commit.c, commit.h

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
Nguyễn Thái Ngọc Duy· Aug 18, 2011, 12:29 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to log-tree.c +12 −0
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
Junio C Hamano· Aug 18, 2011, 18:10 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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.

Nguyễn Thái Ngọc Duy· Aug 18, 2011, 12:29 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to log-tree.c +15 −1
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
Junio C Hamano· Aug 18, 2011, 18:16 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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...
Show 8 quoted lines
> +	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?

Show 7 quoted lines
> +			add_name_decoration(DECORATION_GRAFTED, "replaced", obj);
> +		return 0;
> +	}
> +
> +	obj = parse_object(sha1);
>  	if (!obj)
>  		return 0;
Nguyễn Thái Ngọc Duy· Aug 19, 2011, 12:43 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to log-tree.c +15 −1
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

← back to recent threads