On Wed, Feb 27, 2019 at 11:05 PM Jeff King <peff@xxxxxxxx> wrote: > > On Wed, Feb 27, 2019 at 09:23:33AM -0500, Eric Sunshine wrote: > > > > If we just cared about saying "is this worktree name valid", I'd suggest > > > actually constructing a sample refname with the worktree name embedded > > > in it and feeding that to check_refname_format(). But because you want > > > to actually sanitize, I don't think there's an easy way to reuse it. > > > > > > So this approach is probably the best we can do, though I do still think > > > it's worth renaming that function (and/or putting a big warning comment > > > in front of it). > > > > The above arguments seem to suggest the introduction of a companion to > > check_refname_format() for sanitizing, perhaps named > > sanitize_refname_format(), in ref.[hc]. The potential difficulty with > > that is defining exactly what "sanitize" means. Will it be contextual? > > (That is, will git-worktree have differently sanitation needs than > > some other facility?) If so, perhaps a 'flags' argument could control > > how sanitization is done. > > I agree that sanitize_refname_format() would be nice, but I'm pretty > sure it's going to end up having to duplicate many of the rules from > check_refname_format(). Which is ugly if the two ever get out of sync. > > But if we could write it in a way that keeps the actual policy logic in > one factored-out portion, I think it would be worth doing. I think we could make check_refname_format() returns the bad position and several different error codes depending on context. Then sanitize_.. can just repeatedly call check_refname_format and fix up whatever error it reports. Performance goes straight to hell but I don't think that's a big deal for git-worktree, and it keeps check_refname_format() simple (relatively speaking). The second option is make check_refname_format() call some callback instead of returning error. This allows sanitize_ to fix up in one go (inside the callback), but check_refname_format could be a lot uglier, and verifying all refs (I think pack-refs does this?) could also be slowed down. -- Duy
