> On 28 Mar 2018, at 17:45, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote: > > On Wed, Mar 28, 2018 at 10:47:27AM +0200, Christophe de Dinechin wrote: >>> What matters in my opinion is that we decide to break it, once we've >>> made that decision, the number of commits which are going to make ABI >>> breaking changes is less important. >> >> Introducing “subtle” changes such as having to rename a variable in the middle makes things harder to understand, not simpler. > > Ah, actually the only reason I changed its name was because I wanted > some less confusing name in the intermediate change. It's probably fine > not changing it so much ;) You were lucky, then. If you don’t change it, then it’s broken. > >> >>> >>>> and you rely on a “side effect” of changing the variable name to avoid >>>> conflicts that would otherwise arise with the version number alone? >>>> >>>> It makes the history fo the code harder to track, and the changes more >>>> “subtle". At the very least, I would add a commit log explaining that >>>> since you can’t rely on version numbers alone since the version >>>> numbering scheme changes, you renamed the variable used to track >>>> version numbering. >>> >>> I don't understand this part :) >> >> You had to rename “spice_streaming_agent_plugin_version” to “spice_streaming_agent_plugin_interface_version”. >> You need at least to explain why in the commit log. >> In my opinion, having to rename that variable proves that the changes I had put in a single patch were not “unrelated”. >> >>> >>>> Also, that means you need to follow the same patterns in locksteps for >>>> the plugins, so you are not just making the history of the agent >>>> complicated, but also the history of the plugins. >>> >>> Hmm how do we cope with ABI breakage with respect to plugins is an >>> interesting question. When the ABI is in flux in git, it's not going to >>> be easy to link an arbitrary plugin commit to the agent commit(s) which >>> have the ABI the plugin expect anyway, so I would say that we want git >>> master of the agent to work with git master of the plugins. I would not >>> require plugins to have at least one commit working with each commit of >>> the agent. >> >> That is not what I was talking about. >> With my change, you have one ABI change in the agent, which is easy to map to one ABI change in the plugin. That leads to 4 possible combinations, 2 of which are supposed to work. >> With your suggestion, you have two ABI changes in the agent, and two ABI changes in the plugin. That leads to 9 possible combinations, 3 of which are supposed to work. >> So it’s more complicated to do right, and more complicated to test. > > My point was, I don't really mind if for the intermediate commit, we > don't have a corresponding plugin change. Oh, I see. This is where I took this differently. If splitting the patch leads to an intermediate state that is broken, I’d rather not split. > So that's only 2 plugin > commits. And the intermediate agent commit will indeed not be working > with anything, as long as the series is pushed at once, I would say it's > acceptable even if suboptimal. So it’s suboptimal, but you still want to do it? Why? > In this specific case, we could probably > introduce the macro early, use that first thing in the plugin, and I > think we could have a plugin commit working with both agent commits (did > not try not looked again at the source). > ABI breaks are going to be messy anyway, and because of that I'm > strongly in the camp of "let's avoid breaking ABI as much as possible” That’s not my camp either. Actually, if I need a solid ABI version checking, it’s because I need to break the ABI for smart streaming, probably more than once. But I’m in the camp of not splitting a patch if the intermediate states are confusing or broken. >> >> >>> On that note, we should strive to never break plugin ABI, only extend >>> it. The plugins are in their infancy at the moment, so we can do some >>> breakage now while to put everything in place, but ideally we'd settle >>> on something stable "soon" >>> >>>> So overall, a lot of additional complication. What is the benefit? >>> >>> The benefit is that we don't have unrelated changes grouped together in >>> a big commit. >> >> Why do you call the changes unrelated? >> >> My definition of “unrelated” or “independent” changes are changes that >> commute with one another. I can apply one or the other, in any order, >> and it does not matter. > > Then it's a bad choice of words from me, this is not what I meant. But > from reading the commit log, we really have 3 separate things, you even > numbered them ;) The fact that I identified three issues that have the same root cause does not mean that I should fix the root cause three times :-) > Moving the version check to the agent is a functional > change, the way the ODR part is phrased make it look as a bugfix, but > it's probably mostly a consequence of moving the version check than > something we can separate. No, two bugs with the same root cause (possible to bypass version check is bug 1, likely crash during version check is bug 2), which is why they are hard to split, but are best fixed with a single change. > And then we've got the change of how we do > the version checking, which would be another logical change. You can play with words all you want, it’s still the same root cause. > If my task is to "move version check to the agent", do I _have_ to change > the semantics of the version check? No. Of course you have to. There is no “PluginVersionIsCompatible” anymore, etc, so the version number semantics have changed whether you like it or not. You may artificially try to make the new version number look like the old one, and I would have if there wasn’t another problem with that numbering. > If my task is to "change the > semantics of the version check", do I have to move it to the agent? No. Of course you have to if you have any interest in the semantics being correct. > I could implement both separately. This is why I see them as separate. > > Of course you are not going to be able to exchange them freely, this is > why they are sent as a single patch series. The split is not an improvement. > > Christophe _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel