From: Daniel Harding <dharding@living180.net>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: "brian m. carlson" <sandals@crustytoothpaste.net>, git@vger.kernel.org
Subject: Re: [PATCH 2/2] t3430: update to test with custom commentChar
Date: Tue, 10 Jul 2018 16:49:14 +0300 [thread overview]
Message-ID: <9628abb4-86ec-a6f1-0c9c-64458949ebdb@living180.net> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1807101504530.75@tvgsbejvaqbjf.bet>
Hi Johannes,
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?
> 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.
Daniel Harding
next prev parent reply other threads:[~2018-07-10 13:49 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-08 18:41 [PATCH 0/2] Fix --rebase-merges with custom commentChar Daniel Harding
2018-07-08 18:41 ` [PATCH 1/2] sequencer: fix " Daniel Harding
2018-07-08 18:41 ` [PATCH 2/2] t3430: update to test " Daniel Harding
2018-07-08 21:02 ` brian m. carlson
2018-07-09 7:52 ` Johannes Schindelin
2018-07-09 16:46 ` Junio C Hamano
2018-07-09 18:22 ` Daniel Harding
2018-07-09 19:09 ` Johannes Schindelin
2018-07-09 20:05 ` Junio C Hamano
2018-07-09 18:48 ` Daniel Harding
2018-07-09 19:14 ` Johannes Schindelin
2018-07-10 12:29 ` Daniel Harding
2018-07-10 13:08 ` Johannes Schindelin
2018-07-10 13:49 ` Daniel Harding [this message]
2018-10-02 14:38 ` Johannes Schindelin
2018-07-09 23:41 ` brian m. carlson
2018-07-09 7:53 ` [PATCH 0/2] Fix --rebase-merges " Johannes Schindelin
2018-07-10 13:24 ` Daniel Harding
2018-07-12 3:02 ` Aaron Schrab
2018-07-12 17:15 ` Junio C Hamano
2018-07-16 4:59 ` [PATCH v3] sequencer: use configured comment character Aaron Schrab
2018-07-16 15:59 ` Johannes Schindelin
2018-07-16 18:49 ` Daniel Harding
2018-07-17 16:46 ` Johannes Schindelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9628abb4-86ec-a6f1-0c9c-64458949ebdb@living180.net \
--to=dharding@living180.net \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=sandals@crustytoothpaste.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.