Some index-base bug in "next" branch ("git-applymbox"-related?)..

Some index-base bug in "next" branch ("git-applymbox"-related?)..

[Linus Torvalds]

Junio, I've been running "next", because of working on the submodule 
stuff.

I  just did some trivial kernel stuff: applied seven patches, and tried to 
do a "pull". And whoa, that failed. I get this:

	[torvalds@woody linux]$ dotest ~/doit
	7 patch(es) to process.

	.. all apply fine ..

	[torvalds@woody linux]$ git pull master.kernel.org:/pub/scm/linux/kernel/git/galak/powerpc.git for_linus
	remote: Generating pack...
	...
	Unpacking 22 objects
	 100% (22/22) done
	Your index is based on '80584ff3b99c36ead7e130e453b3a48b18072d18' commit, but the branch tip you are on is at '6a04de6dbe1772d98fddf5099738d6f508e86e21'

That "80584ff.." commit is the commit *before* the "dotest", and HEAD is 
(correctly) 6a04de.. that is the end result of the "dotest". That "dotest" 
thing is just because "git-applymbox" isn't in my brain stem:

	[torvalds@woody linux]$ alias dotest
	alias dotest='git-applymbox -u'

so it's not actually anything strange.

Doing a "git reset" obviously fixed it, but if I didn't know that you had 
been working on that "index BASE" thing, I would have been very worried.

That index base thing is definitely *not* ready for merging into master 
yet!

		Linus

Re: Some index-base bug in "next" branch ("git-applymbox"-related?)..

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> That index base thing is definitely *not* ready for merging into master 
> yet!

Heh, I know, as I even left applymbox unpatched (there is a
trivial change made to git-am which should be portable to
applymbox) more or less on purpose to see if anybody noticed
;-).

I've sent messages to the list asking for help in testing and
fixing for a handful times already, and, if we do not count
Shawn mentioning one glitch on #git, this is the first time
anybody said anything about the series.  I am kind of depressed
that you are bringing it up now.

Re: Some index-base bug in "next" branch ("git-applymbox"-related?)..

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> That "80584ff.." commit is the commit *before* the "dotest", and HEAD is 
> (correctly) 6a04de.. that is the end result of the "dotest". That "dotest" 
> thing is just because "git-applymbox" isn't in my brain stem:
>
> 	[torvalds@woody linux]$ alias dotest
> 	alias dotest='git-applymbox -u'
>
> so it's not actually anything strange.

This will teach applymbox the index base safety, but it shows
that the division between applymbox and applypatch makes things
less efficient than how git-am can do the same.  In 5ddb93e that
teaches the same to git-am, we need to deal with the index base
only once at the beginning to check and once at the end to
update.  applypatch being a separate program means we cannot do
that easily inside applymbox.

-- >8 --
[PATCH] Teach applypatch about the index base

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 git-applypatch.sh |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/git-applypatch.sh b/git-applypatch.sh
index 8df2aee..c6d88e4 100755
--- a/git-applypatch.sh
+++ b/git-applypatch.sh
@@ -14,6 +14,8 @@
 USAGE='<msg> <patch> <info> [<signoff>]'
 . git-sh-setup
 
+check_base || exit
+
 case "$#" in 3|4) ;; *) usage ;; esac
 
 final=.dotest/final-commit
@@ -205,6 +207,7 @@ parent=$(git-rev-parse --verify HEAD) &&
 commit=$(git-commit-tree $tree -p $parent <"$final") || exit 1
 echo Committed: $commit
 git-update-ref -m "applypatch: $SUBJECT" HEAD $commit $parent || exit
+git update-index --set-base "$commit"
 
 if test -x "$GIT_DIR"/hooks/post-applypatch
 then
-- 
1.5.1.1.772.gab9f7

Re: Some index-base bug in "next" branch ("git-applymbox"-related?)..

[Linus Torvalds]

On Thu, 12 Apr 2007, Junio C Hamano wrote:
> 
> This will teach applymbox the index base safety, but it shows
> that the division between applymbox and applypatch makes things
> less efficient than how git-am can do the same.

Hey, I'd happily use git-am too..

If you want to deprecare git-applymbox, why don't you just make it an 
alias for git-am, potentially doing the defaults and command line argument 
conversion?

