Removing a newly added file

Removing a newly added file

[Santi Béjar]

Hi *,

  when you try removing a newly added file it success and removes the
file from the working directory. So if you do:

$ echo "newly added file" > new
$ git add new
$ git rm new

the file "new" is lost, it is not in the index, neither in HEAD. At
this moment the only way to recover the file new is searching for
unreachable objects. (Am I missing something?)

  I think that the "git rm new" should remove "new" from the index or
should fail, maybe with:

$ git rm new
error: 'new' is not in HEAD (hint: try --cached)

  Santi

[PATCH] git-rm: don't remove newly added file without -f

[Jeff King]

Given this set of commands:

  $ echo "newly added file" >new
  $ git add new
  $ git rm new

the file "new" was previously removed from the working
directory and the index. Because it was not in HEAD, it is
available only by searching for unreachable objects.

Instead, we now err on the safe side and refuse to remove
a file which is not referenced by HEAD.

Signed-off-by: Jeff King <peff@peff.net>
---
> I think that the "git rm new" should remove "new" from the index or
> should fail, maybe with:

Something like this?

I think there are still some issues with the safety valve, though; this
triggers on 'git rm --cached new' which should be a perfectly safe
operation. However, that is not new to this change; we already
erroneously trigger the valve on a --cached removal of a file with
matching index and working directory, but mismatch with HEAD. Example:

  git-add foo
  git-commit -m 'added foo'
  echo changes >>foo
  git-add foo
  git-rm --cached foo ;# should be OK because we have working copy

I think the logic for "safe to remove" and "safe to remove --cached"
needs to be separate. I will take a look.

Also, this just implements "error, try -f" when it would be safe to drop
back to --cached for that file. Is there any interest in trying to make
git-rm more clever in this way, or is simple and predictable preferred?

 builtin-rm.c |   18 ++++--------------
 1 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/builtin-rm.c b/builtin-rm.c
index 00dbe39..bf42003 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c
@@ -89,20 +89,10 @@ static int check_local_mod(unsigned char *head)
 		if (ce_match_stat(ce, &st, 0))
 			errs = error("'%s' has local modifications "
 				     "(hint: try -f)", ce->name);
-		if (no_head)
-			continue;
-		/*
-		 * It is Ok to remove a newly added path, as long as
-		 * it is cache-clean.
-		 */
-		if (get_tree_entry(head, name, sha1, &mode))
-			continue;
-		/*
-		 * Otherwise make sure the version from the HEAD
-		 * matches the index.
-		 */
-		if (ce->ce_mode != create_ce_mode(mode) ||
-		    hashcmp(ce->sha1, sha1))
+		if (no_head
+		     || get_tree_entry(head, name, sha1, &mode)
+		     || ce->ce_mode != create_ce_mode(mode)
+		     || hashcmp(ce->sha1, sha1))
 			errs = error("'%s' has changes staged in the index "
 				     "(hint: try -f)", name);
 	}
-- 
1.5.1.rc2.615.g992d-dirty

Re: [PATCH] git-rm: don't remove newly added file without -f

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Given this set of commands:
>
>   $ echo "newly added file" >new
>   $ git add new
>   $ git rm new
>
> the file "new" was previously removed from the working
> directory and the index. Because it was not in HEAD, it is
> available only by searching for unreachable objects.

I am not sure if this is a problem, as the user asked it to be
removed.  It somehow feels to me that the change (I am not
convinced your patch should be called a "fix" rather than a
"behaviour change") would cause more confusion.

Unstaging request would have looked like "git reset HEAD new",
wouldn't it?

Re: [PATCH] git-rm: don't remove newly added file without -f

[Johannes Schindelin]

Hi,

On Mon, 26 Mar 2007, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Given this set of commands:
> >
> >   $ echo "newly added file" >new
> >   $ git add new
> >   $ git rm new
> >
> > the file "new" was previously removed from the working
> > directory and the index. Because it was not in HEAD, it is
> > available only by searching for unreachable objects.
> 
> I am not sure if this is a problem, as the user asked it to be
> removed.  It somehow feels to me that the change (I am not
> convinced your patch should be called a "fix" rather than a
> "behaviour change") would cause more confusion.

I agree it would add to confusion.

It is well established that "git rm" also removes the file _in the working 
directory_.

If you don't want the file to be deleted, but only the corresponding 
entry _in the index_, use "git rm --cached".

Ciao,
Dscho

Re: [PATCH] git-rm: don't remove newly added file without -f

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Mon, 26 Mar 2007, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > Given this set of commands:
>> >
>> >   $ echo "newly added file" >new
>> >   $ git add new
>> >   $ git rm new
>> >
>> > the file "new" was previously removed from the working
>> > directory and the index. Because it was not in HEAD, it is
>> > available only by searching for unreachable objects.
>> 
>> I am not sure if this is a problem, as the user asked it to be
>> removed.  It somehow feels to me that the change (I am not
>> convinced your patch should be called a "fix" rather than a
>> "behaviour change") would cause more confusion.
>
> I agree it would add to confusion.
>
> It is well established that "git rm" also removes the file _in the working 
> directory_.
>
> If you don't want the file to be deleted, but only the corresponding 
> entry _in the index_, use "git rm --cached".

Actually, thinking about it a bit more, I think Jeff's patch is
in line with the current behaviour.

Looking at the cases where we prevent 'git rm' without '-f'
succeeding currently, the motivation is to save the user from
mistakenly saying "remove" when the user earlier said "this
change matters in the next commit" to us.

For example, with "edit existing-file; git add existing-file",
the user said the new state of the file matters in the next
commit.  And we refuse to remove, saying "foo has changes
staged".

By saying "edit new-file; git add new-file", the user expressed
the intent to add that content to the upcoming commit.  Saying
"git rm" later is reverting that intent.  Jeff's patch does
exactly the same thing for new files what we already do for
existing ones --- we ask for a confirmation when "git rm" is
given for existing files that has staged changes, saying "you
earlier said you want changes to this file in the next commit.
are you sure you have changed your mind?"

Having said that, we do _not_ ask for confirmation when you do
"git add existing-file" after doing "edit ; git add", which is
theoretically inconsistent, but rm is special so that is
probably Ok.

Re: [PATCH] git-rm: don't remove newly added file without -f

[Jeff King]

On Mon, Mar 26, 2007 at 02:11:42PM -0700, Junio C Hamano wrote:

> Unstaging request would have looked like "git reset HEAD new",
> wouldn't it?

Yes, I think I am still in the habit of using "git rm" from its old
usage. I unstage so rarely that I don't think my fingers have adjusted.

> "git rm" later is reverting that intent.  Jeff's patch does
> exactly the same thing for new files what we already do for
> existing ones --- we ask for a confirmation when "git rm" is

Yes, regardless of how git-rm is "supposed" to be used, I think this
patch is worth it for two reasons:
  1. consistency; we should treat newly added files just like existing
     files (after all, content is content, right?)
  2. safety; I work under the assumption that I can do whatever I want
     with git-rm, and it will _never_ lose my data unless I use -f.
     Granted, without this patch my data would still be available
     somewhere in the object database, but it is hard to find and
     susceptible to pruning.

> Having said that, we do _not_ ask for confirmation when you do
> "git add existing-file" after doing "edit ; git add", which is
> theoretically inconsistent, but rm is special so that is
> probably Ok.

Hmm, yes, I agree that is inconsistent. However, it's such a common
workflow that forcing a '-f' would become meaningless. And you _can_
rescue the blob from the object database in a pinch, though finding it
can be tedious.

-Peff

Re: [PATCH] git-rm: don't remove newly added file without -f

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Hmm, yes, I agree that is inconsistent. However, it's such a common
> workflow that forcing a '-f' would become meaningless. And you _can_
> rescue the blob from the object database in a pinch, though finding it
> can be tedious.

That's exactly the reason why I mentioned this inconsistency
between add vs rm "tongue-in-cheek".

We really do not want to require -f for common situations so
much that it ends up training people's fingers to type -f
without thinking, which would render the safety option
meaningless.