Skip to content

Diff fixes#4160

Merged
pks-t merged 5 commits intolibgit2:masterfrom
pks-t:pks/diff-fixes
Mar 20, 2017
Merged

Diff fixes#4160
pks-t merged 5 commits intolibgit2:masterfrom
pks-t:pks/diff-fixes

Conversation

@pks-t
Copy link
Copy Markdown
Member

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

This fixes #4158 along with some other issues

Under the existing logic, we try to load patch contents differently,
depending on whether the patch files stem from the working directory or
not. But actually, the executed code paths are completely equal to each
other -- so we were always the code despite the condition.

Remove the condition altogether and conflate both code paths.
@ethomson
Copy link
Copy Markdown
Member

So I think that you found (and fixed) the last bit of refactoring that I had forgotten to do when working on the new generated/parsed patch system. At least, I hope there's no more bits lurking. 😛

Especially now that this is generalized, I think that it might make sense to hoist the git_diff_foreach function out of patch_generate.c and into diff.c or one of its friends...

@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Mar 14, 2017

I certainly agree with you. I actually already started moving it over, but back then I didn't finish the unification yet, so I ran into some problems. Will add another commit

pks-t added 4 commits March 14, 2017 13:08
The current logic of `git_diff_foreach` makes the assumption that all
diffs passed in are actually derived from generated diffs. With these
assumptions we try to derive the actual diff by inspecting either the
working directory files or blobs of a repository. This obviously cannot
work for diffs parsed from a file, where we do not necessarily have a
repository at hand.

Since the introduced split of parsed and generated patches, there are
multiple functions which help us to handle patches generically, being
indifferent from where they stem from. Use these functions and remove
the old logic specific to generated patches. This allows re-using the
same code for invoking the callbacks on the deltas.
Now that the `git_diff_foreach` function does not depend on internals of
the `git_patch_generated` structure anymore, we can easily move it to
the actual diff code.
In a diff, the shortest possible hunk with a modification (that is, no
deletion) results from a file with only one line with a single character
which is removed. Thus the following hunk

    @@ -1 +1 @@
    -a
    +

is the shortest valid hunk modifying a line. The function parsing the
hunk body though assumes that there must always be at least 4 bytes
present to make up a valid hunk, which is obviously wrong in this case.
The absolute minimum number of bytes required for a modification is
actually 2 bytes, that is the "+" and the following newline. Note: if
there is no trailing newline, the assumption will not be offended as the
diff will have a line "\ No trailing newline" at its end.

This patch fixes the issue by lowering the amount of bytes required.
The function `diff_parsed_alloc` allocates and initializes a
`git_diff_parsed` structure. This structure also contains diff options.
While we initialize its flags, we fail to do a real initialization of
its values. This bites us when we want to actually use the generated
diff as we do not se the option's version field, which is required to
operate correctly.

Fix the issue by executing `git_diff_init_options` on the embedded
struct.
@pks-t pks-t merged commit e30a6ee into libgit2:master Mar 20, 2017
@pks-t pks-t deleted the pks/diff-fixes branch April 7, 2017 09:20
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.

git_diff_from_buffer: there is no initialization of git_diff_options version

2 participants