Hi Daniel, [I forgot to address this mail earlier, my apologies] On Tue, 10 Jul 2018, Daniel Harding wrote: > On Tue, 10 Jul 2018 at 16:08:57 +0300, Johannes Schindelin wrote:> > > On Tue, 10 Jul 2018, Daniel Harding wrote: > > > > > On Mon, 09 Jul 2018 at 22:14:58 +0300, Johannes Schindelin wrote: > > > > > > > > On Mon, 9 Jul 2018, Daniel Harding wrote: > > > > > > > > > > On Mon, 09 Jul 2018 at 00:02:00 +0300, brian m. carlson wrote: > > > > > > > > > > > > Should this affect the "# Merge the topic branch" line (and the "# > > > > > > C", > > > > > > "# E", and "# H" lines in the next test) that appears below this? > > > > > > It > > > > > > would seem those would qualify as comments as well. > > > > > > > > > > I intentionally did not change that behavior for two reasons: > > > > > > > > > > a) from a Git perspective, comment characters are only effectual for > > > > > comments > > > > > if they are the first character in a line > > > > > > > > > > and > > > > > > > > > > b) there are places where a '#' character from the todo list is > > > > > actually > > > > > parsed and used e.g. [0] and [1]. I have not yet gotten to the point > > > > > of > > > > > grokking what is going on there, so I didn't want to risk breaking > > > > > something I > > > > > didn't understand. Perhaps Johannes could shed some light on whether > > > > > the > > > > > cases you mentioned should be changed to use the configured > > > > > commentChar or > > > > > not. > > > > > > > > > > [0] > > > > > https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L2869 > > > > > [1] > > > > > https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L3797 > > > > > > > > These are related. The first one tries to support > > > > > > > > merge -C cafecafe second-branch third-branch # Octopus 2nd/3rd branch > > > > > > > > i.e. use '#' to separate between the commit(s) to merge and the oneline > > > > (the latter for the reader's pleasure, just like the onelines in the > > > > `pick > > > > <hash> <oneline>` lines. > > > > > > > > The second ensures that there is no valid label `#`. > > > > > > > > I have not really thought about the ramifications of changing this to > > > > comment_line_char, but I guess it *could* work if both locations were > > > > changed. > > > > > > Is there interest in such a change? I'm happy to take a stab at it if > > > there > > > is, otherwise I'll leave things as they are. > > > > I think it would be a fine change, once we convinced ourselves that it > > does not break things (I am a little worried about this because I remember > > just how long I had to reflect about the ramifications with regards to the > > label: `#` is a valid ref name, after all, and that was the reason why I > > had to treat it specially, and I wonder whether allowing arbitrary comment > > chars will require us to add more such special handling that is not > > necessary if we stick to `#`). > > Would it simpler/safer to perhaps put the oneline on its own commented line > above? I know it isn't quite consistent with the way onelines are displayed > for normal commits, but it might be a worthwhile tradeoff for the sake of the > code. As an idea of what I am suggesting, your example above would become > perhaps > > # Merge: Octopus 2nd/3rd branch > merge -C cafecafe second-branch third-branch > > or perhaps just > > # Octopus 2nd/3rd branch > merge -C cafecafe second-branch third-branch > > Thoughts? That is a very good idea, if you ask me! Unfortunately, I forgot about this suggestion, and IIUC v2.19.0 shipped with my previously-suggested version. But maybe you want to spend a little time on the code to change it according to your suggestion? The `merge` commands are generated here: https://github.com/git/git/blob/v2.19.0/sequencer.c#L4096-L4120 > > Not that the comment line char feature seems to be all that safe. I could > > imagine that setting it to ' ' (i.e. a single space) wreaks havoc with > > Git, and we have no safeguard to error out in this obviously broken case. > > Technically, I think a single space might actually work with commit messages > (at least, I can't off the top of my head think of a case where git would > insert a non-comment line starting with a space if it wasn't already present > in a commit message). But if someone were actually crazy enough to do that I > might suggest a diagnosis of "if it hurts, don't do that" rather than trying > to equip git defend against that sort of thing. I did want to have an extra special visual marker for users (such as myself), so a space would not quite be enough. That's why I settled for the comment char. Ciao, Johannes
