threads / patch / 58079

patchadd --interactive: allow `update` to stage deleted files

Subject: [PATCH] add --interactive: allow `update` to stage deleted files

## tl;dr

4 messages between Jun 28, 2022 and Jun 29, 2022. Diffs are folded; open one to read it.

replies: 3people: 4as markdown or json

Johannes Schindelin via GitGitGadget· Jun 28, 2022, 22:22 UTC · lore

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The scripted version of `git add -i` used `git update-index --add --remove`, but the built-in version implemented only the `--add` part.

This fixes https://github.com/msys2/MSYS2-packages/issues/3066

Reported-by: Christoph Reiter <reiter.christoph@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    add --interactive: fix bug in built-in variant
    
    This fixes a bug when using the built-in version of git add -i to update
    a file that has been deleted (in order to stage its deletion), where it
    fails with:
    
    fatal: unable to stat 'myfile': No such file or directory
    
    
    Since the built-in version of git add -i has been made the default in
    v2.37.0, from the users' point of view this is a regression, and this
    patch fixes it. I therefore consider this v2.37.1 material.
    
    This addresses https://github.com/msys2/MSYS2-packages/issues/3066

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1273%2Fdscho%2Fadd-i-update-deleted-file-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1273/dscho/add-i-update-deleted-file-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1273

 add-interactive.c          | 12 ++++++++++--
 t/t3701-add-interactive.sh |  9 +++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

Show changes to 2 files +19 −2

add-interactive.c, t/t3701-add-interactive.sh

