threads / rfc / 18026

RFC patch, 4 partsExpose reflog_info struct in header

Subject: [RFC PATCH 1/4] Expose reflog_info struct in header

## tl;dr

7 messages between Feb 26, 2009 and Feb 26, 2009. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Deskin Miller· Feb 26, 2009, 04:44 UTC · lore

[RFC PATCH 0/4] git log -g --pretty=format: learns about reflog

This series exposes reflog information to the pretty-printer when using --pretty=format: to git log -g and git rev-list --walk-reflogs, through similar format codes as are used currently.

Thomas Rast mentioned almost as an aside that this would be useful in 
http://article.gmane.org/gmane.comp.version-control.git/108009

and I've wanted to hack this series for a long time, even before that comment, but my first attempts broke stash horribly. Hopefully this version fares better.

I'm a little leery of using commit->util to carry the reflog info, but I didn't see a much better way, without altering the signature of pretty_print_commit to take an optional reflog parameter. The code checks that util is not being used before assigning the reflog to it, so it shouldn't break any existing uses of util of which I am unaware. I confess to having not done an appropriate amount of research into whether my use of util will actually break anything. For what it's worth this patch series doesn't break any existing tests.

One of the things I was hoping to do in the unsuccessful version was to allow the revision walker to use the reflog, but let the commit structures retain their real parents. That way, one could still obtain meaningful information from %p even when walking reflogs, which would be distinct from %rp. It is primarily with this in mind that %rp exists at all. However, as I mentioned, my attempt to change the revision walker broke existing tests, and I didn't have the wherewithal to discover how to correct it. It's possible with the current series that one could simply try to re-parse the commit, but I haven't investigated the feasibility of doing that.

I strongly suspect that there's a cleaner way to do this, but don't really know what that would be, so any comments regarding alternate approaches would be very appreciated. Also, I'm more than happy to write testcases and documentation once there is some agreement about what the preferred implementation is.

Deskin Miller (4):
  Expose reflog_info struct in header
  Add attach_reflog_info
  Attach reflog to commit prior to pretty-printing
  Teach pretty-printer new reflog format codes
 list-objects.c |    3 +++
 log-tree.c     |    3 +++
 pretty.c       |   38 ++++++++++++++++++++++++++++++++++++++
 reflog-walk.c  |   26 +++++++++++++++++++-------
 reflog-walk.h  |   12 ++++++++++++
 5 files changed, 75 insertions(+), 7 deletions(-)
Deskin Miller· Feb 26, 2009, 04:44 UTC · re: Deskin Miller · lore

The pretty-printing code needs to know this struct in order to print fields from it in log output.

Signed-off-by: Deskin Miller <deskinm@umich.edu>
---
 reflog-walk.c |    8 +-------
 reflog-walk.h |   10 ++++++++++
 2 files changed, 11 insertions(+), 7 deletions(-)
Show changes to 2 files +11 −7

reflog-walk.c, reflog-walk.h

diff --git a/reflog-walk.c b/reflog-walk.c
index f751fdc..dd172ae 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -8,13 +8,7 @@
 
 struct complete_reflogs {
 	char *ref;
-	struct reflog_info {
-		unsigned char osha1[20], nsha1[20];
-		char *email;
-		unsigned long timestamp;
-		int tz;
-		char *message;
-	} *items;
+	struct reflog_info *items;
 	int nr, alloc;
 };
 
diff --git a/reflog-walk.h b/reflog-walk.h
index 7ca1438..7b00993 100644
--- a/reflog-walk.h
+++ b/reflog-walk.h
@@ -1,6 +1,16 @@
 #ifndef REFLOG_WALK_H
 #define REFLOG_WALK_H
 
+struct reflog_walk_info;
+
+struct reflog_info {
+	unsigned char osha1[20], nsha1[20];
+	char *email;
+	unsigned long timestamp;
+	int tz;
+	char *message;
+};
+
 extern void init_reflog_walk(struct reflog_walk_info** info);
 extern int add_reflog_for_walk(struct reflog_walk_info *info,
 		struct commit *commit, const char *name);

-- 
1.6.2.rc0.90.g0753
Deskin Miller· Feb 26, 2009, 04:44 UTC · re: Deskin Miller · lore

[RFC PATCH 2/4] Add attach_reflog_info

To enable pretty-printing to print reflog information, the commit object needs to carry its associated reflog entry. This function uses the util field to point to the appropriate struct reflog_info.

Signed-off-by: Deskin Miller <deskinm@umich.edu>
---
 reflog-walk.c |   18 ++++++++++++++++++
 reflog-walk.h |    2 ++
 2 files changed, 20 insertions(+), 0 deletions(-)
Show changes to 2 files +20 −0

reflog-walk.c, reflog-walk.h

diff --git a/reflog-walk.c b/reflog-walk.c
index dd172ae..3e8457f 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -235,6 +235,24 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)
 	commit->object.flags &= ~(ADDED | SEEN | SHOWN);
 }
 
+void attach_reflog_info(struct reflog_walk_info *info, struct commit *commit)
+{
+	if (info && info->last_commit_reflog) {
+		struct commit_reflog *commit_reflog = info->last_commit_reflog;
+
+		if (commit->util &&
+		    (((void *)(commit_reflog->reflogs->items
+			       + commit_reflog->reflogs->nr) < commit->util)
+		     || ((void *)(commit_reflog->reflogs->items +
+				  commit_reflog->recno + 1) > commit->util))) {
+			die("commit->util being used, cannot attach reflog");
+		}
+
+		commit->util = &commit_reflog->reflogs->items
+			[commit_reflog->recno+1];
+	}
+}
+
 void show_reflog_message(struct reflog_walk_info* info, int oneline,
 	int relative_date)
 {
diff --git a/reflog-walk.h b/reflog-walk.h
index 7b00993..44f97e1 100644
--- a/reflog-walk.h
+++ b/reflog-walk.h
@@ -16,6 +16,8 @@ extern int add_reflog_for_walk(struct reflog_walk_info *info,
 		struct commit *commit, const char *name);
 extern void fake_reflog_parent(struct reflog_walk_info *info,
 		struct commit *commit);
+extern void attach_reflog_info(struct reflog_walk_info *info,
+		struct commit *commit);
 extern void show_reflog_message(struct reflog_walk_info *info, int, int);
 
 #endif

-- 
1.6.2.rc0.90.g0753
Deskin Miller· Feb 26, 2009, 04:44 UTC · re: Deskin Miller · lore

[RFC PATCH 3/4] Attach reflog to commit prior to pretty-printing

Use attach_reflog_info just prior to pretty-printing the commit, so the reflog info may be printed if desired.

Signed-off-by: Deskin Miller <deskinm@umich.edu>
---
 list-objects.c |    3 +++
 log-tree.c     |    3 +++
 2 files changed, 6 insertions(+), 0 deletions(-)
Show changes to 2 files +6 −0

list-objects.c, log-tree.c

diff --git a/list-objects.c b/list-objects.c
index c8b8375..fd9d23e 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -5,6 +5,7 @@
 #include "blob.h"
 #include "diff.h"
 #include "tree-walk.h"
+#include "reflog-walk.h"
 #include "revision.h"
 #include "list-objects.h"
 
@@ -146,6 +147,8 @@ void traverse_commit_list(struct rev_info *revs,
 
 	while ((commit = get_revision(revs)) != NULL) {
 		process_tree(revs, commit->tree, &objects, NULL, "");
+		if (revs->reflog_info)
+			attach_reflog_info(revs->reflog_info, commit);
 		show_commit(commit);
 	}
 	for (i = 0; i < revs->pending.nr; i++) {
diff --git a/log-tree.c b/log-tree.c
index 84a74e5..5ccca0e 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -387,6 +387,9 @@ void show_log(struct rev_info *opt)
 	 */
 	if (need_8bit_cte >= 0)
 		need_8bit_cte = has_non_ascii(opt->add_signoff);
+	if (opt->reflog_info) {
+		attach_reflog_info(opt->reflog_info, commit);
+	}
 	pretty_print_commit(opt->commit_format, commit, &msgbuf,
 			    abbrev, subject, extra_headers, opt->date_mode,
 			    need_8bit_cte);

-- 
1.6.2.rc0.90.g0753
Deskin Miller· Feb 26, 2009, 04:44 UTC · re: Deskin Miller · lore

[RFC PATCH 4/4] Teach pretty-printer new reflog format codes

By using %r<char>, one may obtain information from the reflog when using --pretty=format in conjunction with git log -g or git rev-list --walk-reflogs. The following format codes are supported:

%rp, %rP: abbreviated and full old commit hash %rm: reflog message %rn, %re, %rt, %rd, etc: all the formats that work for committer and author work with the reflog printer as well.

Signed-off-by: Deskin Miller <deskinm@umich.edu>
---
 pretty.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)
Show changes to pretty.c +38 −0
diff --git a/pretty.c b/pretty.c
index 6cd9149..9303958 100644
--- a/pretty.c
+++ b/pretty.c
@@ -7,6 +7,7 @@
 #include "mailmap.h"
 #include "log-tree.h"
 #include "color.h"
+#include "reflog-walk.h"
 
 static char *user_format;
 
@@ -544,6 +545,40 @@ static void format_decoration(struct strbuf *sb, const struct commit *commit)
 		strbuf_addch(sb, ')');
 }
 
+static size_t format_reflog_item(struct strbuf *sb, void *util, char part,
+				 enum date_mode dmode)
+{
+	struct reflog_info *reflog_info = (struct reflog_info *)util;
+	struct strbuf buf = STRBUF_INIT;
+	ssize_t ret;
+
+	if (!util) {
+		return 0;
+	}
+
+	switch (part) {
+	case 'P':
+		strbuf_addstr(sb, sha1_to_hex(reflog_info->osha1));
+		return 2;
+	case 'p':		/* abbreviated commit hash */
+		strbuf_addstr(sb, find_unique_abbrev(reflog_info->osha1,
+						DEFAULT_ABBREV));
+		return 2;
+	case 'm':
+		/* don't print trailing newline */
+		strbuf_add(sb, reflog_info->message,
+			   strlen(reflog_info->message) - 1);
+		return 2;
+	}
+
+	strbuf_addf(&buf, "%s %lu %+04d", reflog_info->email,
+		reflog_info->timestamp, reflog_info->tz);
+	ret = format_person_part(sb, part, buf.buf, buf.len, dmode);
+	strbuf_release(&buf);
+	return ret;
+
+}
+
 static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
                                void *context)
 {
@@ -650,6 +685,9 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	case 'd':
 		format_decoration(sb, commit);
 		return 1;
+	case 'r':
+		return format_reflog_item(sb, commit->util, placeholder[1],
+					c->dmode);
 	}
 
 	/* For the rest we have to parse the commit header. */

