Bug 1469098 - git am doesn't apply for base64 encoded mail
Summary: git am doesn't apply for base64 encoded mail
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: git
Version: 27
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Petr Stodulka
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-07-10 12:08 UTC by Jaroslav Škarvada
Modified: 2018-01-25 20:30 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-01-25 20:30:38 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
Reproducer (10.83 KB, message/rfc822)
2017-07-10 12:08 UTC, Jaroslav Škarvada
no flags Details

Description Jaroslav Škarvada 2017-07-10 12:08:12 UTC
Created attachment 1295789 [details]
Reproducer

Description of problem:
It seems it strips CR in mailsplit during preprocessing but it doesn't strip them later after base64 decoding.

Version-Release number of selected component (if applicable):
git-2.13.2-1

How reproducible:
Always

Steps to Reproduce:
1. git clone https://github.com/redhat-performance/tuned.git
2. git checkout -b test 55c7128d1c3cb8e85b4b1c73ebac9da6235bd487
3. git am ./reproducer

Actual results:
Applying: Python traces are seen when tuned-adm verifiy -i is executed for cpu-partitioning profile.
.git/rebase-apply/patch:14: trailing whitespace.
	try:
.git/rebase-apply/patch:15: trailing whitespace.
           interruptdirs.remove("2")
.git/rebase-apply/patch:16: trailing whitespace.
        except ValueError:
.git/rebase-apply/patch:17: trailing whitespace.
           pass
.git/rebase-apply/patch:21: trailing whitespace.
	try:
error: patch failed: libexec/defirqaffinity.py:61
error: libexec/defirqaffinity.py: patch does not apply
Patch failed at 0001 Python traces are seen when tuned-adm verifiy -i is executed for cpu-partitioning profile.
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Expected results:
It applies

Additional info:
If I decode the base64 encoded part with the 'base64 --decode' add the e-mail original header and remove the 'Content-Transfer-Encoding: base64' line, the 'git am' applies OK.

Comment 1 Jan Kurik 2017-08-15 07:20:15 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 27 development cycle.
Changing version to '27'.

Comment 2 Todd Zullinger 2017-11-12 16:58:50 UTC
The patch applies if you pass --ignore-whitespace to git am:

$ git am --ignore-whitespace /tmp/reproducer.patch
Applying: Python traces are seen when tuned-adm verifiy -i is executed for cpu-partitioning profile.

Testing this, I first ran git am with --reject and with GIT_TRACE set, to see where it went wrong:

$ GIT_TRACE=1 git am --reject /tmp/reproducer.patch
11:36:13.090859 git.c:344               trace: built-in: git 'am' '--reject' '/tmp/reproducer.patch'
11:36:13.091754 run-command.c:626       trace: run_command: 'mailsplit' '-d4' '-o.git/rebase-apply' '-b' '--' '/tmp/reproducer.patch'
11:36:13.093487 git.c:344               trace: built-in: git 'mailsplit' '-d4' '-o.git/rebase-apply' '-b' '--' '/tmp/reproducer.patch'
Applying: Python traces are seen when tuned-adm verifiy -i is executed for cpu-partitioning profile.
Checking patch libexec/defirqaffinity.py...
error: while searching for:
	# now verify each /proc/irq/$num/smp_affinity?
	interruptdirs = [ f for f in os.listdir(irqpath) if os.path.isdir(os.path.join(irqpath,f)) ]?
	# IRQ 2 - cascaded signals from IRQs 8-15 (any devices configured to use IRQ 2 will actually be using IRQ 9)?
	interruptdirs.remove("2")?
	# IRQ 0 - system timer (cannot be changed)?
	interruptdirs.remove("0")?
?
	for i in interruptdirs:?
		inplacemask = 0?
		fname = irqpath + i + "/smp_affinity"?

error: patch failed: libexec/defirqaffinity.py:61
Applying patch libexec/defirqaffinity.py with 1 reject...
Rejected hunk #1.
Patch failed at 0001 Python traces are seen when tuned-adm verifiy -i is executed for cpu-partitioning profile.
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

The "?" made me suspicious that it was a whitespace issue.  Checking the content of the extracted patch shows it's got CRLF line ends:

$ file .git/rebase-apply/patch 
.git/rebase-apply/patch: unified diff output, ASCII text, with CRLF, LF line terminators

This is present in the base64-decoded output as well:

$ sed '1,/^$/d' /tmp/reproducer.patch | base64 -d - | file -
/dev/stdin: unified diff output, ASCII text, with CRLF, LF line terminators

It does seem that git is better able to apply the patch after it's decoded and it might be nice if it did that when decoding base64 content as well.  But IMO, the patch is fundamentally whitespace damaged.  (Whether that damage came from the sender or was mangled by the mailing-list or other software on route, I can't say.)

Additionally, as well as the bogus CRLF line ends, the patch uses spaces for indentation rather than tabs as the rest of the code does.  With python, this is at best sloppy and at worst a bug which will prevent proper execution.  I tend to include -tt in the shebang of my python scripts make such random mixing of tabs and spaces an error:

$ python --help
...
-t     : issue warnings about inconsistent tab usage (-tt: issue errors)

(I am more sensitive to whitespace than many people though.  I've seen it cause plenty of bugs in shell scripts, python code, and elsewhere.)

Comment 3 Todd Zullinger 2018-01-25 20:30:38 UTC
I'm going to close this as the problem looks to be in the whitespace-damaged patch more than in git.  If someone really wants to improve git's error handling of such whitespace-damaged & base64-encoded patch files, it's probably best to do that work upstream.


Note You need to log in before you can comment on or make changes to this bug.