On Sun, Nov 21, 2010 at 12:35 AM, Jeff King <peff@xxxxxxxx> 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...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html