Re: [PATCH 29/30] tree.h API users: rename read_tree_fn_t's "mode" to "raw_mode"
- From
Elijah Newren <newren@gmail.com>
- Date
- Mar 9, 2021, 19:02 UTC
- Message-ID
- <CABPp-BGGGrYs4HSmT4kq__90K8OxDKkF_Nb8nLAhouLONYxsEA@mail.gmail.com>
- In-Reply-To
- <20210308150650.18626-30-avarab@gmail.com>
On Mon, Mar 8, 2021 at 7:07 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 52 quoted lines
>
> Rename the "mode" variable passed to read_tree_fn_t callbacks to
> "raw_mode". This variable comes to us from the tree-walk.h API. By
> renaming this variable we can easily see where its downstream users
> are in a subsequent commit where we'll sprinkle some canon_mode()
> here.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> archive.c | 5 +++--
> builtin/log.c | 2 +-
> builtin/ls-files.c | 11 ++++++-----
> builtin/ls-tree.c | 6 +++---
> merge-recursive.c | 2 +-
> 5 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/archive.c b/archive.c
> index bc8f1c7546f..5b85aae8106 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -232,10 +232,11 @@ static int write_directory(struct archiver_context *c)
>
> static int queue_or_write_archive_entry(const struct object_id *oid,
> struct strbuf *base, const char *filename,
> - enum object_type object_type, unsigned mode,
> + enum object_type object_type, unsigned raw_mode,
> void *context)
> {
> struct archiver_context *c = context;
> + unsigned mode = raw_mode;
>
> while (c->bottom &&
> !(base->len >= c->bottom->len &&
> @@ -382,7 +383,7 @@ struct path_exists_context {
>
> static int reject_entry(const struct object_id *oid, struct strbuf *base,
> const char *filename,
> - enum object_type object_type, unsigned mode,
> + enum object_type object_type, unsigned raw_mode,
> void *context)
> {
> int ret = -1;
> diff --git a/builtin/log.c b/builtin/log.c
> index 19a916221d5..c3ef1b3e22d 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -599,7 +599,7 @@ static int show_tag_object(const struct object_id *oid, struct rev_info *rev)
>
> static int show_tree_object(const struct object_id *oid,
> struct strbuf *base, const char *pathname,
> - enum object_type object_type, unsigned mode,
> + enum object_type object_type, unsigned raw_mode,This was surprising to me and had me worried, but it looks like this function doesn't even use mode or raw_mode.
Show 17 quoted lines
> void *context)
> {
> FILE *file = context;
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index f38df439410..391e6a9f141 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -425,10 +425,11 @@ static int read_one_entry_opt(struct index_state *istate,
> const struct object_id *oid,
> struct strbuf *base,
> const char *pathname,
> - unsigned mode, int opt)
> + unsigned raw_mode, int opt)
> {
> int len;
> struct cache_entry *ce;
> + unsigned mode = raw_mode;I was about to comment, but checked out the code in question and it looks like you've modified further. So perhaps I should wait until the last patch in the series.
Show 58 quoted lines
>
> if (S_ISDIR(mode))
> return READ_TREE_RECURSIVE;
> @@ -447,12 +448,12 @@ static int read_one_entry_opt(struct index_state *istate,
>
> static int read_one_entry(const struct object_id *oid, struct strbuf *base,
> const char *pathname,
> - enum object_type object_type, unsigned mode,
> + enum object_type object_type, unsigned raw_mode,
> void *context)
> {
> struct index_state *istate = context;
> return read_one_entry_opt(istate, oid, base, pathname,
> - mode,
> + raw_mode,
> ADD_CACHE_OK_TO_ADD|ADD_CACHE_SKIP_DFCHECK);
> }
>
> @@ -462,12 +463,12 @@ static int read_one_entry(const struct object_id *oid, struct strbuf *base,
> */
> static int read_one_entry_quick(const struct object_id *oid, struct strbuf *base,
> const char *pathname,
> - enum object_type object_type, unsigned mode,
> + enum object_type object_type, unsigned raw_mode,
> void *context)
> {
> struct index_state *istate = context;
> return read_one_entry_opt(istate, oid, base, pathname,
> - mode,
> + raw_mode,
> ADD_CACHE_JUST_APPEND);
> }
>
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index c6ec3ca751e..3f84603d391 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -63,7 +63,7 @@ static int show_recursive(const char *base, int baselen, const char *pathname)
>
> static int show_tree(const struct object_id *oid, struct strbuf *base,
> const char *pathname,
> - enum object_type object_type, unsigned mode,
> + enum object_type object_type, unsigned raw_mode,
> void *context)
> {
> int retval = 0;
> @@ -103,11 +103,11 @@ static int show_tree(const struct object_id *oid, struct strbuf *base,
> "%"PRIuMAX, (uintmax_t)size);
> } else
> xsnprintf(size_text, sizeof(size_text), "-");
> - printf("%06o %s %s %7s\t", mode, type,
> + printf("%06o %s %s %7s\t", raw_mode, type,
> find_unique_abbrev(oid, abbrev),
> size_text);
> } else
> - printf("%06o %s %s\t", mode, type,
> + printf("%06o %s %s\t", raw_mode, type,
> find_unique_abbrev(oid, abbrev));This looks like a behavioral change. It might be desirable, but shouldn't it be called out and documented with a testcase?
(Or is there no change yet because the raw_mode isn't actually yet raw?)
Show 15 quoted lines
> }
> baselen = base->len;
> diff --git a/merge-recursive.c b/merge-recursive.c
> index b26d9d418f9..30fbe72ca06 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -453,7 +453,7 @@ static void unpack_trees_finish(struct merge_options *opt)
>
> static int save_files_dirs(const struct object_id *oid,
> struct strbuf *base, const char *path,
> - enum object_type object_type, unsigned int mode,
> + enum object_type object_type, unsigned int raw_mode,
> void *context)
> {
> struct path_hashmap_entry *entry;Oh, man, merge-recursive.c gets modes from the index, from the diff machinery, and from its own direct calls (get_tree_entry*() calls). And might have comparisons in all kinds of places. I'd be _very_ leery, much more so than with merge-ort.c, of having it deal with raw modes.