Re: [WIP v3 5/7] mv: use flags mode for update_mode
From: Victoria Dye <hidden>
Date: 2022-06-21 22:33:12
Shaoxuan Yuan wrote:
As suggested by Derrick [1], move the in-line definition of "enum update_mode" to the top of the file and make it use "flags" mode (each state is a different bit in the word).
This message doesn't quite cover all of what's done in the commit. In addition to moving the enum definition, you introduce a 'SKIP_WORKTREE_DIR' flag and change the flag assignments to '|=' (additive) from '=' (single assignment). If those changes belong in this commit (not a later one), they should be explained in the message here.
quoted hunk ↗ jump to hunk
[1] https://lore.kernel.org/git/22aadea2-9330-aa9e-7b6a-834585189144@github.com/ (local) Signed-off-by: Shaoxuan Yuan <redacted> --- builtin/mv.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)diff --git a/builtin/mv.c b/builtin/mv.c index abb90d3266..7ce7992d6c 100644 --- a/builtin/mv.c +++ b/builtin/mv.c@@ -19,6 +19,14 @@ static const char * const builtin_mv_usage[] = { NULL }; +enum update_mode { + BOTH = 0,
I know this comes from the original inline enum, but I don't see 'BOTH' used anywhere. The name itself is somewhat confusing (I have no idea what "both" is referring to - possibly "both" 'WORKING_DIRECTORY' and 'INDEX'??), so would you mind removing it in the next re-roll?
+ WORKING_DIRECTORY = (1 << 1), + INDEX = (1 << 2), + SPARSE = (1 << 3), + SKIP_WORKTREE_DIR = (1 << 4),
You're not introducing any assignment of 'SKIP_WORKTREE_DIR' in this commit (looks like that's done in the next one, patch [6/7]), so you should probably 'SKIP_WORKTREE_DIR' and its corresponding usage in that patch instead of this one.
+};
When the update modes were mutually-exclusive, it made sense for them to be represented by an enum. Now that they're flags that can be combined, should they instead be pre-processor '#define' values (e.g., like the 'RESET_*' modes in 'reset.h' or 'CE_*' flags in 'cache.h')? I don't actually know what the standard is, since I also see one or two examples of using enums as flags (e.g., 'commit_graph_split_flags' in 'commit-graph.h'). Maybe another contributor could clarify?
quoted hunk ↗ jump to hunk
+ #define DUP_BASENAME 1 #define KEEP_TRAILING_SLASH 2@@ -129,7 +137,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) OPT_END(), }; const char **source, **destination, **dest_path, **submodule_gitfile; - enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX, SPARSE } *modes; + enum update_mode *modes; struct stat st; struct string_list src_for_dst = STRING_LIST_INIT_NODUP; struct lock_file lock_file = LOCK_INIT;@@ -191,7 +199,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) pos = cache_name_pos(src, length); if (pos < 0) { /* only error if existence is expected. */ - if (modes[i] != SPARSE) + if (!(modes[i] & SPARSE)) bad = _("bad source"); goto act_on_entry; }@@ -207,14 +215,14 @@ int cmd_mv(int argc, const char **argv, const char *prefix) } /* Check if dst exists in index */ if (cache_name_pos(dst, strlen(dst)) < 0) { - modes[i] = SPARSE; + modes[i] |= SPARSE; goto act_on_entry; } if (!force) { bad = _("destination exists"); goto act_on_entry; } - modes[i] = SPARSE; + modes[i] |= SPARSE; goto act_on_entry; } if (!strncmp(src, dst, length) &&@@ -242,7 +250,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) } /* last - first >= 1 */ - modes[i] = WORKING_DIRECTORY; + modes[i] |= WORKING_DIRECTORY; n = argc + last - first; REALLOC_ARRAY(source, n); REALLOC_ARRAY(destination, n);@@ -258,7 +266,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix) source[argc + j] = path; destination[argc + j] = prefix_path(dst, dst_len, path + length + 1); - modes[argc + j] = ce_skip_worktree(ce) ? SPARSE : INDEX; + memset(modes + argc + j, 0, sizeof(enum update_mode));
One benefit of using '#define' values would be that 'modes' would just be an array of unsigned ints, so you could just assign '0' rather than using memset. In terms of the implementation as-is, though, I think what you have is correct.
quoted hunk ↗ jump to hunk
+ modes[argc + j] |= ce_skip_worktree(ce) ? SPARSE : INDEX; submodule_gitfile[argc + j] = NULL; } argc += last - first;@@ -355,7 +364,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix) printf(_("Renaming %s to %s\n"), src, dst); if (show_only) continue; - if (mode != INDEX && mode != SPARSE && rename(src, dst) < 0) { + if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) && + rename(src, dst) < 0) {
Nit: could you align 'rename' with the line above it (per the highlighted section in the CodingGuidelines [1])? As far as I can tell, the "align with tabs and spaces" approach is what's *intended* to be used in 'mv.c' (although it's admittedly pretty inconsistent). [1] https://github.com/git/git/blob/master/Documentation/CodingGuidelines#L371-L383
quoted hunk ↗ jump to hunk
if (ignore_errors) continue; die_errno(_("renaming '%s' failed"), src);@@ -369,7 +379,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) 1); } - if (mode == WORKING_DIRECTORY) + if (mode & (WORKING_DIRECTORY | SKIP_WORKTREE_DIR)) continue; pos = cache_name_pos(src, strlen(src));