Re: [RFC PATCH v4 00/10] Fix various issues around removal of untracked files/directories

[Elijah Newren <newren@gmail.com>]

On Sun, Oct 3, 2021 at 6:12 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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@mail.gmail.com/
[3] https://lore.kernel.org/git/CABPp-BH4ubjJ98Nvgp2iyKxmU9X+ypw4m1o=iL9Z4vSNZ-QTDw@mail.gmail.com/
[4] https://lore.kernel.org/git/CABPp-BGE+e1er6qFuG90j9+eVG34O8TN=imX=jtcb+jbQjN1QQ@mail.gmail.com/
[5] https://lore.kernel.org/git/CABPp-BEPkukGz32rrro1hzMvSQzX4v7U17CAcV-G2NS6v0u55g@mail.gmail.com/
[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@mail.gmail.com/