[llvm-dev] amount of camelCase refactoring causing some downstream overhead
Nicolai Hähnle via llvm-dev
llvm-dev at lists.llvm.org
Fri Feb 21 01:39:51 PST 2020
On Thu, Feb 20, 2020 at 7:16 PM Mehdi AMINI via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>> What I’m aware of is a series of 5 separate patches, each of which re-capitalized a select subset of APIs in the same area. This fixing-up is *long overdue* and I’m very happy to see it done. But I will dispute the “reasonableness” of doing it in 5 separate stages (plus a few other related cleanups that were not simply re-capitalizing existing names).
>>
>> The notion of “reviewable size” is not relevant, because these patches were not reviewed (in accordance with current policy, that this kind of mechanical fixup is “obvious” and does not require pre-commit review).
>>
>> The patches were not really very distinct, with a HUGE overlap in the set of affected files.
>>
>>
>>
>> We do like to minimize churn; this kind of fixup clearly qualifies as churn, but I respectfully submit that the way it was done does not *minimize* the churn.
>>
>>
>>
>> And minimizing the churn is good for both the project and those of us who live downstream of it.
>
>
> If you ignore downstream users, and focus on upstream: what makes it the least risky and most convenient for the *upstream contributor* in this case? It seems they believed it was to split their patch up, which I would have done as well: if you can split it, then in most case you should.
> If the patches can land separately, to me their are distinct (they are not unrelated, but that's not a reason for merging them in a single commit). Just like David mentioned separately, I may have done this over a much longer series of individual commits.
As I have already stated in another email, every upstream contributor
is really their own little downstream as well. This is especially true
with git. If you're working upstream, but on a more complex feature,
then you can easily have a set of patches for some weeks -- and that's
before taking the slowness of the review process into account. So you
run into the same rebasing and merging problems that a "true
downstream" does. The point is, the distinction between "upstream
contributor" and "downstream" is a pretty fuzzy one for this kind of
thing.
Every time that a rebase on master causes a merge conflict is a
significant annoyance even for this use case. So if you're doing a
change that really should be mechanized anyway, then just do it all at
once.
For other kinds of changes, I agree with you though: if they're
logically separate, they should be in different commits.
Cheers,
Nicolai
--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
More information about the llvm-dev
mailing list