I certainly don't care about the *implementation* details. As long as my 
mailbox applicator continues to work ;)

			Linus

Re: Some index-base bug in "next" branch ("git-applymbox"-related?)..

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, 12 Apr 2007, Junio C Hamano wrote:
>> 
>> This will teach applymbox the index base safety, but it shows
>> that the division between applymbox and applypatch makes things
>> less efficient than how git-am can do the same.
>
> Hey, I'd happily use git-am too..
>
> If you want to deprecare git-applymbox, why don't you just make it an 
> alias for git-am, potentially doing the defaults and command line argument 
> conversion?

I'd rather not.

The whole reason I did not _enhance_ git-applymbox but wrote a
separate git-am was because I found git-applymbox's command line
parameter quite insane and felt that supporting it at the same
time doing a saner command line parameter was too much kludge.

Re: Some index-base bug in "next" branch ("git-applymbox"-related?)..

[Linus Torvalds]

On Thu, 12 Apr 2007, Junio C Hamano wrote:
> 
> The whole reason I did not _enhance_ git-applymbox but wrote a
> separate git-am was because I found git-applymbox's command line
> parameter quite insane and felt that supporting it at the same
> time doing a saner command line parameter was too much kludge.

Heh. I'll try to make my alias be

	alias dotest='git-am --utf8'

and see what happens ;)

		Linus

Re: Some index-base bug in "next" branch ("git-applymbox"-related?)..

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, 12 Apr 2007, Junio C Hamano wrote:
>> 
>> The whole reason I did not _enhance_ git-applymbox but wrote a
>> separate git-am was because I found git-applymbox's command line
>> parameter quite insane and felt that supporting it at the same
>> time doing a saner command line parameter was too much kludge.
>
> Heh. I'll try to make my alias be
>
> 	alias dotest='git-am --utf8'
>
> and see what happens ;)
>
> 		Linus

You should not need -u; it's default these days.

Reverting the whole index-base series

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> That index base thing is definitely *not* ready for merging into master 
> yet!

I have been thinking about the approach using index-base to
guard against somebody else updating the tip of branch you are
currently on (let's call that "gremlin updates (to the HEAD)"
for lack of better wording).  Unlike the earlier cache-tree
based write-tree optimization, it turns out to be an uphill
battle to make it an "opt-in" enhancement [*1*].

This is primarily because updating the branch tip is not tied
closely to writing out a commit, and writing out a commit is not
tied closely to writing out a tree (to be contained in that
commit) out of the index.

If a command creates a commit that has HEAD as a parent, treats
what is in the index as derived from HEAD, and/or modifies the
index and/or HEAD and leaves the index in a state suitable to
create the next commit out of that has the HEAD commit as a
parent, in order to make such a command aware of the index-base
based guard, the rules are:

 (1) You are relying on the index to be actually based on the
     HEAD you are going to record as one of the parents of the
     resulting commit.  Hence, you need to make sure the HEAD
     agrees with the commit the current index is based on.  The
     former can be read with 'rev-parse --verify HEAD', and the
     latter can be read from the BASE extension ('update-index
     --get-base').

     The design goal is not to require everybody to be
     index-base aware.  So if the index does not record BASE,
     the check should succeed, assuming the HEAD has not been
     moved.

 (2) In order to leave the index in a state suitable to create
     the next commit out of that has the updated HEAD commit as
     a parent, you need to tell the next command that performs
     the check (1) which commit the index is based on.  Use
     'update-index --set-base' to record the commit you think
     the index is based on.

     The design goal again is not to require everybody to be
     index-base aware.  Because most basic operations would not
     usually move HEAD when they update index, read_cache()
     followed by write_cache() can just keep the base if one is
     already recorded in the index, but read-tree by default
     invalidates the base, as any command that makes the index
     based on a different commit needs to lose the base by
     default (if the command is updated to be index-base aware,
     instead of losing the base, it would record the right base,
     of course).

Updating HEAD commit without changing the index can be done in
two ways.  Building on top of HEAD (write-tree then commit-tree
then update-ref), or changing the HEAD commit (using
symbolic-ref to switch branches).  I do not think there is a
sane way to make this "opt-in", and that is where recently
triggered problems come from (applymbox problem was noticed by
Linus; I have a small patch for contrib/emacs/git.el, git-gui,
quiltimport, and git-svn).  If we make symbolic-ref and
update-ref invalidate the base recorded in the index by default
to avoid false positives, that would make the feature "opt-in",
but that would defeat the whole point of detecting gremlin
update, which would update the ref using these exact commands.

Which leads me to conclude that the current approach based on
index-base needs to be rethought.  For now, I'll revert the
whole index base series from 'next'; git will remember it if we
need it later ;-).


