Re: [PATCH 5/5] submodule: submodule_move_head omits old argument in forced case

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 19, 2017, 22:44 UTC
Message-ID: <20171219224431.GG240141@aiede.mtv.corp.google.com>
In-Reply-To: <20171219222636.216001-6-sbeller@google.com>

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 ...

Show 7 quoted lines

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

Show 17 quoted lines

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  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