[BUG] mv: can trigger assertion failure with three parameters (builtin/mv.c:481)

You can trigger an assertion by giving these arguments to `git mv`:

    <dir>/file <dir> <other dir>

``` git config --global --add safe.directory /tmp dir=$(mktemp -d) cd $dir git init mkdir a touch a/a.txt git add a/a.txt git commit -m 'init' mkdir b # Assertion triggered git mv a/a.txt a b # `.git/index.lock` still lingers after this; commands like `git add # <file>` will fail ```

The output:

``` git: builtin/mv.c:481: cmd_mv: Assertion `pos >= 0' failed. Aborted (core dumped) ```

Also `.git/index.lock` is still there.

A normal error message if the command is nonsensical (I don’t know; that’s not the point). Also `.git/index.lock` to be cleaned up.

An assertion failed. `.git/index.lock` is not cleaned up.

See above.

Same behavior on `master` (a26002b628 (The fifth batch, 2024-01-02)).

``` ./bin-wrappers/git config --global --add safe.directory /tmp dir=$(mktemp -d) ./bin-wrappers/git -C $dir init mkdir $dir/a touch $dir/a/a.txt ./bin-wrappers/git -C $dir add $dir/a/a.txt ./bin-wrappers/git -C $dir commit -m 'init' mkdir $dir/b # Assertion triggered ./bin-wrappers/git -C $dir mv $dir/a/a.txt $dir/a $dir/b ```

[System Info] git version: git version 2.43.0 cpu: x86_64 no commit associated with this build sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh uname: Linux 6.2.0-39-generic #40~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Thu Nov 16 10:53:04 UTC 2 x86_64 compiler info: gnuc: 11.4 libc info: glibc: 2.35 $SHELL (typically, interactive shell): /bin/bash

[Enabled Hooks]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

Good find.

Not just that, but when the command fails in the middle like this, it leaves the working tree in a half-updated state, i.e.

will first move a/a.txt to b/a.txt, then try to move a (actually, all contents of it, including a/a.txt) to b/a and finds that "the command is nonsensical" and aborts, and by that time, there is no a/a.txt (i.e. the working tree has been modified). The failure should be made atomic, just like "git switch" to another branch may stop _without_ touching anything in the working tree when it may have to fail (e.g., due to a file being dirty).

Thanks for reporting, Kristoffer.

Any takers?

$ git shortlog --since=3.years -s -n -e --no-merges v2.43.0 builtin/mv.c
    15	Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
    10	Elijah Newren <newren@gmail.com>
     5	Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     2	Junio C Hamano <gitster@pobox.com>
     1	Andrzej Hunt <ajrhunt@google.com>
     1	Calvin Wan <calvinwan@google.com>
     1	Derrick Stolee <stolee@gmail.com>
     1	Sebastian Thiel <sebastian.thiel@icloud.com>
     1	Torsten Bögershausen <tboegi@web.de>

On 2024-01-05 19:52, Junio C Hamano wrote:

Yes, thanks to Kristoffer for reporting this issue.

This looks like a rather interesting bugfix to me. :) Though, I've unfortunately contracted some _nasty_ flu, so I'm still simply unable to work on pretty much anything in the next 5-6 days or so, at which point I hope to be operational again.

Thus, unless someone else can get it done faster, I should be able to start working on it in about a week or so. Hopefully, that is.

Here’s a failing test. This fails on top of `master` (3e0d3cd5c7 (Merge branch 'jx/dirstat-parseopt-help', 2024-02-15)).

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>

-- >8 --

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

git-mv(1) gets very confused:

    git: builtin/mv.c:506: cmd_mv: Assertion `pos >= 0' failed.
    Aborted (core dumped)
    test_must_fail: died by signal 6: git mv a/a.txt a b
    fatal: Unable to create '<path>.git/index.lock': File exists.

    Another git process seems to be running in this repository, e.g.
    an editor opened by 'git commit'. Please make sure all processes
    are terminated then try again. If it still fails, a git process
    may have crashed in this repository earlier:
    remove the file manually to continue.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    It’s been a good while.  Let’s just add this as a known breakage?

 t/t7001-mv.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

On Mon, Oct 21, 2024 at 12:14:46AM +0200, kristofferhaugsbakk@fastmail.com wrote:

There was some good analysis of what the problem was in the earlier parts of this thread. I think it is probably worth capturing some of those here, too.

Do we want to be so specific about the line number that the assertion failure occurs on? The actual coredump triggered by this test will tell us that information. But in the meantime this line is likely to go stale as builtin/mv.c changes over time.

Thanks, Taylor

On Mon, Oct 21, 2024, at 23:21, Taylor Blau wrote:

I will try to incorporate Junio’s analysis into the commit message tomorrow. :)

You’re right, it’s overly specific/volatile.

Thanks

On Mon, Oct 21, 2024 at 11:25:26PM +0200, Kristoffer Haugsbakk wrote:

Thanks, I look forward to it!

Thanks, Taylor

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

`git mv a/a.txt a b/` is a nonsense instruction. Instead of failing gracefully the command trips over itself,[1] leaving behind unfinished work:

1. first it moves `a/a.txt` to `b/a.txt`; then
2. tries to move `a/`, including `a/a.txt`; then
3. figures out that it’s in a bad state (assertion); and finally
4. aborts.

Now you’re left with a partially-updated index.

The command should instead fail gracefully and make no changes to the index until it knows that it can complete a sensible action.

For now just add a failing test since this has been known about for a while.[2]

† 1: Caused by a `pos >= 0` assertion [2]: https://lore.kernel.org/git/d1f739fe-b28e-451f-9e01-3d2e24a0fe0d@app.fastmail.com/

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v3:
    • Rewrite commit message based on Junio’s reply
    • Tweak test description: less volatile.  Also mention index state.
    v1/v2:
    • It’s been a good while.  Let’s just add this as a known breakage?

Notes (meta-trailers):
    Helped-by: Junio C Hamano <gitster@pobox.com>
        Comment: Commit message is based on his description
        Link: https://lore.kernel.org/git/xmqqil47obnw.fsf@gitster.g/

 t/t7001-mv.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

On Tue, Oct 22, 2024 at 11:14:33PM +0200, kristofferhaugsbakk@fastmail.com wrote:

Thanks, this the new version looks good to me. Let's start merging this one down.

Thanks, Taylor