From: Johannes Sixt <j6t@kdbg.org>

The intent of this particular call to 'git read-tree' was to fill an
index. But in fact, it only allocated an empty index. Later in the
program, the index is filled anyway by calling read-tree with specific
commits, and considering that elsewhere the index is even removed (i.e.,
it is not relied upon that the index file exists), this first call of
read-tree is completely redundant.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 Calling read-tree without arguments is not allowed according to the
 documentation. The next patch will enforce this.

 -- Hannes

 git-filter-branch.sh |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index cb9d202..195b5ef 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -259,7 +259,6 @@ test -s "$tempdir"/heads ||

 GIT_INDEX_FILE="$(pwd)/../index"
 export GIT_INDEX_FILE
-git read-tree || die "Could not seed the index"

 # map old->new commit ids for rewriting parents
 mkdir ../map || die "Could not create map/ directory"
-- 
1.6.6.rc1.46.g1635

--

[ message continues ]

From: Johannes Sixt <j6t@kdbg.org>

Previously, it was possible to run read-tree without any arguments,
whereupon it purged the index!

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 Discovered by typing

   git ..daab02

 with help.autocorrect > 0 :-)

 -- Hannes

 builtin-read-tree.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 50413ca..31623b9 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -125,6 +125,9 @@ int cmd_read_tree(int argc, const char **argv,
 		stage = opts.merge = 1;
 	}

+	if (argc == 0)
+		usage_with_options(read_tree_usage, read_tree_options);
+
 	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];

-- 
1.6.6.rc1.46.g1635

--

[ message continues ]

You have queued 1/2 (filter-branch: remove an unnecessary use of
'git read-tree') of this 2-patch series, but I haven't seen any comments
about this 2/2 nor is it queued. Did it fall through the cracks?

--- 8< ---
From: Johannes Sixt <j6t@kdbg.org>
Date: Tue, 15 Dec 2009 09:21:53 +0100
Subject: [PATCH] read-tree: at least one tree-ish argument is required

Previously, it was possible to run read-tree without any arguments,
whereupon it purged the index!

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 builtin-read-tree.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 50413ca..31623b9 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -125,6 +125,9 @@ int cmd_read_tree(int argc, const char **argv,
 		stage = opts.merge = 1;
 	}

+	if (argc == 0)
+		usage_with_options(read_tree_usage, read_tree_options);
+
 	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];

-- 
1.6.6.rc3.54.g0f72d

--

[ message continues ]

No.  I think saying "is not allowed" is going a bit too far [*1*].  The
documentation does not list it as a commonly useful thing, that's all.

When a user wants to have an empty index, and does not want to touch files
under $GIT_DIR with any non-git tools (e.g. "rm -f $GIT_DIR/index) for
some religious reasons, "read-tree" without a tree would be one valid way
to do so [*2*].  That is not documented (the synopsis section even hints
that read-tree wants to take at least one tree), and I think it is a
problem that the documentation does not match what the program actually
allows.

The approach taken by [2/2] is to match the program to the documentation
by forbidding what has been allowed and what people may have been relying
on being able to do,

It was obvious that the line removed by [1/2] was a no-op not only in the
usual case but also in an error case (i.e. when the user does not have
write access to the repository), as I wrote in my review of the patch.
Compared to that, I do not think it is cut-and-dried clear that [2/2] is
the right kind of improvement.  An alternative approach to document the
zero-tree case would be a valid way to address the documentation mismatch
problem.

We need to make arguments like "'read-tree' given by mistake to empty the
index is risky", "'read-tree' as 'rm -f $GIT_DIR/index' replacement has
little value", and "'read-tree' as 'rm -f $GIT_DIR/index' replacement is
the best way to get an empty index" to weigh pros and cons between two
possible approaches before deciding which way to go, but in a pre-release
freeze, I wasn't in the mood to be one who is doing the arguments myself.


[Footnote]

*1* Didn't I say that somewhere?

*2* I suspect "rm --cached ." might be another, but it would probably be
much more expensive.
--

[ message continues ]

IMO, not only is it not useful, but it is also dangerous - it erases the 

For an operation like this, shouldn't we advocate this alternate instruction 
(which explicitly tells what is wanted) rather than the implicit and 

Sorry to drag you into this discussion, but I felt this change is maint-worthy 
(because the behavior is not only risky, but dangerous).

-- Hannes
--

[ message continues ]

Heya,


I agree, shouldn't such a dangerous function at least require a flag?
'git read-tree --empty' or something?

-- 
Cheers,

Sverre Rabbelier
--

[ message continues ]

In short, that is where we differ, as I don't think it is dangerous at
all in the "maint-worthy" sense.  

"read-tree" without -m is to "populate the index from emptiness with given
trees".  Unless you are hit by the bug in the auto-correction (whose fix
was maint-worthy), nobody would say read-tree without parameter and expect
that it wouldn't touch the index.

