Skip to content

Fix HEAD reflog on reference renames#4294

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

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

Conversation

@tiennou
Copy link
Contributor

@tiennou tiennou commented Jul 7, 2017

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
Copy link
Member

pks-t commented Jul 7, 2017

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
Copy link
Contributor Author

tiennou commented Jul 7, 2017

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

@jgrosso
Copy link

jgrosso commented Jul 7, 2017

@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
Copy link
Contributor Author

tiennou commented Jul 7, 2017

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