Skip to content

Correct "new" id for reattached-HEAD reflog entry#2095

Merged
vmg merged 3 commits intodevelopmentfrom
update-head-reflog
Feb 3, 2014
Merged

Correct "new" id for reattached-HEAD reflog entry#2095
vmg merged 3 commits intodevelopmentfrom
update-head-reflog

Conversation

@ben
Copy link
Copy Markdown
Member

@ben ben commented Feb 1, 2014

  1. Detach HEAD
  2. Attach HEAD to a branch

Expected: the last reflog entry's "new" id should be the ID of HEAD^{commit}

Actual: A zero ID, like so:

32eab9cb1f450b5fe7ab663462b77d7f4b703344 6dcf9bf7541ee10456529833502442f385010c3d Ben Straub <bs@github.com> 1391281566 -0800   checkout: moving from master to 6dcf9bf
6dcf9bf7541ee10456529833502442f385010c3d 0000000000000000000000000000000000000000 Ben Straub <bs@github.com> 1391281663 -0800   checkout: moving from 6dcf9bf7541ee10456529833502442f385010c3d to master

I fixed the issue, and added more complete test coverage for detaching and reattaching HEAD.

  • Add a unit test exposing the bug.
  • Fix the bug.

@ben ben mentioned this pull request Feb 1, 2014
4 tasks
src/refdb_fs.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should check the failure case here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? What if the new target branch is unborn? Doing a if (call < 0) goto cleanup here causes that functionality to fail.

Maybe we don't care about a failure; if we can't resolve the new target reference to an OID, we use zeros. Is that okay?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an explicit exit code from git_reference_symbolic_target() in that case? (GIT_ENOTFOUND?)

I think we should test for that error and giterr_clear() if we find it and abort on other errors.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ethomson uh, or maybe this?

vmg pushed a commit that referenced this pull request Feb 3, 2014
Correct "new" id for reattached-HEAD reflog entry
@vmg vmg merged commit 3b6a5ba into development Feb 3, 2014
@ben ben deleted the update-head-reflog branch February 3, 2014 18:37
phatblat pushed a commit to phatblat/libgit2 that referenced this pull request Sep 13, 2014
Correct "new" id for reattached-HEAD reflog entry
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