* Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes
From: Johannes Sixt @ 2016-10-25 21:39 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, Johannes.Schindelin, e, jnareb, gitster
In-Reply-To: <4C96209A-756F-45F1-B075-037FE32B3291@gmail.com>
Am 24.10.2016 um 21:53 schrieb Lars Schneider:
>
>> On 24 Oct 2016, at 21:22, Johannes Sixt <j6t@kdbg.org> wrote:
>>
>> 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.)
>
> Yes, this is the failure meant here :-)
I trust your word and Dscho's that it fixes a failure on Windows. In my
case, however, it was an outdated perl (5.8.8) which left a message
along the lines of "lookup of member flush in IO::Handle failed" in one
of the *.log files. I came up with the following workaround.
Fix-method: shot-in-the-dark
Perl-foo: not-present
Not-signed-off-by: Me
diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index ae4c50f..bb88c5f 100755
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -22,6 +22,7 @@
use strict;
use warnings;
+use FileHandle;
my $MAX_PACKET_CONTENT_SIZE = 65516;
my @capabilities = @ARGV;
--
2.10.0.343.g37bc62b
^ permalink raw reply related
* Re: [PATCH] Allow stashes to be referenced by index only
From: Junio C Hamano @ 2016-10-25 21:41 UTC (permalink / raw)
To: Aaron M Watson
Cc: git, Jon Seymour, David Caldwell, Øystein Walle, Jeff King,
Ævar Arnfjörð Bjarmason, David Aguilar,
Alex Henrie
In-Reply-To: <1477352413-4628-1-git-send-email-watsona4@gmail.com>
Aaron M Watson <watsona4@gmail.com> writes:
Aaron M Watson <watsona4@gmail.com> writes:
> Instead of referencing "stash@{n}" explicitly, it can simply be
> referenced as "n".
> Most users only reference stashes by their position
> in the stash stask (what I refer to as the "index").
It is unclear if the first sentence is a statement of the fact, an
expression of desire, or something else. With the current codebase,
it cannot simply be referenced as "n", and you either "wish it were
possible", or "make it possible to do so", or perhaps little bit of
both.
This is why we tend to use imperative mood to give an order to the
codebase to "be like so" to make it clear.
Perhaps
Instead of referencing "stash@{n}" explicitly, make it possible to
simply reference as "n". Most users only reference stashes by their
position in the stash stask (what I refer to as the "index" here).
or something like that (which is what I tenatively rewritten this to
while queuing).
> @@ -404,6 +403,9 @@ parse_flags_and_rev()
> die "$(eval_gettext "unknown option: \$opt")"
> FLAGS="${FLAGS}${FLAGS:+ }$opt"
> ;;
> + *)
> + REV="${REV}${REV:+ }'$opt'"
> + ;;
> esac
> done
>
> @@ -422,6 +424,15 @@ parse_flags_and_rev()
> ;;
> esac
>
> + case "$1" in
> + *[!0-9]*)
> + :
> + ;;
> + *)
> + set -- "${ref_stash}@{$1}"
> + ;;
> + esac
I can see that you inherited the brokenness from an existing one in
the earlier hunk, but case arms in these two case statements are
indented one level too deep. It would be good to fix it in a
follow-up patch (not a reroll of this patch).
Thanks. Will queue.
^ permalink raw reply
* Re: [PATCH v3 3/3] read-cache: make sure file handles are not inherited by child processes
From: Eric Wong @ 2016-10-25 21:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Lars Schneider, Johannes Schindelin
In-Reply-To: <20161025181621.4201-4-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> wrote:
> @@ -156,7 +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);
> + static int cloexec = O_CLOEXEC;
> + int fd = open(ce->name, O_RDONLY | cloexec);
> +
> + if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
> + /* Try again w/o O_CLOEXEC: the kernel might not support it */
> + cloexec &= ~O_CLOEXEC;
> + fd = open(ce->name, O_RDONLY | cloexec);
> + }
Seems fine, I prefer not using recursion so it's
easier-to-review.
But I have a _slight_ preference towards Dscho's version in
<alpine.DEB.2.20.1610251230150.3264@virtualbox> in case we
decide to start using another O_* flag in here.
(but I'm not usually a C programmer)
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #06; Mon, 24)
From: Johannes Sixt @ 2016-10-25 21:24 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, git@vger.kernel.org, Johannes Schindelin
In-Reply-To: <CAGZ79kb2XD8+y-Y_oiwSj3DsXjmTS=bd6fj5dn9NADmvDO5xtQ@mail.gmail.com>
Am 25.10.2016 um 20:13 schrieb Stefan Beller:
> On Tue, Oct 25, 2016 at 10:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> - the "off-by-one fix" part of sb/submodule-ignore-trailing-slash
>> needs to be in the upcoming release but the "trailing /. in base
>> should not affect the resolution of ../relative/path" part that
>> is still under discussion can wait. Which means we'd need a few
>> more !MINGW prerequisites in the tests by -rc0.
>>[...]
>
> So maybe instead of adding !MINGW we rather want to apply
> https://public-inbox.org/git/2908451e-4273-8826-8989-5572263cc283@kdbg.org/
> instead for now?
I was about to submit this very patch again, and only then saw your
message. So, yes, that's what I propose, too.
Dscho, does this patch fix the test failures that you observed, too?
Unfortunately, it goes against our endeavor to reduce subshells.
-- Hannes
^ permalink raw reply
* Re: [PATCH] git-svn: do not reuse caches memoized for a different architecture
From: Eric Wong @ 2016-10-25 21:23 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Gavin Lambert, Junio C Hamano
In-Reply-To: <653aa0cd566a2486bbc38cfd82ddfcfdfe48271c.1477398004.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> +++ b/perl/Git/SVN.pm
> @@ -1658,6 +1658,11 @@ sub tie_for_persistent_memoization {
> if ($memo_backend > 0) {
> tie %$hash => 'Git::SVN::Memoize::YAML', "$path.yaml";
> } else {
> + # first verify that any existing file can actually be loaded
> + # (it may have been saved by an incompatible version)
> + if (-e "$path.db") {
> + unlink "$path.db" unless eval { retrieve("$path.db"); 1 };
> + }
That retrieve() call is unlikely to work without "use Storable"
to import it into the current package.
I also favor setting "$path.db" once to detect typos and avoid
going over 80 columns. Additionally, having error-checking for
unlink might be useful.
So perhaps squashing this on top:
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 025c894..b3c1460 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -1660,10 +1660,15 @@ sub tie_for_persistent_memoization {
} else {
# first verify that any existing file can actually be loaded
# (it may have been saved by an incompatible version)
- if (-e "$path.db") {
- unlink "$path.db" unless eval { retrieve("$path.db"); 1 };
+ my $db = "$path.db";
+ if (-e $db) {
+ use Storable qw(retrieve);
+
+ if (!eval { retrieve($db); 1 }) {
+ unlink $db or die "unlink $db failed: $!";
+ }
}
- tie %$hash => 'Memoize::Storable', "$path.db", 'nstore';
+ tie %$hash => 'Memoize::Storable', $db, 'nstore';
}
}
Thoughts? Thanks.
^ permalink raw reply related
* Re: password forgot
From: Dennis Kaarsemaker @ 2016-10-25 19:10 UTC (permalink / raw)
To: Luciano Schillagi; +Cc: git
In-Reply-To: <CAK99BNBop7aCbwbYUAVoKc9JRrR2d+SAa1=WOFuEzmdmTtNdog@mail.gmail.com>
That is asking for your local computer password. This uninstaller also
has nothing to do with git itself, but with the third-party git package
you installed. The git project does not ship an 'uninstall.sh'
On Tue, 2016-10-25 at 13:23 -0300, Luciano Schillagi wrote:
> sorry, I'm a little confused
>
> and this?
>
>
>
>
> 2016-10-25 13:16 GMT-03:00 Dennis Kaarsemaker <dennis@kaarsemaker.net>:
> > On Tue, 2016-10-25 at 12:52 -0300, Luciano Schillagi wrote:
> > > Hi,
> > >
> > > I forgot my password in git, such as resetting?
> >
> > Hi Luciano,
> >
> > Git itself doesn't do any authentication, so I assume you lost the
> > password for an account on a hosted git solution such as gitlab or
> > github.
> >
> > You should contact the support team of whatever hoster you use, the git
> > developers cannot help you here.
> >
> > D.
> >
>
>
>
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #06; Mon, 24)
From: Junio C Hamano @ 2016-10-25 18:41 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161025183057.x24gqm56tgshyuvu@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Oct 24, 2016 at 06:09:00PM -0700, Junio C Hamano wrote:
>
>> - lt/abbrev-auto and its follow-up jk/abbrev-auto are about auto
>> scaling the default abbreviation length when Git produces a short
>> object name to adjust to the modern times. Peff noticed one
>> fallout from it recently and its fix jc/abbrev-auto is not yet in
>> 'next'. I would not be surprised if there are other uncovered
>> fallouts remaining in the code, but at the same time, I expect
>> they are all cosmetic kind that do not affect correctness, so I
>> am inclined to include all of them in the upcoming release.
>
> Yeah, I'd agree any fallouts are likely to be purely cosmetic (and if
> there _is_ some script broken by this, it was an accident waiting to
> happen as soon as it was used in a repo with a partial hash collision).
>
> I'm still not sure if people will balk just at the increased length in
> all of their output. I think I'm finally starting to get used to it. :)
I am finally getting used to it. At this point, I think the
transition plan would be to tell them to set core.abbrev to
whatever default they like.
>> * jc/abbrev-auto (2016-10-22) 4 commits
>> - transport: compute summary-width dynamically
>> - transport: allow summary-width to be computed dynamically
>> - fetch: pass summary_width down the callchain
>> - transport: pass summary_width down the callchain
>> (this branch uses jk/abbrev-auto and lt/abbrev-auto.)
>>
>> "git push" and "git fetch" reports from what old object to what new
>> object each ref was updated, using abbreviated refnames, and they
>> attempt to align the columns for this and other pieces of
>> information. The way these codepaths compute how many display
>> columns to allocate for the object names portion of this output has
>> been updated to match the recent "auto scale the default
>> abbreviation length" change.
>>
>> Will merge to 'next'.
>
> In case it was not obvious, I think this topic is good-to-go. And
> clearly any decision on lt/abbrev-auto should apply to this one, too. I
> notice you built it on jk/abbrev-auto, though, which is listed as
> "undecided". That's fine by me, but I think it would technically hold
> this topic hostage. You might want to adjust that before merging to
> next.
I am planning to merge both lt/* and jk/*; I should have said it
more clearly.
Thanks.
^ permalink raw reply
* Re: [PATCH] hex: use unsigned index for ring buffer
From: Junio C Hamano @ 2016-10-25 18:37 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Git List
In-Reply-To: <20161025183347.u3cvowf2h6tabtuw@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> diff --git a/path.c b/path.c
>> index fe3c4d96c6..9bfaeda207 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -24,7 +24,8 @@ static struct strbuf *get_pathname(void)
>> STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
>> };
>> static int index;
>> - struct strbuf *sb = &pathname_array[3 & ++index];
>> + struct strbuf *sb = &pathname_array[index];
>> + index = (index + 1) % ARRAY_SIZE(pathname_array);
>> strbuf_reset(sb);
>> return sb;
>
> This converts the pre-increment to a post-increment, but I don't think
> it matters.
Yes, I think that using the ring buffer from the beginning, not from
the second element from the beginning, is conceptually cleaner ;-).
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #06; Mon, 24)
From: Junio C Hamano @ 2016-10-25 18:34 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kb2XD8+y-Y_oiwSj3DsXjmTS=bd6fj5dn9NADmvDO5xtQ@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> Ok. The first 2 patches are in good shape for this cycle, though.
I thought both of you knew we are in agreement on that by now, but
yes, the off-by-one fix needs to be in the upcoming release.
> So maybe instead of adding !MINGW we rather want to apply
> https://public-inbox.org/git/2908451e-4273-8826-8989-5572263cc283@kdbg.org/
> instead for now?
That sounds good to me.
The "/." thing we would want to come to agreement during the
upcoming feature freeze and it would be very good if we can push the
result out early in the next cycle, but I feel that it is premature
for the upcoming release.
Thanks.
^ permalink raw reply
* Re: [PATCH] hex: use unsigned index for ring buffer
From: Jeff King @ 2016-10-25 18:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, Git List
In-Reply-To: <xmqqd1ios2p3.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 25, 2016 at 11:28:40AM -0700, Junio C Hamano wrote:
> OK, here is what I'll queue then.
> I assumed that René wants to sign it off ;-).
>
> -- >8 --
> From: René Scharfe <l.s.r@web.de>
> Date: Sun, 23 Oct 2016 19:57:30 +0200
> Subject: [PATCH] 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.
Looks good to me.
> diff --git a/path.c b/path.c
> index fe3c4d96c6..9bfaeda207 100644
> --- a/path.c
> +++ b/path.c
> @@ -24,7 +24,8 @@ static struct strbuf *get_pathname(void)
> STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
> };
> static int index;
> - struct strbuf *sb = &pathname_array[3 & ++index];
> + struct strbuf *sb = &pathname_array[index];
> + index = (index + 1) % ARRAY_SIZE(pathname_array);
> strbuf_reset(sb);
> return sb;
This converts the pre-increment to a post-increment, but I don't think
it matters.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #06; Mon, 24)
From: Jeff King @ 2016-10-25 18:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq1sz5tetv.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 24, 2016 at 06:09:00PM -0700, Junio C Hamano wrote:
> - lt/abbrev-auto and its follow-up jk/abbrev-auto are about auto
> scaling the default abbreviation length when Git produces a short
> object name to adjust to the modern times. Peff noticed one
> fallout from it recently and its fix jc/abbrev-auto is not yet in
> 'next'. I would not be surprised if there are other uncovered
> fallouts remaining in the code, but at the same time, I expect
> they are all cosmetic kind that do not affect correctness, so I
> am inclined to include all of them in the upcoming release.
Yeah, I'd agree any fallouts are likely to be purely cosmetic (and if
there _is_ some script broken by this, it was an accident waiting to
happen as soon as it was used in a repo with a partial hash collision).
I'm still not sure if people will balk just at the increased length in
all of their output. I think I'm finally starting to get used to it. :)
> * jc/abbrev-auto (2016-10-22) 4 commits
> - transport: compute summary-width dynamically
> - transport: allow summary-width to be computed dynamically
> - fetch: pass summary_width down the callchain
> - transport: pass summary_width down the callchain
> (this branch uses jk/abbrev-auto and lt/abbrev-auto.)
>
> "git push" and "git fetch" reports from what old object to what new
> object each ref was updated, using abbreviated refnames, and they
> attempt to align the columns for this and other pieces of
> information. The way these codepaths compute how many display
> columns to allocate for the object names portion of this output has
> been updated to match the recent "auto scale the default
> abbreviation length" change.
>
> Will merge to 'next'.
In case it was not obvious, I think this topic is good-to-go. And
clearly any decision on lt/abbrev-auto should apply to this one, too. I
notice you built it on jk/abbrev-auto, though, which is listed as
"undecided". That's fine by me, but I think it would technically hold
this topic hostage. You might want to adjust that before merging to
next.
-Peff
^ permalink raw reply
* Re: [PATCH] hex: use unsigned index for ring buffer
From: Junio C Hamano @ 2016-10-25 18:28 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Git List
In-Reply-To: <20161025003023.6vaqofsixana3zno@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Oct 24, 2016 at 04:53:50PM -0700, Junio C Hamano wrote:
>
>> > So how about this? It gets rid of magic number 3 and works for array
>> > size that's not a power of two. And as a nice side effect it can't
>> > trigger a signed overflow anymore.
>>
>> Looks good to me. Peff?
>
> Any of the variants discussed in this thread is fine by me.
OK, here is what I'll queue then.
I assumed that René wants to sign it off ;-).
-- >8 --
From: René Scharfe <l.s.r@web.de>
Date: Sun, 23 Oct 2016 19:57:30 +0200
Subject: [PATCH] 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.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
hex.c | 3 ++-
path.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/hex.c b/hex.c
index ab2610e498..845b01a874 100644
--- a/hex.c
+++ b/hex.c
@@ -78,7 +78,8 @@ char *sha1_to_hex(const unsigned char *sha1)
{
static int bufno;
static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
- return sha1_to_hex_r(hexbuffer[3 & ++bufno], sha1);
+ bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);
+ return sha1_to_hex_r(hexbuffer[bufno], sha1);
}
char *oid_to_hex(const struct object_id *oid)
diff --git a/path.c b/path.c
index fe3c4d96c6..9bfaeda207 100644
--- a/path.c
+++ b/path.c
@@ -24,7 +24,8 @@ static struct strbuf *get_pathname(void)
STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
};
static int index;
- struct strbuf *sb = &pathname_array[3 & ++index];
+ struct strbuf *sb = &pathname_array[index];
+ index = (index + 1) % ARRAY_SIZE(pathname_array);
strbuf_reset(sb);
return sb;
}
--
2.10.1-777-gd068e6bde7
^ permalink raw reply related
* [PATCH v3 3/3] read-cache: make sure file handles are not inherited by child processes
From: Junio C Hamano @ 2016-10-25 18:16 UTC (permalink / raw)
To: git; +Cc: Lars Schneider, Eric Wong, Johannes Schindelin
In-Reply-To: <20161025181621.4201-1-gitster@pobox.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 O_CLOEXEC flag
to ensure that the file descriptor does not remain open in a newly
spawned process similar to 05d1ed6148 ("mingw: ensure temporary file
handles are not inherited by child processes", 2016-08-22).
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* With Eric's suggestion to avoid repeated EINVAL and fallback,
implemented with Dscho's "write open twice explicitly, we are
retrying after all" coding style.
read-cache.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/read-cache.c b/read-cache.c
index 38d67faf70..db5d910642 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -156,7 +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);
+ static int cloexec = O_CLOEXEC;
+ int fd = open(ce->name, O_RDONLY | cloexec);
+
+ if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
+ /* Try again w/o O_CLOEXEC: the kernel might not support it */
+ cloexec &= ~O_CLOEXEC;
+ fd = open(ce->name, O_RDONLY | cloexec);
+ }
if (fd >= 0) {
unsigned char sha1[20];
--
2.10.1-777-gd068e6bde7
^ permalink raw reply related
* [PATCH v3 0/3] quick reroll of Lars's git_open() w/ O_CLOEXEC
From: Junio C Hamano @ 2016-10-25 18:16 UTC (permalink / raw)
To: git; +Cc: Eric Wong, Johannes Schindelin, Lars Schneider
In-Reply-To: <alpine.DEB.2.20.1610251327050.3264@virtualbox>
Here is to make sure everybody is on the same page regarding the
series. The patches are adjusted for comments from Eric and Dscho.
Lars Schneider (3):
sha1_file: rename git_open_noatime() to git_open()
sha1_file: open window into packfiles with O_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 | 9 ++++++++-
sha1_file.c | 25 +++++++++++++++----------
5 files changed, 26 insertions(+), 14 deletions(-)
--
2.10.1-777-gd068e6bde7
^ permalink raw reply
* [PATCH v3 1/3] sha1_file: rename git_open_noatime() to git_open()
From: Junio C Hamano @ 2016-10-25 18:16 UTC (permalink / raw)
To: git; +Cc: Lars Schneider, Eric Wong, Johannes Schindelin
In-Reply-To: <20161025181621.4201-1-gitster@pobox.com>
From: Lars Schneider <larsxschneider@gmail.com>
This function is meant to be used when reading from files in the
object store, and the original objective was to avoid smudging atime
of loose object files too often, hence its name. Because we'll be
extending its role in the next commit to also arrange the file
descriptors they return auto-closed in the child processes, rename
it to lose "noatime" part that is too specific.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This is a pure-rename step suggested by Dscho.
builtin/pack-objects.c | 2 +-
cache.h | 2 +-
pack-bitmap.c | 2 +-
sha1_file.c | 12 ++++++------
4 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1e7c2a98a5..0fd52bd6b4 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 0dc39a998c..a902ca1f8e 100644
--- a/cache.h
+++ b/cache.h
@@ -1122,7 +1122,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 b949e51716..39bcc16846 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 266152de36..5d2bcd3ed1 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++;
@@ -1559,7 +1559,7 @@ 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;
@@ -1605,7 +1605,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;
@@ -1613,7 +1613,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.1-777-gd068e6bde7
^ permalink raw reply related
* [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Junio C Hamano @ 2016-10-25 18:16 UTC (permalink / raw)
To: git; +Cc: Lars Schneider, Eric Wong, Johannes Schindelin
In-Reply-To: <20161025181621.4201-1-gitster@pobox.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 O_CLOEXEC flag similar to 05d1ed61 to fix the leaked file
descriptors.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* And the remainder of original 1/2, again taking suggestion by DScho.
sha1_file.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 5d2bcd3ed1..09045df1dc 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1561,7 +1561,7 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
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;
@@ -1571,12 +1571,17 @@ int git_open(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 ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {
+ 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;
}
}
--
2.10.1-777-gd068e6bde7
^ permalink raw reply related
* Re: What's cooking in git.git (Oct 2016, #06; Mon, 24)
From: Stefan Beller @ 2016-10-25 18:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <xmqqoa28s63l.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 25, 2016 at 10:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> One of the initial ways to work around the bugfix was to
>>
>> git clone . root # <- add in this step and it works again.
>> git clone root super
>>
>> but instead I will do the preparation for the 'super' project not
>> in '.' but in 'root', just called differently ("super_remote" ?)
>>
>> An additional new test for cloning from '.' will be introduced, too.
>>
>> I plan on working on that with highest priority for git after finishing
>> some attr stuff that I currently have open. So expect a patch (or two)
>> this week.
>
> Hmph, I personally would prefer to defer the "correct behaviour for
> /." part for the next cycle, which is why I wrote:
Ok. The first 2 patches are in good shape for this cycle, though.
And the /. thing will wait until next cycle then, i.e. I can drop priority
as I wish
>
> - the "off-by-one fix" part of sb/submodule-ignore-trailing-slash
> needs to be in the upcoming release but the "trailing /. in base
> should not affect the resolution of ../relative/path" part that
> is still under discussion can wait. Which means we'd need a few
> more !MINGW prerequisites in the tests by -rc0.
>
> at the beginning of the message you are responding to, and I also
> thought that was consistent and in agreement with what you said
> earlier in <CAGZ79kaq85c1Gk1aRSrdQGp1Nm9p6tN0jXbFvTN0v+9ehooxYg@mail.gmail.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.
>
> to which I responded in <xmqqpomp33km.fsf@gitster.mtv.corp.google.com>
and you said:
> It of course needs help from
> Windows folks to validate the results.
So maybe instead of adding !MINGW we rather want to apply
https://public-inbox.org/git/2908451e-4273-8826-8989-5572263cc283@kdbg.org/
instead for now?
^ permalink raw reply
* Re: [PATCH] reset: --unmerge
From: Sergey Organov @ 2016-10-25 17:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqa8dttkbw.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> The procedure to resolve a merge conflict typically goes like this:
>
> - first open the file in the editor, and with the help of conflict
> markers come up with a resolution.
>
> - save the file.
>
> - look at the output from "git diff" to see the combined diff to
> double check if the resolution makes sense.
>
> - perform other tests, like trying to build the result with "make".
>
> - finally "git add file" to mark that you are done.
>
> and repeating the above until you are done with all the conflicted
> paths. If you, for whatever reason, accidentally "git add file" by
> mistake until you are convinced that you resolved it correctly (e.g.
> doing "git add file" immediately after saving, without a chance to
> peruse the output from "git diff"), there is no good way to recover.
"git reset --unmerge file"
to undo accidental
"git add file"
during conflict resolution?
I'm afraid "unmerge" sounds like revert of "merge", rather than revert
of "resolve". I'd rather prefer to see something like:
git add --undo file
git merge --unresolve file
git reset --unresolve file
in that order, to deal with the issue.
-- Sergey
^ permalink raw reply
* Re: [PATCH v1 00/19] Add configuration options for split-index
From: Junio C Hamano @ 2016-10-25 17:21 UTC (permalink / raw)
To: Duy Nguyen
Cc: Christian Couder, Git Mailing List,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8BBwWRGSyJDYQ7THj7quu4=T1xT_-KojQd45Vye4Kgcng@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> ... But those files
> people generate manually and refer to them with $GIT_INDEX_FILE, we
> can't know where they are.
Then we probably should stop doing that, i.e. disable split-index
automatically for these temporary ones, perhaps, and even with
Christian's series which allows use of split index on the primary
one easier (which is a good idea), make sure we don't auto split the
temporary ones the users create?
> Timestamps allow us to say, ok this base index file has not been read
> by anybody for N+ hours (or better, days), it's most likely not
> referenced by any temporary index files (including
> $GIT_DIR/index.lock) anymore because those files, by the definition of
> "temporary", must be gone by now....
and if we guessed wrong, users will have a "temporary index" that
they meant to keep for longer term that is now broken here. I am
not sure if that risk is worth taking.
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #06; Mon, 24)
From: Junio C Hamano @ 2016-10-25 17:15 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kbtyuSXDiyi2eY6HKCAwEiKgk2KmNakSn=f+RiXCAC1bA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> One of the initial ways to work around the bugfix was to
>
> git clone . root # <- add in this step and it works again.
> git clone root super
>
> but instead I will do the preparation for the 'super' project not
> in '.' but in 'root', just called differently ("super_remote" ?)
>
> An additional new test for cloning from '.' will be introduced, too.
>
> I plan on working on that with highest priority for git after finishing
> some attr stuff that I currently have open. So expect a patch (or two)
> this week.
Hmph, I personally would prefer to defer the "correct behaviour for
/." part for the next cycle, which is why I wrote:
- the "off-by-one fix" part of sb/submodule-ignore-trailing-slash
needs to be in the upcoming release but the "trailing /. in base
should not affect the resolution of ../relative/path" part that
is still under discussion can wait. Which means we'd need a few
more !MINGW prerequisites in the tests by -rc0.
at the beginning of the message you are responding to, and I also
thought that was consistent and in agreement with what you said
earlier in <CAGZ79kaq85c1Gk1aRSrdQGp1Nm9p6tN0jXbFvTN0v+9ehooxYg@mail.gmail.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.
to which I responded in <xmqqpomp33km.fsf@gitster.mtv.corp.google.com>
> If you are referring the "trailing /. should not make difference
> when resolving ../relative/path" change with "rever that fix", I
> think that may be a reasonable way to proceed. Even though that
> change is a bugfix (at least from the point of view by me and j6t in
> the recent discussion), it is a behaviour change that we would want
> to see feedback from existing submodule users and deserves a longer
> gestation period. And that part is not yet in 'next' yet ;-)
>
> > 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).
>
> Yes.
>
> It would mean that we would need a separate patch that adds the
> !MINGW prerequisite to some tests to what is on 'next', as the early
> patches on sb/submodule-ignore-trailing-slash~ that fixes off-by-one
> is the right thing to do either way. It of course needs help from
> Windows folks to validate the results.
So...
^ 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-25 17:02 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Eric Wong, larsxschneider, git, jnareb
In-Reply-To: <alpine.DEB.2.20.1610251230150.3264@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> That still looks overly complicated, repeatedly ORing cloexec and
> recursing without need. How about this instead?
>
> static int oflags = O_RDONLY | O_CLOEXEC;
> int fd = open(ce->name, oflags);
>
> if ((O_CLOEXEC & oflags) && fd < 0 && errno == EINVAL) {
> /* Try again w/o O_CLOEXEC: the kernel might not support it */
> oflags &= ~O_CLOEXEC;
> fd = open(ce->name, oflags);
> }
I deliberately separated the part that can and designed to be
toggled (O_CLOEXEC) and the part that is meant to be constant
(O_RDONLY), and I do not think the first part of suggestion is
particularly a good idea.
I didn't write the same open twice, but I agree that an extra "we
fallback to opening with a different flags" inside the if () { }
block that is there exactly for implementing that fallback is an
excellent idea. I like that part of the suggestion.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/2] sha1_file: open window into packfiles with CLOEXEC
From: Junio C Hamano @ 2016-10-25 16:58 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Lars Schneider, git, e, jnareb
In-Reply-To: <alpine.DEB.2.20.1610251220280.3264@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> The patch series may be a little bit more pleasant to read if you renamed
> git_open_noatime() to git_open() first, in a separate commit.
Possibly.
If this were "we added a new parameter at the same time and each
calling site of the renamed function with an extra parameter chooses
to pass different value to it", then keeping the rename and the
interface enhancement as two separate steps would help a lot.
But this one only renames and updates the internal implementation,
without changing what the calling sites need to do. I am OK with
having them together in a single patch like the one posted.
>> @@ -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;
>
> How about
>
> if ((O_CLOEXEC & sha1_file_open_flag) && errno == EINVAL) {
> sha1_file_open_flag &= ~O_CLOEXEC;
>
> instead? It is shorter and should be just as easily optimized out by a
> C compiler if O_CLOEXEC was defined as 0.
Yup, I think that makes things easier to read for humans, too.
^ permalink raw reply
* pre-planning for git contributor's summit: Feb 2, 2017, Brussels
From: Jeff King @ 2016-10-25 16:28 UTC (permalink / raw)
To: git
GitHub is organizing Git Merge 2017, and is offering space to do another
contributor's summit. I want to share what details I have so far, get
any input on what we want the event to be like, and see who's interested
in coming (for venue planning purposes).
The details below are still tentative, but I imagine the date and rough
location will not change at this point, as it's scheduled to be adjacent
to FOSDEM.
The plan is to do the contributor summit on Thursday, Feb 2,
2017. The main conference track for Git Merge is the day after, then
FOSDEM on the weekend. The location will be a conference space in
Brussels, Belgium (probably the one in [1]).
Right now we have a room with a projector and whiteboard that fits about
20-25 people. It's pretty conference-roomish. If we're bigger or smaller
than that, there are some alternative rooms. I think we'd ideally know
our approximate size by Dec 1st. We'll probably have the room 10am-3pm,
but that's in flux.
The format, attendee list, etc, are up to us in the Git community. In
past year's it's been pretty loosely organized. One room, about 20 or so
people, and people bringing up topics for discussion, presenting short
talks, etc. I think I'd like to do a _little_ more planning this year,
and have people propose topics a few days ahead of time, rather than on
a whiteboard the morning of the conference.
As far as attendees, in the past it's basically been open to anybody who
participates in development of Git or the surrounding ecosystem. That's
seemed fine to me, but I'm open to input. I'd also say that people who
are _interested_ in joining Git development would be welcome, even if
they haven't contributed a patch yet. Maybe we can convince them. :)
I think in the past it has technically been "invite only", and I've just
had interested people email me and cross-checked them in the list
archive and git-log, or sent a polite "who are you" email to anybody I
didn't recognize. I'm open to changing that to something more
democratic, but AFAIK I didn't exclude anybody in past years.
The registration fee for Git Merge (the whole thing) is $100, and I
think contributor summit registration will be tied to that. I should be
getting some registration codes to hand out that will waive the fee
completely. All of the proceeds are going to Software Freedom
Conservancy, so I'd encourage people who can get their company to send
them to a conference to consider having them pay. But it shouldn't be a
big deal for me to waive fees for contributors who want to do so.
Speaking of money, we've used some Git project money to subsidize travel
for contributors in past years. That's always been done in a very ad hoc
way, with people emailing the Git program committee (me, Junio, Shawn)
and working it out privately. I'm open to alternate proposals there.
I think that's about it. What I'd like to hear from you:
1. A rough count of interested folks. Even if you're not sure you can
make it, let me know (either on the list or via private email) so I
can get an idea and communicate it to the folks booking the room.
2. Any ideas or discussion on the event, format, etc. If there's
something we need from the venue or event organizers, I can try to
arrange it. Sooner is better for such things.
-Peff
PS There isn't an official RFP for talks for the main track yet, nor for
the workshops which will happen the same day as the contributor
summit. But if you have an idea for a talk or a hands-on workshop,
let me know and I can put you in touch with the event organizers.
[1] https://www.google.com/maps/place/The+Egg/@50.8357424,4.321425,15z/data=!4m13!1m7!3m6!1s0x0:0xe028c6680611d1da!2sThe+Egg!3b1!8m2!3d50.8331946!4d4.3270469!3m4!1s0x0:0xe028c6680611d1da!8m2!3d50.8331946!4d4.3270469
^ permalink raw reply
* Re: password forgot
From: Dennis Kaarsemaker @ 2016-10-25 16:16 UTC (permalink / raw)
To: Luciano Schillagi, git
In-Reply-To: <14FB7E4A-7134-4343-AFF7-9E17AC94ACE3@gmail.com>
On Tue, 2016-10-25 at 12:52 -0300, Luciano Schillagi wrote:
> Hi,
>
> I forgot my password in git, such as resetting?
Hi Luciano,
Git itself doesn't do any authentication, so I assume you lost the
password for an account on a hosted git solution such as gitlab or
github.
You should contact the support team of whatever hoster you use, the git
developers cannot help you here.
D.
^ permalink raw reply
* password forgot
From: Luciano Schillagi @ 2016-10-25 15:52 UTC (permalink / raw)
To: git
Hi,
I forgot my password in git, such as resetting?
Thank You
Luciano
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox