Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit

[Ramsay Jones <ramsay@ramsay1.demon.co.uk>]

On 14/07/14 16:54, Junio C Hamano wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
> 
>> On Sat, Jul 12, 2014 at 11:44 AM, David Turner <dturner@twopensource.com> wrote:
>>> @@ -342,6 +342,15 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
>>>
>>>                 discard_cache();
>>>                 read_cache_from(index_lock.filename);
>>> +               if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {
>>> +                       fd = open(index_lock.filename, O_WRONLY);
>>> +                       if (fd >= 0)
>>> +                               if (write_cache(fd, active_cache, active_nr) == 0) {
>>> +                                       close_lock_file(&index_lock);
>>
>> If write_cache() returns a negative value, index.lock is probably
>> corrupted. Should we die() instead of moving on and returning
>> index_lock.filename to the caller? The caller may move index.lock to
>> index later on and officially ruin "index".
> 
> Perhaps true, but worse yet, this will not play nicely together with
> your split index series, no?  After taking the lock and writing and
> closing, we spawn the interactive while still holding the lock, and
> the "open" we see here is because we want to further update the same
> under the same lock.  Perhaps write_locked_index() API in the split
> index series can notice that the underlying fd in index_lock has
> been closed earlier, realize that the call is to re-update the
> index under the same lock and open the file again for writing?

Hmm, I was just about to suggest that there was some negative interplay
between the 'dt/cache-tree-repair' and 'nd/split-index' branches as well.

The pu branch fails the testsuite for me. In particular, t0090-cache-tree.sh
fails like so:

    $ ./t0090-cache-tree.sh -i -v
    ...
    ok 9 - second commit has cache-tree

    expecting success: 
    	cat <<-\EOT >foo.c &&
    	int foo()
    	{
    		return 42;
    	}
    	int bar()
    	{
    		return 42;
    	}
    	EOT
    	git add foo.c &&
    	test_invalid_cache_tree &&
    	git commit -m "add a file" &&
    	test_cache_tree &&
    	cat <<-\EOT >foo.c &&
    	int foo()
    	{
    		return 43;
    	}
    	int bar()
    	{
    		return 44;
    	}
    	EOT
    	(echo p; echo 1; echo; echo s; echo n; echo y; echo q) |
    	git commit --interactive -m foo &&
    	test_cache_tree
    
    [master d1075a6] add a file
     Author: A U Thor <author@example.com>
     1 file changed, 8 insertions(+)
     create mode 100644 foo.c
               staged     unstaged path
      1:    unchanged        +2/-2 foo.c
    
    *** Commands ***
      1: [s]tatus	  2: [u]pdate	  3: [r]evert	  4: [a]dd untracked
      5: [p]atch	  6: [d]iff	  7: [q]uit	  8: [h]elp
    What now>            staged     unstaged path
      1:    unchanged        +2/-2 [f]oo.c
    Patch update>>            staged     unstaged path
    * 1:    unchanged        +2/-2 [f]oo.c
    Patch update>> diff --git a/foo.c b/foo.c
    index 75522e2..3f7f049 100644
    --- a/foo.c
    +++ b/foo.c
    @@ -1,8 +1,8 @@
     int foo()
     {
    -return 42;
    +return 43;
     }
     int bar()
     {
    -return 42;
    +return 44;
     }
    Stage this hunk [y,n,q,a,d,/,s,e,?]? Split into 2 hunks.
    @@ -1,6 +1,6 @@
     int foo()
     {
    -return 42;
    +return 43;
     }
     int bar()
     {
    Stage this hunk [y,n,q,a,d,/,j,J,g,e,?]? @@ -4,5 +4,5 @@
     }
     int bar()
     {
    -return 42;
    +return 44;
     }
    Stage this hunk [y,n,q,a,d,/,K,g,e,?]? 
    *** Commands ***
      1: [s]tatus	  2: [u]pdate	  3: [r]evert	  4: [a]dd untracked
      5: [p]atch	  6: [d]iff	  7: [q]uit	  8: [h]elp
    What now> Bye.
    [master 65d7dde] foo
     Author: A U Thor <author@example.com>
     1 file changed, 1 insertion(+), 1 deletion(-)
    --- expect	2014-07-14 17:10:13.755209229 +0000
    +++ filtered	2014-07-14 17:10:13.763209258 +0000
    @@ -1 +1 @@
    -SHA  (3 entries, 0 subtrees)
    +invalid                                   (0 subtrees)
    not ok 10 - commit --interactive gives cache-tree on partial commit
    #	
    #		cat <<-\EOT >foo.c &&
    #		int foo()
    #		{
    #			return 42;
    #		}
    #		int bar()
    #		{
    #			return 42;
    #		}
    #		EOT
    #		git add foo.c &&
    #		test_invalid_cache_tree &&
    #		git commit -m "add a file" &&
    #		test_cache_tree &&
    #		cat <<-\EOT >foo.c &&
    #		int foo()
    #		{
    #			return 43;
    #		}
    #		int bar()
    #		{
    #			return 44;
    #		}
    #		EOT
    #		(echo p; echo 1; echo; echo s; echo n; echo y; echo q) |
    #		git commit --interactive -m foo &&
    #		test_cache_tree
    #	
    $ 

Note that I haven't even looked at the test failure itself yet.

However, I noticed that commit 002ccda ("cache-tree: write updated
cache-tree after commit", 11-07-2014) passes that test just fine, but
that the merge commit 7608c87e fails. Looking at the details of the
merge resolution, made me think of Duy's split index work.

I probably won't look at this further tonight, so this is just a
heads-up on a possible problem.

HTH

ATB,
Ramsay Jones