Re: [PATCH 1/1] rebase -r: let `label` generate safer labels
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Nov 18, 2019, 20:51 UTC
- Message-ID
- <nycvar.QRO.7.76.6.1911182150330.46@tvgsbejvaqbjf.bet>
- In-Reply-To
- <xmqqo901tfn8.fsf@gitster-ct.c.googlers.com>
Hi Junio,
On Tue, 3 Sep 2019, Junio C Hamano wrote:
Show 18 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > >> I'm sightly concerned that this opens the possibility for unexpected effects > >> if two different labels get sanitized to the same string. I suspect it's > >> unlikely to happen in practice but doing something like percent encoding > >> non-alphanumeric characters would avoid the problem entirely. > > > > Oh, but we make sure that the labels are unique, via the `label_oid()` > > function! Otherwise, we would not be able to label more than one merge > > parent ;-) > > It somewhat feels suboptimal, from code followability's point of > view, to have this "pre-sanitization" to replace isspace() to a > dash, which is being extended to "all non-alnums", and the uniquefy > of labels in label_oid(), in two separate places. I wonder if the > resulting code becomes easier to follow and harder to introduce new > bugs, if this part is made to just yield label.buf it obtained form > the log message as-is and leave the munging to label_oid()?
While the patch looks somewhat bulky without `-w` (due to the indentation change), I agree that it is conceptually a good idea, therefore I made it so.
Ciao, Dscho