* Re: [PATCH] Fix config lockfile handling.
From: Junio C Hamano @ 2007-12-14 21:57 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
In-Reply-To: <1197665998-32386-2-git-send-email-krh@redhat.com>
Kristian Høgsberg <krh@redhat.com> writes:
> When we commit or roll back the lock file the fd is automatically closed,
> so don't do that again.
With your change, we do not check the return status from close(2)
anymore, which means that we may have run out of diskspace without
noticing and renamed the incomplete file into the real place. Oops?
At least the original code wouldn't have had that problem.
The right fix in the longer term would be to check the return value from
the close(2) in commit_lock_file(), but it currently does not check on
purpose, because the callers may have already closed the fd.
^ permalink raw reply
* Re: [PATCH] provide advance warning of some future pack default changes
From: Joel Becker @ 2007-12-14 21:52 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Jakub Narebski, Junio C Hamano, git
In-Reply-To: <alpine.LFD.0.999999.0712140836140.8467@xanadu.home>
On Fri, Dec 14, 2007 at 08:38:51AM -0500, Nicolas Pitre wrote:
> On Fri, 14 Dec 2007, Jakub Narebski wrote:
> > Which means what? Local clone with shortcut (hardlinking and remotes)?
> > Dumb protocols (http, ftp, rsync)?
>
> Right, or simply shared repo over NFS or the like.
>
> The 1.5.5 release notes will contain a note reminding people to set the
> corresponding config variables if they wish to retain the legacy
> behaviors.
We've seen that release notes are a poor way to communicate
this. What will happen to a 1.4.4 user when they try to access the
repository? Corruption, cryptic error message, or clean "this repo is
not compatible" message?
Joel
--
"Depend on the rabbit's foot if you will, but remember, it didn't
help the rabbit."
- R. E. Shay
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127
^ permalink raw reply
* Re: testsuite failures in mainline...
From: Alex Riesen @ 2007-12-14 21:45 UTC (permalink / raw)
To: David Miller; +Cc: gitster, git
In-Reply-To: <20071214.111736.258936000.davem@davemloft.net>
David Miller, Fri, Dec 14, 2007 20:17:36 +0100:
> ++ git show-ref -q refs/remotes/local/master
> ++ git branch my3 local/master
> fatal: Out of memory, malloc failed
Something unusual about the system? Like a malloc debugger in
LD_PRELOAD configuration?
Maybe you could retry with a little bit instrumentation?
(The program last failed (git-branch) is normally very benign...)
Something like this:
diff --git a/git-compat-util.h b/git-compat-util.h
index 79eb10e..a9cc249 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -230,7 +230,8 @@ static inline char* xstrdup(const char *str)
return ret;
}
-static inline void *xmalloc(size_t size)
+#define xmalloc(size) xmalloc_((size),__FILE__,__LINE__)
+static inline void *xmalloc_(size_t size, const char *file, int line)
{
void *ret = malloc(size);
if (!ret && !size)
@@ -241,7 +242,8 @@ static inline void *xmalloc(size_t size)
if (!ret && !size)
ret = malloc(1);
if (!ret)
- die("Out of memory, malloc failed");
+ die("Out of memory, malloc(%u) at %s:%d failed",
+ size, file, line);
}
#ifdef XMALLOC_POISON
memset(ret, 0xA5, size);
@@ -263,7 +265,8 @@ static inline char *xstrndup(const char *str, size_t len)
return xmemdupz(str, p ? p - str : len);
}
-static inline void *xrealloc(void *ptr, size_t size)
+#define xrealloc(ptr,size) xrealloc_((ptr),(size),__FILE__,__LINE__)
+static inline void *xrealloc_(void *ptr, size_t size, const char *file, int line)
{
void *ret = realloc(ptr, size);
if (!ret && !size)
@@ -274,7 +277,8 @@ static inline void *xrealloc(void *ptr, size_t size)
if (!ret && !size)
ret = realloc(ptr, 1);
if (!ret)
- die("Out of memory, realloc failed");
+ die("Out of memory, realloc(%u) at %s:%d failed",
+ size, file, line);
}
return ret;
}
^ permalink raw reply related
* Re: [PATCH 1/2] Fix config lockfile handling.
From: Junio C Hamano @ 2007-12-14 21:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Kristian Høgsberg, git
In-Reply-To: <Pine.LNX.4.64.0712142015240.27959@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > AFAICT this cannot work. At least not reliably. An atexit() handler
>> > will access all (even closed) lockfiles.
>>
>> Correct. It cannot be on the stack.
>
> Note that this behaviour will be another obstacle to libification.
By your definition of "obstacle", there is no work at all in
libification if the system is obstacle free.
Libification is all about removing run-once-and-exit and atexit() is
just a part of it.
I think we can do this step-by-step by first introducing a new function
"get_lockfile()" that takes a list of active lockfiles (perhaps that
would be a part of "client context" thing in the library) and gives back
a lockfile structure allocated on heap, registers it to the list of
lockfiles that need to be eventually cleaned up, and another function
"rollback_lockfiles()" that take the list of lockfiles in the "client
context" and rolls them all back. Once there is such a "client contex",
in the current unlibified "main" routines can then declare a global
client context, obtain and use lockfiles for that context, and directly
call rollback_lockfiles() from the atexit() handler.
But that is all post 1.5.4.
^ permalink raw reply
* [PATCH 2/2] Fix random sha1 in error message in http-fetch and http-push
From: Mike Hommey @ 2007-12-14 21:18 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <1197667081-9909-1-git-send-email-mh@glandium.org>
When a downloaded ref doesn't contain a sha1, the error message displays
a random sha1 because of uninitialized memory. This happens when cloning
a repository that is already a clone of another one, in which case
refs/remotes/origin/HEAD is a symref.
Signed-off-by: Mike Hommey <mh@glandium.org>
---
Please note that this is already fixed in my strbuf patch for these files,
which had been applied in pu, but it seems to have disappeared from pu's
history. This also means the strbuf patch conflicts with this one. Please
tell me if you want a new strbuf patch made after this one.
http-push.c | 5 +++--
http-walker.c | 5 +++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/http-push.c b/http-push.c
index fc60bfd..7f1d043 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1149,9 +1149,10 @@ int fetch_ref(char *ref, unsigned char *sha1)
return error("Unable to start request");
}
+ if (buffer.posn != 41)
+ return 1;
hex[40] = '\0';
- get_sha1_hex(hex, sha1);
- return 0;
+ return get_sha1_hex(hex, sha1);
}
static void one_remote_object(const char *hex)
diff --git a/http-walker.c b/http-walker.c
index a3fb596..68b5108 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -986,9 +986,10 @@ static int fetch_ref(struct walker *walker, char *ref, unsigned char *sha1)
return error("Unable to start request");
}
+ if (buffer.posn != 41)
+ return 1;
hex[40] = '\0';
- get_sha1_hex(hex, sha1);
- return 0;
+ return get_sha1_hex(hex, sha1);
}
static void cleanup(struct walker *walker)
--
1.5.4.rc0.8.gbf4af-dirty
^ permalink raw reply related
* [PATCH 1/2] Fix some more memory leaks in http-push.c
From: Mike Hommey @ 2007-12-14 21:18 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
Signed-off-by: Mike Hommey <mh@glandium.org>
---
http-push.c | 13 +++++++++++--
1 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/http-push.c b/http-push.c
index fffbe9c..fc60bfd 100644
--- a/http-push.c
+++ b/http-push.c
@@ -925,11 +925,14 @@ static int fetch_index(unsigned char *sha1)
hex);
}
} else {
+ free(url);
return error("Unable to start request");
}
- if (has_pack_index(sha1))
+ if (has_pack_index(sha1)) {
+ free(url);
return 0;
+ }
if (push_verbosely)
fprintf(stderr, "Getting index for pack %s\n", hex);
@@ -939,9 +942,11 @@ static int fetch_index(unsigned char *sha1)
filename = sha1_pack_index_name(sha1);
snprintf(tmpfile, sizeof(tmpfile), "%s.temp", filename);
indexfile = fopen(tmpfile, "a");
- if (!indexfile)
+ if (!indexfile) {
+ free(url);
return error("Unable to open local file %s for pack index",
tmpfile);
+ }
slot = get_active_slot();
slot->results = &results;
@@ -1135,10 +1140,12 @@ int fetch_ref(char *ref, unsigned char *sha1)
curl_easy_setopt(slot->curl, CURLOPT_URL, url);
if (start_active_slot(slot)) {
run_active_slot(slot);
+ free(url);
if (results.curl_result != CURLE_OK)
return error("Couldn't get %s for %s\n%s",
url, ref, curl_errorstr);
} else {
+ free(url);
return error("Unable to start request");
}
@@ -2107,6 +2114,7 @@ static int remote_exists(const char *path)
if (start_active_slot(slot)) {
run_active_slot(slot);
+ free(url);
if (results.http_code == 404)
return 0;
else if (results.curl_result == CURLE_OK)
@@ -2114,6 +2122,7 @@ static int remote_exists(const char *path)
else
fprintf(stderr, "HEAD HTTP error %ld\n", results.http_code);
} else {
+ free(url);
fprintf(stderr, "Unable to start HEAD request\n");
}
--
1.5.4.rc0.8.gbf4af-dirty
^ permalink raw reply related
* Re: [PATCH] Authentication support for pserver
From: Ævar Arnfjörð Bjarmason @ 2007-12-14 21:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, martyn, martin
In-Reply-To: <7vir31u210.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 181 bytes --]
Junio C Hamano <gitster@pobox.com> writes:
> Looks good. I'll queue only so that I won't lose it and wait for Acks
> from Mart[iy]ns. Please sign off your patch.
Will this do?
[-- Attachment #2: signed-off patch to git-cvsserver --]
[-- Type: text/plain, Size: 6200 bytes --]
Signed-off-by: Ævar Arnfjörð Bjarmason <avar@cpan.org>
---
Documentation/git-cvsserver.txt | 21 +++++++++--
git-cvsserver.perl | 74 ++++++++++++++++++++++++++++++++++----
2 files changed, 83 insertions(+), 12 deletions(-)
diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt
index 258a62f..5ca84fc 100644
--- a/Documentation/git-cvsserver.txt
+++ b/Documentation/git-cvsserver.txt
@@ -69,9 +69,6 @@ plugin. Most functionality works fine with both of these clients.
LIMITATIONS
-----------
-Currently cvsserver works over SSH connections for read/write clients, and
-over pserver for anonymous CVS access.
-
CVS clients cannot tag, branch or perform GIT merges.
git-cvsserver maps GIT branches to CVS modules. This is very different
@@ -81,7 +78,7 @@ one or more directories.
INSTALLATION
------------
-1. If you are going to offer anonymous CVS access via pserver, add a line in
+1. If you are going to offer CVS access via pserver, add a line in
/etc/inetd.conf like
+
--
@@ -98,6 +95,22 @@ looks like
cvspserver stream tcp nowait nobody /usr/bin/git-cvsserver git-cvsserver pserver
------
+
+Only anonymous access is provided by pserve by default. To commit you
+will have to create pserver accounts, simply add a [gitcvs.users]
+section to the repositories you want to access, for example:
+
+------
+
+ [gitcvs.users]
+ someuser = somepassword
+ otheruser = otherpassword
+
+------
+Then provide your password via the pserver method, for example:
+------
+ cvs -d:pserver:someuser:somepassword@server/path/repo.git co <HEAD_name>
+------
No special setup is needed for SSH access, other than having GIT tools
in the PATH. If you have clients that do not accept the CVS_SERVER
environment variable, you can rename git-cvsserver to cvs.
diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index ecded3b..6f08bed 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -150,12 +150,35 @@ if ($state->{method} eq 'pserver') {
exit 1;
}
$line = <STDIN>; chomp $line;
- unless ($line eq 'anonymous') {
- print "E Only anonymous user allowed via pserver\n";
- print "I HATE YOU\n";
- exit 1;
+ my $user = $line;
+ $line = <STDIN>; chomp $line;
+ my $password = $line;
+
+ unless ($user eq 'anonymous') {
+ # Trying to authenticate a user
+ if (not exists $cfg->{gitcvs}->{users}) {
+ print "E the repo config file needs a [gitcvs.users] section with user/password key-value pairs\n";
+ print "I HATE YOU\n";
+ exit 1;
+ } elsif (exists $cfg->{gitcvs}->{users} and not exists $cfg->{gitcvs}->{users}->{$user}) {
+ #print "E the repo config file has a [gitcvs.users] section but the user $user is not defined in it\n";
+ print "I HATE YOU\n";
+ exit 1;
+ } else {
+ my $descrambled_password = descramble($password);
+ my $cleartext_password = $cfg->{gitcvs}->{users}->{$user};
+ if ($descrambled_password ne $cleartext_password) {
+ #print "E The password supplied for user $user was incorrect\n";
+ print "I HATE YOU\n";
+ exit 1;
+ }
+ # else fall through to LOVE
+ }
}
- $line = <STDIN>; chomp $line; # validate the password?
+
+ # For checking whether the user is anonymous on commit
+ $state->{user} = $user;
+
$line = <STDIN>; chomp $line;
unless ($line eq "END $request REQUEST") {
die "E Do not understand $line -- expecting END $request REQUEST\n";
@@ -273,7 +296,7 @@ sub req_Root
}
foreach my $line ( @gitvars )
{
- next unless ( $line =~ /^(gitcvs)\.(?:(ext|pserver)\.)?([\w-]+)=(.*)$/ );
+ next unless ( $line =~ /^(gitcvs)\.(?:(ext|pserver|users)\.)?([\w-]+)=(.*)$/ );
unless ($2) {
$cfg->{$1}{$3} = $4;
} else {
@@ -1176,9 +1199,9 @@ sub req_ci
$log->info("req_ci : " . ( defined($data) ? $data : "[NULL]" ));
- if ( $state->{method} eq 'pserver')
+ if ($state->{method} eq 'pserver' and $state->{user} eq 'anonymous')
{
- print "error 1 pserver access cannot commit\n";
+ print "error 1 anonymous user cannot commit via pserver\n";
exit;
}
@@ -2107,6 +2130,41 @@ sub kopts_from_path
}
}
+
+sub descramble
+{
+ # This table is from src/scramble.c in the CVS source
+ my @SHIFTS = (
+ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,
+ 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31,
+ 114,120, 53, 79, 96,109, 72,108, 70, 64, 76, 67,116, 74, 68, 87,
+ 111, 52, 75,119, 49, 34, 82, 81, 95, 65,112, 86,118,110,122,105,
+ 41, 57, 83, 43, 46,102, 40, 89, 38,103, 45, 50, 42,123, 91, 35,
+ 125, 55, 54, 66,124,126, 59, 47, 92, 71,115, 78, 88,107,106, 56,
+ 36,121,117,104,101,100, 69, 73, 99, 63, 94, 93, 39, 37, 61, 48,
+ 58,113, 32, 90, 44, 98, 60, 51, 33, 97, 62, 77, 84, 80, 85,223,
+ 225,216,187,166,229,189,222,188,141,249,148,200,184,136,248,190,
+ 199,170,181,204,138,232,218,183,255,234,220,247,213,203,226,193,
+ 174,172,228,252,217,201,131,230,197,211,145,238,161,179,160,212,
+ 207,221,254,173,202,146,224,151,140,196,205,130,135,133,143,246,
+ 192,159,244,239,185,168,215,144,139,165,180,157,147,186,214,176,
+ 227,231,219,169,175,156,206,198,129,164,150,210,154,177,134,127,
+ 182,128,158,208,162,132,167,209,149,241,153,251,237,236,171,195,
+ 243,233,253,240,194,250,191,155,142,137,245,235,163,242,178,152
+ );
+ my ($str) = @_;
+
+ # This should never happen, the same password format (A) bas been
+ # used by CVS since the beginning of time
+ $str =~ s/^(.)//;
+ die "invalid password format $1" unless $1 eq 'A';
+
+ $str =~ s/(.)/chr $SHIFTS[ord $1]/ge;
+
+ return $str;
+}
+
+
package GITCVS::log;
####
--
1.5.3.6.gea559
^ permalink raw reply related
* Re: [PATCH] Don't use the pager when running "git diff --check"
From: Junio C Hamano @ 2007-12-14 20:54 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: Jeff King, git
In-Reply-To: <26AEA0A9-9F15-4245-9D27-61050DA57E6F@wincent.com>
Wincent Colaiuta <win@wincent.com> writes:
> El 14/12/2007, a las 6:11, Junio C Hamano escribió:
>
>> You are right. While I do not personally miss paging output, it is a
>> regression not to page --check output by default.
>
> I thought this was ok because "git diff --exit-code" also produces
> useful output and also turns off the pager.
It is different. --exit-code was that way from day one. The primary
use of --check has been (and I suspect it will continue to be) for
people to _view_ the diff, spot problems so that they can fix them up,
and for that use case, exit code does not matter but pageability does.
You are introducing a new behaviour and a new use case --- that does not
give you a license to break other existing use cases.
^ permalink raw reply
* Re: [Funky] "git -p cmd" inside a bare repository
From: Johannes Schindelin @ 2007-12-14 20:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <7vejdpqbbh.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 899 bytes --]
Hi,
On Fri, 14 Dec 2007, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > I have no time left to work on git for a few days, so I cannot even
> > review your patch. But Jeff's patch being smaller, I could, and
> > AFAICT it solves the problem.
>
> We have more than a few days ;-) Hope you are feeling better now.
>
> You got familialized yourself with the work-tree part because the
> initial round was so broken and you had to step in during the last round
> (unfortuantely ;-), and Nguyễn also is familiar with that part of the
> code. It would be nice if we can have a proper fix that is not papering
> over deeper breakage.
Like always, you are correct.
Will look into it over the weekend (although I will only be able to report
back on Monday, due to the wonderful administrators who broke my internet
connection at home).
Ciao,
Dscho
^ permalink raw reply
* [PATCH] Use a strbuf for building up section header and key/value pair strings.
From: Kristian Høgsberg @ 2007-12-14 20:59 UTC (permalink / raw)
To: gitster; +Cc: git, Kristian Høgsberg
In-Reply-To: <1197665998-32386-2-git-send-email-krh@redhat.com>
Avoids horrible 1-byte write(2) calls and cleans up the logic a bit.
Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
config.c | 91 ++++++++++++++++++++++++++------------------------------------
1 files changed, 38 insertions(+), 53 deletions(-)
diff --git a/config.c b/config.c
index 2e52b17..4c8e15d 100644
--- a/config.c
+++ b/config.c
@@ -610,46 +610,36 @@ static int write_error(void)
static int store_write_section(int fd, const char* key)
{
- const char *dot = strchr(key, '.');
- int len1 = store.baselen, len2 = -1;
+ const char *dot;
+ int i, success;
+ struct strbuf sb;
- dot = strchr(key, '.');
+ strbuf_init(&sb, 0);
+ dot = memchr(key, '.', store.baselen);
if (dot) {
- int dotlen = dot - key;
- if (dotlen < len1) {
- len2 = len1 - dotlen - 1;
- len1 = dotlen;
+ strbuf_addf(&sb, "[%.*s \"", dot - key, key);
+ for (i = dot - key + 1; i < store.baselen; i++) {
+ if (key[i] == '"')
+ strbuf_addch(&sb, '\\');
+ strbuf_addch(&sb, key[i]);
}
+ strbuf_addstr(&sb, "\"]\n");
+ } else {
+ strbuf_addf(&sb, "[%.*s]\n", store.baselen, key);
}
- if (write_in_full(fd, "[", 1) != 1 ||
- write_in_full(fd, key, len1) != len1)
- return 0;
- if (len2 >= 0) {
- if (write_in_full(fd, " \"", 2) != 2)
- return 0;
- while (--len2 >= 0) {
- unsigned char c = *++dot;
- if (c == '"')
- if (write_in_full(fd, "\\", 1) != 1)
- return 0;
- if (write_in_full(fd, &c, 1) != 1)
- return 0;
- }
- if (write_in_full(fd, "\"", 1) != 1)
- return 0;
- }
- if (write_in_full(fd, "]\n", 2) != 2)
- return 0;
+ success = write_in_full(fd, sb.buf, sb.len) == sb.len;
+ strbuf_release(&sb);
- return 1;
+ return success;
}
static int store_write_pair(int fd, const char* key, const char* value)
{
- int i;
- int length = strlen(key+store.baselen+1);
- int quote = 0;
+ int i, success;
+ int length = strlen(key + store.baselen + 1);
+ const char *quote = "";
+ struct strbuf sb;
/*
* Check to see if the value needs to be surrounded with a dq pair.
@@ -659,43 +649,38 @@ static int store_write_pair(int fd, const char* key, const char* value)
* configuration parser.
*/
if (value[0] == ' ')
- quote = 1;
+ quote = "\"";
for (i = 0; value[i]; i++)
if (value[i] == ';' || value[i] == '#')
- quote = 1;
- if (i && value[i-1] == ' ')
- quote = 1;
+ quote = "\"";
+ if (i && value[i - 1] == ' ')
+ quote = "\"";
+
+ strbuf_init(&sb, 0);
+ strbuf_addf(&sb, "\t%.*s = %s",
+ length, key + store.baselen + 1, quote);
- if (write_in_full(fd, "\t", 1) != 1 ||
- write_in_full(fd, key+store.baselen+1, length) != length ||
- write_in_full(fd, " = ", 3) != 3)
- return 0;
- if (quote && write_in_full(fd, "\"", 1) != 1)
- return 0;
for (i = 0; value[i]; i++)
switch (value[i]) {
case '\n':
- if (write_in_full(fd, "\\n", 2) != 2)
- return 0;
+ strbuf_addstr(&sb, "\\n");
break;
case '\t':
- if (write_in_full(fd, "\\t", 2) != 2)
- return 0;
+ strbuf_addstr(&sb, "\\t");
break;
case '"':
case '\\':
- if (write_in_full(fd, "\\", 1) != 1)
- return 0;
+ strbuf_addch(&sb, '\\');
default:
- if (write_in_full(fd, value+i, 1) != 1)
- return 0;
+ strbuf_addch(&sb, value[i]);
break;
}
- if (quote && write_in_full(fd, "\"", 1) != 1)
- return 0;
- if (write_in_full(fd, "\n", 1) != 1)
- return 0;
- return 1;
+ strbuf_addf(&sb, "%s\n", quote);
+
+ success = write_in_full(fd, sb.buf, sb.len) == sb.len;
+ strbuf_release(&sb);
+
+ return success;
}
static ssize_t find_beginning_of_line(const char* contents, size_t size,
--
1.5.3.4
^ permalink raw reply related
* Re: [RFH] convert shortlog to use parse_options
From: Junio C Hamano @ 2007-12-14 20:34 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Jeff King, Kristian Høgsberg, git
In-Reply-To: <20071214083943.GA24475@artemis.madism.org>
Pierre Habouzit <madcoder@debian.org> writes:
> On Fri, Dec 14, 2007 at 05:59:52AM +0000, Junio C Hamano wrote:
>> git cmd --abbrev=10 -n=4
>
> actually -n=4 isn't understood atm, only -n4 and -n 4 are.
Ah, my mistake. And I do not think accepting -n=4 is a good idea (it is
not historically done).
After thinking about it a bit more, I think I was worried too much about
burdening the users to remember the differences between options with,
without and optional option-arguments [*1*]. They need to know the
difference between options with and without option-arguments already
because single letter options can be combined if they are without
option-arguments, and they have to write "shortlog -new72" but not
"shortlog -wen72". If they want to be extra sure, they can be more
explicit and say "shortlog -n -e -w72".
So let's go with the version you outlined --- options that take optional
option-arguments must get their option-arguments stuck to them, but
otherwise option-arguments can also be given as a separate word that
follows the option.
[Footnote]
*1* The fact some of our commands support options with optional
option-arguments is already against Guideline #7 in "12.2 Utility Syntax
Guidelines", so other POSIX guidelines are not useful for us in deciding
what behaviour to model after.
^ permalink raw reply
* Re: config.c fixes
From: Kristian Høgsberg @ 2007-12-14 20:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v3au5rrjv.fsf@gitster.siamese.dyndns.org>
On Fri, 2007-12-14 at 11:43 -0800, Junio C Hamano wrote:
> Kristian Høgsberg <krh@redhat.com> writes:
>
> > While strace'ing builtin-clone I saw this horror:
> > I think the best solution is to just parse up the entire config file
> > up front and keep it in a data structure, make the changes and then
> > write it all out at the end.
>
> Yeah, that was what I suggested a few times when other people have done
> config writing side, but without successfully getting past their skulls
> (it is not Linus's nor my code). It's about time somebody started to
> clean up that mess.
>
> The timing is a bit unfortunate, though. I would have preferred to have
> a week or so to cook this in 'next' before merging it part of -rc0.
Right, what I was describing above was more of a long term thing. I
sent two patches more suitable for 1.5.4 that fixes the double close and
the 1-byte writes in a less intrusive way. Except as the Johanneses
point out, I can't use the lock as a local variable, but must allocate
so the atexit handler doesn't break. If it has to be allocated, the API
should help/enforce that. I've sent out a couple of new patches that
fixes this.
Kristian
^ permalink raw reply
* config.c fixes, take 2
From: Kristian Høgsberg @ 2007-12-14 20:59 UTC (permalink / raw)
To: gitster; +Cc: git
Hi,
Here's a follow-up series that allocates the lock_file on the heap.
Also, git_config_rename_section() did an extra close too so I added
a fix for that in this series.
cheers,
Kristian
^ permalink raw reply
* [PATCH] Fix config lockfile handling.
From: Kristian Høgsberg @ 2007-12-14 20:59 UTC (permalink / raw)
To: gitster; +Cc: git, Kristian Høgsberg
In-Reply-To: <1197665998-32386-1-git-send-email-krh@redhat.com>
When we commit or roll back the lock file the fd is automatically closed,
so don't do that again.
Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
config.c | 17 +++--------------
1 files changed, 3 insertions(+), 14 deletions(-)
diff --git a/config.c b/config.c
index 49d2b42..2e52b17 100644
--- a/config.c
+++ b/config.c
@@ -751,7 +751,7 @@ int git_config_set_multivar(const char* key, const char* value,
const char* value_regex, int multi_replace)
{
int i, dot;
- int fd = -1, in_fd;
+ int fd, in_fd;
int ret;
char* config_filename;
struct lock_file *lock = NULL;
@@ -955,26 +955,15 @@ int git_config_set_multivar(const char* key, const char* value,
munmap(contents, contents_sz);
}
- if (close(fd) || commit_lock_file(lock) < 0) {
+ if (commit_lock_file(lock) < 0) {
fprintf(stderr, "Cannot commit config file!\n");
ret = 4;
goto out_free;
}
- /* fd is closed, so don't try to close it below. */
- fd = -1;
- /*
- * lock is committed, so don't try to roll it back below.
- * NOTE: Since lockfile.c keeps a linked list of all created
- * lock_file structures, it isn't safe to free(lock). It's
- * better to just leave it hanging around.
- */
- lock = NULL;
ret = 0;
out_free:
- if (0 <= fd)
- close(fd);
if (lock)
rollback_lock_file(lock);
free(config_filename);
@@ -1072,7 +1061,7 @@ int git_config_rename_section(const char *old_name, const char *new_name)
}
fclose(config_file);
unlock_and_out:
- if (close(out_fd) || commit_lock_file(lock) < 0)
+ if (commit_lock_file(lock) < 0)
ret = error("Cannot commit config file!");
out:
free(config_filename);
--
1.5.3.4
^ permalink raw reply related
* Re: [Funky] "git -p cmd" inside a bare repository
From: Junio C Hamano @ 2007-12-14 20:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <Pine.LNX.4.64.0712141943280.27959@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I have no time left to work on git for a few days, so I cannot even review
> your patch. But Jeff's patch being smaller, I could, and AFAICT it solves
> the problem.
We have more than a few days ;-) Hope you are feeling better now.
You got familialized yourself with the work-tree part because the
initial round was so broken and you had to step in during the last round
(unfortuantely ;-), and Nguyễn also is familiar with that part of the
code. It would be nice if we can have a proper fix that is not papering
over deeper breakage.
^ permalink raw reply
* Re: [PATCH 1/2] Fix config lockfile handling.
From: Johannes Schindelin @ 2007-12-14 20:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kristian Høgsberg, git
In-Reply-To: <7vmysdqbui.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 481 bytes --]
Hi,
On Fri, 14 Dec 2007, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Fri, 14 Dec 2007, Kristian Høgsberg wrote:
> >
> >> - struct lock_file *lock = NULL;
> >> + struct lock_file lock;
> >
> > AFAICT this cannot work. At least not reliably. An atexit() handler
> > will access all (even closed) lockfiles.
>
> Correct. It cannot be on the stack.
Note that this behaviour will be another obstacle to libification.
Ciao,
Dscho
^ permalink raw reply
* Re: testsuite failures in mainline...
From: Junio C Hamano @ 2007-12-14 20:10 UTC (permalink / raw)
To: David Miller; +Cc: git
In-Reply-To: <20071214.111736.258936000.davem@davemloft.net>
David Miller <davem@davemloft.net> writes:
> From: Junio C Hamano <gitster@pobox.com>
> Date: Fri, 14 Dec 2007 11:15:01 -0800
>
>> When I can reproduce a breakage, in our tests, I'd run
>>
>> cd t && sh -x t3200-branch.sh -i -v
>>
>> (replace "t3200-*" with the failing test) and see which one of the steps
>> chained with && is breaking first.
>
> Looks like a malloc() failure:
> ...
> ++ git branch my3 local/master
> fatal: Out of memory, malloc failed
Do you mean this is a malloc() failure that you can reliable reproduce?
Puzzled...
^ permalink raw reply
* Re: [PATCH 1/2] Fix config lockfile handling.
From: Junio C Hamano @ 2007-12-14 20:07 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Kristian Høgsberg, gitster, git
In-Reply-To: <Pine.LNX.4.64.0712141928240.27959@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi,
>
> On Fri, 14 Dec 2007, Kristian Høgsberg wrote:
>
>> - struct lock_file *lock = NULL;
>> + struct lock_file lock;
>
> AFAICT this cannot work. At least not reliably. An atexit() handler will
> access all (even closed) lockfiles.
Correct. It cannot be on the stack.
^ permalink raw reply
* [PATCH] commit: allow --amend to reuse message from another commit
From: Junio C Hamano @ 2007-12-14 19:57 UTC (permalink / raw)
To: git; +Cc: Kristian Høgsberg
After tentatively applying a patch from a contributor, you can get a
replacement patch with corrected code and unusable commit log message.
In such a case, this sequence ought to give you an editor based on the
message in the earlier commit, to let you describe an incremental
improvement:
git reset --hard HEAD^ ;# discard the earlier one
git am <corrected-patch
git commit --amend -c HEAD@{1}
Unfortunately, --amend insisted reusing the message from the commit
being amended, ignoring the -c option. This corrects it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This was supported in the scripted version if --amend came before -c,
but it was broken if the flags were given in the other order.
builtin-commit.c | 2 +-
t/t7501-commit.sh | 17 +++++++++++++++++
2 files changed, 18 insertions(+), 1 deletions(-)
diff --git a/builtin-commit.c b/builtin-commit.c
index ad9f921..518ebe0 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -537,7 +537,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
die("Option -m cannot be combined with -c/-C/-F.");
if (edit_message)
use_message = edit_message;
- if (amend)
+ if (amend && !use_message)
use_message = "HEAD";
if (use_message) {
unsigned char sha1[20];
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 05aa97d..d1a415a 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -310,4 +310,21 @@ test_expect_success 'same tree (merge and amend merge)' '
'
+test_expect_success 'amend using the message from another commit' '
+
+ git reset --hard &&
+ test_tick &&
+ git commit --allow-empty -m "old commit" &&
+ old=$(git rev-parse --verify HEAD) &&
+ test_tick &&
+ git commit --allow-empty -m "new commit" &&
+ new=$(git rev-parse --verify HEAD) &&
+ test_tick &&
+ git commit --allow-empty --amend -C "$old" &&
+ git show --pretty="format:%ad %s" "$old" >expected &&
+ git show --pretty="format:%ad %s" HEAD >actual &&
+ diff -u expected actual
+
+'
+
test_done
^ permalink raw reply related
* Re: [Funky] "git -p cmd" inside a bare repository
From: Johannes Schindelin @ 2007-12-14 19:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <7vmysexdvw.fsf@gitster.siamese.dyndns.org>
Hi,
On Thu, 13 Dec 2007, Junio C Hamano wrote:
> If you have a bare repository and try this there:
>
> $ PAGER=head git show HEAD:gcc/ChangeLog
>
> it works as expected, but if you explicitly ask for pagination, like
> this:
>
> $ PAGER=head git -p show HEAD:gcc/ChangeLog
I have no time left to work on git for a few days, so I cannot even review
your patch. But Jeff's patch being smaller, I could, and AFAICT it solves
the problem.
Ciao,
Dscho
^ permalink raw reply
* Re: config.c fixes
From: Junio C Hamano @ 2007-12-14 19:43 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
In-Reply-To: <20071214192852.GA24187@bitplanet.net>
Kristian Høgsberg <krh@redhat.com> writes:
> While strace'ing builtin-clone I saw this horror:
> I think the best solution is to just parse up the entire config file
> up front and keep it in a data structure, make the changes and then
> write it all out at the end.
Yeah, that was what I suggested a few times when other people have done
config writing side, but without successfully getting past their skulls
(it is not Linus's nor my code). It's about time somebody started to
clean up that mess.
The timing is a bit unfortunate, though. I would have preferred to have
a week or so to cook this in 'next' before merging it part of -rc0.
^ permalink raw reply
* Re: [PATCH 1/2] Fix config lockfile handling.
From: Johannes Sixt @ 2007-12-14 19:32 UTC (permalink / raw)
To: Kristian Høgsberg, git, gitster
In-Reply-To: <1197660157-24109-2-git-send-email-krh@redhat.com>
Kristian Høgsberg wrote:
> When we commit or roll back the lock file the fd is automatically closed,
> so don't do that again. Also, just keep the lock on the stack.
IIRC, locks are accessed from atexit(), e.g. during a die(). So you must not
put one on the stack.
-- Hannes
^ permalink raw reply
* Re: [PATCH 1/2] Fix config lockfile handling.
From: Johannes Schindelin @ 2007-12-14 19:29 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: gitster, git
In-Reply-To: <1197660157-24109-2-git-send-email-krh@redhat.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 240 bytes --]
Hi,
On Fri, 14 Dec 2007, Kristian Høgsberg wrote:
> - struct lock_file *lock = NULL;
> + struct lock_file lock;
AFAICT this cannot work. At least not reliably. An atexit() handler will
access all (even closed) lockfiles.
Ciao,
Dscho
^ permalink raw reply
* Re: testsuite failures in mainline...
From: David Miller @ 2007-12-14 19:17 UTC (permalink / raw)
To: gitster; +Cc: git
In-Reply-To: <7vfxy5rsui.fsf@gitster.siamese.dyndns.org>
From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 14 Dec 2007 11:15:01 -0800
> When I can reproduce a breakage, in our tests, I'd run
>
> cd t && sh -x t3200-branch.sh -i -v
>
> (replace "t3200-*" with the failing test) and see which one of the steps
> chained with && is breaking first.
Looks like a malloc() failure:
+ test_expect_success 'test tracking setup via config' 'git config branch.autosetupmerge true &&
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
(git show-ref -q refs/remotes/local/master || git-fetch local) &&
git branch my3 local/master &&
test $(git config branch.my3.remote) = local &&
test $(git config branch.my3.merge) = refs/heads/master'
+ test 2 = 2
+ test_skip 'test tracking setup via config' 'git config branch.autosetupmerge true &&
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
(git show-ref -q refs/remotes/local/master || git-fetch local) &&
git branch my3 local/master &&
test $(git config branch.my3.remote) = local &&
test $(git config branch.my3.merge) = refs/heads/master'
++ expr ./t3200-branch.sh : '.*/\(t[0-9]*\)-[^/]*$'
+ this_test=t3200
++ expr 19 + 1
+ this_test=t3200.20
+ to_skip=
+ case "$to_skip" in
+ false
+ say 'expecting success: git config branch.autosetupmerge true &&
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
(git show-ref -q refs/remotes/local/master || git-fetch local) &&
git branch my3 local/master &&
test $(git config branch.my3.remote) = local &&
test $(git config branch.my3.merge) = refs/heads/master'
+ say_color info 'expecting success: git config branch.autosetupmerge true &&
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
(git show-ref -q refs/remotes/local/master || git-fetch local) &&
git branch my3 local/master &&
test $(git config branch.my3.remote) = local &&
test $(git config branch.my3.merge) = refs/heads/master'
+ test -z info
+ shift
+ echo '* expecting success: git config branch.autosetupmerge true &&
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
(git show-ref -q refs/remotes/local/master || git-fetch local) &&
git branch my3 local/master &&
test $(git config branch.my3.remote) = local &&
test $(git config branch.my3.merge) = refs/heads/master'
* expecting success: git config branch.autosetupmerge true &&
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
(git show-ref -q refs/remotes/local/master || git-fetch local) &&
git branch my3 local/master &&
test $(git config branch.my3.remote) = local &&
test $(git config branch.my3.merge) = refs/heads/master
+ test_run_ 'git config branch.autosetupmerge true &&
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
(git show-ref -q refs/remotes/local/master || git-fetch local) &&
git branch my3 local/master &&
test $(git config branch.my3.remote) = local &&
test $(git config branch.my3.merge) = refs/heads/master'
+ eval 'git config branch.autosetupmerge true &&
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
(git show-ref -q refs/remotes/local/master || git-fetch local) &&
git branch my3 local/master &&
test $(git config branch.my3.remote) = local &&
test $(git config branch.my3.merge) = refs/heads/master'
++ git config branch.autosetupmerge true
++ git config remote.local.url .
++ git config remote.local.fetch 'refs/heads/*:refs/remotes/local/*'
++ git show-ref -q refs/remotes/local/master
++ git branch my3 local/master
fatal: Out of memory, malloc failed
+ eval_ret=128
+ return 0
+ '[' 0 = 0 -a 128 = 0 ']'
+ test_failure_ 'test tracking setup via config' 'git config branch.autosetupmerge true &&
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
(git show-ref -q refs/remotes/local/master || git-fetch local) &&
git branch my3 local/master &&
test $(git config branch.my3.remote) = local &&
test $(git config branch.my3.merge) = refs/heads/master'
++ expr 19 + 1
+ test_count=20
++ expr 0 + 1
+ test_failure=1
+ say_color error 'FAIL 20: test tracking setup via config'
+ test -z error
+ shift
+ echo '* FAIL 20: test tracking setup via config'
* FAIL 20: test tracking setup via config
+ shift
+ echo 'git config branch.autosetupmerge true &&
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
(git show-ref -q refs/remotes/local/master || git-fetch local) &&
git branch my3 local/master &&
test $(git config branch.my3.remote) = local &&
test $(git config branch.my3.merge) = refs/heads/master'
+ sed -e 's/^/ /'
git config branch.autosetupmerge true &&
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
(git show-ref -q refs/remotes/local/master || git-fetch local) &&
git branch my3 local/master &&
test $(git config branch.my3.remote) = local &&
test $(git config branch.my3.merge) = refs/heads/master
+ test t = ''
+ trap - exit
+ exit 1
^ permalink raw reply
* Re: [PATCH 2/2] xdi_diff: trim common trailing lines
From: Junio C Hamano @ 2007-12-14 19:15 UTC (permalink / raw)
To: Peter Baumann; +Cc: Nicolas Pitre, git
In-Reply-To: <20071214090614.GB15610@xp.machine.xx>
Peter Baumann <waste.manager@gmx.de> writes:
> So you are loosing some values in your trim_common_tail function by
> making ctx only an int. (Not sure that it matters, but I noticed it
> while glancing over your code).
While it is true that this does not matter in practice (because the
context value initially comes from the end user via -U parameter that is
stored in a field of type int in diff_options structure), I agre that it
is the right thing to do to use the same type as underlying xdiff
library uses at the interface level. From the layering point of view.
xdiff-interface.[ch] are meant to be a thin usability wrapper, it should
not needlessly deviate from how the underlying xdiff operates.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox