# Unresolved issues 39 messages from 2007-02-20 to 2007-02-28. Participants: Junio C Hamano, Andy Parkins, Linus Torvalds, Nicolas Pitre, Johannes Schindelin, David Lang, Theodore Tso, Martin Waitz, Robin Rosenberg, Brian Gernhardt, Shawn O. Pearce, Julian Phillips, Jakub Narebski, Martin Langhoff. Thread: https://gitlist.dev/t/6891 ## Junio C Hamano, 2007-02-20 07:28 Subject: Unresolved issues Message-ID: <7virdx1e58.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7virdx1e58.fsf%40assigned-by-dhcp.cox.net ``` Here are some issues recently raised on the list, some of them with patches, that have not been resolved satisfactory. [gmane=http://thread.gmane.org/gmane.comp.version-control.git] * "git status" is not a read-only operation. It needs to do enough lstat(2) to run "update-index --refresh" to come up with the information it needs to give. We could do so internally without writing out the result to the index (there is a patch to do this) even if a repository is not writable. $gmane/39205 $gmane/39206 However, a big downside of this approach is that doing so unconditionally would mean the expensive lstat(2) is wasted afterwards. $gmane/39246 Currently an workaround is to run git-runstatus and live with the fact that otherwise unmodified but stat-dirty paths to show up in the output. I think (iff somebody feels strongly about it) a possible compromise would be to see if we can update the index, and do what the current code does if we can, and otherwise fall back on the new code that does the internal "update-index --refresh". * "git -p status" does not honor color. The problem is that when git-runstatus is run, it already runs as an upstream process of a pipe and git_config_colorbool() would say "oh, our output is not a terminal, and we did not start pager ourselves". $gmane/39919 A patch was proposed to propagate pager_in_use as an environment down to subprocesses, $gmane/39936 but I think this would have unintended side effects when scripts want to run commands and redirect their output internally for their own use (they will get colorized output from lower level in their temporary files or v=$(cmd) redirect). * "git log -r --raw -z -- path | grep -z SHA-1" is not very useful. We would need a separate -Z option that means "output records are separated with NUL, but output fields are not". $gmane/39207 This would help to solve "someone mails me a blob, git please tell me what it is" problem. $gmane/39925 * "git fetch" between repositories with hundreds of refs. $gmane/39330 There are partial rewrite of the most expensive parts of git-fetch in C parked in 'pu'. It might be good enough for public consumption without going the whole nine yards. I dunno. I am not very keen on rewriting all of "git fetch" in C right now, as people seem to be still interested in touching it (including "git bundle" topic). * core.autocrlf Linus and I laid out most of the infrastructure and the basic things already seem to work. We even added some tests ;-) commit 6716027108f426c83038b05baf3f20ceefe6fbd1 commit 634ede32ae7d4c76e96e88f9cd5c1b3a70ea08ac commit d7f4633405acf3dc09798a759463c616c7c49dfd commit 6c510bee2013022fbce52f4b0ec0cc593fc0cc48 What's still missing is support for .gitignore like "these files are text" information. One thing that might be tricky is what should be done while making a merge or checking out from a tree. Ideally, the information should be read from the tree that is being extracted, but that would make the code structure a little bit, eh, "interesting". * Use update-ref in cvsserver. It currently does it by hand, which is racy and does not leave traces in reflog. $gmane/39541 * Dissociating a repository from its alternates I sent out a rather elaborate changes in the binary, but what Johannes suggests is much easier to implement. $gmane/39834 Volunteers? * User-wide ignore list $gmane/39809 I am not really sure if this is even a desirable feature, but assuming it is, one possible solution would be to do this: $gmane/39820 But I am not going to do this myself; I suspect that it would be fairly simple and straightforward that some git-hacker-hopeful should be able to. * git-diff2 It somehow feels a tad ugly that this is a separate command from git-diff, but I do not feel strongly enough to fix it myself. Currently parked in 'pu'. ``` ## Andy Parkins, 2007-02-20 08:57 Subject: Re: Unresolved issues Message-ID: <200702200857.02779.andyparkins@gmail.com> URL: https://gitlist.dev/e/200702200857.02779.andyparkins%40gmail.com In-Reply-To: <7virdx1e58.fsf@assigned-by-dhcp.cox.net> ``` On Tuesday 2007 February 20 07:28, Junio C Hamano wrote: > * Use update-ref in cvsserver. > > It currently does it by hand, which is racy and does not leave traces > in reflog. > > $gmane/39541 I've got a patch for this - I thought I'd sent it and it left my mind - I'll send when I get home. Andy -- Dr Andy Parkins, M Eng (hons), MIEE andyparkins@gmail.com ``` ## Linus Torvalds, 2007-02-20 17:41 Subject: Re: Unresolved issues Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0702200934270.20368%40woody.linux-foundation.org In-Reply-To: <7virdx1e58.fsf@assigned-by-dhcp.cox.net> ``` On Mon, 19 Feb 2007, Junio C Hamano wrote: > > * core.autocrlf > > What's still missing is support for .gitignore like "these files are > text" information. Well, we could actually just do that in stages. We might start off with just saying "unlike .gitignore", we only support one top-level ".gitattributes" file. That makes the problem space much simpler, and then we can have code like enum file_type { FILE_AUTO, FILE_BINARY, FILE_TEXT }; static enum file_type get_file_type(const char *pathname) { static int has_initialized = 0; if (!has_initialized) { has_initialized = 1; read_file_attributes_file(); } ... check the filename against our attribute rules .. } which would be fairly straightforward, and efficient. It gets more complicated with per-directory attributes files, because then you need to either open those files *all* the time (stupid and expensive if you have thousands of files and hundreds of directories), or you need to have some way to cache just the ones you need. (In fact, it might be perfectly fine to have just a *single* cache, which is keyed on the dirname of the pathname: if the dirname changes, just throw the cache away, and read it in from all the subdirectories leading to that directory - you'd still re-read stuff, but all the common cases will walk the directory structure in a nice pattern, so you'd have a very simple cache that actually gets good cache hit behaviour) > One thing that might be tricky is what should be done while making a > merge or checking out from a tree. Ideally, the information should be > read from the tree that is being extracted, but that would make the > code structure a little bit, eh, "interesting". No, that would be pretty horrid. So just tell everybody that it's based on the working tree. I don't think it's likely to be a problem in practice. Linus ``` ## Andy Parkins, 2007-02-20 20:10 Subject: [PATCH] Use git-update-ref to update a ref during commit in git-cvsserver Message-ID: <200702202010.02128.andyparkins@gmail.com> URL: https://gitlist.dev/e/200702202010.02128.andyparkins%40gmail.com In-Reply-To: <200702200857.02779.andyparkins@gmail.com> ``` Nicholas Pitre mentioned that updating a reference should be done with git-update-ref. This patch does that and includes the -m option to have the reflog updated as a bonus. Signed-off-by: Andy Parkins --- As promised... git-cvsserver.perl | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/git-cvsserver.perl b/git-cvsserver.perl index b4ef6bc..54d943a 100755 --- a/git-cvsserver.perl +++ b/git-cvsserver.perl @@ -1216,9 +1216,9 @@ sub req_ci } close LOCKFILE; - my $reffile = "$ENV{GIT_DIR}refs/heads/$state->{module}"; - unlink($reffile); - rename($lockfile, $reffile); + my $reffile = "refs/heads/$state->{module}"; + `git-update-ref -m "git-cvsserver commit" $reffile $commithash $parenthash`; + unlink($lockfile); chdir "/"; print "ok\n"; -- 1.5.0.rc4.gb4d2 ``` ## Junio C Hamano, 2007-02-20 21:43 Subject: Re: Unresolved issues Message-ID: <7vfy90v729.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vfy90v729.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Linus Torvalds writes: > (In fact, it might be perfectly fine to have just a *single* cache, which > is keyed on the dirname of the pathname: if the dirname changes, just > throw the cache away, and read it in from all the subdirectories leading > to that directory - you'd still re-read stuff, but all the common cases > will walk the directory structure in a nice pattern, so you'd have a very > simple cache that actually gets good cache hit behaviour) I'd agree that we can start with just a single one at the toplevel and if somebody wants to extend it we can do so later. >> One thing that might be tricky is what should be done while making a >> merge or checking out from a tree. Ideally, the information should be >> read from the tree that is being extracted, but that would make the >> code structure a little bit, eh, "interesting". > > No, that would be pretty horrid. So just tell everybody that it's based on > the working tree. I don't think it's likely to be a problem in practice. Except for the initial checkout... ``` ## Nicolas Pitre, 2007-02-20 21:57 Subject: Re: [PATCH] Use git-update-ref to update a ref during commit in git-cvsserver Message-ID: URL: https://gitlist.dev/e/alpine.LRH.0.82.0702201654310.31945%40xanadu.home In-Reply-To: <200702202010.02128.andyparkins@gmail.com> ``` On Tue, 20 Feb 2007, Andy Parkins wrote: > Nicholas Pitre mentioned that updating a reference should be done with > git-update-ref. > > This patch does that and includes the -m option to have the reflog > updated as a bonus. > > Signed-off-by: Andy Parkins > --- > As promised... What if git-update-ref fails? This may occur if the server repo was updated and the client needs to "cvs up" again before commit. Nicolas ``` ## Linus Torvalds, 2007-02-21 00:21 Subject: Re: Unresolved issues Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0702201621050.4043%40woody.linux-foundation.org In-Reply-To: <7vfy90v729.fsf@assigned-by-dhcp.cox.net> ``` On Tue, 20 Feb 2007, Junio C Hamano wrote: > > > > No, that would be pretty horrid. So just tell everybody that it's based on > > the working tree. I don't think it's likely to be a problem in practice. > > Except for the initial checkout... Yeah, that's true. That's indeed pretty nasty. There's also a rather strange special case when you do merges: you can certainly always use the .gitattributes of the working tree, but it will cause some interesting issues if new files were added with new patterns. However, we're a bit lucky here (or perhaps "lucky" is not the right word: we basically have a good design) where all these actions come down to "git read-tree", regardless of whether it's checking out the end result of a totally new clone, or a fast-forward update, or a merge. Or a "git checkout" or "git reset". They all boil down to one thing: git read-tree -u and it should be fairly easy to add some simple logic just to "cmd_read_tree()" to do the right thing. It has the "main tree" to use, and the logic could be as simple as fd = open(".gitattributes", O_RDONLY); if (fd < 0) { .. try in "$tree:.gitattributes" instead .. and it would do the right thing for all the common operations. Again, the special case (as always) is - git cat-file - the file-level merger code (which uses the equivalent of git-cat-file) which would need to add their own logic for this. Linus ``` ## Junio C Hamano, 2007-02-21 00:25 Subject: Re: Unresolved issues Message-ID: <7vabz8uzjo.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vabz8uzjo.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Linus Torvalds writes: > On Tue, 20 Feb 2007, Junio C Hamano wrote: >> > >> > No, that would be pretty horrid. So just tell everybody that it's based on >> > the working tree. I don't think it's likely to be a problem in practice. >> >> Except for the initial checkout... > > Yeah, that's true. That's indeed pretty nasty. > > There's also a rather strange special case when you do merges: you can > certainly always use the .gitattributes of the working tree, but it will > cause some interesting issues if new files were added with new patterns. Let alone the case where you need to merge .gitattributes file itself and the conflicting part says something conflicting about other paths that needed to be merged and the result need to be checked out ;-). > However, we're a bit lucky here (or perhaps "lucky" is not the right word: > we basically have a good design) where all these actions come down to "git > read-tree", regardless of whether it's checking out the end result of a > totally new clone, or a fast-forward update, or a merge. Or a "git > checkout" or "git reset". They all boil down to one thing: > > git read-tree -u > > and it should be fairly easy to add some simple logic just to > "cmd_read_tree()" to do the right thing. It has the "main tree" to use, > and the logic could be as simple as > > fd = open(".gitattributes", O_RDONLY); > if (fd < 0) { > .. try in "$tree:.gitattributes" instead .. > > > and it would do the right thing for all the common operations. Yes. That is what I had in mind when I said "initial checkout". > Again, the special case (as always) is > - git cat-file > - the file-level merger code (which uses the equivalent of git-cat-file) > which would need to add their own logic for this. > > Linus And git-apply which is (as usual) on its own. ``` ## Johannes Schindelin, 2007-02-21 00:39 Subject: Re: Unresolved issues Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0702210136050.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: ``` Hi, On Tue, 20 Feb 2007, Linus Torvalds wrote: > > On Tue, 20 Feb 2007, Junio C Hamano wrote: > > > > > > No, that would be pretty horrid. So just tell everybody that it's based on > > > the working tree. I don't think it's likely to be a problem in practice. > > > > Except for the initial checkout... > > Yeah, that's true. That's indeed pretty nasty. Um, I don't want to spoil the party, but was not the original idea of this auto-CRLF thing some sort of "emulation" of the CVS text checkout behaviour? In that case, .gitattributes (I mean a tracked one) would be wrong, wrong, wrong. It's a local setup if you want auto-CRLF or not. So, why not just make it a local setting (if in config or $GIT_DIR/info/gitattributes, I don't care) which shell patterns are to be transformed on input and/or output? Ciao, Dscho ``` ## David Lang, 2007-02-21 00:51 Subject: Re: Unresolved issues Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0702201648450.14284%40qynat.qvtvafvgr.pbz In-Reply-To: ``` On Tue, 20 Feb 2007, Linus Torvalds wrote: > On Wed, 21 Feb 2007, Johannes Schindelin wrote: >> >> Um, I don't want to spoil the party, but was not the original idea of this >> auto-CRLF thing some sort of "emulation" of the CVS text checkout >> behaviour? >> >> In that case, .gitattributes (I mean a tracked one) would be wrong, wrong, >> wrong. >> >> It's a local setup if you want auto-CRLF or not. So, why not just make it >> a local setting (if in config or $GIT_DIR/info/gitattributes, I don't >> care) which shell patterns are to be transformed on input and/or output? > > That is a good point. We *could* just make it a ".git/config" issue, which > has the nice benefit that you can just set up some user-wide rules rather > than making it be per-repo. some of the things that .gitattributes is being talked being used for about for are local, some are per-repo per the prior discussion of how many places to check a local configuration should override the per-repo setting. David Lang > > Of course, the config language may not be wonderful for this. But we could > certainly have something like > > [format "crlf"] > enable = true > text = *.[ch] > binary = *.jpg > > which would just override the built-in rules (where anything that doesn't > match is just "auto-content"). And make the default built-in ones be good > enough that in _practice_ you never even need this in the first place. > > Linus > - > 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 > ``` ## Linus Torvalds, 2007-02-21 00:56 Subject: Re: Unresolved issues Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0702201648000.4043%40woody.linux-foundation.org In-Reply-To: ``` On Wed, 21 Feb 2007, Johannes Schindelin wrote: > > Um, I don't want to spoil the party, but was not the original idea of this > auto-CRLF thing some sort of "emulation" of the CVS text checkout > behaviour? > > In that case, .gitattributes (I mean a tracked one) would be wrong, wrong, > wrong. > > It's a local setup if you want auto-CRLF or not. So, why not just make it > a local setting (if in config or $GIT_DIR/info/gitattributes, I don't > care) which shell patterns are to be transformed on input and/or output? That is a good point. We *could* just make it a ".git/config" issue, which has the nice benefit that you can just set up some user-wide rules rather than making it be per-repo. Of course, the config language may not be wonderful for this. But we could certainly have something like [format "crlf"] enable = true text = *.[ch] binary = *.jpg which would just override the built-in rules (where anything that doesn't match is just "auto-content"). And make the default built-in ones be good enough that in _practice_ you never even need this in the first place. Linus ``` ## Johannes Schindelin, 2007-02-21 01:12 Subject: Re: Unresolved issues Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0702210206390.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: ``` Hi, On Tue, 20 Feb 2007, Linus Torvalds wrote: > On Wed, 21 Feb 2007, Johannes Schindelin wrote: > > > > Um, I don't want to spoil the party, but was not the original idea of this > > auto-CRLF thing some sort of "emulation" of the CVS text checkout > > behaviour? > > > > In that case, .gitattributes (I mean a tracked one) would be wrong, wrong, > > wrong. > > > > It's a local setup if you want auto-CRLF or not. So, why not just make it > > a local setting (if in config or $GIT_DIR/info/gitattributes, I don't > > care) which shell patterns are to be transformed on input and/or output? > > That is a good point. We *could* just make it a ".git/config" issue, > which has the nice benefit that you can just set up some user-wide rules > rather than making it be per-repo. Yes, that's a nice side effect. > Of course, the config language may not be wonderful for this. Like you wrote in your example, most shell patterns are no problem in the config. > And make the default built-in ones be good enough that in _practice_ you > never even need this in the first place. This is really, really important. We already see many users using git, expecting it somehow to figure out what they want without them having read TFM. Anyhow, I am right now talking with Junio (on IRC; my 2nd day wasting time in chatspace ;-), among other things about the import/export filters. We should at least keep in mind that this would be nice to have, and leave a door open for them when we do this stuff. Ciao, Dscho ``` ## Theodore Tso, 2007-02-21 01:49 Subject: Re: Unresolved issues Message-ID: <20070221014924.GC27391@thunk.org> URL: https://gitlist.dev/e/20070221014924.GC27391%40thunk.org In-Reply-To: ``` On Wed, Feb 21, 2007 at 01:39:48AM +0100, Johannes Schindelin wrote: > In that case, .gitattributes (I mean a tracked one) would be wrong, wrong, > wrong. Well, some of the uses of .gitattributes that I had propose was to associate file types (and from the file types, a set of check-in, check-out, diff, and pretty-print helper progams) for things like OpenOffice files. For those a tracked .gitattributes makes a lot of sense. - Ted ``` ## Nicolas Pitre, 2007-02-21 01:51 Subject: Re: Unresolved issues Message-ID: URL: https://gitlist.dev/e/alpine.LRH.0.82.0702202003370.31945%40xanadu.home In-Reply-To: ``` On Tue, 20 Feb 2007, Linus Torvalds wrote: > > > It's a local setup if you want auto-CRLF or not. So, why not just make it > > a local setting (if in config or $GIT_DIR/info/gitattributes, I don't > > care) which shell patterns are to be transformed on input and/or output? > > That is a good point. We *could* just make it a ".git/config" issue, which > has the nice benefit that you can just set up some user-wide rules rather > than making it be per-repo. > > Of course, the config language may not be wonderful for this. But we could > certainly have something like > > [format "crlf"] > enable = true > text = *.[ch] > binary = *.jpg I think this is not generic enough. For one thing this should not be used for crlf only. There is also the binary patch generation code that wants to know if a file is binary or not. What about: [filetype "text"] match=*.[ch] attribute=text crlfmangle=true [filetype "images"] match=*.jpg attribute=binary merge=special_jpg_merger etc. Nicolas ``` ## Linus Torvalds, 2007-02-21 02:03 Subject: Re: Unresolved issues Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0702201758560.4043%40woody.linux-foundation.org In-Reply-To: ``` On Tue, 20 Feb 2007, Nicolas Pitre wrote: > > I think this is not generic enough. For one thing this should not be > used for crlf only. There is also the binary patch generation code that > wants to know if a file is binary or not. > > What about: > > [filetype "text"] > match=*.[ch] > attribute=text > crlfmangle=true > > [filetype "images"] > match=*.jpg > attribute=binary > merge=special_jpg_merger Yes, that's a much nicer format - both more readable, and more generic. Although I'd just suggest skipping the "crlfmangle". Just document the fact that for "attribute=text", we mangle line-endings as per the rules defined elsewhere (which is possibly different for input/output in addition for the normal unix/windows rule changes) And then you can just have multiple "match=" rules, so that you don't need to make one complex one. Linus ``` ## Junio C Hamano, 2007-02-21 05:54 Subject: Re: [PATCH] Use git-update-ref to update a ref during commit in git-cvsserver Message-ID: <7vmz38t5r4.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vmz38t5r4.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <200702202010.02128.andyparkins@gmail.com> ``` Andy Parkins writes: > As promised... > > git-cvsserver.perl | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/git-cvsserver.perl b/git-cvsserver.perl > index b4ef6bc..54d943a 100755 > --- a/git-cvsserver.perl > +++ b/git-cvsserver.perl > @@ -1216,9 +1216,9 @@ sub req_ci > } > > close LOCKFILE; > - my $reffile = "$ENV{GIT_DIR}refs/heads/$state->{module}"; > - unlink($reffile); > - rename($lockfile, $reffile); > + my $reffile = "refs/heads/$state->{module}"; > + `git-update-ref -m "git-cvsserver commit" $reffile $commithash $parenthash`; > + unlink($lockfile); > chdir "/"; > > print "ok\n"; Using its own lockfile to update ref by hand while running update-ref alongside it feels _very_ wrong. How about this one instead? -- >8 -- [PATCH] Make 'cvs ci' lockless This makes "ci" codepath lockless by following the usual "remember the tip, do your thing, then compare and swap at the end" update pattern using update-ref. Incidentally, by updating the code that reads where the tip of the head is to use show-ref, it makes it safe to use in a repository whose refs are pack-pruned. I noticed that other parts of the program are not yet pack-refs safe, but tried to keep the changes to the minimum. Now I rarely use git-cvsserver myself, so I may be completely breaking the check-in codepath. Buyers beware... --- git-cvsserver.perl | 41 ++++++++++++++++------------------------- 1 files changed, 16 insertions(+), 25 deletions(-) diff --git a/git-cvsserver.perl b/git-cvsserver.perl index 9371788..471621b 100755 --- a/git-cvsserver.perl +++ b/git-cvsserver.perl @@ -1031,36 +1031,35 @@ sub req_ci exit; } - my $lockfile = "$state->{CVSROOT}/refs/heads/$state->{module}.lock"; - unless ( sysopen(LOCKFILE,$lockfile,O_EXCL|O_CREAT|O_WRONLY) ) - { - $log->warn("lockfile '$lockfile' already exists, please try again"); - print "error 1 Lock file '$lockfile' already exists, please try again\n"; - exit; - } - # Grab a handle to the SQLite db and do any necessary updates my $updater = GITCVS::updater->new($state->{CVSROOT}, $state->{module}, $log); $updater->update(); my $tmpdir = tempdir ( DIR => $TEMP_DIR ); my ( undef, $file_index ) = tempfile ( DIR => $TEMP_DIR, OPEN => 0 ); - $log->info("Lock successful, basing commit on '$tmpdir', index file is '$file_index'"); + $log->info("Lockless commit start, basing commit on '$tmpdir', index file is '$file_index'"); $ENV{GIT_DIR} = $state->{CVSROOT} . "/"; $ENV{GIT_INDEX_FILE} = $file_index; + # Remember where the head was at the beginning. + my $parenthash = `git show-ref -s refs/heads/$state->{module}`; + chomp $parenthash; + if ($parenthash !~ /^[0-9a-f]{40}$/) { + print "error 1 pserver cannot find the current HEAD of module"; + exit; + } + chdir $tmpdir; # populate the temporary index based - system("git-read-tree", $state->{module}); + system("git-read-tree", $parenthash); unless ($? == 0) { die "Error running git-read-tree $state->{module} $file_index $!"; } $log->info("Created index '$file_index' with for head $state->{module} - exit status $?"); - my @committedfiles = (); # foreach file specified on the command line ... @@ -1095,8 +1094,6 @@ sub req_ci { # fail everything if an up to date check fails print "error 1 Up to date check failed for $filename\n"; - close LOCKFILE; - unlink($lockfile); chdir "/"; exit; } @@ -1139,16 +1136,12 @@ sub req_ci { print "E No files to commit\n"; print "ok\n"; - close LOCKFILE; - unlink($lockfile); chdir "/"; return; } my $treehash = `git-write-tree`; - my $parenthash = `cat $ENV{GIT_DIR}refs/heads/$state->{module}`; chomp $treehash; - chomp $parenthash; $log->debug("Treehash : $treehash, Parenthash : $parenthash"); @@ -1165,13 +1158,16 @@ sub req_ci { $log->warn("Commit failed (Invalid commit hash)"); print "error 1 Commit failed (unknown reason)\n"; - close LOCKFILE; - unlink($lockfile); chdir "/"; exit; } - print LOCKFILE $commithash; + if (system(qw(git update-ref -m), "cvsserver ci", + "refs/heads/$state->{module}", $commithash, $parenthash)) { + $log->warn("update-ref for $state->{module} failed."); + print "error 1 Cannot commit -- update first\n"; + exit; + } $updater->update(); @@ -1200,12 +1196,7 @@ sub req_ci } } - close LOCKFILE; - my $reffile = "$ENV{GIT_DIR}refs/heads/$state->{module}"; - unlink($reffile); - rename($lockfile, $reffile); chdir "/"; - print "ok\n"; } ``` ## Andy Parkins, 2007-02-21 09:08 Subject: Re: [PATCH] Use git-update-ref to update a ref during commit in git-cvsserver Message-ID: <200702210908.59579.andyparkins@gmail.com> URL: https://gitlist.dev/e/200702210908.59579.andyparkins%40gmail.com In-Reply-To: <7vmz38t5r4.fsf@assigned-by-dhcp.cox.net> ``` On Wednesday 2007 February 21 05:54, Junio C Hamano wrote: > This makes "ci" codepath lockless by following the usual > "remember the tip, do your thing, then compare and swap at the > end" update pattern using update-ref. Incidentally, by updating Looks much better than mine (obviously). I'll run it for a few days and report back. Andy -- Dr Andy Parkins, M Eng (hons), MIEE andyparkins@gmail.com ``` ## Martin Waitz, 2007-02-21 10:42 Subject: Re: Unresolved issues Message-ID: <20070221104209.GM21842@admingilde.org> URL: https://gitlist.dev/e/20070221104209.GM21842%40admingilde.org In-Reply-To: ``` hoi :) On Wed, Feb 21, 2007 at 01:39:48AM +0100, Johannes Schindelin wrote: > In that case, .gitattributes (I mean a tracked one) would be wrong, wrong, > wrong. I don't think so. > It's a local setup if you want auto-CRLF or not. So, why not just make it > a local setting (if in config or $GIT_DIR/info/gitattributes, I don't > care) which shell patterns are to be transformed on input and/or output? The file-type/whatever information about paths is per repository. Whether you want to do crlf conversion for "text" files is a local setting. So I do think that a tracked .gitattributes makes sense. Of course you also need some local settings (e.g. in .git/config) to use that information. Perhaps something like: [filetype "text*"] AutoCRLF = yes [filetype "text/xml"] Merge = xml-merge -whatever -- Martin Waitz ``` ## Johannes Schindelin, 2007-02-21 12:55 Subject: Re: Unresolved issues Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0702211348060.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <20070221104209.GM21842@admingilde.org> ``` Hi, On Wed, 21 Feb 2007, Martin Waitz wrote: > On Wed, Feb 21, 2007 at 01:39:48AM +0100, Johannes Schindelin wrote: > > In that case, .gitattributes (I mean a tracked one) would be wrong, > > wrong, wrong. > > I don't think so. What you conveniently "forgot" to quote was the case: if we want this to decide on when to use crlf<->lf transformation, we should decide that locally. But you are probably right: the information if a file _is_ fair game for crlf munging is probably something we might want to _be able_ to have tracked. BUT there are a whole lot of problems with that approach, as Junio pointed out, like merging attributes files, like what to do if a file is not changed by a commit, but its attributes are, etc. So, why not make the autodetection really brilliant at first, and _if_ we hit a hard case which cannot be autodetect, _then_ add .gitattributes which should _only_ force settings on misdetected files? Ciao, Dscho ``` ## Robin Rosenberg, 2007-02-21 16:32 Subject: Re: Unresolved issues Message-ID: <200702211732.38268.robin.rosenberg.lists@dewire.com> URL: https://gitlist.dev/e/200702211732.38268.robin.rosenberg.lists%40dewire.com In-Reply-To: ``` onsdag 21 februari 2007 01:56 skrev Linus Torvalds: > > On Wed, 21 Feb 2007, Johannes Schindelin wrote: > > > > Um, I don't want to spoil the party, but was not the original idea of this > > auto-CRLF thing some sort of "emulation" of the CVS text checkout > > behaviour? > > > > In that case, .gitattributes (I mean a tracked one) would be wrong, wrong, > > wrong. > > > > It's a local setup if you want auto-CRLF or not. So, why not just make it > > a local setting (if in config or $GIT_DIR/info/gitattributes, I don't > > care) which shell patterns are to be transformed on input and/or output? > > That is a good point. We *could* just make it a ".git/config" issue, which > has the nice benefit that you can just set up some user-wide rules rather > than making it be per-repo. > > Of course, the config language may not be wonderful for this. But we could > certainly have something like > > [format "crlf"] > enable = true > text = *.[ch] > binary = *.jpg The decision whether to mangel at all shoule be local. Which files to mangle, if mangle is "on", should be a per version (not like CVS' setting for all versions). Otherwise it won't be propagated properly on push/pull, and people *will* get it wrong over and over. It it's .gitattributes or similar it can be merged as any other file and conflicts can be resolved like any other file. For efficiency you can have one .gitattributes. Hopefully it won't happen often because autodetection is soo good, but when it get's wrong it's important that it can be fixed and distributed properly. -- robin ``` ## Brian Gernhardt, 2007-02-21 16:57 Subject: Re: Unresolved issues Message-ID: <09D527A1-43E2-41A1-AC46-71F64BC409C2@silverinsanity.com> URL: https://gitlist.dev/e/09D527A1-43E2-41A1-AC46-71F64BC409C2%40silverinsanity.com In-Reply-To: ``` On Feb 21, 2007, at 7:55 AM, Johannes Schindelin wrote: > What you conveniently "forgot" to quote was the case: if we want > this to > decide on when to use crlf<->lf transformation, we should decide that > locally. It seems to me that a tracked .gitattributes file should have things like *.txt: text *.gif: binary *.[ch]: text And the .git/config should have [attribute "text"] mangle = crlf [attribute "binary"] merge = none The type of each file should be tracked, but what to do with each type is a local issue. Trying to merge the two is madness. ~~ Brian ``` ## Shawn O. Pearce, 2007-02-21 17:05 Subject: Re: Unresolved issues Message-ID: <20070221170539.GI25559@spearce.org> URL: https://gitlist.dev/e/20070221170539.GI25559%40spearce.org In-Reply-To: <09D527A1-43E2-41A1-AC46-71F64BC409C2@silverinsanity.com> ``` Brian Gernhardt wrote: > It seems to me that a tracked .gitattributes file should have things > like > > *.txt: text > *.gif: binary > *.[ch]: text > > And the .git/config should have > > [attribute "text"] > mangle = crlf > > [attribute "binary"] > merge = none > > The type of each file should be tracked, but what to do with each > type is a local issue. Trying to merge the two is madness. Yes, exactly. :-) I would also recommend that we encourage use of standard MIME types to define the file types, but don't enforce it. Thus I can setup something like: cat >.gitattributes <>.git/config < URL: https://gitlist.dev/e/7vmz36k33y.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <7virdx1e58.fsf@assigned-by-dhcp.cox.net> ``` This makes git-status work semi-decently in a read-only repository. Earlier, the command simply died with "cannot lock the index file" before giving any useful information to the user. Because index won't be updated in a read-only repository, stat-dirty paths appear in the "Changed but not updated" list. Signed-off-by: Junio C Hamano --- Junio C Hamano writes: > [gmane=http://thread.gmane.org/gmane.comp.version-control.git] > > * "git status" is not a read-only operation. > > It needs to do enough lstat(2) to run "update-index --refresh" to come > up with the information it needs to give. We could do so internally > without writing out the result to the index (there is a patch to do > this) even if a repository is not writable. > > $gmane/39205 > $gmane/39206 > > However, a big downside of this approach is that doing so > unconditionally would mean the expensive lstat(2) is wasted > afterwards. > > $gmane/39246 > > Currently an workaround is to run git-runstatus and live with the fact > that otherwise unmodified but stat-dirty paths to show up in the > output. I think (iff somebody feels strongly about it) a possible > compromise would be to see if we can update the index, and do what the > current code does if we can, and otherwise fall back on the new code > that does the internal "update-index --refresh". I did not feel strongly enough about it, so here is another approach. git-commit.sh | 21 +++++++++++---------- 1 files changed, 11 insertions(+), 10 deletions(-) diff --git a/git-commit.sh b/git-commit.sh index ec506d9..cfa1511 100755 --- a/git-commit.sh +++ b/git-commit.sh @@ -13,10 +13,10 @@ git-rev-parse --verify HEAD >/dev/null 2>&1 || initial_commit=t case "$0" in *status) status_only=t - unmerged_ok_if_status=--unmerged ;; + ;; *commit) status_only= - unmerged_ok_if_status= ;; + ;; esac refuse_partial () { @@ -389,16 +389,17 @@ else USE_INDEX="$THIS_INDEX" fi -GIT_INDEX_FILE="$USE_INDEX" \ - git-update-index -q $unmerged_ok_if_status --refresh || exit - -################################################################ -# If the request is status, just show it and exit. - -case "$0" in -*status) +case "$status_only" in +t) + # This will silently fail in a read-only repository, which is + # what we want. + GIT_INDEX_FILE="$USE_INDEX" git-update-index -q --unmerged --refresh run_status exit $? + ;; +'') + GIT_INDEX_FILE="$USE_INDEX" git-update-index -q --refresh || exit + ;; esac ################################################################ -- 1.5.0.1.619.g04c5c ``` ## Junio C Hamano, 2007-02-22 08:30 Subject: [PATCH] update-index: do not die too early in a read-only repository. Message-ID: <7vhctek30q.fsf_-_@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vhctek30q.fsf_-_%40assigned-by-dhcp.cox.net In-Reply-To: <7vmz36k33y.fsf@assigned-by-dhcp.cox.net> ``` This delays the error exit from hold_lock_file_for_update() in update-index, so that "update-index --refresh" in a read-only repository can still report what paths are stat-dirty before exiting. Also it makes -q to squelch the error message. Signed-off-by: Junio C Hamano --- builtin-update-index.c | 11 ++++++++++- 1 files changed, 10 insertions(+), 1 deletions(-) diff --git a/builtin-update-index.c b/builtin-update-index.c index 1ac613a..3fbdc67 100644 --- a/builtin-update-index.c +++ b/builtin-update-index.c @@ -486,6 +486,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) int prefix_length = prefix ? strlen(prefix) : 0; char set_executable_bit = 0; unsigned int refresh_flags = 0; + int lock_error = 0; struct lock_file *lock_file; git_config(git_default_config); @@ -493,7 +494,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) /* We can't free this memory, it becomes part of a linked list parsed atexit() */ lock_file = xcalloc(1, sizeof(struct lock_file)); - newfd = hold_lock_file_for_update(lock_file, get_index_file(), 1); + newfd = hold_lock_file_for_update(lock_file, get_index_file(), 0); + if (newfd < 0) + lock_error = errno; entries = read_cache(); if (entries < 0) @@ -650,6 +653,12 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) finish: if (active_cache_changed) { + if (newfd < 0) { + if (refresh_flags & REFRESH_QUIET) + exit(128); + die("unable to create '%s.lock': %s", + get_index_file(), strerror(lock_error)); + } if (write_cache(newfd, active_cache, active_nr) || close(newfd) || commit_lock_file(lock_file)) die("Unable to write new index file"); -- 1.5.0.1.619.g04c5c ``` ## Julian Phillips, 2007-02-26 01:33 Subject: Re: Unresolved issues Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0702260112160.12555%40beast.quantumfyre.co.uk In-Reply-To: <7virdx1e58.fsf@assigned-by-dhcp.cox.net> ``` On Mon, 19 Feb 2007, Junio C Hamano wrote: > * "git fetch" between repositories with hundreds of refs. > > $gmane/39330 > > There are partial rewrite of the most expensive parts of git-fetch in > C parked in 'pu'. It might be good enough for public consumption > without going the whole nine yards. I dunno. I am not very keen on > rewriting all of "git fetch" in C right now, as people seem to be > still interested in touching it (including "git bundle" topic). The current changes in jc/fetch take things from "unusable" to "a bit slow", which I think could quite easily be considered a separate task from "a bit slow" to "something that even Linus would consider reasonable". So my opinion would be to get the current improvements in so that they can be combined with the other good work happening in this area, and wait for things to settle before going the last mile (after all anyone converting from Subversion or CVS probably won't find 30s to be slow anyway ... ;)). I would be happy to work on this if you would rather spend your time on more generally useful things ... -- Julian --- Stewie Griffin: [thinks] How wonderful it will be to have mother back! Brian Griffin: [thinks] I heard that. Stewie Griffin: [thinks] Damn! ``` ## Junio C Hamano, 2007-02-26 03:39 Subject: Re: Unresolved issues Message-ID: <7vtzx9oaeb.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vtzx9oaeb.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Julian Phillips writes: > On Mon, 19 Feb 2007, Junio C Hamano wrote: > >> * "git fetch" between repositories with hundreds of refs. >> >> $gmane/39330 >> >> There are partial rewrite of the most expensive parts of git-fetch in >> C parked in 'pu'. It might be good enough for public consumption >> without going the whole nine yards. I dunno. I am not very keen on >> rewriting all of "git fetch" in C right now, as people seem to be >> still interested in touching it (including "git bundle" topic). > > The current changes in jc/fetch take things from "unusable" to "a bit > slow", which I think could quite easily be considered a separate task > from "a bit slow" to "something that even Linus would consider > reasonable". So my opinion would be to get the current improvements > in so that they can be combined with the other good work happening in > this area, and wait for things to settle before going the last mile > (after all anyone converting from Subversion or CVS probably won't > find 30s to be slow anyway ... ;)). I was kind of waiting for dust from Santi's code shuffling to settle down, because the series moderately conflicts with it. I wanted to take Santi's patch first as it was supposed to be a clean-up without any functionality changes, although it was kind of painful to really make sure there is no regression. If what jc/fetch topic tries to do helps real users, let's merge it in 'next' first, as Santi's change is not supposed to bring any improvements by itself even when it proves regression-free. In the short term, this means we have to ask Santi to rebase his patch instead of the other way around as I planned first, which is a bit unfortunate. ``` ## Julian Phillips, 2007-02-26 05:10 Subject: Re: Unresolved issues Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0702260455020.16708%40beast.quantumfyre.co.uk In-Reply-To: <7vtzx9oaeb.fsf@assigned-by-dhcp.cox.net> ``` On Sun, 25 Feb 2007, Junio C Hamano wrote: > Julian Phillips writes: > >> On Mon, 19 Feb 2007, Junio C Hamano wrote: >> >>> * "git fetch" between repositories with hundreds of refs. >>> >>> $gmane/39330 >>> >>> There are partial rewrite of the most expensive parts of git-fetch in >>> C parked in 'pu'. It might be good enough for public consumption >>> without going the whole nine yards. I dunno. I am not very keen on >>> rewriting all of "git fetch" in C right now, as people seem to be >>> still interested in touching it (including "git bundle" topic). >> >> The current changes in jc/fetch take things from "unusable" to "a bit >> slow", which I think could quite easily be considered a separate task >> from "a bit slow" to "something that even Linus would consider >> reasonable". So my opinion would be to get the current improvements >> in so that they can be combined with the other good work happening in >> this area, and wait for things to settle before going the last mile >> (after all anyone converting from Subversion or CVS probably won't >> find 30s to be slow anyway ... ;)). > > I was kind of waiting for dust from Santi's code shuffling to > settle down, because the series moderately conflicts with it. I > wanted to take Santi's patch first as it was supposed to be a > clean-up without any functionality changes, although it was kind > of painful to really make sure there is no regression. Indeed. I was thinking pretty much the same. It seems unnecessary to make Santi rebase his patch without any evidence that the jc/fetch topic is actually urgently needed by anyone. I was advocating a two step approach, but I didn't mean to give the impressions that I wanted the topic merged now. I envisioned both steps as being after the current active work that is affecting git-fetch. Thanks to the power of git I have got myself a master+jc/fetch branch so I'm happy - so unless there is anyone else out there working with stupidly large numbers of refs I'm quite happy to let Santi's work go in first. I'll even have a go at rebasing the fetch improvements ontop of Santi's work ... > > If what jc/fetch topic tries to do helps real users, let's merge > it in 'next' first, as Santi's change is not supposed to bring > any improvements by itself even when it proves regression-free. > > In the short term, this means we have to ask Santi to rebase his > patch instead of the other way around as I planned first, which > is a bit unfortunate. Unless there is someone else out there that wants jc/fetch badly, I would say "after you" ... -- Julian --- This report is filled with omissions. ``` ## Junio C Hamano, 2007-02-26 05:33 Subject: Re: Unresolved issues Message-ID: <7v8xelo52x.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v8xelo52x.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Julian Phillips writes: >> I was kind of waiting for dust from Santi's code shuffling to >> settle down, because the series moderately conflicts with it. I >> wanted to take Santi's patch first as it was supposed to be a >> clean-up without any functionality changes, although it was kind >> of painful to really make sure there is no regression. > > Indeed. I was thinking pretty much the same. It seems unnecessary to > make Santi rebase his patch without any evidence that the jc/fetch > topic is actually urgently needed by anyone. Sorry, too late --- it's a done deal. Partial C rewrite of git-fetch is now in 'next'. ``` ## Andy Parkins, 2007-02-27 12:48 Subject: [PATCH 1/2] Make 'cvs ci' lockless in git-cvsserver by using git-update-ref Message-ID: <200702271248.59652.andyparkins@gmail.com> URL: https://gitlist.dev/e/200702271248.59652.andyparkins%40gmail.com In-Reply-To: <200702210908.59579.andyparkins@gmail.com> ``` This makes "ci" codepath lockless by following the usual "remember the tip, do your thing, then compare and swap at the end" update pattern using update-ref. Incidentally, by updating the code that reads where the tip of the head is to use show-ref, it makes it safe to use in a repository whose refs are pack-pruned. I noticed that other parts of the program are not yet pack-refs safe, but tried to keep the changes to the minimum. Signed-off-by: Junio C Hamano --- This patch is actually yours (with one extra removal of lock file reference that you'd missed, and a change of shortlog), but I don't know how to send an email that comes from me but attributes authorship to you. git-cvsserver.perl | 43 ++++++++++++++++--------------------------- 1 files changed, 16 insertions(+), 27 deletions(-) diff --git a/git-cvsserver.perl b/git-cvsserver.perl index 84520e7..8e12f81 100755 --- a/git-cvsserver.perl +++ b/git-cvsserver.perl @@ -1031,36 +1031,35 @@ sub req_ci exit; } - my $lockfile = "$state->{CVSROOT}/refs/heads/$state->{module}.lock"; - unless ( sysopen(LOCKFILE,$lockfile,O_EXCL|O_CREAT|O_WRONLY) ) - { - $log->warn("lockfile '$lockfile' already exists, please try again"); - print "error 1 Lock file '$lockfile' already exists, please try again\n"; - exit; - } - # Grab a handle to the SQLite db and do any necessary updates my $updater = GITCVS::updater->new($state->{CVSROOT}, $state->{module}, $log); $updater->update(); my $tmpdir = tempdir ( DIR => $TEMP_DIR ); my ( undef, $file_index ) = tempfile ( DIR => $TEMP_DIR, OPEN => 0 ); - $log->info("Lock successful, basing commit on '$tmpdir', index file is '$file_index'"); + $log->info("Lockless commit start, basing commit on '$tmpdir', index file is '$file_index'"); $ENV{GIT_DIR} = $state->{CVSROOT} . "/"; $ENV{GIT_INDEX_FILE} = $file_index; + # Remember where the head was at the beginning. + my $parenthash = `git show-ref -s refs/heads/$state->{module}`; + chomp $parenthash; + if ($parenthash !~ /^[0-9a-f]{40}$/) { + print "error 1 pserver cannot find the current HEAD of module"; + exit; + } + chdir $tmpdir; # populate the temporary index based - system("git-read-tree", $state->{module}); + system("git-read-tree", $parenthash); unless ($? == 0) { die "Error running git-read-tree $state->{module} $file_index $!"; } $log->info("Created index '$file_index' with for head $state->{module} - exit status $?"); - my @committedfiles = (); # foreach file specified on the command line ... @@ -1095,8 +1094,6 @@ sub req_ci { # fail everything if an up to date check fails print "error 1 Up to date check failed for $filename\n"; - close LOCKFILE; - unlink($lockfile); chdir "/"; exit; } @@ -1139,16 +1136,12 @@ sub req_ci { print "E No files to commit\n"; print "ok\n"; - close LOCKFILE; - unlink($lockfile); chdir "/"; return; } my $treehash = `git-write-tree`; - my $parenthash = `cat $ENV{GIT_DIR}refs/heads/$state->{module}`; chomp $treehash; - chomp $parenthash; $log->debug("Treehash : $treehash, Parenthash : $parenthash"); @@ -1165,8 +1158,6 @@ sub req_ci { $log->warn("Commit failed (Invalid commit hash)"); print "error 1 Commit failed (unknown reason)\n"; - close LOCKFILE; - unlink($lockfile); chdir "/"; exit; } @@ -1179,14 +1170,17 @@ sub req_ci { $log->warn("Commit failed (update hook declined to update ref)"); print "error 1 Commit failed (update hook declined)\n"; - close LOCKFILE; - unlink($lockfile); chdir "/"; exit; } } - print LOCKFILE $commithash; + if (system(qw(git update-ref -m), "cvsserver ci", + "refs/heads/$state->{module}", $commithash, $parenthash)) { + $log->warn("update-ref for $state->{module} failed."); + print "error 1 Cannot commit -- update first\n"; + exit; + } $updater->update(); @@ -1215,12 +1209,7 @@ sub req_ci } } - close LOCKFILE; - my $reffile = "$ENV{GIT_DIR}refs/heads/$state->{module}"; - unlink($reffile); - rename($lockfile, $reffile); chdir "/"; - print "ok\n"; } -- 1.5.0.2.778.gdcb06 -- Dr Andy Parkins, M Eng (hons), MIET andyparkins@gmail.com ``` ## Andy Parkins, 2007-02-27 12:49 Subject: [PATCH 2/2] cvsserver: Remove trailing "\n" from commithash in checkin function Message-ID: <200702271249.09596.andyparkins@gmail.com> URL: https://gitlist.dev/e/200702271249.09596.andyparkins%40gmail.com In-Reply-To: <200702210908.59579.andyparkins@gmail.com> ``` The commithash for updating the ref is obtained from a call to git-commit-tree. However, it was returned (and stored) with the trailing newline. This meant that the later call to git-update-ref that was trying to update to $commithash was including the newline in the parameter - obviously that hash would never exist, and so git-update-ref would always fail. The solution is to chomp() the commithash as soon as it is returned by git-commit-tree. Signed-off-by: Andy Parkins --- git-cvsserver.perl | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/git-cvsserver.perl b/git-cvsserver.perl index 8e12f81..f4b8bd2 100755 --- a/git-cvsserver.perl +++ b/git-cvsserver.perl @@ -1152,6 +1152,7 @@ sub req_ci close $msg_fh; my $commithash = `git-commit-tree $treehash -p $parenthash < $msg_filename`; + chomp($commithash); $log->info("Commit hash : $commithash"); unless ( $commithash =~ /[a-zA-Z0-9]{40}/ ) -- 1.5.0.2.778.gdcb06 ``` ## Jakub Narebski, 2007-02-27 13:55 Subject: Re: [PATCH 1/2] Make 'cvs ci' lockless in git-cvsserver by using git-update-ref Message-ID: URL: https://gitlist.dev/e/es1d3n%24kt6%241%40sea.gmane.org In-Reply-To: <200702271248.59652.andyparkins@gmail.com> ``` Andy Parkins wrote: > --- > This patch is actually yours (with one extra removal of lock file reference > that you'd missed, and a change of shortlog), but I don't know how to send > an email that comes from me but attributes authorship to you. You just leave From: header in the body of message... -- Jakub Narebski Warsaw, Poland ShadeHawk on #git ``` ## Nicolas Pitre, 2007-02-27 14:35 Subject: Re: [PATCH 1/2] Make 'cvs ci' lockless in git-cvsserver by using git-update-ref Message-ID: URL: https://gitlist.dev/e/alpine.LRH.0.82.0702270933060.29426%40xanadu.home In-Reply-To: <200702271248.59652.andyparkins@gmail.com> ``` On Tue, 27 Feb 2007, Andy Parkins wrote: > This patch is actually yours (with one extra removal of lock file reference > that you'd missed, and a change of shortlog), but I don't know how to send > an email that comes from me but attributes authorship to you. Start your email body with a "From: " and the address of the person. Nicolas ``` ## Johannes Schindelin, 2007-02-27 20:10 Subject: Re: Unresolved issues Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0702272107560.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: ``` Hi, On Mon, 26 Feb 2007, Julian Phillips wrote: > On Mon, 19 Feb 2007, Junio C Hamano wrote: > > > * "git fetch" between repositories with hundreds of refs. > > I would be happy to work on this if you would rather spend your time on > more generally useful things ... I would be grateful indeed if you worked on a builtin fetch. But beware, I think it is quite some work... The good thing: We have some tests involving git-fetch, so if all the tests pass, chances are that the builtin fetch works correctly. (Of course, we do not have tests for HTTP, SSH and GIT fetch... Anyone?) Ciao, Dscho ``` ## Martin Langhoff, 2007-02-27 23:37 Subject: Re: [PATCH] Use git-update-ref to update a ref during commit in git-cvsserver Message-ID: <45E4C0BC.5090506@catalyst.net.nz> URL: https://gitlist.dev/e/45E4C0BC.5090506%40catalyst.net.nz In-Reply-To: <7vmz38t5r4.fsf@assigned-by-dhcp.cox.net> ``` Hi Junio, Andy, sorry for the long delay, I'm catching up with a sizable backlog at work and in my foss projects. I like Junio's patch -- it fixes cvsserver to work with packed refs (which needed to be done!) and does things lockless, which is a great bonus. The meat of the matter is Junio C Hamano wrote: > - print LOCKFILE $commithash; > + if (system(qw(git update-ref -m), "cvsserver ci", > + "refs/heads/$state->{module}", $commithash, $parenthash)) { > + $log->warn("update-ref for $state->{module} failed."); > + print "error 1 Cannot commit -- update first\n"; > + exit; > + } > > $updater->update(); Running the commit lockless makes it a little bit more likely that we'll fail the commit after all files have been sent. Some older CVS clients have broken error handling in the late stages of the commit, but we cannot really fix that - and such cvs clients are so broken that they probably don't deserve our attention. The other area I checked is that we don't get a nasty race condition between the update-ref and calling $updater->update() - but it is safe. So ack from this corner and thanks for the patch! cheers, martin -- ----------------------------------------------------------------------- Martin @ Catalyst .Net .NZ Ltd, PO Box 11-053, Manners St, Wellington WEB: http://catalyst.net.nz/ PHYS: Level 2, 150-154 Willis St OFFICE: +64(4)916-7224 UK: 0845 868 5733 ext 7224 MOB: +64(21)364-017 Make things as simple as possible, but no simpler - Einstein ----------------------------------------------------------------------- ``` ## Junio C Hamano, 2007-02-27 23:44 Subject: Re: [PATCH 1/2] Make 'cvs ci' lockless in git-cvsserver by using git-update-ref Message-ID: <7v1wkbb1yf.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v1wkbb1yf.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <200702271248.59652.andyparkins@gmail.com> ``` Andy Parkins writes: > This patch is actually yours (with one extra removal of lock file reference > that you'd missed, and a change of shortlog), but I don't know how to send > an email that comes from me but attributes authorship to you. The extra one was introduced by a later patch to honor the update hook since I wrote the original patch you forward ported. Thanks. Will apply. ``` ## Junio C Hamano, 2007-02-27 23:45 Subject: Re: [PATCH 2/2] cvsserver: Remove trailing "\n" from commithash in checkin function Message-ID: <7vwt239nbm.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vwt239nbm.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <200702271249.09596.andyparkins@gmail.com> ``` Andy Parkins writes: > The commithash for updating the ref is obtained from a call to > git-commit-tree. However, it was returned (and stored) with the > trailing newline. This meant that the later call to git-update-ref that > was trying to update to $commithash was including the newline in the > parameter - obviously that hash would never exist, and so git-update-ref > would always fail. > > The solution is to chomp() the commithash as soon as it is returned by > git-commit-tree. > > Signed-off-by: Andy Parkins > my $commithash = `git-commit-tree $treehash -p $parenthash < $msg_filename`; > + chomp($commithash); > $log->info("Commit hash : $commithash"); > Thanks. Do we need to compensate with a trailing LF in the $log line? ``` ## Andy Parkins, 2007-02-28 08:44 Subject: Re: [PATCH 1/2] Make 'cvs ci' lockless in git-cvsserver by using git-update-ref Message-ID: <200702280844.45949.andyparkins@gmail.com> URL: https://gitlist.dev/e/200702280844.45949.andyparkins%40gmail.com In-Reply-To: <7v1wkbb1yf.fsf@assigned-by-dhcp.cox.net> ``` On Tuesday 2007 February 27 23:44, Junio C Hamano wrote: > The extra one was introduced by a later patch to honor the > update hook since I wrote the original patch you forward ported. Apologies. I must have applied your patch in the wrong place on my branch. It did seem unlikely that you would have made a mistake. Andy -- Dr Andy Parkins, M Eng (hons), MIET andyparkins@gmail.com ``` ## Andy Parkins, 2007-02-28 08:45 Subject: Re: [PATCH 2/2] cvsserver: Remove trailing "\n" from commithash in checkin function Message-ID: <200702280845.42910.andyparkins@gmail.com> URL: https://gitlist.dev/e/200702280845.42910.andyparkins%40gmail.com In-Reply-To: <7vwt239nbm.fsf@assigned-by-dhcp.cox.net> ``` On Tuesday 2007 February 27 23:45, Junio C Hamano wrote: > Thanks. Do we need to compensate with a trailing LF in the $log > line? Interestingly, no. I checked that, and the log has always had an unnecessary blank line in it - which is of course now removed. Andy -- Dr Andy Parkins, M Eng (hons), MIET andyparkins@gmail.com ``` ## Junio C Hamano, 2007-02-28 18:06 Subject: Re: [PATCH 1/2] Make 'cvs ci' lockless in git-cvsserver by using git-update-ref Message-ID: <7v1wka2m3h.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v1wka2m3h.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <200702280844.45949.andyparkins@gmail.com> ``` Andy Parkins writes: > On Tuesday 2007 February 27 23:44, Junio C Hamano wrote: > >> The extra one was introduced by a later patch to honor the >> update hook since I wrote the original patch you forward ported. > > Apologies. I must have applied your patch in the wrong place on my branch. > It did seem unlikely that you would have made a mistake. Apologies if I sounded I was complaining -- not at all. I was _thanking_ you for the forward porting of my patch so I did not have to do that ;-). ```