From: Phillip Wood Date: Sat, 20 Jan 2024 18:23:16 GMT Subject: Re: [PATCH v3] merge-ll: expose revision names to custom drivers Message-ID: <7dbe4006-bfc2-402e-8073-870d7ec38319@gmail.com> In-Reply-To: Hi Junio On 20/01/2024 17:37, Junio C Hamano wrote: > Phillip Wood writes: > >> Not part of this patch but I noticed that we're passing the filenames >> for '%A' etc. unquoted which is a bit scary. > > May be scary but safe, as long as create_temp() gives a reasonable > temporary filename. We pass ".merge_file_XXXXXX" to xmkstemp(), > which calls into mkstemp(), which should give us a shell safe name? Yes. I'd mis-read create_temp() and thought we were appending ".merge_file_XXXXX" to the path being merged but looking at it again it is safe. > It also should be a safe conversion to change strbuf_addstr() used > for these three to sq_quote_buf(), as the string with these %[OAB] > placeholders are passed to the shell that eats the quoting before > invoking the end-user supplied external merge driver, which means > the merge driver would not notice any difference. I agree that would be a safe conversion , but I'm not sure it is worth it. Best Wishes Phillip