[Footnote]

*1* Making it an "opt-in" enhancement to optimize write-tree
    using cache-tree was simpler, as there was only one place
    that actually writes out the contents of the index.  When
    the command updates paths with well-defined API implemented
    in read-cache.c, we can incrementally invalidate the
    cache-tree we originally read from the index and write the
    modified cache-tree out to keep write-tree optimized.  When
    keeping track of cache-tree entries and incrementally
    invalidating affected paths is more trouble than its worth,
    on the other hand, we can just invalidate the cache-tree
    upfront.  So the commands that do not want to bother
    spending cycles to keep cache-tree up-to-date can easily be
    prevented from writing out an out-of-sync cache-tree to
    confuse later write-tree.

Re: Reverting the whole index-base series

[Linus Torvalds]

On Sun, 15 Apr 2007, Junio C Hamano wrote:
> 
> I have been thinking about the approach using index-base to
> guard against somebody else updating the tip of branch you are
> currently on (let's call that "gremlin updates (to the HEAD)"
> for lack of better wording).  Unlike the earlier cache-tree
> based write-tree optimization, it turns out to be an uphill
> battle to make it an "opt-in" enhancement [*1*].

I never understood what you were trying to do.

The SHA1 doesn't really help.

If "next" and "pu" are the same, and you have "next" checked out, and you 
push into "pu", what happens? Since the two branches were the same, the 
SHA1 was the same before, so the BASE commit in the index will be the one 
that is updated.

The only thing that matters is that if you update the branch that HEAD 
points to, and then you'd always need to do something special, but I don't 
see that it has anything to do with what the "BASE" commit was.. It's 
purely a matter of "what does HEAD point to", independently of the index.

But no, I wasn't following that series, so I probably totally 
misunderstood what you were going after..

		Linus

Re: Reverting the whole index-base series

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> If "next" and "pu" are the same, and you have "next" checked out, and you 
> push into "pu", what happens? Since the two branches were the same, the 
> SHA1 was the same before, so the BASE commit in the index will be the one 
> that is updated.

But it does not matter if "pu" was not checked out.  You will be
building on top of "next" which was not changed.

> The only thing that matters is that if you update the branch that HEAD 
> points to, and then you'd always need to do something special, but I don't 
> see that it has anything to do with what the "BASE" commit was.. It's 
> purely a matter of "what does HEAD point to", independently of the index.
>
> But no, I wasn't following that series, so I probably totally 
> misunderstood what you were going after..

Could be.

The issue in short was about:

	$ git checkout $branch

At this point you think your HEAD is at $branch head, and you
are working towards building a commit that has that commit as
one of the parents.

Then a gremlin updates the commit HEAD points at.  Maybe
somebody else pushed into $branch.  Or you had another working
tree that shares refs (but not index nor HEAD -- perhaps set up
with contrib/workdir/git-new-workdir) with this repository and
made a commit there by mistake on the branch.

And you try to make a commit.

	$ git commit

The work you did in your repository were mostly based on the
contents of the commit you checked out but this "git commit"
will create a commit on top of something else (i.e. the one the
gremlin updated to).

To detect this case, we needed to record "which commit are we
expecting to base the next commit on".  The place to record that
information does not have to be in the index (I could have
picked a separate file .git/current-head-commit and stored the
information there), but the index was a convenient place to do
so.

So it does not have anything to do with the index, but very much
about the HEAD.  The problem was about keeping it in sync with
what really was going on in the repository / working tree.

Re: Reverting the whole index-base series

[Linus Torvalds]

On Sun, 15 Apr 2007, Junio C Hamano wrote:
> >
> > But no, I wasn't following that series, so I probably totally 
> > misunderstood what you were going after..
> 
> Could be.
> 
> The issue in short was about:
> 
> 	$ git checkout $branch
> 
> At this point you think your HEAD is at $branch head, and you
> are working towards building a commit that has that commit as
> one of the parents.
> 
> Then a gremlin updates the commit HEAD points at.

Ahh, ok. I was kind of expecting that you'd actually do something at 
"receive-pack" time instead, not at the next commit. That would also solve 
it - just have an option saying "update the working tree when receiving". 

And since I thought that was what you were aiming at, I didn't see why you 
wanted to hide the previous commit in the index..

All clear now. My confusion.

		Linus

Re: Reverting the whole index-base series

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sun, 15 Apr 2007, Junio C Hamano wrote:
>> >
>> > But no, I wasn't following that series, so I probably totally 
>> > misunderstood what you were going after..
>> 
>> Could be.
>> 
>> The issue in short was about:
>> 
>> 	$ git checkout $branch
>> 
>> At this point you think your HEAD is at $branch head, and you
>> are working towards building a commit that has that commit as
>> one of the parents.
>> 
>> Then a gremlin updates the commit HEAD points at.
>
> Ahh, ok. I was kind of expecting that you'd actually do something at 
> "receive-pack" time instead, not at the next commit. That would also solve 
> it - just have an option saying "update the working tree when receiving". 

I specifically did not want to do that, as it would break
existing workflow of pushing into a live repository, knowing the
branch might be checked out at the remote.

Also, the case of multiple working trees sharing the same refs/
namespace cannot be solved by trapping receive-pack alone.

Re: Reverting the whole index-base series

[Linus Torvalds]

On Sun, 15 Apr 2007, Junio C Hamano wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> >
> > Ahh, ok. I was kind of expecting that you'd actually do something at 
> > "receive-pack" time instead, not at the next commit. That would also solve 
> > it - just have an option saying "update the working tree when receiving". 
> 
> I specifically did not want to do that, as it would break
> existing workflow of pushing into a live repository, knowing the
> branch might be checked out at the remote.

Yeah. That said, maybe we could make it an option. There's really a few 
different things we could do:

 (a) what we do now - the working tree and index is totally unaffected
 (b) do a "git-read-tree -m old new" and if that fails, fail the push.
     This would at least ensure the *index* matches
 (b') same as (b), but with "-u" to actually check it out
 (c) do a "git-read-tree --reset new"
 (c') same as (c), but with "-u"

and we could just keep the *default* the same, but allow the receiving 
side to say what it wants to happen.

> Also, the case of multiple working trees sharing the same refs/
> namespace cannot be solved by trapping receive-pack alone.

Right. I think any working-tree thing would have to be conditional on the 
receive being done on a non-bare git repository (obviously) and we'd only 
update the directory that the push was aimed at.

So if somebody does

	git push remote:my-git-tree/.git/

then we'd *always* do (a), since we pushed into the "bare" part, but if 
somebody did

	git push remote:my-git-tree

and the receiver ended up doing a "cd .git", it would remember what the 
checked-out tree was and update that one (and no other).

			Linus

Re: Reverting the whole index-base series

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> ... That said, maybe we could make it an option. There's really a few 
> different things we could do:
>
>  (a) what we do now - the working tree and index is totally unaffected
>  (b) do a "git-read-tree -m old new" and if that fails, fail the push.
>      This would at least ensure the *index* matches
>  (b') same as (b), but with "-u" to actually check it out
>  (c) do a "git-read-tree --reset new"
>  (c') same as (c), but with "-u"
>
> and we could just keep the *default* the same, but allow the receiving 
> side to say what it wants to happen.

Yeah, throw another one in:

 (d) what we do now, but detach HEAD.

> So if somebody does
>
> 	git push remote:my-git-tree/.git/
>
> then we'd *always* do (a), since we pushed into the "bare" part, but if 
> somebody did
>
> 	git push remote:my-git-tree
>
> and the receiver ended up doing a "cd .git", it would remember what the 
> checked-out tree was and update that one (and no other).

I suspect that this would make the pending "GIT_WORK_TREE"
series more interesting.

Re: Reverting the whole index-base series

[Linus Torvalds]

On Sun, 15 Apr 2007, Junio C Hamano wrote:
> 
> > ... That said, maybe we could make it an option. There's really a few 
> > different things we could do:
> 
> Yeah, throw another one in:
> 
>  (d) what we do now, but detach HEAD.

Yeah, that actually sounds like a really nice option. I think that's the 
one I'd personally use.

		Linus