Hi Junio, On Tue, 3 Sep 2019, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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
