| Message ID | pull.1345.git.git.1664294909011.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | abspath.h is created. | expand |
"skrab-sah via GitGitGadget" <gitgitgadget@gmail.com> writes: > diff --git a/abspath.h b/abspath.h > new file mode 100644 > index 00000000000..edebc3a53ba > --- /dev/null > +++ b/abspath.h > @@ -0,0 +1,9 @@ > +/* This file was automatically generated. Do not edit! */ No thanks. I suspect this change is taking us in a wrong direction. People grep for function and struct declarations in <*.h> files and expect to find the associated comments.
What is wrong here and what should i do to make it correct. On Wed, 28 Sept 2022 at 06:11, Junio C Hamano <gitster@pobox.com> wrote: > > "skrab-sah via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > diff --git a/abspath.h b/abspath.h > > new file mode 100644 > > index 00000000000..edebc3a53ba > > --- /dev/null > > +++ b/abspath.h > > @@ -0,0 +1,9 @@ > > +/* This file was automatically generated. Do not edit! */ > > No thanks. > > I suspect this change is taking us in a wrong direction. People > grep for function and struct declarations in <*.h> files and expect > to find the associated comments.
Hi Skrab,
On Wed, Sep 28, 2022 at 01:02:22PM +0530, Skrab Sah wrote:
> What is wrong here and what should i do to make it correct.
I suspect that Junio's concern is that it appears the tool you're using
to auto-generate these .h files moves the comments away from the
declaration and into the header.
I imagine that such a change would be improved if:
- You kept the comment attached to the function declaration (and
ensured that it was present in the header file).
- And we dropped the "this file is auto-generated by ..." comment,
since I don't imagine that folks will be interested in adding a new
tool to the development toolchain.
Thanks.
Taylor
Skrab Sah <skrab.sah@gmail.com> writes:
> What is wrong here and what should i do to make it correct.
Almost everything in what the patch is wrong.
* It creates <abspath.h> as a tracked file, but has a funny comment
with control-G in it that says "do not edit"
* It is entirely unclear how the contents of <abspath.h> is meant
to evolve, if the contributors are forbidden to edit it, and
without any instruction how to update it.
* The new <abspath.h> lost all function comments, grouping by
category, etc. that were in the original <cache.h>.
* There is an "#undef INTERFACE" that has no use to us, those who
develop Git, for no clearly explained reason.
* The proposed log message says what it did, but it does not even
try to justify why it is a good idea to do so.
* There is no need to create a new <abspath.h> in the first place.
If we were in an alternate world where we did not have
<abspath.c>, it may be reasonable to add <abspath.h> along with
it, instead of adding declarations for new functions and types in
<cache.h>. But the difference falls into "once it is in, it is
not worth the patch churn to change it" category.
The easiest way to make it correct is to drop it, I guess.
Thanks.
diff --git a/abspath.c b/abspath.c index 39e06b58486..1c163bbe651 100644 --- a/abspath.c +++ b/abspath.c @@ -262,6 +262,16 @@ char *absolute_pathdup(const char *path) return strbuf_detach(&sb, NULL); } +/* + * Concatenate "prefix" (if len is non-zero) and "path", with no + * connecting characters (so "prefix" should end with a "/"). + * Unlike prefix_path, this should be used if the named file does + * not have to interact with index entry; i.e. name of a random file + * on the filesystem. + * + * The return value is always a newly allocated string (even if the + * prefix was empty). + */ char *prefix_filename(const char *pfx, const char *arg) { struct strbuf path = STRBUF_INIT; diff --git a/abspath.h b/abspath.h new file mode 100644 index 00000000000..edebc3a53ba --- /dev/null +++ b/abspath.h @@ -0,0 +1,9 @@ +/* This file was automatically generated. Do not edit! */ +#undef INTERFACE +char *prefix_filename(const char *pfx,const char *arg); +char *absolute_pathdup(const char *path); +const char *absolute_path(const char *path); +char *real_pathdup(const char *path,int die_on_error); +char *strbuf_realpath_forgiving(struct strbuf *resolved,const char *path,int die_on_error); +char *strbuf_realpath(struct strbuf *resolved,const char *path,int die_on_error); +int is_directory(const char *path); diff --git a/cache.h b/cache.h index 26ed03bd6de..e226dbcc7d5 100644 --- a/cache.h +++ b/cache.h @@ -646,18 +646,6 @@ const char *setup_git_directory(void); char *prefix_path(const char *prefix, int len, const char *path); char *prefix_path_gently(const char *prefix, int len, int *remaining, const char *path); -/* - * Concatenate "prefix" (if len is non-zero) and "path", with no - * connecting characters (so "prefix" should end with a "/"). - * Unlike prefix_path, this should be used if the named file does - * not have to interact with index entry; i.e. name of a random file - * on the filesystem. - * - * The return value is always a newly allocated string (even if the - * prefix was empty). - */ -char *prefix_filename(const char *prefix, const char *path); - int check_filename(const char *prefix, const char *name); void verify_filename(const char *prefix, const char *name, @@ -1299,14 +1287,7 @@ static inline int is_absolute_path(const char *path) { return is_dir_sep(path[0]) || has_dos_drive_prefix(path); } -int is_directory(const char *); -char *strbuf_realpath(struct strbuf *resolved, const char *path, - int die_on_error); -char *strbuf_realpath_forgiving(struct strbuf *resolved, const char *path, - int die_on_error); -char *real_pathdup(const char *path, int die_on_error); -const char *absolute_path(const char *path); -char *absolute_pathdup(const char *path); +#include "abspath.h" const char *remove_leading_path(const char *in, const char *prefix); const char *relative_path(const char *in, const char *prefix, struct strbuf *sb); int normalize_path_copy_len(char *dst, const char *src, int *prefix_len);