Hi, I had trouble understanding what this fixes, so I'll try nitpicking a bit as a sideways way to address that. Stefan Beller wrote: > With the previous patch applied (fix of the same() function), This tripped me up a bit. Usually commits assume that all previous patches have already been applied, since that allows the maintainer to apply the early part of a series on one day and the later part another day without losing too much context. I think this intends to say something like Now that we allow recursing into an unchanged submodule (see "unpack-trees: Fix same() for submodules", 2017-12-19), it is possible for ... > the > function `submodule_move_head` may be invoked with the same argument > for the `old` and `new` state of a submodule, for example when you > run `reset --hard --recurse-submodules` in the superproject that has no > change in the gitlink entry, but only worktree related change in the > submodule. The read-tree call in the submodule is not amused about > the duplicate argument. What is the symptom of read-tree being unamused? Is that a sign that these patches are out of order (i.e. that we should prepare to handle an unchanged submodule first, and then adjust the caller to pass in unchanged submodules)? > It turns out that we can omit the duplicate old argument in all forced > cases anyway, so let's do that. What is the end-user visibile effect of such a change? E.g. what becomes possible to a user that wasn't possible before? Among the commands you mentioned before: checkout -f I think I would expect this not to touch a submodule that hasn't changed, since that would be consistent with its behavior on files that haven't changed. reset --hard Nice! Yes, recursing into unchanged submodules is a big part of the psychological comfort of being able to say "you can always use reset --hard <commit> to get back to a known state". read-tree -u --reset This is essentially the plumbing equivalent of reset --hard, so it makes sense for them to be consistent. Ok. Based on the checkout -f case, if I've understood correctly then patch 4/5 goes too far on it (but I could easily be convinced otherwise). > Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx> > --- > submodule.c | 4 +++- > t/lib-submodule-update.sh | 2 +- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/submodule.c b/submodule.c > index fa25888783..db0f7ac51e 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -1653,7 +1653,9 @@ int submodule_move_head(const char *path, > else > argv_array_push(&cp.args, "-m"); > > - argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); > + if (!(flags & SUBMODULE_MOVE_HEAD_FORCE)) > + argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); Interesting. What is the effect when old != new? Thanks, Jonathan
