* Re: [RFH] limiting ref advertisements
From: Duy Nguyen @ 2016-10-25 11:46 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20161024132932.i42rqn2vlpocqmkq@sigill.intra.peff.net>
On Mon, Oct 24, 2016 at 8:29 PM, Jeff King <peff@peff.net> wrote:
> I'm looking into the oft-discussed idea of reducing the size of ref
> advertisements by having the client say "these are the refs I'm
> interested in". Let's set aside the protocol complexities for a
> moment and imagine we magically have some way to communicate a set of
> patterns to the server.
>
> What should those patterns look like?
>
> I had hoped that we could keep most of the pattern logic on the
> client-side. Otherwise we risk incompatibilities between how the client
> and server interpret a pattern. I had also hoped we could do some kind
> of prefix-matching, which would let the server look only at the
> interesting bits of the ref tree (so if you don't care about
> refs/changes, and the server has some ref storage that is hierarchical,
> they can literally get away without opening that sub-tree).
>
> The patch at the end of this email is what I came up with in that
> direction. It obviously won't compile without the twenty other patches
> implementing transport->advertise_prefixes
Yes! git-upload-pack-2 is making a come back, one form or another.
> but it gives you a sense of what I'm talking about.
>
> Unfortunately it doesn't work in all cases, because refspec sources may
> be unqualified. If I ask for:
>
> git fetch $remote master:foo
>
> then we have to actually dwim-resolve "master" from the complete list of
> refs we get from the remote. It could be "refs/heads/master",
> "refs/tags/master", etc. Worse, it could be "refs/master". In that case,
> at least, I think we are OK because we avoid advertising refs directly
> below "refs/" in the first place. But if you have a slash, like:
>
> git fetch $remote jk/foo
>
> then that _could_ be "refs/jk/foo". Likewise, we cannot even optimize
> the common case of a fully-qualified ref, like "refs/heads/foo". If it
> exists, we obviously want to use that. But if it doesn't, then it
> could be refs/something-else/refs/heads/foo. That's unlikely, but it
> _does_ work now, and optimizing the advertisement would break it.
>
> So it seems like left-anchoring the refspecs can never be fully correct.
> We can communicate "master" to the server, who can then look at every
> ref it would advertise and ask "could this be called master"? But it
> will be setting in stone the set of "could this be" patterns. Granted,
> those haven't changed much over the history of git, but it seems awfully
> fragile.
The first thought that comes to mind is, if left anchoring does not
work, let's support both left and right anchoring. I guess you
considered and discarded this.
If prefix matching does not work, and assuming "some-prefix" sent by
client to be in fact "**/some-prefix" pattern at server side will set
the "could this be" in stone, how about use wildmatch? It's flexible
enough and we have full control over the pattern matching engine so C
Git <-> C Git should be good regardless of platforms. I understand
that wildmatch is still complicated enough that a re-implementation
can easily divert in behavior. But a pattern with only '*', '/**',
'/**/' and '**/' wildcards (in other words, no [] or ?) could make the
engine a lot simpler and still fit our needs (and give some room for
client-optimization).
> In an ideal world the client and server would negotiate to come to some
> agreement on the patterns being used. But as we are bolting this onto
> the existing protocol, I was really trying to do it without introducing
> an extra capabilities phase or extra round-trips. I.e., something like
> David Turner's "stick the refspec in the HTTP query parameters" trick,
> but working everywhere[1].
--
Duy
^ permalink raw reply
* Re: [PATCH 1/7] read info/{attributes,exclude} only when in repository
From: Duy Nguyen @ 2016-10-25 12:24 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20161020061641.3enppkoxfzr76has@sigill.intra.peff.net>
On Thu, Oct 20, 2016 at 1:16 PM, Jeff King <peff@peff.net> wrote:
> The low-level attribute and gitignore code will try to look
> in $GIT_DIR/info for any repo-level configuration files,
> even if we have not actually determined that we are in a
> repository (e.g., running "git grep --no-index"). In such a
> case they end up looking for ".git/info/attributes", etc.
>
> This is generally harmless, as such a file is unlikely to
> exist outside of a repository, but it's still conceptually
> the wrong thing to do.
>
> Let's detect this situation explicitly and skip reading the
> file (i.e., the same behavior we'd get if we were in a
> repository and the file did not exist).
On the other hand, if we invoke attr machinery too early by mistake,
before setup_git_directory* is called, then we skip
.git/info/attributes file as well even though I think we should shout
"call setup_git_directory first!" so the developer can fix it.
I wonder if we should have two flags in startup_info to say "yes
setup_git_dir... has been called, you can trust
startup_info->have_repository" and "yes, i do not call setup_git_dir
on purpose, quit complaining" then we could still catch unintended
.git/info/attributes ignore while letting git grep --no-index work
correctly.
--
Duy
^ permalink raw reply
* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Duy Nguyen @ 2016-10-25 12:38 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20161020062430.rxupwheaeydtcvf3@sigill.intra.peff.net>
On Thu, Oct 20, 2016 at 1:24 PM, Jeff King <peff@peff.net> wrote:
> This passes the test suite (after the adjustments in the
> previous patches), but there's a risk of regression for any
> cases where the fallback usually works fine but the code
> isn't exercised by the test suite. So by itself, this
> commit is a potential step backward, but lets us take two
> steps forward once we've identified and fixed any such
> instances.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> environment.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/environment.c b/environment.c
> index cd5aa57..b1743e6 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -164,8 +164,11 @@ static void setup_git_env(void)
> const char *replace_ref_base;
>
> git_dir = getenv(GIT_DIR_ENVIRONMENT);
> - if (!git_dir)
> + if (!git_dir) {
> + if (!startup_info->have_repository)
> + die("BUG: setup_git_env called without repository");
YES!!! Thank you for finally fixing this.
The "once we've identified" part could be tricky though. This message
alone will not give us any clue where it's called since it's buried
deep in git_path() usually, which is buried deep elsewhere. Without
falling back to core dumps (with debug info), glibc's backtrace
(platform specifc), the best we could do is turn git_path() into a
macro that takes __FILE__ and __LINE__ and somehow pass the info down
here, but "..." in macros is C99 specific, sigh..
Is it too bad to turn git_path() into a macro when we know the
compiler is C99 ? Older compilers will have no source location info in
git_path(), Hopefully they are rare, which means chances of this fault
popping up are also reduced.
--
Duy
^ permalink raw reply
* Re: %C(auto) not working as expected
From: Duy Nguyen @ 2016-10-25 12:52 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Tom Hale, git, Junio C Hamano
In-Reply-To: <20161010142818.lglwrxpks6l6aqrm@sigill.intra.peff.net>
On Mon, Oct 10, 2016 at 9:28 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Oct 10, 2016 at 04:26:18PM +0700, Duy Nguyen wrote:
>
>> > If we do a revamp of the pretty-formats to bring them more in line with
>> > ref-filter (e.g., something like "%(color:red)") maybe that would be an
>> > opportunity to make minor adjustments. Though, hmm, it looks like
>> > for-each-ref already knows "%(color:red)", and it's unconditional.
>> > <sigh> So perhaps we would need to go through some deprecation period or
>> > other transition.
>>
>> We could add some new tag to change the behavior of all following %C
>> tags. Something like %C(tty) maybe (probably a bad name), then
>> discourage the use if "%C(auto" for terminal detection?
>
> Yeah, adding a "%C(enable-auto-color)" or something would be backwards
> compatible and less painful than using "%C(auto)" everywhere. I do
> wonder if anybody actually _wants_ the "always show color, even if
> --no-color" behavior. I'm having trouble thinking of a good use for it.
>
> IOW, I'm wondering if anyone would disagree that the current behavior is
> simply buggy.
Silence in two weeks. I vote (*) making %(<color-name>) honor --color
and turning the %(auto, no-op, for both log family and for-each-ref.
We could keep old behavior behind some environment variable if it's
not much work so it keeps working while people come here and tell us
about their use cases.
(*) I know.. voting is not how things work around here, unless you
vote with patches, but I can't take on another topic.
> Reading the thread at:
>
> http://public-inbox.org/git/7v4njkmq07.fsf@alter.siamese.dyndns.org/
>
> I don't really see any compelling reason against it (there was some
> question of which config to use, but we already answered that with
> "%C(auto)", and use the value from the pretty_ctx).
>
> -Peff
--
Duy
^ permalink raw reply
* Re: %C(auto) not working as expected
From: Jeff King @ 2016-10-25 12:58 UTC (permalink / raw)
To: Duy Nguyen; +Cc: René Scharfe, Tom Hale, git, Junio C Hamano
In-Reply-To: <CACsJy8B_AQxm1=vF8i4FPtinq0id1QZPrqp9vvAmAgUns_kgGg@mail.gmail.com>
On Tue, Oct 25, 2016 at 07:52:21PM +0700, Duy Nguyen wrote:
> > Yeah, adding a "%C(enable-auto-color)" or something would be backwards
> > compatible and less painful than using "%C(auto)" everywhere. I do
> > wonder if anybody actually _wants_ the "always show color, even if
> > --no-color" behavior. I'm having trouble thinking of a good use for it.
> >
> > IOW, I'm wondering if anyone would disagree that the current behavior is
> > simply buggy.
>
> Silence in two weeks. I vote (*) making %(<color-name>) honor --color
> and turning the %(auto, no-op, for both log family and for-each-ref.
> We could keep old behavior behind some environment variable if it's
> not much work so it keeps working while people come here and tell us
> about their use cases.
Yeah, sorry. I was blocked on making %(color:) in ref-filter work, which
required a bunch of refactoring in ref-filter, which conflicted heavily
with the kn/ref-filter-branch-list (which wants to do a lot of the same
things), and then I got blocked on reviewing that series (which overall
looks pretty sane, but I wanted to really dig in because I think it
hasn't gotten very careful review, or at least not recently).
So I'm still hoping to shave that yak at some point. Maybe this week.
-Peff
^ permalink raw reply
* Re: %C(auto) not working as expected
From: Duy Nguyen @ 2016-10-25 13:02 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Tom Hale, git, Junio C Hamano
In-Reply-To: <20161025125856.p2paxt7erl2szptv@sigill.intra.peff.net>
On Tue, Oct 25, 2016 at 7:58 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 25, 2016 at 07:52:21PM +0700, Duy Nguyen wrote:
>
>> > Yeah, adding a "%C(enable-auto-color)" or something would be backwards
>> > compatible and less painful than using "%C(auto)" everywhere. I do
>> > wonder if anybody actually _wants_ the "always show color, even if
>> > --no-color" behavior. I'm having trouble thinking of a good use for it.
>> >
>> > IOW, I'm wondering if anyone would disagree that the current behavior is
>> > simply buggy.
>>
>> Silence in two weeks. I vote (*) making %(<color-name>) honor --color
>> and turning the %(auto, no-op, for both log family and for-each-ref.
>> We could keep old behavior behind some environment variable if it's
>> not much work so it keeps working while people come here and tell us
>> about their use cases.
>
> Yeah, sorry.
Just to be clear (after re-reading my mail), the unwritten part after
"Silence in two weeks" was "so nobody disagreed with you", not "how is
your progress?". It's great that you keep working on this regardless
:D
--
Duy
^ permalink raw reply
* Re: [PATCH 1/7] read info/{attributes,exclude} only when in repository
From: Jeff King @ 2016-10-25 14:56 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8Bk32TcivD-5UO28UhdbpvCcxTE71cxFO2p_A4TZ1+GVw@mail.gmail.com>
On Tue, Oct 25, 2016 at 07:24:50PM +0700, Duy Nguyen wrote:
> > Let's detect this situation explicitly and skip reading the
> > file (i.e., the same behavior we'd get if we were in a
> > repository and the file did not exist).
>
> On the other hand, if we invoke attr machinery too early by mistake,
> before setup_git_directory* is called, then we skip
> .git/info/attributes file as well even though I think we should shout
> "call setup_git_directory first!" so the developer can fix it.
> I wonder if we should have two flags in startup_info to say "yes
> setup_git_dir... has been called, you can trust
> startup_info->have_repository" and "yes, i do not call setup_git_dir
> on purpose, quit complaining" then we could still catch unintended
> .git/info/attributes ignore while letting git grep --no-index work
> correctly.
Yeah, it would be nice for the low-level code to be able to detect such
errors. I don't mind if you want to extend startup_info in that way, but
it will probably introduce a period of instability and regressions
(sites that are perfectly fine, but forgot to set the "I know what I'm
doing" flag).
-Peff
^ permalink raw reply
* Re: Integrating submodules with no side effects
From: Robert Dailey @ 2016-10-25 15:05 UTC (permalink / raw)
To: Stefan Beller; +Cc: Git
In-Reply-To: <CAHd499DKz+tpP3zRrXX3_serhoC_GCZst2y75JtC0Eiy1zfEew@mail.gmail.com>
On Wed, Oct 19, 2016 at 2:51 PM, Robert Dailey <rcdailey.lists@gmail.com> wrote:
> On Wed, Oct 19, 2016 at 2:45 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Wed, Oct 19, 2016 at 12:19 PM, Robert Dailey
>> <rcdailey.lists@gmail.com> wrote:
>>> On Wed, Oct 19, 2016 at 11:23 AM, Stefan Beller <sbeller@google.com> wrote:
>>>> You could try this patch series:
>>>> https://github.com/jlehmann/git-submod-enhancements/tree/git-checkout-recurse-submodules
>>>> (rebased to a newer version; no functional changes:)
>>>> https://github.com/stefanbeller/git/tree/submodule-co
>>>> (I'll rebase that later to origin/master)
>>>>
>>>>>
>>>>> Do you have any info on how I can prevent that error? Ideally I want
>>>>> the integration to go smoothly and transparently, not just for the
>>>>> person doing the actual transition (me) but for everyone else that
>>>>> gets those changes from upstream. They should not even notice that it
>>>>> happened (i.e. no failed commands, awkward behavior, or manual steps).
>>>>
>>>> It depends on how long you want to postpone the transition, but I plan to
>>>> upstream the series referenced above in the near future,
>>>> which would enable your situation to Just Work (tm). ;)
>>>
>>> At first glance, what you've linked to essentially looks like
>>> automated `git submodule update` for every `git checkout`. Am I
>>> misunderstanding?
>>
>> Essentially yes, except with stricter rules than the actual submodule update
>> IIRC.
>>
>>>
>>> If I'm correct, this is not the same as what I'm talking about. The
>>> problem appears to be more internal: When a submodule is removed, the
>>> physical files that were there are not removed by Git.
>>
>> That is also done by that series: submodules ought to be treated as files:
>> If you checkout a new version where a file is deleted, the checkout command
>> will actually remove the file for you (and e.g. solve any
>> directory/file conflicts
>> that may happen in the transition.)
>>
>>> It leaves them
>>> there in the working copy as untracked files.
>>
>> That is the current behavior as checkout tries hard to ignore submodules.
>>
>>> The next step Git takes
>>> (again, just from outside observation) is to add those very same files
>>> to the working copy, since they were added to a commit. However, at
>>> this point Git fails because it's trying to create (write) files to
>>> the working copy when an exact file of that name already exists there.
>>> Git will not overwrite untracked files, so at this point it fails.
>>>
>>> What needs to happen, somehow, is Git sees that the files were
>>> actually part of a submodule (which was removed) and remove the
>>> physical files as well, assuming that they were not modified in the
>>> submodule itself. This will ensure that the next step (creating the
>>> files) will succeed since the files no longer block it.
>>
>> Yep.
>
> It's great we're finally on the same page ;-)
>
> However, I don't see how this problem can be solved with your script,
> or solved in general outside of that. Does this mean that Git needs to
> change to treat submodules as it does normal files, per your previous
> assertion, which means submodules should *not* be left behind in the
> working copy as untracked files?
I'll assume (due to the lack of responses) that the only viable
solution here is to integrate the submodule using a different
directory name than the one used by the submodule itself. It's
unfortunate but I'll do it if I have no other option.
^ permalink raw reply
* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Jeff King @ 2016-10-25 15:15 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8DZWN0RRaDy9w2BVG5pn4FWCY=1YQDsP0V5obrr1wSzZQ@mail.gmail.com>
On Tue, Oct 25, 2016 at 07:38:30PM +0700, Duy Nguyen wrote:
> > diff --git a/environment.c b/environment.c
> > index cd5aa57..b1743e6 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -164,8 +164,11 @@ static void setup_git_env(void)
> > const char *replace_ref_base;
> >
> > git_dir = getenv(GIT_DIR_ENVIRONMENT);
> > - if (!git_dir)
> > + if (!git_dir) {
> > + if (!startup_info->have_repository)
> > + die("BUG: setup_git_env called without repository");
>
> YES!!! Thank you for finally fixing this.
Good, I'm glad somebody besides me is excited about this. I've been
wanting to write this patch for a long time, but it took years of
chipping away at all the edge cases.
> The "once we've identified" part could be tricky though. This message
> alone will not give us any clue where it's called since it's buried
> deep in git_path() usually, which is buried deep elsewhere. Without
> falling back to core dumps (with debug info), glibc's backtrace
> (platform specifc), the best we could do is turn git_path() into a
> macro that takes __FILE__ and __LINE__ and somehow pass the info down
> here, but "..." in macros is C99 specific, sigh..
>
> Is it too bad to turn git_path() into a macro when we know the
> compiler is C99 ? Older compilers will have no source location info in
> git_path(), Hopefully they are rare, which means chances of this fault
> popping up are also reduced.
I think you could conditionally make git_path() and all of its
counterparts macros, similar to the way the trace code works. It seems
like a pretty maintenance-heavy solution, though. I'd prefer
conditionally compiling backtrace(); that also doesn't hit 100% of
cases, but at least it isn't too invasive.
But I think I still prefer just letting the corefile and the debugger do
their job. This error shouldn't happen much, and when it does, it should
be easily reproducible. Getting the bug reporter to give either a
reproduction recipe, or to run "gdb git" doesn't seem like that big a
hurdle.
For fun, here's a patch that uses backtrace(), but it does not actually
print the function names unless you compile with "-rdynamic" (and even
then it misses static functions). There are better libraries, but of
course that's one more thing for the user to deal with when building.
-Peff
---
diff --git a/usage.c b/usage.c
index 17f52c1b5c..4917c6bdfd 100644
--- a/usage.c
+++ b/usage.c
@@ -5,6 +5,9 @@
*/
#include "git-compat-util.h"
#include "cache.h"
+#ifdef HAVE_BACKTRACE
+#include <execinfo.h>
+#endif
static FILE *error_handle;
static int tweaked_error_buffering;
@@ -24,6 +27,32 @@ void vreportf(const char *prefix, const char *err, va_list params)
fputc('\n', fh);
}
+#ifdef HAVE_BACKTRACE
+static void maybe_backtrace(void)
+{
+ void *bt[100];
+ char **symbols;
+ int nr;
+
+ if (!git_env_bool("GIT_BACKTRACE_ON_DIE", 0))
+ return;
+
+ nr = backtrace(bt, ARRAY_SIZE(bt));
+ symbols = backtrace_symbols(bt, nr);
+ if (symbols) {
+ FILE *fh = error_handle ? error_handle : stderr;
+ int i;
+
+ fprintf(fh, "die() called from:\n");
+ for (i = 0; i < nr; i++)
+ fprintf(fh, " %s\n", symbols[i]);
+ free(symbols);
+ }
+}
+#else
+#define maybe_backtrace()
+#endif
+
static NORETURN void usage_builtin(const char *err, va_list params)
{
vreportf("usage: ", err, params);
@@ -33,6 +62,7 @@ static NORETURN void usage_builtin(const char *err, va_list params)
static NORETURN void die_builtin(const char *err, va_list params)
{
vreportf("fatal: ", err, params);
+ maybe_backtrace();
exit(128);
}
^ permalink raw reply related
* [PATCH] git-svn: do not reuse caches memoized for a different architecture
From: Johannes Schindelin @ 2016-10-25 15:30 UTC (permalink / raw)
To: Eric Wong, git; +Cc: Gavin Lambert, Junio C Hamano
From: Gavin Lambert <github@mirality.co.nz>
Reusing cached data speeds up git-svn by quite a fair bit. However, if
the YAML module is unavailable, the caches are written to disk in an
architecture-dependent manner. That leads to problems when upgrading,
say, from 32-bit to 64-bit Git for Windows.
Let's just try to read those caches back if we detect the absence of the
YAML module and the presence of the file, and delete the file if it
could not be read back correctly.
Note that the only way to catch the error when the memoized cache could
not be read back is to put the call inside an `eval { ... }` block
because it would die otherwise; the `eval` block should also return `1`
in case of success explicitly since the function reading back the cached
data does not return an appropriate value to test for success.
This fixes https://github.com/git-for-windows/git/issues/233.
Signed-off-by: Gavin Lambert <github@mirality.co.nz>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/svn-multi-arch-v1
Fetch-It-Via: git fetch https://github.com/dscho/git svn-multi-arch-v1
We carried this for some time in Git for Windows.
perl/Git/SVN.pm | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 018beb8..025c894 100644
--- a/perl/Git/SVN.pm
+++ 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 };
+ }
tie %$hash => 'Memoize::Storable', "$path.db", 'nstore';
}
}
base-commit: 659889482ac63411daea38b2c3d127842ea04e4d
--
2.10.1.583.g721a9e0
^ permalink raw reply related
* 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
* 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
* 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: [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
* 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: 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 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: [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: 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
* [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
* [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 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
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