Re: [PATCH 1/7] path.c: implement xdg_config_home()
I need to learn to read the whole sentence. :( Apologies. On Tue, Apr 14, 2015 at 3:30 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> On Tue, Apr 14, 2015 at 1:39 PM, Junio C Hamano wrote: >> >>> It is OK to omit the name in the extern declaration here. >> >> It is OK, but I think this is bad practice. > > Take a special note on the word "here", meaning "in this particular > case." It is perfectly fine when the meaning of the parameter is > clear from its type. > > I was assuming that our developers have common sense to disambiguate > ambiguous cases, of course ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] path.c: implement xdg_config_home()
Stefan Beller writes: > On Tue, Apr 14, 2015 at 1:39 PM, Junio C Hamano wrote: > >> It is OK to omit the name in the extern declaration here. > > It is OK, but I think this is bad practice. Take a special note on the word "here", meaning "in this particular case." It is perfectly fine when the meaning of the parameter is clear from its type. I was assuming that our developers have common sense to disambiguate ambiguous cases, of course ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] path.c: implement xdg_config_home()
On Tue, Apr 14, 2015 at 1:39 PM, Junio C Hamano wrote: > Johannes Schindelin writes: > >>> diff --git a/cache.h b/cache.h >>> index 3d3244b..7f9bab0 100644 >>> --- a/cache.h >>> +++ b/cache.h >>> @@ -836,6 +836,13 @@ char *strip_path_suffix(const char *path, const >>> char *suffix); >>> int daemon_avoid_alias(const char *path); >>> extern int is_ntfs_dotgit(const char *name); >>> >>> +/** >>> + * Returns the newly allocated string "$XDG_CONFIG_HOME/git/%s". If >>> + * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string >>> + * "$HOME/.config/git/%s". Returns NULL if an error occurred. >>> + */ >>> +extern char *xdg_config_home(const char *fn); >> >> Should this not be inserted close to home_config_paths()? Also, the >> name "fn" sounds more like "function" than like "filename" to me, >> especially keeping the name `config_fn_t` in mind. Maybe call the >> parameter "filename" to avoid confusion? > > It is OK to omit the name in the extern declaration here. We have > to have a sensible variable name in the definition in path.c, of > course ;-), and "filename" sounds like a very sensible suggestion. > It is OK, but I think this is bad practice. Have a look at strbuf.h, there you'll find: extern int strbuf_getwholeline_fd(struct strbuf *, int, int); It's not clear what the 2 ints are, probably a fd and a max size? Even if guessed correctly, you'd still need another guess for the order. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] path.c: implement xdg_config_home()
Johannes Schindelin writes: >> diff --git a/cache.h b/cache.h >> index 3d3244b..7f9bab0 100644 >> --- a/cache.h >> +++ b/cache.h >> @@ -836,6 +836,13 @@ char *strip_path_suffix(const char *path, const >> char *suffix); >> int daemon_avoid_alias(const char *path); >> extern int is_ntfs_dotgit(const char *name); >> >> +/** >> + * Returns the newly allocated string "$XDG_CONFIG_HOME/git/%s". If >> + * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string >> + * "$HOME/.config/git/%s". Returns NULL if an error occurred. >> + */ >> +extern char *xdg_config_home(const char *fn); > > Should this not be inserted close to home_config_paths()? Also, the > name "fn" sounds more like "function" than like "filename" to me, > especially keeping the name `config_fn_t` in mind. Maybe call the > parameter "filename" to avoid confusion? It is OK to omit the name in the extern declaration here. We have to have a sensible variable name in the definition in path.c, of course ;-), and "filename" sounds like a very sensible suggestion. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] path.c: implement xdg_config_home()
On Mon, Apr 13, 2015 at 2:43 PM, Matthieu Moy wrote: > Paul Tan writes: > >> As such, implement a simpler function xdg_config_home() for constructing >> the XDG base dir spec configuration file path. This function, together >> with expand_user_path(), can replace all uses of home_config_paths(). > > Indeed. The code looks much better after your patch series than before. > > I agree with Dscho's remark that "fn" sounds like "function" more than > "filename". Perhaps just "name" would be better. I'd go with fname but that's just bikeshedding now. > > Anyway, the series is > > Reviewed-by: Matthieu Moy and Reviewed-by: Stefan Beller > > -- > Matthieu Moy > http://www-verimag.imag.fr/~moy/ > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] path.c: implement xdg_config_home()
Paul Tan writes: > As such, implement a simpler function xdg_config_home() for constructing > the XDG base dir spec configuration file path. This function, together > with expand_user_path(), can replace all uses of home_config_paths(). Indeed. The code looks much better after your patch series than before. I agree with Dscho's remark that "fn" sounds like "function" more than "filename". Perhaps just "name" would be better. Anyway, the series is Reviewed-by: Matthieu Moy -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] path.c: implement xdg_config_home()
Hi Paul, maybe it would be a good idea to add a `0/7` mail that describes the overall goal of this patch series, much like a Pull Request? I found it very useful -- even for myself -- to set a description via `git branch --edit-description` and to let `git format-patch` use that via the `--cover-letter` option. below just two minor nits because the rest of the patches looks fine to me: On 2015-04-12 09:46, Paul Tan wrote: > diff --git a/cache.h b/cache.h > index 3d3244b..7f9bab0 100644 > --- a/cache.h > +++ b/cache.h > @@ -836,6 +836,13 @@ char *strip_path_suffix(const char *path, const > char *suffix); > int daemon_avoid_alias(const char *path); > extern int is_ntfs_dotgit(const char *name); > > +/** > + * Returns the newly allocated string "$XDG_CONFIG_HOME/git/%s". If > + * $XDG_CONFIG_HOME is unset or empty, returns the newly allocated string > + * "$HOME/.config/git/%s". Returns NULL if an error occurred. > + */ > +extern char *xdg_config_home(const char *fn); Should this not be inserted close to home_config_paths()? Also, the name "fn" sounds more like "function" than like "filename" to me, especially keeping the name `config_fn_t` in mind. Maybe call the parameter "filename" to avoid confusion? Thanks, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
