threads / bug / 7633

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

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

## tl;dr

15 messages between Apr 12, 2007 and Apr 16, 2007.

replies: 14people: 2as markdown or json

Linus Torvalds· Apr 12, 2007, 22:43 UTC · lore

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
Junio C Hamano· Apr 12, 2007, 23:07 UTC · re: Linus Torvalds · lore

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

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.

Junio C Hamano· Apr 13, 2007, 00:22 UTC · re: Linus Torvalds · lore

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

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 8 quoted lines
> 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
Linus Torvalds· Apr 13, 2007, 01:15 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Apr 13, 2007, 03:54 UTC · re: Linus Torvalds · lore

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

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 11 quoted lines
> 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.

Linus Torvalds· Apr 13, 2007, 04:16 UTC · re: Junio C Hamano · lore

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

On Thu, 12 Apr 2007, Junio C Hamano wrote:
Show 5 quoted lines
> 
> 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
Junio C Hamano· Apr 13, 2007, 04:19 UTC · re: Linus Torvalds · lore

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

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 14 quoted lines
> 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.
Junio C Hamano· Apr 15, 2007, 19:56 UTC · re: Linus Torvalds · lore

Reverting the whole index-base series

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.
Linus Torvalds· Apr 15, 2007, 21:01 UTC · re: Junio C Hamano · lore

Re: Reverting the whole index-base series

On Sun, 15 Apr 2007, Junio C Hamano wrote:
Show 7 quoted lines
> 
> 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
Junio C Hamano· Apr 15, 2007, 21:25 UTC · re: Linus Torvalds · lore

Re: Reverting the whole index-base series

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.

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

Linus Torvalds· Apr 15, 2007, 23:33 UTC · re: Junio C Hamano · lore

Re: Reverting the whole index-base series

On Sun, 15 Apr 2007, Junio C Hamano wrote:
Show 15 quoted lines
> >
> > 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
Junio C Hamano· Apr 15, 2007, 23:39 UTC · re: Linus Torvalds · lore

Re: Reverting the whole index-base series

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 20 quoted lines
> 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.

Linus Torvalds· Apr 15, 2007, 23:58 UTC · re: Junio C Hamano · lore

Re: Reverting the whole index-base series

On Sun, 15 Apr 2007, Junio C Hamano wrote:
Show 9 quoted lines
> 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
Junio C Hamano· Apr 16, 2007, 00:11 UTC · re: Linus Torvalds · lore

Re: Reverting the whole index-base series

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 12 quoted lines
> ... 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.
Show 11 quoted lines
> 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.

Linus Torvalds· Apr 16, 2007, 16:32 UTC · re: Junio C Hamano · lore

Re: Reverting the whole index-base series

On Sun, 15 Apr 2007, Junio C Hamano wrote:
Show 7 quoted lines
> 
> > ... 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

← back to recent threads