diff --git a/add-interactive.c b/add-interactive.c
index 6047e8f6489..22fcd3412ca 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -697,8 +697,16 @@ static int run_update(struct add_i_state *s, const struct pathspec *ps,
 
 	for (i = 0; i < files->items.nr; i++) {
 		const char *name = files->items.items[i].string;
-		if (files->selected[i] &&
-		    add_file_to_index(s->r->index, name, 0) < 0) {
+		struct stat st;
+
+		if (!files->selected[i])
+			continue;
+		if (lstat(name, &st) && is_missing_file_error(errno)) {
+			if (remove_file_from_index(s->r->index, name) < 0) {
+				res = error(_("could not stage '%s'"), name);
+				break;
+			}
+		} else if (add_file_to_index(s->r->index, name, 0) < 0) {
 			res = error(_("could not stage '%s'"), name);
 			break;
 		}
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index fc26cb8bae8..b354fb39de8 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -103,6 +103,15 @@ test_expect_success 'status works (commit)' '
 	grep "+1/-0 *+2/-0 file" output
 '
 
+test_expect_success 'update can stage deletions' '
+	>to-delete &&
+	git add to-delete &&
+	rm to-delete &&
+	test_write_lines u t "" | git add -i &&
+	git ls-files to-delete >output &&
+	test_must_be_empty output
+'
+
 test_expect_success 'setup expected' '
 	cat >expected <<-\EOF
 	index 180b47c..b6f2c08 100644

base-commit: e4a4b31577c7419497ac30cebe30d755b97752c5

-- 
gitgitgadget

Junio C Hamano· Jun 28, 2022, 22:35 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH] add --interactive: allow `update` to stage deleted files

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 23 quoted lines

> diff --git a/add-interactive.c b/add-interactive.c
> index 6047e8f6489..22fcd3412ca 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -697,8 +697,16 @@ static int run_update(struct add_i_state *s, const struct pathspec *ps,
>  
>  	for (i = 0; i < files->items.nr; i++) {
>  		const char *name = files->items.items[i].string;
> -		if (files->selected[i] &&
> -		    add_file_to_index(s->r->index, name, 0) < 0) {
> +		struct stat st;
> +
> +		if (!files->selected[i])
> +			continue;
> +		if (lstat(name, &st) && is_missing_file_error(errno)) {
> +			if (remove_file_from_index(s->r->index, name) < 0) {
> +				res = error(_("could not stage '%s'"), name);
> +				break;
> +			}
> +		} else if (add_file_to_index(s->r->index, name, 0) < 0) {
>  			res = error(_("could not stage '%s'"), name);
>  			break;
>  		}

The fix looks quite straight-forward. It is somewhat disappointing that it was caught after this loooong time, but as they say, better late than never.

Thanks for a quick report and a fix.

Show 22 quoted lines

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index fc26cb8bae8..b354fb39de8 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -103,6 +103,15 @@ test_expect_success 'status works (commit)' '
>  	grep "+1/-0 *+2/-0 file" output
>  '
>  
> +test_expect_success 'update can stage deletions' '
> +	>to-delete &&
> +	git add to-delete &&
> +	rm to-delete &&
> +	test_write_lines u t "" | git add -i &&
> +	git ls-files to-delete >output &&
> +	test_must_be_empty output
> +'
> +
>  test_expect_success 'setup expected' '
>  	cat >expected <<-\EOF
>  	index 180b47c..b6f2c08 100644
>
> base-commit: e4a4b31577c7419497ac30cebe30d755b97752c5

Taylor Blau· Jun 28, 2022, 23:08 UTC · re: Junio C Hamano · lore

Re: [PATCH] add --interactive: allow `update` to stage deleted files

On Tue, Jun 28, 2022 at 03:35:20PM -0700, Junio C Hamano wrote:

> The fix looks quite straight-forward.  It is somewhat disappointing
> that it was caught after this loooong time, but as they say, better
> late than never.

I had the same thought, but I think that it points to something I've suspected privately for a while which is that we don't actually get many users to opt-in to features early when they are hidden behind a configuration.

I had hoped that things like feature.experimental would cause more users to try features that weren't quite ready for prime-time more often, and that we'd get more and better feedback as a result.

But I think that this proves that is generally not so. I think we should probably err on the side of enabling new features by default so long as there is sufficiently low risk, rather than hide them behind config. Or at least, not hide them behind a config variable for so long (though I am guilty of this myself with the pack.writeReverseIndex, which I have been meaning to flip the default on for a little while now).

In either case...

> Thanks for a quick report and a fix.

Yes, indeed. This fix looks good to me.

Thanks, Taylor

Ævar Arnfjörð Bjarmason· Jun 29, 2022, 10:22 UTC · re: Taylor Blau · lore

Re: [PATCH] add --interactive: allow `update` to stage deleted files

On Tue, Jun 28 2022, Taylor Blau wrote:

Show 21 quoted lines

> On Tue, Jun 28, 2022 at 03:35:20PM -0700, Junio C Hamano wrote:
>> The fix looks quite straight-forward.  It is somewhat disappointing
>> that it was caught after this loooong time, but as they say, better
>> late than never.
>
> I had the same thought, but I think that it points to something I've
> suspected privately for a while which is that we don't actually get many
> users to opt-in to features early when they are hidden behind a
> configuration.
>
> I had hoped that things like feature.experimental would cause more users
> to try features that weren't quite ready for prime-time more often, and
> that we'd get more and better feedback as a result.
>
> But I think that this proves that is generally not so. I think we
> should probably err on the side of enabling new features by default so
> long as there is sufficiently low risk, rather than hide them behind
> config. Or at least, not hide them behind a config variable for so long
> (though I am guilty of this myself with the pack.writeReverseIndex,
> which I have been meaning to flip the default on for a little while
> now).

I think we could get quite far by piggy-backing on the advice we emit when you haven't set user.{email,name}, or otherwise find some minimally annoying way to ask users to opt-in.

But while an opt-out being the default isn't quite the same as just changing the code (as you can still toggle it off without downgrading), that also has the disadvantage that we'd be more conservative about adding such features, which is partially why we have feature.* in the first place.

The X-Y problem of how we get early code to users who'd be able to tell us that it breaks has been discussed several times, and all the solutions are trade-offs.

One thing I thought of now but I don't think has been brought up before: I wonder if doing "more stable" (or "LTS"?) releases would be a good compromise between "early bird" and long-term stability, where we'd just make every Nth release (say those divisible by 5) have feature.experimental=false.

But any such scheme also quickly runs into the problem that we're just not adding a lot of these, and for e.g. the built-in conversion we might do the whole feature cycle in 3-4 releases, so such "stable" releases would force us to keep the code around longer.