On Fri, Jul 26, 2024 at 06:40:49PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@xxxxxxxxx> writes: > > > In any case, that is an appropriate thing to say in a commit that > > fixes use of such a construct, but not a commit that uses the right > > constuct from the get-go. > > > > I have to say that the [4/4] in the previous round, i.e., fc87b2f7 > > (add-patch: render hunks through the pager, 2024-07-25) in my tree, > > is better than this version. > > I do recall that you once had a version where the code violates the > guidelines (and breaks dash) in one patch, and gets fixed in another > patch in the same series. The above material would be a perfect fit > in the proposed log message of the latter step. If we spent so much > effort and digging to figure out exactly how it breaks with which > shell, a separate patch to fix, primarily to document the fix, would > have made sense. > > But the latest squashes the two and avoids making the mistake in the > first place, eliminating the need for a documented fix. We generally > prefer to do so to avoid breaking bisection (and the recommendation > to keep the fix separate so that we can document it better was made > as an exception), so squashing them into one is fine. But if we > commit to that approach to pretend that there was no silly mistake, > we should be consistent in pretending that is the case. > Fixing a problematic change with a new commit isn't the best idea if we have the opportunity to prevent the problem in the first place, as Phillip pointed out. Since rj/add-p-pager is still open, it's worthwhile to amend the problematic commit. Of course, we've now updated the documentation [*1*] and reinforced [*2*] the mechanisms to prevent this from happening again. However, I think adding a comment about the issue to the amended commit, which I think it has been suggested at some point, seems to me like a good addition. I do not believe that a future reading of the change will lead to confusion for this reason. The added comment does not document a fix, I think, but rather it is an explanation of what we're doing in the commit. Furthermore, we capture in the history, IMHO, notes of how things have happened, which is also why I intend to apply this series on 506f457e489b2097e2d4fc5ceffd6e242502b2bd, to only amend the last two commits. 1.- jc/doc-one-shot-export-with-shell-func 2.- es/shell-check-updates
