On 14/07/14 16:54, Junio C Hamano wrote:
> Duy Nguyen <[email protected]> writes:
>
>> On Sat, Jul 12, 2014 at 11:44 AM, David Turner <[email protected]>
>> 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 <[email protected]>
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 <[email protected]>
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
