* Reporting Bug in Git Version Control System
From: Yash Jain @ 2016-10-24 14:28 UTC (permalink / raw)
To: git
Hello,
I have two accounts on github("yj291197" and "yaki29").
Both the accounts have different gmail IDs("yj291197@gmail.com" and
"yashjain.lnm@gmail.com" respectively) but same passwords.
I used to use git for "yj291197" account and a few days earlier I made
this new account and used git commit to commit on "yaki29" but it
appeared as "yj291197" committed on "yaki29's" repo.
Then I pulled a request of that commit then it appeared "yaki29"
pulled a request with a commit of "yj291197".
And during this whole session I was signed in as "yaki29" on github.com .
Please reply ....
^ permalink raw reply
* [PATCH] gitk: Fix Japanese translation for "marked commit"
From: Satoshi Yasushima @ 2016-10-24 15:35 UTC (permalink / raw)
To: git; +Cc: Paul Mackerras, Satoshi Yasushima
Signed-off-by: Satoshi Yasushima <s.yasushima@gmail.com>
---
po/ja.po | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/po/ja.po b/po/ja.po
index f143753..510306b 100644
--- a/po/ja.po
+++ b/po/ja.po
@@ -2,16 +2,17 @@
# Copyright (C) 2005-2015 Paul Mackerras
# This file is distributed under the same license as the gitk package.
#
-# YOKOTA Hiroshi <yokota@netlab.cs.tsukuba.ac.jp>, 2015.
# Mizar <mizar.jp@gmail.com>, 2009.
# Junio C Hamano <gitster@pobox.com>, 2009.
+# YOKOTA Hiroshi <yokota@netlab.cs.tsukuba.ac.jp>, 2015.
+# Satoshi Yasushima <s.yasushima@gmail.com>, 2016.
msgid ""
msgstr ""
"Project-Id-Version: gitk\n"
"Report-Msgid-Bugs-To: \n"
"POT-Creation-Date: 2015-05-17 14:32+1000\n"
"PO-Revision-Date: 2015-11-12 13:00+0900\n"
-"Last-Translator: YOKOTA Hiroshi <yokota@netlab.cs.tsukuba.ac.jp>\n"
+"Last-Translator: Satoshi Yasushima <s.yasushima@gmail.com>\n"
"Language-Team: Japanese\n"
"Language: ja\n"
"MIME-Version: 1.0\n"
@@ -314,11 +315,11 @@ msgstr "マークを付けたコミットと比較する"
#: gitk:2630 gitk:2641
msgid "Diff this -> marked commit"
-msgstr "これと選択したコミットのdiffを見る"
+msgstr "これとマークを付けたコミットのdiffを見る"
#: gitk:2631 gitk:2642
msgid "Diff marked commit -> this"
-msgstr "選択したコミットとこれのdiffを見る"
+msgstr "マークを付けたコミットとこれのdiffを見る"
#: gitk:2632
msgid "Revert this commit"
--
2.10.1.windows.1
^ permalink raw reply related
* Re: RFC Failover url for fetches?
From: Junio C Hamano @ 2016-10-24 16:54 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <7fdaa160-9262-5d52-7035-8362ca94beea@gmail.com>
Jakub Narębski <jnareb@gmail.com> writes:
>> As to fetching from two or more places as "fallback", I am
>> moderately negative to add it as a dumb feature that does nothing
>> more than "My fetch from A failed, so let's blindly try it from B".
>> I'd prefer to keep the "My fetch from A is failing" knowledge near
>> the surface of end user's consciousness as a mechanism to pressure A
>> to fix it--that way everybody who is fetching from A benefits.
>> After all, doing "git remote add B" once (you'd need to tell the URL
>> for B anyway to Git) and issuing "git fetch B" after seeing your
>> regular "git fetch" fails once in a blue moon is not all that
>> cumbersome, I would think.
>
> One would need to configure fallback B remote to use the same
> remote-branch namespace as remote A, if it is to be used as fallback,
> I would think.
Yeah, I left it out because I thought that was obvious, but spelling
it out explicitly may have helped those who weren't reading carefully.
Thanks
^ permalink raw reply
* Re: [PATCH] hex: use unsigned index for ring buffer
From: Junio C Hamano @ 2016-10-24 17:15 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Git List
In-Reply-To: <20161024130015.awlmgpfzixiy6wkb@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> > You could also write the second line like:
>> >
>> > bufno %= ARRAY_SIZE(hexbuffer);
>> >
>> > which is less magical (right now the set of buffers must be a power of
>> > 2). I expect the compiler could turn that into a bitmask itself.
>> ...
>
> I think it would be preferable to just fix it inline in each place.
Yeah, probably.
My initial reaction to this was
char *sha1_to_hex(const unsigned char *sha1)
{
- static int bufno;
+ static unsigned int bufno;
static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
return sha1_to_hex_r(hexbuffer[3 & ++bufno], sha1);
"ah, we do not even need bufno as uint; it could be ushort or even
uchar". If this were a 256 element ring buffer and the index were
uchar, we wouldn't even be having this discussion, and "3 &" is a
way to get a fake type that is a 2-bit unsigned integer that wraps
around when incremented.
But being explicit, especially when we know that we can rely on the
fact that the compilers are usually intelligent enough, is a good
idea, I would think.
Isn't size_t often wider than uint, by the way? It somehow makes me
feel dirty to use it when we know we only care about the bottom two
bit, especially with the explicit "bufno %= ARRAY_SIZE(hexbuffer)",
but I may be simply superstitious in this case. I dunno.
^ permalink raw reply
* Re: [PATCH] hex: use unsigned index for ring buffer
From: Junio C Hamano @ 2016-10-24 17:27 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Git List
In-Reply-To: <xmqqwpgx7jn6.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>> I think it would be preferable to just fix it inline in each place.
>
> Yeah, probably.
>
> My initial reaction to this was
>
> char *sha1_to_hex(const unsigned char *sha1)
> {
> - static int bufno;
> + static unsigned int bufno;
> static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
> return sha1_to_hex_r(hexbuffer[3 & ++bufno], sha1);
>
> "ah, we do not even need bufno as uint; it could be ushort or even
> uchar". If this were a 256 element ring buffer and the index were
> uchar, we wouldn't even be having this discussion, and "3 &" is a
> way to get a fake type that is a 2-bit unsigned integer that wraps
> around when incremented.
>
> But being explicit, especially when we know that we can rely on the
> fact that the compilers are usually intelligent enough, is a good
> idea, I would think.
>
> Isn't size_t often wider than uint, by the way? It somehow makes me
> feel dirty to use it when we know we only care about the bottom two
> bit, especially with the explicit "bufno %= ARRAY_SIZE(hexbuffer)",
> but I may be simply superstitious in this case. I dunno.
If we are doing the wrap-around ourselves, I suspect that the index
should stay "int" (not even unsigned), as that is supposed to be the
most natural and performant type on the architecture. Would it
still result in better code to use size_t instead?
Author: René Scharfe <l.s.r@web.de>
Date: Sun Oct 23 19:57:30 2016 +0200
hex: make wraparound of the index into ring-buffer explicit
Overflow is defined for unsigned integers, but not for signed ones.
We could make the ring-buffer index in sha1_to_hex() and
get_pathname() unsigned to be on the safe side to resolve this, but
let's make it explicit that we are wrapping around at whatever the
number of elements the ring-buffer has. The compiler is smart enough
to turn modulus into bitmask for these codepaths that use
ring-buffers of a size that is a power of 2.
---
cache.h | 3 +++
hex.c | 4 ++--
path.c | 4 ++--
3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/cache.h b/cache.h
index 4cba08ecb1..5429df0f92 100644
--- a/cache.h
+++ b/cache.h
@@ -547,6 +547,9 @@ extern int daemonize(void);
} \
} while (0)
+#define NEXT_RING_ITEM(array, index) \
+ (array)[(index) = ((index) + 1) % ARRAY_SIZE(array)]
+
/* Initialize and use the cache information */
struct lock_file;
extern int read_index(struct index_state *);
diff --git a/hex.c b/hex.c
index ab2610e498..5e711b9e32 100644
--- a/hex.c
+++ b/hex.c
@@ -76,9 +76,9 @@ char *oid_to_hex_r(char *buffer, const struct object_id *oid)
char *sha1_to_hex(const unsigned char *sha1)
{
- static int bufno;
+ static size_t bufno;
static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
- return sha1_to_hex_r(hexbuffer[3 & ++bufno], sha1);
+ return sha1_to_hex_r(NEXT_RING_ITEM(hexbuffer, bufno), sha1);
}
char *oid_to_hex(const struct object_id *oid)
diff --git a/path.c b/path.c
index fe3c4d96c6..5b2ab2271f 100644
--- a/path.c
+++ b/path.c
@@ -23,8 +23,8 @@ static struct strbuf *get_pathname(void)
static struct strbuf pathname_array[4] = {
STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
};
- static int index;
- struct strbuf *sb = &pathname_array[3 & ++index];
+ static size_t index;
+ struct strbuf *sb = &NEXT_RING_ITEM(pathname_array, index);
strbuf_reset(sb);
return sb;
}
^ permalink raw reply related
* Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"
From: Junio C Hamano @ 2016-10-24 17:46 UTC (permalink / raw)
To: Johannes Sixt
Cc: Stefan Beller, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <61637cd9-8f83-c988-15c0-54f948153c07@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 22.10.2016 um 22:46 schrieb Stefan Beller:
>> I have looked into it again, and by now I think the bug is a feature,
>> actually.
>>
>> Consider this:
>>
>> git clone . super
>> git -C super submodule add ../submodule
>> # we thought the previous line is buggy
>> git clone super super-clone
>
> At this point, we *should* have this if there were no bugs (at least
> that is my assumption):
>
> /tmp
> !
> + submodule <- submodule's remote repo
> !
> + foo <- we are here (.), super's remote repo
> !
> + super <- remote.origin.url=/tmp/foo/.
> !
> + submodule <- remote.origin.url=/tmp/foo/./../submodule
> submodule.submodule.url=../submodule
>
> When I test this, 'git submodule add' fails:
>
> foo@master> git -C super submodule add ../submodule
> fatal: repository '/tmp/foo/submodule' does not exist
> fatal: clone of '/tmp/foo/submodule' into submodule path
> '/tmp/foo/super/submodule' failed
>
>> Now in the super-clone the ../submodule is the correct
>> relative url, because the url where we cloned from doesn't
>> end in /.
>
> I do not understand why this would be relevant. The question is not
> how the submodule's remote URL ends, but how the submodule's remote
> URL is constructed from the super-project's URL and the relative path
> specified for 'git submodule add'.
FWIW, that matches my understanding.
> Whether ../submodule or ./submodule is the correct relative URL
> depends on where the origin of the submodule is located relative to
> the origin of the super-project. In the above example, it is
> ../submodule. However, the error message tells us that git looked in
> /tmp/foo/submodule, which looks like the /. bug!
>
> I do not understand where you see a feature here. What am I missing?
>
> -- Hannes
^ permalink raw reply
* Re: [PATCH 0/4] nd/ita-empty-commit update
From: Junio C Hamano @ 2016-10-24 17:58 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20161024104222.31128-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> The index_differs_from() also takes a flag to set/clear this new
> flag instead of relying on has_ita_entries like the old 2/3.
I think that probably is a good move.
> The name ita-invisible-in-index is not perfect but I could not think
> of any better. Another name could be diff-cached-ignores-ita, but
> that's just half of what it does. The other half is diff-files-includes-ita...
I can't either, and it is one of the reasons why I am reluctant.
Not being able to be named with a short-and-sweet name often is a
sign that the thing to be named is conceptually not well thought
out.
But as we need to give it some name to the flat to ease
experimenting, let's take that name as-is.
^ permalink raw reply
* [PATCH v2 0/2] Use CLOEXEC to avoid fd leaks
From: larsxschneider @ 2016-10-24 18:02 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, e, jnareb, gitster, Lars Schneider
From: Lars Schneider <larsxschneider@gmail.com>
Use the CLOEXEC flag similar to 05d1ed61 to fix leaked file descriptors.
This mini patch series is necessary to make the "ls/filter-process" topic
work properly for Windows. Right now the topic generates test failures
on Windows as reported by Dscho:
http://public-inbox.org/git/alpine.DEB.2.20.1610211457030.3264@virtualbox/
Patch 1/2 was contemplated by Junio here:
http://public-inbox.org/git/xmqq37lnbbpk.fsf@gitster.mtv.corp.google.com/
Thanks to
Eric, Jakub, Dscho, and Junio for the review of v1,
Lars
## Changes since v1
* add fallbacks in case O_CLOEXEC is not available
* rename 'git_open_noatime_cloexec' to 'git_open' (Eric, Dscho)
* rebased the topic on `next` to fix a merge conflict
Lars Schneider (2):
sha1_file: open window into packfiles with CLOEXEC
read-cache: make sure file handles are not inherited by child
processes
builtin/pack-objects.c | 2 +-
cache.h | 2 +-
pack-bitmap.c | 2 +-
read-cache.c | 6 +++++-
sha1_file.c | 26 ++++++++++++++++----------
5 files changed, 24 insertions(+), 14 deletions(-)
## Interdiff (v1..v2)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 0fdd331..0fd52bd 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -720,7 +720,7 @@ static off_t write_reused_pack(struct sha1file *f)
if (!is_pack_valid(reuse_packfile))
die("packfile is invalid: %s", reuse_packfile->pack_name);
- fd = git_open_noatime_cloexec(reuse_packfile->pack_name);
+ fd = git_open(reuse_packfile->pack_name);
if (fd < 0)
die_errno("unable to open packfile for reuse: %s",
reuse_packfile->pack_name);
diff --git a/cache.h b/cache.h
index 0dea548..419b7a0 100644
--- a/cache.h
+++ b/cache.h
@@ -1125,7 +1125,7 @@ extern int write_sha1_file(const void *buf, unsigned long len, const char *type,
extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);
extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
extern int force_object_loose(const unsigned char *sha1, time_t mtime);
-extern int git_open_noatime_cloexec(const char *name);
+extern int git_open(const char *name);
extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);
extern int parse_sha1_header(const char *hdr, unsigned long *sizep);
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 1b39e5d..39bcc16 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -266,7 +266,7 @@ static int open_pack_bitmap_1(struct packed_git *packfile)
return -1;
idx_name = pack_bitmap_filename(packfile);
- fd = git_open_noatime_cloexec(idx_name);
+ fd = git_open(idx_name);
free(idx_name);
if (fd < 0)
diff --git a/read-cache.c b/read-cache.c
index 200d4fa..b594865 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -158,6 +158,10 @@ static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
int match = -1;
int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
+ if (O_CLOEXEC && fd < 0 && errno == EINVAL)
+ /* Try again w/o O_CLOEXEC: the kernel might not support it */
+ fd = open(ce->name, O_RDONLY);
+
if (fd >= 0) {
unsigned char sha1[20];
if (!index_fd(sha1, fd, st, OBJ_BLOB, ce->name, 0))
diff --git a/sha1_file.c b/sha1_file.c
index dbe027b..93b836b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -370,7 +370,7 @@ void read_info_alternates(const char * relative_base, int depth)
int fd;
path = xstrfmt("%s/info/alternates", relative_base);
- fd = git_open_noatime_cloexec(path);
+ fd = git_open(path);
free(path);
if (fd < 0)
return;
@@ -663,7 +663,7 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)
struct pack_idx_header *hdr;
size_t idx_size;
uint32_t version, nr, i, *index;
- int fd = git_open_noatime_cloexec(path);
+ int fd = git_open(path);
struct stat st;
if (fd < 0)
@@ -1069,7 +1069,7 @@ static int open_packed_git_1(struct packed_git *p)
while (pack_max_fds <= pack_open_fds && close_one_pack())
; /* nothing */
- p->pack_fd = git_open_noatime_cloexec(p->pack_name);
+ p->pack_fd = git_open(p->pack_name);
if (p->pack_fd < 0 || fstat(p->pack_fd, &st))
return -1;
pack_open_fds++;
@@ -1586,7 +1586,7 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
return hashcmp(sha1, real_sha1) ? -1 : 0;
}
-int git_open_noatime_cloexec(const char *name)
+int git_open(const char *name)
{
static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
@@ -1598,12 +1598,18 @@ int git_open_noatime_cloexec(const char *name)
if (fd >= 0)
return fd;
- /* Might the failure be due to O_NOATIME? */
- if (errno != ENOENT && sha1_file_open_flag) {
- sha1_file_open_flag = 0;
+ /* Try again w/o O_CLOEXEC: the kernel might not support it */
+ if (O_CLOEXEC && errno == EINVAL &&
+ (sha1_file_open_flag & O_CLOEXEC)) {
+ sha1_file_open_flag &= ~O_CLOEXEC;
continue;
}
+ /* Might the failure be due to O_NOATIME? */
+ if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
+ sha1_file_open_flag &= ~O_NOATIME;
+ continue;
+ }
return -1;
}
}
@@ -1632,7 +1638,7 @@ static int open_sha1_file(const unsigned char *sha1)
struct alternate_object_database *alt;
int most_interesting_errno;
- fd = git_open_noatime_cloexec(sha1_file_name(sha1));
+ fd = git_open(sha1_file_name(sha1));
if (fd >= 0)
return fd;
most_interesting_errno = errno;
@@ -1640,7 +1646,7 @@ static int open_sha1_file(const unsigned char *sha1)
prepare_alt_odb();
for (alt = alt_odb_list; alt; alt = alt->next) {
const char *path = alt_sha1_path(alt, sha1);
- fd = git_open_noatime_cloexec(path);
+ fd = git_open(path);
if (fd >= 0)
return fd;
if (most_interesting_errno == ENOENT)
--
2.10.0
^ permalink raw reply related
* [PATCH v2 1/2] sha1_file: open window into packfiles with CLOEXEC
From: larsxschneider @ 2016-10-24 18:02 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, e, jnareb, gitster, Lars Schneider
In-Reply-To: <20161024180300.52359-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
All processes that the Git main process spawns inherit the open file
descriptors of the main process. These leaked file descriptors can
cause problems.
Use the CLOEXEC flag similar to 05d1ed61 to fix the leaked file
descriptors. Since `git_open_noatime` does not describe the function
properly anymore rename it to `git_open`.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
builtin/pack-objects.c | 2 +-
cache.h | 2 +-
pack-bitmap.c | 2 +-
sha1_file.c | 26 ++++++++++++++++----------
4 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1e7c2a9..0fd52bd 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -720,7 +720,7 @@ static off_t write_reused_pack(struct sha1file *f)
if (!is_pack_valid(reuse_packfile))
die("packfile is invalid: %s", reuse_packfile->pack_name);
- fd = git_open_noatime(reuse_packfile->pack_name);
+ fd = git_open(reuse_packfile->pack_name);
if (fd < 0)
die_errno("unable to open packfile for reuse: %s",
reuse_packfile->pack_name);
diff --git a/cache.h b/cache.h
index b7f34b4..419b7a0 100644
--- a/cache.h
+++ b/cache.h
@@ -1125,7 +1125,7 @@ extern int write_sha1_file(const void *buf, unsigned long len, const char *type,
extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);
extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
extern int force_object_loose(const unsigned char *sha1, time_t mtime);
-extern int git_open_noatime(const char *name);
+extern int git_open(const char *name);
extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);
extern int parse_sha1_header(const char *hdr, unsigned long *sizep);
diff --git a/pack-bitmap.c b/pack-bitmap.c
index b949e51..39bcc16 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -266,7 +266,7 @@ static int open_pack_bitmap_1(struct packed_git *packfile)
return -1;
idx_name = pack_bitmap_filename(packfile);
- fd = git_open_noatime(idx_name);
+ fd = git_open(idx_name);
free(idx_name);
if (fd < 0)
diff --git a/sha1_file.c b/sha1_file.c
index 1e41954..93b836b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -370,7 +370,7 @@ void read_info_alternates(const char * relative_base, int depth)
int fd;
path = xstrfmt("%s/info/alternates", relative_base);
- fd = git_open_noatime(path);
+ fd = git_open(path);
free(path);
if (fd < 0)
return;
@@ -663,7 +663,7 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)
struct pack_idx_header *hdr;
size_t idx_size;
uint32_t version, nr, i, *index;
- int fd = git_open_noatime(path);
+ int fd = git_open(path);
struct stat st;
if (fd < 0)
@@ -1069,7 +1069,7 @@ static int open_packed_git_1(struct packed_git *p)
while (pack_max_fds <= pack_open_fds && close_one_pack())
; /* nothing */
- p->pack_fd = git_open_noatime(p->pack_name);
+ p->pack_fd = git_open(p->pack_name);
if (p->pack_fd < 0 || fstat(p->pack_fd, &st))
return -1;
pack_open_fds++;
@@ -1586,9 +1586,9 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
return hashcmp(sha1, real_sha1) ? -1 : 0;
}
-int git_open_noatime(const char *name)
+int git_open(const char *name)
{
- static int sha1_file_open_flag = O_NOATIME;
+ static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
for (;;) {
int fd;
@@ -1598,12 +1598,18 @@ int git_open_noatime(const char *name)
if (fd >= 0)
return fd;
- /* Might the failure be due to O_NOATIME? */
- if (errno != ENOENT && sha1_file_open_flag) {
- sha1_file_open_flag = 0;
+ /* Try again w/o O_CLOEXEC: the kernel might not support it */
+ if (O_CLOEXEC && errno == EINVAL &&
+ (sha1_file_open_flag & O_CLOEXEC)) {
+ sha1_file_open_flag &= ~O_CLOEXEC;
continue;
}
+ /* Might the failure be due to O_NOATIME? */
+ if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
+ sha1_file_open_flag &= ~O_NOATIME;
+ continue;
+ }
return -1;
}
}
@@ -1632,7 +1638,7 @@ static int open_sha1_file(const unsigned char *sha1)
struct alternate_object_database *alt;
int most_interesting_errno;
- fd = git_open_noatime(sha1_file_name(sha1));
+ fd = git_open(sha1_file_name(sha1));
if (fd >= 0)
return fd;
most_interesting_errno = errno;
@@ -1640,7 +1646,7 @@ static int open_sha1_file(const unsigned char *sha1)
prepare_alt_odb();
for (alt = alt_odb_list; alt; alt = alt->next) {
const char *path = alt_sha1_path(alt, sha1);
- fd = git_open_noatime(path);
+ fd = git_open(path);
if (fd >= 0)
return fd;
if (most_interesting_errno == ENOENT)
--
2.10.0
^ permalink raw reply related
* [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes
From: larsxschneider @ 2016-10-24 18:03 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, e, jnareb, gitster, Lars Schneider
In-Reply-To: <20161024180300.52359-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
This fixes "convert: add filter.<driver>.process option" (edcc8581) on
Windows.
Consider the case of a file that requires filtering and is present in
branch A but not in branch B. If A is the current HEAD and we checkout B
then the following happens:
1. ce_compare_data() opens the file
2. index_fd() detects that the file requires to run a clean filter and
calls index_stream_convert_blob()
4. index_stream_convert_blob() calls convert_to_git_filter_fd()
5. convert_to_git_filter_fd() calls apply_filter() which creates a
new long running filter process (in case it is the first file
of this kind to be filtered)
6. The new filter process inherits all file handles. This is the
default on Linux/OSX and is explicitly defined in the
`CreateProcessW` call in `mingw.c` on Windows.
7. ce_compare_data() closes the file
8. Git unlinks the file as it is not present in B
The unlink operation does not work on Windows because the filter process
has still an open handle to the file. On Linux/OSX the unlink operation
succeeds but the file descriptors still leak into the child process.
Fix this problem by opening files in read-cache with the CLOEXEC flag to
ensure that the file descriptor does not remain open in a newly spawned
process similar to 05d1ed61.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
read-cache.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/read-cache.c b/read-cache.c
index 38d67fa..b594865 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
{
int match = -1;
- int fd = open(ce->name, O_RDONLY);
+ int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
+
+ if (O_CLOEXEC && fd < 0 && errno == EINVAL)
+ /* Try again w/o O_CLOEXEC: the kernel might not support it */
+ fd = open(ce->name, O_RDONLY);
if (fd >= 0) {
unsigned char sha1[20];
--
2.10.0
^ permalink raw reply related
* Re: [PATCH v1 00/19] Add configuration options for split-index
From: Junio C Hamano @ 2016-10-24 18:07 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161023092648.12086-1-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> The design is similar as the previous work that introduced
> "core.untrackedCache".
>
> The new "core.splitIndex" configuration option can be either true,
> false or undefined which is the default.
>
> When it is true, the split index is created, if it does not already
> exists, when the index is read. When it is false, the split index is
> removed if it exists, when the index is read. Otherwise it is left as
> is.
I admit I haven't thought it through, but this sounds OK.
> Along with this new configuration variable, the two following options
> are also introduced:
>
> - splitIndex.maxPercentChange
>
> This is to avoid having too many changes accumulating in the split
> index while in split index mode. The git-update-index
> documentation says:
>
> If split-index mode is already enabled and `--split-index` is
> given again, all changes in $GIT_DIR/index are pushed back to
> the shared index file.
>
> but it is probably better to not expect the user to think about it
> and to have a mechanism that pushes back all changes to the shared
> index file automatically when some threshold is reached.
>
> The default threshold is when the number of entries in the split
> index file reaches 20% (by default) of the number of entries in
> the shared index file. The new "splitIndex.maxPercentChange"
> config option lets people tweak this value.
OK.
> - splitIndex.sharedIndexExpire
>
> To make sure that old sharedindex files are eventually removed
> when a new one has been created, we "touch" the shared index file
> every time it is used by a new split index file. Then we can
> delete shared indexes with an mtime older than one week (by
> default), when we create a new shared index file. The new
> "splitIndex.sharedIndexExpire" config option lets people tweak
> this grace period.
I do not quite understand this justification. Doesn't each of the
"this hold only changes since the base index file" files have a
backpointer that names the base index file it is a delta against?
Is it safe to remove a base index file when there is still a split
index file that points at it?
IOW, I do not see why it can be safe for the expiration decision to
be based on timestamp (I would understand it if it were based on a
refcnt, though).
^ permalink raw reply
* Re: [PATCH 0/3] fix travis TAP/--verbose conflict
From: Lars Schneider @ 2016-10-24 18:06 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git
In-Reply-To: <20161021104107.vh3bjx6x6pd6izat@sigill.intra.peff.net>
> On 21 Oct 2016, at 12:41, Jeff King <peff@peff.net> wrote:
>
> On Fri, Oct 21, 2016 at 04:43:48AM -0400, Jeff King wrote:
>
>> The obvious fix would be to send "--verbose" output to stderr, but I
>> suspect that would end up annoying for people who do:
>>
>> ./t5547-push-quarantine.sh -v | less
>>
>> to read long output. Probably we need some option like "--log" which
>> logs in the same way that "--tee" does, but _without_ sending the data
>> to stdout. Naively, that just means replacing the "tee" invocation with
>> "cat", but I suspect it will be a lot more complicated than that,
>> because we still need to let the TAP output go to stdout.
>
> Yeah, it was definitely a lot more complicated. This patch series fixes
> it.
Thanks a lot for this detailed and quick fix :-)
Cheers,
Lars
^ permalink raw reply
* Re: [PATCH v2 0/2] Use CLOEXEC to avoid fd leaks
From: Junio C Hamano @ 2016-10-24 18:23 UTC (permalink / raw)
To: larsxschneider; +Cc: git, Johannes.Schindelin, e, jnareb
In-Reply-To: <20161024180300.52359-1-larsxschneider@gmail.com>
larsxschneider@gmail.com writes:
> ## Changes since v1
> * add fallbacks in case O_CLOEXEC is not available
That is a good idea.
> * rename 'git_open_noatime_cloexec' to 'git_open' (Eric, Dscho)
OK. This is the old git_open_noatime() that is meant to be used
ONLY for the files Git uses for its internal implementation, and
never for end-user files. I think it is a good idea to open them
with O_CLOEXEC.
And the separate patch to use O_CLOEXEC in ce_compare_data() that
opens a working tree file for reading does not use git_open(), which
is also correct. I like it.
> * rebased the topic on `next` to fix a merge conflict
I think this applies cleanly to 'master', so that is where I'd fork
my copy at.
Thanks.
^ permalink raw reply
* Re: Reporting Bug in Git Version Control System
From: Stefan Beller @ 2016-10-24 18:23 UTC (permalink / raw)
To: Yash Jain; +Cc: git@vger.kernel.org
In-Reply-To: <CAN8fUZe4iWJCZYqBBDbNyPq1Dz7f4xvTNRVEZgg5AYN2NrKCbg@mail.gmail.com>
This is a Github issue, so ask Github support.
Or read up on .mailmap files.
On Mon, Oct 24, 2016 at 7:28 AM, Yash Jain <yashjain.lnm@gmail.com> wrote:
> Hello,
> I have two accounts on github("yj291197" and "yaki29").
> Both the accounts have different gmail IDs("yj291197@gmail.com" and
> "yashjain.lnm@gmail.com" respectively) but same passwords.
> I used to use git for "yj291197" account and a few days earlier I made
> this new account and used git commit to commit on "yaki29" but it
> appeared as "yj291197" committed on "yaki29's" repo.
> Then I pulled a request of that commit then it appeared "yaki29"
> pulled a request with a commit of "yj291197".
>
>
>
> And during this whole session I was signed in as "yaki29" on github.com .
>
>
> Please reply ....
^ permalink raw reply
* Re: Reporting Bug in Git Version Control System
From: Junio C Hamano @ 2016-10-24 18:32 UTC (permalink / raw)
To: Stefan Beller; +Cc: Yash Jain, git@vger.kernel.org
In-Reply-To: <CAGZ79ka+_CUxQjB2R-EEk2nKmc_rbj_m7884fLvU9+NgJ5gUug@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Mon, Oct 24, 2016 at 7:28 AM, Yash Jain <yashjain.lnm@gmail.com> wrote:
>> Hello,
>> I have two accounts on github("yj291197" and "yaki29").
>> Both the accounts have different gmail IDs("yj291197@gmail.com" and
>> "yashjain.lnm@gmail.com" respectively) but same passwords.
>> I used to use git for "yj291197" account and a few days earlier I made
>> this new account and used git commit to commit on "yaki29" but it
>> appeared as "yj291197" committed on "yaki29's" repo.
>> Then I pulled a request of that commit then it appeared "yaki29"
>> pulled a request with a commit of "yj291197".
>>
>> And during this whole session I was signed in as "yaki29" on github.com .
>>
>
> This is a Github issue, so ask Github support.
>
> Or read up on .mailmap files.
I am (obviously) not a GitHub support, but I think the confusion is
coming from not understanding who the committer and the author of a
commit are and where they are coming from. They are both recorded
locally, taken from user.name and user.email configuration variables
when the commits are made. "git push" to propagate them to GitHub
will NOT change these values of a commit, once a commit is created.
The story described looks quite consistent if the user has
yj291197@gmail.com configured as user.email and kept making commits
in the local repository, and pushed them to either yj291197 or yaki29
accounts at GitHub, without ever changing the local configuration to
use the other e-mail address. All commits would record the user and
e-mail address yj291197, and the only one that may be attributed to
the new one yaki29 would be the automerge created at GitHub when a
pull request is responded on-site without first fetching and making
a merge locally.
^ permalink raw reply
* Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes
From: Eric Wong @ 2016-10-24 18:39 UTC (permalink / raw)
To: larsxschneider; +Cc: git, Johannes.Schindelin, jnareb, gitster
In-Reply-To: <20161024180300.52359-3-larsxschneider@gmail.com>
larsxschneider@gmail.com wrote:
> +++ b/read-cache.c
> @@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
> static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
> {
> int match = -1;
> - int fd = open(ce->name, O_RDONLY);
> + int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
> +
> + if (O_CLOEXEC && fd < 0 && errno == EINVAL)
> + /* Try again w/o O_CLOEXEC: the kernel might not support it */
> + fd = open(ce->name, O_RDONLY);
In the case of O_CLOEXEC != 0 and repeated EINVALs,
it'd be good to use something like sha1_file_open_flag as in 1/2
so we don't repeatedly hit EINVAL. Thanks.
^ permalink raw reply
* Re: [PATCH 27/36] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-24 18:55 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, pclouds
In-Reply-To: <20161022233225.8883-28-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> This revamps the API of the attr subsystem to be thread safe.
> Before we had the question and its results in one struct type.
> The typical usage of the API was
>
> static struct git_attr_check *check;
>
> if (!check)
> check = git_attr_check_initl("text", NULL);
>
> git_check_attr(path, check);
> act_on(check->value[0]);
>
> This has a couple of issues when it comes to thread safety:
>
> * the initialization is racy in this implementation. To make it
> thread safe, we need to acquire a mutex, such that only one
> thread is executing the code in git_attr_check_initl.
> As we do not want to introduce a mutex at each call site,
> this is best done in the attr code. However to do so, we need
> to have access to the `check` variable, i.e. the API has to
> look like
> git_attr_check_initl(struct git_attr_check*, ...);
Make that a double-asterisk. The same problem appears in an updated
example in technical/api-gitattributes.txt doc, but the example in
the commit log message (below) is correct.
> The usage of the new API will be:
>
> /*
> * The initl call will thread-safely check whether the
> * struct git_attr_check has been initialized. We only
> * want to do the initialization work once, hence we do
> * that work inside a thread safe environment.
> */
> static struct git_attr_check *check;
> git_attr_check_initl(&check, "text", NULL);
>
> /*
> * Obtain a pointer to a correctly sized result
> * statically allocated on the stack; this macro:
> */
> GIT_ATTR_RESULT_INIT_FOR(myresult, 1);
Are you sure about this? We've called attr_check_initl() already so
if this is declaring myresult, it would be decl-after-stmt.
> /* Perform the check and act on it: */
> git_check_attr(path, check, myresult);
> act_on(myresult->value[0]);
>
> /*
> * No need to free the check as it is static, hence doesn't leak
> * memory. The result is also static, so no need to free there either.
> */
The latter half is questionable. If it is "static" it wouldn't be
thread safe, no? I think the diff in this patch for archive.c shows
that we only expect
struct git_attr_result result[2];
upfront without RESULT_INIT_FOR(), and the reason why there is no
need to free the result[] is because it is on the stack. And each
element in result[] may point at a string, but the string belongs to
the attr subsystem and must not be freed.
^ permalink raw reply
* Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"
From: Stefan Beller @ 2016-10-24 19:10 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <61637cd9-8f83-c988-15c0-54f948153c07@kdbg.org>
On Sun, Oct 23, 2016 at 3:14 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 22.10.2016 um 22:46 schrieb Stefan Beller:
>>
>> I have looked into it again, and by now I think the bug is a feature,
>> actually.
>>
>> Consider this:
>>
>> git clone . super
>> git -C super submodule add ../submodule
>> # we thought the previous line is buggy
>> git clone super super-clone
>
>
> At this point, we *should* have this if there were no bugs (at least that is
> my assumption):
>
> /tmp
> !
> + submodule <- submodule's remote repo
> !
> + foo <- we are here (.), super's remote repo
> !
> + super <- remote.origin.url=/tmp/foo/.
> !
> + submodule <- remote.origin.url=/tmp/foo/./../submodule
> submodule.submodule.url=../submodule
>
> When I test this, 'git submodule add' fails:
Yes this setup currently fails because the /tmp/foo/./../submodule
is computed to be /tmp/foo/submodule.
In the test suite the directory "foo" is usually called
"trash\ directory.tXXXX-description" and the remote of
the submodule is created inside of it, such that:
/tmp
!
+ foo <- we are here (.), super's remote repo
!
+ submodule <- submodule's remote repo
!
+ super <- remote.origin.url=/tmp/foo/.
!
+ submodule <- remote.origin.url=/tmp/foo/./../submodule
submodule.submodule.url=../submodule
current result=/tmp/foo/submodule
works.
>
> foo@master> git -C super submodule add ../submodule
> fatal: repository '/tmp/foo/submodule' does not exist
> fatal: clone of '/tmp/foo/submodule' into submodule path
> '/tmp/foo/super/submodule' failed
>
>> Now in the super-clone the ../submodule is the correct
>> relative url, because the url where we cloned from doesn't
>> end in /.
>
>
> I do not understand why this would be relevant. The question is not how the
> submodule's remote URL ends, but how the submodule's remote URL is
> constructed from the super-project's URL and the relative path specified for
> 'git submodule add'.
I was not precise here. If you have the working setup as above, you can clone
super to super-clone and it keeps working, because of the current "bug".
The difference between "super" that is cloned from . and "super-clone" that
is cloned from "super" is only the remote url, and currently
/tmp/foo/super
/tmp/foo/.
+ relative path ../submodule resolve to the same submodule remote.
>
> Whether ../submodule or ./submodule is the correct relative URL depends on
> where the origin of the submodule is located relative to the origin of the
> super-project. In the above example, it is ../submodule. However, the error
> message tells us that git looked in /tmp/foo/submodule, which looks like the
> /. bug!
Correct.
At the time I was trying to fix the urls in the test suite with the
bug fix and I then
realized that this bugfix introduces a lot of pain to our test suite,
because we do
these secondary clones quite a few times. The pattern usually is:
* setup super (cloned from .)
* clone super to super-clone
* trash super-clone for testing purposes and delete it.
>
> I do not understand where you see a feature here. What am I missing?
The ease of use in our own testing suite is the feature I was talking about.
When we would fix the bug, we would not just need to fix
s|../submodule|./submodule| when setting up super, but we would need to
fix it every time again in reverse when creating these short lived
"super-clones"
that get tested on and deleted.
So maybe the correct fix for the testing suite is a double clone, i.e. instead
of
# prepare some stuff
git clone . super
we rather do:
# mkdir upstream && prepare stuff in upstream
git clone upstream super
However that way we would end up not exercising the
code path to be fixed with the actual bug fix i.e. we'd never clone from /.
because it is silly conceptually. We could add a new test for that though
that only tests that cloning from . constructs the correct URL.
However that is already tested in t0060 resolving the relative URLs
via the git submodule helper.
Thanks for bearing this discussion,
I feel like I am missing the obvious point here,
Stefan
^ permalink raw reply
* Re: [PATCH 28/36] attr: keep attr stack for each check
From: Junio C Hamano @ 2016-10-24 19:07 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, pclouds
In-Reply-To: <20161022233225.8883-29-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Instead of having a global attr stack, attach the stack to each check.
Two threads may be working on "git checkout", one "git_attr_check"
in convert.c may be used by them to learn the EOL conversion for
each path, and these threads are working in different parts of
worktree in parallel. The set of .gitattributes files each of these
threads wants to be cached at one time is tied to where in the
directory hierarchy the thread is working in.
The view by API users would not have to change from the point on
since 27/36 or so, but I think attr_stack needs to become per
<check, thread> tuple when we are fully thread-ready for the above
reason.
But we need to start somewhere to move away from the current "one
single attr stack" to "there are multiple attr stacks", and this
"two checks may and do use different attr stacks" is probably a
reasonable first step. It may give a single-threaded API users
immediate benefit if the "read and keep only the entries relevant
to the query" optimization is done with this step alone, without
making the cache per <check, thread> pair.
> This allows to use the attr in a multithreaded way.
With manipulation of attr stack protected with a single Big
Attributes Lock, this should be safe. It may not perform very well
when used by multiple threads, though ;-)
^ permalink raw reply
* Re: [PATCH 27/36] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-24 19:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams, Duy Nguyen
In-Reply-To: <xmqqr37560gv.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 24, 2016 at 11:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Make that a double-asterisk. The same problem appears in an updated
> example in technical/api-gitattributes.txt doc, but the example in
> the commit log message (below) is correct.
The implementation is actually using a double pointer, see below,
I forgot commit message and documentation
>> GIT_ATTR_RESULT_INIT_FOR(myresult, 1);
>
> Are you sure about this? We've called attr_check_initl() already so
> if this is declaring myresult, it would be decl-after-stmt.
I forgot to update the commit message and Documentation.
GIT_ATTR_RESULT_INIT_FOR is gone in the header
and in the implementation. I'll update that patch
to be consistent throughout all of {Documentation,
commit message, implementation}.
>
> The latter half is questionable. If it is "static" it wouldn't be
> thread safe, no? I think the diff in this patch for archive.c shows
> that we only expect
>
> struct git_attr_result result[2];
>
> upfront without RESULT_INIT_FOR(), and the reason why there is no
> need to free the result[] is because it is on the stack. And each
> element in result[] may point at a string, but the string belongs to
> the attr subsystem and must not be freed.
>
Same as above, it's bogus.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes
From: Johannes Sixt @ 2016-10-24 19:22 UTC (permalink / raw)
To: larsxschneider; +Cc: git, Johannes.Schindelin, e, jnareb, gitster
In-Reply-To: <20161024180300.52359-3-larsxschneider@gmail.com>
Am 24.10.2016 um 20:03 schrieb larsxschneider@gmail.com:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> This fixes "convert: add filter.<driver>.process option" (edcc8581) on
> Windows.
Today's next falls flat on its face on Windows in t0021.15 "required
process filter should filter data"; might it be the failure meant here?
(I haven't dug deeper, yet.)
++ test_config_global filter.protocol.process
'/d/Src/mingw-git/t/t0021/rot13-filter.pl clean smudge'
++ test_when_finished 'test_unconfig --global
'\''filter.protocol.process'\'''
++ test 0 = 0
++ test_cleanup='{ test_unconfig --global '\''filter.protocol.process'\''
} && (exit "$eval_ret"); eval_ret=$?; :'
++ git config --global filter.protocol.process
'/d/Src/mingw-git/t/t0021/rot13-filter.pl clean smudge'
++ test_config_global filter.protocol.required true
++ test_when_finished 'test_unconfig --global
'\''filter.protocol.required'\'''
++ test 0 = 0
++ test_cleanup='{ test_unconfig --global '\''filter.protocol.required'\''
} && (exit "$eval_ret"); eval_ret=$?; { test_unconfig
--global '\''filter.protocol.process'\''
} && (exit "$eval_ret"); eval_ret=$?; :'
++ git config --global filter.protocol.required true
++ rm -rf repo
++ mkdir repo
++ cd repo
++ git init
Initialized empty Git repository in d:/Src/mingw-git/t/trash
directory.t0021-conversion/repo/.git/
++ echo git-stderr.log
++ echo '*.r filter=protocol'
++ git add .
++ git commit . -m 'test commit 1'
[master (root-commit) aa5dd37] test commit 1
Author: A U Thor <author@example.com>
2 files changed, 2 insertions(+)
create mode 100644 .gitattributes
create mode 100644 .gitignore
++ git branch empty-branch
++ cp 'd:/Src/mingw-git/t/trash directory.t0021-conversion/test.o' test.r
++ cp 'd:/Src/mingw-git/t/trash directory.t0021-conversion/test2.o' test2.r
++ mkdir testsubdir
++ cp 'd:/Src/mingw-git/t/trash directory.t0021-conversion/test3
'\''sq'\'',$x.o' 'testsubdir/test3 '\''sq'\'',$x.r'
+++ file_size test.r
+++ cat test.r
+++ wc -c
+++ sed 's/^[ ]*//'
++ S=57
+++ file_size test2.r
+++ cat test2.r
+++ wc -c
+++ sed 's/^[ ]*//'
++ S2=14
+++ file_size 'testsubdir/test3 '\''sq'\'',$x.r'
+++ cat 'testsubdir/test3 '\''sq'\'',$x.r'
+++ wc -c
+++ sed 's/^[ ]*//'
++ S3=49
++ filter_git add .
++ rm -f rot13-filter.log
++ git add .
error: last command exited with $?=128
not ok 15 - required process filter should filter data
^ permalink raw reply
* Re: [PATCH 28/36] attr: keep attr stack for each check
From: Stefan Beller @ 2016-10-24 19:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams, Duy Nguyen
In-Reply-To: <xmqqmvht5zwx.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 24, 2016 at 12:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Instead of having a global attr stack, attach the stack to each check.
>
> Two threads may be working on "git checkout", one "git_attr_check"
> in convert.c may be used by them to learn the EOL conversion for
> each path, and these threads are working in different parts of
> worktree in parallel. The set of .gitattributes files each of these
> threads wants to be cached at one time is tied to where in the
> directory hierarchy the thread is working in.
>
> The view by API users would not have to change from the point on
> since 27/36 or so, but I think attr_stack needs to become per
> <check, thread> tuple when we are fully thread-ready for the above
> reason.
I looked for a platform independent way to get a thread id as a natural
number, i.e. I want to get 1,2,3,... such that I could have just added
list/array of attr stacks to each check, which would be the
<check, thread> tuple you envision.
However I think we do not really need it to be per check. If we had
an easy portable way of getting such a thread id, I would have implemented
a list of stacks per thread first. (Because each thread only looks at one
check at a time.)
So this is not a baby step because I did not want to do it all at once, but
because I did not find a suitable API to use.
>
> But we need to start somewhere to move away from the current "one
> single attr stack" to "there are multiple attr stacks", and this
> "two checks may and do use different attr stacks" is probably a
> reasonable first step. It may give a single-threaded API users
> immediate benefit if the "read and keep only the entries relevant
> to the query" optimization is done with this step alone, without
> making the cache per <check, thread> pair.
>
>> This allows to use the attr in a multithreaded way.
>
> With manipulation of attr stack protected with a single Big
> Attributes Lock, this should be safe. It may not perform very well
> when used by multiple threads, though ;-)
I agree. So maybe it is not really a good fit for general consumption yet.
^ permalink raw reply
* Re: [PATCH v5 00/27] Prepare the sequencer for the upcoming rebase -i patches
From: Stefan Beller @ 2016-10-24 19:36 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, git@vger.kernel.org, Jeff King,
Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <xmqqinsk8g1b.fsf@gitster.mtv.corp.google.com>
On Sat, Oct 22, 2016 at 10:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> There isn't enough time to include this topic in the upcoming
> release within the current https://tinyurl.com/gitCal calendar,
> however, which places the final on Nov 11th.
>
> I am wondering if it makes sense to delay 2.11 by moving the final
> by 4 weeks to Dec 9th.
>
> Thoughts?
>
> Speaking of what to and not to include in the upcoming release, we
> do want to include Stefan's off-by-one fix to the submodule-helper,
> but that is blocked on Windows end due to the test.
I'd be happy either way, i.e. we could revert that fix and make a release?
AFAICT, Windows only has broken tests, not broken functionality with that
submodule bug fix.
> I think
> everybody agreed that a longer time "right thing to do" fix is to
> address the "when base is /path/to/dir/., where is ../sub relative
> to it?" issue, but if we are to do so, it would need a longer
> gestation period once it hits 'next', as it can affect the current
> users and we may even need B/C notes in the release notes for the
> change. Giving ourselves a few more weeks of breathing room would
> help us to make sure the fix to relative URL issue is sound, too.
If we want a longer gestation period, we'd ideally merge it to master
just after a release, such that we "cook" it in master without having
it in any release (we had a similar discussion for the diff heuristics IIRC).
So please don't let the release schedule depend on my ability to deliver a
proper patch for the submodule path issue.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"
From: Johannes Sixt @ 2016-10-24 19:47 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <CAGZ79kZrsxjSfa=CSs5V56ePabnD3-W7FHC7KkRO6jytX+GrAw@mail.gmail.com>
Am 24.10.2016 um 21:10 schrieb Stefan Beller:
> The ease of use in our own testing suite is the feature I was talking about.
Ok, thanks for the clarification. Next steps would then be, IMO, to fix
the tests to match the future world order and mark tests that fail due
to the bug with test_expect_failure, and then switch them back to
test_expect_success in the patch with the bug fix.
-- Hannes
^ permalink raw reply
* Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes
From: Junio C Hamano @ 2016-10-24 19:53 UTC (permalink / raw)
To: Eric Wong; +Cc: larsxschneider, git, Johannes.Schindelin, jnareb
In-Reply-To: <20161024183900.GA12769@starla>
Eric Wong <e@80x24.org> writes:
> larsxschneider@gmail.com wrote:
>> +++ b/read-cache.c
>> @@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
>> static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
>> {
>> int match = -1;
>> - int fd = open(ce->name, O_RDONLY);
>> + int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
>> +
>> + if (O_CLOEXEC && fd < 0 && errno == EINVAL)
>> + /* Try again w/o O_CLOEXEC: the kernel might not support it */
>> + fd = open(ce->name, O_RDONLY);
>
> In the case of O_CLOEXEC != 0 and repeated EINVALs,
> it'd be good to use something like sha1_file_open_flag as in 1/2
> so we don't repeatedly hit EINVAL. Thanks.
Sounds sane.
It's just only once, so perhaps we do not mind a recursion like
this?
read-cache.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index b594865d89..a6978b9321 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -156,11 +156,14 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
{
int match = -1;
- int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
+ static int cloexec = O_CLOEXEC;
+ int fd = open(ce->name, O_RDONLY | cloexec);
- if (O_CLOEXEC && fd < 0 && errno == EINVAL)
+ if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
/* Try again w/o O_CLOEXEC: the kernel might not support it */
- fd = open(ce->name, O_RDONLY);
+ cloexec &= ~O_CLOEXEC;
+ return ce_compare_data(ce, st);
+ }
if (fd >= 0) {
unsigned char sha1[20];
^ 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