From: Ramsay Jones <ramsay@ramsay1•demon.co.uk>
To: Junio C Hamano <gitster@pobox•com>, Duy Nguyen <pclouds@gmail•com>
Cc: David Turner <dturner@twopensource•com>,
Git Mailing List <git@vger•kernel.org>,
David Turner <dturner@twitter•com>
Subject: Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit
Date: Mon, 14 Jul 2014 18:33:10 +0100 [thread overview]
Message-ID: <53C41456.2000006@ramsay1.demon.co.uk> (raw)
In-Reply-To: <xmqqr41oylyo.fsf@gitster.dls.corp.google.com>
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
next prev parent reply other threads:[~2014-07-14 17:33 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-12 4:44 [PATCH v8 1/4] cache-tree: Create/update cache-tree on checkout David Turner
2014-07-12 4:44 ` [PATCH v8 2/4] test-dump-cache-tree: invalid trees are not errors David Turner
2014-07-12 4:44 ` [PATCH v8 3/4] cache-tree: subdirectory tests David Turner
2014-07-12 4:44 ` [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit David Turner
2014-07-13 5:09 ` Duy Nguyen
2014-07-14 15:54 ` Junio C Hamano
2014-07-14 17:33 ` Ramsay Jones [this message]
2014-07-14 17:51 ` Junio C Hamano
2014-07-14 18:41 ` Ramsay Jones
2014-07-14 22:16 ` Junio C Hamano
2014-07-14 22:32 ` David Turner
2014-07-15 2:15 ` Duy Nguyen
2014-07-15 6:38 ` Junio C Hamano
2014-07-15 10:23 ` Duy Nguyen
2014-07-15 16:45 ` Junio C Hamano
2014-07-16 10:18 ` Duy Nguyen
2014-07-16 17:33 ` Junio C Hamano
2014-07-14 17:43 ` Junio C Hamano
2014-07-14 20:19 ` [PATCH v2] lockfile: allow reopening a closed but still locked file Junio C Hamano
2014-08-31 12:07 ` [PATCH v8 1/4] cache-tree: Create/update cache-tree on checkou John Keeping
2014-09-01 20:49 ` David Turner
2014-09-01 22:13 ` John Keeping
2014-09-02 20:52 ` Junio C Hamano
2014-09-02 21:10 ` Junio C Hamano
2014-09-02 21:24 ` [PATCH] cache-tree: propagate invalidation up when punting Junio C Hamano
2014-09-02 22:39 ` [PATCH v2] cache-tree: do not try to use an invalidated subtree info to build a tree Junio C Hamano
2014-09-03 2:56 ` David Turner
2014-09-03 12:02 ` Eric Sunshine
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53C41456.2000006@ramsay1.demon.co.uk \
--to=ramsay@ramsay1$(echo .)demon.co.uk \
--cc=dturner@twitter$(echo .)com \
--cc=dturner@twopensource$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=pclouds@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox