threads / patch / 21958

patch, 2 partsfilter-branch: remove an unnecessary use of 'git read-tree'

Subject: [PATCH 1/2] filter-branch: remove an unnecessary use of 'git read-tree'

## tl;dr

21 messages between Dec 15, 2009 and Dec 19, 2009. Diffs are folded; open one to read it.

replies: 20people: 6as markdown or json

Johannes Sixt· Dec 15, 2009, 08:42 UTC · lore
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(-)
Show changes to git-filter-branch.sh +0 −1
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
Johannes Sixt· Dec 15, 2009, 08:43 UTC · re: Johannes Sixt · lore

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

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(-)
Show changes to builtin-read-tree.c +3 −0
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
Johannes Sixt· Dec 18, 2009, 09:51 UTC · re: Johannes Sixt · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

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(-)
Show changes to builtin-read-tree.c +3 −0
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
Junio C Hamano· Dec 18, 2009, 18:11 UTC · re: Johannes Sixt · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

Johannes Sixt <j.sixt@viscovery.net> writes:
> 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?

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.

Johannes Sixt· Dec 18, 2009, 19:04 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

On Freitag, 18. Dezember 2009, Junio C Hamano wrote:
Show 6 quoted lines
> Johannes Sixt <j.sixt@viscovery.net> writes:
> > 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?
>
> No.  I think saying "is not allowed" is going a bit too far [*1*].
Yes, you said that, but in response to the footnote in 1/2.
> The 
> documentation does not list it as a commonly useful thing, that's all.

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

Show 6 quoted lines
> 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*].
> *2* I suspect "rm --cached ." might be another, but it would probably be
> much more expensive.

For an operation like this, shouldn't we advocate this alternate instruction (which explicitly tells what is wanted) rather than the implicit and undocumented operation of parameter-less read-tree?

Show 6 quoted lines
> 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.

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
Sverre Rabbelier· Dec 18, 2009, 19:24 UTC · re: Johannes Sixt · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

Heya,
On Fri, Dec 18, 2009 at 13:04, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Sorry to drag you into this discussion, but I felt this change is maint-worthy
> (because the behavior is not only risky, but dangerous).

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

-- 
Cheers,

Sverre Rabbelier
Junio C Hamano· Dec 18, 2009, 19:32 UTC · re: Johannes Sixt · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

Johannes Sixt <j.sixt@viscovery.net> writes:
> 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

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.
Sverre Rabbelier· Dec 18, 2009, 19:37 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

Heya,
On Fri, Dec 18, 2009 at 13:32, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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
Junio C Hamano· Dec 18, 2009, 19:49 UTC · re: Sverre Rabbelier · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

Sverre Rabbelier <srabbelier@gmail.com> writes:
Show 9 quoted lines
> On Fri, Dec 18, 2009 at 13:32, Junio C Hamano <gitster@pobox.com> wrote:
>> 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.
>
> 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...

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.
Sverre Rabbelier· Dec 18, 2009, 19:59 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

Heya,
On Fri, Dec 18, 2009 at 13:49, Junio C Hamano <gitster@pobox.com> wrote:
> You might type
> "commit" when you meant to say "commit -a" and record an incomplete state;
> it is "dangerous" in that sense.

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 second-guess me when I already have something in the index.

> These are part of their feature.

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
Junio C Hamano· Dec 18, 2009, 20:13 UTC · re: Sverre Rabbelier · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

Sverre Rabbelier <srabbelier@gmail.com> writes:
Show 10 quoted lines
> On Fri, Dec 18, 2009 at 13:49, Junio C Hamano <gitster@pobox.com> wrote:
>> You might type
>> "commit" when you meant to say "commit -a" and record an incomplete state;
>> it is "dangerous" in that sense.
>
> 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
> second-guess me when I already have something in the index.

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

Show 6 quoted lines
> 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).

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".

Sverre Rabbelier· Dec 18, 2009, 20:21 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

Heya,
On Fri, Dec 18, 2009 at 14:13, Junio C Hamano <gitster@pobox.com> wrote:
> Sounds like "commit.confirm = xxx" configuration patches are coming?
Not a bad idea.
> What
> other questionable operations we might want to let users configure git to
> ask for confirmation?

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 'git rm' on a modified file (which we already prevent). But see below.

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

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 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 .".

I don't think that's quite right, since you don't lose any data if you do only additive commands (e.g., create new commits, etc).

> 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.

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 through the reflog).

> "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".

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
Johannes Sixt· Dec 18, 2009, 22:04 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

On Freitag, 18. Dezember 2009, Junio C Hamano wrote:
Show 10 quoted lines
> 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".
>
> [...]  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.

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>
Jakub Narebski· Dec 18, 2009, 22:17 UTC · re: Johannes Sixt · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 6 quoted lines
> 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.

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

-- 
Jakub Narebski
Poland
ShadeHawk on #git
Junio C Hamano· Dec 18, 2009, 23:46 UTC · re: Jakub Narebski · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

Jakub Narebski <jnareb@gmail.com> writes:
Show 11 quoted lines
> Johannes Sixt <j.sixt@viscovery.net> writes:
>
>> 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.
>
> One can (I think) also always use "git read-tree <empty tree>",
> where <empty tree> = 4b825dc642cb6eb9a060e54bf8d69288fbee4904
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.

Nanako Shiraishi· Dec 19, 2009, 03:25 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

Quoting Junio C Hamano <gitster@pobox.com>
> 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.
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/
Junio C Hamano· Dec 19, 2009, 04:43 UTC · re: Nanako Shiraishi · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

Nanako Shiraishi <nanako3@lavabit.com> writes:
Show 20 quoted lines
> Quoting Junio C Hamano <gitster@pobox.com>
>
>> 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.
>
> 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?
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).

Junio C Hamano· Dec 19, 2009, 04:53 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> 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).

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(-)
Show changes to builtin-read-tree.c +3 −0
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;
Johannes Schindelin· Dec 19, 2009, 10:56 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] read-tree: at least one tree-ish argument is required

Hi,
On Fri, 18 Dec 2009, Junio C Hamano wrote:
Show 13 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > 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).
> 
> 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.

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

Johannes Schindelin· Dec 15, 2009, 17:19 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/2] filter-branch: remove an unnecessary use of 'git read-tree'

Hi,
On Tue, 15 Dec 2009, Johannes Sixt wrote:
Show 8 quoted lines
> 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.
Yes, indeed.

Ciao, Dscho

Junio C Hamano· Dec 16, 2009, 00:19 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/2] filter-branch: remove an unnecessary use of 'git read-tree'

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 10 quoted lines
> 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>

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 maint (i.e. nothing is broken without this patch).

>  Calling read-tree without arguments is not allowed according to the
>  documentation.

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).

← back to recent threads