kristofferhaugsbakk@xxxxxxxxxxxx writes: > See the review on the patch: > > https://lore.kernel.org/git/xmqq4jp326oj.fsf@gitster.g/ > > There was concern about the order of options: > > > ... One more thing need to note is "the order of > > the options matter", [...] > > This sounds more like a design/implementation mistake that we may > want to fix. That is a comment on the 8th iteration of the series, which is different from what you are documenting, which is 11th if I am reading the thread correctly. Comparing [v8 6/6] with [v11 6/7], https://lore.kernel.org/git/f60f743203d78a489b90df81472e71391b45367d.1682429602.git.dyroneteng@xxxxxxxxx/ https://lore.kernel.org/git/19865941bb29c28a690ed3e28fc4f6870e352923.1685174012.git.dyroneteng@xxxxxxxxx/ I notice that the later version has .stripspace member in note_msg as well as note_data. note_data has a collection of multiple note_msg instances. Each of note_msg comes from various sources, like '-m', '-C', etc., and each of these types have hardcoded default values for note_msg.stripspace (mostly to strip but note_msg that comes from an existing note blob has stripspace unset). When concat_messages() takes note_msg instances in a note_data together, it _could_ do: - if note_data.stripspace is explicitly specified, use that, otherwise use note_msg.stripspace, to cleanse the message contained in note_msg. and concatenate the results. But even at v11 of the series, the implementation of concat_messages() uses the stripspace determined to instead cleanse the concatenated result. Which I think is simply buggy. What it should do probably is - Loop over d->messages[] and: - if d->stripspace is set or d->messages[]->stripspace is set, cleanse the message (i.e. d->messages[]->buf) and append to d->buf - otherwise append d->messages[]->buf as-is to d->buf - If d->stripspace is set, cleanse d->buf (may not be necessary, as the messages that are concatenated have _all_ lost leading and trailing blank lines before concatenation). and that would truly fix the problem pointed out during the review of v8. > +-- > ++ > +`--stripspace` is the default except for > +`-C`/`--reuse-message`. However, keep in mind that this depends on the > +order of similar options. For example, for `-C <object> -m<message>`, > +`--stripspace` will be used because the default for `-m` overrides the > +previous `-C`. So, I think this is an accurate description of the current behaviour, but we might want to phrase the part that comes after "however" as documenting a known bug, which we would want to fix later. Thanks.