Sure, it will empty the index, so it is dangerous in the same sense that
"reset --hard" is dangerous because it will wipe all your local changes,
or "rm -rf it" will remove everything underneath it.  But these are all
features that are "dangerous if you didn't mean to do so but wanted to do
something else."

Removal of such features might turn out to be maint-worthy but 

    Previously, it was possible to run read-tree without any arguments,
    whereupon it purged the index!

doesn't justify it well enough.
--

[ message continues ]

Heya,


With the difference that both 'reset --hard' and 'rm -rf' need a flag
to do their destructive work? Although 'git reset' might be just as
destructive if you've been using 'git add -p' a lot or something,
mhh...

-- 
Cheers,

Sverre Rabbelier
--

[ message continues ]

I'll grant you that at least "rm -rf it" names "it" that will be wiped
very explicitly.  But just like the index and the work tree plus the index
are the implicit targets to "reset" and "reset --hard" respectively, the
index is the implicit target to "read-tree".

So it may be "dangerous" in the sense that "it would change things and if
you meant to do something else the end result would be different from what
you wanted to do".  In that sense, "log", "cat-file" and friends may be
danger-free commands and all others would be dangerous.  You might type
"commit" when you meant to say "commit -a" and record an incomplete state;
it is "dangerous" in that sense.

These are part of their feature.
--

[ message continues ]

Heya,


Speaking of which, it has hit me multiple times that I craft out a
commit with 'git add -p' and then do "git commit -am 'foo some bars'"
and lose all my hard work (because I'm used to typing 'git commit -am'
for temporary commits). I'd be happy if "git commit -am" learned to

Fair enough, then perhaps it is time for "core.nodataloss" which
either logs states to a seperate reflog (so that you can go back to
the state you were in before doing 'git read-tree') or interactively
informs the user that this will command will result in data loss
(although that sounds a tad too much like Window's "Are you sure?"
dialogs).

-- 
Cheers,

Sverre Rabbelier
--

[ message continues ]

Sounds like "commit.confirm = xxx" configuration patches are coming?  What
other questionable operations we might want to let users configure git to

I somehow suspect that you had your morning coffee yet ;-)  Aren't we
talking about the index, and why are you bringing up the reflog?

More importantly, if you accept that there are non-interrogator commands
in the git command set, I somehow suspect that you will soon realize that
"git config core.nodataloss true" is equivalent to "chmod a-w -R .".  It
might be useful mode of non-operation (read-only historical archive) but I
do not think it deserves a configuration of its own with checks in the
code all over the place that might possibly change any states of the
repository.

"git config user.novice true" to increase the verbosity and degree of
hand-holding is an entirely different matter, but if that is what you are
advocating, you shouldn't call it "core.nodataloss".



--

[ message continues ]

Heya,



I don't see any other of the above category, that is, the category of
operations that contain 'git commit -a' with a non-empty index, and

