On 06/11/2023 00:50, Junio C Hamano wrote:
Andy Koppe <andy.koppe@xxxxxxxxx> writes: Our log message convention is to first describe what happens in the system in the present tense to illustrate why it is suboptimal, to prepare readers' minds to anticipate the solution, which is described next. When asking reviews on a new iteration [PATCH v(N+1)], please summarize the differences relative to [PATCH vN]. For explaining such incremental changes for individual patches, here between the three-dash line and the diffstat is the place to do so. When you have a cover letter [PATCH 0/X], it can be done in that messaage. Either way is OK. Doing both is also helpful as long as the explanation done in two places do not contradict with each other.
If you tried to format the documentation before sending this patch, you'd have seen the second paragraph formatted as if it were a code snippet. Dedent the second paragraph (and later ones if you had more than one extra paragraphs), and turn the blank line between the paragraphs into a line with "+" (and nothing else) on it. See the description of `--autosquash` option in Documentation/git-rebase.txt for an example.
Sad thing is that I knew most of that from reading the contribution guidelines and previous experience, but obviously I don't always remember. So thanks for you patience in re-explaining that.
OK, by clearing opts->config_autosquash in this function, you keep the rebase.autosquash to be "the last one wins" as a whole. If a configuration file with lower precedence (e.g., /etc/gitconfig) says "[rebase] autosquash" to set it to "interactive,no-interactive", a separate setting in your ~/.gitconfig "[rebase] autosquash = false" would override both bits. A more involved design may let the users override these bits independently by allowing something like "!no-i" (take whatever the lower precedence configuration file says for the interactive case, but disable autosquash when running a non-interactive rebase) as the value, but I think the approach taken by this patch to allow replacing as a whole is OK. It is simpler to explain. Giving short-hands for often used command line options is one thing, but I do not think a short-hand is warranted here, especially when the other one needs to be a less-than-half legible "no-i" that does not allow "no-int" and friends, for configuration variable values. I'd strongly suggest dropping them.
Dropped in v4, along with the attempt to expand rebase.autoSquash, following Phillip's review.
Regards, Andy
