[BUG] assertion failure in builtin-mv.c with "git mv -k"

Hi,

Just found a bug in builtin-mv.c. Here's a script to reproduce:

mkdir git cd git git init touch controled git add controled && git commit -m "init" touch foo1 foo2 mkdir dir git mv -k foo* dir/

The output is the following:

Initialized empty Git repository in /tmp/git/.git/
[master (root-commit)]: created 694563b: "init"
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 controled
git: builtin-mv.c:216: cmd_mv: Assertion `pos >= 0' failed.
./bug.sh: line 10: 12919 Aborted                 git mv -k foo* dir/

Apparently, this happens when using "git mv -k" with more than one unversionned file. The code ignores the first one, but still goes through this

	for (i = 0; i < argc; i++) {
		const char *src = source[i], *dst = destination[i];
		enum update_mode mode = modes[i];
		int pos;
		if (show_only || verbose)
			printf("Renaming %s to %s\n", src, dst);
		if (!show_only && mode != INDEX &&
				rename(src, dst) < 0 && !ignore_errors)
			die ("renaming %s failed: %s", src, strerror(errno));

		if (mode == WORKING_DIRECTORY)
			continue;

		pos = cache_name_pos(src, strlen(src));
		assert(pos >= 0); /* <----- this is the one */
		if (!show_only)
			rename_cache_entry_at(pos, dst);
	}

for the second, and it crashes on the assertion (gdb says "pos" here is an unversionned file name).

If anyone has time to fix this ...

Thanks,

Matthieu Moy venit, vidit, dixit 14.01.2009 14:20:

The problem is actually in the loop before, with just one line missing. I'll send a patch but I'm not sure if this needs a test case.

Michael

Hi,

On Wed, 14 Jan 2009, Michael J Gruber wrote:

Umm, Michael, you have been here long enough to know that the answer is a "YES!". If you fix something, you want to provide a test case just to make sure you do not need to fix it again later.

Ciao, Dscho

Johannes Schindelin venit, vidit, dixit 14.01.2009 16:54:

It was a lame attempt at getting around it, it's just one line... I didn't know I've been being noticed long enough ;) So, should I prepare a series like:

1: test case and mark known fail 2: the 1 line fix 3: mark test pass

Or should 2+3 be squashed into one?

Cheers, Michael

On Wed, Jan 14, 2009 at 05:04:44PM +0100, Michael J Gruber wrote:

Definitely the fix and marking the test as passing should be one patch, since then you see that it is the fix which causes the test to pass. But really, for a simple fix I usually just squash it all into a single patch. Then somebody looking at the patch later says "Oh, this is what was broken, and here is how it was fixed."

-Peff

Michael J Gruber <git@drmicha.warpmail.net> writes:

If "git mv" already has its own sets of tests with a good coverage, please strive to add a case that covers your fix to an existing script. Then step #1 above would be a patch to add a few "test_expect_failure" tests to an existing file, and step #3 would be a patch to flip expect_failure to expect_success.

And in such a case, for a single liner, all three can be squashed in to a single patch. It would show what changed in the code and have a few new test_expect_success tests added to the test suite, and it would be obvious to anybody who looks at such a change 6 months down the road that the test cases added by the patch are the cases that did not work without the changes to the code. It never makes sense to separate steps #2 and #3 for any fix.

If "git mv" did not have adequate test coverage, then please add a test script with both expect_success (for cases that should have been there when somebody worked on "git mv" originally), and expect_failure to expose the bug you found in your first patch. Again, the second patch would update the code and flip expect_failure to expect_success.

I see there is t7001-mv and even though it claims to concentrate on its operation from subdirectory, it has tests for more basic modes of the operation.

So my recommendation would be to have a single patch that:

 (1) retitles t7001;
 (2) adds your new -k tests to it; and
 (3) adds the 1-liner fix.

Junio C Hamano venit, vidit, dixit 14.01.2009 20:02: ...

Sorry to bother you again, even after your detailed reply, but I'm a bit confused in multiple ways (I guess that makes for multiple bits...). First, you replied to my post, not my patch v2, but (time-wise) after my patch v2, so I'm not sure whether your reply applies to v2 as well or that one is OK.

Second, I took the title of t7001 to mean "mv into subdir" or "mv in and out subdir" in order to distiguish it from "mv oldname newname", albeit in English that leaves room from improvement.

Third, various parts of that test are in vastly different styles, and I haven't found any test writing directions other than "follow what is there", which leaves several alternatives. (Some use the test-lib repo, some create their own underneath, some make assumptions about the contents of "$TEST_DIRECTORY"/../.)

Fourth, t7001-mv is missing any test for "mv -k", and 2 of my 3 additional tests cover cases which work (pass) without the fix, which is why I kept it separate, being unrelated. Following all your arguments lead to the conclusion I should squash 2+3 (fix + mark expect_pass) - until I read your conclusion, which says squash all.

I'm happy to follow any variant ("1+2+3", "1 2+3", "1 2 3", in increasing order of preference) so there's no need to discuss or explain this further, just tell me "do x" ;)

Cheers, Michael

Michael J Gruber <git@drmicha.warpmail.net> writes:

Do nothing ;-) Your 1=3772923 and 2+3=be17262d are already in and we can include the fix in the next 1.6.1.X maintenance release.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Thanks!

This adds test cases for the "-k" option of "git mv", including one known breakage reported by Matthieu Moy <Matthieu.Moy@imag.fr> which appears when multiple untracked files are specified as consecutive arguments. This breakage is fixed in the second patch and marked "expect_pass" in the last one.

The cumulative code/other ratio is 1 line/27 lines which I blame solely on Dscho ;) Seriously, feel free to squash. But on the other hand: How else can I see the beautiful "1 known breakage fixed" other than by having 2 and 3 separate in this series...

The patch is off master but builtin-mv.c hasn't changed since 81dc2307d0ad87a4da2e753a9d1b5586d6456eed tags/v1.6.0-rc1~1, so I suggest this patch for maint.

Michael J Gruber (3):
  add test cases for "git mv -k"
  fix handling of multiple untracked files for git mv -k
  mark fixed breakage as expect pass for "git mv -k" multiple files

 builtin-mv.c  |    1 +
 t/t7001-mv.sh |   25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

Add test cases for ignoring nonexisting and untracked files using the -k option to "git mv". There is one known breakage related to multiple untracked files specfied as consecutive arguments.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7001-mv.sh |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

The "-k" option to "git mv" should allow specifying multiple untracked files. Currently, multiple untracked files raise an assertion if they appear consecutively as arguments. Fix this by decrementing the loop index after removing one entry from the array of arguments.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin-mv.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

The new tests pass now so mark them as such.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7001-mv.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

The "-k" option to "git mv" should allow specifying multiple untracked files. Currently, multiple untracked files raise an assertion if they appear consecutively as arguments. Fix this by decrementing the loop index after removing one entry from the array of arguments.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin-mv.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

Reported by the OP. Do we need a test case for this? It's a really trivial change. The patch is off master but builtin-mv.c hasn't change since 81dc2307d0ad87a4da2e753a9d1b5586d6456eed tags/v1.6.0-rc1~1, so I suggest this patch for maint.