patch_parse: implement state machine for parsing patch headers - #4308
Merged
Conversation
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
force-pushed
the
pks/header-state-machine
branch
from
August 25, 2017 15:26
28d2491 to
57bc9da
Compare
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.
pks-t
commented
Sep 1, 2017
| /* Next patch */ | ||
| { "diff --git " , STATE_END, 0, NULL }, | ||
| { "@@ -" , STATE_END, 0, NULL }, | ||
| { "-- " , STATE_END, 0, NULL }, |
Member
Author
There was a problem hiding this comment.
Doesn't the fix to #4322 look nice with this new state machine? :)
Member
Author
Member
|
THIS IS AWESOME. 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.