* Re: [PATCH] t6026-merge-attr: wait for process to release trash directory
From: Johannes Schindelin @ 2016-09-08 8:05 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Johannes Sixt, Git Mailing List
In-Reply-To: <20160907190004.dw3p6fxkdaubwuvu@sigill.intra.peff.net>
Hi Peff & Junio,
On Wed, 7 Sep 2016, Jeff King wrote:
> On Wed, Sep 07, 2016 at 11:39:57AM -0700, Junio C Hamano wrote:
>
> > > Can we do some signaling with fifos to tell the hook when it is safe to
> > > exit? Then we would just need to `wait` for its parent process.
> >
> > Is fifo safe on Windows, though?
>
> No clue. We seem to use mkfifo unconditionally in lib-daemon, but
> perhaps people do not run that test on Windows. Other invocations seem
> to be protected by the PIPE prerequisite. But...
AFAICT we do not use mkfifo on Windows. Let's see what t/test-lib.sh has
to say about the matter:
test_lazy_prereq PIPE '
# test whether the filesystem supports FIFOs
case $(uname -s) in
CYGWIN*|MINGW*)
false
;;
*)
rm -f testfifo && mkfifo testfifo
;;
esac
'
So there you go.
The reason it is disabled is that Cygwin/MSYS2 do have a concept of a
FIFO. But `git.exe` won't be able to access such a FIFO because it is
emulated by the POSIX emulation layer, which Git cannot access.
> > With v2 that explicitly kills, I guess we can make the sleep longer
> > without slowing down in the optimistic case?
>
> Yeah, I think the v2 one is non-racy (I thought at first we might race
> with the "echo", but it should be synchronous; the hook will not exit
> until we have written the pid file, and git will not exit until the hook
> is done running).
Please note that Hannes and I discussed this (as I originally suggested to
increase it to 10 seconds, and Hannes rightfully pointed out that we would
have to change the script name, too, as it says sleep-one-second.sh, and
that would have made the patch less readable) and we came to the same
conclusion: it's not necessary.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v2 3/3] Use the newly-introduced regexec_buf() function
From: Jeff King @ 2016-09-08 8:10 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609080954010.129229@virtualbox>
On Thu, Sep 08, 2016 at 09:54:51AM +0200, Johannes Schindelin wrote:
> > diff.c | 3 ++-
> > diffcore-pickaxe.c | 18 ++++++++----------
> > xdiff-interface.c | 13 ++++---------
> > 3 files changed, 14 insertions(+), 20 deletions(-)
>
> I just realized that this should switch the test_expect_failure from 1/3
> to a test_expect_success.
Yep. I wonder if we also would want to test that we correctly find
regexes inside binary files.
E.g., given a mixed binary/text file like:
printf 'binary\0text' >file &&
git add file &&
git commit -m file
then "git log -Stext" will find that file, but "--pickaxe-regex" will
not (using stock git). Ditto for "-Gtext".
Your patch should fix that.
-Peff
^ permalink raw reply
* Re: [PATCH v2 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
From: Jeff King @ 2016-09-08 8:13 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609080931250.129229@virtualbox>
On Thu, Sep 08, 2016 at 09:33:29AM +0200, Johannes Schindelin wrote:
> On Thu, 8 Sep 2016, Johannes Schindelin wrote:
>
> > We solve this by introducing a helper, regexec_buf(), that takes a
> > pointer and a length instead of a NUL-terminated string.
>
> BTW I should have clarified why I decided on another name than regexecn()
> (I had considered this even before reading Peff's proposed patch): the <n>
> in string functions suggest a limiting of NUL-terminated strings. In other
> words, if n = 100 and the provided pointer points to a NUL-terminated
> string of length 3, the *n function will treat it as a string of length 3.
>
> That is not what regexec_buf() does: it ignores the NUL. Hence the
> different name.
I agree that is a better name (this was the exact thing I wondered about
with REG_STARTEND, but certainly what we _want_ is true "_buf"
semantics).
I guess an argument that REG_STARTEND does what we want everywhere is
that the GNU implementation does what we want, and since it has been
around for over a decade presumably _somebody_ would have complained if
it did not match the NetBSD behavior.
-Peff
^ permalink raw reply
* Re: [PATCH v2 3/3] Use the newly-introduced regexec_buf() function
From: Jeff King @ 2016-09-08 8:14 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <20160908081024.yku2xlb3jj4rplkp@sigill.intra.peff.net>
On Thu, Sep 08, 2016 at 04:10:24AM -0400, Jeff King wrote:
> On Thu, Sep 08, 2016 at 09:54:51AM +0200, Johannes Schindelin wrote:
>
> > > diff.c | 3 ++-
> > > diffcore-pickaxe.c | 18 ++++++++----------
> > > xdiff-interface.c | 13 ++++---------
> > > 3 files changed, 14 insertions(+), 20 deletions(-)
> >
> > I just realized that this should switch the test_expect_failure from 1/3
> > to a test_expect_success.
>
> Yep. I wonder if we also would want to test that we correctly find
> regexes inside binary files.
>
> E.g., given a mixed binary/text file like:
>
> printf 'binary\0text' >file &&
> git add file &&
> git commit -m file
>
> then "git log -Stext" will find that file, but "--pickaxe-regex" will
> not (using stock git). Ditto for "-Gtext".
>
> Your patch should fix that.
Of course if I had actually _looked carefully_ at your patch, I would
have seen that your test doesn't just check that we don't segfault, but
actually confirms that we find the entry.
Sorry for the noise.
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers
From: Jeff King @ 2016-09-08 8:22 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609080933470.129229@virtualbox>
On Thu, Sep 08, 2016 at 09:49:38AM +0200, Johannes Schindelin wrote:
> > > diff --git a/diff.c b/diff.c
> > > index 534c12e..2c5a360 100644
> > > --- a/diff.c
> > > +++ b/diff.c
> > > @@ -951,7 +951,13 @@ static int find_word_boundaries(mmfile_t *buffer,
> > > regex_t *word_regex,
> > > {
> > > if (word_regex && *begin < buffer->size) {
> > > regmatch_t match[1];
> > > - if (!regexec(word_regex, buffer->ptr + *begin, 1, match,
> > > 0)) {
> > > + int f = 0;
> > > +#ifdef REG_STARTEND
> > > + match[0].rm_so = 0;
> > > + match[0].rm_eo = *end - *begin;
> > > + f = REG_STARTEND;
> > > +#endif
> > > + if (!regexec(word_regex, buffer->ptr + *begin, 1, match,
> > > f)) {
>
> Heh. You introduced the same bug I did. Or maybe you just fetched my
> mmap-regexec branch and looked at an intermediate iteration?
I do not think I introduced anything. The quoted text is what you
sent. Which is perhaps why it has your bug. :)
> > But I much prefer this approach to copying the data just to add a NUL.
>
> I think it is not worth the burden. The only regex implementation in
> semi-widespread use that do not support REG_STARTEND seems to be musl.
>
> I'd rather not spend *so much* effort just to support an obscure platform.
> Not when the users of that obscure platform could spend that effort
> themselves. And probably won't, because we only copy data to add a NUL on
> those platforms when regexec() is called on an mmfile_t.
I'm confused about what you think I'm proposing. I was saying I _like_
something like regexec_buf() instead of copying the data. Which seems
like the simpler thing to me (and presumably to you). Or do you mean
using compat/regex to build on re_search() consistently? I do not think
that is all that complex; the question is only whether people really
want to use their own regex libraries.
Between the two options for regexec_buf(), I think you have convinced me
that REG_STARTEND is better than just using compat/regex everywhere. I
do think the fallback for platforms like musl should be "use
compat/regex" and not doing an expensive copy (which in most cases is
not even necessary).
-Peff
^ permalink raw reply
* Re: [PATCH] t6026-merge-attr: wait for process to release trash directory
From: Jeff King @ 2016-09-08 8:27 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Johannes Sixt, Git Mailing List
In-Reply-To: <alpine.DEB.2.20.1609081002490.129229@virtualbox>
On Thu, Sep 08, 2016 at 10:05:58AM +0200, Johannes Schindelin wrote:
> > > Is fifo safe on Windows, though?
> >
> > No clue. We seem to use mkfifo unconditionally in lib-daemon, but
> > perhaps people do not run that test on Windows. Other invocations seem
> > to be protected by the PIPE prerequisite. But...
>
> AFAICT we do not use mkfifo on Windows. Let's see what t/test-lib.sh has
> to say about the matter:
>
> test_lazy_prereq PIPE '
> # test whether the filesystem supports FIFOs
> case $(uname -s) in
> CYGWIN*|MINGW*)
> false
> ;;
> *)
> rm -f testfifo && mkfifo testfifo
> ;;
> esac
> '
>
> So there you go.
>
> The reason it is disabled is that Cygwin/MSYS2 do have a concept of a
> FIFO. But `git.exe` won't be able to access such a FIFO because it is
> emulated by the POSIX emulation layer, which Git cannot access.
Regarding my "unconditionally" above: coincidentally, I happened to be
looking in lib-git-daemon.sh about an hour ago and noticed that we do
indeed check "test_have_prereq PIPE" (just not near the mkfifo, of
course, because we are not in a test block).
It seems to have been added by a "Johannes Schindelin". Any relation?
So yeah. It definitely would be a bad idea to use a fifo in a test that
is already Windows-specific.
-Peff
^ permalink raw reply
* Re: [PATCH v2 3/3] Use the newly-introduced regexec_buf() function
From: Jeff King @ 2016-09-08 8:35 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <20160908081446.zlcmz6ci4cw4vc2e@sigill.intra.peff.net>
On Thu, Sep 08, 2016 at 04:14:46AM -0400, Jeff King wrote:
> On Thu, Sep 08, 2016 at 04:10:24AM -0400, Jeff King wrote:
>
> > On Thu, Sep 08, 2016 at 09:54:51AM +0200, Johannes Schindelin wrote:
> >
> > > > diff.c | 3 ++-
> > > > diffcore-pickaxe.c | 18 ++++++++----------
> > > > xdiff-interface.c | 13 ++++---------
> > > > 3 files changed, 14 insertions(+), 20 deletions(-)
> > >
> > > I just realized that this should switch the test_expect_failure from 1/3
> > > to a test_expect_success.
> >
> > Yep. I wonder if we also would want to test that we correctly find
> > regexes inside binary files.
> >
> > E.g., given a mixed binary/text file like:
> >
> > printf 'binary\0text' >file &&
> > git add file &&
> > git commit -m file
> >
> > then "git log -Stext" will find that file, but "--pickaxe-regex" will
> > not (using stock git). Ditto for "-Gtext".
> >
> > Your patch should fix that.
>
> Of course if I had actually _looked carefully_ at your patch, I would
> have seen that your test doesn't just check that we don't segfault, but
> actually confirms that we find the entry.
>
> Sorry for the noise.
Actually, I take it back again. Your test case doesn't have an embedded
NUL in it (so we check that git finds it, but aside from the lack of
segfault, stock git would already find it).
Sorry for the double-noise.
-Peff
^ permalink raw reply
* Re: [ANNOUNCE] Git for Windows 2.10.0
From: stefan.naewe @ 2016-09-08 10:05 UTC (permalink / raw)
To: johannes.schindelin, git-for-windows, git
In-Reply-To: <0MZD0K-1bNOQ13s6O-00Ku7h@mail.gmx.com>
Am 03.09.2016 um 15:17 schrieb Johannes Schindelin:
> Dear Git users,
>
> It is my pleasure to announce that Git for Windows 2.10.0 is available.
> This time, I even blogged about it, primarily because I am so excited
> about the speed improvements of rebase -i:
>
> https://blogs.msdn.microsoft.com/visualstudioalm/2016/09/03/whats-new-in-git-for-windows-2-10/
>
> As always, you can download it from: https://git-for-windows.github.io/
>
> Changes since Git for Windows v2.9.3(2) (August 25th 2016)
>
> New Features
>
> • Comes with Git v2.10.0.
> • The git rebase -i command was made faster by reimplementing large
> parts in C.
I finally had the chance to do a "bigger" rebase and what shall I say...
F***k, has this thing become fast, or what!
Thank you so much for doing this!!!!
Stefan
--
----------------------------------------------------------------
/dev/random says: A Cat's courage is as strong as a dog's chain
python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF
^ permalink raw reply
* Re: "fatal error in commit_refs" from pushing to github
From: Duy Nguyen @ 2016-09-08 11:03 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20160908012553.q2aubze4qggfzjxz@sigill.intra.peff.net>
On Thu, Sep 8, 2016 at 8:25 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Sep 08, 2016 at 07:49:12AM +0700, Duy Nguyen wrote:
>
>> I got the message in the subject when pushing to github today. Yes I
>> know it's github, not git. But according to stackoveflow [1] it's a
>> local problem. Which makes me think, if we know exactly what this is
>> (or at least roughly the problem area), maybe we could improve git to
>> catch it locally in the first place (and because other git servers may
>> not have the same protection as github). Jeff maybe you can reveal
>> something about this "fatal error in commit_refs"? I'm sure it's not
>> in git code. But I would understand if the answer is "no".
>
> The short answer is that it's nothing to do with Git or the client; it's
> GitHub-specific code running on the server that is outside of Git
> entirely.
>
> The long answer is that pushes to GitHub don't hit Git directly these
> days. They hit a proxy layer that speaks just enough of the Git protocol
> to relay to N separate receives spread across N replica servers[1]. Those
> receive-packs take in the pack and verify it, but don't actually update
> any refs[2]. Then the proxy layer runs its own set of policy hooks, and
> speaks a commit-protocol to each of the replicas so that they all agree
> on the new ref state. That last step is called "commit_refs" internally.
>
> So this is really an internal failure at the ref-update stage. There
> _should_ be a reasonable error message, but I think "fatal error in
> commit_refs" is the generic last-ditch fallback. I'll pass this along to
> people in charge of that code, as we should be generating a more useful
> error message.
Hmm.. I'm interested in this because the "fix" is from client side. I
did "git gc" and "git fetch" and the problem was gone. From this
description, I suppose C Git sends a good pack (phew!), but probably
with some stale ref or something that upsets this this last stage.
It's hard to make a connection back to either gc or fetch. Maybe gc
does ref trimming or something (that should probably be done by
git-push as well). Oh well.. maybe next time I see it, I'll get a nice
and clear message :)
--
Duy
^ permalink raw reply
* Re: [PATCH v2 6/6] git-gui: Update Japanese information
From: Satoshi Yasushima @ 2016-09-08 12:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Pat Thoyts, JakubNar?bski, Satoshi Yasushima
In-Reply-To: <xmqqk2enobol.fsf@gitster.mtv.corp.google.com>
On Wed, 07 Sep 2016 10:35:22 -0700 Junio C Hamano wrote:
>Since I received the patch directly bypassing vger, I queued it on
>gitgui-0.20.0 from Pat and tentatively merged it to my 'pu'.
wow, thanks so much.
^ permalink raw reply
* Re: How to simulate a real checkout to test a new smudge filter?
From: Jakub Narębski @ 2016-09-08 13:12 UTC (permalink / raw)
To: john smith, Torsten Bögershausen; +Cc: git
In-Reply-To: <CAKmQUfbemaid61xPyvNheLM2jVGXGjiyF_x=NZnxkZ=5wccQ=Q@mail.gmail.com>
W dniu 06.09.2016 o 23:01, john smith pisze:
> I'd prefer smudge/clean filters instead of `make' scripts etc. to
> convert template dotfiles into something usable and back because
> filters:
>
> 1. could be run automatically
>
> 2. do not modify files as shown by `git show HEAD:<file>' and
> therefore no files are reported as modified by git status and also
> there are not conflicts when merging master into work/home branch.
>
> I have problems because with point 1 because apparently smudge filter
> is not run automatically every time when branch is changed if files
> listed in .gitattributes do not change. As the last resort I could
> force smudge/clean filter to run just to keep advantage specified in
> point 2.
Couldn't you use post-checkout hook plus clean filter instead of
clean/smudge filter pair, if the smudge part depends on the branch?
Or make post-checkout hook invoke smudge filter... though
`git cat-file --filters` is not in any released version, I think...
Best,
--
Jakub Narębski
^ permalink raw reply
* [PATCH 0/3] Fix git-init in linked worktrees
From: Nguyễn Thái Ngọc Duy @ 2016-09-08 13:47 UTC (permalink / raw)
To: git; +Cc: git, max.nordlund, Nguyễn Thái Ngọc Duy
In-Reply-To: <CACsJy8CZf0O+uyQaeJ4gcx4XN8ivfFyni+3586WX_R2QM4XgVw@mail.gmail.com>
My ASAP is not so ASAP. Sorry about that but I think I have fixed it.
Side note about 2/3. I've known this problem (in general) for years
(accidentally reading .git config file before .git is searched) and
could not do anything about it. And because test_expect_failure should
only be there if someone will eventually fix it, and I don't see
myself doing it, and I don't see it super important that other people
would bother, so I decided to simply sweep it under the rug. I could
make a test_expect_failure if people think otherwise.
Side note about 3/3. Yes there's a FIXME in there. It will take a lot
more time to remove that FIXME (because setup_git_directory is not as
simple). For now it should be ok to leave it there. When we find a use
for get_first_git_dir outside git-init, we can fix it then.
Nguyễn Thái Ngọc Duy (3):
init: correct re-initialization from a linked worktree
t0001: work around the bug that reads config file before repo setup
init: do not set core.worktree more often than necessary
builtin/init-db.c | 4 ++--
cache.h | 1 +
environment.c | 16 +++++++++++++++-
t/t0001-init.sh | 19 +++++++++++++++++++
4 files changed, 37 insertions(+), 3 deletions(-)
--
2.8.2.524.g6ff3d78
^ permalink raw reply
* [PATCH 1/3] init: correct re-initialization from a linked worktree
From: Nguyễn Thái Ngọc Duy @ 2016-09-08 13:47 UTC (permalink / raw)
To: git; +Cc: git, max.nordlund, Nguyễn Thái Ngọc Duy
In-Reply-To: <20160908134719.27955-1-pclouds@gmail.com>
When 'git init' is called from a linked worktree, '.git' dir as the main
'.git' (i.e. $GIT_COMMON_DIR) and populate the whole repository skeleton
in there. It does not harm anything (*) but it is still wrong.
Since 'git init' calls set_git_dir() at preparation time, which
indirectly calls get_common_dir() and correctly detects multiple
worktree setup, all git_path_buf() calls in create_default_files() will
return correct paths in both single and multiple worktree setups. The
only thing left is copy_templates(), which targets $GIT_DIR, not
$GIT_COMMON_DIR.
Fix that with get_git_common_dir(). This function will return $GIT_DIR
in single-worktree setup, so we don't have to make a special case for
multiple-worktree here.
(*) It does in fact, thanks to another bug. More on that later.
Noticed-by: Max Nordlund <max.nordlund@sqore.com>
Helped-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/init-db.c | 2 +-
t/t0001-init.sh | 15 +++++++++++++++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 3a45f0b..6d9552e 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -138,7 +138,7 @@ static void copy_templates(const char *template_dir)
goto close_free_return;
}
- strbuf_addstr(&path, get_git_dir());
+ strbuf_addstr(&path, get_git_common_dir());
strbuf_complete(&path, '/');
copy_templates_1(&path, &template_path, dir);
close_free_return:
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index a6fdd5e..d64e5e3 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -384,4 +384,19 @@ test_expect_success MINGW 'bare git dir not hidden' '
! is_hidden newdir
'
+test_expect_success 're-init from a linked worktree' '
+ git init main-worktree &&
+ (
+ cd main-worktree &&
+ test_commit first &&
+ git worktree add ../linked-worktree &&
+ mv .git/info/exclude expected-exclude &&
+ find .git/worktrees -print | sort >expected &&
+ git -C ../linked-worktree init &&
+ test_cmp expected-exclude .git/info/exclude &&
+ find .git/worktrees -print | sort >actual &&
+ test_cmp expected actual
+ )
+'
+
test_done
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 2/3] t0001: work around the bug that reads config file before repo setup
From: Nguyễn Thái Ngọc Duy @ 2016-09-08 13:47 UTC (permalink / raw)
To: git; +Cc: git, max.nordlund, Nguyễn Thái Ngọc Duy
In-Reply-To: <20160908134719.27955-1-pclouds@gmail.com>
git-init somehow reads '.git/config' at current directory and sets
log_all_ref_updates based on this file. Because log_all_ref_updates is
not unspecified (-1) any more. It will not be written to the new repo's
config file (see create_default_files() function).
This will affect our tests in the next patch as we will compare the
config file and expect that core.logallrefupdates is already set to true
by "git init main-worktree".
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
t/t0001-init.sh | 2 ++
1 file changed, 2 insertions(+)
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index d64e5e3..393c940 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -385,7 +385,9 @@ test_expect_success MINGW 'bare git dir not hidden' '
'
test_expect_success 're-init from a linked worktree' '
+ mv .git/config work-around-init-reading-wrong-file &&
git init main-worktree &&
+ mv work-around-init-reading-wrong-file .git/config &&
(
cd main-worktree &&
test_commit first &&
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH 3/3] init: do not set core.worktree more often than necessary
From: Nguyễn Thái Ngọc Duy @ 2016-09-08 13:47 UTC (permalink / raw)
To: git; +Cc: git, max.nordlund, Nguyễn Thái Ngọc Duy
In-Reply-To: <20160908134719.27955-1-pclouds@gmail.com>
When "git init" is called with GIT_WORK_TREE environment set, we want to
keep this worktree's location in core.worktree so the user does not have
to set the environment again and again. See ef6f0af (git-init: set
core.worktree if GIT_WORK_TREE is specified - 2007-07-04)
We detect that by this logic (in needs_work_tree_config): normally
worktree's top dir would contains ".git" directory, if this is not true,
worktree is probably set to elsewhere by the user.
Unfortunately when it calls get_git_dir() it does not take ".git" files
into account. When we find a .git file, we immediately follow the file
until we find the real ".git" directory. The location of this first
".git" file is lost.
The .git file would satisfy the logic above and not create
core.worktree (correct). But because the final .git's location is used,
needs_work_tree_config() is misled and creates core.worktree anyway.
This would not be a huge deal normally. But if this happens in a
multiple worktree setup it becomes a real problem because up until now,
core.worktree will be applied to the main worktree only. If you
accidentally do "git init" from a linked worktree, you set
core.worktree (for the main repo) pointing to the _linked_ worktree.
After that point, may you live in interesting times.
Record the .git file location and use it here.
Noticed-by: Max Nordlund <max.nordlund@sqore.com>
Helped-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/init-db.c | 2 +-
cache.h | 1 +
environment.c | 16 +++++++++++++++-
t/t0001-init.sh | 2 ++
4 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 6d9552e..36255f2 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -254,7 +254,7 @@ static int create_default_files(const char *template_path)
/* allow template config file to override the default */
if (log_all_ref_updates == -1)
git_config_set("core.logallrefupdates", "true");
- if (needs_work_tree_config(get_git_dir(), work_tree))
+ if (needs_work_tree_config(get_first_git_dir(), work_tree))
git_config_set("core.worktree", work_tree);
}
diff --git a/cache.h b/cache.h
index b780a91..e6c05f8 100644
--- a/cache.h
+++ b/cache.h
@@ -460,6 +460,7 @@ extern char *git_work_tree_cfg;
extern int is_inside_work_tree(void);
extern const char *get_git_dir(void);
extern const char *get_git_common_dir(void);
+extern const char *get_first_git_dir(void);
extern char *get_object_directory(void);
extern char *get_index_file(void);
extern char *get_graft_file(void);
diff --git a/environment.c b/environment.c
index ca72464..8cfb8f3 100644
--- a/environment.c
+++ b/environment.c
@@ -100,7 +100,7 @@ static char *work_tree;
static const char *namespace;
static size_t namespace_len;
-static const char *git_dir, *git_common_dir;
+static const char *git_dir, *git_common_dir, *first_git_dir;
static char *git_object_dir, *git_index_file, *git_graft_file;
int git_db_env, git_index_env, git_graft_env, git_common_dir_env;
@@ -168,6 +168,8 @@ static void setup_git_env(void)
if (!git_dir)
git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
gitfile = read_gitfile(git_dir);
+ if (gitfile && !first_git_dir)
+ first_git_dir = xstrdup(git_dir);
git_dir = xstrdup(gitfile ? gitfile : git_dir);
if (get_common_dir(&sb, git_dir))
git_common_dir_env = 1;
@@ -203,6 +205,18 @@ const char *get_git_dir(void)
return git_dir;
}
+/*
+ * Return the first ".git" that we have encountered.
+ * FIXME this function for not entirely correct because
+ * setup_git_directory() and enter_repo() do not update first_git_dir
+ * when they follow .git files. The function in its current state is
+ * only suitable for "git init".
+ */
+const char *get_first_git_dir(void)
+{
+ return first_git_dir ? first_git_dir : git_dir;
+}
+
const char *get_git_common_dir(void)
{
return git_common_dir;
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 393c940..d59669a 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -393,9 +393,11 @@ test_expect_success 're-init from a linked worktree' '
test_commit first &&
git worktree add ../linked-worktree &&
mv .git/info/exclude expected-exclude &&
+ cp .git/config expected-config &&
find .git/worktrees -print | sort >expected &&
git -C ../linked-worktree init &&
test_cmp expected-exclude .git/info/exclude &&
+ test_cmp expected-config .git/config &&
find .git/worktrees -print | sort >actual &&
test_cmp expected actual
)
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* Re: [PATCH v3 2/2] connect: advertized capability is not a ref
From: Junio C Hamano @ 2016-09-08 16:18 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jonathan Tan, git, spearce, sbeller, peff
In-Reply-To: <20160908013431.GC25016@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
> I think we can make this stricter. The capabilities^{} line is supposed
> to be the first advertised ref, before any 'shallow' lines or .have
> extra refs.
"The first", or "the first and only"? I thought that it would be
the latter.
^ permalink raw reply
* Re: [PATCH] connect: tighten check for unexpected early hang up (Re: [PATCH v3 2/2] connect: advertized capability is not a ref)
From: Stefan Beller @ 2016-09-08 16:28 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jonathan Tan, git@vger.kernel.org, Shawn Pearce, Jeff King,
Junio C Hamano, Heiko Voigt
In-Reply-To: <20160908014555.GD25016@google.com>
On Wed, Sep 7, 2016 at 6:45 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> (+cc: Heiko)
> Jonathan Nieder wrote:
>
>> 'die_initial_contact' uses got_at_least_one_head to determine whether
>> it was on the first line but code paths added later that use
>> 'continue' don't populate it properly (see b06dcd7d, 40c155ff, and
>> 1a7141ff). We could do
>>
>> int first_line = 1;
>>
>> for (;; first_line = 0) {
>> ...
>> }
>>
>> and use !first_line instead of got_at_least_one_head (removing
>> got_at_least_one_head in the process since it has no other purpose).
>
> I got the history wrong. It looks like this was always confused
> by the 'continue' cases. Unless I'm missing something subtle ---
> thoughts?
I was a bit confused by the line
for (;; first_line = 0) {
at first, but the explanation of 'continue's make sense for this pattern.
However I'd rather prefer if we'd have
int first_line = 1;
for(;;) {
... // stuff with no continue here
if (len < 0)
die_initial_contact(!first_line);
first_line = 0;
... // here we may have some continues, but that doesn't matter
// w.r.t. first_line
}
^ permalink raw reply
* Re: [PATCH] connect: tighten check for unexpected early hang up (Re: [PATCH v3 2/2] connect: advertized capability is not a ref)
From: Junio C Hamano @ 2016-09-08 16:42 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jonathan Tan, git, spearce, sbeller, peff, Heiko Voigt
In-Reply-To: <20160908015040.GF25016@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Jonathan Nieder wrote:
>
>> Subject: connect: tighten check for unexpected early hang up
> [...]
>> @@ -131,7 +131,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>> PACKET_READ_GENTLE_ON_EOF |
>> PACKET_READ_CHOMP_NEWLINE);
>> if (len < 0)
>> - die_initial_contact(got_at_least_one_head);
>> + die_initial_contact(first_line);
>
> This should say !first_line.
>
> I'll add tests if the patch seems like a good idea.
I tried to write one-paragraph comment for the die_initial_contact()
function, like so:
+/*
+ * A remote end that is unwilling to talk to us would not give
+ * any response to us before hanging up. After seeing some
+ * response, we know the hang-up is unexpected.
+ */
+static void die_initial_contact(int saw_any_response)
but then I got stuck.
We may know that after seeing any response (not necessarily a ref,
but .have or shallow) the other end is willing to talk to us, but
the reverse is not necessarily true (it may be willing to talk to
us, but the network between us may have prevented it from doing so).
For that reason, the above comment is inappropriate for a function
that takes a bool and gives an "unexpected hung-up" or an
"unreachable, possible ACL or problems" message.
So my second attempt was to comment on the variable that keeps track
of the status of the conversation, which turned out to be better
(attached).
I think I fixed your "oops, the bool needs polarity flip". A test
may be a good idea, but I am not sure how you plan to produce a
failure after sending some response.
-- >8 --
From: Jonathan Nieder <jrnieder@gmail.com>
Date: Wed, 7 Sep 2016 18:45:55 -0700
Subject: [PATCH] connect: tighten check for unexpected early hang up
A server hanging up immediately to mark access being denied does not
send any .have refs, shallow lines, or anything else before hanging
up. If the server has sent anything, then the hangup is unexpected.
That is, if the server hangs up after a shallow line but before sending
any refs, then git should tell me so:
fatal: The remote end hung up upon initial contact
instead of suggesting an access control problem:
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
Noticed while examining this code. This case isn't likely to come up
in practice but tightening the check makes the code easier to read and
manipulate.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
connect.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/connect.c b/connect.c
index c53f3f1..067cf40 100644
--- a/connect.c
+++ b/connect.c
@@ -43,9 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)
return check_ref(ref->name, flags);
}
-static void die_initial_contact(int got_at_least_one_head)
+static void die_initial_contact(int unexpected)
{
- if (got_at_least_one_head)
+ if (unexpected)
die("The remote end hung up upon initial contact");
else
die("Could not read from remote repository.\n\n"
@@ -115,10 +115,17 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
struct sha1_array *shallow_points)
{
struct ref **orig_list = list;
- int got_at_least_one_head = 0;
+
+ /*
+ * A hang-up after seeing some response from the other end
+ * means that it is unexpected, as we know the other end is
+ * willing to talk to us. A hang-up before seeing any
+ * response does not necessarily mean an ACL problem, though.
+ */
+ int saw_response;
*list = NULL;
- for (;;) {
+ for (saw_response = 0; ; saw_response = 1) {
struct ref *ref;
struct object_id old_oid;
char *name;
@@ -131,7 +138,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
PACKET_READ_GENTLE_ON_EOF |
PACKET_READ_CHOMP_NEWLINE);
if (len < 0)
- die_initial_contact(got_at_least_one_head);
+ die_initial_contact(saw_response);
if (!len)
break;
@@ -171,7 +178,6 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
oidcpy(&ref->old_oid, &old_oid);
*list = ref;
list = &ref->next;
- got_at_least_one_head = 1;
}
annotate_refs_with_symref_info(*orig_list);
--
2.10.0-267-g7db2ae3
^ permalink raw reply related
* Re: git-gui, was Re: [PATCH v2 6/6] git-gui: Update Japanese information
From: Junio C Hamano @ 2016-09-08 16:48 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Satoshi Yasushima, Pat Thoyts, git, Jakub Narębski
In-Reply-To: <alpine.DEB.2.20.1609080857460.129229@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Wed, 7 Sep 2016, Junio C Hamano wrote:
>
>> Pat, we haven't heard from you for a long time.
>
> Indeed. There are a couple of git-gui patches in Git for Windows that have
> been contributed a long time ago and not been picked up.
>
> Maybe it is time to just accept git-gui patches directly into Git, after
> some other Tcl/Tk savvy people ACKed them?
Yes, that was my thinking, though I'd prefer to see somebodyto be
acting as a central point of contact so that I do not have to pay
any attention to the part of the system other than responding to a
pull request. The subsystem maintainer does not have to forever be
Pat, of course, and can be handed over to somebody else, but if that
is what is going to happen, I'd prefer to see a volunteer or two to
step up and Pat to bless them.
^ permalink raw reply
* Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers
From: Junio C Hamano @ 2016-09-08 16:57 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
In-Reply-To: <20160908082246.saf7vlw2xgjo7jvg@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Between the two options for regexec_buf(), I think you have convinced me
> that REG_STARTEND is better than just using compat/regex everywhere. I
> do think the fallback for platforms like musl should be "use
> compat/regex" and not doing an expensive copy (which in most cases is
> not even necessary).
I agree with you that it would be the best approach to build
regexec_buf() that unconditionally uses REG_STARTEND and tell people
without REG_STARTEND to use compat/regex instead of their platform
regex library.
The description in Makefile may want to be rephrased to clarify.
-# Define NO_REGEX if you have no or inferior regex support in your C library.
+# Define NO_REGEX if your C library lacks regex support with REG_STARTEND
+# feature.
The word "inferior" is not giving any useful information there.
^ permalink raw reply
* Re: [PATCH v3 2/3] Introduce a function to run regexec() on non-NUL-terminated buffers
From: Junio C Hamano @ 2016-09-08 17:03 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King
In-Reply-To: <94ee698b2736929d37640012a1b1735b134dd3d6.1473321437.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> We just introduced a test that demonstrates that our sloppy use of
> regexec() on a mmap()ed area can result in incorrect results or even
> hard crashes.
>
> So what we need to fix this is a function that calls regexec() on a
> length-delimited, rather than a NUL-terminated, string.
>
> Happily, there is an extension to regexec() introduced by the NetBSD
> project and present in all major regex implementation including
> Linux', MacOSX' and the one Git includes in compat/regex/: by using
> the (non-POSIX) REG_STARTEND flag, it is possible to tell the
> regexec() function that it should only look at the offsets between
> pmatch[0].rm_so and pmatch[0].rm_eo.
>
> That is exactly what we need.
Yes, that is good.
> Since support for REG_STARTEND is so widespread by now, let's just
> introduce a helper function that uses it, and fall back to allocating
> and constructing a NUL-terminated when REG_STARTEND is not available.
I do not think this fallback is good; we do ship a compat/ fallback
that does support REG_STARTEND and you'd want to use that. Not
having the copying fallback means you do not even have to worry
about the size+1 overflow and fix it with xmallocz() ;-)
^ permalink raw reply
* Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function
From: Junio C Hamano @ 2016-09-08 17:09 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King
In-Reply-To: <d0537819a3676fda6928e7ad3282aa71643f0755.1473321437.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> @@ -33,11 +32,8 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)
> * caller early.
> */
> return;
> - /* Yuck -- line ought to be "const char *"! */
> - hold = line[len];
> - line[len] = '\0';
> - data->hit = !regexec(data->regexp, line + 1, 1, ®match, 0);
> - line[len] = hold;
> + data->hit = !regexec_buf(data->regexp, line + 1, len - 1, 1,
> + ®match, 0);
This is an unexpected happy surprise. It really feels good to see
that "Yuck" line go.
> @@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,
> len--;
> }
>
> - line_buffer = xstrndup(line, len); /* make NUL terminated */
> -
> for (i = 0; i < regs->nr; i++) {
> struct ff_reg *reg = regs->array + i;
> - if (!regexec(®->re, line_buffer, 2, pmatch, 0)) {
> + if (!regexec_buf(®->re, line, len, 2, pmatch, 0)) {
So is this hunk. Removing unnecessary copying is a very good thing.
Please give these three patches a common prefix, e.g.
regex: -G<pattern> feeds a non NUL-terminated string to regexec() and fails
regex: add regexec_buf() that can work on a non NUL-terminated string
regex: use regexec_buf()
or something like that.
Also I agree with Peff that a test with an embedded NUL would be a
good thing.
This round is so close to perfect.
^ permalink raw reply
* Re: [PATCH v1 2/2] read-cache: make sure file handles are not inherited by child processes
From: Junio C Hamano @ 2016-09-08 17:37 UTC (permalink / raw)
To: Lars Schneider; +Cc: Eric Wong, Git Mailing List, tboegi, Johannes.Schindelin
In-Reply-To: <F33245FC-C53A-4977-8E72-68AF3D2BB8BB@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
>>> We probably should be using O_NOATIME for all O_RDONLY cases
>>> to get the last bit of performance out (especially since
>>> non-modern-Linux systems probably still lack relatime).
>>
>> No, please do not go there.
>>
>> The user can read from a file in a working tree using "less",
>> "grep", etc., and they all update the atime, so should "git grep".
>> We do not use atime ourselves on these files but we should let
>> outside tools rely on the validity of atime (e.g. "what are the
>> files that were looked at yesterday?").
>>
>> If you grep for noatime in our current codebase, you'd notice that
>> we use it only for files in objects/ hierarchy, and that makes very
>> good sense. These files are what we create for our _sole_ use and
>> no other tools can peek at them and expect to get any useful
>> information out of them (we hear from time to time that virus
>> scanners leaving open file descriptors on them causing trouble, but
>> that is an example of a useless access), and that makes a file in
>> objects/ hierarchy a fair game for noatime optimization.
>
> How do we deal with read-cache:ce_compare_data, though?
We are using open() in our current code in that codepath, without
NOATIME. We shouldn't start using git_open_noatime() merely for
convenience.
We would probably want to do a preliminary refactoring so that we
can say "we want only CLOEXEC and not NOATIME". Perhaps we would
want to make the existing one in sha1_file.c to something like:
int git_open_noatime(const char *name)
{
return git_open(name, GIT_OPEN_NOATIME);
}
and then add
#define GIT_OPEN_NOATIME 01
to cache.h and add
int git_open(const char *name, unsigned int flag)
{
static int open_noatime = O_NOATIME;
for (;;) {
int fd, open_flag;
open_flag = 0;
if (flag & GIT_OPEN_NOATIME)
open_flag |= open_noatime;
errno = 0;
fd = open(name, O_RDONLY | open_flag);
if (fd >= 0)
return fd;
if (errno == ENOENT || !open_flag)
return -1;
/* The failure may be due to additional flags */
if ((flag & GIT_OPEN_NOATIME) &&
(open_flag & O_NOATIME)) {
flag &= ~GIT_OPEN_NOATIME;
open_noatime = 0;
}
}
}
to wrapper.c in the first step, which is a "no-op refactoring" step.
Then add
#define GIT_OPEN_CLOEXEC 02
and update git_open(), perhaps like so:
int git_open(const char *name, unsigned int flag)
{
static int open_noatime = O_NOATIME;
static int open_cloexec = O_CLOEXEC;
for (;;) {
int fd, open_flag;
open_flag = 0;
if (flag & GIT_OPEN_NOATIME)
open_flag |= open_noatime;
if (flag & GIT_OPEN_CLOEXEC)
open_flag |= open_cloexec;
errno = 0;
fd = open(name, O_RDONLY | open_flag);
if (fd >= 0)
return fd;
if (errno == ENOENT || !open_flag)
return -1;
/* The failure may be due to additional flags */
if ((flag & GIT_OPEN_NOATIME) &&
(open_flag & O_NOATIME)) {
flag &= ~GIT_OPEN_NOATIME;
open_noatime = 0;
}
if ((flag & GIT_OPEN_CLOEXEC) &&
(open_flag & O_CLOEXEC)) {
flag &= ~GIT_OPEN_CLOEXEC;
open_cloexec = 0;
}
}
}
The retry logic is "if we were asked to do this flag, and if we did
pass that flag, then we know open() with that flag fails here, so we
won't waste time trying with it again", which came from the NOATIME
codepath we already have, but it may not match what we use CLOEXEC
for and may need to be adjusted. I didn't think that part of the
code through.
Then the ce codepath that reads from the working tree would use
git_open(ce->name, GIT_OPEN_CLOEXEC);
to obtain the file descriptor for reading, perhaps?
^ permalink raw reply
* Re: [PATCH 4/5] versioncmp: pass full tagnames to swap_prereleases()
From: Junio C Hamano @ 2016-09-08 17:49 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Jeff King, Leho Kraav, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20160907151251.30978-5-szeder@ira.uka.de>
SZEDER Gábor <szeder@ira.uka.de> writes:
> - * Note that we don't have to deal with the situation when both p1 and
> - * p2 start with the same suffix because the common part is already
> + * Note that we don't have to deal with the situation when both s1 and
> + * s2 contain the same suffix because the common part is already
> * consumed by the caller.
"The common part is already consumed" was relevant while the
function was fed p1 and p2, i.e. the first difference, but the whole
point of passing the original s1 and s2 with ofs is so that the
function can look behind ofs as necessary. Is "already consumed"
still correct (or relevant) with s/p/s/ you did to its calling
convention?
^ permalink raw reply
* Re: [ANNOUNCE] Git for Windows 2.10.0
From: Johannes Schindelin @ 2016-09-08 18:19 UTC (permalink / raw)
To: stefan.naewe; +Cc: git-for-windows, git
In-Reply-To: <3ed6f32f-6330-3453-1581-3a2d4008bbfa@atlas-elektronik.com>
[-- Attachment #1: Type: text/plain, Size: 522 bytes --]
Hi Stefan,
On Thu, 8 Sep 2016, stefan.naewe@atlas-elektronik.com wrote:
> Am 03.09.2016 um 15:17 schrieb Johannes Schindelin:
>
> > New Features
> >
> > • Comes with Git v2.10.0.
> > • The git rebase -i command was made faster by reimplementing large
> > parts in C.
>
> I finally had the chance to do a "bigger" rebase and what shall I say...
> F***k, has this thing become fast, or what!
>
> Thank you so much for doing this!!!!
I *love* that kind of feedback! *beams*
Ciao,
Dscho
^ 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