Conversation
To e.g. remove the first character from Line instances.
Overview:
This allows to use the `split-patch` to fully check for correctness of
patch files without having to split on --changes as before; in fact
allows to check without generating output files at all.
Also, this is a major clean-up of the library design, and makes it
*much* more suitable for *generating* patch/diff files from
scratch. The data structure is now largely a proper abstract syntax
tree of patch/diff files, with (almost?) no redundancy anymore (the
exceptions are the overlapping areas between changes, and, if so
chosen, storing the unparsed representation as well). The parsed
representation is for the 'canonical' format--it cannot (currently)
represent variations (i.e. is not reversible). It does allow to store
the unparsed representation, though, and can use it for
re-serialisation to keep round-trips identical as much as possible.
The most major changes:
- `Hunk` now has multiple modes (it becomes an enum), to allow to
store unparsed, parsed or both variants; the parsed variants are
purely about canonical representation (which is the proper way to go
when using the structs to construct patch/diff files from scratch),
while the unparsed data is for when there is an interest in
maintaining the original representation as much as possible. There
is a new parsed representation, `ParsedHunk`.
- `Change` becomes the representation of a single change within
`ParsedHunk`; it cannot be a self-standing Hunk-equivalent any
more. Splitting a `ParsedHunk` now again yields `ParsedHunk`s.
Details:
patchparser:
- from_lines.rs: trait FromLines is pretty much obsolete (still used
in patch.rs; maybe keep and add another trait for the other `from_*`
methods?)
- patch/change.rs:
- struct Change:
- Remove lifetime `'h` (the "h" stood for "hunk") since Hunk
does not own any non-Copy data (BumpaloCow) any more.
- The start and len fields are removed, since those are now
freshly calculated when serializing the data, which is the
proper reliable approach (especially when users are creating
instances). The start and `head_post` information is now in
the `ParsedHunk` instead.
- `group` is replaced with `minus` and `plus` sections, as the
canonical way to serialize hunks is to have them in that
order, there is no reason to keep the lines within the group
in a mixed order. (This means that those sections possibly
have to be freshly (bumpalo-)allocated arrays, but that is
now necessary anyway, see the next change.)
- The referenced lines now do not have their first character
anymore, instead that character is determined by the section
(field). This means that besides the `Line` instances also
all slices are freshly allocated ones (i.e. not slices of
the original file lines array anymore).
- `backslash` is now just a boolean, the full line is replaced
with a 'canonical' one on serialisation.
- Since `Change` is now a building block within `ParsedHunk`,
it has to not print the part of the context that the
previous `Change` of the same hunk already contains in that
one's `post` field. OTOH it is probably (perhaps?) useful
for a `Change` to have the full information; hence add the
`post_from_previous_change` field which shares those lines
but doesn't print them when preceeded by another (presumably
that) change (it needs `Position` for that, see next
point). (This area may still benefit from more thought.)
- Add `struct Position`, to pass the information to the `Change`
about how it is positioned, so it knows how to print itself.
- Add tests to make sure the logic around max context length works
properly.
- patch/change_line.rs:
- Add `ChangeLineKind::prefix` to get the character for the lines
in each section back.
- `kind_or_terminator` now returns a new `enum KindOrTerminator`,
to make error handling more straightforward (the old
`try_split_before_in` approach really made this overly complex;
track further back from that)
- patch/hunk.rs:
- Delete `trait WriteAsHunk`, as now Changes are not "hunk-like"
anymore; splitting a hunk yields a list of hunks of the same
type now, hence no trait needed.
- As mentioned in the overview, `struct Hunk` becomes an `enum
Hunk`, and a `struct ParsedHunk` (in a new file
`patch/parsed_hunk.rs`).
- `Hunk::from_lines` is now outside the FromLines trait, as it
needs additional arguments: (1) the construction mode (parsing
or not?, in a later commit that boolean will become an enum,
too), (2) a callback to optionally check consistency and handle
any errors.
- The old `split_into_changes` method is moved to
`patch/parsed_hunk.rs` and morphed into `fn
split_hunk_into_changes` there that generates a vector of the
new `Change`, and used as part of the `ParsedHunk` parsing
process. (The API for splitting a hunk is now via a new
`ParsedHunk::split_by_change` method, also in
`patch/parsed_hunk.rs`.)
- Update and add more unit tests
- New file `patch/parsed_hunk.rs`:
- New `struct FullHunkHead` that is the result from parsing a hunk
head-line, including the (redundant, since derivable from
counting the lines in the change bodies) range length
information.
- New `struct MinimalHunkHead` that does not contain the redundant
range length information.
- New `struct ParsedHunk` that contains a `MinimalHunkHead` and
the list of `Change`s, with zero redundancy (except for the
`Change.post_from_previous_change` field, that is!)
- New `enum CheckError` that represents inconsistencies (will it
always only be about those hunk range numbers, change to a
struct?)
- `ParsedHunk::from_lines` takes a `handle_check_error` callback,
that receives a thunk for checking consistency that returns a
`CheckError`, and can decide what to do with that. `from_lines`
stops with the error that this callback returns, if any. This is
to make it possible to ignore the range numbers altogether, or
e.g. to warn about them but not stop.
- `fn split_hunk_into_changes`:
- replace higher-order functionality with plain loops (either
find a parser library with *good* abstractions for this kind
of work, or find better simple abstractions than the
`try_split_before_in`, or be happy with those loops). Note
how for the middle group, it separates out `-` and `+` lines
regardless of whether they are mixed.
- patch/diff.rs:
- struct DiffDifferences: do not own data anymore (bc::Vec), just
reference slices in bumpalo. Those are not mutable anyway once
the struct itself lives in bumpalo--a pure FP modification
approach has to be used instead (or clone then mutate outside,
but making new slices is easy enough, there is not much value
for a bc::Vec here; the mutating `set_hunks` method now takes a
slice and the user code can provide it "just fine"). And then
never owning data makes the down-stream lifetime situation "so
much" simpler.
- Also the ReborrowIn implementation becomes simpler, too (in fact
the struct will now be covariant again, hence does not even need
to be reborrowed).
- Pass through `handle_check_error` (see explanation about it
above)
- Replace `impl FromLines for Diff` with `impl Diff` as
`from_lines` needs more arguments.
- patch/patch.rs:
- Take `Patch::from_lines` out of the FromLines trait, here, too.
split-patch:
- split_options.rs: add the mentioned options
- core.rs:
- Don't need to go through bumpalo::vec! to get a slice, one can
just alloc any array (const-parameterized)
- Update API change around lazy Hunk parsing
- `fn write_patch_file`: now takes split options, handles `dry_run`
- Parse Patch according to parse mode determined from user options
(copy-paste of the callback from further above, meh XXX)
Looks like the Perl split-patch had a bug!
Because even I forgot that `--check-only` exists for this.
- patch/hunk.rs:
- Add `ParseMode` which mirrors the possible `Hunk` states.
- Replace `parse: bool` with `parse_mode: ParseMode` everywhere.
- split_options.rs:
- Add `SplitArgs.regenerate`
- Add `SplitOptions.parse_mode`, replacing `full_check`
To allow to share the callback code The complexities around closures is high--can only do it with dyn here (a name as a sub-trait of the closure trait would be definable but not implementable)?
Re-add a closure wrapper though (`CheckErrorClosure`) to make it easy for those cases. This allows to implement the trait directly on SplitOptions. Observations: - Old question of mine: why do we have to manually add 'proxy implementations' for references when implementing traits? At least can do it globally like here? At the risk of conflicts. What are the rules for when it is safe to do it? - I'm surprised that `impl HandleCheckError for &SplitOptions` works even though the `handle_check_error` method takes a `&mut self`. Interesting.
As per
CLIPPY_ARGS="-- -W clippy::complexity" make clippy
(Disabled warning by default, which should still stay that way.)
Most(?) of the useful reports from
CLIPPY_ARGS="-- -W clippy::pedantic" make clippy
Contributor
Author
|
I have added two more commits fixing things from clippy suggestions. 2 of the changes from the last commit are about new code added in this PR; I could split those off and sqash them back but then it would change the commit ids that I verified in the docs, so I think I'll just leave them as is, but I add them to this PR so you can see what cleanup changes to the previous code here I've done. |
Contributor
Author
|
And now I've added 3 more commits to fine tune split-patch's options handling. |
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
1 participant
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.
This refactors the patchparser crate to allow to use it:
It updates the split-patch tool and adds --check, --check-only, --dry-run, --ignore-range-errors and --regenerate options to make use of those new features.
This is definitely a semver-breaking change.
This is a pretty large refactor; take your time to understand it. I'm happy to explain or meet (maybe not this week anymore). I think it is very worth it, it finally brings the parser data structure into a good form.
Open work:
There are 4 "XXX" in
patchparser/src/patch/parsed_hunk.rswhich all deal with termination criteria. I left this unfinished as I still haven't merged tests for this (tests with broken files to detect bugs, of which there are or have been in the previous form of this code, endless loops too!). So, the code already was broken here, and I do have tests lying around that wait to get finished and merged, so I would like to merge the code as is and tackle this part later after I have picked up the proper testing again.const MAX_CONTEXT_LEN: usize = 3;should (preferably) be configurable; easy to do, let's make a new issueIdea to add a
--no-splitoption that in combination with--regeneratewould regenerate the input files (in place if no output dir is provided), i.e. bring them into the canonical form (as per patchparser's current preferences).A couple more new
XX(2 not 3Xs). To be tackled them whenever it fits, not important.(Update the library version already?, although that is confusing since +-nobody uses tags in the Rust world.)
I mentioned in the commit message of
split-patch: implement --check, --check-only, --ignore-range-errors(8ab6538) that a previous version usedtry_split_before_in, but actually that function is new here. I need to check what I meant when I'm awake and then perhaps fix the message.Commit overview:
This is on top of cj_various, as
test-split-patches-in-dir.rs: share common options(839e164) is required.Utility add-ons:
write_line_to(20a63c9)Line::with_changed_contents(a43b6f0)try_split_before_in(b82d386)The actual meat:
split-patch: implement --check, --check-only, --ignore-range-errors (8ab6538): The largest commit here. Read the commit message for an explanation, hopefully good (the message is long, but still just 10% of the length of the whole diff).
test-split-patches-in-dir.rs: use
--checkoption (36cf0ef): actually be picky about the input files; no change here as all input files check out fine. This will change once we add faulty files to the test suite.split-patch/test/: fix expected outputs (27d8c56): apparently the files from the perl script were faulty!
split_options.rs: add an explicit
--dry-runoption (2740768): usage redundancy, shrug.Usage for file "regeneration":
--regenerate(c116af5): Test it.--regeneratetest (87f785d): The outputs are the same as in the--subdirs except for the ",1" difference.A fix:
head_postfield (fb16af2): that was wrong before already.The API of the library to allow flexibility in how to handle the results of (or even carry out) consistency checks caused me a bit of a headache: closures would seem nice at first, but it just gets quite painful.
handle_check_errorAPI (b21eddd): try to add a name for it, as copy-pasting the type is painful. But can only do it by making the outer closure dyn, too (right?).I hope this has been the last biggest work on at least the
patchparserlibrary. At least that should be the last big refactor. New functionality to help generate files could still come.