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. > 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.) 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? -George
