Menu
▾
▴
isync-devel
|
From: Andreas G. <an...@gr...> - 2020-11-29 07:39:57
|
Hello, I know there’s been interest in adding XDG_CONFIG_HOME support for the config file locations of mbsync from a previous thread on this list: https://sourceforge.net/p/isync/mailman/message/37004353/ <https://sourceforge.net/p/isync/mailman/message/37004353/> and from this reasonably well-received feature request: https://sourceforge.net/p/isync/feature-requests/14/ <https://sourceforge.net/p/isync/feature-requests/14/> Attached to this message is a patch implementing one half of the requested feature. The patch changes the way that mbsync looks for a config file in the following way: 1.) it expands XDG_CONFIG_HOME, if set, and otherwise defaults to $HOME/.config as per the specification 2.) it attempts to open the config file at location $XDG_CONFIG_HOME/mbsync/config 3.) if that failed, it will fall back to $HOME/.mbsyncrc I should note this approach still falls short of the specification in that the directories in $XDG_CONFIG_DIRS are not considered. A documentation change is included as well. Let me know if you want any changes to this patch, I’ll happily go through a couple of iterations. I think it’s important to get this right :) If this change is welcome, I can look into using XDG_DATA_HOME for ~/.local/share/mbsync as well, as requested in the original feature request. Thanks, Andreas |
|
From: Oswald B. <osw...@gm...> - 2020-11-29 13:47:38
|
On Sun, Nov 29, 2020 at 08:23:45AM +0100, Andreas Grapentin wrote:
>The patch changes the way that mbsync looks for a config file in the
>following way:
>
>1.) it expands XDG_CONFIG_HOME, if set, and otherwise defaults to $HOME/.config as per the specification
>2.) it attempts to open the config file at location
>$XDG_CONFIG_HOME/mbsync/config
>
any particular reason why this isn't simply $XDG_CONFIG_HOME/mbsyncrc? i
see that a lot of apps have the extra dir, but that's just pointless
noise when only one file is expected (of course, in principle, there can
be also ssl certificates, but storing auxiliary files with the same file
name stem directly in $XDG_CONFIG_HOME also has many precedents, and it
would be the rare case here).
>3.) if that failed, it will fall back to $HOME/.mbsyncrc
>I should note this approach still falls short of the specification in
>that the directories in $XDG_CONFIG_DIRS are not considered.
>
that appears acceptable.
>If this change is welcome, I can look into using XDG_DATA_HOME for
>~/.local/share/mbsync as well, as requested in the original feature
>request.
>
yes, absolutely.
>--- a/src/config.c
>+++ b/src/config.c
>@@ -329,21 +329,45 @@ load_config( const char *where, int pseudo )
> int cops, gcops, fn, i;
> char path[_POSIX_PATH_MAX];
> char buf[1024];
>+ const char *config_home;
>
that can be local to the assignment - we rely on C99 now.
> if (!where) {
> assert( !pseudo );
>
whoops, i should push out some pending patches, as there are some
conflicts here ...
>+ config_home = getenv("XDG_CONFIG_HOME");
>+ if (config_home) {
>
excess braces.
>+ nfsnprintf( path, sizeof(path), "%s/" EXE "/config", config_home );
>+ } else {
>+ nfsnprintf( path, sizeof(path), "%s/.config/" EXE "/config", Home );
>+ }
> cfile.file = path;
>- } else
>- cfile.file = where;
>
>- if (!pseudo)
> info( "Reading configuration file %s\n", cfile.file );
>+ cfile.fp = fopen( cfile.file, "r" );
>
>- if (!(cfile.fp = fopen( cfile.file, "r" ))) {
>- sys_error( "Cannot open config file '%s'", cfile.file );
>- return 1;
>+ if (!cfile.fp) {
>
that should also check for errno == ENOFILE.
>+ nfsnprintf( path, sizeof(path), "%s/." EXE "rc", Home );
>+ cfile.file = path;
>+
>+ info( "Reading configuration file %s\n", cfile.file );
>+ cfile.fp = fopen( cfile.file, "r" );
>+ }
>+
>+ if (!cfile.fp) {
>+ sys_error( "Cannot open user config file" );
>+ return 1;
>+ }
>+ } else {
>+ cfile.file = where;
>+
>+ if (!pseudo)
>+ info( "Reading configuration file %s\n", cfile.file );
>+
>+ if (!(cfile.fp = fopen( cfile.file, "r" ))) {
>+ sys_error( "Cannot open config file '%s'", cfile.file );
>+ return 1;
>+ }
>
i don't like the repetitiveness of this code. it should be easier to fix
that after you rebase.
> }
>--- a/src/mbsync.1
>+++ b/src/mbsync.1
>@@ -47,7 +47,8 @@ Multiple replicas of each mailbox can be maintained.
> .TP
> \fB-c\fR, \fB--config\fR \fIfile\fR
> Read configuration from \fIfile\fR.
>-By default, the configuration is read from ~/.mbsyncrc.
>+By default, the configuration is read from $XDG_CONFIG_HOME/mbsync/config,
>+or alternatively ~/.mbsyncrc.
>
this sounds like the precedence would be undefined. "with a fallback to"
or something like that would do.
|
|
From: Andreas G. <an...@gr...> - 2020-11-29 16:38:07
|
On Sun, Nov 29, 2020 at 02:47:15PM +0100, Oswald Buddenhagen wrote: > On Sun, Nov 29, 2020 at 08:23:45AM +0100, Andreas Grapentin wrote: > > 2.) it attempts to open the config file at location > > $XDG_CONFIG_HOME/mbsync/config > > > any particular reason why this isn't simply $XDG_CONFIG_HOME/mbsyncrc? i see > that a lot of apps have the extra dir, but that's just pointless noise when > only one file is expected (of course, in principle, there can be also ssl > certificates, but storing auxiliary files with the same file name stem > directly in $XDG_CONFIG_HOME also has many precedents, and it would be the > rare case here). I have not found any text in the specification on whether any format is to be preferred here. I think the subdirectory has the advantage of being extendable in the future, in case of a change that adds more configuration files I think all of the following would be fine: - XDG_CONFIG_HOME/mbsync/mbsyncrc - XDG_CONFIG_HOME/mbsync/config - XDG_CONFIG_HOME/isync/mbsyncrc - XDG_CONFIG_HOME/mbsyncrc and probably more. maybe the version XDG_CONFIG_HOME/isync/mbsyncrc makes sense, in case any other utilities that are part of isync will provide config files in the future? > > If this change is welcome, I can look into using XDG_DATA_HOME for > > ~/.local/share/mbsync as well, as requested in the original feature > > request. > > > yes, absolutely. In the interest of granularity, I'll prepare a separate patch for that, is that ok? Thank you for the feedback on the code changes! I have attached an updated version of the patch below. Best, Andreas -- ------------------------------------------------------------------------------ my GPG Public Key: https://files.grapentin.org/.gpg/public.key ------------------------------------------------------------------------------ |
|
From: Oswald B. <osw...@gm...> - 2020-11-29 18:31:02
|
On Sun, Nov 29, 2020 at 05:37:43PM +0100, Andreas Grapentin wrote:
>maybe the version XDG_CONFIG_HOME/isync/mbsyncrc makes sense, in case
>any other utilities that are part of isync will provide config files in
>the future?
>
i grok the argument, but i just don't think that this is going to happen
given the track record of the last 25 years. in the worst case, we'd
just support another fallback location. so XDG_CONFIG_HOME/mbsyncrc it
is.
>In the interest of granularity, I'll prepare a separate patch for that,
>is that ok?
>
of course. it might make sense to squash in the end due to overlap, but
for now let's work with separate patches.
speaking of possible overlap, you need to add a NEWS entry.
>--- a/src/config.c
>+++ b/src/config.c
>@@ -331,17 +332,36 @@ load_config( const char *where )
> char buf[1024];
>
> if (!where) {
>+ const char *config_home = getenv("XDG_CONFIG_HOME");
>+ if (config_home)
>+ nfsnprintf( path, sizeof(path), "%s/" EXE "/config", config_home );
>+ else
>+ nfsnprintf( path, sizeof(path), "%s/.config/" EXE "/config", Home );
> cfile.file = path;
>+
>+ info( "Reading configuration file %s\n", cfile.file );
>
let's move that below the final error handler - the error message is
self-contained, and the operation itself takes microseconds.
(listing the attempted locations would theoretically make sense as debug
output, but i don't think this would actually serve a purpose in this
context.)
>+ cfile.fp = fopen( cfile.file, "r" );
>+
>+ if (!cfile.fp && errno == ENOENT) {
>+ errno = 0;
>
that's unnecessary - the next error would overwrite it anyway, and no
sane code would rely on this being zero without resetting it immediately
before.
>+ nfsnprintf( path, sizeof(path), "%s/." EXE "rc", Home );
>+ cfile.file = path;
>+
that's also unnecessary, as the address didn't change.
>+ info( "Reading configuration file %s\n", cfile.file );
s.a.
>+ cfile.fp = fopen( cfile.file, "r" );
>+ }
>+ } else {
> cfile.file = where;
>
>+ info( "Reading configuration file %s\n", cfile.file );
s.a.
>+ cfile.fp = fopen( cfile.file, "r" );
>+ }
>
>+ if (!cfile.fp) {
>+ sys_error( "Cannot open user config file '%s'", cfile.file );
> return 1;
> }
|
|
From: Andreas G. <an...@gr...> - 2020-11-30 18:31:38
|
On Sun, Nov 29, 2020 at 07:30:44PM +0100, Oswald Buddenhagen wrote:
> On Sun, Nov 29, 2020 at 05:37:43PM +0100, Andreas Grapentin wrote:
> > maybe the version XDG_CONFIG_HOME/isync/mbsyncrc makes sense, in case
> > any other utilities that are part of isync will provide config files in
> > the future?
> >
> i grok the argument, but i just don't think that this is going to happen
> given the track record of the last 25 years. in the worst case, we'd just
> support another fallback location. so XDG_CONFIG_HOME/mbsyncrc it is.
Ok, XDG_CONFIG_HOME/mbsyncrc it is.
> speaking of possible overlap, you need to add a NEWS entry.
Added.
>
> > + cfile.fp = fopen( cfile.file, "r" );
> > +
> > + if (!cfile.fp && errno == ENOENT) {
>
> > + errno = 0;
> >
> that's unnecessary - the next error would overwrite it anyway, and no sane
> code would rely on this being zero without resetting it immediately before.
I don't like letting the state of errno leak to later code, if the
reported error has been handled. this can lead to unexpected output in
error reporting code in unrelated issues that don't overwrite errno,
such as a later call to sys_error.
>
> > + nfsnprintf( path, sizeof(path), "%s/." EXE "rc", Home );
>
> > + cfile.file = path;
> > +
> that's also unnecessary, as the address didn't change.
Could you please clarify -- do you mean the assignment of path to
cfile.file is unnecssary in general, because I could just use path as
parameter to fopen? or do you believe that this particular assignment is
unnecessary, because path did not change since the last assignment of
cfile.file?
Updated patch is attached again.
Thanks!
-A
--
------------------------------------------------------------------------------
my GPG Public Key: https://files.grapentin.org/.gpg/public.key
------------------------------------------------------------------------------
|
|
From: Andreas G. <an...@gr...> - 2020-11-30 18:37:21
|
On Mon, Nov 30, 2020 at 07:31:20PM +0100, Andreas Grapentin wrote: > > > > > + nfsnprintf( path, sizeof(path), "%s/." EXE "rc", Home ); > > > > > + cfile.file = path; > > > + > > that's also unnecessary, as the address didn't change. > > Could you please clarify -- do you mean the assignment of path to > cfile.file is unnecssary in general, because I could just use path as > parameter to fopen? or do you believe that this particular assignment is > unnecessary, because path did not change since the last assignment of > cfile.file? Please ignore this comment. Of course the assignment is unnecessary. Updated Patch is attached. -A -- ------------------------------------------------------------------------------ my GPG Public Key: https://files.grapentin.org/.gpg/public.key ------------------------------------------------------------------------------ |
|
From: Oswald B. <osw...@gm...> - 2020-11-30 18:56:44
|
On Mon, Nov 30, 2020 at 07:31:20PM +0100, Andreas Grapentin wrote: >On Sun, Nov 29, 2020 at 07:30:44PM +0100, Oswald Buddenhagen wrote: >> On Sun, Nov 29, 2020 at 05:37:43PM +0100, Andreas Grapentin wrote: >> > + errno = 0; >> > >> that's unnecessary - the next error would overwrite it anyway, and no sane >> code would rely on this being zero without resetting it immediately before. > >I don't like letting the state of errno leak to later code, if the >reported error has been handled. this can lead to unexpected output in >error reporting code in unrelated issues that don't overwrite errno, >such as a later call to sys_error. > the thing is that this is a fool's errand. errno(3) on linux is rather explicit about what you should expect. |
|
From: Andreas G. <an...@gr...> - 2020-11-30 19:44:30
|
On Mon, Nov 30, 2020 at 07:56:36PM +0100, Oswald Buddenhagen wrote: > On Mon, Nov 30, 2020 at 07:31:20PM +0100, Andreas Grapentin wrote: > > On Sun, Nov 29, 2020 at 07:30:44PM +0100, Oswald Buddenhagen wrote: > > > On Sun, Nov 29, 2020 at 05:37:43PM +0100, Andreas Grapentin wrote: > > > > + errno = 0; > > > > that's unnecessary - the next error would overwrite it anyway, and > > > no sane > > > code would rely on this being zero without resetting it immediately before. > > > > I don't like letting the state of errno leak to later code, if the > > reported error has been handled. this can lead to unexpected output in > > error reporting code in unrelated issues that don't overwrite errno, > > such as a later call to sys_error. > > > the thing is that this is a fool's errand. errno(3) on linux is rather > explicit about what you should expect. I think there's some minor benefit to this assignment, but if you want it gone, it can go. Updated patch is attached. -A -- ------------------------------------------------------------------------------ my GPG Public Key: https://files.grapentin.org/.gpg/public.key ------------------------------------------------------------------------------ |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X