On Wed, Nov 03, 2021 at 11:23:28AM +0000, Phillip Wood wrote: > > That description makes sense, and the patch matches. Not being that > > familiar with this area, my biggest question would be: are there are > > other cases that would need the same treatment? And is there a way we > > can make it easier to avoid forgetting such a case in the future? > > I don't think there are any other cases (but then I thought that when I > wrote the buggy patch...). The only time we change the authorship is if the > user passes --committer-date-is-author-date or --reset-author-date. I agree > it would be good to have a way to avoid this problem in the future but I > haven't come up with an easy way to do that. One possibility would be to go > back to always reading the author script. That would mean revisiting the > changes to do_merge() in baf8ec8d3a so that it always writes the author > script and .git/MERGE_MSG but removes them when fast-forwarding (the problem > that baf8ec8d3a tried to solve was a left over .git/MERGE_MSG when > do_merge() fast-forwarded) I don't want to do that in the rc window though. I suspected the answers to my questions were "I hope so" and "not really", which I think matches what you wrote. :) If there isn't an easy way to make it more future-proof, then I'm content that you've given it some thought and didn't find any other cases. We can proceed from here with this fix, and be on the lookout for any other cases that people report (on the plus side, the BUG() made it quite obvious that there was a problem, rather than a subtle behavior change). > Thanks for your comments, are you happy for this to go in as is or should I > look at simplifying the conditional? I'm happy enough with it. I don't know what the plan is for the -rc period, though. AFAICT the bug is in v2.33.1, so it's not technically a v2.34-rc problem. It could wait for the next maint release. -Peff
