| Message ID | c9e7cd78576527571fd70b953e340b5bdd196221.1696021277.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | afd2a1d5f1fc371fe1fda0ed07e0f2f27100fbab |
| Headers | show |
| Series | Preliminary patches before git-std-lib | expand |
Hi Jonathan On 29/09/2023 22:20, Jonathan Tan wrote: > From: Calvin Wan <calvinwan@google.com> > > remove_or_warn() is only used by entry.c and apply.c, but it is > currently declared and defined in wrapper.{h,c}, so it has a scope much > greater than it needs. This needlessly large scope also causes wrapper.c > to need to include object.h, when this file is largely unconcerned with > Git objects. > > Move remove_or_warn() to entry.{h,c}. The file apply.c still has access > to it, since it already includes entry.h for another reason. This looks good. On a related note wrapper.c includes repository.h but does use anything declared in that header. Best Wishes Phillip
phillip.wood123@gmail.com writes: > Hi Jonathan > > On 29/09/2023 22:20, Jonathan Tan wrote: >> From: Calvin Wan <calvinwan@google.com> >> remove_or_warn() is only used by entry.c and apply.c, but it is >> currently declared and defined in wrapper.{h,c}, so it has a scope much >> greater than it needs. This needlessly large scope also causes wrapper.c >> to need to include object.h, when this file is largely unconcerned with >> Git objects. >> Move remove_or_warn() to entry.{h,c}. The file apply.c still has >> access >> to it, since it already includes entry.h for another reason. > > This looks good. On a related note wrapper.c includes repository.h but > does use anything declared in that header. > > Best Wishes > > Phillip Thanks for a review. I just checked 'master', 'next', and 'seen' and in all '#include <repository.h>' can safely be dropped from there, it seems. It may be too trivial even for a microproject, but nevertheless a nice clean-up.
Junio C Hamano <gitster@pobox.com> writes: > phillip.wood123@gmail.com writes: > > > Hi Jonathan > > > > On 29/09/2023 22:20, Jonathan Tan wrote: > >> From: Calvin Wan <calvinwan@google.com> > >> remove_or_warn() is only used by entry.c and apply.c, but it is > >> currently declared and defined in wrapper.{h,c}, so it has a scope much > >> greater than it needs. This needlessly large scope also causes wrapper.c > >> to need to include object.h, when this file is largely unconcerned with > >> Git objects. > >> Move remove_or_warn() to entry.{h,c}. The file apply.c still has > >> access > >> to it, since it already includes entry.h for another reason. > > > > This looks good. On a related note wrapper.c includes repository.h but > > does use anything declared in that header. > > > > Best Wishes > > > > Phillip > > Thanks for a review. I just checked 'master', 'next', and 'seen' > and in all '#include <repository.h>' can safely be dropped from > there, it seems. It may be too trivial even for a microproject, > but nevertheless a nice clean-up. Ah, Calvin fixed this in one of the subsequent patches, but I'll put it into its own patch in my updated version.
diff --git a/entry.c b/entry.c index 43767f9043..076e97eb89 100644 --- a/entry.c +++ b/entry.c @@ -581,3 +581,8 @@ void unlink_entry(const struct cache_entry *ce, const char *super_prefix) return; schedule_dir_for_removal(ce->name, ce_namelen(ce)); } + +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/entry.h b/entry.h index 7329f918a9..ca3ed35bc0 100644 --- a/entry.h +++ b/entry.h @@ -62,4 +62,10 @@ int fstat_checkout_output(int fd, const struct checkout *state, struct stat *st) void update_ce_after_write(const struct checkout *state, struct cache_entry *ce, struct stat *st); +/* + * 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 /* ENTRY_H */ diff --git a/wrapper.c b/wrapper.c index 48065c4f53..453a20ed99 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" @@ -632,11 +631,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 79c7321bb3..1b2b047ea0 100644 --- a/wrapper.h +++ b/wrapper.h @@ -106,11 +106,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"