* Re: merging .gitignore
From: Johan Herland @ 2007-10-03 9:28 UTC (permalink / raw)
To: git; +Cc: Andy Parkins, martin f krafft, Johannes Schindelin
In-Reply-To: <200710030923.22767.andyparkins@gmail.com>
On Wednesday 03 October 2007, Andy Parkins wrote:
> I am still having difficult seeing why you want to hide conflicts
> in .gitignore. It's just as possible to get and resolve conflicts in
> gitignore as in any other file.
Agreed. What about the following:
- No special merge rules for .gitignore
- Teach the .gitignore parser to ignore conflict markers (i.e. regard them
as comments)
This way, the user will have to merge .gitignore like any other file, but if
for some reason, the user is not able to do so (say, git needs to
consult .gitignore before the user has had a chance to resolve the
conflict), the fallback result will be similar to a union merge, which
shouldn't be extremely harmful in .gitignore's case.
I do not think we really want to create an auto-merge algorithm
for .gitignore, and then depend on this auto-merge algorithm to _always_
succeed with the _correct_ result and _no_ conflicts. These algorithms tend
to be extremely tricky to get right, especially for the "always correct"
requirement.
<rant>
Mercurial had a similar problem some months ago. They have their tag
definitions stored in a versioned file in the working tree (.hgtags IIRC).
But the repo tag state (i.e. Mercurial's opinion at any time as to _which_
tags are defined and _where_ they point) is not deduced from the copy in
your current working tree at all. (That would of course limit you to only
ba able to refer to tags defined earlier on the particular branch you're
currently on.) Instead the repo tag state was found by consulting
the "head-most" copy of the .hgtags file (for some definition
of "head-most" including non-obvious things like which branch has the most
recent commit, etc). The end result was that you could get some _really_
interesting behaviour depending on the order in which you commited totally
unrelated changes to two different branches which happened to have
different .hgtags files. (E.g.: Given two branches A, B, and - in .hgtags
on branch A - tag Foo is defined to point at rev X, and - in .hgtags on
branch B - Foo points at rev Y. Now, whether you got rev X or rev Y when
you checked out Foo, depended on which of branch A or branch B had the most
recent (totally unrelated, i.e. not even touching .hgtags) commit.)
I (and others) pointed out this bug on their ML, and instead of fixing the
braindeadness of allowing branched tag definitions in the repo in the first
place, they set about trying to create an auto-merge algorithm for deducing
the repo tag state from the various versions/branches of .hgtags found in
the repo. I didn't stick around for long enough to see how well this
auto-merge algorithm works (the misdesign of tags in Mercurial was one of
the reasons I started looking at git), but I would be surprised if
Mercurial today has a simple and straightforward way of deducing the repo
tag state that _always_ gives _correct_ (i.e. unsurprising) results, even
in the corner cases.
</rant>
Have fun! :)
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: git push (mis ?)behavior
From: Pierre Habouzit @ 2007-10-03 9:03 UTC (permalink / raw)
To: Miles Bader; +Cc: Junio C Hamano, git
In-Reply-To: <buobqbgmv6z.fsf@dhapc248.dev.necel.com>
[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]
On Wed, Oct 03, 2007 at 08:57:56AM +0000, Miles Bader wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> > There definitely is a point: with the current behaviour you sometimes
> > end up pushing more than what you meant, with sometimes WIP that you
> > intend to rebase, and it hurts. Git porcelains should help you avoid to
> > shoot yourself in the foot, hence I think that (especially to git
> > newcomers), the current default _is_ dangerous.
>
> What's "dangerous" for newbies, often ends up being what doesn't
> correspond with their mental model. I think the current default
> behavior without any <refspec> specified corresponds well to the
> operation of many other git commands (and unix command) in similar
> circumstances: If you don't specify something to operate on, it
> essentially uses a wild card and operates on "every reasonable thing"
> (e.g., consider "git commit FILE" versus "git commit").
Git commit is hardly a wildcard as it only operates on what you put in
your index, which is hardly something that happens behind your back.
> To the extent that a command _is_ "dangerous", there's always a tradeoff
> between convenience and "danger". Some systems (e.g. those aimed at
> newbies) might have as a goal to do the absolute minimum with every
> command and always, always, err on the side of safety. I don't think git
> is that system.
I beg to differ then. I believe that "git push" default behavior is
wrong. I'm not really a newbie, and it often did not do what I meant. So
it could also be that there isn't a sane default either. I just say the
current one can lead to gross mistakes.
I know that some porcelains are risky: if you rebase "under" a point
that was published you are shooting yourself in the foot e.g.. But
git-rebase _is_ a command that rewrites history. You're warned from the
first second you use it. But git-push is supposedly only a transport
command, not something that messes with remotes history behind your
back.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH] rebase: make the warning more useful when the work tree is unclean.
From: Matthieu Moy @ 2007-10-03 9:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Steffen Prohaska
In-Reply-To: <7vejgchijf.fsf_-_@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Instead of letting "update-index --refresh" report paths needing
> updates and merges, use git-status to give more useful output.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * I won't be a good judge of the updated behaviour, as I never
> start rebase in an unclear tree. Running git-status in a
> large tree may be too expensive to be worth changing the
> output.
I see your patch as an improvement too. status is a bit expansive, but
you hit this portion of code only when trying "rebase" by mistake, so
it's acceptable to let git take a bit of time to explain your mistake.
That said, I think it's still worth making the messages of
"update-index" a little more verbose and consistant with "status" (my
previous patch), since I think there are other occurences of
user-visible output of update-index in porcelain git.
Ideally, update-index would be a C function returning a struct
containing all the information about the status, with a function
is_clean(...) and another print_as_status(...), to allow the same
functionality with better performances, but I won't have time to do
this.
--
Matthieu
^ permalink raw reply
* Re: merging .gitignore
From: Pierre Habouzit @ 2007-10-03 8:58 UTC (permalink / raw)
To: martin f krafft; +Cc: git, Andy Parkins, Johannes Schindelin
In-Reply-To: <20071003084259.GA25838@lapse.madduck.net>
[-- Attachment #1: Type: text/plain, Size: 878 bytes --]
On Wed, Oct 03, 2007 at 08:42:59AM +0000, martin f krafft wrote:
> also sprach Pierre Habouzit <madcoder@debian.org> [2007.10.02.2307 +0100]:
> > (a*)
> > / \
> > v v
> > (ab*) (ac*)
> > \ /
> > v v
> > ????
> >
> > This is a perfectly sensible history. Or I miss sth on your end.
>
> So these are revs, not branches?
Yes, those are the contents of the .gitignores on refs, on top is the
common ancestor, and you have two branches that you want to merge into
one. I pretend that if in one branch the content of the .gitignore was
a* and becomes ab* and than on the other it was a* and became ac*, then
you cannot know how to merge.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: git push (mis ?)behavior
From: Miles Bader @ 2007-10-03 8:57 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Junio C Hamano, git
In-Reply-To: <20071003073554.GA8110@artemis.corp>
Pierre Habouzit <madcoder@debian.org> writes:
> There definitely is a point: with the current behaviour you sometimes
> end up pushing more than what you meant, with sometimes WIP that you
> intend to rebase, and it hurts. Git porcelains should help you avoid to
> shoot yourself in the foot, hence I think that (especially to git
> newcomers), the current default _is_ dangerous.
What's "dangerous" for newbies, often ends up being what doesn't
correspond with their mental model. I think the current default
behavior without any <refspec> specified corresponds well to the
operation of many other git commands (and unix command) in similar
circumstances: If you don't specify something to operate on, it
essentially uses a wild card and operates on "every reasonable thing"
(e.g., consider "git commit FILE" versus "git commit").
Even if the default were changed, it could very well end up causing many
problems because it _didn't_ push as many heads as the user thought it
would (I don't think I'm the only one that might expect the default
action to be "push everything"). When I was a git newbie, I sometimes
got into situations where I screwed something up because heads I thought
had been pushed to another system actually hadn't been.
To the extent that a command _is_ "dangerous", there's always a tradeoff
between convenience and "danger". Some systems (e.g. those aimed at
newbies) might have as a goal to do the absolute minimum with every
command and always, always, err on the side of safety. I don't think git
is that system.
-Miles
--
Yo mama's so fat when she gets on an elevator it HAS to go down.
^ permalink raw reply
* Re: merging .gitignore
From: martin f krafft @ 2007-10-03 8:42 UTC (permalink / raw)
To: git; +Cc: Pierre Habouzit, Andy Parkins, Johannes Schindelin
In-Reply-To: <20071002220749.GE19710@artemis.corp>
[-- Attachment #1: Type: text/plain, Size: 670 bytes --]
also sprach Pierre Habouzit <madcoder@debian.org> [2007.10.02.2307 +0100]:
> (a*)
> / \
> v v
> (ab*) (ac*)
> \ /
> v v
> ????
>
> This is a perfectly sensible history. Or I miss sth on your end.
So these are revs, not branches?
--
.''`. martin f. krafft <madduck@debian.org>
: :' : proud Debian developer, author, administrator, and user
`. `'` http://people.debian.org/~madduck - http://debiansystem.info
`- Debian - when you have better things to do than fixing systems
seen on an advertising for an elaborate swiss men's watch:
"almost as complicated as a woman. except it's on time"
[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: What's cooking in git.git (topics)
From: David Kastrup @ 2007-10-03 8:20 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Git Mailing List
In-Reply-To: <alpine.LFD.0.999.0710021916080.3579@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> [ This is the discussed stupid approach - just sort the dang hash array,
> so that we can use a linear scan over the src/dst ]
>
> On Tue, 2 Oct 2007, David Kastrup wrote:
>>
>> This does not actually require an actual merge _sort_ AFAICS: do the
>> "sort file.hashed" step using qsort. The comparison step does not
>> actually need to produce merged output, but merely advances through
>> two hash arrays and generates statistics.
>>
>> This should already beat the pants off the current implementation,
>> even when the hash array is sparse, simply because our inner loop then
>> has perfect hash coherence.
>
> Sadly, that's not the case. It *does* seem to beat the current
> implementation, but it's not "beat the pants off".
Part of the reason is that it is not actually what I had in mind. Why
create the hash array as a hash array? Filling the hash array in
basically random order, then sort+compressing it is what is causing
much of the costs. My idea was to just fill the "hash array"
linearly. It is quite pointless (and certainly very inefficient with
regard to cache poisoning) to do it in hash order when we are going to
sort it anyway.
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Re: WIP: asciidoc replacement
From: David Kastrup @ 2007-10-03 8:12 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: Junio C Hamano, Johannes Schindelin, git, msysgit
In-Reply-To: <39F3EE1B-7BD4-4927-AB90-2EB4BBAF05D0@wincent.com>
Wincent Colaiuta <win@wincent.com> writes:
> El 3/10/2007, a las 6:48, Junio C Hamano escribió:
>
>> - Does it make sense in the longer term for us to maintain
>> in-house documentation tools? Can we afford it?
>>
>> It appears that we heard about breakages for every minor docbook
>> updates, and it is really appealing if we do not have to rely on
>> xsl toolchain for manpage generation.
>
> Indeed, especially seeing as asciidoc and the xsl toolchain are the
> trickiest build dependencies to install. If all that could be
> replaced by a single simple script like this one then that would be
> awesome, and probably more maintainable in the long run seeing as it
> would eliminate those intermittent breakages caused by changes in
> third-party tools.
What with output in print, HTML, info? The advantage of a toolchain
in that it is flexible. I am the first to admit that getting the
AsciiDoc/Docbook/Docbook2X toolchain to get it to do what one wants to
is like baking cake in a lightless kitchen. But it is not like we go
through that pain without any reason.
Personally, I think it might make sense to just step away from the
AsciiDoc documentation to Docbook: plain text (without cutified
formatting control like in AsciiDoc) can be generated _from_ Docbook.
And AsciiDoc keeps us from documenting the formatting: Docbook, which
is a source format and looks it, can easily admit comments that won't
get through to the formatted versions. Sure, the first version would
likely be generated with AsciiDoc and thus basically uncommented.
--
David Kastrup, Kriemhildstr. 15, 44793 Bochum
^ permalink raw reply
* Re: git push (mis ?)behavior
From: Miles Bader @ 2007-10-03 8:32 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: Junio C Hamano, Pierre Habouzit, git
In-Reply-To: <0F838789-B02F-4081-8C75-ED06B551D4C0@wincent.com>
Wincent Colaiuta <win@wincent.com> writes:
> Such differences of opinion would be easily accommodated if the
> default behaviour were made configurable. That way everyone can have
> the behaviour they want.
Indeed.
-Miles
--
"Suppose we've chosen the wrong god. Every time we go to church we're
just making him madder and madder." -- Homer Simpson
^ permalink raw reply
* [PATCH v2] INSTALL: Update section on external dependencies
From: Johan Herland @ 2007-10-03 8:27 UTC (permalink / raw)
To: git; +Cc: Miklos Vajna, Junio C Hamano, Reece Dunn
In-Reply-To: <20071003074007.GA15339@genesis.frugalware.org>
Includes:
- Mention dependency on "core" utilities, including coreutils, sed, cut, grep
- Mention dependency on cpio
- Fix up some whitespace and linebreaking issues
Signed-off-by: Johan Herland <johan@herland.net>
---
On Wednesday 03 October 2007, Miklos Vajna wrote:
> On Tue, Oct 02, 2007 at 05:14:25PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > Are there other commands we rely on that may not be universally
> > installed? I myself consider "cut" to be in the category, but
> > other than that I do not think of anything offhand.
>
> when using git in a chroot, i obviously had coreutils/sed/grep installed
> and the only "extra" package i needed (besides the curl an openssl libs)
> was cpio
Ok, here's a more complete patch.
...Johan
INSTALL | 18 +++++++++++-------
1 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/INSTALL b/INSTALL
index 289b046..244470f 100644
--- a/INSTALL
+++ b/INSTALL
@@ -54,6 +54,8 @@ Issues of note:
- Git is reasonably self-sufficient, but does depend on a few external
programs and libraries:
+ - Common "core" utilities including coreutils, sed, cut, and grep.
+
- "zlib", the compression library. Git won't build without it.
- "openssl". Unless you specify otherwise, you'll get the SHA1
@@ -63,22 +65,24 @@ Issues of note:
that come with git (git includes the one from Mozilla, and has
its own PowerPC and ARM optimized ones too - see the Makefile).
- - "libcurl" and "curl" executable. git-http-fetch and
- git-fetch use them. If you do not use http
- transfer, you are probably OK if you do not have
- them.
+ - "libcurl" and "curl" executable. git-http-fetch and git-fetch
+ use them. If you do not use http transfer, you are probably OK
+ if you do not have them.
- expat library; git-http-push uses it for remote lock
management over DAV. Similar to "curl" above, this is optional.
- - "wish", the Tcl/Tk windowing shell is used in gitk to show the
- history graphically, and in git-gui.
+ - "wish", the Tcl/Tk windowing shell is used in gitk to show the
+ history graphically, and in git-gui.
- - "ssh" is used to push and pull over the net
+ - "ssh" is used to push and pull over the net.
- "perl" and POSIX-compliant shells are needed to use most of
the barebone Porcelainish scripts.
+ - "cpio" is used by git-merge for saving and restoring the index,
+ and by git-clone when doing a local (possibly hardlinked) clone.
+
- Some platform specific issues are dealt with Makefile rules,
but depending on your specific installation, you may not
have all the libraries/tools needed, or you may have
--
1.5.3.3.1144.gf10f2
^ permalink raw reply related
* Re: merging .gitignore
From: Andy Parkins @ 2007-10-03 8:23 UTC (permalink / raw)
To: git; +Cc: martin f krafft, Johannes Schindelin
In-Reply-To: <20071002195148.GA14171@lapse.madduck.net>
On Tuesday 2007 October 02, martin f krafft wrote:
> Well, with gitignore I am ready to say that merges should be
> resolved in an additive way. Remember that I am talking about an
> intergration branch, and if feature branches A and B used to ignore
> .o files, and now B suddenly does not ignore them anymore, the only
Okay; *.o was obviously not a good example. A more detailed one: how about a
change like this to a makefile (excuse bastardised diff format)
diff Makefile
-include depends.make
+include depends.mak
diff .gitignore
-depends.make
+depends.mak
> cat $gitignore_files | sort -u
Now, say there is another branch that makes exactly this change but
chooses "depends.inc" as the filename. Your "additive only" merge
of .gitignore will not flag the conflict and will leave a .gitignore with
depends.mak
depends.inc
The makefile conflict will have been resolved one way or the other but the
gitignore conflict will not. While it's not a serious fault it is wrong, and
no one was signalled that it was wrong.
I am still having difficult seeing why you want to hide conflicts
in .gitignore. It's just as possible to get and resolve conflicts in
gitignore as in any other file.
Andy
--
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com
^ permalink raw reply
* Re: [PATCH] Change "refs/" references to symbolic constants
From: Andy Parkins @ 2007-10-03 7:50 UTC (permalink / raw)
To: git; +Cc: Jeff King
In-Reply-To: <20071002191104.GA7901@coredump.intra.peff.net>
On Tuesday 2007 October 02, Jeff King wrote:
> > - if (prefixcmp(head, "refs/heads/"))
> > - die("HEAD not found below refs/heads!");
> > - head += 11;
> > + if (prefixcmp(head, PATH_REFS_HEADS))
> > + die("HEAD not found below " PATH_REFS_HEADS "!");
> > + head += STRLEN_PATH_REFS_HEADS;
>
> This slightly changes the message (extra "/"), but I don't think that is
> a big deal...
Actually, I felt that was an improvement. Personally I always like paths
shown to a user to have the trailing slash so that they can be clear that it
is a path not a file.
> > - strcpy(path + len, "refs");
> > + strcpy(path + len, PATH_REFS);
> > safe_create_dir(path, 1);
> > - strcpy(path + len, "refs/heads");
> > + strcpy(path + len, PATH_REFS_HEADS);
> > safe_create_dir(path, 1);
> > - strcpy(path + len, "refs/tags");
> > + strcpy(path + len, PATH_REFS_TAGS);
> > safe_create_dir(path, 1);
>
> ...but here it's not immediately obvious if the extra trailing "/" is
> OK. Looks like the path just gets handed off to system calls trhough
> safe_create_dir, and they are happy with the trailing slash. But it is a
> behavior change.
It's been a while, but at the time I did it I think I checked
safe_create_dir() to be sure that it was happy with trailing slashes.
> I find the 'PATH_REFS_TAGS "%s"' (with a space) you used earlier a
> little easier to read.
Okay.
> > - if (len < 5 || memcmp(name, "refs/", 5))
> > + if (len < STRLEN_PATH_REFS || memcmp(name, PATH_REFS,
> > STRLEN_PATH_REFS))
>
> I imagine this was one of the times you mentioned before where prefixcmp
> would be more readable. I would agree.
It is. A patch to fix these is in my pending list.
> > - strcpy(posn, "/objects/");
> > + strcpy(posn, "/" PATH_OBJECTS);
> > posn += 9;
>
> should be posn += 1 + STRLEN_PATH_OBJECTS ?
Well spotted. Fixed.
> > - url = xmalloc(strlen(repo->base) + 64);
> > - sprintf(url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
> > + url = xmalloc(strlen(repo->base) + STRLEN_PATH_OBJECTS + 56);
> > + sprintf(url, "%s/" PATH_OBJECTS "pack/pack-%s.idx", repo->base, hex);
>
> The '56' is still quite hard to verify as correct ("/" + "pack/pack-" +
> ".idx" + "\0"). But I wonder if trying to fix that will just make it
> harder to read (perhaps a comment is in order?).
I put a comment above the other changes like this in the same file, but
thought I was being overly verbose putting it in every single time. I'm
happy to copy the comment around in the file though.
> Or maybe using a strbuf here would be much more obviously correct?
>
> > - url = xmalloc(strlen(base) + 31);
> > - sprintf(url, "%s/objects/info/http-alternates", base);
> > + url = xmalloc(strlen(base) + STRLEN_PATH_OBJECTS + 23);
> > + sprintf(url, "%s/" PATH_OBJECTS "info/http-alternates", base);
>
> Also a potential strbuf. Ther are more of this same form, but I'm not
> going to bother pointing out each one.
I was trying, as far as I could, to leave the code unchanged. If strbuf would
be better I think I'd rather do it with another patch after this.
> Man that was tedious. But I think every other change is purely
> syntactic, so there shouldn't be any bugs.
It really was wasn't it? :-) While I was trying to find that bug that you
caught yesterday I thought I was going blind. I have this to say though:
thank heaven for git's colourised diffs. Those that think coloured output is
just for prettiness sake should try that review with and without.
Thank you for expending so much effort on this, it is appreciated.
Andy
--
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com
^ permalink raw reply
* Re: [PATCH 1/2] Change "refs/" references to symbolic constants
From: Andy Parkins @ 2007-10-03 7:40 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <7v1wcdmm7p.fsf@gitster.siamese.dyndns.org>
On Tuesday 2007 October 02, Junio C Hamano wrote:
> Andy Parkins <andyparkins@gmail.com> writes:
> > I noticed a couple of places where memcmp() has been used where
> > prefixcmp() would work fine. I'm tempted to change them too - what do
> > you think? Perhaps a separate patch?
>
> In general, probably it is preferable to have a separate
> "preliminary patch" to normalize the existing code without using
> the new infrastructure (i.e. REF_* macros), and then to have the
> main patch. That way would make the main patch more about
> mechanical conversion, which would be easier to verify
> independently.
I only noticed them memcmp() use while I was reviewing the PATH_REFS patch :-)
So making it preliminary would have meant travelling backwards in time.
I imagine I can do a bit of rebase-interactive to rearrange should that be the
route you'd like me to go. Your call.
Andy
--
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com
^ permalink raw reply
* Re: [PATCH] Mention 'cpio' dependency in INSTALL
From: Miklos Vajna @ 2007-10-03 7:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johan Herland, git, Reece Dunn
In-Reply-To: <7v1wcdjbq6.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 436 bytes --]
On Tue, Oct 02, 2007 at 05:14:25PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Are there other commands we rely on that may not be universally
> installed? I myself consider "cut" to be in the category, but
> other than that I do not think of anything offhand.
when using git in a chroot, i obviously had coreutils/sed/grep installed
and the only "extra" package i needed (besides the curl an openssl libs)
was cpio
- VMiklos
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH] Change "refs/" references to symbolic constants
From: Andy Parkins @ 2007-10-03 7:37 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <20071002204848.GA8284@coredump.intra.peff.net>
On Tuesday 2007 October 02, Jeff King wrote:
> Perhaps a better quest would be to eliminate all of those counts
> entirely with code that is obviously correct. I think it is much more
> readable to replace:
I've got a patch replacing every appropriate memcmp() with prefixcmp(), but it
goes on top of this one, so wanted to get this through review to save
constantly spamming the list with the same patch slightly modified because of
changes in a different patch.
> url = xmalloc(strlen(repo->base) + 64);
> sprintf(url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
>
> with something like:
>
> strbuf_init(&url);
> strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
I've not been following the strbuf() changes, so have missed the appearance of
these handy new functions. They would appear to be an improvement for cases
just like this.
> > constants in CAPITAL_LETTERS_WITH_UNDERSCORE shout too loudly to
>
> Part of the problem is also that they're long. Perhaps REFS_HEADS, while
> being less unique in the C namespace, would look better?
I completely agree with the length and loudness concerns, but my worry was
polluting the namespace while maintaining some sort of rationality between
PATH_REFS_HEADS and STRLEN_PATH_REFS_HEADS. My reasoning was that
"refs/heads" -> PATH_REFS_HEADS
is only three extra characters, and
strlen("refs/heads/") -> STRLEN_PATH_REFS_HEADS
is only one extra character.
However I have no strong feelings about changing them.
Andy
--
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com
^ permalink raw reply
* Re: git push (mis ?)behavior
From: Pierre Habouzit @ 2007-10-03 7:35 UTC (permalink / raw)
To: Miles Bader; +Cc: Junio C Hamano, git
In-Reply-To: <buoprzwn5qm.fsf@dhapc248.dev.necel.com>
[-- Attachment #1: Type: text/plain, Size: 1370 bytes --]
On Wed, Oct 03, 2007 at 05:10:09AM +0000, Miles Bader wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> > I think it is sensible to have an option to make it push only the
> > current branch. I am not sure if it is sensible to make that the
> > default.
>
> I really like the current default, it matches my mental model well: I
> generally use "push" to mean "synchronize the remote repository with my
> current one"; if multiple branches have changed, I want those changes
> propagated too.
I understand that … and I know some people rely on the current
behavior…
> I think changing it would be a bad idea, it just seems a pointlessly
> incompatible change.
There definitely is a point: with the current behaviour you sometimes
end up pushing more than what you meant, with sometimes WIP that you
intend to rebase, and it hurts. Git porcelains should help you avoid to
shoot yourself in the foot, hence I think that (especially to git
newcomers), the current default _is_ dangerous.
Though, OTOH, I believe that git push <remote> could keep the current
behavior. I'm also okay with the fact that git push could be
configurable in that regard.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* [PATCH] Add test case for ls-files --with-head
From: Carl Worth @ 2007-10-03 7:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Keith Packard, Git Mailing List
In-Reply-To: <7vtzp8g2s2.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 2528 bytes --]
This tests basic functionality and also exercises a bug noticed
by Keith Packard, (prune_cache followed by add_index_entry can
trigger an attempt to realloc a pointer into the middle of an
allocated buffer).
---
t/t3060-ls-files-with-head.sh | 53 +++++++++++++++++++++++++++++++++++++++++
1 files changed, 53 insertions(+), 0 deletions(-)
create mode 100755 t/t3060-ls-files-with-head.sh
On Tue, 02 Oct 2007 22:55:57 -0700, Junio C Hamano wrote:
>
> Thanks for catching this. This code originally was perfectly
> Ok, but I broke it with the overlay_tree() change.
Yeah, Keith and I were really scratching our heads as to how this
hadn't caused more problems earlier. I wrote the test case below to
explore the issue and found the recent overlay_tree change as you
mention, and that made more sense.
I didn't notice any existing --with-tree test case, so perhaps the
patch below is useful.
-Carl
diff --git a/t/t3060-ls-files-with-head.sh b/t/t3060-ls-files-with-head.sh
new file mode 100755
index 0000000..4ead08b
--- /dev/null
+++ b/t/t3060-ls-files-with-head.sh
@@ -0,0 +1,53 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Carl D. Worth
+#
+
+test_description='git ls-files test (--with-head).
+
+This test runs git ls-files --with-head and in particular in
+a scenario known to trigger a crash with some versions of git.
+'
+. ./test-lib.sh
+
+# The bug we're exercising requires a fair number of entries in a
+# sub-directory so that add_index_entry will trigger a realloc
+echo file > expected
+mkdir sub
+for num in $(seq -f%04g 1 50); do
+ touch sub/file-$num
+ echo file-$num >> expected
+done
+git add .
+git commit -m "add a bunch of files"
+
+# We remove them all so that we'll have something to add back with
+# --with-head and so that we'll definitely be under the realloc size
+# to trigger the bug.
+rm -r sub
+git commit -a -m "remove them all"
+
+# The bug also requires some entry before our directory so that
+# prune_path will modify the_index.cache
+mkdir a_directory_that_sorts_before_sub
+touch a_directory_that_sorts_before_sub/file
+mkdir sub
+touch sub/file
+git add .
+
+# We have to run from a sub-directory to trigger prune_path
+cd sub
+
+# Then we finally get to run our --with-tree test
+test_expect_success \
+ 'git -ls-files --with-tree should succeed.' \
+ 'git ls-files --with-tree=HEAD~1 >../output'
+
+cd ..
+test_expect_success \
+ 'git -ls-files --with-tree should add entries from named tree.' \
+ 'diff output expected'
+
+test_done
+
+
--
1.5.3.3.131.g34c6d
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply related
* Re: WIP: asciidoc replacement
From: Wincent Colaiuta @ 2007-10-03 6:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git, msysgit
In-Reply-To: <7vprzwhkgd.fsf@gitster.siamese.dyndns.org>
El 3/10/2007, a las 6:48, Junio C Hamano escribió:
> - Does it make sense in the longer term for us to maintain
> in-house documentation tools? Can we afford it?
>
> It appears that we heard about breakages for every minor docbook
> updates, and it is really appealing if we do not have to rely on
> xsl toolchain for manpage generation.
Indeed, especially seeing as asciidoc and the xsl toolchain are the
trickiest build dependencies to install. If all that could be
replaced by a single simple script like this one then that would be
awesome, and probably more maintainable in the long run seeing as it
would eliminate those intermittent breakages caused by changes in
third-party tools.
Cheers,
Wincent
^ permalink raw reply
* Re: What's cooking in git.git (topics)
From: Jeff King @ 2007-10-03 6:54 UTC (permalink / raw)
To: Linus Torvalds; +Cc: David Kastrup, Git Mailing List
In-Reply-To: <alpine.LFD.0.999.0710021916080.3579@woody.linux-foundation.org>
On Tue, Oct 02, 2007 at 07:28:19PM -0700, Linus Torvalds wrote:
> Sadly, that's not the case. It *does* seem to beat the current
> implementation, but it's not "beat the pants off". It looks like an
> improvement of about 15%, which is nothing to sneeze at, but it's not an
> order-of-magnitude improvement either.
>
> Here's a test-patch. I don't guarantee anything, except that when I did
> the timings I also did a "wc" on the result, and they matched..
I get slightly better speedups with my pathological case (around 30%):
Before:
$ /usr/bin/time git-diff --raw -M -l0 06d288^ 06d288 >/dev/null
105.38user 3.65system 2:14.90elapsed 80%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (15432major+542627minor)pagefaults 0swaps
After:
$ /usr/bin/time git-diff --raw -M -l0 06d288^ 06d288 >/dev/null
71.70user 3.47system 1:40.43elapsed 74%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (15065major+551778minor)pagefaults 0swaps
But yes, it's not the order of magnitude we were looking for.
> [torvalds@woody linux]$ time git diff -l0 --stat -C v2.6.22.. | wc
I found less noise in the timing by using --raw, since the patch
computation takes an appreciable amount of time.
> but the diff is fairly simple, so if somebody will go over it and say
> whether it's likely to be *correct* too, that 15% may well be worth it.
Patch looks correct, and it produces correct results on my (admittedly
limited) test data.
I think it's worth applying (though I agree that a comment on the
assumption of a zero "cnt" at the end is worth adding) unless some
drastically different solution comes along (e.g., David's idea to try
avoiding the outer O(n^2) loop). But I don't think there is much more to
be gained from a different approach to comparing the two hash tables.
-Peff
^ permalink raw reply
* Re: git push (mis ?)behavior
From: Wincent Colaiuta @ 2007-10-03 6:47 UTC (permalink / raw)
To: Miles Bader; +Cc: Junio C Hamano, Pierre Habouzit, git
In-Reply-To: <buoprzwn5qm.fsf@dhapc248.dev.necel.com>
El 3/10/2007, a las 7:10, Miles Bader escribió:
> Junio C Hamano <gitster@pobox.com> writes:
>> I think it is sensible to have an option to make it push only the
>> current branch. I am not sure if it is sensible to make that the
>> default.
>
> I really like the current default, it matches my mental model well: I
> generally use "push" to mean "synchronize the remote repository
> with my
> current one"; if multiple branches have changed, I want those changes
> propagated too.
>
> I think changing it would be a bad idea, it just seems a pointlessly
> incompatible change. The reasons I've seen offered on this thread for
> changing the default seem pretty weak, e.g., "it's more conservative"
> (but more annoying)
It could be more annoying for some, yet a life saver for others. So
before changing the default obviously we would need to get a clear
idea of whether or not the majority would approve of such a move.
Such differences of opinion would be easily accommodated if the
default behaviour were made configurable. That way everyone can have
the behaviour they want.
Cheers,
Wincent
PS. I'm the one who mentioned SVK, but I didn't offer it as a reason
to justify the change (I agree, more than "weak" it's not really any
reason at all); I just mentioned to indicate why it is that the
current behaviour caught me off guard.
^ permalink raw reply
* Re: WIP: asciidoc replacement
From: Wincent Colaiuta @ 2007-10-03 6:40 UTC (permalink / raw)
To: Sam Vilain; +Cc: Johannes Schindelin, git, msysgit
In-Reply-To: <4702F6BB.60908@vilain.net>
El 3/10/2007, a las 3:56, Sam Vilain escribió:
> However I have a suspicion that your script is doing line-based
> parsing
> instead of recursive descent; I don't know whether that's the right
> thing for asciidoc. It's actually fairly easy to convert a grammar to
> code blocks using tricks from MJD's _Higher Order Perl_. Is it
> necessary for the asciidoc grammar?
I haven't looked at all of the asciidoc source for the Git
documentation but I suspect that almost all of it is entirely
regular, and if there is any nesting in it is is probably of a
limited scope (ie. sections, subsections, subsubsections in the user
manual) and so you can avoid the complexity of a full recursive
descent parser. I'd also expect Johannes' approach to be faster (is
it faster than the existing tool chain? I would expect so).
Cheers,
Wincent
^ permalink raw reply
* Re: [PATCH] Mention 'cpio' dependency in INSTALL
From: Chris Larson @ 2007-10-03 6:09 UTC (permalink / raw)
To: git
In-Reply-To: <7v1wcdjbq6.fsf@gitster.siamese.dyndns.org>
On 10/2/07, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>
> > reveals that cpio is not mentioned anywhere in the documentation,
> > nor in the requirements section of the INSTALL file.
>
> Thanks.
>
> We use many other tools that are typically found in bog-standard
> UNIX environments, like sed, echo, cat, sort, etc. and we do not
> list them in the INSTALL file (nor we would want to). cpio used
> to be in the "bog standard" category but perhaps Linux distros
> do not install it by default, so it is worth listing it there.
>
> Are there other commands we rely on that may not be universally
> installed? I myself consider "cut" to be in the category, but
> other than that I do not think of anything offhand.
I'd think it might be a good idea to list any utilities used that
aren't in, say, the Single Unix Specification, for example.
--
Chris Larson - clarson at kergoth dot com
Software Engineer - MontaVista - clarson at mvista dot com
Core Developer/Architect - TSLib, BitBake, OpenEmbedded, OpenZaurus
^ permalink raw reply
* Re: [PATCH] Must not modify the_index.cache as it may be passed to realloc at some point.
From: Junio C Hamano @ 2007-10-03 5:55 UTC (permalink / raw)
To: Keith Packard; +Cc: Git Mailing List
In-Reply-To: <1191390255.16292.2.camel@koto.keithp.com>
Keith Packard <keithp@keithp.com> writes:
> The index cache is not static, growing as new entries are added. If entries
> are added after prune_cache is called, cache will no longer point at the
> base of the allocation, and realloc will not be happy.
Thanks for catching this. This code originally was perfectly
Ok, but I broke it with the overlay_tree() change.
^ permalink raw reply
* Re: Clone corruption to G4 MacOSX
From: Perry Wagle @ 2007-10-03 5:50 UTC (permalink / raw)
To: Kyle McMartin; +Cc: git
In-Reply-To: <20071003052834.GC13738@fattire.cabal.ca>
Yeah, that'd be it. Thanks!
-- Perry
On Oct 2, 2007, at 10:28 PM, Kyle McMartin wrote:
> On Tue, Oct 02, 2007 at 09:29:07PM -0700, Perry Wagle wrote:
>> If I clone Linus's repository to a x86 machine, I get no corruption.
>>
>> My wild ass guess is that being big-endian is causing trouble.
>>
>
> The problem is MacOSX uses a case-insensitive filesystem by default...
>
> Cheers,
> Kyle
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] Must not modify the_index.cache as it may be passed to realloc at some point.
From: Keith Packard @ 2007-10-03 5:44 UTC (permalink / raw)
To: Git Mailing List; +Cc: keithp
[-- Attachment #1: Type: text/plain, Size: 1277 bytes --]
The index cache is not static, growing as new entries are added. If entries
are added after prune_cache is called, cache will no longer point at the
base of the allocation, and realloc will not be happy.
I verified that this was the only place in the current source which modified
any index_state.cache elements aside from the alloc/realloc calls in read-cache by
changing the type of the element to 'struct cache_entry ** const cache' and
recompiling.
A more efficient patch would create a separate 'cache_base' value to track
the allocation and then fix things up when reallocation was necessary,
instead of the brute-force memmove used here.
---
builtin-ls-files.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index 6c1db86..0028b8a 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -280,7 +280,7 @@ static void prune_cache(const char *prefix)
if (pos < 0)
pos = -pos-1;
- active_cache += pos;
+ memmove (active_cache, active_cache + pos, (active_nr - pos) * sizeof (struct cache_entry *));
active_nr -= pos;
first = 0;
last = active_nr;
--
1.5.3.3.131.g34c6d-dirty
--
keith.packard@intel.com
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply related
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