git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Heiko Voigt <hvoigt@hvoigt.net>
Cc: Stefan Beller <sbeller@google.com>, "git\@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [PATCH v2] add test for bug in git-mv for recursive submodules
Date: Sun, 17 Sep 2017 09:46:35 +0900
Message-ID: <xmqqlgleup78.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170915115021.GB76244@book.hvoigt.net>

Heiko Voigt <hvoigt@hvoigt.net> writes:

> When using git-mv with a submodule it will detect that and update the
> paths for its configurations (.gitmodules, worktree and gitfile). This
> does not work for recursive submodules where a user renames the root
> submodule.
>
> We discovered this fact when working on on-demand fetch for renamed
> submodules. Lets add a test to document.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---
> On Fri, Aug 18, 2017 at 12:04:03PM -0700, Stefan Beller wrote:
>> > I just copied the shortcut that they were adding themselfes as submodule
>> > in 'setup submodule'. The whole setup of submodules in this test is like
>> > this. This way we already had a nested submodule structure which I could
>> > just add.
>> >
>> > I agree that this is unrealistic so I can change that in the test I am
>> > adding. But from what I have seen, this shortcut is taken in quite some
>> > places when dealing with submodules.
>> 
>> Please do not make it worse.
>> Once upon a time (late '16 IIRC) I had a series floating on the
>> list removing all occurrences, but there were issues with the
>> series and it did not land.
>
> Took a little while but here is a more clean patch creating individual
> submodules for the nesting.
>
> Cheers Heiko

Thanks.  Stefan, does this look good to you now?

It is not quite clear which step is expected to fail with the
current code by reading the test or the proposed log message.  Does
"mv" refuse to work and we do not get to run "status", or does
"status" report a failure, or do we fail well before that?

The log message that only says "This does not work when ..." is not
helpful in figuring it out, either.  Something like "This does not
work and fails to update the paths for its configurations" or
whatever that describes "what actually happens" (in contrast to
"what ought to happen", which you described clearly) should be
there.  

Description on how you happened to have discovered the issue feels a
lot less relevant compared to that, and it is totally useless if it
is unclear what the issue is in the first place.

>  t/t7001-mv.sh | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> index e365d1ff77..cbc5fb37fe 100755
> --- a/t/t7001-mv.sh
> +++ b/t/t7001-mv.sh
> @@ -491,4 +491,29 @@ test_expect_success 'moving a submodule in nested directories' '
>  	test_cmp actual expect
>  '
>  
> +test_expect_failure 'moving nested submodules' '
> +	git commit -am "cleanup commit" &&
> +	mkdir sub_nested_nested &&
> +	(cd sub_nested_nested &&
> +		touch nested_level2 &&
> +		git init &&
> +		git add . &&
> +		git commit -m "nested level 2"
> +	) &&
> +	mkdir sub_nested &&
> +	(cd sub_nested &&
> +		touch nested_level1 &&
> +		git init &&
> +		git add . &&
> +		git commit -m "nested level 1"
> +		git submodule add ../sub_nested_nested &&
> +		git commit -m "add nested level 2"
> +	) &&
> +	git submodule add ./sub_nested nested_move &&
> +	git commit -m "add nested_move" &&
> +	git submodule update --init --recursive &&
> +	git mv nested_move sub_nested_moved &&
> +	git status
> +'
> +
>  test_done

  reply index

Thread overview: 8+ messages in thread (expand / mbox.gz / Atom feed / [top])
2017-08-17 10:34 [PATCH] add test for bug in git-mv with nested " Heiko Voigt
2017-08-17 19:05 ` Stefan Beller
2017-08-18 16:06   ` Heiko Voigt
2017-08-18 19:04     ` Stefan Beller
2017-09-15 11:50       ` [PATCH v2] add test for bug in git-mv for recursive " Heiko Voigt
2017-09-17  0:46         ` Junio C Hamano [this message]
2017-09-18 20:03           ` Stefan Beller
2017-09-20 13:46             ` Heiko Voigt

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply to all the recipients using the --to, --cc,
  and --in-reply-to switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqlgleup78.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=hvoigt@hvoigt.net \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox