* Re: [msysGit] [PATCH] git grep: be careful to use mutices only when they are initialized
From: Pat Thoyts @ 2011-10-26 9:19 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: msysgit, gitster, git
In-Reply-To: <alpine.DEB.1.00.1110251223500.32316@s15462909.onlinehome-server.info>
On 25 October 2011 18:25, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Rather nasty things happen when a mutex is not initialized but locked
> nevertheless. Now, when we're not running in a threaded manner, the mutex
> is not initialized, which is correct. But then we went and used the mutex
> anyway, which -- at least on Windows -- leads to a hard crash (ordinarily
> it would be called a segmentation fault, but in Windows speak it is an
> access violation).
>
> This problem was identified by our faithful tests when run in the msysGit
> environment.
I did not see this failure when running the tests on my machine. But
then threaded issues are often intermittent depending on load, number
of cores, phase of the moon, etc. You never said _which_ test either
although there are only 3 to try - most likey t7810-grep.sh
I was going to point out that it should be "mutexes" but I see it is
committed already :)
> To avoid having to wrap the line due to the 80 column limit, we use
So last century!
> the name "WHEN_THREADED" instead of "IF_USE_THREADS" because it is one
> character shorter. Which is all we need in this case.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> I looked around a bit but ran out of time to identify the reason why
> this was not caught earlier.
>
> builtin/grep.c | 9 +++++----
> 1 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 92eeada..e94c5fe 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -78,10 +78,11 @@ static pthread_mutex_t grep_mutex;
> /* Used to serialize calls to read_sha1_file. */
> static pthread_mutex_t read_sha1_mutex;
>
> -#define grep_lock() pthread_mutex_lock(&grep_mutex)
> -#define grep_unlock() pthread_mutex_unlock(&grep_mutex)
> -#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)
> -#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)
> +#define WHEN_THREADED(x) do { if (use_threads) (x); } while (0)
> +#define grep_lock() WHEN_THREADED(pthread_mutex_lock(&grep_mutex))
> +#define grep_unlock() WHEN_THREADED(pthread_mutex_unlock(&grep_mutex))
> +#define read_sha1_lock() WHEN_THREADED(pthread_mutex_lock(&read_sha1_mutex))
> +#define read_sha1_unlock() WHEN_THREADED(pthread_mutex_unlock(&read_sha1_mutex))
>
> /* Signalled when a new work_item is added to todo. */
> static pthread_cond_t cond_add;
> --
> 1.7.5.3.4540.g15f89
Works for me.
Pat.
^ permalink raw reply
* Recovery after interrupted repack
From: Hannu Koivisto @ 2011-10-26 9:15 UTC (permalink / raw)
To: git
Hi,
I interrupted git repack -a -f -d and now most commands say "fatal:
bad object HEAD". Additionally an attempt to run git repack again
says a bunch of "error: refs/heads/foo does not point to a valid
object!" messages.
git fsck prints a huge list of "missing blob <sha-1>" lines.
I'm using a Cygwin build of git 1.7.5.1 (I also have a build of the
latest master available).
--
Hannu
^ permalink raw reply
* Re: [msysGit] [PATCH] git grep: be careful to use mutices only when they are initialized
From: Tay Ray Chuan @ 2011-10-26 9:10 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: msysgit, gitster, git
In-Reply-To: <alpine.DEB.1.00.1110251223500.32316@s15462909.onlinehome-server.info>
On Wed, Oct 26, 2011 at 1:25 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Rather nasty things happen when a mutex is not initialized but locked
> nevertheless. Now, when we're not running in a threaded manner, the mutex
> is not initialized, which is correct. But then we went and used the mutex
> anyway, which -- at least on Windows -- leads to a hard crash (ordinarily
> it would be called a segmentation fault, but in Windows speak it is an
> access violation).
>
> This problem was identified by our faithful tests when run in the msysGit
> environment.
May I ask which test are you talking about specifically?
I ask as I'm curious how this is triggered; git-grep works fine for me
so far (1.7.6.msysgit.0.584.g2cbf)
--
Cheers,
Ray Chuan
^ permalink raw reply
* Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER
From: Atsushi Nakagawa @ 2011-10-26 3:05 UTC (permalink / raw)
To: msysgit; +Cc: git, johannes.schindelin, j.sixt
In-Reply-To: <1319554509-6532-1-git-send-email-kusmabite@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1344 bytes --]
On Oct 25, 11:55 pm, Erik Faye-Lund <kusmab...@gmail.com> wrote:
> [...]
> +int pthread_mutex_init(pthread_mutex_t *mutex, const pthread_mutexattr_t
*attr)
> +{
> + InitializeCriticalSection(&mutex->cs);
> + mutex->autoinit = 0;
> + return 0;
> +}
> +
> +int pthread_mutex_lock(pthread_mutex_t *mutex)
> +{
> + if (mutex->autoinit) {
> + if (InterlockedCompareExchange(&mutex->autoinit, -1, 1)
!= -1) {
I'm making the assumption that mutex->autoinit starts off as 1 before
things get multi-threaded..
I've only looked at what's in the patch so I could be missing vital
context.. Anyways, is there a reason why you made this
"InterlockedCompareExchange(..., -1, 1) != -1" and not
"InterlockedCompareExchange(..., -1, 1) == 1"?
It looks to me the former adds a race condition after "if (mutex->autoinit)
{". e.g. A second thread could reinitialize mutex->cs after the first
thread has already entered EnterCriticalSection(...).
> + pthread_mutex_init(mutex, NULL);
> + mutex->autoinit = 0;
> + } else
> + while (mutex->autoinit != 0)
> + ; /* wait for other thread */
> + }
> +
> + EnterCriticalSection(&mutex->cs);
> + return 0;
> +}
> [...]
--
Atsushi Nakagawa
[-- Attachment #2: Type: text/html, Size: 2272 bytes --]
^ permalink raw reply
* Re: [PATCH] completion: fix issue with process substitution not working on Git for Windows
From: Stefan Näwe @ 2011-10-26 6:52 UTC (permalink / raw)
To: Johannes Sixt; +Cc: spearce, git, gitster
In-Reply-To: <4EA71E8C.8010704@kdbg.org>
Am 25. Oktober 2011 22:39 schrieb Johannes Sixt <j6t@kdbg.org>:
> Am 25.10.2011 20:01, schrieb Stefan Naewe:
>> Git for Windows comes with a bash that doesn't support process substitution.
>> It issues the following error when using git-completion.bash with
>> GIT_PS1_SHOWUPSTREAM set:
>>
>> $ export GIT_PS1_SHOWUPSTREAM=1
>> sh.exe": cannot make pipe for process substitution: Function not implemented
>> sh.exe": cannot make pipe for process substitution: Function not implemented
>> sh.exe": <(git config -z --get-regexp '^(svn-remote\..*\.url|bash\.showupstream)$' 2>/dev/null | tr '\0\n' '\n '): ambiguous redirect
>>
>> Replace the process substitution with a simple "echo $var | while...".
>>
>> Signed-off-by: Stefan Naewe <stefan.naewe@gmail.com>
>> ---
>> contrib/completion/git-completion.bash | 4 +++-
>> 1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 8648a36..926db80 100755
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -110,6 +110,8 @@ __git_ps1_show_upstream ()
>> local upstream=git legacy="" verbose=""
>>
>> # get some config options from git-config
>> + output="$(git config -z --get-regexp '^(svn-remote\..*\.url|bash\.showupstream)$' 2>/dev/null | tr '\0\n' '\n ')"
>> + echo "$output" | \
>> while read key value; do
>> case "$key" in
>> bash.showupstream)
>> @@ -125,7 +127,7 @@ __git_ps1_show_upstream ()
>> upstream=svn+git # default upstream is SVN if available, else git
>> ;;
>> esac
>> - done < <(git config -z --get-regexp '^(svn-remote\..*\.url|bash\.showupstream)$' 2>/dev/null | tr '\0\n' '\n ')
>> + done
>>
>> # parse configuration values
>> for option in ${GIT_PS1_SHOWUPSTREAM}; do
>
> Are you sure that the result still works as intended? The while loop
> sets a few variables. When you place it in a pipe, the loop runs in a
> subshell, and subsequent code will not see the modified values. Unless
> bash knows how to optimize away the subshell, that is.
I doesn't work in the 'git svn' case, I guess.
> OTOH, when you use while ...; do ...; done < <(...), the while loop is
> not in a subshell.
>
> An alternative is to use: while ...; do ...; done <<< "$output"
I'll try that.
> BTW, you don't need to protect the end-of-line with a backslash if the
> line ends with the pipe symbol.
OK. Will do.
> -- Hannes
Thanks,
Stefan
--
----------------------------------------------------------------
python -c "print '73746566616e2e6e6165776540676d61696c2e636f6d'.decode('hex')"
^ permalink raw reply
* Re: [msysGit] Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER
From: Kyle Moffett @ 2011-10-26 3:44 UTC (permalink / raw)
To: kusmabite; +Cc: Johannes Sixt, msysgit, git, johannes.schindelin
In-Reply-To: <CABPQNSY6-j7iNagsJc3WKVZ94=yZHdfBswA-v0XY7vH+RxyjYQ@mail.gmail.com>
On Tue, Oct 25, 2011 at 16:51, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> On Tue, Oct 25, 2011 at 10:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> Am 25.10.2011 17:42, schrieb Erik Faye-Lund:
>>> On Tue, Oct 25, 2011 at 5:28 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>>> Am 10/25/2011 16:55, schrieb Erik Faye-Lund:
>>>>> +int pthread_mutex_lock(pthread_mutex_t *mutex)
>>>>> +{
>>>>> + if (mutex->autoinit) {
>>>>> + if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {
>>>>> + pthread_mutex_init(mutex, NULL);
>>>>> + mutex->autoinit = 0;
>>>>> + } else
>>>>> + while (mutex->autoinit != 0)
>>>>> + ; /* wait for other thread */
>>>>> + }
>>>>
>>>> The double-checked locking idiom. Very suspicious. Can you explain why it
>>>> works in this case? Why are no Interlocked functions needed for the other
>>>> accesses of autoinit? ("It is volatile" is the wrong answer to this last
>>>> question, BTW.)
>>>
>>> I agree that it should look a bit suspicious; I'm generally skeptical
>>> whenever I see 'volatile' in threading-code myself. But I think it's
>>> the right answer in this case. "volatile" means that the compiler
>>> cannot optimize away accesses, which is sufficient in this case.
>>
>> No, it is not, and it took me a train ride to see what's wrong. It has
>> nothing to do with autoinit, but with all the other memory locations
>> that are written. See here, with pthread_mutex_init() inlined:
>>
>> if (mutex->autoinit) {
>>
>> Assume two threads enter this block.
>>
>> if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {
>>
>> Only one thread, A, say on CPU A, will enter this block.
>>
>> InitializeCriticalSection(&mutex->cs);
>>
>> Thread A writes some values. Note that there are no memory barriers
>> involved here. Not that I know of or that they would be documented.
>>
>> mutex->autoinit = 0;
>>
>> And it writes another one. Thread A continues below to contend for the
>> mutex it just initialized.
>>
>> } else
>>
>> Meanwhile, thread B, say on CPU B, spins in this loop:
>>
>> while (mutex->autoinit != 0)
>> ; /* wait for other thread */
>>
>> When thread B arrives here, it sees the value of autoinit that thread A
>> has written above.
>>
>> HOWEVER, when it continues, there is NO [*] guarantee that it will also
>> see the values that InitializeCriticalSection() has written, because
>> there were no memory barriers involved. When it continues, there is a
>> chance that it calls EnterCriticalSection() with uninitialized values!
>>
>
> Thanks for pointing this out, I completely forgot about write re-ordering.
>
> This is indeed a problem. So, shouldn't replacing "mutex->autoinit =
> 0;" with "InterlockedExchange(&mutex->autoinit, 0)" solve the problem?
> InterlockedExchange generates a full memory barrier:
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms683590(v=vs.85).aspx
No, I'm afraid that won't solve the issue (at least in GCC, not sure about MSVC)
A write barrier in one thread is only effective if it is paired with a
read barrier in the other thread.
Since there's no read barrier in the "while(mutex->autoinit != 0)",
you don't have any guaranteed ordering.
I guess if MSVC assumes that volatile reads imply barriers then it might work...
Cheers,
Kyle Moffett
--
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/
^ permalink raw reply
* Re: [PATCH] gitweb/Makefile: Remove static/gitweb.js in the clean target
From: Jakub Narebski @ 2011-10-26 0:36 UTC (permalink / raw)
To: Drew Northup; +Cc: Ramsay Jones, Junio C Hamano, GIT Mailing-list
In-Reply-To: <1319583484.10399.41.camel@drew-northup.unet.maine.edu>
Drew Northup napisał:
> On Tue, 2011-10-25 at 18:15 +0100, Ramsay Jones wrote:
> > Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> > ---
> > gitweb/Makefile | 4 +++-
> > 1 files changed, 3 insertions(+), 1 deletions(-)
> >
> > diff --git a/gitweb/Makefile b/gitweb/Makefile
> > index 1c85b5f..4191c6b 100644
> > --- a/gitweb/Makefile
> > +++ b/gitweb/Makefile
> > @@ -185,7 +185,9 @@ install: all
> > ### Cleaning rules
> >
> > clean:
> > - $(RM) gitweb.cgi static/gitweb.min.js static/gitweb.min.css GITWEB-BUILD-OPTIONS
> > + $(RM) gitweb.cgi static/gitweb.js \
> > + static/gitweb.min.js static/gitweb.min.css \
> > + GITWEB-BUILD-OPTIONS
> >
> > .PHONY: all clean install test test-installed .FORCE-GIT-VERSION-FILE FORCE
> >
>
> Forgive me for sounding a bit numb, but what does this fix? I don't see
> it in the commit message.
gitweb.js is nowadays a generated file. Though that bit should be
in commit message...
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists
From: Nguyen Thai Ngoc Duy @ 2011-10-26 0:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vd3dk516p.fsf@alter.siamese.dyndns.org>
2011/10/26 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> In this case, "foo" is considered a submodule and bar, if added,
>> belongs to foo/.git. "git add" should only allow "git add foo" in this
>> case, but it passes somehow.
>
> I do not think the above description is correct.
>
> The test:
>
> - populates the current directory;
> - makes a clone in ./clone2;
> - creates a file clone2/b;
> - runs "git add clone2/b" with GIT_DIR set to clone2/.git, without
> setting GIT_WORK_TREE nor having core.worktree in clone2/.git/config.
>
> The last step should add a path "clone2/b" to $GIT_DIR/index (which is
> clone2/.git/index in this case). The clone2 is not a submodule to the top
> level repository in this case, but even if it were, that would not change
> the definition of what the command should do when GIT_DIR is set without
> GIT_WORK_TREE nor core.worktree in $GIT_DIR/config.
Now look from "git add" perspective, it does not really care where
$GIT_DIR is. It assumes that $(pwd) is working directory's top. So it
- reads content of current directory, it sees "clone2" as a directory
- it descends in and see ".git" so "clone2" must be a git link
- because clone2 is a separate repository (again $GIT_DIR is not
consulted), "b" should be managed by "clone2"
- so it stops.
This is the only place I see a submodule (from the first glance) is
actually top level repository. Yes I guess we can support this, but
it's just too weird to be widely used in pratice..
> Running (cd clone2 && git add b) is a _more natural_ thing to do, and it
> will result in a path "b" added to the clone2 repository, so that the
> result is more useful if you are going to chdir to the repository and keep
> working on it. But that does not mean the existing test is incorrect. It
> does not just pass somehow but the test passes by design.
>
> I did not check if later tests look at the contents of commit "new2" to
> make sure it contains "clone2/b", but if they do this change should break
> such tests.
>
> So I am puzzled by this change; what is this trying to achieve?
--
Duy
^ permalink raw reply
* [PATCH] replace sha1 with another algorithm
From: Jeff King @ 2011-10-26 0:12 UTC (permalink / raw)
To: git
SHA-1 is due to be cryptographically broken sometime in the
next decade, with collision attacks becoming possible. But
we don't have to wait! We can act now and replace it,
treating us to all of the pain of a flag day without any
delay!
We could of course use the SHA-2 family, or wait for the
upcoming SHA-3. But any good cryptographer knows that you
should _never_ use a standard algorithm. It's always better
to roll your own. After all, if _you_ can't break it, how
could anyone else?
Signed-off-by: Jeff King <peff@peff.net>
Reviewed-by: Brandon Casey <drafnel@gmail.com>
Mocked-by: Rick Balocca <richard.balocca@ericsson.com>
Enjoyed-by: Elijah Newren <newren@gmail.com>
---
block-sha1/sha1.h | 2 +-
cache.h | 4 +++-
sha1_file.c | 32 ++++++++++++++++++++++++++++++++
3 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/block-sha1/sha1.h b/block-sha1/sha1.h
index b864df6..49331e3 100644
--- a/block-sha1/sha1.h
+++ b/block-sha1/sha1.h
@@ -19,4 +19,4 @@
#define git_SHA_CTX blk_SHA_CTX
#define git_SHA1_Init blk_SHA1_Init
#define git_SHA1_Update blk_SHA1_Update
-#define git_SHA1_Final blk_SHA1_Final
+#define real_git_SHA1_Final blk_SHA1_Final
diff --git a/cache.h b/cache.h
index 2e6ad36..068062b 100644
--- a/cache.h
+++ b/cache.h
@@ -13,9 +13,11 @@
#define git_SHA_CTX SHA_CTX
#define git_SHA1_Init SHA1_Init
#define git_SHA1_Update SHA1_Update
-#define git_SHA1_Final SHA1_Final
+#define real_git_SHA1_Final SHA1_Final
#endif
+void git_SHA1_Final(unsigned char out[20], git_SHA_CTX *ctx);
+
#include <zlib.h>
typedef struct git_zstream {
z_stream z;
diff --git a/sha1_file.c b/sha1_file.c
index 27f3b9b..23e0107 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2833,3 +2833,35 @@ void assert_sha1_type(const unsigned char *sha1, enum object_type expect)
die("%s is not a valid '%s' object", sha1_to_hex(sha1),
typename(expect));
}
+
+static void xor_bytes(unsigned char *out, unsigned char *a, unsigned char *b,
+ unsigned n)
+{
+ unsigned i;
+ for (i = 0; i < n; i++)
+ out[i] = a[i] ^ b[i];
+}
+
+static void mix_hash(unsigned char *h, unsigned n)
+{
+ unsigned char out[20];
+ unsigned mid = n / 2;
+
+ if (2*mid < n)
+ return;
+
+ xor_bytes(out, h, h + mid, mid);
+ xor_bytes(out + mid, h + mid, h, mid);
+ memcpy(h, out, n);
+
+ /* If a little bit of mixing is good, then a lot must be GREAT! */
+ mix_hash(h, mid);
+ mix_hash(h + mid, mid);
+}
+
+void git_SHA1_Final(unsigned char out[20], git_SHA_CTX *ctx)
+{
+ /* We build on top of the regular SHA1, but then "enhance" it. */
+ real_git_SHA1_Final(out, ctx);
+ mix_hash(out, 20);
+}
--
1.7.7.troll
^ permalink raw reply related
* Re: [PATCH/WIP 02/11] notes-merge: use opendir/readdir instead of using read_directory()
From: Nguyen Thai Ngoc Duy @ 2011-10-26 0:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vzkgo3m9b.fsf@alter.siamese.dyndns.org>
2011/10/26 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> notes_merge_commit() only needs to list all entries (non-recursively)
>> under a directory, which can be easily accomplished with
>> opendir/readdir and would be more lightweight than read_directory().
>>
>> read_directory() is designed to list paths inside a working
>> directory. Using it outside of its scope may lead to undesired effects.
>
> Technically isn't the directory structure this codepath looks at a working
> tree that has extract of a notes tree commit?
Yes it's like a secondary working tree, only for notes, if I read the
code correctly. The thing is this space is inside ".git".
Current read_directory() treats given path separately from contents
inside the path. If the given path has ".git", it's ok (but it'll stop
at .git if during tree recursion). The new read_directory() does not
make this exception, so when note-merge call
read_directory(".git/NOTES_MERGE_WORKTREE"), read_directory() sees
".git" and stops immediately, assuming it's a gitlink.
One could say we should keep current behavior, but I don't really see
it's worth the effort.
--
Duy
^ permalink raw reply
* Re: [PATCH] gitweb/Makefile: Remove static/gitweb.js in the clean target
From: Drew Northup @ 2011-10-25 22:58 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Jakub Narebski, GIT Mailing-list
In-Reply-To: <4EA6EEA8.3000204@ramsay1.demon.co.uk>
On Tue, 2011-10-25 at 18:15 +0100, Ramsay Jones wrote:
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
> gitweb/Makefile | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/gitweb/Makefile b/gitweb/Makefile
> index 1c85b5f..4191c6b 100644
> --- a/gitweb/Makefile
> +++ b/gitweb/Makefile
> @@ -185,7 +185,9 @@ install: all
> ### Cleaning rules
>
> clean:
> - $(RM) gitweb.cgi static/gitweb.min.js static/gitweb.min.css GITWEB-BUILD-OPTIONS
> + $(RM) gitweb.cgi static/gitweb.js \
> + static/gitweb.min.js static/gitweb.min.css \
> + GITWEB-BUILD-OPTIONS
>
> .PHONY: all clean install test test-installed .FORCE-GIT-VERSION-FILE FORCE
>
Forgive me for sounding a bit numb, but what does this fix? I don't see
it in the commit message.
--
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
^ permalink raw reply
* Re: [IGNORETHIS/PATCH] Choosing the sha1 prefix of your commits
From: Drew Northup @ 2011-10-25 22:35 UTC (permalink / raw)
To: Jeff King
Cc: Ted Ts'o, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Git Mailing List
In-Reply-To: <20111020155611.GB16114@sigill.intra.peff.net>
On Thu, 2011-10-20 at 11:56 -0400, Jeff King wrote:
> On Thu, Oct 20, 2011 at 09:14:55AM -0400, Ted Ts'o wrote:
>
> > Another possibility is to warn if the commit messages are not NULL
> > terminated.
>
> A minor nit, but it's not whether they are terminated with NUL, but
> rather whether they have embedded NUL. But yeah, this could maybe just
> be something fsck looks for.
God (or your deity of choice) forbid that anybody ever writes code that
saves raw UTF-16 as the commit message...
--
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
^ permalink raw reply
* Re: [msysGit] Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER
From: Johannes Sixt @ 2011-10-25 21:13 UTC (permalink / raw)
To: kusmabite; +Cc: msysgit, git, johannes.schindelin
In-Reply-To: <CABPQNSY6-j7iNagsJc3WKVZ94=yZHdfBswA-v0XY7vH+RxyjYQ@mail.gmail.com>
Am 25.10.2011 22:51, schrieb Erik Faye-Lund:
> On Tue, Oct 25, 2011 at 10:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> HOWEVER, when it continues, there is NO [*] guarantee that it will also
>> see the values that InitializeCriticalSection() has written, because
>> there were no memory barriers involved. When it continues, there is a
>> chance that it calls EnterCriticalSection() with uninitialized values!
>>
>
> Thanks for pointing this out, I completely forgot about write re-ordering.
>
> This is indeed a problem. So, shouldn't replacing "mutex->autoinit =
> 0;" with "InterlockedExchange(&mutex->autoinit, 0)" solve the problem?
> InterlockedExchange generates a full memory barrier:
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms683590(v=vs.85).aspx
That should do it.
>> [*] If you compile this code with MSVC >= 2005, "No guarantee" is not
>> true, it's exactly the opposite because Microsoft extended the meaning
>> of 'volatile' to imply a memory barriere.
>
> Do you have a source for this? I'm not saying it isn't true, I just
> never heard of this, and would like to read up on it :)
http://msdn.microsoft.com/en-us/library/ms686355%28VS.85%29.aspx
-- Hannes
^ permalink raw reply
* Re: [msysGit] Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER
From: Erik Faye-Lund @ 2011-10-25 20:51 UTC (permalink / raw)
To: Johannes Sixt; +Cc: msysgit, git, johannes.schindelin
In-Reply-To: <4EA716FC.2010804@kdbg.org>
On Tue, Oct 25, 2011 at 10:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 25.10.2011 17:42, schrieb Erik Faye-Lund:
>> On Tue, Oct 25, 2011 at 5:28 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> Am 10/25/2011 16:55, schrieb Erik Faye-Lund:
>>>> +int pthread_mutex_lock(pthread_mutex_t *mutex)
>>>> +{
>>>> + if (mutex->autoinit) {
>>>> + if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {
>>>> + pthread_mutex_init(mutex, NULL);
>>>> + mutex->autoinit = 0;
>>>> + } else
>>>> + while (mutex->autoinit != 0)
>>>> + ; /* wait for other thread */
>>>> + }
>>>
>>> The double-checked locking idiom. Very suspicious. Can you explain why it
>>> works in this case? Why are no Interlocked functions needed for the other
>>> accesses of autoinit? ("It is volatile" is the wrong answer to this last
>>> question, BTW.)
>>
>> I agree that it should look a bit suspicious; I'm generally skeptical
>> whenever I see 'volatile' in threading-code myself. But I think it's
>> the right answer in this case. "volatile" means that the compiler
>> cannot optimize away accesses, which is sufficient in this case.
>
> No, it is not, and it took me a train ride to see what's wrong. It has
> nothing to do with autoinit, but with all the other memory locations
> that are written. See here, with pthread_mutex_init() inlined:
>
> if (mutex->autoinit) {
>
> Assume two threads enter this block.
>
> if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {
>
> Only one thread, A, say on CPU A, will enter this block.
>
> InitializeCriticalSection(&mutex->cs);
>
> Thread A writes some values. Note that there are no memory barriers
> involved here. Not that I know of or that they would be documented.
>
> mutex->autoinit = 0;
>
> And it writes another one. Thread A continues below to contend for the
> mutex it just initialized.
>
> } else
>
> Meanwhile, thread B, say on CPU B, spins in this loop:
>
> while (mutex->autoinit != 0)
> ; /* wait for other thread */
>
> When thread B arrives here, it sees the value of autoinit that thread A
> has written above.
>
> HOWEVER, when it continues, there is NO [*] guarantee that it will also
> see the values that InitializeCriticalSection() has written, because
> there were no memory barriers involved. When it continues, there is a
> chance that it calls EnterCriticalSection() with uninitialized values!
>
Thanks for pointing this out, I completely forgot about write re-ordering.
This is indeed a problem. So, shouldn't replacing "mutex->autoinit =
0;" with "InterlockedExchange(&mutex->autoinit, 0)" solve the problem?
InterlockedExchange generates a full memory barrier:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms683590(v=vs.85).aspx
> }
>
>
> [*] If you compile this code with MSVC >= 2005, "No guarantee" is not
> true, it's exactly the opposite because Microsoft extended the meaning
> of 'volatile' to imply a memory barriere.
Do you have a source for this? I'm not saying it isn't true, I just
never heard of this, and would like to read up on it :)
> This is *NOT* true for gcc in
> general. It may be true for MinGW gcc, but I do not know.
>
>> Basically, the thread that gets the original 1 returned from
>> InterlockedCompareExchange is the only one who writes to
>> mutex->autoinit. All other threads only read the value, and the
>> volatile should make sure they actually do. Since all 32-bit reads and
>> writes are atomic on Windows (see
>> http://msdn.microsoft.com/en-us/library/windows/desktop/ms684122(v=vs.85).aspx
>> "Simple reads and writes to properly-aligned 32-bit variables are
>> atomic operations.") and mutex->autoinit is a LONG, this should be
>> safe AFAICT. In fact, Windows specifically does not have any
>> explicitly atomic writes exactly for this reason.
>
> There is a difference between atomic and coherent: Yes, 32-bit accesses
> are atomic, but they are not automatically coherent: A 32-bit value
> written by one CPU is not instantly visible on the other CPU. 'volatile'
> as per the C lanugage does not add any guarantees that would be of
> interest here. OTOH, Microsoft's definition of 'volatile' does.
>
I never meant to imply this either, I simply forgot about write re-ordering ;)
>> The only ways mutex->autoinit can be updated is:
>> - InterlockedCompareExchange compares it to 1, finds it's identical
>> and inserts -1
>> - intialization is done
>> Both these updates happens from the same thread.
>>
>> Yes, details like this should probably go into the commit message ;)
>
> A comment in the function is preferred!
>
Yes, good point. This is code that looks dangerous (and in this case
potentially was - hopefully eventual future iteration won't be), and
should of course be documented inline...
^ permalink raw reply
* Re: [PATCH] completion: fix issue with process substitution not working on Git for Windows
From: Johannes Sixt @ 2011-10-25 20:39 UTC (permalink / raw)
To: Stefan Naewe; +Cc: spearce, git, gitster
In-Reply-To: <1319565695-5976-1-git-send-email-stefan.naewe@gmail.com>
Am 25.10.2011 20:01, schrieb Stefan Naewe:
> Git for Windows comes with a bash that doesn't support process substitution.
> It issues the following error when using git-completion.bash with
> GIT_PS1_SHOWUPSTREAM set:
>
> $ export GIT_PS1_SHOWUPSTREAM=1
> sh.exe": cannot make pipe for process substitution: Function not implemented
> sh.exe": cannot make pipe for process substitution: Function not implemented
> sh.exe": <(git config -z --get-regexp '^(svn-remote\..*\.url|bash\.showupstream)$' 2>/dev/null | tr '\0\n' '\n '): ambiguous redirect
>
> Replace the process substitution with a simple "echo $var | while...".
>
> Signed-off-by: Stefan Naewe <stefan.naewe@gmail.com>
> ---
> contrib/completion/git-completion.bash | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 8648a36..926db80 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -110,6 +110,8 @@ __git_ps1_show_upstream ()
> local upstream=git legacy="" verbose=""
>
> # get some config options from git-config
> + output="$(git config -z --get-regexp '^(svn-remote\..*\.url|bash\.showupstream)$' 2>/dev/null | tr '\0\n' '\n ')"
> + echo "$output" | \
> while read key value; do
> case "$key" in
> bash.showupstream)
> @@ -125,7 +127,7 @@ __git_ps1_show_upstream ()
> upstream=svn+git # default upstream is SVN if available, else git
> ;;
> esac
> - done < <(git config -z --get-regexp '^(svn-remote\..*\.url|bash\.showupstream)$' 2>/dev/null | tr '\0\n' '\n ')
> + done
>
> # parse configuration values
> for option in ${GIT_PS1_SHOWUPSTREAM}; do
Are you sure that the result still works as intended? The while loop
sets a few variables. When you place it in a pipe, the loop runs in a
subshell, and subsequent code will not see the modified values. Unless
bash knows how to optimize away the subshell, that is.
OTOH, when you use while ...; do ...; done < <(...), the while loop is
not in a subshell.
An alternative is to use: while ...; do ...; done <<< "$output"
BTW, you don't need to protect the end-of-line with a backslash if the
line ends with the pipe symbol.
-- Hannes
^ permalink raw reply
* Re: general protection faults with "git grep" version 1.7.7.1
From: Jim Meyering @ 2011-10-25 20:24 UTC (permalink / raw)
To: Thomas Rast
Cc: Richard W.M. Jones, Markus Trippelsdorf, git, Shawn O. Pearce,
Jeff King, Nicolas Pitre
In-Reply-To: <201110251854.43369.trast@student.ethz.ch>
Thomas Rast wrote:
...
>> The real problem seems to be in glibc, with its addition of
>> the "leaf" attribute to those synchronization primitives:
>>
>> http://bugzilla.redhat.com/747377#c22
>
> Aha. Glad you found it :-)
>
> Meanwhile I read
>
> http://www.hpl.hp.com/techreports/2004/HPL-2004-209.html
>
> which discusses a similar issue in section 4.3, but is very
> interesting on its own. It's funny how it says
>
> We know of at least three optimizing compilers (two of them
> production compilers) that performed this transformation at some
> point during their lifetime; usually at least partially reversing
> the decision when the implications on multi-threaded code became
> known.
>
> I guess that would be four now if it was literally the same problem.
Yep. For those not following the BZ comments at the about URL,
POSIX is quite clear. Quoting from
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_11:
The following functions synchronize memory with respect to other threads:
fork
pthread_barrier_wait
pthread_cond_broadcast
pthread_cond_signal
pthread_cond_timedwait
pthread_cond_wait
pthread_create
pthread_join
pthread_mutex_lock
pthread_mutex_timedlock
pthread_mutex_trylock
pthread_mutex_unlock
pthread_spin_lock
pthread_spin_trylock
pthread_spin_unlock
pthread_rwlock_rdlock
pthread_rwlock_timedrdlock
pthread_rwlock_timedwrlock
pthread_rwlock_tryrdlock
pthread_rwlock_trywrlock
pthread_rwlock_unlock
pthread_rwlock_wrlock
sem_post
sem_timedwait
sem_trywait
sem_wait
semctl
semop
wait
waitpid
glibc's addition of the leaf attribute to any of those
appears to make gcc violate that.
^ permalink raw reply
* Re: [msysGit] Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER
From: Johannes Sixt @ 2011-10-25 20:07 UTC (permalink / raw)
To: kusmabite; +Cc: msysgit, git, johannes.schindelin
In-Reply-To: <CABPQNSZ8wesy-px-n1LYbVwFT3gBNcrHfe+_553sinTferqsog@mail.gmail.com>
Am 25.10.2011 17:42, schrieb Erik Faye-Lund:
> On Tue, Oct 25, 2011 at 5:28 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> Am 10/25/2011 16:55, schrieb Erik Faye-Lund:
>>> +int pthread_mutex_lock(pthread_mutex_t *mutex)
>>> +{
>>> + if (mutex->autoinit) {
>>> + if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {
>>> + pthread_mutex_init(mutex, NULL);
>>> + mutex->autoinit = 0;
>>> + } else
>>> + while (mutex->autoinit != 0)
>>> + ; /* wait for other thread */
>>> + }
>>
>> The double-checked locking idiom. Very suspicious. Can you explain why it
>> works in this case? Why are no Interlocked functions needed for the other
>> accesses of autoinit? ("It is volatile" is the wrong answer to this last
>> question, BTW.)
>
> I agree that it should look a bit suspicious; I'm generally skeptical
> whenever I see 'volatile' in threading-code myself. But I think it's
> the right answer in this case. "volatile" means that the compiler
> cannot optimize away accesses, which is sufficient in this case.
No, it is not, and it took me a train ride to see what's wrong. It has
nothing to do with autoinit, but with all the other memory locations
that are written. See here, with pthread_mutex_init() inlined:
if (mutex->autoinit) {
Assume two threads enter this block.
if (InterlockedCompareExchange(&mutex->autoinit, -1, 1) != -1) {
Only one thread, A, say on CPU A, will enter this block.
InitializeCriticalSection(&mutex->cs);
Thread A writes some values. Note that there are no memory barriers
involved here. Not that I know of or that they would be documented.
mutex->autoinit = 0;
And it writes another one. Thread A continues below to contend for the
mutex it just initialized.
} else
Meanwhile, thread B, say on CPU B, spins in this loop:
while (mutex->autoinit != 0)
; /* wait for other thread */
When thread B arrives here, it sees the value of autoinit that thread A
has written above.
HOWEVER, when it continues, there is NO [*] guarantee that it will also
see the values that InitializeCriticalSection() has written, because
there were no memory barriers involved. When it continues, there is a
chance that it calls EnterCriticalSection() with uninitialized values!
}
[*] If you compile this code with MSVC >= 2005, "No guarantee" is not
true, it's exactly the opposite because Microsoft extended the meaning
of 'volatile' to imply a memory barriere. This is *NOT* true for gcc in
general. It may be true for MinGW gcc, but I do not know.
> Basically, the thread that gets the original 1 returned from
> InterlockedCompareExchange is the only one who writes to
> mutex->autoinit. All other threads only read the value, and the
> volatile should make sure they actually do. Since all 32-bit reads and
> writes are atomic on Windows (see
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms684122(v=vs.85).aspx
> "Simple reads and writes to properly-aligned 32-bit variables are
> atomic operations.") and mutex->autoinit is a LONG, this should be
> safe AFAICT. In fact, Windows specifically does not have any
> explicitly atomic writes exactly for this reason.
There is a difference between atomic and coherent: Yes, 32-bit accesses
are atomic, but they are not automatically coherent: A 32-bit value
written by one CPU is not instantly visible on the other CPU. 'volatile'
as per the C lanugage does not add any guarantees that would be of
interest here. OTOH, Microsoft's definition of 'volatile' does.
> The only ways mutex->autoinit can be updated is:
> - InterlockedCompareExchange compares it to 1, finds it's identical
> and inserts -1
> - intialization is done
> Both these updates happens from the same thread.
>
> Yes, details like this should probably go into the commit message ;)
A comment in the function is preferred!
-- Hannes
^ permalink raw reply
* Re: [PATCH/WIP 02/11] notes-merge: use opendir/readdir instead of using read_directory()
From: Junio C Hamano @ 2011-10-25 19:27 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1319438176-7304-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> notes_merge_commit() only needs to list all entries (non-recursively)
> under a directory, which can be easily accomplished with
> opendir/readdir and would be more lightweight than read_directory().
>
> read_directory() is designed to list paths inside a working
> directory. Using it outside of its scope may lead to undesired effects.
Technically isn't the directory structure this codepath looks at a working
tree that has extract of a notes tree commit?
Looking at the result of the patch I do not have strong opinions either
way, though. It isn't like we care about gitignore or attributes rules in
the notes tree, so using read_directory() does feel like an overkill.
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> notes-merge.c | 45 +++++++++++++++++++++++++++------------------
> 1 files changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/notes-merge.c b/notes-merge.c
> index e9e4199..80d64a2 100644
> --- a/notes-merge.c
> +++ b/notes-merge.c
> @@ -680,48 +680,57 @@ int notes_merge_commit(struct notes_merge_options *o,
> * commit message and parents from 'partial_commit'.
> * Finally store the new commit object SHA1 into 'result_sha1'.
> */
> - struct dir_struct dir;
> - char *path = xstrdup(git_path(NOTES_MERGE_WORKTREE "/"));
> - int path_len = strlen(path), i;
> + DIR *dir;
> + struct dirent *e;
> + struct strbuf path = STRBUF_INIT;
> const char *msg = strstr(partial_commit->buffer, "\n\n");
> + int baselen;
>
> - OUTPUT(o, 3, "Committing notes in notes merge worktree at %.*s",
> - path_len - 1, path);
> + strbuf_addstr(&path, git_path(NOTES_MERGE_WORKTREE));
> + OUTPUT(o, 3, "Committing notes in notes merge worktree at %s", path.buf);
>
> if (!msg || msg[2] == '\0')
> die("partial notes commit has empty message");
> msg += 2;
>
> - memset(&dir, 0, sizeof(dir));
> - read_directory(&dir, path, path_len, NULL);
> - for (i = 0; i < dir.nr; i++) {
> - struct dir_entry *ent = dir.entries[i];
> + dir = opendir(path.buf);
> + if (!dir)
> + die_errno("could not open %s", path.buf);
> +
> + strbuf_addch(&path, '/');
> + baselen = path.len;
> + while ((e = readdir(dir)) != NULL) {
> struct stat st;
> - const char *relpath = ent->name + path_len;
> unsigned char obj_sha1[20], blob_sha1[20];
>
> - if (ent->len - path_len != 40 || get_sha1_hex(relpath, obj_sha1)) {
> - OUTPUT(o, 3, "Skipping non-SHA1 entry '%s'", ent->name);
> + if (is_dot_or_dotdot(e->d_name))
> + continue;
> +
> + if (strlen(e->d_name) != 40 || get_sha1_hex(e->d_name, obj_sha1)) {
> + OUTPUT(o, 3, "Skipping non-SHA1 entry '%s%s'", path.buf, e->d_name);
> continue;
> }
>
> + strbuf_addstr(&path, e->d_name);
> /* write file as blob, and add to partial_tree */
> - if (stat(ent->name, &st))
> - die_errno("Failed to stat '%s'", ent->name);
> - if (index_path(blob_sha1, ent->name, &st, HASH_WRITE_OBJECT))
> - die("Failed to write blob object from '%s'", ent->name);
> + if (stat(path.buf, &st))
> + die_errno("Failed to stat '%s'", path.buf);
> + if (index_path(blob_sha1, path.buf, &st, HASH_WRITE_OBJECT))
> + die("Failed to write blob object from '%s'", path.buf);
> if (add_note(partial_tree, obj_sha1, blob_sha1, NULL))
> die("Failed to add resolved note '%s' to notes tree",
> - ent->name);
> + path.buf);
> OUTPUT(o, 4, "Added resolved note for object %s: %s",
> sha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));
> + strbuf_setlen(&path, baselen);
> }
>
> create_notes_commit(partial_tree, partial_commit->parents, msg,
> result_sha1);
> OUTPUT(o, 4, "Finalized notes merge commit: %s",
> sha1_to_hex(result_sha1));
> - free(path);
> + strbuf_release(&path);
> + closedir(dir);
> return 0;
> }
^ permalink raw reply
* Re: [PATCH/WIP 05/11] symbolize return values of tree_entry_interesting()
From: Junio C Hamano @ 2011-10-25 19:24 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1319438176-7304-6-git-send-email-pclouds@gmail.com>
Makes it a lot easier to read for first-time readers. Nice.
Just one minor formatting nit of lacking SP near ":", though.
^ permalink raw reply
* Re: [PATCH/WIP 04/11] tree-walk.c: do not leak internal structure in tree_entry_len()
From: Junio C Hamano @ 2011-10-25 19:20 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1319438176-7304-5-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> tree_entry_len() does not simply take two random arguments and return
> a tree length. The two pointers must point to a tree item structure,
> or struct name_entry. Passing random pointers will return incorrect
> value.
>
> Force callers to pass struct name_entry instead of two pointers (with
> hope that they don't manually construct struct name_entry themselves)
Makes quite a lot of sense.
^ permalink raw reply
* Re: [PATCH/WIP 03/11] t5403: avoid doing "git add foo/bar" where foo/.git exists
From: Junio C Hamano @ 2011-10-25 19:19 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1319438176-7304-4-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> In this case, "foo" is considered a submodule and bar, if added,
> belongs to foo/.git. "git add" should only allow "git add foo" in this
> case, but it passes somehow.
I do not think the above description is correct.
The test:
- populates the current directory;
- makes a clone in ./clone2;
- creates a file clone2/b;
- runs "git add clone2/b" with GIT_DIR set to clone2/.git, without
setting GIT_WORK_TREE nor having core.worktree in clone2/.git/config.
The last step should add a path "clone2/b" to $GIT_DIR/index (which is
clone2/.git/index in this case). The clone2 is not a submodule to the top
level repository in this case, but even if it were, that would not change
the definition of what the command should do when GIT_DIR is set without
GIT_WORK_TREE nor core.worktree in $GIT_DIR/config.
Running (cd clone2 && git add b) is a _more natural_ thing to do, and it
will result in a path "b" added to the clone2 repository, so that the
result is more useful if you are going to chdir to the repository and keep
working on it. But that does not mean the existing test is incorrect. It
does not just pass somehow but the test passes by design.
I did not check if later tests look at the contents of commit "new2" to
make sure it contains "clone2/b", but if they do this change should break
such tests.
So I am puzzled by this change; what is this trying to achieve?
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> t/t5403-post-checkout-hook.sh | 17 ++++++++++-------
> 1 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
> index 1753ef2..3b3e2c1 100755
> --- a/t/t5403-post-checkout-hook.sh
> +++ b/t/t5403-post-checkout-hook.sh
> @@ -16,10 +16,13 @@ test_expect_success setup '
> git update-ref refs/heads/master $commit0 &&
> git clone ./. clone1 &&
> git clone ./. clone2 &&
> - GIT_DIR=clone2/.git git branch new2 &&
> - echo Data for commit1. >clone2/b &&
> - GIT_DIR=clone2/.git git add clone2/b &&
> - GIT_DIR=clone2/.git git commit -m new2
> + (
> + cd clone2 &&
> + git branch new2 &&
> + echo Data for commit1. >b &&
> + git add b &&
> + git commit -m new2
> + )
> '
>
> for clone in 1 2; do
> @@ -48,7 +51,7 @@ test_expect_success 'post-checkout runs as expected ' '
> '
>
> test_expect_success 'post-checkout args are correct with git checkout -b ' '
> - GIT_DIR=clone1/.git git checkout -b new1 &&
> + ( cd clone1 && git checkout -b new1 ) &&
> old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
> new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
> flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
> @@ -56,7 +59,7 @@ test_expect_success 'post-checkout args are correct with git checkout -b ' '
> '
>
> test_expect_success 'post-checkout receives the right args with HEAD changed ' '
> - GIT_DIR=clone2/.git git checkout new2 &&
> + ( cd clone2 && git checkout new2 ) &&
> old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
> new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
> flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
> @@ -64,7 +67,7 @@ test_expect_success 'post-checkout receives the right args with HEAD changed ' '
> '
>
> test_expect_success 'post-checkout receives the right args when not switching branches ' '
> - GIT_DIR=clone2/.git git checkout master b &&
> + ( cd clone2 && git checkout master b ) &&
> old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
> new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
> flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
^ permalink raw reply
* Re: pull is not a git command - 1.7.6.4
From: Eugene Sajine @ 2011-10-25 18:10 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Junio C Hamano, git
In-Reply-To: <vpqr521j98h.fsf@bauges.imag.fr>
On Tue, Oct 25, 2011 at 1:01 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Eugene Sajine <euguess@gmail.com> writes:
>
>> On Tue, Oct 25, 2011 at 12:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> Just a wild guess. perhaps you specified prefix=/usr/local/git-1.7.4.1/
>>> eons ago when you built and installed 1.7.4.1 like this:
>>>
>>> make prefix=/usr/local/git-1.7.4.1 all install
>>>
>>> and then you did it differently when you installed 1.7.6.4, e.g.
>>>
>>> make all
>>> make prefix=/usr/local/git-1.7.6.4 install
>>>
>>>
>>
>>
>> Are you saying that the first command is more correct?
>> I will check it.
>
> At build time, Git registers the "exec path" (i.e. where to find
> git-<command> executables). So, if you run "make all" without specifying
> the install path, Git will set an arbitrary exec-path, and the
> installation won't work.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
>
Matthieu/Junio,
Thank you very much for your help - there was a mistake made during
the build where the exec path folder was incorrect and unreachable.
Thanks!
^ permalink raw reply
* [PATCH] completion: fix issue with process substitution not working on Git for Windows
From: Stefan Naewe @ 2011-10-25 18:01 UTC (permalink / raw)
To: spearce; +Cc: git, gitster, Stefan Naewe
Git for Windows comes with a bash that doesn't support process substitution.
It issues the following error when using git-completion.bash with
GIT_PS1_SHOWUPSTREAM set:
$ export GIT_PS1_SHOWUPSTREAM=1
sh.exe": cannot make pipe for process substitution: Function not implemented
sh.exe": cannot make pipe for process substitution: Function not implemented
sh.exe": <(git config -z --get-regexp '^(svn-remote\..*\.url|bash\.showupstream)$' 2>/dev/null | tr '\0\n' '\n '): ambiguous redirect
Replace the process substitution with a simple "echo $var | while...".
Signed-off-by: Stefan Naewe <stefan.naewe@gmail.com>
---
contrib/completion/git-completion.bash | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8648a36..926db80 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -110,6 +110,8 @@ __git_ps1_show_upstream ()
local upstream=git legacy="" verbose=""
# get some config options from git-config
+ output="$(git config -z --get-regexp '^(svn-remote\..*\.url|bash\.showupstream)$' 2>/dev/null | tr '\0\n' '\n ')"
+ echo "$output" | \
while read key value; do
case "$key" in
bash.showupstream)
@@ -125,7 +127,7 @@ __git_ps1_show_upstream ()
upstream=svn+git # default upstream is SVN if available, else git
;;
esac
- done < <(git config -z --get-regexp '^(svn-remote\..*\.url|bash\.showupstream)$' 2>/dev/null | tr '\0\n' '\n ')
+ done
# parse configuration values
for option in ${GIT_PS1_SHOWUPSTREAM}; do
--
1.7.7.1
^ permalink raw reply related
* Re: [PATCH] read-cache.c: fix index memory allocation
From: René Scharfe @ 2011-10-25 18:00 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Junio C Hamano, Jeff King, John Hsing, Matthieu Moy, git
In-Reply-To: <CACsJy8A7yVk15aAgqDkKTz31rChA7Oj-kS2VT2y2tWS6h01GyA@mail.gmail.com>
Am 25.10.2011 02:01, schrieb Nguyen Thai Ngoc Duy:
> On Tue, Oct 25, 2011 at 10:34 AM, Nguyen Thai Ngoc Duy
> <pclouds@gmail.com> wrote:
>> "git status" is slow. If your changes causes slowdown, it won't likely
>> stand out while other fast commands may show (read_cache() is used in
>> nearly all commands). So I tested using the following patch.
>>
>> The result on linux-2.6 shows about 10-20 us slowdown per each
>> read_cache() call (30-40 us on webkit, ~50k files) I think your patch
>> is good enough :-)
>
> That was with -O0 by the way. valgrind/massif shows about 200kb memory
> more with your patch on webkit repository (7.497 MB vs 7.285 MB),
> using the same test, so memory overhead is ok too.
We can reduce that a bit -- unless block allocation of index entries
is still done somewhere.
-- >8 --
Subject: [PATCH 2/1] cache.h: put single NUL at end of struct cache_entry
Since in-memory index entries are allocated individually now, the
variable slack at the end meant to provide an eight byte alignment
is not needed anymore. Have a single NUL instead. This saves zero
to seven bytes for an entry, depending on its filename length.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
cache.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/cache.h b/cache.h
index ec0e571..bd106b5 100644
--- a/cache.h
+++ b/cache.h
@@ -306,7 +306,7 @@ static inline unsigned int canon_mode(unsigned int mode)
}
#define flexible_size(STRUCT,len) ((offsetof(struct STRUCT,name) + (len) + 8) & ~7)
-#define cache_entry_size(len) flexible_size(cache_entry,len)
+#define cache_entry_size(len) (offsetof(struct cache_entry,name) + (len) + 1)
#define ondisk_cache_entry_size(len) flexible_size(ondisk_cache_entry,len)
#define ondisk_cache_entry_extended_size(len) flexible_size(ondisk_cache_entry_extended,len)
--
1.7.7
^ permalink raw reply related
* [PATCH] git grep: be careful to use mutices only when they are initialized
From: Johannes Schindelin @ 2011-10-25 17:25 UTC (permalink / raw)
To: msysgit; +Cc: gitster, git
Rather nasty things happen when a mutex is not initialized but locked
nevertheless. Now, when we're not running in a threaded manner, the mutex
is not initialized, which is correct. But then we went and used the mutex
anyway, which -- at least on Windows -- leads to a hard crash (ordinarily
it would be called a segmentation fault, but in Windows speak it is an
access violation).
This problem was identified by our faithful tests when run in the msysGit
environment.
To avoid having to wrap the line due to the 80 column limit, we use
the name "WHEN_THREADED" instead of "IF_USE_THREADS" because it is one
character shorter. Which is all we need in this case.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
I looked around a bit but ran out of time to identify the reason why
this was not caught earlier.
builtin/grep.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 92eeada..e94c5fe 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -78,10 +78,11 @@ static pthread_mutex_t grep_mutex;
/* Used to serialize calls to read_sha1_file. */
static pthread_mutex_t read_sha1_mutex;
-#define grep_lock() pthread_mutex_lock(&grep_mutex)
-#define grep_unlock() pthread_mutex_unlock(&grep_mutex)
-#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)
-#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)
+#define WHEN_THREADED(x) do { if (use_threads) (x); } while (0)
+#define grep_lock() WHEN_THREADED(pthread_mutex_lock(&grep_mutex))
+#define grep_unlock() WHEN_THREADED(pthread_mutex_unlock(&grep_mutex))
+#define read_sha1_lock() WHEN_THREADED(pthread_mutex_lock(&read_sha1_mutex))
+#define read_sha1_unlock() WHEN_THREADED(pthread_mutex_unlock(&read_sha1_mutex))
/* Signalled when a new work_item is added to todo. */
static pthread_cond_t cond_add;
--
1.7.5.3.4540.g15f89
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox