Add support for complicated filenames and file copies #61

kwi-dk · 2021-03-22T14:44:19Z

This fixes parsing of unquoted filenames with multiple spaces in them, quoted filenames with spaces in them, and diffs containing file copies (bug #48).

When filenames contain special characters, they are quoted, but not if they only contain spaces, meaning the diff --git argument line becomes ambiguous. It is, however, possible to reconstruct the filenames using the extended headers.

When filenames contain special characters, they are quoted, but not if they only contain spaces, meaning the 'git --diff' argument line becomes AMBIGUOUS. It is, however, possible to reconstruct the filenames using the extended headers. This fixes parsing of unquoted filenames with multiple spaces in them, quoted filenames with spaces in them, and diffs containing file copies.

codecov · 2021-03-22T14:46:50Z

Codecov Report

Merging #61 (28094f6) into master (a768196) will increase coverage by 2.75%.
The diff coverage is 97.18%.

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
+ Coverage   74.59%   77.34%   +2.75%     
==========================================
  Files           4        4              
  Lines         429      490      +61     
==========================================
+ Hits          320      379      +59     
- Misses         62       63       +1     
- Partials       47       48       +1

Impacted Files	Coverage Δ
diff/parse.go	`84.03% <97.18%> (+2.61%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a768196...28094f6. Read the comment docs.

mrnugget · 2021-03-26T08:34:04Z

Hey @kwi-dk! Thanks for opening this PR and sorry for not leaving a comment earlier. I plan on reviewing this PR, but have to admit that I most likely will only get to it next week.

mrnugget

Thanks for the contribution! Especially for adding the tests.

I left a bunch of comments to make the code a bit more idiomatic. Let me know if you have bandwidth to incorporate them, otherwise I'm happyt to take the PR over.

diff/diff_test.go

mrnugget · 2021-03-31T08:34:43Z

diff/diff_test.go

+	for _, input := range tests {
+		_, _, err := readQuotedFilename(input)
+		if err == nil {
+			t.Errorf("readQuotedFilename(`%s`): expected error", input)


Suggested change

t.Errorf("readQuotedFilename(`%s`): expected error", input)

t.Errorf("readQuotedFilename(%q): expected error", input)

Same concern about quoting as above.

mrnugget · 2021-03-31T08:35:16Z

diff/diff_test.go

+}
+
+func TestParseDiffGitArgs_Success(t *testing.T) {
+	tests := []string{


Same as above: I think structs would make this easier to understand.

Changed to use structs.

mrnugget · 2021-03-31T08:37:16Z

diff/parse.go

+// readQuotedFilename extracts a quoted filename from the beginning of a string,
+// returning the unquoted filename and any remaining text after the filename.
+func readQuotedFilename(text string) (value string, remainder string, err error) {
+	if text[0] != '"' {


Suggested change

if text[0] != '"' {

if text != "" && text[0] != '"' {

Otherwise this will blow up when passed an empty string (another test case?)

mrnugget · 2021-03-31T08:37:38Z

diff/parse.go

+// returning the unquoted filename and any remaining text after the filename.
+func readQuotedFilename(text string) (value string, remainder string, err error) {
+	if text[0] != '"' {
+		panic("caller must ensure filename is quoted! " + text)


I don't think this should be a panic but an error, what do you think?

The idea here was that readQuotedFilename is a private function, and its callers always ensure that text starts with a quote. This test is thus just a sanity check; if it fails, it's a programming error, and a panic is fine.

But since this is evidently not clear at all, I'll just change to ordinary error handling, and silence the Codecov bot while I'm at it. :)

mrnugget · 2021-03-31T09:52:13Z

diff/parse.go

+}
+
+// parseDiffGitArgs extracts the two filenames from a 'diff --git' line.
+func parseDiffGitArgs(diffArgs string) (bool, string, string) {


The more idiomatic signature for a function like this in Go would be this:

Suggested change

func parseDiffGitArgs(diffArgs string) (bool, string, string) {

func parseDiffGitArgs(diffArgs string) (string, string, bool) {

diff/parse.go

mrnugget · 2021-03-31T09:59:27Z

diff/parse.go

+		// One or both filenames contain a space, but the names are
+		// unquoted. Here, the 'diff --git' syntax is ambiguous, and
+		// we have to obtain the filenames elsewhere (e.g. from the
+		// chunk headers or extended headers). HOWEVER, if the file


Suggested change

// chunk headers or extended headers). HOWEVER, if the file

// hunk headers or extended headers). HOWEVER, if the file

diff/parse.go

kwi-dk

Sorry for the delay; turns out my GitHub notifications are broken. 😅

Feedback addressed in 28094f6.

kwi-dk · 2021-04-23T12:12:23Z

diff/diff_test.go

+	for _, input := range tests {
+		_, _, err := readQuotedFilename(input)
+		if err == nil {
+			t.Errorf("readQuotedFilename(`%s`): expected error", input)


Same concern about quoting as above.

diff/diff_test.go

kwi-dk · 2021-05-25T09:07:13Z

diff/diff_test.go

+}
+
+func TestParseDiffGitArgs_Success(t *testing.T) {
+	tests := []string{


Changed to use structs.

diff/diff_test.go

diff/parse.go

diff/diff_test.go

kwi-dk · 2021-05-25T09:35:16Z

diff/parse.go

+// returning the unquoted filename and any remaining text after the filename.
+func readQuotedFilename(text string) (value string, remainder string, err error) {
+	if text[0] != '"' {
+		panic("caller must ensure filename is quoted! " + text)


The idea here was that readQuotedFilename is a private function, and its callers always ensure that text starts with a quote. This test is thus just a sanity check; if it fails, it's a programming error, and a panic is fine.

But since this is evidently not clear at all, I'll just change to ordinary error handling, and silence the Codecov bot while I'm at it. :)

diff/parse.go

mrnugget

Nice! Thank you so much for this follow-up!

Improved test coverage (and slightly stricter error handling)

953267f

keegancsmith requested review from mrnugget and eseliger March 23, 2021 06:59

mrnugget suggested changes Mar 31, 2021

View reviewed changes

Address PR feedback

28094f6

kwi-dk commented May 25, 2021

View reviewed changes

kwi-dk requested a review from mrnugget May 25, 2021 11:35

mrnugget approved these changes May 26, 2021

View reviewed changes

mrnugget merged commit 35b24a7 into sourcegraph:master May 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for complicated filenames and file copies #61

Add support for complicated filenames and file copies #61

kwi-dk commented Mar 22, 2021 •

edited

Loading

codecov bot commented Mar 22, 2021 •

edited

Loading

mrnugget commented Mar 26, 2021

mrnugget left a comment

mrnugget Mar 31, 2021

kwi-dk Apr 23, 2021

mrnugget Mar 31, 2021

kwi-dk May 25, 2021

mrnugget Mar 31, 2021

mrnugget Mar 31, 2021

kwi-dk May 25, 2021

mrnugget Mar 31, 2021

kwi-dk May 25, 2021

mrnugget Mar 31, 2021

kwi-dk left a comment

kwi-dk Apr 23, 2021

kwi-dk May 25, 2021

kwi-dk May 25, 2021

mrnugget left a comment

	t.Errorf("readQuotedFilename(`%s`): expected error", input)
	t.Errorf("readQuotedFilename(%q): expected error", input)

	func parseDiffGitArgs(diffArgs string) (bool, string, string) {
	func parseDiffGitArgs(diffArgs string) (string, string, bool) {

	// chunk headers or extended headers). HOWEVER, if the file
	// hunk headers or extended headers). HOWEVER, if the file

Add support for complicated filenames and file copies #61

Add support for complicated filenames and file copies #61

Conversation

kwi-dk commented Mar 22, 2021 • edited Loading

codecov bot commented Mar 22, 2021 • edited Loading

Codecov Report

mrnugget commented Mar 26, 2021

mrnugget left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwi-dk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrnugget left a comment

Choose a reason for hiding this comment

kwi-dk commented Mar 22, 2021 •

edited

Loading

codecov bot commented Mar 22, 2021 •

edited

Loading