From: Junio C Hamano Date: Mon, 02 Sep 2019 18:29:56 GMT Subject: Re: [PATCH 1/1] rebase -r: let `label` generate safer labels Message-ID: In-Reply-To: <444f3ec4-abdf-1aa9-e8a8-8b5346b939e8@gmail.com> Phillip Wood writes: > Being picking I'll point out that ':' is not a valid in refs > either. Looking at > https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file I > think only " and | are not allowed on NTFS/FAT but are valid in refs > (see the man page for git check-ref-format for all the details). So > the main limitation is actually what git allows in refs. Yeah, trying to use the contents of the log message without sufficient sanitization is looking for trouble. >> for (p1 = label.buf; *p1; p1++) >> - if (isspace(*p1)) >> + if (!(*p1 & 0x80) && !isalnum(*p1)) >> *(char *)p1 = '-'; > > 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. I'd rather see 'x' used instead of '-' (double-or-more dashes and leading dash in refnames may currently be allowed but double-or-more exes and leading ex would be much more likely to stay valid) if we just want to redact invalid characters. I see there are "lets make sure it is unique by suffixing "-%d" in other codepaths; would that help if this piece of code yields a label that is not unique?