* Re: gitk problems: can't unset "idinlist(...)"
From: Jeff King @ 2007-07-21 6:54 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List, Paul Mackerras, Junio C Hamano
In-Reply-To: <alpine.LFD.0.999.0707202335570.27249@woody.linux-foundation.org>
On Fri, Jul 20, 2007 at 11:47:24PM -0700, Linus Torvalds wrote:
> So the difference between your version and mine is that the
>
> while ((p = *pp) != NULL)
I actually like your version better (I like saving for loops for
honest-to-god, for-each-element-in-this-list iteration; munging the list
in the middle is something that you _should_ call attention to). I just
found your patch much harder to verify, because I had to make sure the
for/while change was equivalent. So you can blame Junio for writing it
wrong in the first place. :)
Junio, whatever you apply, please credit Linus; he beat me to the fix
and so my patch is completely cribbed from his. :)
-Peff
^ permalink raw reply
* Re: gitk problems: can't unset "idinlist(...)"
From: Linus Torvalds @ 2007-07-21 6:47 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Paul Mackerras, Junio C Hamano
In-Reply-To: <20070721062448.GA8722@coredump.intra.peff.net>
On Sat, 21 Jul 2007, Jeff King wrote:
>
> By which I mean you can keep the 'for' loop, since in either case we are
> always reading p->next (if we skip, then we set *pp to p->next anyway,
> and if we don't, then pp becomes a pointer to p->next).
>
> So this is a more readable patch which I believe is equivalent.
Fair enough, that also works.
Anyway, just because this is actually something I've noticed a lot of
people do wrong, I actually do tend to think that when you traverse a
singly-linked list and remove entries, you should *never* traverse the
list itself, you should only ever traverse the "pointer to the pointer".
So the difference between your version and mine is that the
while ((p = *pp) != NULL)
model really only has one single "iterator variable": "pp". Which makes it
a lot easier to think about and avoid problems.
In contrast, your for-loop actually keeps track of *two* independent
variables, the "tail pointer of the list we're building up" (pp) and the
"head pointer of the old list we're traversing" (p).
Does it work? Yes. But I tend to prefer the "there's a single list" model,
where you just remove entries as you traverse it through a single indirect
pointer.
They're certainly equivalent, but slightly different mindsets. I at least
personally find the "single pointer" less likely to have problems, because
you can never get out of sync (which was basically exactly the problem
that Junio's code had), because there is nothing to get out of sync _with_
when you just have a single main iterator variable.
Linus
^ permalink raw reply
* Re: gitk problems: can't unset "idinlist(...)"
From: Linus Torvalds @ 2007-07-21 6:34 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Paul Mackerras, Junio C Hamano
In-Reply-To: <20070721061857.GB29820@coredump.intra.peff.net>
On Sat, 21 Jul 2007, Jeff King wrote:
>
> I just independently came to the same conclusion. Your patch looks
> correct to me (though there are some unrelated formatting changes which
> made it a little harder to read).
Yeah, I shouldn't have done that - I just ended up changing the "for()" to
a "while()" (because we should *not* do the same thing at the end of the
loop each time). And as a part of that I changed the initialization to be
at the head of the while-loop rather than in the variable declaration,
because that's how I tend to mostly write while-loops.
But I probably should have tried to keep the patch minimal instead, so
that it would show the changes better. Probably not a big deal.
Linus
^ permalink raw reply
* Re: gitk problems: can't unset "idinlist(...)"
From: Jeff King @ 2007-07-21 6:24 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List, Paul Mackerras, Junio C Hamano
In-Reply-To: <20070721061857.GB29820@coredump.intra.peff.net>
On Sat, Jul 21, 2007 at 02:18:57AM -0400, Jeff King wrote:
> correct to me (though there are some unrelated formatting changes which
> made it a little harder to read).
By which I mean you can keep the 'for' loop, since in either case we are
always reading p->next (if we skip, then we set *pp to p->next anyway,
and if we don't, then pp becomes a pointer to p->next).
So this is a more readable patch which I believe is equivalent.
---
diff --git a/revision.c b/revision.c
index 28b5f2e..7def867 100644
--- a/revision.c
+++ b/revision.c
@@ -1329,10 +1329,11 @@ static void remove_duplicate_parents(struct commit *commit)
/* Examine existing parents while marking ones we have seen... */
for (p = commit->parents; p; p = p->next) {
struct commit *parent = p->item;
- if (parent->object.flags & TMP_MARK)
+ if (parent->object.flags & TMP_MARK) {
+ *pp = p->next;
continue;
+ }
parent->object.flags |= TMP_MARK;
- *pp = p;
pp = &p->next;
}
/* ... and clear the temporary mark */
^ permalink raw reply related
* Re: gitk problems: can't unset "idinlist(...)"
From: Jeff King @ 2007-07-21 6:18 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List, Paul Mackerras, Junio C Hamano
In-Reply-To: <alpine.LFD.0.999.0707202304350.27249@woody.linux-foundation.org>
On Fri, Jul 20, 2007 at 11:11:19PM -0700, Linus Torvalds wrote:
> Here's a patch. Not very well tested, but it makes gitk happy and passes
> all the tests. And I really think Junio's code was very dubious (it did
> that "p = p->next" *every* time, and then did "pp = &p->next", so "pp"
> would end up jumping by multiple entries.
I just independently came to the same conclusion. Your patch looks
correct to me (though there are some unrelated formatting changes which
made it a little harder to read).
Acked-by: Jeff King <peff@peff.net>
-Peff
^ permalink raw reply
* Re: gitk problems: can't unset "idinlist(...)"
From: Linus Torvalds @ 2007-07-21 6:11 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Paul Mackerras, Junio C Hamano
In-Reply-To: <alpine.LFD.0.999.0707202226130.27249@woody.linux-foundation.org>
On Fri, 20 Jul 2007, Linus Torvalds wrote:
>
> Junio, didn't we have some parent simplification patches recently?
Yeah. Junio, I think your 11d6596709e04b8d2b429f230b2ed570d013f812 is
buggy.
Here's a patch. Not very well tested, but it makes gitk happy and passes
all the tests. And I really think Junio's code was very dubious (it did
that "p = p->next" *every* time, and then did "pp = &p->next", so "pp"
would end up jumping by multiple entries.
The new code only ever changes "pp" - either by moving p->next into it
(delete the current entry) or by moving pp forward by one (keep the
current entry).
That's much more logical, but somebody should double-check me anyway.
Linus
---
Subject: Fix up duplicate parents removal
This removes duplicate parents properly, making gitk happy again.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
revision.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/revision.c b/revision.c
index 28b5f2e..7036cf2 100644
--- a/revision.c
+++ b/revision.c
@@ -1323,16 +1323,17 @@ static enum rewrite_result rewrite_one(struct rev_info *revs, struct commit **pp
static void remove_duplicate_parents(struct commit *commit)
{
- struct commit_list *p;
- struct commit_list **pp = &commit->parents;
+ struct commit_list **pp, *p;
/* Examine existing parents while marking ones we have seen... */
- for (p = commit->parents; p; p = p->next) {
+ pp = &commit->parents;
+ while ((p = *pp) != NULL) {
struct commit *parent = p->item;
- if (parent->object.flags & TMP_MARK)
+ if (parent->object.flags & TMP_MARK) {
+ *pp = p->next;
continue;
+ }
parent->object.flags |= TMP_MARK;
- *pp = p;
pp = &p->next;
}
/* ... and clear the temporary mark */
^ permalink raw reply related
* Re: gitk problems: can't unset "idinlist(...)"
From: Jeff King @ 2007-07-21 6:06 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List, Paul Mackerras, Junio C Hamano
In-Reply-To: <20070721055918.GA22399@coredump.intra.peff.net>
On Sat, Jul 21, 2007 at 01:59:18AM -0400, Jeff King wrote:
> v1.5.2 works, so I am bisecting.
Hmm. The bad commit is 1ed84157. The commit message is a very suspicious
"Revert 88494423 (removal of duplicate parents in the output codepath)."
-Peff
^ permalink raw reply
* Re: gitk problems: can't unset "idinlist(...)"
From: Jeff King @ 2007-07-21 5:59 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List, Paul Mackerras, Junio C Hamano
In-Reply-To: <alpine.LFD.0.999.0707202226130.27249@woody.linux-foundation.org>
On Fri, Jul 20, 2007 at 10:53:10PM -0700, Linus Torvalds wrote:
> With "arch/i386", I get the 7ed409 one, with "arch/i386 arch/x86_64" I get
> a complaint about 57d1c91fa. In neither case do I see the reason.
Oops, I thought you were using a different command line.
> git log --topo-order --parents --pretty=one arch/i386/ arch/x86_64/
>
> and search for it, it first shows up as a parent for commit 9fdb62af92:
> and not just _one_ parent, but three! So it looks like the parent
> simplification is broken, and maybe gitk is unhappy for that reason.
OK, that should make it easier to find (note also that 9fdb62af92 is an
octopus, which might be related).
> Junio, didn't we have some parent simplification patches recently?
v1.5.2 works, so I am bisecting.
-Peff
^ permalink raw reply
* Re: gitk problems: can't unset "idinlist(...)"
From: Junio C Hamano @ 2007-07-21 5:56 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Git Mailing List, Paul Mackerras
In-Reply-To: <alpine.LFD.0.999.0707202226130.27249@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> It seems to depend on the path limiter.
>
> With "arch/i386", I get the 7ed409 one, with "arch/i386 arch/x86_64" I get
> a complaint about 57d1c91fa. In neither case do I see the reason.
>
> But I think I found a clue. One thing special about that 57d1c91fa is that
> when you do
>
> git log --topo-order --parents --pretty=one arch/i386/ arch/x86_64/
>
> and search for it, it first shows up as a parent for commit 9fdb62af92:
> and not just _one_ parent, but three! So it looks like the parent
> simplification is broken, and maybe gitk is unhappy for that reason.
Yeah, I think if you limit the range at the bottom
(e.g. v2.6.21..) the problem disappears.
> The same is true of 7ed40918a38, btw, so yeah, I think that's it.
>
> Junio, didn't we have some parent simplification patches recently?
We had a patch to remove duplicated parent or something, I
think.
^ permalink raw reply
* Re: gitk problems: can't unset "idinlist(...)"
From: Linus Torvalds @ 2007-07-21 5:53 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Paul Mackerras, Junio C Hamano
In-Reply-To: <20070721050912.GB20622@coredump.intra.peff.net>
On Sat, 21 Jul 2007, Jeff King wrote:
>
> Interestingly, I repeatably get the exact same error but with a
> different commit:
>
> 7ed40918a386afc2e14a6d3da563ea6d13686c25
>
> which looks like a totally uninteresting commit.
It seems to depend on the path limiter.
With "arch/i386", I get the 7ed409 one, with "arch/i386 arch/x86_64" I get
a complaint about 57d1c91fa. In neither case do I see the reason.
But I think I found a clue. One thing special about that 57d1c91fa is that
when you do
git log --topo-order --parents --pretty=one arch/i386/ arch/x86_64/
and search for it, it first shows up as a parent for commit 9fdb62af92:
and not just _one_ parent, but three! So it looks like the parent
simplification is broken, and maybe gitk is unhappy for that reason.
The same is true of 7ed40918a38, btw, so yeah, I think that's it.
Junio, didn't we have some parent simplification patches recently?
Linus
^ permalink raw reply
* Re: [RFC PATCH] Re: Empty directories...
From: David Kastrup @ 2007-07-21 5:28 UTC (permalink / raw)
To: git
In-Reply-To: <alpine.LFD.0.999.0707202154220.27249@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Fri, 20 Jul 2007, Linus Torvalds wrote:
>>
>> As far as I can tell, it would have been exactly the same thing as the
>> S_IFDIR, just instead of the S_IFDIR check, you'd have had to check the
>> end of the filename for being '/'.
>
> BTW, there is actually one big difference, and the '/' at the end actually
> has one huge advantage.
>
> Why? Because my preliminary patches sort the index entries wrong. A
> directory should always sort *as*if* it had the '/' at the end.
Hm, that's bad. The thing is that the directory names I am tracking
are called "." (that's what I was currently trying to reconcile your
code with).
> And I *should* have done it that way, but I never did. It now makes
> the S_ISDIR handling harder, because directories really do have to
> be sorted as if they had the '/' at the end, or "git-fsck" will
> complain about bad sorting.
Hm, I'll have to check what git-fsck does.
> Of course, it seldom matters, but basically, you should test a directory
> structure that has the files
>
> dir.c
> dir/test
>
> in it, and the "dir" directory should always sort _after_ "dir.c".
>
> And yes, having the index entry with a '/' at the end would handle
> that automatically.
You completely lost me here. I guess I'll be able to pick this up
only after investing considerable more time into the data structures.
And I have to goto bed right now.
> As it is, with the "mode" difference, it instead needs to fix up
> "cache_name_compare()". Admittedly, that would actually be a cleanup
> (since it would now match base_name_compare() in logic, and could
> actually use that to do the name comparison!), but it's a damn
> painful cleanup because we don't even pass in the mode to
> "cache_name_compare()", since we never needed it.
>
> Gaah.
>
> cache_name_compare itself isn't used in that many places, but it's
> used by "index_name_pos()/cache_name_pos()", which *is* used in many
> places. And again, that one doesn't even have the mode, so it
> cannot pass it down.
>
> So it probably *is* easier to add the '/' at the end of the name instead,
> to make directories sort the right way in the index. I'd still suggest you
> *also* make the mode be S_IFDIR, though (and preferably make git-fsck
> actually verify that the mode and the last character of the name
> matches!).
The _flattened_ directory name would end in /. in my scheme. I would
not want to use "xxx/" for a directory name, and "xxx" for a tree:
that would be completely backwards. And I also don't like the
duplication of xxx when listing objects.
Sure, that's an implementation detail, but I don't like
implementations hurting my eyes...
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Re: [RFC PATCH] Re: Empty directories...
From: David Kastrup @ 2007-07-21 5:15 UTC (permalink / raw)
To: git
In-Reply-To: <?= =?ISO-8859-1?Q?alpine.LFD.0.999?= =?ISO-8859-1?Q?.07072=0402135450.?= =?ISO-8859-1?Q?27249@woody.linu?= =?ISO-8859-1?Q?x-foundation.org?= =?ISO-8859-1?Q?>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Sat, 21 Jul 2007, David Kastrup wrote:
>>
>> Ok, I have now acquired enough passing familiarity with the code
>> that I find part of my way around it. Most of your patch looks
>> like it caters for the S_ISDIR type not previously in use in the
>> index (how about the repository?).
>
> The object database has always had S_ISDIR (well, "always" is since
> very early on, when I realized that flat trees didn't cut it).
Then I think I have a bit of a problem: I should think that S_ISDIR in
the repository presumably marks a tree object (still very fuzzy around
the concepts here). An explicitly checked-in directory (under my
scheme always named "." inside of its tree) would presumably also have
S_ISDIR in the repository but behave quite differently.
> As far as I can tell, it would have been exactly the same thing as the
> S_IFDIR, just instead of the S_IFDIR check, you'd have had to check the
> end of the filename for being '/'.
Relative file name of ".", more or less. Both names satisfy S_IFDIR
in the filesystem, though.
> Otherwise? Exactly the same.
> The more important thing is in many ways the object storage, and
> that's also the reason for doing the index the way I did - it more
> closely matches what the object storage does (ie the "index" ends up
> mirroring a linearized and unpacked "tree" object).
I still have to get enough of a clue about the object store to see how
this pans out. I would not want to have the "." objects marked as
type "tree" and empty if I can avoid it. It seems unclean, would need
extra case separations all over the place, violate the "empty trees
evaporate" property and also waste a good place for tracking
permissions or other attributes in future.
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Re: gitk problems: can't unset "idinlist(...)"
From: Jeff King @ 2007-07-21 5:09 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List, Paul Mackerras, Junio C Hamano
In-Reply-To: <alpine.LFD.0.999.0707201554540.27249@woody.linux-foundation.org>
On Fri, Jul 20, 2007 at 04:05:54PM -0700, Linus Torvalds wrote:
> can't unset "idinlist(57d1c91fa6d9146b309b7511f6432dea2a24858b)": no such element in array
> [...]
> I'm not seeing anything interesting or special about that named commit, or
> anything else that would make gitk unhappy. But it is.
Interestingly, I repeatably get the exact same error but with a
different commit:
7ed40918a386afc2e14a6d3da563ea6d13686c25
which looks like a totally uninteresting commit.
I would expect, given that our repositories are at presumably the same
state, that we would get the same error at the same spot, but we don't.
Which implies to me there is some randomness, except that I get the
_same_ commit every time (and presumably so do you).
My master (and all other branches) are at c2e6805.
-Peff
^ permalink raw reply
* Re: [RFC PATCH] Re: Empty directories...
From: Linus Torvalds @ 2007-07-21 5:08 UTC (permalink / raw)
To: David Kastrup; +Cc: git
In-Reply-To: <alpine.LFD.0.999.0707202135450.27249@woody.linux-foundation.org>
On Fri, 20 Jul 2007, Linus Torvalds wrote:
>
> As far as I can tell, it would have been exactly the same thing as the
> S_IFDIR, just instead of the S_IFDIR check, you'd have had to check the
> end of the filename for being '/'.
BTW, there is actually one big difference, and the '/' at the end actually
has one huge advantage.
Why? Because my preliminary patches sort the index entries wrong. A
directory should always sort *as*if* it had the '/' at the end.
See base_name_compare() for details.
And we've never done that for the index, because the index has never had
this issue (since it never contained directories). So sit down and compare
base_name_compare (for tree entries) with cache_name_compare() (for index
entries), and see how the latter doesn't care about the type of names.
This was actually something that I hit already with subproject support,
and one of my very first patches even had some (aborted) code to start
sorting subprojects in the index the way we sort directories.
And I *should* have done it that way, but I never did. It now makes the
S_ISDIR handling harder, because directories really do have to be sorted
as if they had the '/' at the end, or "git-fsck" will complain about bad
sorting.
Sad, sad, sad. It effectively means that S_IFGITLINK is *not* quite the
same as S_IFDIR, because they sort differently. Duh.
Of course, it seldom matters, but basically, you should test a directory
structure that has the files
dir.c
dir/test
in it, and the "dir" directory should always sort _after_ "dir.c".
And yes, having the index entry with a '/' at the end would handle that
automatically.
As it is, with the "mode" difference, it instead needs to fix up
"cache_name_compare()". Admittedly, that would actually be a cleanup
(since it would now match base_name_compare() in logic, and could actually
use that to do the name comparison!), but it's a damn painful cleanup
because we don't even pass in the mode to "cache_name_compare()", since we
never needed it.
Gaah.
cache_name_compare itself isn't used in that many places, but it's used
by "index_name_pos()/cache_name_pos()", which *is* used in many places.
And again, that one doesn't even have the mode, so it cannot pass it down.
So it probably *is* easier to add the '/' at the end of the name instead,
to make directories sort the right way in the index. I'd still suggest you
*also* make the mode be S_IFDIR, though (and preferably make git-fsck
actually verify that the mode and the last character of the name
matches!).
Linus
^ permalink raw reply
* Re: [RFC PATCH] Re: Empty directories...
From: Linus Torvalds @ 2007-07-21 4:51 UTC (permalink / raw)
To: David Kastrup; +Cc: git
In-Reply-To: <851wf2bcqy.fsf@lola.goethe.zz>
On Sat, 21 Jul 2007, David Kastrup wrote:
>
> Ok, I have now acquired enough passing familiarity with the code that
> I find part of my way around it. Most of your patch looks like it
> caters for the S_ISDIR type not previously in use in the index (how
> about the repository?).
The object database has always had S_ISDIR (well, "always" is since very
early on, when I realized that flat trees didn't cut it).
> The disadvantage is that it introduces a new data type and thus one
> has to check all the code paths to see how older versions of git will
> cater with newer data.
Take a look at the "subproject" patches - those did the same (adding the
ntion of a gitlink to the index), except those also changed how the tree
object looked, since now a tree could contain pointers to commits too.
> My idea of a fake zero-length file would have had predictable side
> effects:
As far as I can tell, it would have been exactly the same thing as the
S_IFDIR, just instead of the S_IFDIR check, you'd have had to check the
end of the filename for being '/'.
Otherwise? Exactly the same.
Except for the fact that we already supported S_IFGITLINK for subprojects
(and there it matches the "struct tree" entry, so it really *does* make
more sense that way), so supporting S_IFDIR was actually easier.
But hey, that's an implementation detail. I don't actually care all that
much. In many ways, the "long-term" data structures are much more
important than the index, the index is a purely temporary - and even more
importantly - a purely local datastructure.
The more important thing is in many ways the object storage, and that's
also the reason for doing the index the way I did - it more closely
matches what the object storage does (ie the "index" ends up mirroring a
linearized and unpacked "tree" object).
Linus
^ permalink raw reply
* Re: [RFC PATCH] Re: Empty directories...
From: David Kastrup @ 2007-07-21 4:29 UTC (permalink / raw)
To: git
In-Reply-To: <alpine.LFD.0.999.0707181557270.27353@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> This really updates three different areas, which are nicely
> separated into three different files, so while it's one single
> patch, you can actually follow along the changes by just looking at
> the differences in each file, which directly translate to separate
> conceptual changes:
Ok, I have now acquired enough passing familiarity with the code that
I find part of my way around it. Most of your patch looks like it
caters for the S_ISDIR type not previously in use in the index (how
about the repository?). So that makes for quite a bit of nicer looks.
The disadvantage is that it introduces a new data type and thus one
has to check all the code paths to see how older versions of git will
cater with newer data.
My idea of a fake zero-length file would have had predictable side
effects:
For checking out, git would have created the directory it needed to
place the "file", then try to write an empty file called "." and
failing. Apart from an error message (if we aren't root on Solaris),
this would have worked exactly as intended.
For deletion on checking out, git would have tried deleting "." and
failed. I have not checked the code to see whether git takes this as
a clue not to attempt deleting the containing directory. If not,
again stuff would have worked as intended. If yes, well, the user
needs to clean up manually.
I am not sure what code paths are executed when using S_ISDIR now in
unmodified git. As a theoretical question for now: do git
repositories carry some versioning inside them? Something like "don't
touch me if you are not at least version x"?
Anyway, the code becomes quite less of a dirty hack by using that data
type, so I am pretty much taking your code (which has no overlap to
the work I have done already) as is. Seems like it should play
together quite nicely with my own stuff.
So thanks for doing the heavy lifting in a difficult area.
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Re: [RFC PATCH] Re: Empty directories...
From: David Kastrup @ 2007-07-21 3:54 UTC (permalink / raw)
To: git
In-Reply-To: <85hcnytuq8.fsf@lola.goethe.zz>
David Kastrup <dak@gnu.org> writes:
> The only difference is that I am calling the file ".". Which is in
> _all_ respects nothing more than a naming convention.
>
> However, this convention has distinct advantages over ".notignore":
>
> a) I don't have to depart as far from reality. Whenever I try
> registering ".", I can rely on the work directory actually _having_
> "." as a _real_, not a pseudofile. It will not actually be a
> _regular_ file as I'll tell git: that's a wart of my prototype
> implementation which will, no doubt, eventually be fixed by others
> _if_ the code does its job fine apart from being ugly to look at.
Update: well, I am still digging through the code, but this is all so
well factored that it might be perfectly feasible to have S_ISDIR
entries after all without too much of a hassle.
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* git-gui i18n repo on repo.or.cz
From: Johannes Schindelin @ 2007-07-21 2:12 UTC (permalink / raw)
To: Christian Stimming, Shawn O. Pearce, Paolo Ciarrocchi,
Sam Ravnborg, Xudong Guan, Brett Schwarz
Cc: git
Hi people,
I proudly present...
http://repo.or.cz/w/git-gui/git-gui-i18n.git
It is meant to be rebased to git-gui's master pretty often. It has also a
mob branch where you can push your new changes into.
For those who do not know what a mob branch is: By using the URL
mob@repo.or.cz:/srv/git/git-gui/git-gui-i18n.git, without password,
_everybody_ can push into the branch named "mob" (but _only_ into that).
This facilitates participation without the need of a password, or commit
permissions, since it is mean to be pulled/cherry-picked by the repo
maintainer, who can decide what is good and what is bad. For the moment,
the maintainer is yours truly. If somebody volunteers, I am certainly not
sad.
I would like to ask people to not push with "+" into the mob branch, but
pull it instead, so that we do not lose any valuable data. It is probably
a good idea to send an email to the repo maintainer, too.
For the time being, I will rebase the master branch relatively often,
and try to stay on top of the mob branch.
At the moment, the repo contains three branches: master (which I would
like to be rebased onto git-gui's master quite frequently),
christian-orig, the patch series which Christian sent, and which branched
off git-gui's master roughly 8 days ago, and the mob branch.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] Internationalization of git-gui
From: Shawn O. Pearce @ 2007-07-21 2:17 UTC (permalink / raw)
To: Christian Stimming; +Cc: Brett Schwarz, git, Paul Mackerras
In-Reply-To: <20070720105602.7dcm241ts0k0ww88@webmail.tu-harburg.de>
Christian Stimming <stimming@tuhh.de> wrote:
> I used (and prefer) "_" because that's the standard function name for
> i18n'd strings when using gettext (talking about a "standard" way).
I thought about this today. I almost want to use _, e.g:
proc _ {args} {
return [eval mc $args]
}
For the translation, but I don't think its worth the CPU cycles in
Tcl to eval mc via _ every time we need a string when it only is
saving us one keystroke on a function name, *and* we are breaking
tradition with Tcl.
So when in Rome, wear a toga. Or in this case, use [mc ...].
> Being a newcomer on this list, could you please explain to me how to
> proceed with the i18n patches so far?
Sure.
> Do you want to have patches
> submitted after some further changes (which ones?)
Yes. Here's a few to get started with and that are really obvious.
Some I'm just asking for more information on.
- Import msgcat::mc and use [mc] instead of [_].
- Please combine the second and third patches into a single change.
There is no reason to switch to [mc {}] only to switch to [mc ""].
- Please use mc's formatting support, rather than [format].
Its shorter code.
- Don't bother trying to translate the strings "Tools" (for the
Tools menu) or "Migrate" (for its only menu option). This block
of code doesn't even belong in git-gui. Its for my day-job and
is a custom hack that I need to strip out and carry as a local
patch there, rather than in the public distribution.
- In our Makefile we do the looping in GNU make using its
$(foreach) operator, rather than using the shell's for builtin.
In other words, can we have the catalog target look more like the
install target?
- Can ALL_LINGUAS be automatically built from the directory
contents of the po/ directory?
- Can we define a dist rule for the maintainer to build the catalog
files, so the maintainer can convert the .po -> .msg for Tcl and
the user doesn't need the GNU tools installed to build git-gui?
> and/or in different
> formats?
Please send one patch per email message, inline and not attached.
This way they are easy to review, respond to and comment on.
> Do you prefer to have all changes in a smaller number of
> commit rather than split the way I did before?
No, this series looks reasonably fine to me structurally.
Did you base the patches on git.git's git-gui/ subdirectory, or
did you base them on the git-gui.git repository? Technically all
patches for git-gui should be against the git-gui repository on
repo.or.cz, as git-gui is its own project. Periodic stable snapshots
are imported into git.git under the git-gui/ subdirectory, for the
ease of distribution with core git.
Dscho recently created a fork of git-gui.git here:
http://repo.or.cz/w/git-gui/git-gui-i18n.git
and added your patch series into it. But I'd like to see some
cleanups before it merges in, and I want to hold off on actually
applying it into git-gui 0.8.0 is released, which should be Real
Soon Now as I'm trying to make it into git 1.5.3, which is coming
Even Sooner Than I'd Hoped. ;-)
> Should I wait for some
> more days/weeks/whatever until you or particular other developers have
> reviewed the patches? Thanks.
I think we're settled on using [mc]. I'm fine with the *.po ->
*.msg thing, especially if the maintainer can produce them and
package the *.msg files in the release tarball, so that the enduser
doesn't need to worry about msgfmt working.
--
Shawn.
^ permalink raw reply
* Re: [RFC PATCH] Re: Empty directories...
From: David Kastrup @ 2007-07-21 1:23 UTC (permalink / raw)
To: git
In-Reply-To: <alpine.LFD.0.999.0707201712150.27249@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> The really sad part about this discussion is that the ".gitignore
> trick" is really technically no different at all from the one that
> David Kastrup has been advocating a few times, except he calls his
> ".gitignore" just ".", and seems to think that it's somehow
> different.
Oh no, I don't think at all that it is somehow different: actually
this is _exactly_ the reason why I think that the implementation will
be doable even by an idiot like myself, and that is because at least
in my first iteration, "." will appear as an empty regular file to
git, just like ".gitignore". The main worry I had was that putting
"." inside of a gitignore entry might stop "git add ." from working
like previously. But I tried it, and it works just like it would with
".gitignore". Or rather like it would with ".notignore" since
".gitignore" _is_ specially treated by git, after all.
> It is true that ".gitignore" and "." _are_ different.
>
> But they are actually different in the sense that the ".gitignore"
> thing is something you can control, while the "." thing is something
> that is in all directories on UNIX, which is exactly why it
> _must_not_ be used by git to mark existence.
But I don't plan to have it used by git to mark existence. The
_existence_ can be taken for granted. But what can't be taken for
granted, like with any other file, is that the file is actually being
tracked by git. To have it tracked, you need to add it, and it must
not be covered by gitignore.
> Exactly because it has thus lost its ability to be something you can
> tune per-directory in the working tree!
But it should not let the user lose his ability to let or let not git
track the file.
> That said, I actually like my patch, because the git tree structures
> actually lend themselves very naturally to the "empty tree", and I
> know people have even built up those kinds of trees on purpose, even
> if the index doesn't support that notion.
And that is the reason I will be working with the "empty file ."
metaphor: it would be way above my head to make the index support new
file types or even structures, and change the evaporate-when-empty
semantics of trees and so on, while catching all special cases.
I have no chance in hell to implement a new feature with a reasonable
amount of time and work. That's a task for people with a larger brain
than mine who have my full admiration and respect. The best I can
hope to achieve is a clever hack.
And if that works, people can still pile exceptions on it and redo it
as a "proper feature".
You are _perfectly_ correct that my proposal is _not_ a jot different
from registering a regular empty file ".notignore", and it is on
_purpose_, because I could not handle the complications if it were.
The only difference is that I am calling the file ".". Which is in
_all_ respects nothing more than a naming convention.
However, this convention has distinct advantages over ".notignore":
a) I don't have to depart as far from reality. Whenever I try
registering ".", I can rely on the work directory actually _having_
"." as a _real_, not a pseudofile. It will not actually be a
_regular_ file as I'll tell git: that's a wart of my prototype
implementation which will, no doubt, eventually be fixed by others
_if_ the code does its job fine apart from being ugly to look at. It
may not be even necessary internally to think of "." other than as an
empty regular file, but git should probably not talk too loud about it
lest people laugh at it.
b) it already means something to people. Now this is a two-edged
sword, since "almost, but not quite, entirely unlike" concepts are not
necessarily helpful in computing. In this case, however, I think the
match is close enough to help people understand what is going on
rather than the other way round. "." was introduced because people
wanted to have a good way to refer to a directory as an element of
itself. So using "." as a self-reference for a directory is quite in
the spirit of that name.
> So in that sense, teaching the index about an empty tree is in some
> ways the "right thing" to do, if only because it means that the
> index can finally express something that the tree objects themselves
> have always been able to validly encode.
If you define the tree objects by the physical in-memory or
in-repository data structures encoding them, then you are correct. I
am somewhat reluctant to parade around another red cape, but in this
particular case, the size of the wet spot in my pants does not as much
relate to the physical layout of the data structure (big deal,
probably 30 lines of code all around), but rather to the extent and
assumptions of functions accessing it. Namely, data layout and
accessor functions _together_ constitute a tree object. So for me the
"evaporate-when-empty" property, while not inherent in the physical
layout of the object, is still an inherent part of its structure which
I would not want to touch: finding and fixing and debugging all code
elements which explicitly or implicitly rely on that assumption is
something I would not entrust myself with.
I might have been more inclined to dabble with that approach if the
tree stuff were written in something more object-oriented, say, clean
and concise C++, except that clean and concise C++ code in the wild is
even more of a mythical beast than clean and concise TeX code, and C++
itself is such a mindboggingly complex contraption... I digress.
All the best,
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Re: GSoC status report
From: Johannes Schindelin @ 2007-07-21 1:24 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <200707210312.11745.jnareb@gmail.com>
Hi,
On Sat, 21 Jul 2007, Jakub Narebski wrote:
> Johannes Schindelin wrote:
>
> > FWIW this is the list of scripts that I would like to see converted by the
> > end of the year (feel free to object), ordered by their size:
> >
> > verify-tag, reset, repack, tag, checkout, rebase, bisect,
> > rebase--interactive, am, commit.
>
> I thought that verify-tag would be put into git-tag, with git-verify-tag
> being alias to "git tag --verify" or something like that.
>
> What about git-revert and git-cherry-pick (it is one script)? Or are you
> simply concentrating on the tools you need most in common workflow?
They are builtins since v1.5.1~170^2~2.
Hth,
Dscho
^ permalink raw reply
* Re: GSoC status report
From: Jakub Narebski @ 2007-07-21 1:12 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0707201118550.14781@racer.site>
Johannes Schindelin wrote:
> FWIW this is the list of scripts that I would like to see converted by the
> end of the year (feel free to object), ordered by their size:
>
> verify-tag, reset, repack, tag, checkout, rebase, bisect,
> rebase--interactive, am, commit.
I thought that verify-tag would be put into git-tag, with git-verify-tag
being alias to "git tag --verify" or something like that.
What about git-revert and git-cherry-pick (it is one script)? Or are you
simply concentrating on the tools you need most in common workflow?
Thanks for the reply
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [RFC PATCH] Re: Empty directories...
From: Linus Torvalds @ 2007-07-21 0:18 UTC (permalink / raw)
To: Julian Phillips; +Cc: Johan Herland, git
In-Reply-To: <Pine.LNX.4.64.0707202320300.16498@reaper.quantumfyre.co.uk>
On Fri, 20 Jul 2007, Julian Phillips wrote:
> On Fri, 20 Jul 2007, Linus Torvalds wrote:
> >
> > So here's my standpoint:
> >
> > - people who use git natively might as well use the ".gitignore" trick.
> > It really *does* work, and there really aren't any downsides. Those
> > directories will stay around forever, until you decide that you don't
> > want them any more. Problem solved.
>
> Personally I quite like this approach - I'm going to use it to keep all the
> empty directories from Subversion in my importer. It seems to address
> everthing quite neatly.
The really sad part about this discussion is that the ".gitignore trick"
is really technically no different at all from the one that David Kastrup
has been advocating a few times, except he calls his ".gitignore" just
".", and seems to think that it's somehow different.
It is true that ".gitignore" and "." _are_ different.
But they are actually different in the sense that the ".gitignore" thing
is something you can control, while the "." thing is something that is in
all directories on UNIX, which is exactly why it _must_not_ be used by git
to mark existence. Exactly because it has thus lost its ability to be
something you can tune per-directory in the working tree!
That said, I actually like my patch, because the git tree structures
actually lend themselves very naturally to the "empty tree", and I know
people have even built up those kinds of trees on purpose, even if the
index doesn't support that notion.
So in that sense, teaching the index about an empty tree is in some ways
the "right thing" to do, if only because it means that the index can
finally express something that the tree objects themselves have always
been able to validly encode.
Linus
^ permalink raw reply
* gitk problems: can't unset "idinlist(...)"
From: Linus Torvalds @ 2007-07-20 23:05 UTC (permalink / raw)
To: Git Mailing List, Paul Mackerras; +Cc: Junio C Hamano
Ok, color me stumped, but gitk is unhappy.
Ingo and Thomas have just announced that they have a unified x86 tree for
both i386/x86-64, so I did
gitk arch/i386 arch/x86_64
to look at what has happened recently in the kernel in that area.
I get the gitk history view, but no actual diffs, and I get an application
error window saying
can't unset "idinlist(57d1c91fa6d9146b309b7511f6432dea2a24858b)": no such element in array
can't unset "idinlist(57d1c91fa6d9146b309b7511f6432dea2a24858b)": no such element in array
while executing
"unset idinlist($id)"
(procedure "layouttail" line 11)
invoked from within
"layouttail"
(procedure "layoutmore" line 35)
invoked from within
"layoutmore $tlimit $allread"
(procedure "chewcommits" line 9)
invoked from within
"chewcommits 1"
("eval" body line 1)
invoked from within
"eval $script"
(procedure "dorunq" line 9)
invoked from within
"dorunq"
("after" script)
which really doesn't tell me much.
It doesn't seem to happen without path limiting, and it also doesn't seem
to happen for all paths (doing "gitk drivers/scsi" doesn't show it), but
somehow that "arch/i386" part seems to bring it out.
I'm not seeing anything interesting or special about that named commit, or
anything else that would make gitk unhappy. But it is.
Current kernel tree. Anybody else has any ideas? Somebody who knows
tcl/tk, and gitk?
Linus
^ permalink raw reply
* Re: [RFC PATCH] Re: Empty directories...
From: Julian Phillips @ 2007-07-20 22:36 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Johan Herland, git
In-Reply-To: <alpine.LFD.0.999.0707201421110.27249@woody.linux-foundation.org>
On Fri, 20 Jul 2007, Linus Torvalds wrote:
>
>
> On Fri, 20 Jul 2007, Johan Herland wrote:
>>
>> Does this mean that you are firmly opposed to the concept of storing
>> directories in the index/tree as such, or that you are only opposed to
>> (some of) the implementation ideas that have been discussed so far?
>
> I've already sent out a *patch* to do so, for chissake. It handled all
> these cases perfectly fine, as far as I know, but I didn't test it all
> that deeply (and made it clear when I sent that patch out).
>
> In fact, in this whole pointless discussion, I think I'm so far the only
> one to have done anything constructive at all. Sad.
There was Dscho's .gitignore based patch too ...
>
> So here's my standpoint:
>
> - people who use git natively might as well use the ".gitignore" trick.
> It really *does* work, and there really aren't any downsides. Those
> directories will stay around forever, until you decide that you don't
> want them any more. Problem solved.
>
> Sure, if you export the git archive into some other format, you might
> well want to do something about the ".gitignore" files (like just
> delete them, since they won't be meaningful in an SVN environment, for
> example, but you might also just convert them into SVN's "attributes"
> or whatever it is that SVN uses to ignore files).
Personally I quite like this approach - I'm going to use it to keep all
the empty directories from Subversion in my importer. It seems to address
everthing quite neatly.
I don't really understand the objections ... especially since I can't see
why you want an empty directory if you're not going to put _something_ in
it - in which case, presumably you want to ignore it (so maybe a
.gitignore containing * would be better than an empty one)? However, I'm
sure that if people want it, they have a reason.
> SCM is too important to play games with. Git gets things right, and I
> doubt people really _realize_ that the "tracks content" is why git is so
> much better, and why git can do merges so much faster and more reliably
> than anybody else.
This is the thing that made me interested in git back in April '05. I
couldn't see what we were going to end up with at that point - but I was
_convinced_ that due to the underlying design it was worth watching.
Being a python type (sorry ... :$) hg looked interesting when it sprang up
- but they threw away what I considered to be one of the most compelling
features of git (at the time there wasn't the wealth of really nice tools
that we now have).
In fact, I really should say "Thank you Linus", since I came that close to
writing an SCM from scratch myself - having been using Subversion with
branches for quite some time (and CVS before that - and yes I do mean
branches + CVS). Now I no longer feel the need to write an SCM - just a
longing to use git. git is probably better than anything I would have
come up with too. :D
--
Julian
---
She is descended from a long line that her mother listened to.
-- Gypsy Rose Lee
^ 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