git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: Truncating HEAD reflog on branch move

From
Jeff King <peff@peff.net>
Date
Jun 22, 2017, 20:21 UTC
Message-ID
<20170622202146.cxrkjca636xl4dgk@sigill.intra.peff.net>
In-Reply-To
<xmqqvannkfp8.fsf@gitster.mtv.corp.google.com>
On Thu, Jun 22, 2017 at 12:03:31PM -0700, Junio C Hamano wrote:
Show 13 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > I don't think this is quite right, though. We've decremented "recno"
> > after assigning the old pointer to "reflog". So in the existing code,
> > "reflog" in that second conditional pointing to the _next_ entry (or
> > previous, really, since we are going in reverse order).
> >
> > So I think you'd need to look at commit->reflog again (after checking
> > that we didn't go past the start of the array).
> 
> Perhaps.  I did the illustration that way simply because I was not
> sure if the current "the entry was NULL from something new, so skip
> and look at the previous entry's new" was correct to begin with.

I'm not sure it makes sense for an entry to have its "after" state as its own parent. On the other hand, I'm not sure it makes sense to consider this fake parentage in the first place.

I think in the old days we used truly rewrite the parents, and traversal like "git log -g -p" would show the diff between reflog entries. But that changed in 838f9a156 (log: use true parents for diff when walking reflogs, 2013-08-03). Given that we disable reflog-walking with things like "--graph" that show the relationship, are the fake parents actually accomplishing anything anymore? The parents we show via "log -g --parents" seem like nonsense, and I'm not sure how we would do any history simplification based no the munged values.

So I'd be tempted to just ditch the whole thing and teach get_revision_1() to just walk through the list of logs, rather than this weird "add a pending commit and then try to figure out which reflog it referred to". For instance, right now:

  git log -g HEAD $(git symbolic-ref HEAD)

only shows _one_ reflog. The patch below is the direction I'm thinking. It fails two tests, but haven't dug yet.

---
 reflog-walk.c | 112 +++++++++--------------------------
 reflog-walk.h |   4 +-
 revision.c    |  24 ++++----
 3 files changed, 43 insertions(+), 97 deletions(-)
diff --git a/reflog-walk.c b/reflog-walk.c
index ed99437ad..77170a3cb 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -78,45 +78,6 @@ static int get_reflog_recno_by_time(struct complete_reflogs *array,
 	return -1;
 }
 
-struct commit_info_lifo {
-	struct commit_info {
-		struct commit *commit;
-		void *util;
-	} *items;
-	int nr, alloc;
-};
-
-static struct commit_info *get_commit_info(struct commit *commit,
-		struct commit_info_lifo *lifo, int pop)
-{
-	int i;
-	for (i = 0; i < lifo->nr; i++)
-		if (lifo->items[i].commit == commit) {
-			struct commit_info *result = &lifo->items[i];
-			if (pop) {
-				if (i + 1 < lifo->nr)
-					memmove(lifo->items + i,
-						lifo->items + i + 1,
-						(lifo->nr - i) *
-						sizeof(struct commit_info));
-				lifo->nr--;
-			}
-			return result;
-		}
-	return NULL;
-}
-
-static void add_commit_info(struct commit *commit, void *util,
-		struct commit_info_lifo *lifo)
-{
-	struct commit_info *info;
-	ALLOC_GROW(lifo->items, lifo->nr + 1, lifo->alloc);
-	info = lifo->items + lifo->nr;
-	info->commit = commit;
-	info->util = util;
-	lifo->nr++;
-}
-
 struct commit_reflog {
 	int recno;
 	enum selector_type {
@@ -128,7 +89,8 @@ struct commit_reflog {
 };
 
 struct reflog_walk_info {
-	struct commit_info_lifo reflogs;
+	struct commit_reflog **logs;
+	size_t nr, alloc, cur;
 	struct string_list complete_reflogs;
 	struct commit_reflog *last_commit_reflog;
 };
@@ -226,50 +188,10 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
 	commit_reflog->selector = selector;
 	commit_reflog->reflogs = reflogs;
 
-	add_commit_info(commit, commit_reflog, &info->reflogs);
-	return 0;
-}
-
-void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)
-{
-	struct commit_info *commit_info =
-		get_commit_info(commit, &info->reflogs, 0);
-	struct commit_reflog *commit_reflog;
-	struct object *logobj;
-	struct reflog_info *reflog;
-
-	info->last_commit_reflog = NULL;
-	if (!commit_info)
-		return;
+	ALLOC_GROW(info->logs, info->nr + 1, info->alloc);
+	info->logs[info->nr++] = commit_reflog;
 
-	commit_reflog = commit_info->util;
-	if (commit_reflog->recno < 0) {
-		commit->parents = NULL;
-		return;
-	}
-	info->last_commit_reflog = commit_reflog;
-
-	do {
-		reflog = &commit_reflog->reflogs->items[commit_reflog->recno];
-		commit_reflog->recno--;
-		logobj = parse_object(&reflog->ooid);
-	} while (commit_reflog->recno && (logobj && logobj->type != OBJ_COMMIT));
-
-	if (!logobj && commit_reflog->recno >= 0 && is_null_oid(&reflog->ooid)) {
-		/* a root commit, but there are still more entries to show */
-		reflog = &commit_reflog->reflogs->items[commit_reflog->recno];
-		logobj = parse_object(&reflog->noid);
-	}
-
-	if (!logobj || logobj->type != OBJ_COMMIT) {
-		commit_info->commit = NULL;
-		commit->parents = NULL;
-		return;
-	}
-	commit_info->commit = (struct commit *)logobj;
-
-	commit->parents = xcalloc(1, sizeof(struct commit_list));
-	commit->parents->item = commit_info->commit;
+	return 0;
 }
 
 void get_reflog_selector(struct strbuf *sb,
@@ -356,3 +278,27 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,
 		strbuf_release(&selector);
 	}
 }
