This is an archive of the discontinued LLVM Phabricator instance.

[docs] use git diff instead of git format-patch
ClosedPublic

Authored by AlexanderLanin on Jan 7 2020, 4:07 PM.

Details

Summary

Uploading output from git format-patch fails when version has more than 2 dots, e.g. git version 2.24.1.windows.2 which is currently recommended by e.g. GitExtensions or 2.24.1.rc on linux.

Phabricator complains with:

Diff Parse Exception: Expected a hunk header, like 'Index: /path/to/file.ext' (svn), 'Property changes on: /path/to/file.ext' (svn properties), 'commit 59bcc3ad6775562f845953cf01624225' (git show), 'diff --git' (git diff), '--- filename' (unified diff), or 'diff -r' (hg diff or patch).

        2318    .. _`https://reviews.llvm.org`: https://reviews.llvm.org
        2319    .. _Code Repository Browser: https://reviews.llvm.org/diffusion/
        2320    .. _Arcanist Quick Start: https://secure.phabricator.com/book/phabricator/article/arcanist_quick_start/
        2321    .. _Arcanist User Guide: https://secure.phabricator.com/book/phabricator/article/arcanist/
        2322    .. _llvm-reviews GitHub project: https://github.com/r4nt/llvm-reviews/
>>>     2323   -- 
        2324   2.24.1.windows.2
        2325

Diff Detail

Event Timeline

AlexanderLanin created this revision.Jan 7 2020, 4:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2020, 4:07 PM

The only difference is the git version in the end:

The only difference is the git version in the end:

I previously reported this here: https://bugs.llvm.org/show_bug.cgi?id=37834

I guess it's not so easy to fix as you reported this 1,5 years ago. That's an even better reason to update the documentation to work around the issue.

Hi, could anyone provide feedback here?
Windows and RC Versions of git do not work with format-patch.

Maybe that's not the majority of users that are affected, but still.
For me it was relevant since I use Windows GitExtensions for the GUI + WSL for the rest.
I personally see more and more people using similar setups.

AlexanderLanin edited the summary of this revision. (Show Details)Feb 1 2020, 1:20 PM
probinson accepted this revision.Mar 10 2020, 6:44 AM
probinson added a subscriber: probinson.

I normally use git diff even on Linux. If someone comes up with a reason why format-patch is better for generating a diff for Phab, they can add it back in.
LGTM

llvm/docs/Phabricator.rst
62–64

While you're here, please delete the svn line.

This revision is now accepted and ready to land.Mar 10 2020, 6:44 AM

Rebased. Solves the svn issue as it's gone in the meantime.

Could someone commit this as I cannot?
Alexander Lanin <alex@lanin.de>

Rebased. Solves the svn issue as it's gone in the meantime.

Could someone commit this as I cannot?
Alexander Lanin <alex@lanin.de>

Sorry--done now, llvmorg-11-init-7559-g6668453dd25

This revision was automatically updated to reflect the committed changes.
ps-19 added a subscriber: ps-19.Mar 29 2022, 11:24 AM
This comment was removed by ps-19.
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2022, 11:24 AM