Re: [PATCH] Add environment variable support to Path

[Oswald Buddenhagen]

On Sat, Aug 05, 2023 at 07:21:02PM +0200, Michiel van den Heuvel wrote:

Do you agree that implementing `IncludeCmd`
would supersede my previous patch?

yeah, i'd say so. it's somewhat more costly at runtime, but that 
shouldn't matter, and it would be more flexible.



I'll try to cook something up.


cool!

you can probably steal some code from cred_from_cmd() in drv_imap.c.
it's probably not sensible to generalize the existing code, because a) 
we want to handle multiple lines here (and also accept empty output, i 
think), and b) the generalization would probably add more code than the 
de-duplication would save.


regards


___
isync-devel mailing list
isync-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/isync-devel

Re: [PATCH] Add environment variable support to Path

[Michiel van den Heuvel]

Hi Oswald,

Thanks for the feedback! I could improve the patch that I sent, but...

The problems you pointed out with implementing it in the parser so
backslash can be the escape can indeed be worked around, but then we'd
end up with the `*Cmd` problems. Fixing those would result in some pretty
ugly code. (Which is basically what you already said)

  > related discussion: https://sourceforge.net/p/isync/feature-requests/17/

Oops, forgot about this one. Do you agree that implementing `IncludeCmd`
would supersede my previous patch?

I'll try to cook something up.

> weird. i didn't run it for quite a while, but previously vg worked just 
> fine on isync.

Could be something on my end.

Thanks for your reply!  
Michiel___
isync-devel mailing list
isync-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/isync-devel

Re: [PATCH] Add environment variable support to Path

[Oswald Buddenhagen]

On Fri, Aug 04, 2023 at 09:44:40PM +0200, Michiel van den Heuvel wrote:

This patch adds support for environment variable expansion to the
configuration options that encode a path. This would get rid of some
templating for my own configuration, and might prove useful for others
with integrating with notmuch, or having the same configuration file
for multiple very different systems (my usecase).


related discussion: https://sourceforge.net/p/isync/feature-requests/17/


 - Bash - or perhaps more correct - Makefile syntax is used. To keep
   the implementation simple, a double dollar sign is used to represent
   a literal, instead of a backslash.

i can see why you would do that, but it's getting a bit messy, as 
technically you'd need to use (only) $() for make-like variable 
expansion. also, the tilde expansion is a bash thing. one might get 
confused ...


considering the discussion on the tracker, it's worth considering doing 
the expansion at the parser level already. that would also enable 
backslash-escaping.
but then, there would be a problem with *Cmd due to suddenly requiring 
escaping of $. make-like layering of the expansions on top actually 
doesn't seem too bad, though it's imperfect, as $() is valid bash/posix 
syntax as well. hmm ...



 - I'd normally use valgrind to check for off-by-one and out of bounds
   errors, but it didn't work with mbsync on my system.

weird. i didn't run it for quite a while, but previously vg worked just 
fine on isync.


two comments on the patch from a cursory read:


+   val = getenv( nfstrndup( s, e - s ) );


this is leaking.


+   r += nfsnprintf( r, sizeof(path) - (r - path), "%s", 
val );

this may fail due to an unexpectedly large variable content, which is 
sort of user-controlled. that makes oob() kinda inappropriate (which is 
supposed to catch internal errors).


regards


___
isync-devel mailing list
isync-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/isync-devel