On 09/12/2018 06:24 PM, Ján Tomko wrote: > On Wed, Sep 12, 2018 at 09:45:32AM -0400, John Ferlan wrote: >> >> >> On 09/12/2018 09:35 AM, Andrea Bolognani wrote: >>> On Wed, 2018-09-12 at 09:09 -0400, John Ferlan wrote: >>>> Any chance this can wait? Would be nice to have other series posted >>>> upstream that have changes to .xml and .replies files to add new >>>> functionality be processed first. >>>> >>>> There's a series to use - memfd from Marc-Andre Laurent that gets >>>> impacted. > > * Lureau > right, my fingers not typing what I'm thinking, mea culpa. >>> >>> I see you've already had to rebase locally to apply the patches at >>> all, due to other changes being pushed in the meantime, so I don't >>> see how pushing this series too would make it any worse. >>> >>> Also IIUC you've asked Marc-André to make some changes that depend >>> on *another series* of yours, that still hasn't been pushed, which >>> means a respin will be necessary either way, won't it? >>> >>> All in all I see no reason to dealy pushing this. >>> >> >> OK - go ahead. It was just a "would be nice" type inquiry. > > I don't see how that negates the need to post another version, > because the intermediate diffs got too hard to follow. > I don't see them as too hard to follow. In fact they're pretty simple as long as you have seen the code. I saw no need to request a repost unless it was because of the .replies differences. The hardest part is the move of the code from domain_conf to qemu_domain and since I requested and made those changes, I can see 'reason' to assist with adjusting the patches I was reviewing. Managing the capabilities conflict was less trivial and I certainly don't remember all the names of the various tools that were created that are supposed to help in the endeavor. >> It's not the >> first time capability changes cause conflicts and it won't be the last >> unless we come up with a better process for them. > > What's wrong with the current process? 1. It's not orderly. Review of upstream series is "ad-hoc" at best. Some series get immediate review, ack, and push. Sometimes certain series languish for no apparent reason. When those series contain capabilities changes it can quickly creates conflict. 2. At certain times there seems to be a "run" or "rush" to make capabilities changes based on priorities perhaps not related to upstream that create conflicts for patches that are languishing. 3. I think if we agree something is "good to add" capability wise and even though all the code isn't ready, the capability changes could go forward while perhaps the resolution of all review comments is handled. Yes, it's more of a problem when .replies changes. > The capabilities conflicts are trivial to resolve and we have automated > tools for most of it (VIR_TEST_REGENRATE_OUTPUT, group-qemu-caps.pl, > qemucapsfixreplies) > OK sometimes trivial to resolve... If one can keep up to date with changes or 'git am' doesn't have conflicts trying to apply changes. One has to go back in time with git checkout to a series before the update that caused the conflict, git apply patches, then --set-upstream-to=origin/master, and then git pull --rebase. And then you get to try to use any of the tools to help resolve conflicts - if you know about them and if they work; otherwise, it's a hand-edit or request to repost. Unless you know of some other neat trick or two... How would qemucapsfixreplies fix the environment where git am fails to do the merge. I didn't recall the name when processing the .replies files. I'm also not sure having taken a quick peak at the code whether it would have helped since there was "conflict" as a result of 4 replies being removed and 1 reply being added in a very similar spot in the file. As noted in the review - whenever I've created .replies files from scratch using the tools - something in my process doesn't create output in the exact format some seem to expect, so I've given up creating them and just wait for someone else to do it. > Also, Andrea's series seems to be innocent in this - the removed > capabilities do not alter .replies and the .[ch] file changes are > contained to the middle of the capabilities array, so they should > be resolved trivially by git. > True... These were, but considering they were reviewed 16 minutes after posting and the previous series was reviewed similarly as quick and those caused merge conflict issues - I reacted to just the fact that capabilities changes were taking place and asked to consider the "orderly processing" of posted patches. I didn't dig into them because I was in the middle of something else. John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