It was a topic hijack of sorts, we were talking about data loss. I
brought up the reflog since I think that was mentioned before when we
discussed ways to prevent data loss (e.g., create a commit of the
current state when doing 'git reset --hard' and record it in such a

I don't think that's quite right, since you don't lose any data if you

That is not quite what I intended with 'core.preventdataloss' (which I
agree is a bad name for what I intend with it). I meant for a
configuration option which insures that all non-interrogation commands
make sure that what they throw away is recoverable (for example

I'm not sure I'd call it "user.novice", since I don't consider myself
a novice user, and I would definitely like to be able to recover data
that I accidentally deleted with 'git reset --hard'.

-- 
Cheers,

Sverre Rabbelier
--

[ message continues ]

Really? "rm -rf", "reset --hard", "commit -a": yes, RTFM. But "read-tree" (w/o 
arguments): no. There is no such sign in the documentation. Since the 
operation of the latter is dubious at best, I'd rather change the program 
than the documentation.

How about this commit message, then?

Subject: [PATCH] read-tree: at least one tree-ish argument is required

Running read-tree without any arguments purges the index, but this is not
documented. This behavior is dubious at best because contrary to many
other commands, it does not use HEAD if nothing else is specified.

If one really wants to clear the index, this can be achieved with
'git rm --cached .' or 'rm -f .git/index' in a more explicit way.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
--

[ message continues ]

One can (I think) also always use "git read-tree <empty tree>",
where <empty tree> = 4b825dc642cb6eb9a060e54bf8d69288fbee4904

-- 
Jakub Narebski
Poland
ShadeHawk on #git
--

[ message continues ]

Come on.  If you genuinely believe that

    $ git read-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904

is a better way to purge the index than

    $ git read-tree

then you need to get your head examined.  No, read-tree does not default
to examine HEAD and that will not change ;-).

Besides, read-tree is a plumbing.

Come back with a proof that there has never existed any script that uses
"read-tree" without arguments to purge the index, and I'd immediately
accept and apply the patch to retroactively forbid what the implementation
has allowed users to do for a long time.  Otherwise, I won't be involved
in discussing this before the next release is cut, as a change like this
needs a reasonable transition strategy as usual, and needs to happen after
the next release.

--

[ message continues ]

For what it's worth, I compiled the very first version of git

 commit e83c5163316f89bfbde7d9ab23ca2e25604af290
 Author: Linus Torvalds <torvalds@ppc970.osdl.org>
 Date:   Thu Apr 7 15:13:13 2005 -0700

     Initial revision of "git", the information manager from hell

and its read-tree fails with 

  read-tree: read-tree <key>

Is it a proof enough?

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

--

[ message continues ]

Ok, that initial one was "read a single tree to populate the index".

I consider it a fundamentally different program from "read-tree" as we
know now, which was introduced by d99082e (Make "read-tree" merge the
trees it reads by giving them consecutive states., 2005-04-15).  Ever
since that "multi-stage" version, read-tree was "starting from an empty
index, read these trees into stages #1, #2, ..."  And even that version
called the program "read-tree", not "git read-tree".  IOW, "git read-tree"
never had that "no tree is an error" restriction during its entire life.

Having said all that, I don't care that deeply either way myself.

As read-tree is a very basic and low-level Porcelain, if somebody were
using it to empty the index in an existing script, this change would
appear as a regression and hopefully will be caught eventually, and
updating such a script can be made reasonably easy if we want to be
helpful (the error message can suggest running "rm $GIT_DIR/index", for
example).

--

[ message continues ]

IOW, I would prefer to queue something like this in the upcoming version,
and then later make it die().  I do not think anybody relies on it, but we
have been wrong before.  If the warning doesn't trigger for anybody, that
is also fine as well.

 builtin-read-tree.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 2a3a32c..311c489 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -111,6 +111,9 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	argc = parse_options(argc, argv, unused_prefix, read_tree_options,
 			     read_tree_usage, 0);
 
+	if (!argc)
+		warning("running read-tree without argument to empty the index is deprecated.");
+
 	newfd = hold_locked_index(&lock_file, 1);
 
 	prefix_set = opts.prefix ? 1 : 0;

--

[ message continues ]

Hi,


I fully expect it not to trigger for anybody (except maybe you, if you 
have hidden a script somewhere that uses read-tree to empty the index), 
because at least for this developer, the standard way of emptying the 
index is simply "rm .git/index".

Ciao,
Dscho
--

[ message continues ]

Hi,


Yes, indeed.

Ciao,
Dscho
--

[ message continues ]

Very true.  The only thing it would have done is to error out early when
the user mistakenly tried to run the command in a directory in which s/he
does not have write access to, before running potentially expensive
commit listing with rev-list, but then the user would have failed to
create the tempdir before that to begin with.

Will queue, but it doesn't seem urgent to put it in 1.6.6 or 1.6.5.X

I think saying "is not allowed" is going a bit too far.  The documentation
simply does not list it as a _useful_ thing to do, that's all.

By the way, I notice that the command still insists on being run in a
clean work tree with a clean index at the beginning, but isn't everything
done inside the tempdir (i.e. ".git-rewrite" by default) these days,
including the temporary work tree tree-filter creates with "checkout-index
-fqua"?  It is obviously not a topic of this patch, but we may want to
stop doing that if we are not rewriting the current commit (which we will
know by the time we list the commits to be rewritten with rev-list before
actually starting to rewrite).
--

[ message continues ]