From 7cf686b9a89d3cea269dfcced2d32dff56a5f01d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 7 Jul 2017 05:06:10 -0400 Subject: [PATCH 1/8] t1414: document some reflog-walk oddities Since its inception, the general strategy of the reflog-walk code has been to start with the tip commit for the ref, and as we traverse replace each commit's parent pointers with fake parents pointing to the previous reflog entry. This lets us traverse the reflog as if it were a real history, but it has some user-visible oddities. Namely: 1. The fake parents are used for commit selection and display. So for example, "--merges" or "--no-merges" are not useful, because the history appears as a linear string of commits. Likewise, pathspec limiting is based on the diff between adjacent entries, not the changes actually introduced by a commit. These are often the same (e.g., because the entry was just running "git commit" and the adjacent entry _is_ the true parent), but it may not be in several common cases. For instance, using "git reset" to jump around history, or "git checkout" to move HEAD. 2. We reverse-map each commit back to its reflog. So when it comes time to show commit X, we say "a-ha, we added X because it was at the tip of the 'foo' reflog, so let's show the foo reflog". But this leads to nonsense results when you ask to traverse multiple reflogs: if two reflogs have the same tip commit, we only map back to one of them. Instead, we should show both. 3. If the tip of the reflog and the ref tip disagree on the current value, we show the ref tip but give no indication of the value in the reflog. This situation isn't supposed to happen (since any ref update should touch the reflog). But if it does, given that the requested operation is to show the reflog, it makes sense to prefer that. This commit adds a new script with several expect_failure tests to demonstrate the problems. This could be part of the existing t1411, but it's a bit easier to start from a fresh state, where we know exactly what will be in the log. Since the new multiple-reflog tests are checking the actual output, we can drop the "make sure we don't segfault" tests from t1411, which are a strict subset of what we're doing here. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1411-reflog-show.sh | 10 ---- t/t1414-reflog-walk.sh | 105 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 10 deletions(-) create mode 100755 t/t1414-reflog-walk.sh diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh index b9cb76654b..6ac7734d79 100755 --- a/t/t1411-reflog-show.sh +++ b/t/t1411-reflog-show.sh @@ -171,14 +171,4 @@ 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_expect_success 'showing multiple reflogs with an old date' ' - git log -g HEAD@{1979-01-01} HEAD >actual -' - test_done diff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh new file mode 100755 index 0000000000..945aa089ec --- /dev/null +++ b/t/t1414-reflog-walk.sh @@ -0,0 +1,105 @@ +#!/bin/sh + +test_description='various tests of reflog walk (log -g) behavior' +. ./test-lib.sh + +test_expect_success 'set up some reflog entries' ' + test_commit one && + test_commit two && + git checkout -b side HEAD^ && + test_commit three && + git merge --no-commit master && + echo evil-merge-content >>one.t && + test_tick && + git commit --no-edit -a +' + +do_walk () { + git log -g --format="%gd %gs" "$@" +} + +sq="'" +test_expect_success 'set up expected reflog' ' + cat >expect.all <<-EOF + HEAD@{0} commit (merge): Merge branch ${sq}master${sq} into side + HEAD@{1} commit: three + HEAD@{2} checkout: moving from master to side + HEAD@{3} commit: two + HEAD@{4} commit (initial): one + EOF +' + +test_expect_success 'reflog walk shows expected logs' ' + do_walk >actual && + test_cmp expect.all actual +' + +test_expect_failure 'reflog can limit with --no-merges' ' + grep -v merge expect.all >expect && + do_walk --no-merges >actual && + test_cmp expect actual +' + +test_expect_failure 'reflog can limit with pathspecs' ' + grep two expect.all >expect && + do_walk -- two.t >actual && + test_cmp expect actual +' + +test_expect_failure 'pathspec limiting handles merges' ' + # we pick up: + # - the initial commit of one + # - the checkout back to commit one + # - the evil merge which touched one + sed -n "1p;3p;5p" expect.all >expect && + do_walk -- one.t >actual && + test_cmp expect actual +' + +test_expect_failure '--parents shows true parents' ' + # convert newlines to spaces + echo $(git rev-parse HEAD HEAD^1 HEAD^2) >expect && + git rev-list -g --parents -1 HEAD >actual && + test_cmp expect actual +' + +test_expect_failure 'walking multiple reflogs shows all' ' + # We expect to see all entries for all reflogs, but interleaved by + # date, with order on the command line breaking ties. We + # can use "sort" on the separate lists to generate this, + # but note two tricks: + # + # 1. We use "{" as the delimiter, which lets us skip to the reflog + # date specifier as our second field, and then our "-n" numeric + # sort ignores the bits after the timestamp. + # + # 2. POSIX leaves undefined whether this is a stable sort or not. So + # we use "-k 1" to ensure that we see HEAD before master before + # side when breaking ties. + { + do_walk --date=unix HEAD && + do_walk --date=unix side && + do_walk --date=unix master + } >expect.raw && + sort -t "{" -k 2nr -k 1 expect && + do_walk --date=unix HEAD master side >actual && + test_cmp expect actual +' + +test_expect_success 'date-limiting does not interfere with other logs' ' + do_walk HEAD@{1979-01-01} HEAD >actual && + test_cmp expect.all actual +' + +test_expect_failure 'walk prefers reflog to ref tip' ' + head=$(git rev-parse HEAD) && + one=$(git rev-parse one) && + ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE" && + echo "$head $one $ident broken reflog entry" >>.git/logs/HEAD && + + echo $one >expect && + git log -g --format=%H -1 >actual && + test_cmp expect actual +' + +test_done From 82fd0f4a4b2c394ddba279eab7072fe87b750d2e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 7 Jul 2017 05:07:16 -0400 Subject: [PATCH 2/8] revision: disallow reflog walking with revs->limited The reflog-walk code doesn't work with limit_list(). That function traverses down the real history graph, not the fake reflog history that get_revision() returns. So it's not going to actually examine all of the commits we're going to show, because we'd add them to the pending list only during the actual traversal. In practice this limitation doesn't really matter, because the options that require list-limiting generally need UNINTERESTING endpoints or symmetric ranges, which already are forbidden for reflog walks. Still, there are likely some corner cases that would behave oddly. We're better off to warn the user that we can't fulfill their request than to generate potentially wrong output. This will also make it easier to refactor the reflog-walking code, because it eliminates a whole area of corner cases we'd have to consider (that already don't work anyway). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- revision.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/revision.c b/revision.c index e181ad1b70..6678de04d9 100644 --- a/revision.c +++ b/revision.c @@ -2366,6 +2366,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s if (revs->reverse && revs->reflog_info) die("cannot combine --reverse with --walk-reflogs"); + if (revs->reflog_info && revs->limited) + die("cannot combine --walk-reflogs with history-limiting options"); if (revs->rewrite_parents && revs->children.name) die("cannot combine --parents and --children"); From 822601e8303270aebed493b11e0988a75f2646b8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 9 Jul 2017 06:13:51 -0400 Subject: [PATCH 3/8] log: clarify comment about reflog cycles When we're walking reflogs, we leave the commit buffer and parents in place. A comment explains that this is due to "cycles". But the interesting thing is the unsaid implication: that the cycles (plus our clearing of the SEEN flag) will cause us to show commits multiple times. Let's spell it out. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/log.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/builtin/log.c b/builtin/log.c index 8ca1de9894..cf303b2c06 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -372,7 +372,10 @@ static int cmd_log_walk(struct rev_info *rev) */ rev->max_count++; if (!rev->reflog_info) { - /* we allow cycles in reflog ancestry */ + /* + * We may show a given commit multiple times when + * walking the reflogs. + */ free_commit_buffer(commit); } free_commit_list(commit->parents); From f35650dff6a4500e317803165b13cc087f48ee85 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 7 Jul 2017 05:07:34 -0400 Subject: [PATCH 4/8] log: do not free parents when walking reflog When we're doing a reflog walk (instead of walking the actual parent pointers), we may see commits multiple times. For this reason, we hold on to the commit buffer for each commit rather than freeing it after we've showed the commit. We should do the same for the parent list. Right now this is just a minor optimization. But once we refactor how reflog walks are performed, keeping the parents will avoid confusing us the second time we see the commit. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/log.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index cf303b2c06..630d6cff2f 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -377,9 +377,9 @@ static int cmd_log_walk(struct rev_info *rev) * walking the reflogs. */ free_commit_buffer(commit); + free_commit_list(commit->parents); + commit->parents = NULL; } - free_commit_list(commit->parents); - commit->parents = NULL; if (saved_nrl < rev->diffopt.needed_rename_limit) saved_nrl = rev->diffopt.needed_rename_limit; if (rev->diffopt.degraded_cc_to_c) From 7c2f08aa7a26e68475abe5c9fd7250aacbb6b7b2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 7 Jul 2017 05:07:58 -0400 Subject: [PATCH 5/8] get_revision_1(): replace do-while with an early return The get_revision_1() function tries to avoid entering its main loop at all when there are no commits to look at. But it's perfectly safe to call pop_commit() on an empty list (in which case it will return NULL). Switching to an early return from the loop lets us skip repeating the loop condition before we enter the do-while. That will get more important when we start pulling reflog-walk commits from a source besides the revs->commits queue, as that condition will get much more complicated. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- revision.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/revision.c b/revision.c index 6678de04d9..4019e8cf23 100644 --- a/revision.c +++ b/revision.c @@ -3111,12 +3111,12 @@ static void track_linear(struct rev_info *revs, struct commit *commit) static struct commit *get_revision_1(struct rev_info *revs) { - if (!revs->commits) - return NULL; - - do { + while (1) { struct commit *commit = pop_commit(&revs->commits); + if (!commit) + return NULL; + if (revs->reflog_info) { save_parents(revs, commit); fake_reflog_parent(revs->reflog_info, commit); @@ -3150,8 +3150,7 @@ static struct commit *get_revision_1(struct rev_info *revs) track_linear(revs, commit); return commit; } - } while (revs->commits); - return NULL; + } } /* From 7f97de5ee1e1f9d28f45c8f7890e752f7b12bed1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 7 Jul 2017 05:08:30 -0400 Subject: [PATCH 6/8] rev-list: check reflog_info before showing usage When git-rev-list sees no pending commits, it shows a usage message. This works even when reflog-walking is requested, because the reflog-walk code currently puts the reflog tips into the pending queue. In preparation for refactoring the reflog-walk code, let's explicitly check whether we have any reflogs to walk. For now this is a noop, but the existing reflog tests will make sure that it kicks in after the refactoring. Likewise, we'll add a test that "rev-list -g" without specifying any reflogs continues to fail (so that we know our check does not kick in too aggressively). Note that the implementation needs to go into its own sub-function, as the walk code does not expose its innards outside of reflog-walk.c. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/rev-list.c | 3 ++- reflog-walk.c | 5 +++++ reflog-walk.h | 2 ++ t/t1414-reflog-walk.sh | 4 ++++ 4 files changed, 13 insertions(+), 1 deletion(-) diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 95d84d5cda..53a746dd89 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -11,6 +11,7 @@ #include "graph.h" #include "bisect.h" #include "progress.h" +#include "reflog-walk.h" static const char rev_list_usage[] = "git rev-list [OPTION] ... [ -- paths... ]\n" @@ -348,7 +349,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) /* Only --header was specified */ revs.commit_format = CMIT_FMT_RAW; - if ((!revs.commits && + if ((!revs.commits && reflog_walk_empty(revs.reflog_info) && (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) && !revs.pending.nr)) || revs.diff) diff --git a/reflog-walk.c b/reflog-walk.c index 081f89b70d..98c2f42de9 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -365,3 +365,8 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline, strbuf_release(&selector); } } + +int reflog_walk_empty(struct reflog_walk_info *info) +{ + return !info || !info->reflogs.nr; +} diff --git a/reflog-walk.h b/reflog-walk.h index 27886f793e..af32361072 100644 --- a/reflog-walk.h +++ b/reflog-walk.h @@ -20,4 +20,6 @@ extern void get_reflog_selector(struct strbuf *sb, const struct date_mode *dmode, int force_date, int shorten); +extern int reflog_walk_empty(struct reflog_walk_info *walk); + #endif diff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh index 945aa089ec..8bda862ca7 100755 --- a/t/t1414-reflog-walk.sh +++ b/t/t1414-reflog-walk.sh @@ -102,4 +102,8 @@ test_expect_failure 'walk prefers reflog to ref tip' ' test_cmp expect actual ' +test_expect_success 'rev-list -g complains when there are no reflogs' ' + test_must_fail git rev-list -g +' + test_done From d08565bf2dd78aa34c68c94f1931539c1c8c5322 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 7 Jul 2017 05:14:07 -0400 Subject: [PATCH 7/8] reflog-walk: stop using fake parents The reflog-walk system works by putting a ref's tip into the pending queue, and then "traversing" the reflog by pretending that the parent of each commit is the previous reflog entry. This causes a number of user-visible oddities, as documented in t1414 (and the commit message which introduced it). We can fix all of them in one go by replacing the fake-reflog system with a much simpler one: just keeping a list of reflogs to show, and walking through them entry by entry. The implementation is fairly straight-forward, but there are a few items to note: 1. We obviously must skip calling add_parents_to_list() when we are traversing reflogs, since we do not want to walk the original parents at all. As a result, we must call try_to_simplify_commit() ourselves. There are other parts of add_parents_to_list() we skip, as well, but none of them should matter for a reflog traversal: - We do not allow UNINTERESTING commits, nor symmetric ranges (and we bail when these are used with "-g"). - Using --source makes no sense, since we aren't traversing. The reflog selector shows the same information with more detail. - Using --first-parent is still sensible, since you may want to see the first-parent diff for each entry. But since we're not traversing, we don't need to cull the parent list here. 2. Since we now just walk the reflog entries themselves, rather than starting with the ref tip, we now look at the "new" field of each entry rather than the "old" (i.e., we are showing entries, not faking parents). This removes all of the tricky logic around skipping past root commits. But note that we have no way to show an entry with the null sha1 in its "new" field (because such a commit obviously does not exist). Normally this would not happen, since we delete reflogs along with refs, but there is one special case. When we rename the currently checked out branch, we write two reflog entries into the HEAD log: one where the commit goes away, and another where it comes back. Prior to this commit, we show both entries with identical reflog messages. After this commit, we show only the "comes back" entry. See the update in t3200 which demonstrates this. Arguably either is fine, as the whole double-entry thing is a bit hacky in the first place. And until a recent fix, we truncated the traversal in such a case anyway, which was _definitely_ wrong. 3. We show individual reflogs in order, but choose which reflog to show at each stage based on which has the most recent timestamp. This interleaves the output from multiple reflogs based on date order, which is probably what you'd want with limiting like "-n 30". Note that the implementation aims for simplicity. It does a linear walk over the reflog queue for each commit it pulls, which may perform badly if you interleave an enormous number of reflogs. That seems like an unlikely use case; if we did want to handle it, we could probably keep a priority queue of reflogs, ordered by the timestamp of their current tip entry. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- reflog-walk.c | 137 +++++++++++++++-------------------------- reflog-walk.h | 4 +- revision.c | 27 ++++---- t/t1414-reflog-walk.sh | 12 ++-- t/t3200-branch.sh | 3 +- 5 files changed, 75 insertions(+), 108 deletions(-) diff --git a/reflog-walk.c b/reflog-walk.c index 98c2f42de9..fbee9e0126 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -94,45 +94,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 { @@ -144,7 +105,8 @@ struct commit_reflog { }; struct reflog_walk_info { - struct commit_info_lifo reflogs; + struct commit_reflog **logs; + size_t nr, alloc; struct string_list complete_reflogs; struct commit_reflog *last_commit_reflog; }; @@ -233,54 +195,12 @@ 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); + ALLOC_GROW(info->logs, info->nr + 1, info->alloc); + info->logs[info->nr++] = commit_reflog; + 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; - - 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 = parse_object(&reflog->ooid); - } - - 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; -} - void get_reflog_selector(struct strbuf *sb, struct reflog_walk_info *reflog_info, const struct date_mode *dmode, int force_date, @@ -368,5 +288,50 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline, int reflog_walk_empty(struct reflog_walk_info *info) { - return !info || !info->reflogs.nr; + return !info || !info->nr; +} + +static struct commit *next_reflog_commit(struct commit_reflog *log) +{ + for (; log->recno >= 0; log->recno--) { + 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; +} + +static timestamp_t log_timestamp(struct commit_reflog *log) +{ + return log->reflogs->items[log->recno].timestamp; +} + +struct commit *next_reflog_entry(struct reflog_walk_info *walk) +{ + struct commit_reflog *best = NULL; + struct commit *best_commit = NULL; + size_t i; + + for (i = 0; i < walk->nr; i++) { + struct commit_reflog *log = walk->logs[i]; + struct commit *commit = next_reflog_commit(log); + + if (!commit) + continue; + + if (!best || log_timestamp(log) > log_timestamp(best)) { + best = log; + best_commit = commit; + } + } + + if (best) { + best->recno--; + walk->last_commit_reflog = best; + return best_commit; + } + + return NULL; } diff --git a/reflog-walk.h b/reflog-walk.h index af32361072..373388cd14 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, @@ -22,4 +20,6 @@ extern void get_reflog_selector(struct strbuf *sb, extern int reflog_walk_empty(struct reflog_walk_info *walk); +struct commit *next_reflog_entry(struct reflog_walk_info *reflog_info); + #endif diff --git a/revision.c b/revision.c index 4019e8cf23..587199739a 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); } @@ -3112,16 +3110,18 @@ static void track_linear(struct rev_info *revs, struct commit *commit) static struct commit *get_revision_1(struct rev_info *revs) { while (1) { - struct commit *commit = pop_commit(&revs->commits); + struct commit *commit; + + if (revs->reflog_info) + commit = next_reflog_entry(revs->reflog_info); + else + commit = pop_commit(&revs->commits); if (!commit) return NULL; - if (revs->reflog_info) { - save_parents(revs, commit); - fake_reflog_parent(revs->reflog_info, commit); + if (revs->reflog_info) commit->object.flags &= ~(ADDED | SEEN | SHOWN); - } /* * If we haven't done the list limiting, we need to look at @@ -3132,7 +3132,10 @@ static struct commit *get_revision_1(struct rev_info *revs) if (revs->max_age != -1 && (commit->date < revs->max_age)) continue; - if (add_parents_to_list(revs, commit, &revs->commits, NULL) < 0) { + + if (revs->reflog_info) + try_to_simplify_commit(revs, commit); + else if (add_parents_to_list(revs, commit, &revs->commits, NULL) < 0) { if (!revs->ignore_missing_links) die("Failed to traverse parents of commit %s", oid_to_hex(&commit->object.oid)); diff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh index 8bda862ca7..eb10fefd40 100755 --- a/t/t1414-reflog-walk.sh +++ b/t/t1414-reflog-walk.sh @@ -34,19 +34,19 @@ test_expect_success 'reflog walk shows expected logs' ' test_cmp expect.all actual ' -test_expect_failure 'reflog can limit with --no-merges' ' +test_expect_success 'reflog can limit with --no-merges' ' grep -v merge expect.all >expect && do_walk --no-merges >actual && test_cmp expect actual ' -test_expect_failure 'reflog can limit with pathspecs' ' +test_expect_success 'reflog can limit with pathspecs' ' grep two expect.all >expect && do_walk -- two.t >actual && test_cmp expect actual ' -test_expect_failure 'pathspec limiting handles merges' ' +test_expect_success 'pathspec limiting handles merges' ' # we pick up: # - the initial commit of one # - the checkout back to commit one @@ -56,14 +56,14 @@ test_expect_failure 'pathspec limiting handles merges' ' test_cmp expect actual ' -test_expect_failure '--parents shows true parents' ' +test_expect_success '--parents shows true parents' ' # convert newlines to spaces echo $(git rev-parse HEAD HEAD^1 HEAD^2) >expect && git rev-list -g --parents -1 HEAD >actual && test_cmp expect actual ' -test_expect_failure 'walking multiple reflogs shows all' ' +test_expect_success 'walking multiple reflogs shows all' ' # We expect to see all entries for all reflogs, but interleaved by # date, with order on the command line breaking ties. We # can use "sort" on the separate lists to generate this, @@ -91,7 +91,7 @@ test_expect_success 'date-limiting does not interfere with other logs' ' test_cmp expect.all actual ' -test_expect_failure 'walk prefers reflog to ref tip' ' +test_expect_success 'walk prefers reflog to ref tip' ' head=$(git rev-parse HEAD) && one=$(git rev-parse one) && ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE" && diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index dd37ac47c5..9d707d2a40 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -166,10 +166,9 @@ 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 && + git log -g --format="%gd %H %gs" -2 HEAD >actual && test_cmp expect actual ' From de239446b69f3b453050af8091e07aa5433421cc Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 7 Jul 2017 05:16:21 -0400 Subject: [PATCH 8/8] reflog-walk: apply --since/--until to reflog dates When doing a reflog walk, we use the commit's date to do any date limiting. In earlier versions of Git, this could lead to nonsense results, since a skipped commit would truncate the traversal. So a sequence like: git commit ... git checkout week-old-branch git checkout - git log -g --since=1.day.ago would stop at the week-old-branch, even though the "git commit" entry further back is still interesting. As of the prior commit, which uses a parent-less traversal of the reflog, you get the whole reflog minus any commits whose dates do not match the specified options. This is arguably useful, as you could scan the reflogs for commits that originated in a certain range. But more likely a user doing a reflog walk wants to limit based on the reflog entries themselves. You can simulate --until with: git log -g @{1.day.ago} but there's no way to ask Git to traverse only back to a certain date. E.g.: # show me reflog entries from the past day git log -g --since=1.day.ago This patch teaches the revision machinery to prefer the reflog entry dates to the commit dates when doing a reflog walk. Technically this is a change in behavior that affects plumbing, but the previous behavior was so buggy that it's unlikely anyone was relying on it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- reflog-walk.c | 12 ++++++++++++ reflog-walk.h | 1 + revision.c | 19 ++++++++++++++++--- t/t1414-reflog-walk.sh | 26 ++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 3 deletions(-) diff --git a/reflog-walk.c b/reflog-walk.c index fbee9e0126..74ebe5148f 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -264,6 +264,18 @@ const char *get_reflog_ident(struct reflog_walk_info *reflog_info) return info->email; } +timestamp_t get_reflog_timestamp(struct reflog_walk_info *reflog_info) +{ + struct commit_reflog *commit_reflog = reflog_info->last_commit_reflog; + struct reflog_info *info; + + if (!commit_reflog) + return 0; + + info = &commit_reflog->reflogs->items[commit_reflog->recno+1]; + return info->timestamp; +} + void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline, const struct date_mode *dmode, int force_date) { diff --git a/reflog-walk.h b/reflog-walk.h index 373388cd14..7553c448fe 100644 --- a/reflog-walk.h +++ b/reflog-walk.h @@ -13,6 +13,7 @@ extern void show_reflog_message(struct reflog_walk_info *info, int, extern void get_reflog_message(struct strbuf *sb, struct reflog_walk_info *reflog_info); extern const char *get_reflog_ident(struct reflog_walk_info *reflog_info); +extern timestamp_t get_reflog_timestamp(struct reflog_walk_info *reflog_info); extern void get_reflog_selector(struct strbuf *sb, struct reflog_walk_info *reflog_info, const struct date_mode *dmode, int force_date, diff --git a/revision.c b/revision.c index 587199739a..41b4375c3c 100644 --- a/revision.c +++ b/revision.c @@ -2965,6 +2965,18 @@ static inline int want_ancestry(const struct rev_info *revs) return (revs->rewrite_parents || revs->children.name); } +/* + * Return a timestamp to be used for --since/--until comparisons for this + * commit, based on the revision options. + */ +static timestamp_t comparison_date(const struct rev_info *revs, + struct commit *commit) +{ + return revs->reflog_info ? + get_reflog_timestamp(revs->reflog_info) : + commit->date; +} + enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit) { if (commit->object.flags & SHOWN) @@ -2975,8 +2987,9 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi return commit_show; if (commit->object.flags & UNINTERESTING) return commit_ignore; - if (revs->min_age != -1 && (commit->date > revs->min_age)) - return commit_ignore; + if (revs->min_age != -1 && + comparison_date(revs, commit) > revs->min_age) + return commit_ignore; if (revs->min_parents || (revs->max_parents >= 0)) { int n = commit_list_count(commit->parents); if ((n < revs->min_parents) || @@ -3130,7 +3143,7 @@ static struct commit *get_revision_1(struct rev_info *revs) */ if (!revs->limited) { if (revs->max_age != -1 && - (commit->date < revs->max_age)) + comparison_date(revs, commit) < revs->max_age) continue; if (revs->reflog_info) diff --git a/t/t1414-reflog-walk.sh b/t/t1414-reflog-walk.sh index eb10fefd40..feb1efd8ff 100755 --- a/t/t1414-reflog-walk.sh +++ b/t/t1414-reflog-walk.sh @@ -91,6 +91,32 @@ test_expect_success 'date-limiting does not interfere with other logs' ' test_cmp expect.all actual ' +test_expect_success 'min/max age uses entry date to limit' ' + # Flip between commits one and two so each ref update actually + # does something (and does not get optimized out). We know + # that the timestamps of those commits will be before our "min". + + git update-ref -m before refs/heads/minmax one && + + test_tick && + min=$test_tick && + git update-ref -m min refs/heads/minmax two && + + test_tick && + max=$test_tick && + git update-ref -m max refs/heads/minmax one && + + test_tick && + git update-ref -m after refs/heads/minmax two && + + cat >expect <<-\EOF && + max + min + EOF + git log -g --since=$min --until=$max --format=%gs minmax >actual && + test_cmp expect actual +' + test_expect_success 'walk prefers reflog to ref tip' ' head=$(git rev-parse HEAD) && one=$(git rev-parse one) &&