* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Johannes Schindelin @ 2016-10-28 11:11 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Torvalds, Jeff King, Git Mailing List, Lars Schneider,
Eric Wong
In-Reply-To: <xmqq60od42s0.fsf@gitster.mtv.corp.google.com>
Hi,
On Thu, 27 Oct 2016, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Linus Torvalds <torvalds@linux-foundation.org> writes:
> >
> >> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >>>
> >>> Would the best endgame shape for this function be to open with
> >>> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)
> >>> but ignoring an error from it, I guess? That would be the closest
> >>> to what we historically had, I would think.
> >>
> >> I think that's the best model.
> >
> > OK, so perhaps like this.
>
> Hmph. This may not fly well in practice, though.
>
> To Unix folks, CLOEXEC is not a huge correctness issue. A child
> process may hold onto an open file descriptor a bit longer than the
> lifetime of the parent but as long as the child eventually exits,
> nothing is affected. Over there, things are different. The parent
> cannot even rename(2) or unlink(2) a file it created and closed
> while the child is still holding the file descriptor open and the
> lack of CLOEXEC will make the parent fail. I do not know how well
> fcntl(2) emulation works on Windows, but I would not be surprised
> if J6t or Dscho comes back and says that FD_CLOEXEC given to F_SETFD
> would not work while O_CLOEXEC given to open(2) does.
You guys. I mean: You guys! You sure make my life hard. A brief look at
mingw.h could have answered your implicit question:
static inline int fcntl(int fd, int cmd, ...)
{
if (cmd == F_GETFD || cmd == F_SETFD)
return 0;
errno = EINVAL;
return -1;
}
So while you discuss in your Linux Ivory Tower how to optimize Git for
Linux, and Linux only, I'll have to drop everything else and spend the
rest of my Friday trying to find a way to adjust a file handle
*immediately after opening it with undesired flags* (when it could have
been opened with the desired flags, as suggested, to begin with).
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] Documenation: fmt-merge-msg: fix markup in example
From: Jeff King @ 2016-10-28 11:08 UTC (permalink / raw)
To: Stefan Christ; +Cc: git
In-Reply-To: <1477648886-12096-1-git-send-email-contact@stefanchrist.eu>
On Fri, Oct 28, 2016 at 12:01:26PM +0200, Stefan Christ wrote:
> diff --git a/Documentation/git-fmt-merge-msg.txt b/Documentation/git-fmt-merge-msg.txt
> index 6526b17..44892c4 100644
> --- a/Documentation/git-fmt-merge-msg.txt
> +++ b/Documentation/git-fmt-merge-msg.txt
> @@ -60,10 +60,10 @@ merge.summary::
> EXAMPLE
> -------
>
> ---
> +---------
> $ git fetch origin master
> $ git fmt-merge-msg --log <$GIT_DIR/FETCH_HEAD
> ---
> +---------
Thanks. Asciidoc generally requires at least 4 delimiter characters to
open a delimited block (including a ListingBlock, which is what we want
here). There is one exception, "--", which is a generic OpenBlock, which
is just used for grouping, and not any special syntactic meaning (so
that's why this _didn't_ render the "--", but did render the contents
without line breaks).
So looks good, modulo the typo in the subject that somebody else pointed
out.
-Peff
^ permalink raw reply
* Re: [PATCH] Documenation: fmt-merge-msg: fix markup in example
From: Robert P. J. Day @ 2016-10-28 10:24 UTC (permalink / raw)
To: Stefan Christ; +Cc: git
In-Reply-To: <1477648886-12096-1-git-send-email-contact@stefanchrist.eu>
"Documenation"?
rday
--
========================================================================
Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca
Twitter: http://twitter.com/rpjday
LinkedIn: http://ca.linkedin.com/in/rpjday
========================================================================
^ permalink raw reply
* [PATCH] Documenation: fmt-merge-msg: fix markup in example
From: Stefan Christ @ 2016-10-28 10:01 UTC (permalink / raw)
To: git
The example was not rendered as verbatim text. Fix it.
Signed-off-by: Stefan Christ <contact@stefanchrist.eu>
---
Documentation/git-fmt-merge-msg.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-fmt-merge-msg.txt b/Documentation/git-fmt-merge-msg.txt
index 6526b17..44892c4 100644
--- a/Documentation/git-fmt-merge-msg.txt
+++ b/Documentation/git-fmt-merge-msg.txt
@@ -60,10 +60,10 @@ merge.summary::
EXAMPLE
-------
---
+---------
$ git fetch origin master
$ git fmt-merge-msg --log <$GIT_DIR/FETCH_HEAD
---
+---------
Print a log message describing a merge of the "master" branch from
the "origin" remote.
--
2.7.3
^ permalink raw reply related
* Re: Expanding Includes in .gitignore
From: Aaron Pelly @ 2016-10-28 9:32 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: git
In-Reply-To: <xmqqwpgt2ng2.fsf@gitster.mtv.corp.google.com>
On 28/10/16 15:54, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> However, as I said elsewhere, I'm not convinced this feature is all that
>> helpful for in-repository .gitignore files, and I think it does
>> introduce compatibility complications. People with older git will not
>> respect your .gitignore.d files. Whereas $GIT_DIR/info is purely a local
>> matter.
>
> As I do not see the point of making in-tree .gitignore to a forest
> of .gitignore.d/ at all, compatibility complications is not worth
> even thinking about, I would have to say.
Well; that saves some work. :)
I do not suggesting making this mandatory. I think it adds value and it
is a common and understood mechanism. But, if it is abhorrent, consider:
There is precedent for including files in git-config. This could be
extended to ignore files. The code is not similar, but the concept is. I
could live with it.
Or how about a new githook that can intelligently create or return the
details? This would be my least favourite option unless it was
configured in an obvious place.
Finally, if this is a bad-idea, as I asked in the beginning, I will
consider the equine expired, cease flagellation and apologise for the noise.
^ permalink raw reply
* Re: [PATCH] attr: convert to new threadsafe API
From: Johannes Schindelin @ 2016-10-28 8:55 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, bmwill, pclouds, j6t, peff, simon
In-Reply-To: <20161027221550.14930-1-sbeller@google.com>
Hi Stefan,
On Thu, 27 Oct 2016, Stefan Beller wrote:
> * use attr_start on Windows to dynamically initialize the Single Big Attr Mutex
I would have preferred that call in common-main.c, but whatevs...
Thanks you for fixing the bug,
Dscho
^ permalink raw reply
* Re: [PATCH] valgrind: support test helpers
From: Johannes Schindelin @ 2016-10-28 8:51 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Duy Nguyen, Junio C Hamano
In-Reply-To: <71c3d13a-fa29-75d3-50ac-81978c08f552@web.de>
[-- Attachment #1: Type: text/plain, Size: 764 bytes --]
Hi,
On Fri, 28 Oct 2016, René Scharfe wrote:
> Tests run with --valgrind call git commands through a wrapper script
> that invokes valgrind on them. This script (valgrind.sh) is in turn
> invoked through symlinks created for each command in t/valgrind/bin/.
>
> Since e6e7530d (test helpers: move test-* to t/helper/ subdirectory)
> these symlinks have been broken for test helpers -- they point to the
> old locations in the root of the build directory. Fix that by teaching
> the code for creating the links about the new location of the binaries,
> and do the same in the wrapper script to allow it to find its payload.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
Apart from the missing accent ("é") in your SOB: ACK.
Ciao,
Dscho
^ permalink raw reply
* Re: feature request
From: Johannes Schindelin @ 2016-10-28 8:49 UTC (permalink / raw)
To: David Lang; +Cc: John Rood, Stefan Beller, git@vger.kernel.org
In-Reply-To: <alpine.DEB.2.02.1610271623260.4123@nftneq.ynat.uz>
Hi,
On Thu, 27 Oct 2016, David Lang wrote:
> On Thu, 27 Oct 2016, John Rood wrote:
>
> > Thanks, I think changing the default for windows is a good idea.
>
> notepad doesn't work well with unix line endings, wordpad handles the files
> much more cleanly.
That is why we have a `notepad` helper in Git for Windows that converts
line endings transparently before and after calling the real notepad.exe.
Ciao,
Johannes
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Jeff King @ 2016-10-28 8:10 UTC (permalink / raw)
To: Aaron Pelly; +Cc: git
In-Reply-To: <91e0f377-ecfd-ab0a-4f4b-8c0f762228aa@pelly.co>
On Fri, Oct 28, 2016 at 11:17:26AM +1300, Aaron Pelly wrote:
> On 28/10/16 10:55, Aaron Pelly wrote:
> > 2) I fetch a repo with a hostile ignore file. It includes files from
> > $GIT_DIR/test-data/ssl/private or some such. Change. Don't pay
> > attention. Commit. Push. Problems if my test data comes from production.
> >
> > Is this mitigated currently?
> >
> > Not that git should be an enabler, but surely it falls on the user of
> > untrusted software to ensure their own security?
>
> Balls, I meant $GIT_WORK_TREE not $GIT_DIR
I was going to ask what you meant by "currently" here, as we do not yet
have an include mechanism, and generally things in the repository are
fair game. But I guess you mean that there could be untracked files even
inside the repository.
I'm not too worried about that in general. You have to be careful of a
lot of things inside the repository, like running "make" on malicious
code. Adding "don't stick secret files inside the repository, even
untracked", does seem like another sensible precaution.
The main thing with malicious repositories is that basic inspection like
"git clone $remote && git log" should not execute arbitrary code, leak
information, etc.
-Peff
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Jeff King @ 2016-10-28 7:51 UTC (permalink / raw)
To: Linus Torvalds
Cc: Junio C Hamano, Git Mailing List, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <CA+55aFwfhFqV74s_O=GucycY9U19ysiACDqX=mK4Gf=eQ0coxQ@mail.gmail.com>
On Thu, Oct 27, 2016 at 03:38:59PM -0700, Linus Torvalds wrote:
> On Thu, Oct 27, 2016 at 3:24 AM, Jeff King <peff@peff.net> wrote:
> >
> > +cc Linus as the original author of 144bde78e9 in case there is
> > something subtle I'm missing, but this really just seems like it's
> > an outdated optimization.
>
> I'd *really* like to keep O_NOATIME if at all possible. It made a huge
> difference on older kernels, and I'm not convinced that relatime
> really fixes it as well as O_NOATIME.
>
> There are people who don't like relatime. And even if you do have
> relatime enabled, it will update atime once every day, so then this
> makes your filesystem have a storm of nasty inode writebacks if you
> haven't touched that git repo in a while.
The existence of "relatime" is only half the story of its outdatedness.
The other half is packfiles, so that we are paying atime only once per
packfile, not once per object (technically once per mmap(), so on a
32-bit system with large packfiles, it would be multiple, depending on
your window size).
So I'm not convinced that "storm" is really the right word in a modern
context. The atime updates due to object accesses are probably smaller
than those from all the other read() calls being done on non-object
files (like config, refs, etc).
That being said, if you really care, it's not that much code to keep.
-Peff
^ permalink raw reply
* Re: Drastic jump in the time required for the test suite
From: Duy Nguyen @ 2016-10-28 6:38 UTC (permalink / raw)
To: Eric Wong; +Cc: Johannes Schindelin, Junio C Hamano, Git Mailing List
In-Reply-To: <20161027204129.GA21619@starla>
On Fri, Oct 28, 2016 at 3:41 AM, Eric Wong <e@80x24.org> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> I know you are a fan of testing things thoroughly in the test suite, but I
>> have to say that it is getting out of hand, in particular due to our
>> over-use of shell script idioms (which really only run fast on Linux, not
>> a good idea for a portable software).
>
> How much effort would it take to optimize a /bin/sh?
>
> Would replacing uses of fork+execve posix_spawn be fast and
> portable enough?
>
> Even on Linux, performance sucks for me. I've been hoping dash
> can use posix_spawn (or using vfork directly) to see if that can
> help things.
Busybox hush uses vfork.on no-mmu builds. You could try it out. I
don't know how complete it is though.
--
Duy
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Jacob Keller @ 2016-10-28 6:11 UTC (permalink / raw)
To: Johannes Sixt
Cc: Stefan Beller, Junio C Hamano, Johannes Schindelin,
git@vger.kernel.org, Simon Ruderich, Jeff King
In-Reply-To: <2ddca5e3-3c4d-b555-4309-a180ceed581e@kdbg.org>
On Thu, Oct 27, 2016 at 10:55 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> One point is that the DCLP idiom must be implemented correctly. There are
> solutions, of course, and when the initialization is over, we have a
> miniscule overhead at each pthread_mutex_lock call.
>
Right, this I understood, but appeared to be solved.
> The main point is that the initialization has to solve a chicken-and-egg
> problem: After we have found an uninitialized critical section, we have to
> have mutual exclusion for the initialization. We need another critical
> section for this, but we cannot have one that is initialized. For this
> reason, the solution uses a different kind of mutual exclusion primitive,
> which is more akin to POSIX semaphores and works across processes. In the
> patch proposed by Stefan, a *session-wide* mutex is used. That means that
> all concurrent git invocations in a user's session synchronize their
> initialization of critical section objects.
>
Thank you for explaining this. Now I understand why this would be
considered a big issue, and potentially worth considering the
alternatives like attr_start. This was missing since I don't think any
of the rest of us knew (correct me if I am wrong) that the
synchronization would be global. For many cases it's probably not that
bad, but we do have a suitable explanation, and I think living with
"attr_start()" in the win32 initialization path which is something
Stefan suggested in a previous email would make the most sense then.
> That's just ridiculous. It's like waiting for a ... no, *the* ... battle
> ship just to get our bouncers in their position. We are talking milliseconds
> here, not nanoseconds.
>
Right. Thanks for filling in the missing details.
Regards,
Jake
> -- Hannes
>
^ permalink raw reply
* Re: [PATCH v15 01/27] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
From: Matthieu Moy @ 2016-10-28 6:02 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Pranit Bauva, Christian Couder, Alex Henrie, Antoine Delaite
In-Reply-To: <xmqqvawd7mnr.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Cc'ed those who touched either "git-bisect.sh" or "builtin/bisect-helper.c"
> in our relatively recent past.
>
> Does any of you (and others on the list) have time and inclination
> to review this series?
Unfortunately, I have essentially zero-bandwidth to do that in the near
future :-(.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Johannes Sixt @ 2016-10-28 5:55 UTC (permalink / raw)
To: Jacob Keller
Cc: Stefan Beller, Junio C Hamano, Johannes Schindelin,
git@vger.kernel.org, Simon Ruderich, Jeff King
In-Reply-To: <CA+P7+xpckfaeHmoEGQBdLD-=Kf7gQ-jOxGFKrKmiFH1SBN7GjA@mail.gmail.com>
Am 27.10.2016 um 23:49 schrieb Jacob Keller:
> Ok, so I've been reading this thread. I don't understand your
> objections to emulating in this way.. Could you clearly spell out why
> you believe this solution isn't acceptable? So far all I've understood
> was "it's not critical sections" and "it penalizes Windows too much"
> but... If Windows cannot statically initialize a pthread mutex, then
> we *have* to dynamically initialize it somewhere. This solution adds a
> single check before each lock and is safe due to use of memory
> barriers. Yes, this will cost a tiny bit extra overhead for each use
> of "pthread_mutex_lock" but I fail to see how that is a huge
> penalty...
One point is that the DCLP idiom must be implemented correctly. There
are solutions, of course, and when the initialization is over, we have a
miniscule overhead at each pthread_mutex_lock call.
The main point is that the initialization has to solve a chicken-and-egg
problem: After we have found an uninitialized critical section, we have
to have mutual exclusion for the initialization. We need another
critical section for this, but we cannot have one that is initialized.
For this reason, the solution uses a different kind of mutual exclusion
primitive, which is more akin to POSIX semaphores and works across
processes. In the patch proposed by Stefan, a *session-wide* mutex is
used. That means that all concurrent git invocations in a user's session
synchronize their initialization of critical section objects.
That's just ridiculous. It's like waiting for a ... no, *the* ... battle
ship just to get our bouncers in their position. We are talking
milliseconds here, not nanoseconds.
-- Hannes
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Eric Wong @ 2016-10-28 5:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Torvalds, Jeff King, Git Mailing List, Lars Schneider,
Johannes Schindelin
In-Reply-To: <xmqq60od42s0.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Linus Torvalds <torvalds@linux-foundation.org> writes:
> >
> >> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >>>
> >>> Would the best endgame shape for this function be to open with
> >>> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)
> >>> but ignoring an error from it, I guess? That would be the closest
> >>> to what we historically had, I would think.
> >>
> >> I think that's the best model.
Actually, I would flip the order of flags. O_CLOEXEC is more
important from a correctness standpoint.
> > OK, so perhaps like this.
>
> Hmph. This may not fly well in practice, though.
>
> To Unix folks, CLOEXEC is not a huge correctness issue. A child
> process may hold onto an open file descriptor a bit longer than the
> lifetime of the parent but as long as the child eventually exits,
I'm not too familiar with C internals of git; but I know we use
threads in some places, and fork+execve in others.
If our usage of threads and execve intersects, and we run
untrusted code in an execve-ed child, then only having cloexec
on open() will save us time when auditing for leaking FDs.
fcntl(fd, F_SETFD, O_CLOEXEC) is racy in if there are other
threads doing execve; so I wouldn't rely on it as a first
choice.
So I suppose something like this:
static int noatime = 1;
int fd = open(... | O_CLOEXEC);
...error checking and retrying...
if (fd >= 0 && noatime && fcntl(fd, F_SETFL, O_NOATIME) != 0)
noatime = 0;
return fd;
^ permalink raw reply
* Re: [RFC PATCH 0/5] recursively grep across submodules
From: Stefan Beller @ 2016-10-28 3:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org
In-Reply-To: <xmqq1sz1425w.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 27, 2016 at 7:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Unless you are imagining "git grep" to initialize and checkout a
> submodule that is not checked out on-demand, I do not think you have
> any reason to even look at ".gitmodules" for the purpose of "I want
> to grep both in superproject and submodules that are checked out."
In tree-ish mode you may have this example:
git -C superproject rm path/to/submodule
git -C superproject commit -a -m "delete submodule"
... time passes ...
git -C superproject grep --recurse-submodule -e <expression> \
HEAD~42 path/to/submodule
In the last command you need to map the path to submodule to
the name of the submodule to find out the place of the object store
for that submodule and see if it exists.
> If it is working with a tree-ish, again, go look at the object store
> in that submodule repository.
and to find out the object store for that submodule you need the
path -> name mapping at that point in time, i.e. you want to look
at the .gitmodules file at the given tree-ish.
^ permalink raw reply
* "git push" says "src refspec XYZ matches more than one" even without explicit XYZ argument.
From: Kannan Goundan @ 2016-10-28 3:00 UTC (permalink / raw)
To: git
1. My repo has a branch named 'v1' that is tracking 'origin/v1'.
2. My repo has a tag named 'v1'.
3. I have "push.default" set to "upstream".
I made a commit on branch 'v1' and tried doing a push:
# git push
error: src refspec v1 matches more than one.
error: failed to push some refs to 'git@github.com:whatever/ns1-go.git'
If I rename the branch to 'v1-dev', then the push goes through.
I understand why the command "git push origin/v1 v1" is ambiguous.
But if I do a plain "git push", I thought Git would know to push my
current branch.
[Git version 2.10.1 from Homebrew on Mac OS 10.11.6.]
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Junio C Hamano @ 2016-10-28 2:54 UTC (permalink / raw)
To: Jeff King; +Cc: Aaron Pelly, git
In-Reply-To: <20161027210753.btc7zbndhdocsbwa@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> However, as I said elsewhere, I'm not convinced this feature is all that
> helpful for in-repository .gitignore files, and I think it does
> introduce compatibility complications. People with older git will not
> respect your .gitignore.d files. Whereas $GIT_DIR/info is purely a local
> matter.
As I do not see the point of making in-tree .gitignore to a forest
of .gitignore.d/ at all, compatibility complications is not worth
even thinking about, I would have to say.
Thanks.
^ permalink raw reply
* Re: [RFC PATCH 0/5] recursively grep across submodules
From: Junio C Hamano @ 2016-10-28 2:50 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, git@vger.kernel.org
In-Reply-To: <CAGZ79kYm1txscyBpmfJQceCLFrZAN09y-2nV1zCjE2a1+_jrLA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> Just a few brief comments, before reading the patches carefully.
>>
>> * It is somewhat surprising that [1/5] is even needed (in other
>> words, I would have expected something like this to be already
>> there, and my knee-jerk reaction was "Heh, how does 'git status'
>> know how to show submodules that are and are not initialized
>> differently without this?"
>
> The issue with much of the existing code is that it is submodule centric,
> i.e. it is written to not care about the rest.
>
> git status for example just calls "git submodule summary" to
> parse and display the submodule information additionally.
> It doesn't integrate submodules and treats them "just like files".
Oh, I know all that after/while writing the above "it is somewhat
surprising" and reading what wt-status.c does. It was just that it
was somewhat surprising ;-)
> My reaction to 1/5 was that the implementation is sound,
> but the design may need rethinking.
>
> Instead of asking all these question, "Is a submodule
> * initialized
> * checked out (== have a working dir)
> * have a .git dir (think of deleted submodules that keep the
> historical git dir around)
> (* have commit X)
> we would want to either extend the submodule-config API
> to also carry these informations just like
> name/path/sha1/url/shallow clone recommendation.
I think you are going in a wrong direction with all the above.
Unless you are imagining "git grep" to initialize and checkout a
submodule that is not checked out on-demand, I do not think you have
any reason to even look at ".gitmodules" for the purpose of "I want
to grep both in superproject and submodules that are checked out."
You only need to detect .gitlink that exists in the index of the
superproject, and then there would be only two cases:
* $path has an empty directory (not even .git in there). The user
is not interested in that submodule.
* $path has ".git", either a directory (old layout or we are
dealing with the repository that originated the submodule) or a
"gitdir:" file that points into .git/modules of the repository of
the superproject. The user is interested in the submodule.
If $path has ".git" and nothing else, the only explanation is that
the user removed the working tree files in the submodule. If your
grep is looking at working tree files, it is correct not to find
anything in there. If it is working with "--cached", go look at the
index of the submodule repository (either the ".git" directory, or
the stashed-away repository in .git/modules/ in the superproject).
If it is working with a tree-ish, again, go look at the object store
in that submodule repository.
>> * It is somewhat surprising that [4/5] does not even use the
>> previous ls-files to find out the paths. Also it is a bit
>> disappointing to see that the way processes are spawned and
>> managed does not share much with Stefan's earlier work, i.e.
>> run_processes_parallel(). I was somehow hoping that it can be
>> extended to support this use case, but apparently there aren't
>> much to be shared.
>
> I think there are 2 issues here:
There is no issue here. I was just giving my impressions (i.e.
"somewhat surprising").
> * git-grep already has its own thread pool
I know. I was expecting that the previous "ls-files" that recurses
will be used to feed into that thread pool, but I didn't find that
in my cursory look at the patch, hence "somewhat surprising".
I hate it when people become overly defensive and start making
excuses when given harmless observations.
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Junio C Hamano @ 2016-10-28 2:37 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jeff King, Git Mailing List, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <xmqqa8dp46wx.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> Would the best endgame shape for this function be to open with
>>> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)
>>> but ignoring an error from it, I guess? That would be the closest
>>> to what we historically had, I would think.
>>
>> I think that's the best model.
>
> OK, so perhaps like this.
Hmph. This may not fly well in practice, though.
To Unix folks, CLOEXEC is not a huge correctness issue. A child
process may hold onto an open file descriptor a bit longer than the
lifetime of the parent but as long as the child eventually exits,
nothing is affected. Over there, things are different. The parent
cannot even rename(2) or unlink(2) a file it created and closed
while the child is still holding the file descriptor open and the
lack of CLOEXEC will make the parent fail. I do not know how well
fcntl(2) emulation works on Windows, but I would not be surprised
if J6t or Dscho comes back and says that FD_CLOEXEC given to F_SETFD
would not work while O_CLOEXEC given to open(2) does.
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Junio C Hamano @ 2016-10-28 1:08 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jeff King, Git Mailing List, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <CA+55aFw83E+zOd+z5h-CA-3NhrLjVr-anL6pubrSWttYx3zu8g@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Would the best endgame shape for this function be to open with
>> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)
>> but ignoring an error from it, I guess? That would be the closest
>> to what we historically had, I would think.
>
> I think that's the best model.
OK, so perhaps like this.
-- >8 --
Subject: git_open(): untangle possible NOATIME and CLOEXEC interactions
The way we structured the fallback-retry for opening with O_NOATIME
and O_CLOEXEC meant that if we failed due to lack of support to open
the file with O_NOATIME option (i.e. EINVAL), we would still try to
drop O_CLOEXEC first and retry, and then drop O_NOATIME. A platform
on which O_NOATIME is defined in the header without support from the
kernel wouldn't have a chance to open with O_CLOEXEC option due to
this code structure.
Arguably, O_CLOEXEC is more important than O_NOATIME, as the latter
is mostly about performance, while the former can affect correctness.
Let's revert the recent changes to the way git_open() attempts to
open a file with O_NOATIME and retries without to the original
sequence, and then use a separate fcntl(fd, F_SETFD, FD_CLOEXEC) on
the resulting file descriptor. The helper to do the latter can be
usable in the codepath in ce_compare_data() that was recently added
to open a file descriptor with O_CLOEXEC, so let's refactor that
codepath with the helper while we are at it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-compat-util.h | 5 +++--
read-cache.c | 12 ++++--------
sha1_file.c | 49 ++++++++++++++++++++++++++++++-------------------
3 files changed, 37 insertions(+), 29 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 43718dabae..a751630db5 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -679,9 +679,10 @@ char *gitstrdup(const char *s);
#define getpagesize() sysconf(_SC_PAGESIZE)
#endif
-#ifndef O_CLOEXEC
-#define O_CLOEXEC 0
+#ifndef FD_CLOEXEC
+#define FD_CLOEXEC 0
#endif
+extern int git_set_cloexec(int);
#ifdef FREAD_READS_DIRECTORIES
#ifdef fopen
diff --git a/read-cache.c b/read-cache.c
index db5d910642..fb91514885 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -156,17 +156,13 @@ 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;
- 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);
- }
+ int fd = open(ce->name, O_RDONLY);
if (fd >= 0) {
unsigned char sha1[20];
+
+ /* do not let child processes to hold onto the open fd */
+ git_set_cloexec(fd);
if (!index_fd(sha1, fd, st, OBJ_BLOB, ce->name, 0))
match = hashcmp(sha1, ce->oid.hash);
/* index_fd() closed the file descriptor already */
diff --git a/sha1_file.c b/sha1_file.c
index 09045df1dc..41383a6c20 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1559,31 +1559,42 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
return hashcmp(sha1, real_sha1) ? -1 : 0;
}
-int git_open(const char *name)
+int git_set_cloexec(int fd)
{
- static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
+ static int cloexec = FD_CLOEXEC;
- for (;;) {
- int fd;
+ if (cloexec) {
+ if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0)
+ cloexec = 0;
+ /*
+ * We might want to diagnose and complain upon seeing
+ * an error from this call, but let's keep the same
+ * behaviour as before for now.
+ */
+ }
+ return 0;
+}
- errno = 0;
- fd = open(name, O_RDONLY | sha1_file_open_flag);
- if (fd >= 0)
- return fd;
+int git_open(const char *name)
+{
+ static int noatime = O_NOATIME;
+ int fd;
- /* 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;
- }
+ errno = 0;
+ fd = open(name, O_RDONLY | noatime);
- /* 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;
+ /* Might the failure be due to O_NOATIME? */
+ if ((noatime & O_NOATIME) && errno != ENOENT) {
+ noatime = 0;
+ fd = open(name, O_RDONLY);
}
+
+ if (fd < 0)
+ return fd;
+
+ /* do not let child processes to hold onto the open fd */
+ git_set_cloexec(fd);
+ return fd;
}
static int stat_sha1_file(const unsigned char *sha1, struct stat *st)
^ permalink raw reply related
* Re: [RFC PATCH 0/5] recursively grep across submodules
From: Stefan Beller @ 2016-10-28 0:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org
In-Reply-To: <xmqqk2ct4bmr.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 27, 2016 at 4:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>> As for the rest of the series, it should be ready for review or comments.
>
> Just a few brief comments, before reading the patches carefully.
>
> * It is somewhat surprising that [1/5] is even needed (in other
> words, I would have expected something like this to be already
> there, and my knee-jerk reaction was "Heh, how does 'git status'
> know how to show submodules that are and are not initialized
> differently without this?"
The issue with much of the existing code is that it is submodule centric,
i.e. it is written to not care about the rest.
git status for example just calls "git submodule summary" to
parse and display the submodule information additionally.
It doesn't integrate submodules and treats them "just like files".
git submodule summary then proceeds to use "submodule--helper list"
that lists submodules *only* ignoring all files.
>
> The implementation that reads from the config of the current
> repository may be OK, but I actually would have expected that a
> check would be "given a $path, check to see if $path/.git is
> there and is a valid repository". In a repository where the
> submodules originate, there may not even be submodule.$name.url
> entries there yet.
My reaction to 1/5 was that the implementation is sound,
but the design may need rethinking.
Instead of asking all these question, "Is a submodule
* initialized
* checked out (== have a working dir)
* have a .git dir (think of deleted submodules that keep the
historical git dir around)
(* have commit X)
we would want to either extend the submodule-config API
to also carry these informations just like
name/path/sha1/url/shallow clone recommendation.
Obtaining the information above is however not as cheap,
because we'd need to do extra work additionally to parsing
the .gitmodules file. So the submodule-config would need to learn
an input that will tell the submodule-config what informations should
be evaluated and which can be omitted.
>
> * It is somewhat surprising that [4/5] does not even use the
> previous ls-files to find out the paths. Also it is a bit
> disappointing to see that the way processes are spawned and
> managed does not share much with Stefan's earlier work, i.e.
> run_processes_parallel(). I was somehow hoping that it can be
> extended to support this use case, but apparently there aren't
> much to be shared.
I think there are 2 issues here:
* The API I designed runs processes in parallel and the order or
output is non-deterministic. git-grep uses threads and output is
alphabetically sorted. The order is fixable though (by e.g. adding
a flag that indicates which parallel processing output the caller
wants).
* git-grep already has its own thread pool; integrating/combining
2 worker pools doesn't sound trivial even to someone who wrote
one of them.
Maybe we could extend/rewrite the run_processes_parallel
API to not just run processes, but instead you could also provide
a function pointer that is used in a thread instead.
Then we'd have one machinery that e.g. keeps track of the
number of parallel processes/threads.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Linus Torvalds @ 2016-10-27 23:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Git Mailing List, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <xmqqfunh4b63.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Would the best endgame shape for this function be to open with
> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)
> but ignoring an error from it, I guess? That would be the closest
> to what we historically had, I would think.
I think that's the best model.
Note that the O_NOATIME code is very much designed to try O_NOATIME
only _once_. Because even when the kernel supports O_NOATIME, if you
have a shared object tree where you may not be the owner of all the
files, a O_NOATIME open can fail with NOPERM ("You are not allowed to
hide your accesses to this file").
This is why it uses that
static unsigned int sha1_file_open_flag = O_NOATIME;
and if the O_NOATIME open ever fails (and the no-O_NOATIME open
succeeds), it clears that flag. Exactly so that it will *not* end up
in some kind of "let's open and fail and re-open" loop. It's designed
to fail once.
Or at least that's how it used to be originally. This code has
obviously changed since that early design. Now it seems to clear it
for any non-ENOENT error. Which looks fine too.
Linus
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Junio C Hamano @ 2016-10-27 23:36 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jeff King, Git Mailing List, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <CA+55aFxdy4maom8byH0FoBBMWx+sQB8J7uWvHOxswjiaAhSjVg@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> But the basic issue still remains - I'd really prefer to have NOATIME
> stay around for all those poor misguided souls that for some reason
> don't like "relatime" or run old kernels. But whether it is with
> O_NOATIME at open time or with F_SETFL, I don't care.
Understood.
Would the best endgame shape for this function be to open with
O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)
but ignoring an error from it, I guess? That would be the closest
to what we historically had, I would think.
^ permalink raw reply
* Re: feature request
From: David Lang @ 2016-10-27 23:24 UTC (permalink / raw)
To: John Rood; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <CALj-rGfAyimf0nFFcDHVHUgj8PQaz6Cvoz_PQfqdhr=QJEpbRw@mail.gmail.com>
On Thu, 27 Oct 2016, John Rood wrote:
> Thanks, I think changing the default for windows is a good idea.
notepad doesn't work well with unix line endings, wordpad handles the files much
more cleanly.
David Lang
^ 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