Re: [WIP v3 5/7] mv: use flags mode for update_mode

[Victoria Dye <vdye@github.com>]

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.

> [1] https://lore.kernel.org/git/22aadea2-9330-aa9e-7b6a-834585189144@github.com/
> 
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  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? 

> +
>  #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 

>  			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));