On Sun, Oct 3, 2021 at 6:12 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > > This is an RFC proposed v4 of Elijah's en/removing-untracked-fixes > series[1] based on top of my memory leak fixes in the "unpack-trees" & > "dir" APIs[2]. > > As noted in [2] Elijah and I have been having a back & forth about the > approach his series takes to fixing memory leaks in those APIs. I > think submitting working code is more productive than continuing that > point-by-point discussion, so here we are. > > I've avoided making any changes to this series except those narrowly > required to rebase it on top of mine, and to those parts of Elijah's > commit messages that became outdated as a result. In particular > 3/10[3]'s is significantly changed, as much of its commit message > dicusses complexities that have gone away due to my preceding > series[2]. > > The "make dir an internal-only struct" has been replaced by a commit > that renames that struct member from "dir" to "private_dir". I think > even that is unnecessary as argued in [4], but I think the judgement > that something must be done to address that is Elijah's design > decision, so I did my best to retain it. > > I did drop the dynamic allocation & it being a pointer, since with my > preceding [2] and subsequent unsubmitted memory leak fixes I've got on > top having it be embedded in "struct unpack_trees_options" makes > things easier to manage. > > Havingn read through all this code quite thoroughly at this point I do > have other comments on it, but I'll reserve those until we've found > out what direction we're going forward with vis-a-vis what this will > be based on top of. > > I'm (obviously) hoping for an answer of either on top of my series[2], > or alternatively that Elijah's series can stick to introducing the > "preserve_ignored" flag, but not change how the memory > management/name/type of the embedded "dir" happens (and we could thus > proceed in parallel). ??? This really bothers me. I'm not quite sure how to put this into words, so let me just try my best. Let me start out by saying that I think you often provide good feedback and ideas. Sure, I sometimes don't agree with some of the feedback or ideas, but overall your feedback and contributions are definitely valuable. I also think your other series you rebased this on has some good ideas and some good bugfixes. There is something that seems off here, though. In this particular case, to start with, Junio already said let's take v3 as-is[1]. So your series should be rebased on mine, not vice-versa. Further while your other series that you are basing this on has some memory leak fixes; to me, it mostly looks like refactorings for stylistic code changes. Even though some of those stylistic changes are good, making a series such as mine that includes bugfixes (to a user reported bug no less), after multiple rounds and most reviewers are fine with it, suddenly depend on a new big and unrelated treewide stylistic refactoring series feels very off to me. But that doesn't quite fully explain my misgivings either; there's a bit more: * Junio has referred to several of your series as "Meh" and "code churn". That makes me think we'd have a higher than normal chance of a user-reported bug ending up blocked on unrelated stylistic changes. (Two of them actually, since I have another series depending on this one that I've waited to submit until this merges to next.) * Your stylistic refactorings also manage to confuse the code in merge-recursive.c, overall making the code potentially much harder to understand[2][3]. And you open a foot-gun in clear_unpack_trees_porcelain[3]. * At least half the series of yours I've reviewed have had significant bugs[4][5][6] (in addition to [2] and [3]). This would be fine if it was complex code that had bugs we were fixing, or if we were adding new features, but: * You submit a huge volume of patches, with a very disproportionately high ratio of stylistic refactorings rather than bugfixes and new features. (This is by no means bad on its own, it's the combination of this with other factors.) * You misrepresent my changes in multiple ways, including ways I had pointed out corrections for in our previous discussions (including some of which you acknowledged and agreed with), and you do so even after you have rebased my patches and added your signed-off-by to them suggesting you ought to be familiar with them[7]. So, I guess trying to distill what bugs me, I'd say: it seems to me that you have ignored what Junio said about taking my series, and then you rebased my series on top of unrelated stylistic churn, with that churn containing three issues that trigger ongoing misgivings I have about the care being put behind these refactorings, especially considering their value compared to the features and bugfixes we are getting, and you seem to fail to try to understand my changes and misrepresent them in the process. I hope I'm not overreacting but something feels wrong to me here. A big thumbs down on this reroll. [1] https://lore.kernel.org/git/xmqq35pk8ylz.fsf@gitster.g/ [2] https://lore.kernel.org/git/CABPp-BFYxWXZQXvDSrM1Y1ZaQ1d2TENQDvx1cyawvrDO1OvExA@xxxxxxxxxxxxxx/ [3] https://lore.kernel.org/git/CABPp-BH4ubjJ98Nvgp2iyKxmU9X+ypw4m1o=iL9Z4vSNZ-QTDw@xxxxxxxxxxxxxx/ [4] https://lore.kernel.org/git/CABPp-BGE+e1er6qFuG90j9+eVG34O8TN=imX=jtcb+jbQjN1QQ@xxxxxxxxxxxxxx/ [5] https://lore.kernel.org/git/CABPp-BEPkukGz32rrro1hzMvSQzX4v7U17CAcV-G2NS6v0u55g@xxxxxxxxxxxxxx/ [6] https://lore.kernel.org/git/xmqqfstppxzm.fsf@gitster.g/ [Note: problem was flagged by j6t; I was about to flag the same problem when I noticed he had already done so.] [7] https://lore.kernel.org/git/CABPp-BEr28xzbpEZc5dq-RVDupXy+h-+PH6CoANF4e0kmxqf0Q@xxxxxxxxxxxxxx/
