Parse ref oids without trailing newline #861

Merged
merged 6 commits into from Aug 9, 2012

Conversation

Projects
None yet
4 participants
Contributor

josh commented Aug 9, 2012

The libgit2 ref oid parser is kinda strict. It requires a trailing newline otherwise you get a reference exception. git rev-parse doesn't care and works without it.

I noticed this issue testing libgit2 on gist. It seems gists created via the web are formatted this way.

Not sure if this is the correct way to handle the issue. Also, my C might be bad.

/cc @vmg @rtomayko

This pull request passes (merged 6ab6829 into e460739).

Member

nulltoken commented Aug 9, 2012

Git supports this as well

👍

Member

arrbee commented Aug 9, 2012

This looks fine to me and I'm happy to merge it as is if you just want to get a quick fix in.

That being said, from what I can tell, core git just trims all trailing whitespace from the loose ref before attempting to parse it. I would be tempted to match that behavior instead. It actually lets us remove more code, too.

I wrote what I think is needed here: https://github.com/arrbee/libgit2/compare/rtrim-loose-refs

That's a bigger change, I realize, but I think it would bring us in line with core git's resolve_ref_unsafe() behavior.

Member

arrbee commented Aug 9, 2012

By the way, I initially wrote that by putting the git_buf_rtrim() call into reference_read() but that fucks with the packed ref parsing code, so I pushed it out into the loose ref parsing.

Contributor

josh commented Aug 9, 2012

core git just trims all trailing whitespace from the loose ref before attempting to parse it. I would be tempted to match that behavior instead. It actually lets us remove more code, too.

Lets do that.

Member

nulltoken commented Aug 9, 2012

Git supports this as well

That being said, from what I can tell, core git just trims all trailing whitespace

@arrbee You're right. Earlier, I've only tested the initial case through the command line, without taking a look at the code.

Adding some spaces and tabs seems to be pretty well handled by git. Maybe would it be interesting to alter the content of the test refs/heads/chomped reference in order to reflect this. Another option would be to keep chomped unchanged and create a new trailed one.

How do you feel about this @josh?

Contributor

josh commented Aug 9, 2012

@nulltoken I'll create another test fixture.

This pull request passes (merged 28e0068 into e460739).

Contributor

josh commented Aug 9, 2012

Merged arrbee/rtrim-loose-refs into this branch.

This pull request passes (merged de65f82 into e460739).

@arrbee arrbee added a commit that referenced this pull request Aug 9, 2012

@arrbee arrbee Merge pull request #861 from josh/parse-chomped-oid
Parse ref oids without trailing newline
d7b3dab

@arrbee arrbee merged commit d7b3dab into libgit2:development Aug 9, 2012

1 check passed

default The Travis build passed
Details

@phatblat phatblat pushed a commit to phatblat/libgit2 that referenced this pull request Sep 13, 2014

@arrbee arrbee Merge pull request #861 from josh/parse-chomped-oid
Parse ref oids without trailing newline
fc10dda
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment