* Re: git status: small difference between stating whole repository and small subdirectory
From: Nguyen Thai Ngoc Duy @ 2012-02-20 14:16 UTC (permalink / raw)
To: Jeff King
Cc: Piotr Krukowiecki, Junio C Hamano, Thomas Rast, Git Mailing List
In-Reply-To: <20120220140653.GC5131@sigill.intra.peff.net>
On Mon, Feb 20, 2012 at 9:06 PM, Jeff King <peff@peff.net> wrote:
> Interestingly, on my git.git repo, I had an empty cache. Running "git
> read-tree HEAD" filled it (according to test-dump-cache-tree). It seems
> that running "git checkout" empties the cache. So perhaps git could do
> better about keeping the cache valid over time.
For fast forward case when result index matches 100% destination tree,
yeah we should repopulate cache-tree. "git reset" does that. Not sure
about other cases though. I don't think we can keep track what
subtrees are unchanged after unpack_trees() in order to keep them.
--
Duy
^ permalink raw reply
* [PATCH] git-svn.perl: fix a false-positive in the "already exists" test
From: Steven Walter @ 2012-02-20 14:17 UTC (permalink / raw)
To: normalperson, gitster, git; +Cc: Steven Walter
In-Reply-To: <20120219105442.GA11889@dcvr.yhbt.net>
open_or_add_dir checks to see if the directory already exists or not.
If it already exists and is not a directory, then we fail. However,
open_or_add_dir did not previously account for the possibility that the
path did exist as a file, but is deleted in the current commit.
In order to prevent this legitimate case from failing, open_or_add_dir
needs to know what files are deleted in the current commit.
Unfortunately that information has to be plumbed through a couple of
layers.
Signed-off-by: Steven Walter <stevenrwalter@gmail.com>
---
git-svn.perl | 43 ++++++++++++++++++++++++++-----------------
t/t9100-git-svn-basic.sh | 33 ++++++++++++++++++---------------
2 files changed, 44 insertions(+), 32 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 06c9322..c7a961d 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -5130,7 +5130,7 @@ sub rmdirs {
}
sub open_or_add_dir {
- my ($self, $full_path, $baton) = @_;
+ my ($self, $full_path, $baton, $deletions) = @_;
my $t = $self->{types}->{$full_path};
if (!defined $t) {
die "$full_path not known in r$self->{r} or we have a bug!\n";
@@ -5139,7 +5139,7 @@ sub open_or_add_dir {
no warnings 'once';
# SVN::Node::none and SVN::Node::file are used only once,
# so we're shutting up Perl's warnings about them.
- if ($t == $SVN::Node::none) {
+ if ($t == $SVN::Node::none || defined($deletions->{$full_path})) {
return $self->add_directory($full_path, $baton,
undef, -1, $self->{pool});
} elsif ($t == $SVN::Node::dir) {
@@ -5154,17 +5154,18 @@ sub open_or_add_dir {
}
sub ensure_path {
- my ($self, $path) = @_;
+ my ($self, $path, $deletions) = @_;
my $bat = $self->{bat};
my $repo_path = $self->repo_path($path);
return $bat->{''} unless (length $repo_path);
+
my @p = split m#/+#, $repo_path;
my $c = shift @p;
- $bat->{$c} ||= $self->open_or_add_dir($c, $bat->{''});
+ $bat->{$c} ||= $self->open_or_add_dir($c, $bat->{''}, $deletions);
while (@p) {
my $c0 = $c;
$c .= '/' . shift @p;
- $bat->{$c} ||= $self->open_or_add_dir($c, $bat->{$c0});
+ $bat->{$c} ||= $self->open_or_add_dir($c, $bat->{$c0}, $deletions);
}
return $bat->{$c};
}
@@ -5221,9 +5222,9 @@ sub apply_autoprops {
}
sub A {
- my ($self, $m) = @_;
+ my ($self, $m, $deletions) = @_;
my ($dir, $file) = split_path($m->{file_b});
- my $pbat = $self->ensure_path($dir);
+ my $pbat = $self->ensure_path($dir, $deletions);
my $fbat = $self->add_file($self->repo_path($m->{file_b}), $pbat,
undef, -1);
print "\tA\t$m->{file_b}\n" unless $::_q;
@@ -5233,9 +5234,9 @@ sub A {
}
sub C {
- my ($self, $m) = @_;
+ my ($self, $m, $deletions) = @_;
my ($dir, $file) = split_path($m->{file_b});
- my $pbat = $self->ensure_path($dir);
+ my $pbat = $self->ensure_path($dir, $deletions);
my $fbat = $self->add_file($self->repo_path($m->{file_b}), $pbat,
$self->url_path($m->{file_a}), $self->{r});
print "\tC\t$m->{file_a} => $m->{file_b}\n" unless $::_q;
@@ -5252,9 +5253,9 @@ sub delete_entry {
}
sub R {
- my ($self, $m) = @_;
+ my ($self, $m, $deletions) = @_;
my ($dir, $file) = split_path($m->{file_b});
- my $pbat = $self->ensure_path($dir);
+ my $pbat = $self->ensure_path($dir, $deletions);
my $fbat = $self->add_file($self->repo_path($m->{file_b}), $pbat,
$self->url_path($m->{file_a}), $self->{r});
print "\tR\t$m->{file_a} => $m->{file_b}\n" unless $::_q;
@@ -5263,14 +5264,14 @@ sub R {
$self->close_file($fbat,undef,$self->{pool});
($dir, $file) = split_path($m->{file_a});
- $pbat = $self->ensure_path($dir);
+ $pbat = $self->ensure_path($dir, $deletions);
$self->delete_entry($m->{file_a}, $pbat);
}
sub M {
- my ($self, $m) = @_;
+ my ($self, $m, $deletions) = @_;
my ($dir, $file) = split_path($m->{file_b});
- my $pbat = $self->ensure_path($dir);
+ my $pbat = $self->ensure_path($dir, $deletions);
my $fbat = $self->open_file($self->repo_path($m->{file_b}),
$pbat,$self->{r},$self->{pool});
print "\t$m->{chg}\t$m->{file_b}\n" unless $::_q;
@@ -5340,9 +5341,9 @@ sub chg_file {
}
sub D {
- my ($self, $m) = @_;
+ my ($self, $m, $deletions) = @_;
my ($dir, $file) = split_path($m->{file_b});
- my $pbat = $self->ensure_path($dir);
+ my $pbat = $self->ensure_path($dir, $deletions);
print "\tD\t$m->{file_b}\n" unless $::_q;
$self->delete_entry($m->{file_b}, $pbat);
}
@@ -5375,10 +5376,18 @@ sub apply_diff {
my ($self) = @_;
my $mods = $self->{mods};
my %o = ( D => 0, C => 1, R => 2, A => 3, M => 4, T => 5 );
+ my %deletions;
+
+ foreach my $m (@$mods) {
+ if ($m->{chg} eq "D") {
+ $deletions{$m->{file_b}} = 1;
+ }
+ }
+
foreach my $m (sort { $o{$a->{chg}} <=> $o{$b->{chg}} } @$mods) {
my $f = $m->{chg};
if (defined $o{$f}) {
- $self->$f($m);
+ $self->$f($m, \%deletions);
} else {
fatal("Invalid change type: $f");
}
diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index b041516..4029f84 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -92,9 +92,11 @@ test_expect_success "$name" '
echo yyy > bar/zzz/yyy &&
git update-index --add bar/zzz/yyy &&
git commit -m "$name" &&
- test_must_fail git svn set-tree --find-copies-harder --rmdir \
- ${remotes_git_svn}..mybranch3' || true
-
+ git svn set-tree --find-copies-harder --rmdir \
+ ${remotes_git_svn}..mybranch3 &&
+ svn_cmd up "$SVN_TREE" &&
+ test -d "$SVN_TREE"/bar/zzz &&
+ test -e "$SVN_TREE"/bar/zzz/yyy ' || true
name='detect node change from directory to file #2'
test_expect_success "$name" '
@@ -134,10 +136,10 @@ test_expect_success "$name" '
test -x "$SVN_TREE"/exec.sh'
-name='executable file becomes a symlink to bar/zzz (file)'
+name='executable file becomes a symlink to file'
test_expect_success "$name" '
rm exec.sh &&
- ln -s bar/zzz exec.sh &&
+ ln -s file exec.sh &&
git update-index exec.sh &&
git commit -m "$name" &&
git svn set-tree --find-copies-harder --rmdir \
@@ -148,14 +150,14 @@ test_expect_success "$name" '
name='new symlink is added to a file that was also just made executable'
test_expect_success "$name" '
- chmod +x bar/zzz &&
- ln -s bar/zzz exec-2.sh &&
- git update-index --add bar/zzz exec-2.sh &&
+ chmod +x file &&
+ ln -s file exec-2.sh &&
+ git update-index --add file exec-2.sh &&
git commit -m "$name" &&
git svn set-tree --find-copies-harder --rmdir \
${remotes_git_svn}..mybranch5 &&
svn_cmd up "$SVN_TREE" &&
- test -x "$SVN_TREE"/bar/zzz &&
+ test -x "$SVN_TREE"/file &&
test -h "$SVN_TREE"/exec-2.sh'
name='modify a symlink to become a file'
@@ -195,14 +197,15 @@ name='check imported tree checksums expected tree checksums'
rm -f expected
if test_have_prereq UTF8
then
- echo tree bf522353586b1b883488f2bc73dab0d9f774b9a9 > expected
+ echo tree dc68b14b733e4ec85b04ab6f712340edc5dc936e > expected
fi
cat >> expected <<\EOF
-tree 83654bb36f019ae4fe77a0171f81075972087624
-tree 031b8d557afc6fea52894eaebb45bec52f1ba6d1
-tree 0b094cbff17168f24c302e297f55bfac65eb8bd3
-tree d667270a1f7b109f5eb3aaea21ede14b56bfdd6e
-tree 56a30b966619b863674f5978696f4a3594f2fca9
+tree c3322890dcf74901f32d216f05c5044f670ce632
+tree d3ccd5035feafd17b030c5732e7808cc49122853
+tree d03e1630363d4881e68929d532746b20b0986b83
+tree 149d63cd5878155c846e8c55d7d8487de283f89e
+tree 312b76e4f64ce14893aeac8591eb3960b065e247
+tree 149d63cd5878155c846e8c55d7d8487de283f89e
tree d667270a1f7b109f5eb3aaea21ede14b56bfdd6e
tree 8f51f74cf0163afc9ad68a4b1537288c4558b5a4
EOF
--
1.7.3.4
^ permalink raw reply related
* Re: git status: small difference between stating whole repository and small subdirectory
From: Jeff King @ 2012-02-20 14:22 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Piotr Krukowiecki, Junio C Hamano, Thomas Rast, Git Mailing List
In-Reply-To: <CACsJy8AHEHDAa2v4DvNgwd1YsBQuRCL9bHaxF70L8O4yWc4gZg@mail.gmail.com>
On Mon, Feb 20, 2012 at 09:16:43PM +0700, Nguyen Thai Ngoc Duy wrote:
> On Mon, Feb 20, 2012 at 9:06 PM, Jeff King <peff@peff.net> wrote:
> > Interestingly, on my git.git repo, I had an empty cache. Running "git
> > read-tree HEAD" filled it (according to test-dump-cache-tree). It seems
> > that running "git checkout" empties the cache. So perhaps git could do
> > better about keeping the cache valid over time.
>
> For fast forward case when result index matches 100% destination tree,
> yeah we should repopulate cache-tree. "git reset" does that. Not sure
> about other cases though. I don't think we can keep track what
> subtrees are unchanged after unpack_trees() in order to keep them.
Yeah, doing it after unpack_trees seems crazy. But I really feel like
unpack_trees should be able to handle cache updates as it unpacks. It
knows what is being updated and what is being merged. But maybe it is
more complicated than that; I haven't looked at the code yet.
-Peff
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Nguyen Thai Ngoc Duy @ 2012-02-20 14:36 UTC (permalink / raw)
To: Thomas Rast
Cc: Jeff King, Piotr Krukowiecki, Junio C Hamano, Git Mailing List
In-Reply-To: <87ty2l38ay.fsf@thomas.inf.ethz.ch>
On Mon, Feb 20, 2012 at 03:09:57PM +0100, Thomas Rast wrote:
> > Interestingly, on my git.git repo, I had an empty cache. Running "git
> > read-tree HEAD" filled it (according to test-dump-cache-tree). It seems
> > that running "git checkout" empties the cache. So perhaps git could do
> > better about keeping the cache valid over time.
>
> test_expect_failure 'checkout gives cache-tree' '
> git checkout HEAD^ &&
> test_shallow_cache_tree
> '
>
> ;-)
Quick and dirty that passes that test. I think we could do better if
we analyse two way merge rules carefully and avoid this diff, but
that's too much for me right now.
-- 8< --
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5bf96ba..c06287a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -319,6 +319,10 @@ static void show_local_changes(struct object *head, struct diff_options *opts)
die(_("diff_setup_done failed"));
add_pending_object(&rev, head, NULL);
run_diff_index(&rev, 0);
+ if (!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES)) {
+ struct tree *tree = parse_tree_indirect(head->sha1);
+ prime_cache_tree(&active_cache_tree, tree);
+ }
}
static void describe_detached_head(const char *msg, struct commit *commit)
@@ -493,13 +497,13 @@ static int merge_working_tree(struct checkout_opts *opts,
}
}
+ if (!opts->force && !opts->quiet)
+ show_local_changes(&new->commit->object, &opts->diff_options);
+
if (write_cache(newfd, active_cache, active_nr) ||
commit_locked_index(lock_file))
die(_("unable to write new index file"));
- if (!opts->force && !opts->quiet)
- show_local_changes(&new->commit->object, &opts->diff_options);
-
return 0;
}
-- 8< --
--
Duy
^ permalink raw reply related
* Re: git status: small difference between stating whole repository and small subdirectory
From: Jeff King @ 2012-02-20 14:39 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Thomas Rast, Piotr Krukowiecki, Junio C Hamano, Git Mailing List
In-Reply-To: <20120220143644.GA13938@do>
On Mon, Feb 20, 2012 at 09:36:44PM +0700, Nguyen Thai Ngoc Duy wrote:
> > test_expect_failure 'checkout gives cache-tree' '
> > git checkout HEAD^ &&
> > test_shallow_cache_tree
> > '
> >
> > ;-)
>
> Quick and dirty that passes that test. I think we could do better if
> we analyse two way merge rules carefully and avoid this diff, but
> that's too much for me right now.
Unpack trees is already sprinkled with cache_tree_invalidate_path. But
something seems to throw away the cache tree entirely (I think it may be
that the extension simply isn't copied over to the destination index).
I'm walking through it right now.
-Peff
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Jeff King @ 2012-02-20 15:11 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Thomas Rast, Piotr Krukowiecki, Junio C Hamano, Git Mailing List
In-Reply-To: <20120220143952.GA8387@sigill.intra.peff.net>
On Mon, Feb 20, 2012 at 09:39:52AM -0500, Jeff King wrote:
> On Mon, Feb 20, 2012 at 09:36:44PM +0700, Nguyen Thai Ngoc Duy wrote:
>
> > > test_expect_failure 'checkout gives cache-tree' '
> > > git checkout HEAD^ &&
> > > test_shallow_cache_tree
> > > '
> > >
> > > ;-)
> >
> > Quick and dirty that passes that test. I think we could do better if
> > we analyse two way merge rules carefully and avoid this diff, but
> > that's too much for me right now.
>
> Unpack trees is already sprinkled with cache_tree_invalidate_path. But
> something seems to throw away the cache tree entirely (I think it may be
> that the extension simply isn't copied over to the destination index).
> I'm walking through it right now.
Hmm. OK, this doesn't pass the test, but I think it is better than the
current behavior.
Basically, what happens now with "git checkout" is this:
1. read_cache pulls the cache_tree from disk into the_index
2. we call unpack_trees with o->src_index == o->dst_index ==
&the_index.
3. during tree traversal, unpack_trees callbacks properly calls
cache_tree_invalidate_path whenever it updates a path. We write the
results into o->result.
4. At the end of unpack_trees, we forget about src_index, and copy
o->result into *o->dst_index byte for byte. I.e., we overwrite
the_index.cache_tree, which has been properly updated the whole
time, with NULL, dropping it entirely (in fact, I believe it even
creates a memory leak of the old cache_tree).
This one-liner improves that a bit:
diff --git a/unpack-trees.c b/unpack-trees.c
index 8be3f6c..e8aedea 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1135,6 +1135,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
}
}
+ o->result.cache_tree = o->src_index->cache_tree;
o->src_index = NULL;
ret = check_updates(o) ? (-2) : 0;
if (o->dst_index)
by copying the cache_tree (which has been updated all along) from
src_index into the result (which will then make it into the
destination index, which of course in this case is the same as the
source index, anyway).
It makes "git checkout" with no changes just work (since we preserve the
cache tree, and it doesn't need updated). It makes something like "git
checkout HEAD^" work, keeping most of the cache-tree intact, but
invalidating trees containing paths that were modified.
But it does not actually insert the _destination_ tree into the cache
tree. Which we can do in certain situations, but only if there were no
paths in the tree that were left unchanged (e.g., you modify "foo", then
"git checkout HEAD^", which updates "bar". Your tree does not match
HEAD^, and must be invalidated). While it would be cool to be able to
handle those complex cases, making this one simple change covers most of
the cases people care about (i.e., leaving the cache-tree intact for all
of the directories that weren't touched at all).
I think this implementation matches the intent of the original calls to
cache_tree_invalidate_path sprinkled throughout unpack-trees.c. But I
have to say that it seems a little odd for us to be modifying the
o->src_index throughout the whole thing. I would think the right thing
would be to make a deep copy of o->src_index->cache_tree into
o->result.cache_tree as the very first thing, and then update
o->result.cache_tree throughout the tree traversal. There is no point in
invalidating src_index's cache_tree, since it is not receiving the
updates.
In practice, this doesn't tend to matter because everybody just sets src
and dst to &the_index anyway. The one exception seems to be diff_cache,
which sets dst_index to NULL. But it doesn't matter there, because we
are only using oneway_diff as our callback, which does not actually
write or invalidate anything in the cache.
-Peff
^ permalink raw reply related
* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Nicolas Mailhot @ 2012-02-20 15:34 UTC (permalink / raw)
To: Jeff King; +Cc: Nicolas Mailhot, git
In-Reply-To: <20120220135639.GA5131@sigill.intra.peff.net>
Le Lun 20 février 2012 14:56, Jeff King a écrit :
> On Mon, Feb 20, 2012 at 06:38:54AM +0100, Nicolas Mailhot wrote:
>
>> > As a non-browser client, what should git do? We can't make sense of the
>> > content at http://login.corporatenetwork, which is most likely an HTML
>> > form asking for credentials (or even money, if the captive portal is
>> > something like a public wireless provider). The best we can probably do
>> > is die and say "apparently you need to go http://login.corporatenetwork
>> > in a browser before making your request".
>>
>> Actually, the best would be to launch something capable of interpreting html
>> forms on the url given by the error.
>
> Doing that portably is near impossible (keep in mind that git runs on
> things like antique versions of Solaris).
Can't the you let the user specify a browser command (firefox, elinks w3m) to
auto-feed the portal page to when needed ?
The main problem with captive portals is when they shut down the connection
and the user has no idea how to restore it (and error 511 is intended to fix
this, but that won't do a lot of good if the user does is not shown the
captive portal url transmitted with the error)
Regards,
--
Nicolas Mailhot
^ permalink raw reply
* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Jeff King @ 2012-02-20 15:44 UTC (permalink / raw)
To: Nicolas Mailhot; +Cc: git
In-Reply-To: <e1d3ddd965eb32717163aaa87fa71e17.squirrel@arekh.dyndns.org>
On Mon, Feb 20, 2012 at 04:34:19PM +0100, Nicolas Mailhot wrote:
> >> Actually, the best would be to launch something capable of interpreting html
> >> forms on the url given by the error.
> >
> > Doing that portably is near impossible (keep in mind that git runs on
> > things like antique versions of Solaris).
>
> Can't the you let the user specify a browser command (firefox, elinks w3m) to
> auto-feed the portal page to when needed ?
Yes, that's why I said "we could add a configuration option" in the part
that you snipped. But doing it out of the box is not going to be
portable.
> The main problem with captive portals is when they shut down the connection
> and the user has no idea how to restore it (and error 511 is intended to fix
> this, but that won't do a lot of good if the user does is not shown the
> captive portal url transmitted with the error)
In my experience, the captive portal process usually goes like this:
1. Connect to network.
2. Try some non-browser command. Wonder why in the world it isn't
working.
3. Open a browser and say "Ah, I see. A captive portal".
The 511 proposal makes step 2 a lot better if the protocol is http[1].
But it pretty much makes it better even without non-browser client
support, because at least you will get a 511 error instead of having git
complain that the remote repository is corrupted (which happens if the
captive portal returns a redirect to an html page).
We should already be doing that. Adding more support could make step 3 a
little nicer, but like I said, I'd be more interested in seeing a real
case first. It may even be a feature that would be more appropriate to
curl (which git builds on for http access).
-Peff
[1] Of course it doesn't help at all for git:// or ssh:// (which are
usually even worse off in the first place, as many captive portals
will simply drop the packets, making it look like the remote server
is down).
^ permalink raw reply
* Re: [PATCH/RFC v2] Document format of basic Git objects
From: Jeff King @ 2012-02-20 16:11 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Junio C Hamano, git, Jonathan Niedier, Shawn O. Pearce,
Scott Chacon
In-Reply-To: <CACsJy8CEeZPf55idLB9NE-rf--ySmZh_9gkMc_zo0VmiVftMUg@mail.gmail.com>
On Mon, Feb 20, 2012 at 08:55:28PM +0700, Nguyen Thai Ngoc Duy wrote:
> > Author-ident is typically utf-8 already, so you cannot assume "ASCII".
>
> I wonder if anyone puts non utf-8 strings in there, or could we
> enforce utf-8 (i.e. validate and reject non utf-8 strings) and accept
> encoded word syntax (rfc 2047) with the help of the new
> $GIT_IDENT_ENCODING variable. The "accept ..." part can wait until
> someone is hit by "utf-8 only" check and steps up.
I was just having a similar discussion with libgit2 folks, who were
wondering if there would ever be non-utf8 in there. When we call
"reencode_commit_message", it looks like we do the whole object. In
other words, your author name _must_ match any encoding you specify in
the "encoding" header.
I.e., if you do:
# latin1 é
e=`printf '\xe9'`
export GIT_AUTHOR_NAME="P${e}ff King"
git init
git config i18n.commitencoding iso8859-1
touch foo && git add foo &&
git commit --allow-empty -m "more latin1 ${e}ncoding"
both the name and the message should show fine on your utf8 terminal if
you do this:
git config i18n.logoutputencoding utf8
git show
And similarly, we do the right thing in format-patch, both with and
without logoutputencoding set:
$ git format-patch --root --stdout | grep -Ei "^(from|subject):"
From: =?iso8859-1?q?P=E9ff=20King?= <peff@peff.net>
Subject: [PATCH] =?iso8859-1?q?more=20latin1=20=E9ncoding?=
$ git config i18n.logoutputencoding utf8
$ git format-patch --root --stdout | grep -Ei "^(from|subject):"
From: =?utf8?q?P=C3=A9ff=20King?= <peff@peff.net>
Subject: [PATCH] =?utf8?q?more=20latin1=20=C3=A9ncoding?=
(where 0xc3a9 is the utf8 equivalent of latin1 0xe9).
So I have no idea if people are using it or not, but it is actually
usable.
-Peff
^ permalink raw reply
* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Nicolas Mailhot @ 2012-02-20 18:27 UTC (permalink / raw)
To: Jeff King; +Cc: Nicolas Mailhot, git
In-Reply-To: <20120220154452.GA27456@sigill.intra.peff.net>
Le Lun 20 février 2012 16:44, Jeff King a écrit :
> In my experience, the captive portal process usually goes like this:
>
> 1. Connect to network.
>
> 2. Try some non-browser command. Wonder why in the world it isn't
> working.
>
> 3. Open a browser and say "Ah, I see. A captive portal".
>
> The 511 proposal makes step 2 a lot better if the protocol is http[1].
> But it pretty much makes it better even without non-browser client
> support, because at least you will get a 511 error instead of having git
> complain that the remote repository is corrupted (which happens if the
> captive portal returns a redirect to an html page).
>
> We should already be doing that. Adding more support could make step 3 a
> little nicer, but like I said, I'd be more interested in seeing a real
> case first. It may even be a feature that would be more appropriate to
> curl (which git builds on for http access).
Step 3 is a quite less obvious on a corporate network, where Internet access
is gated by a filtering proxy, that will let some sites pass transparently but
require credentials to let you access others. Worst case, there are several
load-balanced gateways on different physical sites (to avoid spofs in case of
planes falling on the wrong place), that do not share authentication (because
propagating auth across physical sites is hard). So no, just launching a
browser is not sufficient to find the captive portal, you need to actually
access the URL returned by error 511 in meta information. Git should at
minimum report this URL.
(and no this is not an hypothetical scenario and yes there are git users
trying to pass the gateways there)
--
Nicolas Mailhot
^ permalink raw reply
* Re: Manually decoding a git object
From: Philip Oakley @ 2012-02-20 18:27 UTC (permalink / raw)
To: Thomas Rast; +Cc: Git List, 徐迪
In-Reply-To: <871uppbwnu.fsf@thomas.inf.ethz.ch>
From: "Thomas Rast" <trast@inf.ethz.ch> Sent: Monday, February 20, 2012
10:56 AM
> Philip Oakley <philipoakley@iee.org> writes:
>
>> From: "Thomas Rast" <trast@inf.ethz.ch> Sent: Monday, February 20,
>> 2012 8:29 AM
>>>
>>> The SHA1 is over the decompressed object contents. The file simply
>>> holds a zlib-compressed stream of those contents. (It's pretty much
>>> like gzip without the file header.)
>>>
>>> You can use any bindings to zlib and something that does sha1, e.g. in
>>> python:
>>>
>>> $ cd g/.git/objects/aa/ # my git.git
>>> $ ls
>>> 592bda986a8380b64acd8cbb3d5bdfcbc0834d
>>> 6322a757bee31919f54edcc127608a3d724c99
>>> $ python
>>> Python 2.7.2 (default, Aug 19 2011, 20:41:43) [GCC] on linux2
>>> Type "help", "copyright", "credits" or "license" for more information.
>>> >>> import hashlib
>>> >>>
>>> hashlib.sha1(open('592bda986a8380b64acd8cbb3d5bdfcbc0834d').read().decode('zlib')).digest().encode('hex')
>>> 'aa592bda986a8380b64acd8cbb3d5bdfcbc0834d'
>>>
>>> Notice that the first byte of the hash goes into the directory name.
>>>
>>
>> At the moment I'm in a Catch 22 situation where I can't make the first
>> step of examining the deflated contents, so I can't do all those next
>> steps to get the sha1 etc.. Have I misunderstood your suggestions?
>
> Huh? The method I showed does not rely on knowing the SHA1. The fact
> that I used it on a properly filed away (by its SHA1) object file is
> immaterial, if perhaps confusing.
>
> I can untangle that python expression for you:
>
> hashlib.sha1(foo).digest() gives the SHA1 digest of the string foo,
> as a (binary) string
> foo.encode('hex') turns foo from (binary) string into its
> hex representation
> open('filename').read() opens the file called filename, and
> returns its whole contents
> foo.decode('zlib') applies the zlib decompressor to foo, and
> returns the resulting data
>
> So that trick works for any file[*], and you can then use its results to
> file it back where it needs to go.
>
>
> [*] that is sufficiently small for Python to hold it in memory, but git
> shares the same problems in that department.
>
I see what you mean now. I'll need to work out how to get Python in
msysgit - the 'minimal' part of msys keeps on biting... That is, I didn't
see it (Python) in the 1.7.8 full install bash - I didn't see it anyway.
I was hopeful that unzip/gunzip would have an option to simply deflate a
(file)stream, rather than it expecting the normal file archive.
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Thomas Rast @ 2012-02-20 18:45 UTC (permalink / raw)
To: Jeff King
Cc: Nguyen Thai Ngoc Duy, Piotr Krukowiecki, Junio C Hamano,
Git Mailing List
In-Reply-To: <20120220151134.GA13135@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 8be3f6c..e8aedea 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1135,6 +1135,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
> }
> }
>
> + o->result.cache_tree = o->src_index->cache_tree;
> o->src_index = NULL;
> ret = check_updates(o) ? (-2) : 0;
> if (o->dst_index)
Brilliant. I know I'm stealing Junio's punchline, but please make it so
:-)
Browsing around in history, it seems that this was silently broken by
34110cd (Make 'unpack_trees()' have a separate source and destination
index, 2008-03-06), which introduced the distinction between source and
destination index. Before that they were the same, so the cache tree
would have been updated correctly.
> It makes "git checkout" with no changes just work (since we preserve the
> cache tree, and it doesn't need updated). It makes something like "git
> checkout HEAD^" work, keeping most of the cache-tree intact, but
> invalidating trees containing paths that were modified.
Great. Here's a test you could use. It's a bit noisy because the
shallow in test_shallow_cache_tree no longer made any sense, but I think
it tests what we want to see.
diff --git i/t/t0090-cache-tree.sh w/t/t0090-cache-tree.sh
index 6c33e28..5706305 100755
--- i/t/t0090-cache-tree.sh
+++ w/t/t0090-cache-tree.sh
@@ -16,14 +16,16 @@ cmp_cache_tree () {
# We don't bother with actually checking the SHA1:
# test-dump-cache-tree already verifies that all existing data is
# correct.
-test_shallow_cache_tree () {
- printf "SHA (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >expect &&
+test_cache_tree () {
+ printf "SHA (%d entries, 1 subtrees)\n" $(git ls-files|wc -l) >expect &&
+ printf "SHA sub/ (%d entries, 0 subtrees)\n" $(git ls-files sub|wc -l) >>expect &&
cmp_cache_tree expect
}
test_invalid_cache_tree () {
- echo "invalid (0 subtrees)" >expect &&
- printf "SHA #(ref) (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >>expect &&
+ echo "invalid (1 subtrees)" >expect &&
+ printf "SHA #(ref) (%d entries, 1 subtrees)\n" $(git ls-files|wc -l) >>expect &&
+ printf "SHA sub/ (%d entries, 0 subtrees)\n" $(git ls-files sub|wc -l) >>expect &&
cmp_cache_tree expect
}
@@ -33,13 +35,16 @@ test_no_cache_tree () {
}
test_expect_failure 'initial commit has cache-tree' '
+ mkdir sub &&
+ echo bar > sub/bar &&
+ git add sub/bar &&
test_commit foo &&
- test_shallow_cache_tree
+ test_cache_tree
'
test_expect_success 'read-tree HEAD establishes cache-tree' '
git read-tree HEAD &&
- test_shallow_cache_tree
+ test_cache_tree
'
test_expect_success 'git-add invalidates cache-tree' '
@@ -59,7 +64,7 @@ test_expect_success 'update-index invalidates cache-tree' '
test_expect_success 'write-tree establishes cache-tree' '
test-scrap-cache-tree &&
git write-tree &&
- test_shallow_cache_tree
+ test_cache_tree
'
test_expect_success 'test-scrap-cache-tree works' '
@@ -70,24 +75,39 @@ test_expect_success 'test-scrap-cache-tree works' '
test_expect_success 'second commit has cache-tree' '
test_commit bar &&
- test_shallow_cache_tree
+ test_cache_tree
'
test_expect_success 'reset --hard gives cache-tree' '
test-scrap-cache-tree &&
git reset --hard &&
- test_shallow_cache_tree
+ test_cache_tree
'
test_expect_success 'reset --hard without index gives cache-tree' '
rm -f .git/index &&
git reset --hard &&
- test_shallow_cache_tree
+ test_cache_tree
'
-test_expect_failure 'checkout gives cache-tree' '
+test_expect_success 'checkout HEAD leaves cache-tree intact' '
+ git read-tree HEAD &&
+ git checkout HEAD &&
+ test_cache_tree
+'
+
+# NEEDSWORK: only one of these two can succeed. The second is there
+# because it would be the better result.
+test_expect_success 'checkout HEAD^ correctly invalidates cache-tree' '
+ git checkout HEAD^ &&
+ test_invalid_cache_tree
+'
+
+test_expect_failure 'checkout HEAD^ gives full cache-tree' '
+ git checkout master &&
+ git read-tree HEAD &&
git checkout HEAD^ &&
- test_shallow_cache_tree
+ test_cache_tree
'
test_done
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply related
* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Jeff King @ 2012-02-20 19:09 UTC (permalink / raw)
To: Daniel Stenberg; +Cc: Nicolas Mailhot, git
In-Reply-To: <alpine.DEB.2.00.1202202002330.28090@tvnag.unkk.fr>
On Mon, Feb 20, 2012 at 08:06:46PM +0100, Daniel Stenberg wrote:
> > 3. Open a browser and say "Ah, I see. A captive portal".
> >
> >We should already be doing that. Adding more support could make
> >step 3 a little nicer, but like I said, I'd be more interested in
> >seeing a real case first. It may even be a feature that would be
> >more appropriate to curl (which git builds on for http access).
>
> We're already discussing the 511 in the curl camp as well, but with
> even more sighs and hands in the air. 511 is clearly intended for
> HTML-understanding user agents and curl is not one of those. IMHO,
> curl will remain to simply help users to figure out that it is 511
> and leave it at that.
Thanks for the input. It sounds like our best bet is to just report the
URL from a 511 better, then. Do you have any idea yet how that
information will be available to curl library users?
> As a git user, I would probably be very surprised if using 'git'
> suddenly caused by browser to pop up a captive portal login. I would
> prefer git to instead properly explain to me that is being the victim
> of a 511 and what I should do to fix it.
I agree. Even if the "step 3" in my list is "then the user starts a
browser given the URL from git's error message", that is a huge
improvement over the current state. And it retains the principle of
least surprise.
-Peff
^ permalink raw reply
* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Jeff King @ 2012-02-20 19:15 UTC (permalink / raw)
To: Nicolas Mailhot; +Cc: git
In-Reply-To: <cb81840f853a1d43a7da03ea24c86445.squirrel@arekh.dyndns.org>
On Mon, Feb 20, 2012 at 07:27:08PM +0100, Nicolas Mailhot wrote:
> Step 3 is a quite less obvious on a corporate network, where Internet access
> is gated by a filtering proxy, that will let some sites pass transparently but
> require credentials to let you access others. Worst case, there are several
> load-balanced gateways on different physical sites (to avoid spofs in case of
> planes falling on the wrong place), that do not share authentication (because
> propagating auth across physical sites is hard). So no, just launching a
> browser is not sufficient to find the captive portal, you need to actually
> access the URL returned by error 511 in meta information. Git should at
> minimum report this URL.
>
> (and no this is not an hypothetical scenario and yes there are git users
> trying to pass the gateways there)
This is exactly the sort of information I wanted to get from a
real-world scenario. From your initial messages, it sounded like a
purely hypothetical thing.
I think a good first step would be improving the error message for a
511, then. Unfortunately, it seems from the rfc draft you sent that
callers are expected to parse the link out of the HTML given in the body
of the response. It seems silly that there is not a Location field
associated with a 511, similar to redirects.
-Peff
^ permalink raw reply
* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Daniel Stenberg @ 2012-02-20 19:06 UTC (permalink / raw)
To: Jeff King; +Cc: Nicolas Mailhot, git
In-Reply-To: <20120220154452.GA27456@sigill.intra.peff.net>
On Mon, 20 Feb 2012, Jeff King wrote:
> 3. Open a browser and say "Ah, I see. A captive portal".
>
> We should already be doing that. Adding more support could make step 3 a
> little nicer, but like I said, I'd be more interested in seeing a real case
> first. It may even be a feature that would be more appropriate to curl
> (which git builds on for http access).
We're already discussing the 511 in the curl camp as well, but with even more
sighs and hands in the air. 511 is clearly intended for HTML-understanding
user agents and curl is not one of those. IMHO, curl will remain to simply
help users to figure out that it is 511 and leave it at that.
As a git user, I would probably be very surprised if using 'git' suddenly
caused by browser to pop up a captive portal login. I would prefer git to
instead properly explain to me that is being the victim of a 511 and what I
should do to fix it.
--
/ daniel.haxx.se
^ permalink raw reply
* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Nicolas Mailhot @ 2012-02-20 19:24 UTC (permalink / raw)
To: Jeff King; +Cc: Nicolas Mailhot, git
In-Reply-To: <20120220191500.GA29228@sigill.intra.peff.net>
Le Lun 20 février 2012 20:15, Jeff King a écrit :
> On Mon, Feb 20, 2012 at 07:27:08PM +0100, Nicolas Mailhot wrote:
>
>> Step 3 is a quite less obvious on a corporate network, where Internet access
>> is gated by a filtering proxy, that will let some sites pass transparently
>> but
>> require credentials to let you access others. Worst case, there are several
>> load-balanced gateways on different physical sites (to avoid spofs in case
>> of
>> planes falling on the wrong place), that do not share authentication
>> (because
>> propagating auth across physical sites is hard). So no, just launching a
>> browser is not sufficient to find the captive portal, you need to actually
>> access the URL returned by error 511 in meta information. Git should at
>> minimum report this URL.
>>
>> (and no this is not an hypothetical scenario and yes there are git users
>> trying to pass the gateways there)
>
> This is exactly the sort of information I wanted to get from a
> real-world scenario. From your initial messages, it sounded like a
> purely hypothetical thing.
>
> I think a good first step would be improving the error message for a
> 511, then. Unfortunately, it seems from the rfc draft you sent that
> callers are expected to parse the link out of the HTML given in the body
> of the response. It seems silly that there is not a Location field
> associated with a 511, similar to redirects.
The URL is not lost in the HTML text, it's in the url meta field
<meta http-equiv="refresh"
content="0; url=https://login.example.net/">
As for while there is no Location field, I think it's because otherwise it
could behave like a redirect, and browser people made it plain they didn't
want redirects of https accesses (but I wasn't there when the spec was
written, and only skimmed the workgroup archives, so there may have been other
reasons for this choice. I'm pretty sure it's deliberate anyway).
Regards,
--
Nicolas Mailhot
^ permalink raw reply
* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Jeff King @ 2012-02-20 19:30 UTC (permalink / raw)
To: Nicolas Mailhot; +Cc: git
In-Reply-To: <72fbd4155349723da1c3c503c1c9c620.squirrel@arekh.dyndns.org>
On Mon, Feb 20, 2012 at 08:24:15PM +0100, Nicolas Mailhot wrote:
> > I think a good first step would be improving the error message for a
> > 511, then. Unfortunately, it seems from the rfc draft you sent that
> > callers are expected to parse the link out of the HTML given in the body
> > of the response. It seems silly that there is not a Location field
> > associated with a 511, similar to redirects.
>
> The URL is not lost in the HTML text, it's in the url meta field
>
> <meta http-equiv="refresh"
> content="0; url=https://login.example.net/">
Sorry, but
1. That is in the HTML in the body of the response (by body I don't
mean the HTML <body>, but the body of the http request).
2. I don't see anything in the rfc indicating that there must be a
meta tag in the response. They use it in the example of the rfc,
but they also have human-readable text with an <a> link. Do we yet
know what will be common among captive portals?
You said you have a non-hypothetical case. Can you show us the response?
> As for while there is no Location field, I think it's because otherwise it
> could behave like a redirect, and browser people made it plain they didn't
> want redirects of https accesses (but I wasn't there when the spec was
> written, and only skimmed the workgroup archives, so there may have been other
> reasons for this choice. I'm pretty sure it's deliberate anyway).
Even if they didn't call it Location, it would be nice to have some
machine-readable format that is understood by non-browser agents that
don't know how to parse HTML. But I recognize that is not your decision,
so don't feel obligated to defend it.
-Peff
^ permalink raw reply
* Re: git-subtree Ready #2
From: David A. Greene @ 2012-02-20 19:34 UTC (permalink / raw)
To: Jeff King; +Cc: git, Avery Pennarun
In-Reply-To: <87ty2ro1zf.fsf@smith.obbligato.org>
greened@obbligato.org (David A. Greene) writes:
> greened@obbligato.org (David A. Greene) writes:
>
>>> But more important than the physical layout is the maintenance plan
>>> going forward. Is Avery going to keep maintaining git-subtree, and we
>>> will just occasionally pull? Are you maintaining it? Where will patches
>>> go? To a github repo? To git@vger?
>
> I've attached Avery's response below. The short summary is that he
> thinks maintaining it in the vger git repository is the way to go and
> that he's fine moving patches to/from GitHub as necessary.
So what's the next step? I guess one of the git maintaners will have to
do a pull and merge. Anything I need to do on this end for that to
happen?
Thanks!
-Dave
^ permalink raw reply
* Re: [PATCH 0/5] diff --ignore-case
From: Junio C Hamano @ 2012-02-20 19:47 UTC (permalink / raw)
To: Thomas Rast; +Cc: Johannes Sixt, git, Chris Leong
In-Reply-To: <871upp4n15.fsf@thomas.inf.ethz.ch>
Thomas Rast <trast@inf.ethz.ch> writes:
> I wonder which one of us misunderstood the original request ;-)
Heh, I did ;-)
> It was
>
> } Is there any way to run diff -G with a case insensitivity flag?
>
> and I took that to mean "I want to find addition/removal of a string
> like -G does, but I don't know how it was capitalized".
I think it is just the matter of checking REG_ICASE that may be set in
revs->grep_filter.regflags, and propagating it down to the regcomp at the
beginning of diffcore_pickaxe_grep().
Want to try and see how well it works?
^ permalink raw reply
* Re: Handle HTTP error 511 Network Authentication Required (standard secure proxy authentification/captive portal detection)
From: Nicolas Mailhot @ 2012-02-20 19:51 UTC (permalink / raw)
To: Jeff King; +Cc: Nicolas Mailhot, git
In-Reply-To: <20120220193006.GA30904@sigill.intra.peff.net>
Le Lun 20 février 2012 20:30, Jeff King a écrit :
> On Mon, Feb 20, 2012 at 08:24:15PM +0100, Nicolas Mailhot wrote:
>
>> > I think a good first step would be improving the error message for a
>> > 511, then. Unfortunately, it seems from the rfc draft you sent that
>> > callers are expected to parse the link out of the HTML given in the body
>> > of the response. It seems silly that there is not a Location field
>> > associated with a 511, similar to redirects.
>>
>> The URL is not lost in the HTML text, it's in the url meta field
>>
>> <meta http-equiv="refresh"
>> content="0; url=https://login.example.net/">
>
> Sorry, but
>
> 1. That is in the HTML in the body of the response (by body I don't
> mean the HTML <body>, but the body of the http request).
>
> 2. I don't see anything in the rfc indicating that there must be a
> meta tag in the response. They use it in the example of the rfc,
> but they also have human-readable text with an <a> link. Do we yet
> know what will be common among captive portals?
>
> You said you have a non-hypothetical case. Can you show us the response?
Not yet because it's currently non-standard custom redirection mess we're
repurposing to follow the ietf spec (got tired of being accused of running a
crap non-standard proxy by users, so now it's ll be a crap standard proxy)
The proxy response is totally configurable (a so there's no reason we won't
follow the new spec to the letter
--
Nicolas Mailhot
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Junio C Hamano @ 2012-02-20 19:56 UTC (permalink / raw)
To: Jeff King
Cc: Piotr Krukowiecki, Thomas Rast, Git Mailing List,
Nguyen Thai Ngoc Duy
In-Reply-To: <20120220140653.GC5131@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Interestingly, on my git.git repo, I had an empty cache. Running "git
> read-tree HEAD" filled it (according to test-dump-cache-tree). It seems
> that running "git checkout" empties the cache. So perhaps git could do
> better about keeping the cache valid over time.
At least in the early days unpack-trees built the result by manually
adding an entry without calling the add_index_entry() all over the place,
which meant that it was futile to pretend that there is even a slight
chance that complex beast would correctly invalidate cached tree
information at all the necessary places. I recall that I added a code to
nuke the cache tree at the very beginning of "merging" codepaths to avoid
any bogus cache tree result to be stored in the resulting index.
These days, we have src_index and dst_index, and dst_index IIRC can start
as empty in which case "start from kept information and selectively
invalidate" would not work at all. When src_index and dst_index are the
same, however, you should be able to keep the cached tree valid, at least
in theory.
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Junio C Hamano @ 2012-02-20 19:57 UTC (permalink / raw)
To: Thomas Rast
Cc: Jeff King, Piotr Krukowiecki, Junio C Hamano, Git Mailing List,
Nguyen Thai Ngoc Duy
In-Reply-To: <87ty2l38ay.fsf@thomas.inf.ethz.ch>
Thomas Rast <trast@inf.ethz.ch> writes:
> test_expect_failure 'checkout gives cache-tree' '
> git checkout HEAD^ &&
> test_shallow_cache_tree
> '
Depending on what state you start the checkout from, that is not a valid
test. Some form of "git reset" before the checkout to ensure the initial
state is needed.
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Thomas Rast @ 2012-02-20 19:59 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Piotr Krukowiecki, Git Mailing List,
Nguyen Thai Ngoc Duy
In-Reply-To: <7vmx8dl1ln.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> test_expect_failure 'checkout gives cache-tree' '
>> git checkout HEAD^ &&
>> test_shallow_cache_tree
>> '
>
> Depending on what state you start the checkout from, that is not a valid
> test. Some form of "git reset" before the checkout to ensure the initial
> state is needed.
Oh, I was just quoting what we already had at the end of t0090 since
4eb0346f. The test preceding it runs 'git reset --hard'.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Junio C Hamano @ 2012-02-20 20:08 UTC (permalink / raw)
To: Jeff King
Cc: Nguyen Thai Ngoc Duy, Thomas Rast, Piotr Krukowiecki,
Junio C Hamano, Git Mailing List
In-Reply-To: <20120220151134.GA13135@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 4. At the end of unpack_trees, we forget about src_index, and copy
> o->result into *o->dst_index byte for byte. I.e., we overwrite
> the_index.cache_tree, which has been properly updated the whole
> time,
I strongly suspect that "properly updated" part needs to be thoroughly
audited. I wouldn't be surprised that this behaviour is what we did when
we split src_index vs dst_index when he rewrote unpack_trees() in order to
emulate the original "unpack-trees is beyond salvation because it does not
maintain cache tree correctly, just nuke it" behaviour.
> But it does not actually insert the _destination_ tree into the cache
> tree. Which we can do in certain situations, but only if there were no
> paths in the tree that were left unchanged (e.g., you modify "foo", then
> "git checkout HEAD^", which updates "bar". Your tree does not match
> HEAD^, and must be invalidated). While it would be cool to be able to
> handle those complex cases,...
It may look cool but it may not be a good change. You are spending extra
cycles to optimize for the next write-tree that may not happen before the
index is further updated.
> I think this implementation matches the intent of the original calls to
> cache_tree_invalidate_path sprinkled throughout unpack-trees.c.
Yes, and as long as we invalidate all the directories that need to be
invalidated during the unpack-tree operation, I think it is a correct
thing to do.
> But I
> have to say that it seems a little odd for us to be modifying the
> o->src_index throughout the whole thing.
Yes, that part is logically *wrong*. I think it is a remnant from the
days when there was no distinction between src_index and dst_index.
> I would think the right thing
> would be to make a deep copy of o->src_index->cache_tree into
> o->result.cache_tree as the very first thing, and then update
> o->result.cache_tree throughout the tree traversal.
Yes.
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Jeff King @ 2012-02-20 20:09 UTC (permalink / raw)
To: Junio C Hamano
Cc: Piotr Krukowiecki, Thomas Rast, Git Mailing List,
Nguyen Thai Ngoc Duy
In-Reply-To: <7vr4xpl1nm.fsf@alter.siamese.dyndns.org>
On Mon, Feb 20, 2012 at 11:56:13AM -0800, Junio C Hamano wrote:
> These days, we have src_index and dst_index, and dst_index IIRC can start
> as empty in which case "start from kept information and selectively
> invalidate" would not work at all. When src_index and dst_index are the
> same, however, you should be able to keep the cached tree valid, at least
> in theory.
Yeah, I was worried that the cache invalidations sprinkled throughout
unpack-trees.c would not be sufficient (and because we are invalidating,
a missing invalidation would give us bogus cache info, which is Very
Bad).
So I think the one-liner I posted before is not sufficient in the
general case, because it definitely doesn't consider where the
destination is starting from. It should at least be more like:
if (src_index == dst_index) {
/* We would ordinarily want to do a deep copy here, but since
* we know that we will be overwriting src_index in the long
* run, it's OK to just take ownership of its cache_tree. */
o->result.cache_tree = o->src_index->cache_tree;
o->src_index->cache_tree = NULL;
}
[... do the usual tree traversal here, except invalidate entries in
o->result.call_tree instead of o->src_index. That makes it a
no-op when src_index != dst_index (because we have no cache tree
defined in result, then), and otherwise we are invalidating what
will go into the result...]
[then as before, we copy the result to dst_index; except now the
result may have src_index's cache_tree plus any invalidations]
o->result = *o->dst_index;
And fortunately that does exactly what we want in all cases, because we
always either read from and write to the_index, or we write to NULL (in
which case we will not bother with a cache_tree for the result, and it
is fixing a minor bug that we might be invalidating src_index's tree in the
first place).
I'm still slightly worried that we are missing some invalidation
somewhere deep in unpack_tree's callbacks (especially because they _are_
callbacks, and invalidating the cache_tree properly is now a promise
that the callbacks have to make).
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox