On Mon, Apr 14, 2025 at 08:19:15PM +0100, Ramsay Jones wrote:
>
>
> On 14/04/2025 08:54, Patrick Steinhardt wrote:
> > On Sun, Apr 06, 2025 at 08:49:54PM +0100, Ramsay Jones wrote:
> >>
> >>
> >> On 06/04/2025 20:38, Ramsay Jones wrote:
> >> [snip]
> >>> diff --git a/meson.build b/meson.build
> >>> index 88a29fd043..efd0bd3319 100644
> >>> --- a/meson.build
> >>> +++ b/meson.build
> >>> @@ -693,10 +693,8 @@ endif
> >>> # These variables are used for building libgit.a.
> >>> libgit_c_args = [
> >>> '-DBINDIR="' + get_option('bindir') + '"',
> >>> - '-DDEFAULT_EDITOR="' + get_option('default_editor') + '"',
> >>> '-DDEFAULT_GIT_TEMPLATE_DIR="' + get_option('datadir') / 'git-core/templates' + '"',
> >>> '-DDEFAULT_HELP_FORMAT="' + get_option('default_help_format') + '"',
> >>> - '-DDEFAULT_PAGER="' + get_option('default_pager') + '"',
> >>> '-DETC_GITATTRIBUTES="' + get_option('gitattributes') + '"',
> >>> '-DETC_GITCONFIG="' + get_option('gitconfig') + '"',
> >>> '-DFALLBACK_RUNTIME_PREFIX="' + get_option('prefix') + '"',
> >>> @@ -708,6 +706,17 @@ libgit_c_args = [
> >>> '-DPAGER_ENV="' + get_option('pager_environment') + '"',
> >>> '-DSHELL_PATH="' + fs.as_posix(shell.full_path()) + '"',
> >>> ]
> >>> +
> >>> +editor_opt = get_option('default_editor')
> >>> +if editor_opt != '' and editor_opt != 'vi'
> >>> + libgit_c_args += '-DDEFAULT_EDITOR="' + editor_opt + '"'
> >>> +endif
> >>> +
> >>> +pager_opt = get_option('default_pager')
> >>> +if pager_opt != '' and pager_opt != 'less'
> >>> + libgit_c_args += '-DDEFAULT_PAGER="' + pager_opt + '"'
> >>> +endif
> >>> +
> >>> libgit_include_directories = [ '.' ]
> >>> libgit_dependencies = [ ]
> >>>
> >>
> >>
> >> It would be somewhat remiss of me to not mention here that this does not
> >> work for any but the simplest of values! :( If you set a simple single
> >> 'bareword' like 'vim' or 'more' (even '~/bin/vi') then every thing works
> >> just fine. However, if the value contains any of (at least) the following
> >> characters: single quote, double quote or backslash, then things
> >> stop working!
> >>
> >> [I spent one whole evening (and a bit - always something else to 'try')
> >> trying to 'fix' this problem, without success]
> >
> > Shouldn't it be possible to escape these values via `.replace()` [1]? I
> > suspect that you already tried, but wanted to ask anyway :)
>
> Yep. :)
>
> I still haven't studied the meson documentation, but when I searched
> for variations of 'quotes', the results showed that '... if you want
> quotes, you will have to do it yourself ...'. So, I eventually found
> '.replace()' in the 'string operations' section of the docs and tried
> to reproduce what the Makefile does (see #2382):
>
>
> ifdef DEFAULT_EDITOR
> DEFAULT_EDITOR_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_EDITOR)))"
> DEFAULT_EDITOR_CQ_SQ = $(subst ','\'',$(DEFAULT_EDITOR_CQ))
>
> BASIC_CFLAGS += -DDEFAULT_EDITOR='$(DEFAULT_EDITOR_CQ_SQ)'
> endif
>
> which I translated into (on top of these patches):
>
> diff --git a/meson.build b/meson.build
> index 8f8a258064..608d665fd3 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -708,7 +708,11 @@ libgit_c_args = [
>
> editor_opt = get_option('default_editor')
> if editor_opt != '' and editor_opt != 'vi'
> - libgit_c_args += '-DDEFAULT_EDITOR="' + editor_opt + '"'
> + editor_opt = editor_opt.replace('\\', '\\\\')
> + editor_opt = editor_opt.replace('"', '\"')
> + editor_opt = '"' + editor_opt + '"'
> + editor_opt = editor_opt.replace('\'', '\\\'')
> + libgit_c_args += '-DDEFAULT_EDITOR=' + editor_opt
> endif
>
> [Actually, I think the very first attempt had:
>
> libgit_c_args += '-DDEFAULT_EDITOR=\'' + editor_opt + '\''
>
> but meson, for some reason, adds a set of ' around the whole
> -D argument to gcc, so I got rid of them - but it still didn't
> work!]
>
> Along with many, many, *many* such permutations! (trying to debug
> this is hard work, with no help from meson).
>
> So, just a little earlier this evening I read an email from Karthik
> ([PATCH v2 3/4] meson: add support for 'hdr-check') in which he
> mentioned a problem with backslashes and referenced a github issue
> on the mesonbuild repo [0], which is worth a read. ;)
>
> Sorry I couldn't fix this issue, but it seems to be (in part) an issue
> with meson. (Of course the example I used, which is taken directly
> from the Makefile, happens to be particularly good at demonstrating
> the problem!)
Fair enough. Maybe I'll try to upstream a feature like this into Meson.
It would be nice to have a `.quoted()` method on `str`, and it shouldn't
be hard to do.
> In any event, I think the current patch is a strict improvement, even
> if it may need to be updated at a later date. I hope you agree.
Agreed.
> Thank you for taking the time to review this series. I think this patch
> was the only review comment that required a response - please let me
> know, if that is not the case!
Yup. The only other thing was missing spaces around assignment
operators, but that alone doesn't feel like it's worth a reroll.
Thanks!
Patrick
