Skip to content

Fix HEAD reflog on reference renames - #4294

Closed
tiennou wants to merge 5 commits into
libgit2:masterfrom
tiennou:fix-ref-rename-reflog
Closed

Fix HEAD reflog on reference renames#4294
tiennou wants to merge 5 commits into
libgit2:masterfrom
tiennou:fix-ref-rename-reflog

Conversation

@tiennou

@tiennou tiennou commented Jul 7, 2017

Copy link
Copy Markdown
Contributor

While investigating #4292, I found that we have a few tests related to what shows up in the reflog, scattered in various test cases.

This PR consolidates all "simple" ones in the same place, and also standardize some helper methods (I might convert reflog_check_entry to something more clar-y though, because line numbers are nice).

I've also added a tentative test case for #4292. The symptoms are that git_reference_rename reflogs :

0000000000000000000000000000000000000000 a65fedf39aefe402d3bb6e24df4d4f5fe4547750 unknown <unknown> 1499373433 +0200	checkout: moving from master to renamed

(which comes from the git_repository_set_head call it does).

git does this :

34bccdaf357e32eaa589f8349fb39400d4da192e 0000000000000000000000000000000000000000 me me <me@me.null> 1499372633 +0200	Branch: renamed refs/heads/development to refs/heads/develop
0000000000000000000000000000000000000000 34bccdaf357e32eaa589f8349fb39400d4da192e me <me@me.null> 1499372633 +0200	Branch: renamed refs/heads/development to refs/heads/develop

Fixing that requires special-casing git_repository_set_head so it doesn't change the reflog, so git_reference_rename can append those entries itself. Does that seem correct ?

While I'm at it, this raised a question: why is that reflog line triggered by git_repository_set_head ? I'm likely missing something but it might be cleaner to have that handled by git_checkout_* ?

@pks-t

pks-t commented Jul 7, 2017

Copy link
Copy Markdown
Member

Makes sense, thanks! There are a few small issues in the patch, but I currently cannot comment inline. So I'm pointing them out here:

  • the new test is missing a git_buf_free in the commented out part
  • new files are missing our license header
  • there's been some trailing (possibly copied) whitespace in in reflog_check_entry

And yet, again we cannot really merge this due to a missing macro like cl_git_breakage which records known breakages. :/ Are you working on a fix for this issue?

@tiennou

tiennou commented Jul 7, 2017

Copy link
Copy Markdown
Contributor Author

I'll drop the tentative test case from that PR then, and resubmit another one with the testcase + fix.

@jgrosso

jgrosso commented Jul 7, 2017

Copy link
Copy Markdown

@tiennou I actually just opened a PR for #4292: #4297 🙂 . Assuming that PR is correct, am I correct in understanding that this PR will contain tests for it?

@tiennou

tiennou commented Jul 7, 2017

Copy link
Copy Markdown
Contributor Author

Yes, the (disabled part of the) test added in 3679251 checks that the HEAD reflog looks like git does when the checked-out ref is renamed.

@tiennou
tiennou force-pushed the fix-ref-rename-reflog branch from 3679251 to 6bca1ca Compare July 7, 2017 17:14
@tiennou tiennou changed the title Gather the reflog entry content tests Fix HEAD reflog on reference renames Jul 7, 2017
@tiennou
tiennou force-pushed the fix-ref-rename-reflog branch 4 times, most recently from 5595cac to 77bbdb2 Compare July 17, 2017 20:42
@tiennou
tiennou force-pushed the fix-ref-rename-reflog branch from 77bbdb2 to b2fb925 Compare July 21, 2017 17:56
@tiennou
tiennou force-pushed the fix-ref-rename-reflog branch from b2fb925 to 5694f2d Compare July 21, 2017 18:12
@tiennou tiennou closed this Jul 21, 2017
@tiennou
tiennou deleted the fix-ref-rename-reflog branch July 21, 2017 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants