From: Duy Nguyen Date: Tue, 12 Aug 2014 14:57:17 GMT Subject: Re: [PATCH v2 6/8] mv: unindent one level for directory move code Message-ID: In-Reply-To: On Tue, Aug 12, 2014 at 1:47 AM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- >> builtin/mv.c | 47 +++++++++++++++++++++-------------------------- >> 1 file changed, 21 insertions(+), 26 deletions(-) >> >> diff --git a/builtin/mv.c b/builtin/mv.c >> index dcfcb11..988945c 100644 >> --- a/builtin/mv.c >> +++ b/builtin/mv.c >> @@ -171,42 +171,37 @@ int cmd_mv(int argc, const char **argv, const char *prefix) >> && lstat(dst, &st) == 0) >> bad = _("cannot move directory over file"); >> else if (src_is_dir) { >> + int first = cache_name_pos(src, length), last; >> if (first >= 0) >> prepare_move_submodule(src, first, >> submodule_gitfile + i); >> + else if (index_range_of_same_dir(src, length, >> + &first, &last) < 1) { > > The function returns (last - first), so (last - first) < 1 holds > inside this block, right? > >> modes[i] = WORKING_DIRECTORY; >> if (last - first < 1) >> bad = _("source directory is empty"); > > Then do you need this conditional, or it is always bad here? > > If it is always bad, then modes[i] do not need to be assigned to, > either, I think. > > Am I missing something? No you're right. >> + } else { /* last - first >= 1 */ >> + int j, dst_len, n; > >> + modes[i] = WORKING_DIRECTORY; >> + n = argc + last - first; >> ... > > Otherwise, perhaps squash this in? Sure. But I'm having second thought about this series. mv.c is centered around files on worktree, which makes it pretty hard if we want to make it more like rm.c where index and worktree entries are treated more equally and pathspec is applied. Doing that in mv.c would require a lot more reorganization that makes this series obsolete. But on the other hand, I'm not even sure if I have time to pursue that. So.. -- Duy