Allow to merge if file path contains " or \

[Ilya33]

Hello.

If a filepath in a repository contains " or \ the owner can't merge pull request with this files
because "git diff-tree" adds double quotes to that filepath.
example: filepath is ab"cd but "git diff-tree" returns "ab"cd"

Now, when the owner click "Merge Pull Request" button the server returns 500.
This commit fix it.

Best Regards, Ilya Pavlov

[zeripath]

Damn! I'm sorry this was missed.

I think the correct thing to do is use the -z option on git diff-tree and change the code to handle the NUL character as a field separator rather than demunge the munging.

https://git-scm.com/docs/git-diff-tree#Documentation/git-diff-tree.txt--z

This needs checking to ensure that early gits have the option though.

I wouldn't be surprised if following this there were other issues in merge with unusual filenames especially on Windows as we do a checkout - albeit a sparse checkout - so we may be affected by these.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@4ee986e). Click here to learn what that means.
The diff coverage is 72.22%.

@@            Coverage Diff            @@
##             master    #8629   +/-   ##
=========================================
  Coverage          ?   41.32%           
=========================================
  Files             ?      539           
  Lines             ?    69493           
  Branches          ?        0           
=========================================
  Hits              ?    28718           
  Misses            ?    37087           
  Partials          ?     3688

Impacted Files	Coverage Δ
services/pull/merge.go	41.4% <72.22%> (ø)

---

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 4ee986e...1b1e257. Read the comment docs.

[guillep2k]

This doesn't seem to deal with \.

$ git diff-tree a06e7ac9fdbed88f73736b876ab63dfb680037ef aad2c7e2d2e381951cbaad3be107742f0b8fc132
:100644 100644 f1e195345db6ee7433b331c3aa333b50d46088f3 5c1b14949828006ed75a3e8858957f86a2f7e2eb M      "hello\\one\\\\two"

The actual file name is helo\one\\two

And there's also other edge cases like "with\nnew\nlines" or "hello\ttabs".

Unless we want to print them that way, which could work.

[guillep2k]

If this helps (with and without -z):

$ git diff-tree f4d8041e0cc6255902b2f55b12eaf9c10432c3bc aad2c7e2d2e381951cbaad3be107742f0b8fc132
:100644 000000 16283e876748c734fcb345c227f69869b568aad3 0000000000000000000000000000000000000000 D      "hello\ttabs"
:100644 100644 f1e195345db6ee7433b331c3aa333b50d46088f3 5c1b14949828006ed75a3e8858957f86a2f7e2eb M      "helo\\one\\\\two"
:100644 000000 4558c9ad94739d4c00005dd28da0c04771f83a87 0000000000000000000000000000000000000000 D      "with\nnew\nlines"

$ git diff-tree -z f4d8041e0cc6255902b2f55b12eaf9c10432c3bc aad2c7e2d2e381951cbaad3be107742f0b8fc132
:100644 000000 16283e876748c734fcb345c227f69869b568aad3 0000000000000000000000000000000000000000 Dhello tabs:100644 100644 f1e195345db6ee7433b331c3aa333b50d46088f3 5c1b14949828006ed75a3e8858957f86a2f7e2eb Mhelo\one\\two:100644 000000 4558c9ad94739d4c00005dd28da0c04771f83a87 0000000000000000000000000000000000000000 Dwith
new
lines

$ git diff-tree -z f4d8041e0cc6255902b2f55b12eaf9c10432c3bc aad2c7e2d2e381951cbaad3be107742f0b8fc | xxd
0000000: 3a31 3030 3634 3420 3030 3030 3030 2031  :100644 000000 1
0000010: 3632 3833 6538 3736 3734 3863 3733 3466  6283e876748c734f
0000020: 6362 3334 3563 3232 3766 3639 3836 3962  cb345c227f69869b
0000030: 3536 3861 6164 3320 3030 3030 3030 3030  568aad3 00000000
0000040: 3030 3030 3030 3030 3030 3030 3030 3030  0000000000000000
0000050: 3030 3030 3030 3030 3030 3030 3030 3030  0000000000000000
0000060: 2044 0068 656c 6c6f 0974 6162 7300 3a31   D.hello.tabs.:1
0000070: 3030 3634 3420 3130 3036 3434 2066 3165  00644 100644 f1e
0000080: 3139 3533 3435 6462 3665 6537 3433 3362  195345db6ee7433b
0000090: 3333 3163 3361 6133 3333 6235 3064 3436  331c3aa333b50d46
00000a0: 3038 3866 3320 3563 3162 3134 3934 3938  088f3 5c1b149498
00000b0: 3238 3030 3665 6437 3561 3365 3838 3538  28006ed75a3e8858
00000c0: 3935 3766 3836 6132 6637 6532 6562 204d  957f86a2f7e2eb M
00000d0: 0068 656c 6f5c 6f6e 655c 5c74 776f 003a  .helo\one\\two.:
00000e0: 3130 3036 3434 2030 3030 3030 3020 3435  100644 000000 45
00000f0: 3538 6339 6164 3934 3733 3964 3463 3030  58c9ad94739d4c00
0000100: 3030 3564 6432 3864 6130 6330 3437 3731  005dd28da0c04771
0000110: 6638 3361 3837 2030 3030 3030 3030 3030  f83a87 000000000
0000120: 3030 3030 3030 3030 3030 3030 3030 3030  0000000000000000
0000130: 3030 3030 3030 3030 3030 3030 3030 3020  000000000000000
0000140: 4400 686f 6c61 0a71 7565 0a74 616c 00    D.hola.que.tal.

[lunny]

It's better to add a unit test.

[Ilya33]

think the correct thing to do is use the -z option on git diff-tree and change the code to handle the NUL character as a field separator rather than demunge the munging.

I will try it

[Ilya33]

• added -z option to getDiffTree
• escape right trailing spaces
• escape an optional prefix !
• escape *, ?, [ symbols

Don't know what to do with the \n symbol in the filename because it is used in sparse-checkout as string separator and there is no escape sequence for this one.

It's better to add a unit test.

Should I create a new file for it or just add it to pull_merge_test.go?

[guillep2k]

Should I create a new file for it or just add it to pull_merge_test.go?

It looks like services/pull/merge_test.go is a good place for it.

[guillep2k]

Just for kicks, I've tested and git has no problem creating and patching files with 0x08 (^H) and 0x7f (DEL) in their names. But it has some problems with wrong encodings:

$ ls | xxd
0000000: 6814 6c6c 6f0a 7878 7808 7575 750a 7878  h.llo.xxx.uuu.xx
0000010: 787f 7979 790a                           x.yyy.

First file was added last, is supposed to have a \0244 (á in CP-1252) in its name. Now:

$ git diff HEAD~1
diff --git "a/h\024llo" "b/h\024llo"
new file mode 100644
index 0000000..304c8f6
--- /dev/null
+++ "b/h\024llo"
@@ -0,0 +1 @@
+badenc

This is actually wrong, because h\024llo should be h\0244llo. 😆

[zeripath]

This will solve the issue. I think there are several better ways of doing this - but those are more refactors than bugfixes. (I also am suspicious that there could be a bug to do with renamed files.)

[zeripath]

Please send backports