On Mon, Mar 8, 2021 at 7:07 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxx>
> ---
> 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.
> 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.
>
> 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?)
> }
> 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.
