Fix forced branch creation on HEAD of a bare repo. by jfultz · Pull Request #3977 · libgit2/libgit2 · GitHub
Skip to content

Fix forced branch creation on HEAD of a bare repo.#3977

Closed
jfultz wants to merge 2 commits into
libgit2:masterfrom
jfultz:fix-forced-branch-creation-on-bare-repo
Closed

Fix forced branch creation on HEAD of a bare repo.#3977
jfultz wants to merge 2 commits into
libgit2:masterfrom
jfultz:fix-forced-branch-creation-on-bare-repo

Conversation

@jfultz

@jfultz jfultz commented Oct 28, 2016

Copy link
Copy Markdown
Contributor

The code correctly detects that forced creation of a branch on a
nonbare repo should not be able to overwrite a branch which is
the HEAD reference. But there's no reason to prevent this on
a bare repo, and in fact, git allows this. I.e.,

git branch -f master new_sha

works on a bare repo with HEAD set to master. This change fixes
that problem, and updates tests so that, for this case, both the
bare and nonbare cases are checked for correct behavior.

@jfultz jfultz force-pushed the fix-forced-branch-creation-on-bare-repo branch from 104359c to c7ac7a2 Compare October 28, 2016 21:51
The code correctly detects that forced creation of a branch on a
nonbare repo should not be able to overwrite a branch which is
the HEAD reference.  But there's no reason to prevent this on
a bare repo, and in fact, git allows this.  I.e.,

   git branch -f master new_sha

works on a bare repo with HEAD set to master.  This change fixes
that problem, and updates tests so that, for this case, both the
bare and nonbare cases are checked for correct behavior.
@jfultz jfultz force-pushed the fix-forced-branch-creation-on-bare-repo branch from c7ac7a2 to bc7c633 Compare October 28, 2016 21:52

@pks-t pks-t left a comment

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.

Looks good besides the memory leak. Thanks for working on this 👍

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.

This leaks memory as you do not free branch here and overwrite it afterwards.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copied and pasted the code from the legacy test case, but in the legacy test case it should fail, so there's nothing to clean up. Oops. :P

@pks-t

pks-t commented Nov 2, 2016

Copy link
Copy Markdown
Member

Looks good. Will wait for comments and merge later this week if nothing comes up. Thanks 🎉

pks-t added a commit that referenced this pull request Nov 4, 2016
@pks-t

pks-t commented Nov 4, 2016

Copy link
Copy Markdown
Member

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.

2 participants