| Message ID | 20230810163654.275023-2-calvinwan@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce Git Standard Library | expand |
Calvin Wan <calvinwan@google.com> writes: > While remove_or_warn() is a simple ternary operator to call two other > wrapper functions, it creates an unnecessary dependency to object.h in > wrapper.c. Therefore move the function to object.[ch] where the concept > of GITLINKs is first defined. An untold assumption here is that we would want to make wrapper.[ch] independent of Git's internals? If so, where the thing is moved to (i.e. object.c) is much less interesting than the fact that the goal of this function is to make wrapper.[ch] less dependent on Git, so the title should reflect that, no? > +/* > + * Calls the correct function out of {unlink,rmdir}_or_warn based on > + * the supplied file mode. > + */ > +int remove_or_warn(unsigned int mode, const char *path); OK. That "file mode" thing is not a regular "struct stat .st_mode", but knows Git's internals, hence it makes sense to have it on our side, not on the wrapper.[ch] side. That makes sense. > #endif /* OBJECT_H */ > diff --git a/wrapper.c b/wrapper.c > index 22be9812a7..118d3033de 100644 > --- a/wrapper.c > +++ b/wrapper.c > @@ -5,7 +5,6 @@ > #include "abspath.h" > #include "config.h" > #include "gettext.h" > -#include "object.h" > #include "repository.h" > #include "strbuf.h" > #include "trace2.h" > @@ -647,11 +646,6 @@ int rmdir_or_warn(const char *file) > return warn_if_unremovable("rmdir", file, rmdir(file)); > } > > -int remove_or_warn(unsigned int mode, const char *file) > -{ > - return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file); > -} > - > static int access_error_is_ok(int err, unsigned flag) > { > return (is_missing_file_error(err) || > diff --git a/wrapper.h b/wrapper.h > index c85b1328d1..272795f863 100644 > --- a/wrapper.h > +++ b/wrapper.h > @@ -111,11 +111,6 @@ int unlink_or_msg(const char *file, struct strbuf *err); > * not exist. > */ > int rmdir_or_warn(const char *path); > -/* > - * Calls the correct function out of {unlink,rmdir}_or_warn based on > - * the supplied file mode. > - */ > -int remove_or_warn(unsigned int mode, const char *path); > > /* > * Call access(2), but warn for any error except "missing file"
Calvin Wan <calvinwan@google.com> writes: > While remove_or_warn() is a simple ternary operator to call two other > wrapper functions, it creates an unnecessary dependency to object.h in > wrapper.c. Therefore move the function to object.[ch] where the concept > of GITLINKs is first defined. As Junio mentioned elsewhere, I think we need to establish that wrapper.c should be free of Git-specific internals. > diff --git a/object.c b/object.c > index 60f954194f..cb29fcc304 100644 > --- a/object.c > +++ b/object.c > @@ -617,3 +617,8 @@ void parsed_object_pool_clear(struct parsed_object_pool *o) > FREE_AND_NULL(o->object_state); > FREE_AND_NULL(o->shallow_stat); > } > + > +int remove_or_warn(unsigned int mode, const char *file) > +{ > + return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file); > +} Since this function really needs S_ISGITLINK (I tried to see if we could just replace it with S_ISDIR and get the same behavior, but we can't), this really is a Git-specific thing, so yes, this should be moved out of wrapper.c. Minor point: I think a better home might be entry.[ch], because those files care about performing changes on the worktree based on the Git-specific file modes in the index, whereas object.[ch] seems more concerned about the format of objects.
Glen Choo <chooglen@google.com> writes: > Minor point: I think a better home might be entry.[ch], because those > files care about performing changes on the worktree based on the > Git-specific file modes in the index, whereas object.[ch] seems more > concerned about the format of objects. Yeah, I wasn't paying much attention on that point while reading the patch, and I do agree with you that entry.[ch] may be a better fit. Thanks.
diff --git a/object.c b/object.c index 60f954194f..cb29fcc304 100644 --- a/object.c +++ b/object.c @@ -617,3 +617,8 @@ void parsed_object_pool_clear(struct parsed_object_pool *o) FREE_AND_NULL(o->object_state); FREE_AND_NULL(o->shallow_stat); } + +int remove_or_warn(unsigned int mode, const char *file) +{ + return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file); +} diff --git a/object.h b/object.h index 5871615fee..e908ef6515 100644 --- a/object.h +++ b/object.h @@ -284,4 +284,10 @@ void clear_object_flags(unsigned flags); */ void repo_clear_commit_marks(struct repository *r, unsigned int flags); +/* + * Calls the correct function out of {unlink,rmdir}_or_warn based on + * the supplied file mode. + */ +int remove_or_warn(unsigned int mode, const char *path); + #endif /* OBJECT_H */ diff --git a/wrapper.c b/wrapper.c index 22be9812a7..118d3033de 100644 --- a/wrapper.c +++ b/wrapper.c @@ -5,7 +5,6 @@ #include "abspath.h" #include "config.h" #include "gettext.h" -#include "object.h" #include "repository.h" #include "strbuf.h" #include "trace2.h" @@ -647,11 +646,6 @@ int rmdir_or_warn(const char *file) return warn_if_unremovable("rmdir", file, rmdir(file)); } -int remove_or_warn(unsigned int mode, const char *file) -{ - return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file); -} - static int access_error_is_ok(int err, unsigned flag) { return (is_missing_file_error(err) || diff --git a/wrapper.h b/wrapper.h index c85b1328d1..272795f863 100644 --- a/wrapper.h +++ b/wrapper.h @@ -111,11 +111,6 @@ int unlink_or_msg(const char *file, struct strbuf *err); * not exist. */ int rmdir_or_warn(const char *path); -/* - * Calls the correct function out of {unlink,rmdir}_or_warn based on - * the supplied file mode. - */ -int remove_or_warn(unsigned int mode, const char *path); /* * Call access(2), but warn for any error except "missing file"
While remove_or_warn() is a simple ternary operator to call two other wrapper functions, it creates an unnecessary dependency to object.h in wrapper.c. Therefore move the function to object.[ch] where the concept of GITLINKs is first defined. Signed-off-by: Calvin Wan <calvinwan@google.com> --- object.c | 5 +++++ object.h | 6 ++++++ wrapper.c | 6 ------ wrapper.h | 5 ----- 4 files changed, 11 insertions(+), 11 deletions(-)