On 6/22/2022 6:32 AM, Victoria Dye wrote:
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.
Sure! It makes the commit sound clearer.
[1] https://lore.kernel.org/git/22aadea2-9330-aa9e-7b6a-834585189144@xxxxxxxxxx/
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@xxxxxxxxx>
---
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?
Yep, this BOTH is confusing.
I think it could be changed to something like UNDECIDED?
If taking it as UNDECIDED, it could potentially help determine if an
argument
is untouched throughout the checking, then we can give the argument a
REGULAR
flag (this may serve a purpose in my planned in-cone to out-of-cone move).
+ 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.
Right. SKIP_WORKTREE_DIR should go to a different patch.
+};
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?
I'm not sure either. Though a enum groups the flags together, which
seems nice to me
as the flags are naturally distinguished from other '#define's.
+
#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.
+ 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
Sure. My 'format-patch' always gives me this issue, will check.
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));
