Re: [PATCH v10 3/5] history: add squash subcommand to fold a range

[Phillip Wood <phillip.wood123@gmail.com>]

On 04/08/2026 22:12, Junio C Hamano wrote:
> Harald Nordgren <haraldnordgren@gmail.com> writes:
> 
>> Hi Phillip and Junio!
>>
>> I admit I’m trying to expedite the process. Especially given what
>> feels like endless review cycles, and some frustration that this topic
>> was queued for ’master’ and now it feels like we’re miles away again.
>> I recognize that this mindset is not a good collaborative attitude, so
>> I’ll try to step back and re-adjust my mindset.
>>
>> I am used to a work process where moving fast is paramount, and good
>> enough is good enough in terms of code quality.

We don't have the luxury of being able to easily roll-back features that 
have been merged and are being used in the wider world so need to have a 
considered approach to designing new features.

Hardly anyone who is employed to work on git is given work time to 
review random patches that are not of direct interest to their employer, 
   the reviews are mostly from people volunteering their own time. 
Therefore comparing it to what happens inside a company where it is part 
of the job to review others code is not a realistic comparison.

>> I’m not seeing a
>> healthy code review process here — any feedback halts the process
>> completely. Should it really take months to merge a topic?

I'm not sure what you mean by "any feedback halts the process 
completely". If you spot something you want to improve, you can flag 
that on the list and still work on your patches while you're waiting for 
a review. Though, if you're frequently doing that, then it is an 
indication that you're posting patches that are not ready for review.

The communication around the code is just as important as the code 
itself and, especially in the early stages of a topic, when the design 
is being thrashed out, it is more important than the code. Sending out a 
re-roll immediately in response to every comment is not productive, as 
it leads to a multitude of different versions with small changes between 
them. It's far better to reply to any questions and comments, and allow 
others time to comment as well, so that we reach a consensus before the 
next set of patches are posted. If you're not following other topics on 
the list then it might be a good idea to start - you'll see others are 
not posting 20 revisions before their work is merged.

> When you reroll too fast without waiting for reviewers, it
> invalidates almost-done-but-yet-unpublished reviews.  If you
> repeatedly do so, it exacerbates the problem by discouraging
> reviewers from even looking at your topic, as they wonder if a
> rapid reroll will invalidate their reviews yet again.

Yes, I've definitely waited a couple of days to see if another iteration 
is going to appear before starting a review.

> If it is taking months, I suspect that half of that is due to your
> rapid rerolls, which have discouraged people from engaging, as they
> cannot make a time commitment to a bottomless time sink.  The summer
> being a relatively slow season certainly has played a role, too.

I've found these series taking more time than I'd like to review. Part 
of that is the rapid re-rolls, the other part is the very brief cover 
letters that are not very detailed or explicit with regard to what 
exactly has changed in the implementation and behavior [1] since the 
last version.

I'm going to be off the list next week, so it be a while before I look 
at these patches again. I'm hoping to see a reply to [2] when I'm back.

Thanks

Phillip

[1] For example in <pull.2285.v20.git.git.1784704238.gitgitgadget@gmail.com>

     * Protect branches transitively required by a surviving local
       upstream stack. Traverse upstream chains once and defer delete-set
       mutation until traversal completes.

That was actually a change in behavior from the previous version but it 
was not clear to me at all. It would have been much more helpful to 
explicitly call that out as a change in behavior and explain why the 
behavior had changed.

[2] 
https://lore.kernel.org/git/ddd0160c-7f4c-41c7-855f-58288db00050@gmail.com