Skip to content

MultiFileDiffReader doesn’t handle messages in diff that certain files are only available in specific version. #49

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sofiia-tesliuk opened this issue Jul 31, 2020 · 5 comments · Fixed by #50

Comments

@sofiia-tesliuk
Copy link
Contributor

In some unified diff files there are such lines as Only in {path}: {filename}, meaning that certain files are only available in specific version. Those messages aren’t handled in any way, except when those lines are just catched in FileDiff.Extended for next FileDiff in the file.
Are there any plans of handling these messages?

For example, those unique files can be picked up in next functions, representing them as FileDiff with only the value OrigName.

func (r *MultiFileDiffReader) ReadAllFiles() ([]*FileDiff, error)
func (r *MultiFileDiffReader) ReadFile() (*FileDiff, error)

Then for this file example, the output below for ReadAllFiles() would be expected.
my_diff.txt

diff -u source_a/file_1.txt  source_b/file_1.txt
--- source_a/file_1.txt   2020-07-28 12:54:18.000000000 +0000
+++ source_b/file_1.txt  2020-07-28 12:54:18.000000000 +0000
@@ -2,3 +3,4 @@
 To be, or not to be, that is the question:
-Whether 'tis nobler in the mind to suffer
+The slings and arrows of outrageous fortune,
+Or to take arms against a sea of troubles
 And by opposing end them. To die—to sleep,
Only in source_a: file_2.txt
Only in source_b: file_3.txt
ReadAllFiles(myMultiFileDiffReader) -> {
    FileDiff{
        OrigName: “source_a/file_1.txt”, 
        OrigTime: ..., 
        NewName: “source_b/file_1.txt”, 
        NewTime: …,
        Entended: ..., 
        Hunks: …
    },
 
    FileDiff{
        OrigName: “source_a/file_2.txt”, 
        OrigTime: nil, 
        NewName: nil, 
        NewTime: nil
        …
    }, 

    FileDiff{
        OrigName: “source_b/file_3.txt”, 
        OrigTime: nil, 
        NewName: nil, 
        …
    }

}, nil
@schachmat
Copy link

Would you accept a pull request to fix this?

@keegancsmith
Copy link
Member

Thanks for the proposal. I think we would. It seems like the set of inputs we understand will only increase + the empty diff makes sense. @eseliger @mrnugget thoughts? Any concerns around the nil time param?

@eseliger
Copy link
Member

eseliger commented Aug 5, 2020

Don't think so, we should probably make sure we do proper nil checks in our codebase, but that shouldn't affect work in this repository.
I couldn't find any resources by a quick google search that defined this behavior, and I am struggling to understand what it actually means. So if it's only in source_a, that basically means deleted, right? And the other way around, that means it's created?
If so, I would have expected the expected result you posted would be [2].NewName: "source_b/file_3.txt and [2].OrigName: nil, so I must be missing something :) Can you clarify how the "Only in ..." is different from deleted/created?

@sofiia-tesliuk
Copy link
Contributor Author

@eseliger yes, you are right :) It mostly clarifies deleted/created files (except maybe files that actually have been renamed, but this couldn't be handled, as they will be marked as deleted with originName and created with NewName). It will just require an extra step for checking whether source_a should be marked as OrigSource or NewSource.

@mrnugget
Copy link
Contributor

mrnugget commented Aug 6, 2020

Sounds like it's another way to express deleted/created in diffs and supporting that would just be additional functionality that would have no effect on the existing code. If so, yeah, I think we should accept a PR :)

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 a pull request may close this issue.

5 participants