+
+struct commit *next_entry_in_log(struct commit_reflog *log)
+{
+	while (log->recno >= 0) {
+		struct reflog_info *entry = &log->reflogs->items[log->recno--];
+		struct object *obj = parse_object(&entry->noid);
+
+		if (obj && obj->type == OBJ_COMMIT)
+			return (struct commit *)obj;
+	}
+	return NULL;
+}
+
+struct commit *next_reflog_entry(struct reflog_walk_info *walk)
+{
+	for (; walk->cur < walk->nr; walk->cur++) {
+		struct commit *ret = next_entry_in_log(walk->logs[walk->cur]);
+		if (ret) {
+			walk->last_commit_reflog = walk->logs[walk->cur];
+			return ret;
+		}
+	}
+	return NULL;
+}
diff --git a/reflog-walk.h b/reflog-walk.h
index 27886f793..5cc6b7bd4 100644
--- a/reflog-walk.h
+++ b/reflog-walk.h
@@ -8,8 +8,6 @@ struct reflog_walk_info;
 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);
-extern void fake_reflog_parent(struct reflog_walk_info *info,
-		struct commit *commit);
 extern void show_reflog_message(struct reflog_walk_info *info, int,
 				const struct date_mode *, int force_date);
 extern void get_reflog_message(struct strbuf *sb,
@@ -20,4 +18,6 @@ extern void get_reflog_selector(struct strbuf *sb,
 		const struct date_mode *dmode, int force_date,
 		int shorten);
 
+struct commit *next_reflog_entry(struct reflog_walk_info *reflog_info);
+
 #endif
diff --git a/revision.c b/revision.c
index 12eb33235..675247cd9 100644
--- a/revision.c
+++ b/revision.c
@@ -148,16 +148,14 @@ static void add_pending_object_with_path(struct rev_info *revs,
 	if (revs->reflog_info && obj->type == OBJ_COMMIT) {
 		struct strbuf buf = STRBUF_INIT;
 		int len = interpret_branch_name(name, 0, &buf, 0);
-		int st;
 
 		if (0 < len && name[len] && buf.len)
 			strbuf_addstr(&buf, name + len);
-		st = add_reflog_for_walk(revs->reflog_info,
-					 (struct commit *)obj,
-					 buf.buf[0] ? buf.buf: name);
+		add_reflog_for_walk(revs->reflog_info,
+				    (struct commit *)obj,
+				    buf.buf[0] ? buf.buf: name);
 		strbuf_release(&buf);
-		if (st)
-			return;
+		return; /* do not add the commit itself */
 	}
 	add_object_array_with_path(obj, name, &revs->pending, mode, path);
 }
@@ -3114,18 +3112,20 @@ static void track_linear(struct rev_info *revs, struct commit *commit)
 
 static struct commit *get_revision_1(struct rev_info *revs)
 {
+	if (revs->reflog_info) {
+		struct commit *commit = next_reflog_entry(revs->reflog_info);
+		if (commit) {
+			commit->object.flags &= ~(ADDED | SEEN | SHOWN);
+			return commit;
+		}
+	}
+
 	if (!revs->commits)
 		return NULL;
 
 	do {
 		struct commit *commit = pop_commit(&revs->commits);
 
-		if (revs->reflog_info) {
-			save_parents(revs, commit);
-			fake_reflog_parent(revs->reflog_info, commit);
-			commit->object.flags &= ~(ADDED | SEEN | SHOWN);
-		}
-
 		/*
 		 * If we haven't done the list limiting, we need to look at
 		 * the parents here. We also need to do the date-based limiting
Previous: Junio C HamanoNext: Junio C Hamano
Message 9 of 69 in “Truncating HEAD reflog on branch move”
  1. brian m. carlsonJun 21, 2017
  2. Junio C HamanoJun 21, 2017
  3. Kyle MeyerJun 21, 2017
  4. Kyle MeyerJun 21, 2017
  5. Jeff KingJun 22, 2017
  6. Junio C HamanoJun 22, 2017
  7. Jeff KingJun 22, 2017
  8. Junio C HamanoJun 22, 2017
  9. Jeff KingJun 22, 2017
  10. Junio C HamanoJun 22, 2017
  11. Jeff KingJun 22, 2017
  12. Jeff KingJun 22, 2017
  13. Jeff KingJun 23, 2017
  14. brian m. carlsonJul 4, 2017
  15. Jeff KingJul 4, 2017
  16. brian m. carlsonJul 4, 2017
  17. Jeff KingJul 4, 2017
  18. Junio C HamanoJul 5, 2017
  19. 0/6 fixing reflog-walk odditiesJeff King, Jul 5, 2017
  20. 1/6 reflog-walk: skip over double-null oid due to HEAD renameJeff King, Jul 5, 2017
  21. Junio C HamanoJul 5, 2017
  22. Jeff KingJul 5, 2017
  23. 2/6 t1414: document some reflog-walk odditiesJeff King, Jul 5, 2017
  24. Junio C HamanoJul 5, 2017
  25. Jeff KingJul 5, 2017
  26. Junio C HamanoJul 5, 2017
  27. Jeff KingJul 6, 2017
  28. Jeff KingJul 6, 2017
  29. Junio C HamanoJul 6, 2017
  30. Jeff KingJul 7, 2017
  31. Junio C HamanoJul 7, 2017
  32. Jeff KingJul 7, 2017
  33. Ramsay JonesJul 5, 2017
  34. Jeff KingJul 5, 2017
  35. 3/6 log: do not free parents when walking reflogJeff King, Jul 5, 2017
  36. 4/6 get_revision_1(): replace do-while with an early returnJeff King, Jul 5, 2017
  37. 5/6 rev-list: check reflog_info before showing usageJeff King, Jul 5, 2017
  38. Junio C HamanoJul 5, 2017
  39. Jeff KingJul 5, 2017
  40. 6/6 reflog-walk: stop using fake parentsJeff King, Jul 5, 2017
  41. Eric WongJul 7, 2017
  42. Jeff KingJul 7, 2017
  43. Jeff KingJul 7, 2017
  44. Eric WongJul 10, 2017
  45. Jeff KingJul 10, 2017
  46. Junio C HamanoJul 10, 2017
  47. 0/4 reflog-walk fixes for maintJeff King, Jul 7, 2017
  48. 1/4 reflog-walk: skip over double-null oid due to HEAD renameJeff King, Jul 7, 2017
  49. 2/4 reflog-walk: duplicate strings in complete_reflogs listJeff King, Jul 7, 2017
  50. 3/4 reflog-walk: don't free reflogs added to cacheJeff King, Jul 7, 2017
  51. 4/4 reflog-walk: include all fields when freeing complete_reflogsJeff King, Jul 7, 2017
  52. 0/7 fixing reflog-walk odditiesJeff King, Jul 7, 2017
  53. 1/7 t1414: document some reflog-walk odditiesJeff King, Jul 7, 2017
  54. Jeff KingJul 7, 2017
  55. Kyle MeyerJul 7, 2017
  56. 2/7 revision: disallow reflog walking with revs->limitedJeff King, Jul 7, 2017
  57. 4/7 get_revision_1(): replace do-while with an early returnJeff King, Jul 7, 2017
  58. 5/7 rev-list: check reflog_info before showing usageJeff King, Jul 7, 2017
  59. 3/7 log: do not free parents when walking reflogJeff King, Jul 7, 2017
  60. Junio C HamanoJul 7, 2017
  61. Jeff KingJul 9, 2017
  62. Junio C HamanoJul 9, 2017
  63. Jeff KingJul 10, 2017
  64. 6/7 reflog-walk: stop using fake parentsJeff King, Jul 7, 2017
  65. Jeff KingJul 7, 2017
  66. Kyle MeyerJul 7, 2017
  67. Jeff KingJul 9, 2017
  68. 7/7 reflog-walk: apply --since/--until to reflog datesJeff King, Jul 7, 2017
  69. Junio C HamanoJul 7, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.