* [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 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
* 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
* 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: 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: 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: 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: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: 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: [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: 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 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: [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 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 v3 0/3] quick reroll of Lars's git_open() w/ O_CLOEXEC
From: Lars Schneider @ 2016-10-25 21:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Wong, Johannes Schindelin, Johannes Sixt
In-Reply-To: <20161025181621.4201-1-gitster@pobox.com>
> On 25 Oct 2016, at 20:16, Junio C Hamano <gitster@pobox.com> wrote:
>
> Here is to make sure everybody is on the same page regarding the
> series. The patches are adjusted for comments from Eric and Dscho.
Thank you, Junio! Your v3 looks good to me and I compiled and tested
it on Windows.
There is one catch though:
I ran the tests multiple times on Windows and I noticed that the
following test seems flaky:
t0021 `required process filter should be used only for "clean" operation only'
This flaky-ness was not introduced by your v3. I was able to reproduce
the same behavior for v2. I wonder why I did not noticed that before.
The only difference to before is that my Windows machines does some
heavy CPU/network task in the background now (I can't/don't want to
stop that ATM).
Here is the relevant part of the log:
++ rm -f rot13-filter.log
++ git checkout --quiet --no-progress .
+ test_eval_ret_=255
+ want_trace
+ test t = t
+ test t = t
+ set +x
error: last command exited with $?=255
Looks like Git exists with 255. Any idea why that might happen?
@Dscho/Johannes: Can you try this on your Windows machines?
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH v3 3/3] read-cache: make sure file handles are not inherited by child processes
From: Junio C Hamano @ 2016-10-25 22:54 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Lars Schneider, Johannes Schindelin
In-Reply-To: <20161025213318.GB8683@starla>
Eric Wong <e@80x24.org> writes:
> 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.
Interesting. The reason why I have a slight preferene to separate
the fixed part and the toggle-able part is exactly because I want
the code to be prepared in case we decide to start using other O_*
flags in the future.
I guess different people have different tastes, as usual ;-)
^ permalink raw reply
* Re: [PATCH v3 0/3] quick reroll of Lars's git_open() w/ O_CLOEXEC
From: Junio C Hamano @ 2016-10-25 22:56 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, Eric Wong, Johannes Schindelin, Johannes Sixt
In-Reply-To: <A6C42931-835D-42FD-B4D9-7F84921909E2@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
> heavy CPU/network task in the background now (I can't/don't want to
> stop that ATM).
>
> Here is the relevant part of the log:
>
> ++ rm -f rot13-filter.log
> ++ git checkout --quiet --no-progress .
> + test_eval_ret_=255
> + want_trace
> + test t = t
> + test t = t
> + set +x
> error: last command exited with $?=255
>
> Looks like Git exists with 255. Any idea why that might happen?
Other than "the --quiet above may be hiding it?", I do not
immediately have a useful guess, sorry.
> @Dscho/Johannes: Can you try this on your Windows machines?
>
> Thanks,
> Lars
^ permalink raw reply
* Re: [PATCH] reset: --unmerge
From: Junio C Hamano @ 2016-10-25 23:28 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8D4cT2EV_t6=+XTHxLmjV+NtAE+KgiN3ZiP7JQzvu29oQ@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> BTW making git-add (and "git commit -a") refuse files with conflict
> markers present could prevent this mistake earlier and is probably a
> better option because the user won't have to discover 'reset
> --unmerge'.
That may help some users, but you are solving a different problem.
I do not say "save" unless I know the editor buffer contents is not
ready. "This is ready to be saved" however is different from "This
resolution is correct", and I need the unmerged states in the index
to verify, namely by looking at "git diff" (no other parameters)
output that shows only the paths with unmerged stages and in the
compact combined diff format.
Somebody with a bright idea decided that vc-git-resolve-conflicts
variable should be on by default in Emacs 25.1 X-<, which causes
"save" after resolving conflicts to automatically run "git add", to
destroy this valuable tool. My knee-jerk reaction, of course, to
such a default is "that's brain dead", but perhaps they did so for
some good reason that I fail to fathom.
^ permalink raw reply
* Re: [PATCH] Allow stashes to be referenced by index only
From: Ramsay Jones @ 2016-10-26 0:12 UTC (permalink / raw)
To: Junio C Hamano, Aaron M Watson
Cc: git, Jon Seymour, David Caldwell, Øystein Walle, Jeff King,
Ævar Arnfjörð Bjarmason, David Aguilar,
Alex Henrie
In-Reply-To: <xmqqshrkqf79.fsf@gitster.mtv.corp.google.com>
On 25/10/16 22:41, Junio C Hamano wrote:
> 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).
s/stask/stack/
ATB,
Ramsay Jones
^ permalink raw reply
* A bug with "git svn show-externals"
From: Tao Peng @ 2016-10-25 23:53 UTC (permalink / raw)
To: git
Hi there,
I met a bug of the "git svn show-externals” command. If a subdirectory item has a svn:externals property, and the format of the property is “URL first, then the local path”, running "git svn show-externals” command at the root level will result in an unusable output.
Example:
$ svn pg svn:externals svn+ssh://src.foo.com/svn/ref/English.lproj/
svn+ssh://src.foo.com/svn/orig/trunk/Resources/English.lproj/Localizable.strings Localizable.strings
$ git svn show-externals
# /English.lproj/
/English.lproj/svn+ssh://src.foo.com/svn/orig/trunk/Resources/English.lproj/Localizable.strings Localizable.strings
This bug is preventing my script from correctly finishing the svn-to-git repo migration work. Does anyone know a workaround to this bug?
Thanks,
Peng
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Jeff King @ 2016-10-26 4:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Lars Schneider, Eric Wong, Johannes Schindelin
In-Reply-To: <20161025181621.4201-3-gitster@pobox.com>
On Tue, Oct 25, 2016 at 11:16:20AM -0700, Junio C Hamano wrote:
> diff --git a/sha1_file.c b/sha1_file.c
> index 5d2bcd3ed1..09045df1dc 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -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;
> }
So if we start with O_CLOEXEC|O_NOATIME, we drop CLOEXEC here and try
again with just O_NOATIME. And then if _that_ fails...
> + /* Might the failure be due to O_NOATIME? */
> + if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
> + sha1_file_open_flag &= ~O_NOATIME;
> + continue;
> + }
We drop O_NOATIME, and end up with an empty flag field.
But we will never have tried just O_CLOEXEC, which might have worked.
Because of the order here, this would not be a regression (i.e., any
system that used to work will still eventually find a working comb), but
it does mean that systems without O_NOATIME do not get the benefit of
the new O_CLOEXEC protection.
Unfortunately, I think covering all of the cases would be 2^nr_flags.
That's only 4 right now, but it does not bode well as a pattern.
I'm not sure it's worth worrying about or not; I don't know which
systems are actually lacking either of the flags, or if they tend to
have both.
-Peff
^ permalink raw reply
* Re: [PATCH 27/36] attr: convert to new threadsafe API
From: Johannes Schindelin @ 2016-10-26 8:52 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, bmwill, pclouds
In-Reply-To: <20161022233225.8883-28-sbeller@google.com>
Hi Stefan,
On Sat, 22 Oct 2016, Stefan Beller wrote:
> @@ -46,6 +47,19 @@ struct git_attr {
> static int attr_nr;
> static struct git_attr *(git_attr_hash[HASHSIZE]);
>
> +#ifndef NO_PTHREADS
> +
> +static pthread_mutex_t attr_mutex;
> +#define attr_lock() pthread_mutex_lock(&attr_mutex)
> +#define attr_unlock() pthread_mutex_unlock(&attr_mutex)
This mutex is never initialized. That may work on the system you tested,
but it is incorrect, and it does segfault on Windows. A lot.
I need *at least* something like this to make it stop crashing all over
the test suite:
-- snipsnap --
diff --git a/attr.c b/attr.c
index d5a6aa9..6933504 100644
--- a/attr.c
+++ b/attr.c
@@ -50,7 +50,16 @@ static struct git_attr *(git_attr_hash[HASHSIZE]);
#ifndef NO_PTHREADS
static pthread_mutex_t attr_mutex;
-#define attr_lock()pthread_mutex_lock(&attr_mutex)
+static inline void attr_lock(void)
+{
+ static int initialized;
+
+ if (!initialized) {
+ pthread_mutex_init(&attr_mutex, NULL);
+ initialized = 1;
+ }
+ pthread_mutex_lock(&attr_mutex);
+}
#define attr_unlock()pthread_mutex_unlock(&attr_mutex)
#else
^ permalink raw reply related
* [PATCH] Documentation/git-diff: document git diff with 3+ commits
From: Michael J Gruber @ 2016-10-26 9:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller
In-Reply-To: <CA+P7+xq1i8AtQ7i=1m_n9HTSL10kFUFBn8jvNcB_t_6Rh29u4w@mail.gmail.com>
That one is difficult to discover but super useful, so document it:
Specifying 3 or more commits makes git diff switch to combined diff.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Notes:
Note that we have the following now:
'git diff A B' displays 'B minus A'
'git diff A B C' displays 'A minus B' and 'A minus C'
While I know why, that (the implicit '-R' seems unfortunate).
(NB: One has to use '-c' if A is an actual merge commiti, it seems.)
If M is a merge base for A and B, we have:
'git diff A..B' equivalent to 'git diff A B'
in contrast to 'git log A..B' listing commits between M and B only
(without the commits between M and A unless they are "in" B).
I would expect 'git diff M B' here.
'git diff A...B' is equivalent to 'git diff M B'
in contrast to 'git log A...B' listing commits between M and A (marked left)
as well as commits between M and B (marked right).
I would expect 'git diff -c -R M A B' here.
Somehow the positive and negative ends of these ranges don't correspond well
with thinking about diffs as differences between these ends.
[I'm not exact with my use of "between" regarding boundary commits.]
Documentation/git-diff.txt | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index bbab35fcaf..2047318a27 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -12,6 +12,7 @@ SYNOPSIS
'git diff' [options] [<commit>] [--] [<path>...]
'git diff' [options] --cached [<commit>] [--] [<path>...]
'git diff' [options] <commit> <commit> [--] [<path>...]
+'git diff' [options] <commit> <commit> <commit> [<commit>...]
'git diff' [options] <blob> <blob>
'git diff' [options] [--no-index] [--] <path> <path>
@@ -75,9 +76,16 @@ two blob objects, or changes between two files on disk.
"git diff $(git-merge-base A B) B". You can omit any one
of <commit>, which has the same effect as using HEAD instead.
+'git diff' [options] <commit> <commit> <commit> [<commit>...]::
+
+ This is to view a combined diff between the first <commit>
+ and the remaining ones, just like viewing a combined diff
+ for a merge commit (see below) where the first <commit>
+ is the merge commit and the remaining ones are the parents.
+
Just in case if you are doing something exotic, it should be
noted that all of the <commit> in the above description, except
-in the last two forms that use ".." notations, can be any
+in the two forms that use ".." notations, can be any
<tree>.
For a more complete list of ways to spell <commit>, see
--
2.10.1.723.g0f00470
^ permalink raw reply related
* Re: [PATCH v1 00/19] Add configuration options for split-index
From: Duy Nguyen @ 2016-10-26 9:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: Christian Couder, Git Mailing List,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <xmqqk2cws5t6.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 26, 2016 at 12:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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.
Even if we ignore user index files (by forcing them all to be stored
in one piece), there is a problem with the special temporary file
index.lock, which must use split-index because it will become the new
index. Handling race conditions could be tricky with ref counting.
Timestamps help in this regard.
--
Duy
^ permalink raw reply
* Re: [PATCH] reset: --unmerge
From: Duy Nguyen @ 2016-10-26 9:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqqmvhsc8kn.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 26, 2016 at 6:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Somebody with a bright idea decided that vc-git-resolve-conflicts
> variable should be on by default in Emacs 25.1 X-<,
Oh good, I have an excuse to stick to 24.5.1 for a while longer then.
> which causes
> "save" after resolving conflicts to automatically run "git add", to
> destroy this valuable tool. My knee-jerk reaction, of course, to
> such a default is "that's brain dead", but perhaps they did so for
> some good reason that I fail to fathom.
I was curious. The default is t since the variable's introduction [1].
Interestingly the thread/bug that resulted in that commit started with
"report this bug to git" [2]. Something about git-stash. I quote the
original mail here in case anyone wants to look into it (not sure if
it's actually reported here before, I don't pay much attention to
git-stash mails)
-- 8< --
Bad news, everyone!
When a stash contains changes for several files, and "stash pop"
encounters conflicts only in some of them, the rest of the files are
stages automatically.
At least, that happens with Git 2.1.0 on my machine, and some
commenters here: http://stackoverflow.com/a/1237337/615245
So then when we unstage the files which had conflicts after resolving
those, the result is mixed. Which doesn't look right.
What shall we do? Unstage the automatically-staged files? Revert the
changes from this bug? It seems Git really wants the changes staged
after the conflict resolution.
-- 8< --
[1] http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=45651154473c7d2f16230da765d034ecfde7968a
[2] https://lists.gnu.org/archive/html/bug-gnu-emacs/2015-05/msg00433.html
--
Duy
^ 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