- From:
- Stefan Sperling <stsp@stsp.name>
- Subject:
- fix stage -p vs. revert bug
- To:
- gameoftrees@openbsd.org
- Date:
- Sun, 13 Jul 2025 11:06:33 +0200
Download raw body.
Thread
fix stage -p vs. revert bug
Download raw body.
fix stage -p vs. revert bug
There is a bug where changes staged via stage -p disappear from the
work tree during 'got revert'. This happens when revert runs on a file
which has some changes staged while other changes in this file are not
yet staged.
The patch below adds test coverages and fixes the bug. ok?
do not clobber changes staged via stage -p during 'got revert'
While we must install staged blob contents during revert if there are
staged changes, the base blob ID recorded in the file index must always
reflect the actual base blob from the base commit. Setting the base blob
ID to that of a staged blob causes problems. The staged blob's ID is
tracked in a separate field.
M TODO | 0+ 1-
M lib/worktree.c | 31+ 10-
M regress/cmdline/revert.sh | 209+ 0-
3 files changed, 240 insertions(+), 11 deletions(-)
commit - 2f46ecd27c3d285e0a82670ff8dc042db5e58d5d
commit + 21fb8932d99b70820f52bb8b3579d9313a685e5c
blob - fdd45a8225acb8f4b0bea8ee43b6062a5c340a16
blob + 1bb437ac42e230173a91b0c482714d6b0fa3e527
--- TODO
+++ TODO
@@ -38,7 +38,6 @@ got:
to make this possible. The C code we publish should use the MIT license.
- 'got send' should support the equivalent to 'got fetch -R', allowing
arbitrary references to be sent
-- stage -p and revert of other changes in the same file loses staged changes
tog:
- make 'tog log' respond to key presses while 'loading...' history; loading
blob - 42e673a56bd12ea603762da85d77d62677e35aaf
blob + e229bb2a3ce1e8dedaa1b6d9cbb7ee90751a6673
--- lib/worktree.c
+++ lib/worktree.c
@@ -5389,11 +5389,21 @@ revert_file(void *arg, unsigned char status, unsigned
goto done;
}
}
- err = got_fileindex_entry_update(ie,
- a->worktree->root_fd, relpath,
- &blob->id, &ie->commit, 0);
- if (err)
- goto done;
+ if (staged_status == GOT_STATUS_ADD ||
+ staged_status == GOT_STATUS_MODIFY) {
+ got_fileindex_entry_get_blob_id(&id, ie);
+ err = got_fileindex_entry_update(ie,
+ a->worktree->root_fd, relpath, &id,
+ &ie->commit, 0);
+ if (err)
+ goto done;
+ } else {
+ err = got_fileindex_entry_update(ie,
+ a->worktree->root_fd, relpath, &blob->id,
+ &ie->commit, 0);
+ if (err)
+ goto done;
+ }
} else {
int is_bad_symlink = 0;
if (te && S_ISLNK(te->mode)) {
@@ -5411,11 +5421,22 @@ revert_file(void *arg, unsigned char status, unsigned
}
if (err)
goto done;
- err = got_fileindex_entry_update(ie,
- a->worktree->root_fd, relpath,
- &blob->id, &ie->commit, 0);
- if (err)
- goto done;
+
+ if (staged_status == GOT_STATUS_ADD ||
+ staged_status == GOT_STATUS_MODIFY) {
+ got_fileindex_entry_get_blob_id(&id, ie);
+ err = got_fileindex_entry_update(ie,
+ a->worktree->root_fd, relpath, &id,
+ &ie->commit, 0);
+ if (err)
+ goto done;
+ } else {
+ err = got_fileindex_entry_update(ie,
+ a->worktree->root_fd, relpath, &blob->id,
+ &ie->commit, 0);
+ if (err)
+ goto done;
+ }
if (is_bad_symlink) {
got_fileindex_entry_filetype_set(ie,
GOT_FILEIDX_MODE_BAD_SYMLINK);
blob - cc12859f70df97800a252ad662cc906397252208
blob + 80353942463567fdee376f1cbfd018b1513b4539
--- regress/cmdline/revert.sh
+++ regress/cmdline/revert.sh
@@ -1865,6 +1865,213 @@ test_revert_patch_binary() {
test_done "$testroot" 0
}
+test_revert_staged_file() {
+ local testroot=`test_init revert_staged_file`
+
+ got checkout $testroot/repo $testroot/wt > /dev/null
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ echo "line 0" > $testroot/wt/epsilon/zeta
+ for i in `seq 1 10`; do
+ echo "line $i" >> $testroot/wt/epsilon/zeta
+ done
+
+ (cd $testroot/wt && got commit -m 'make zeta a multi-line file' \
+ > /dev/null)
+ local commit_id=`git_show_head $testroot/repo`
+
+ sed -i -e 's/line 0/line 0a/' $testroot/wt/epsilon/zeta
+ sed -i -e 's/line 4/line 4a/' $testroot/wt/epsilon/zeta
+ sed -i -e 's/line 6/line 6a/' $testroot/wt/epsilon/zeta
+
+ # stage line 0 and line 6
+ printf "y\n" > $testroot/patchscript
+ for i in `seq 1 5`; do
+ printf "n\n" >> $testroot/patchscript
+ done
+ printf "y\n" >> $testroot/patchscript
+ for i in `seq 7 10`; do
+ printf "n\n" >> $testroot/patchscript
+ done
+
+ (cd $testroot/wt && got stage > /dev/null)
+
+ echo ' M epsilon/zeta' > $testroot/stdout.expected
+
+ (cd $testroot/wt && got status > $testroot/stdout)
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ blobid_zeta=$(get_blob_id $testroot/repo epsilon zeta)
+ stageid_zeta=$(cd $testroot/wt && got stage -l epsilon/zeta | cut -d' ' -f 1)
+
+ (cd $testroot/wt && got revert epsilon/zeta > $testroot/stdout)
+
+ echo -n '' > $testroot/stdout.expected
+
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ echo ' M epsilon/zeta' > $testroot/stdout.expected
+ (cd $testroot/wt && got status > $testroot/stdout)
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ cat <<-eof >$testroot/stdout.expected
+ diff -s $testroot/wt
+ path + $testroot/wt (staged changes)
+ commit - $commit_id
+ blob - $blobid_zeta
+ blob + $stageid_zeta
+ --- epsilon/zeta
+ +++ epsilon/zeta
+ @@ -1,10 +1,10 @@
+ -line 0
+ +line 0a
+ line 1
+ line 2
+ line 3
+ -line 4
+ +line 4a
+ line 5
+ -line 6
+ +line 6a
+ line 7
+ line 8
+ line 9
+ eof
+
+ (cd $testroot/wt && got diff -s > $testroot/stdout)
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ test_done "$testroot" "$ret"
+}
+
+test_revert_partially_staged_file() {
+ local testroot=`test_init revert_partially_staged_file`
+
+ got checkout $testroot/repo $testroot/wt > /dev/null
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ echo "line 0" > $testroot/wt/epsilon/zeta
+ for i in `seq 1 10`; do
+ echo "line $i" >> $testroot/wt/epsilon/zeta
+ done
+
+ (cd $testroot/wt && got commit -m 'make zeta a multi-line file' \
+ > /dev/null)
+ local commit_id=`git_show_head $testroot/repo`
+
+ sed -i -e 's/line 0/line 0a/' $testroot/wt/epsilon/zeta
+ sed -i -e 's/line 4/line 4a/' $testroot/wt/epsilon/zeta
+ sed -i -e 's/line 6/line 6a/' $testroot/wt/epsilon/zeta
+
+ # stage line 0 and line 6
+ printf "y\n" > $testroot/patchscript
+ for i in `seq 1 5`; do
+ printf "n\n" >> $testroot/patchscript
+ done
+ printf "y\n" >> $testroot/patchscript
+ for i in `seq 7 10`; do
+ printf "n\n" >> $testroot/patchscript
+ done
+
+ (cd $testroot/wt && got stage -F $testroot/patchscript -p > /dev/null)
+
+ echo 'MM epsilon/zeta' > $testroot/stdout.expected
+
+ (cd $testroot/wt && got status > $testroot/stdout)
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ blobid_zeta=$(get_blob_id $testroot/repo epsilon zeta)
+ stageid_zeta=$(cd $testroot/wt && got stage -l epsilon/zeta | \
+ cut -d' ' -f 1)
+
+ (cd $testroot/wt && got revert epsilon/zeta > $testroot/stdout)
+
+ echo 'R epsilon/zeta' > $testroot/stdout.expected
+
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ echo ' M epsilon/zeta' > $testroot/stdout.expected
+ (cd $testroot/wt && got status > $testroot/stdout)
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ cat <<-eof >$testroot/stdout.expected
+ diff -s $testroot/wt
+ path + $testroot/wt (staged changes)
+ commit - $commit_id
+ blob - $blobid_zeta
+ blob + $stageid_zeta
+ --- epsilon/zeta
+ +++ epsilon/zeta
+ @@ -1,4 +1,4 @@
+ -line 0
+ +line 0a
+ line 1
+ line 2
+ line 3
+ eof
+
+ (cd $testroot/wt && got diff -s > $testroot/stdout)
+ cmp -s $testroot/stdout.expected $testroot/stdout
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stdout.expected $testroot/stdout
+ test_done "$testroot" "$ret"
+ return 1
+ fi
+
+ test_done "$testroot" "$ret"
+}
+
test_parseargs "$@"
run_test test_revert_basic
run_test test_revert_rm
@@ -1885,3 +2092,5 @@ run_test test_revert_symlink
run_test test_revert_patch_symlink
run_test test_revert_umask
run_test test_revert_patch_binary
+run_test test_revert_staged_file
+run_test test_revert_partially_staged_file
fix stage -p vs. revert bug