Re: First/oldest entry in reflog dropped

[Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>]

On Sun, Nov 21, 2010 at 12:35 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Nov 20, 2010 at 10:11:34PM -0500, Martin von Zweigbergk wrote:
>
>> Can someone explain the behavior in the execution below?
>>
>> # I expected this reflog...
>> $ git branch tmp
>> $ git reflog show refs/heads/tmp
>> b60a214 refs/heads/tmp@{0}: branch: Created from master
>>
>> # ... and this one as well...
>> $ git update-ref refs/heads/tmp HEAD^
>> $ git reflog show refs/heads/tmp
>> 7d1a0b8 refs/heads/tmp@{0}:
>> b60a214 refs/heads/tmp@{1}: branch: Created from master
>>
>> # ... but why is the first entry (i.e. "branch: Created from master")
>> # dropped here?
>> $ git update-ref refs/heads/tmp HEAD
>> $ git reflog show refs/heads/tmp
>> b60a214 refs/heads/tmp@{0}:
>> 7d1a0b8 refs/heads/tmp@{1}:
>>
>> If the ref is updated once more (to e.g. HEAD^^) before being moved back
>> to HEAD, the first entry will be shown in the output.
>>
>> If this is a bug, it seems to be in reflog, rather than in update-ref,
>> because the first entry does exist in .git/logs/refs/heads/tmp.
>
> I think it's a bug in the reflog-walking machinery, which is sort of
> bolted onto the regular revision traversal machinery. When we hit
> b60a214 the first time, we show it and set the SHOWN flag (since the
> normal traversal machinery would not want to show a commit twice). When
> we hit it again, simplify_commit() sees that it is SHOWN and tells us to
> skip it.
>
> However, the bolted-on reflog-walking machinery does have a way of
> handling this. While we are traversing via get_revision(), we notice
> that we are doing a reflog walk and call fake_reflog_parent. This
> function is responsible for replacing the actual parents of the commit
> with a fake list consisting of the previous reflog entry (so we
> basically pretend that the history consists of a string of commits, each
> one pointing to the previous reflog entry, not the actual parent).
>
> This function _also_ clears some flags, including the SHOWN flag, in
> what almost seems like a tacked-on side effect. So if we hit the same
> commit twice, we will actually show it again. Which is what makes
> reflogs with repeated commits work at all.
>
> However, there is a subtle bug: it clears the flags at the very end of
> the function. But through the function, if we see that there are no fake
> parents (because we are on the very first reflog entry), we do an early
> return. But we not only skip the later "set up parents" code, we also
> accidentally skip the "clear SHOWN flag" side-effect code.
>
> So I believe we will always fail to show the very first reflog if it is
> a repeated commit.
>
> The fix, AFAICT, is to just move the flag clearing above the early
> returns (patch below). But I have to admit I do not quite understand
> what the ADDED and SEEN flags are doing here, as this is the first time
> I have ever looked at the reflog-walk code. So possibly just the SHOWN
> flag should be unconditionally cleared.
>
> This patch clears up your bug, and doesn't break any tests. But I'd
> really like to get a second opinion on the significance of those other
> flags, or why the flag clearing was at the bottom of the function in the
> first place.
>
> -Peff
>
> ---
> diff --git a/reflog-walk.c b/reflog-walk.c
> index 4879615..d5d055b 100644
> --- a/reflog-walk.c
> +++ b/reflog-walk.c
> @@ -210,44 +210,45 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
>        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 reflog_info *reflog;
>
> +       commit->object.flags &= ~(ADDED | SEEN | SHOWN);
> +
>        info->last_commit_reflog = NULL;
>        if (!commit_info)
>                return;
>
>        commit_reflog = commit_info->util;
>        if (commit_reflog->recno < 0) {
>                commit->parents = NULL;
>                return;
>        }
>
>        reflog = &commit_reflog->reflogs->items[commit_reflog->recno];
>        info->last_commit_reflog = commit_reflog;
>        commit_reflog->recno--;
>        commit_info->commit = (struct commit *)parse_object(reflog->osha1);
>        if (!commit_info->commit) {
>                commit->parents = NULL;
>                return;
>        }
>
>        commit->parents = xcalloc(sizeof(struct commit_list), 1);
>        commit->parents->item = commit_info->commit;
> -       commit->object.flags &= ~(ADDED | SEEN | SHOWN);
>  }
>
>  void get_reflog_selector(struct strbuf *sb,
>                         struct reflog_walk_info *reflog_info,
>                         enum date_mode dmode,
>                         int shorten)
>  {
>        struct commit_reflog *commit_reflog = reflog_info->last_commit_reflog;
>        struct reflog_info *info;
>        const char *printed_ref;
>
>

Good job! And yes, if the first commit is included in any cycles, it
seems to be dropped. I must have been blind yesterday...