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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 4, 2021, 16:08 UTC
Message-ID: <87bl44n070.fsf@evledraar.gmail.com>
In-Reply-To: <CABPp-BEaJAG=pqyjCR93YUoqj06WwSqjiPENDHjgSTWwzd_C2A@mail.gmail.com>

On Mon, Oct 04 2021, Elijah Newren wrote:

Show 49 quoted lines

> 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.

Just for Junio / anyone else following along: let's drop this RFC & the relateded/proposed "unpack-trees & dir APIs: fix memory leaks". Point-by-point commentary below (probably not needed/interesting for those just interested in the state of those two serieses).

> 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.

I understand your annoyance at that, I wouldn't have submitted this if I'd seen that before, I somehow managed to miss that mail in my mail queue. I believed the status was at "Will merge to 'next'?" upthread of [1].

We've then been having an extended back & forth about how to manage "private" data/structs/leak patterns starting at https://lore.kernel.org/git/87ilyjviiy.fsf@evledraar.gmail.com/.

At least some of which has been confused by my having quoted a working but out of context diff from what I ended up submitting as https://lore.kernel.org/git/cover-00.10-00000000000-20211004T002226Z-avarab@gmail.com/

So, sorry about stepping on your toes. I figured having a discussion with working patches would be more productive, and that it would help focus on the important changes in your series.

E.g. your 2nd and 3rd patch setup a "dir_clear()" that your 4th consolidates, which as shown in this series are intermediate steps that can be skipped. So perhaps there's some added churn, but also reduced churn in your resulting series on top...

> 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. [...]

Are you referring to the s/memset/UNPACK_TREES_OPTIONS_INIT/ bulk change at the start?

I agree that it's not strictly necessary, but it's pretty much the same as your earlier eceba532141 (dir: fix problematic API to avoid memory leaks, 2020-08-18), and makes e.g. a later change you seemed to like possible: https://lore.kernel.org/git/CABPp-BFpyyJ-e8p5fbmCvyaEsfUow=RP45Nw0ckiwNEvVC4zrg@mail.gmail.com/

Show 6 quoted lines

> 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:

[...]

Show 5 quoted lines

>   * 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.)

I'll stay out of this area for a while. Sorry about that.

Show 6 quoted lines

> [...]
>   * 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].

You're absolutely right about that, and the comment starting at "*Sigh*" in your linked [7] is entirely accurate. I'd like to apologize for that.

If I was in your shoes I'd be *very* annoyed at that chain from [7] to the upthread cover-letter here making that false claim again.

For what it's worth I do know that your patches aren't allocating the "struct dir_struct" on the heap, having rebased them etc. But through some combination of a brainfart and it being late when I wrote that CL last night I falsely claimed that they did, PBCAK.

What I *meant* to say was some summary that the your series's end state has a pointer to a dir struct that's dynamically set up v.s. mine of just initializing it via the macro from the start.

Show 9 quoted lines

> 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.

I don't think you're overreacting, and sorry again. Hopefully it helps somewhat that I for the "ignoring Junio [and charging ahead with this]" and the 2nd false claim about about heap allocation I was (believe it or not) just honestly mistaken instead of trying to get on your nerves.

As some of my early feedback on the whole topic of gitignore related-shredding/precious etc. should hopefully indicate I'm really happy that you've picked up this topic.

I thought this would help it along, but that's clearly not the case. Sorry again.