On 07.02.2024 04:07, George Dunlap wrote: > On Thu, Feb 1, 2024 at 10:15 PM Jan Beulich <[email protected]> wrote: >> On 01.02.2024 14:32, George Dunlap wrote: >>> On Thu, Jun 23, 2022 at 12:54 PM Jan Beulich <[email protected]> wrote: >>> >>>> By using | instead of || or (in the negated form) && chances increase >>>> for the compiler to recognize that both predicates can actually be >>>> folded into an expression requiring just a single branch (via OR-ing >>>> together the respective P2M_*_TYPES constants). >>>> >>>> Signed-off-by: Jan Beulich <[email protected]> >>>> >>> >>> Sorry for the delay. Git complains that this patch is malformed: >>> >>> error: `git apply --index`: error: corrupt patch at line 28 >>> >>> Similar complaint from patchew when it was posted: >>> >>> https://patchew.org/Xen/[email protected]/ >> >> Not sure what to say. The patch surely is well-formed. It applies fine >> using patch (when not taken from email). When taken from email, patch >> mentions that it strips CRs (I'm running my email client on Windows), >> but the saved email still applies fine. "git am" indeed is unhappy >> when taking the plain file as saved from email, albeit here with an >> error different from yours. If I edit the saved email to retain just >> the From: and Subject: tags, all is fine. >> > > That still doesn't work for me. > > >> I can't tell what git doesn't like. The error messages (the one you >> see and the one I got) tell me nothing. > > > The raw email looks like this: > > ``` > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -379,7 +379,7 @@ struct page_info *p2m_get_page_from_gfn( > return page; > =20 > /* Error path: not a suitable GFN at all */ > - if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) && !p2m_is_pod(*t) && > + if ( !(p2m_is_ram(*t) | p2m_is_paging(*t) | p2m_is_pod(*t)) && > !mem_sharing_is_fork(p2m->domain) ) > return NULL; > } > ``` > > Note the "=20" at the beginning of the empty line. Why `patch` handles it > but `git am` doesn't, who knows.
Hmm. Nothing like that seen when I save that mail. Plus I recall having an issue with this when applying patches coming from Shawn, where those =20 got in the way, but only if I pruned the saved email before handing to "git am". >> I'm also not aware of there >> being a requirement that patches I send via email need to be >> "git am"-able (unlike in xsa.git, where I edit patches enough to be >> suitable for that), nor am I aware how I would convince my email >> client and/or server to omit whatever git doesn't like or to add >> whatever git is missing. >> >> Bottom line - your response would be actionable by me only in so far >> as I could switch to using "git send-email". Which I'm afraid I'm not >> going to do unless left with no other choice. The way I've been >> sending patches has worked well for over 20 years, and for different >> projects. (I'm aware Andrew has some special "Jan" command to apply >> patches I send, but I don't know any specifics.) >> > > In the general case, I'm not going to review a patch without being able to > see it in context; and it's not reasonable to expect reviewers to have > specific contributor-specific scripts for doing so. If we run into this > issue in the future, and you want my review, you may have to post a git > tree somewhere, or attach the patch as an attachment or something. (Or you > can try to figure out why `git am` isn't working and try to upstream a fix.) Based on my own observation mentioned above, I assume "git am" is capable of dealing with the =20, provided some specific further encoding specification is present in the mail. Which I'd then have to assume is missing from what Thunderbird sends, or the =20 is being introduced without Thunderbird being involved. > That said, in this case, context isn't really necessary to understand the > change, so it won't be necessary. > > The logic of the change is obviously correct; but it definitely reduces the > readability. I kind of feel like whether this sort of optimization is > worth the benefits is more a general x86 maintainer policy decision. Maybe > we can talk about it at the next maintainer's meeting I'll be at? I see no problem doing so. Jan