-- 
1.6.2.rc0.90.g0753
Johannes Schindelin· Feb 26, 2009, 10:30 UTC · re: Deskin Miller · lore

Re: [RFC PATCH 0/4] git log -g --pretty=format: learns about reflog

Hi,
On Wed, 25 Feb 2009, Deskin Miller wrote:
Show 5 quoted lines
> I strongly suspect that there's a cleaner way to do this, but don't 
> really know what that would be, so any comments regarding alternate 
> approaches would be very appreciated.  Also, I'm more than happy to 
> write testcases and documentation once there is some agreement about 
> what the preferred implementation is.
>From skimming your mail, I am not quite sure if I understand correctly

what you want to do. Is it that you want to be able to output the "Reflog:" and "Reflog message:" information that "git log -g" prints?

If so, I suggest expanding the signature of format_commit_message.

Even better: define a "struct format_commit_options" a pointer to which gets passed around, and which should make the code cleaner anyway.

You could then put a pointer to the reflog_info into that struct.

Ciao, Dscho

Jeff King· Feb 26, 2009, 10:55 UTC · re: Deskin Miller · lore

Re: [RFC PATCH 0/4] git log -g --pretty=format: learns about reflog

On Wed, Feb 25, 2009 at 11:44:03PM -0500, Deskin Miller wrote:
> I'm a little leery of using commit->util to carry the reflog info, but I
> didn't see a much better way, without altering the signature of
> pretty_print_commit to take an optional reflog parameter.  The code

Personally I am not opposed to some refactoring of the pretty_print code. A few months ago I wanted to add a new pretty-printing feature (it was keeping count of the total and printed patches, so you could say something "Patch %count/%total"). Passing information around through all of the functions got quite burdensome, and I started a refactoring to have a "pretty_print_context" that could be passed around and contain a lot of the pseudo-global variables.

I didn't get very far before giving up and working on something else. But my point is that any refactoring you do may end up helping other features in the future.

As a side note, I also think figuring out what the formats look like by reading the pretty-print code is insane, because it is a mass of special cases and switch statements on format. I think it would be much more manageable "inside-out": each format has its own primary function, and the common stuff is factored out into helpers.

But that's just based on memory from a few months ago.
-Peff

← back to recent threads