Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case
From: Jonathan Nieder <hidden>
Date: 2017-12-19 22:44:40
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).
quoted hunk ↗ jump to hunk
Signed-off-by: Stefan Beller <redacted> --- 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