From 2272d3e5426a65f3fdf456f8e1bfbea40d037a59 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 5 Jul 2017 03:57:37 -0400 Subject: reflog-walk: skip over double-null oid due to HEAD rename Since 39ee4c6c2f (branch: record creation of renamed branch in HEAD's log, 2017-02-20), a rename on the currently checked out branch will create two entries in the HEAD reflog: one where the branch goes away (switching to the null oid), and one where it comes back (switching away from the null oid). This confuses the reflog-walk code. When walking backwards, it first sees the null oid in the "old" field of the second entry. Thanks to the "root commit" logic added by 71abeb753f (reflog: continue walking the reflog past root commits, 2016-06-03), we keep looking for the next entry by scanning the "new" field from the previous entry. But that field is also null! We need to go just a tiny bit further, and look at its "old" field. But with the current code, we decide the reflog has nothing else to show and just give up. To the user this looks like the reflog was truncated by the rename operation, when in fact those entries are still there. This patch does the absolute minimal fix, which is to look back that one extra level and keep traversing. The resulting behavior may not be the _best_ thing to do in the long run (for example, we show both reflog entries each with the same commit id), but it's a simple way to fix the problem without risking further regressions. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t3200-branch.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 't') diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 48d152b9a9..dd37ac47c5 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -162,6 +162,17 @@ test_expect_success 'git branch -M baz bam should add entries to .git/logs/HEAD' grep "^0\{40\}.*$msg$" .git/logs/HEAD ' +test_expect_success 'resulting reflog can be shown by log -g' ' + oid=$(git rev-parse HEAD) && + cat >expect <<-EOF && + HEAD@{0} $oid $msg + HEAD@{1} $oid $msg + HEAD@{2} $oid checkout: moving from foo to baz + EOF + git log -g --format="%gd %H %gs" -3 HEAD >actual && + test_cmp expect actual +' + test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' ' git checkout master && git worktree add -b baz bazdir && -- cgit v1.2.3 From 75afe7ac8728128873c001d82b02ada4a3aa8181 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 7 Jul 2017 04:39:50 -0400 Subject: reflog-walk: duplicate strings in complete_reflogs list As part of the add_reflog_to_walk() function, we keep a string_list mapping refnames to their reflog contents. This serves as a cache so that accessing the same reflog twice requires only a single copy of the log in memory. The string_list is initialized via xcalloc, meaning its strdup_strings field is set to 0. But after inserting a string into the list, we unconditionally call free() on the string, leaving the list pointing to freed memory. If another reflog is added (e.g., "git log -g HEAD HEAD"), then the second one may have unpredictable results. The extra free was added by 5026b47175 (add_reflog_for_walk: avoid memory leak, 2017-05-04). Though if you look carefully, you can see that the code was buggy even before then. If we tried to read the reflogs by time but came up with no entries, we exited with an error, freeing the string in that code path. So the bug was harder to trigger, but still there. We can fix it by just asking the string list to make a copy of the string. Technically we could fix the problem by not calling free() on our string (and just handing over ownership to the string list), but there are enough conditionals that it's quite hard to figure out which code paths need the free and which do not. Simpler is better here. The new test reliably shows the problem when run with --valgrind or ASAN. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1411-reflog-show.sh | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 't') diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh index 6ac7734d79..ae96eeb66a 100755 --- a/t/t1411-reflog-show.sh +++ b/t/t1411-reflog-show.sh @@ -171,4 +171,10 @@ test_expect_success 'reflog exists works' ' ! git reflog exists refs/heads/nonexistent ' +# The behavior with two reflogs is buggy and the output is in flux; for now +# we're just checking that the program works at all without segfaulting. +test_expect_success 'showing multiple reflogs works' ' + git log -g HEAD HEAD >actual +' + test_done -- cgit v1.2.3 From 8aae3cf7558f00cf3c05636f888cb11e5e548e51 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 7 Jul 2017 04:41:49 -0400 Subject: reflog-walk: don't free reflogs added to cache The add_reflog_for_walk() function keeps a cache mapping refnames to their reflog contents. We use a cached reflog entry if available, and otherwise allocate and store a new one. Since 5026b47175 (add_reflog_for_walk: avoid memory leak, 2017-05-04), when we hit an error parsing a date-based reflog spec, we free the reflog memory but leave the cache entry pointing to the now-freed memory. We can fix this by just leaving the memory intact once it has made it into the cache. This may leave an unused entry in the cache, but that's OK. And it means we also catch a similar situation: we may not have allocated at all in this invocation, but simply be pointing to a cached entry from a previous invocation (which is relying on that entry being present). The new test in t1411 exercises this case and fails when run with --valgrind or ASan. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1411-reflog-show.sh | 4 ++++ 1 file changed, 4 insertions(+) (limited to 't') diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh index ae96eeb66a..b9cb76654b 100755 --- a/t/t1411-reflog-show.sh +++ b/t/t1411-reflog-show.sh @@ -177,4 +177,8 @@ test_expect_success 'showing multiple reflogs works' ' git log -g HEAD HEAD >actual ' +test_expect_success 'showing multiple reflogs with an old date' ' + git log -g HEAD@{1979-01-01} HEAD >actual +' + test_done -- cgit v1.2.3