Skip to content

patch_parse: implement state machine for parsing patch headers - #4308

Merged
ethomson merged 2 commits into
libgit2:masterfrom
pks-t:pks/header-state-machine
Nov 11, 2017
Merged

patch_parse: implement state machine for parsing patch headers#4308
ethomson merged 2 commits into
libgit2:masterfrom
pks-t:pks/header-state-machine

Conversation

@pks-t

@pks-t pks-t commented Jul 14, 2017

Copy link
Copy Markdown
Member

Our code parsing Git patch headers is rather lax in parsing headers of a
Git-style patch. Most notably, we do not care for the exact order in
which header lines appear and as such, we may parse patch files which
are not really valid after all. Furthermore, the state transitions
inside of the parser are not as obvious as they could be, making it
harder than required to follow its logic.

To improve upon this situation, this patch introduces a real state
machine to parse the patches. Instead of simply parsing each line
without caring for previous state and the exact ordering, we define a
set of states with their allowed transitions. This makes the patch
parser more strict in only allowing valid successions of header lines.
As the transition table is defined inside of a single structure with
the expected line, required state as well as the state that we end up
in, all state transitions are immediately obvious from just having a
look at this structure. This improves both maintainability and eases
reasoning about the patch parser.

Our code parsing Git patch headers is rather lax in parsing headers of a
Git-style patch. Most notably, we do not care for the exact order in
which header lines appear and as such, we may parse patch files which
are not really valid after all. Furthermore, the state transitions
inside of the parser are not as obvious as they could be, making it
harder than required to follow its logic.

To improve upon this situation, this patch introduces a real state
machine to parse the patches. Instead of simply parsing each line
without caring for previous state and the exact ordering, we define a
set of states with their allowed transitions. This makes the patch
parser more strict in only allowing valid successions of header lines.
As the transition table is defined inside of a single structure with
the expected line, required state as well as the state that we end up
in, all state transitions are immediately obvious from just having a
look at this structure. This improves both maintainability and eases
reasoning about the patch parser.
@pks-t
pks-t force-pushed the pks/header-state-machine branch from 28d2491 to 57bc9da Compare August 25, 2017 15:26
@pks-t

pks-t commented Aug 25, 2017

Copy link
Copy Markdown
Member Author

Rebased on current master. This is mainly to have the base same commit in #4310, which builds upon this PR.

Patches which contain exact renames only will not contain an actual diff
body, but only a list of files that were renamed. Thus, the patch header
is immediately followed by the terminating sequence "-- ". We currently
do not recognize this character sequence as a possible terminating
sequence. Add it and create a test to catch the failure.
Comment thread src/patch_parse.c
/* Next patch */
{ "diff --git " , STATE_END, 0, NULL },
{ "@@ -" , STATE_END, 0, NULL },
{ "-- " , STATE_END, 0, NULL },

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.

Doesn't the fix to #4322 look nice with this new state machine? :)

@pks-t

pks-t commented Sep 15, 2017

Copy link
Copy Markdown
Member Author

@ethomson You think you might get some time soonish to review? I'd like to move on with the common parser interface in #4310, which depends on this PR here. More specifically, I wanted to also take a look at moving our attributes parser to use the new parsing interface, as well.

@ethomson

Copy link
Copy Markdown
Member

THIS IS AWESOME. 🎉

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