* Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files
From: Christian Couder @ 2016-10-27 12:14 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8BPe085Qu4GpJ3MLTPeZM_7pbVrX31enQrGQn0oyjSrdg@mail.gmail.com>
On Thu, Oct 27, 2016 at 12:25 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, Oct 25, 2016 at 5:43 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> static int write_shared_index(struct index_state *istate,
>>> @@ -2211,8 +2269,11 @@ static int write_shared_index(struct index_state *istate,
>>> }
>>> ret = rename_tempfile(&temporary_sharedindex,
>>> git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
>>> - if (!ret)
>>> + if (!ret) {
>>> hashcpy(si->base_sha1, si->base->sha1);
>>> + clean_shared_index_files(sha1_to_hex(si->base->sha1));
>>
>> This operation is technically garbage collection and should belong to
>> "git gc --auto", which is already called automatically in a few
>> places. Is it not called often enough that we need to do the cleaning
>> up right after a new shared index is created?
>
> Christian, if we assume to go with Junio's suggestion to disable
> split-index on temporary files, the only files left we have to take
> care of are index and index.lock. I believe pruning here in this code
> will have an advantage over in "git gc --auto" because when this is
> executed, we know we're holding index.lock, so nobody else is updating
> the index, it's race-free. All we need to do is peek in $GIT_DIR/index
> to see what shared index file it requires and keep it alive too, the
> remaining of shared index files can be deleted safely. We don't even
> need to fall back to mtime.
>
> git-gc just can't match this because while it's running, somebody else
> may be updating $GIT_DIR/index. Handling races would be a lot harder.
Yeah, I agree that if we disable split-index on temporary files and on
commands like "git read-tree --index-output=<file>" then it is the
right thing to do it in write_shared_index(). (In fact when I wrote
the previous RFC series I didn't think about those special cases, and
that was the reason why I did it like this. So I just need to go back
to the implementation that was in the previous RFC series.)
I am just still wondering if disabling split-index on temporary files
could not have a bad performance impact for some use cases, but I
guess we could always come back to problem again if that happens.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Jeff King @ 2016-10-27 10:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Torvalds, git, Lars Schneider, Eric Wong,
Johannes Schindelin
In-Reply-To: <xmqqd1imbymi.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 26, 2016 at 02:15:33PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Wed, Oct 26, 2016 at 10:52:41AM -0700, Junio C Hamano wrote:
> >
> >> > I actually wonder if it is worth carrying around the O_NOATIME hack at
> >> > all.
> >>
> >> Yes, I share the thought. We no longer have too many loose objects
> >> to matter.
> >>
> >> I do not mind flipping the order, but I'd prefer to cook the result
> >> even longer. I am tempted to suggest we take two step route:
> >>
> >> - ship 2.11 with the "atime has been there and we won't regress it"
> >> shape, while cooking the "cloexec is semantically more
> >> important" version in 'next' during the feature freeze
> >>
> >> - immediately after 2.11 merge it to 'master' for 2.12 to make sure
> >> there is no fallout.
> >
> > That sounds reasonable, though I'd consider jumping straight to "NOATIME
> > is not worth it; drop it" as the patch for post-2.11.
>
> That endgame is fine by me too. Thanks for a sanity-check.
So here's that endgame patch. My main concern with it was that there
might be non-Linux systems that could be affected. But when I dug into
it, I found that this code was never activated anywhere besides Linux in
the first place. So I really doubt this will have any negative impact at
all. I certainly don't mind cooking it until post-2.11, though.
+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.
-- >8 --
Subject: [PATCH] sha1_file: stop opening files with O_NOATIME
When we open object files, we try to do so with O_NOATIME.
This dates back to 144bde78e9 (Use O_NOATIME when opening
the sha1 files., 2005-04-23), which is an optimization to
avoid creating a bunch of dirty inodes when we're accessing
many objects. But a few things have changed since then:
1. In June 2005, git learned about packfiles, which means
we would do a lot fewer atime updates (rather than one
per object access, we'd generally get one per packfile).
2. In late 2006, Linux learned about "relatime", which is
generally the default on modern installs. So
performance around atimes updates is a non-issue there
these days.
All the world isn't Linux, but as it turns out, Linux
is the only platform to implement O_NOATIME in the
first place.
So it's very unlikely that this code is helping anybody
these days.
It's not a particularly large amount of code, but the
fallback-retry creates complexity. E.g., we do a similar
fallback for CLOEXEC; which one should take precedence, or
should we try all possible combinations? Dropping O_NOATIME
makes those questions go away.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_file.c | 15 +--------------
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 09045df1dc..6f02a57d8b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -27,14 +27,6 @@
#include "list.h"
#include "mergesort.h"
-#ifndef O_NOATIME
-#if defined(__linux__) && (defined(__i386__) || defined(__PPC__))
-#define O_NOATIME 01000000
-#else
-#define O_NOATIME 0
-#endif
-#endif
-
#define SZ_FMT PRIuMAX
static inline uintmax_t sz_fmt(size_t s) { return s; }
@@ -1561,7 +1553,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 | O_CLOEXEC;
+ static int sha1_file_open_flag = O_CLOEXEC;
for (;;) {
int fd;
@@ -1577,11 +1569,6 @@ int git_open(const char *name)
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.916.g0d2035c
^ permalink raw reply related
* Re: [PATCH] rebase: add --forget to cleanup rebase, leave HEAD untouched
From: Duy Nguyen @ 2016-10-27 10:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqq60ofcavd.fsf@gitster.mtv.corp.google.com>
On Wed, Oct 26, 2016 at 11:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> There are occasions when you decide to abort an in-progress rebase and
>> move on to do something else but you forget to do "git rebase --abort"
>> first. Or the rebase has been in progress for so long you forgot about
>> it. By the time you realize that (e.g. by starting another rebase)
>> it's already too late to retrace your steps. The solution is normally
>>
>> rm -r .git/<some rebase dir>
>>
>> and continue with your life. But there could be two different
>> directories for <some rebase dir> (and it obviously requires some
>> knowledge of how rebase works), and the ".git" part could be much
>> longer if you are not at top-dir, or in a linked worktree. And
>> "rm -r" is very dangerous to do in .git, a mistake in there could
>> destroy object database or other important data.
>>
>> Provide "git rebase --forget" for this exact use case.
>
> Two and a half comments.
>
> - The title says "leave HEAD untouched". Are my working tree files
> and my index also safe from this operation, or is HEAD the only
> thing that is protected?
Everything is protected. I will rephrase the title a bit. The option
is basically a safe form of "rm -r .git/rebase-{apply,merge}".
> - I think I saw a variant of this gotcha for an unconcluded
> cherry-pick that was left behind, which the bash-prompt script
> did not notice but the next "git cherry-pick" did by complaining
> "you are in the middle" or something like that. Perhaps we would
> want to have a similarly sounding option to help that case, too,
> not in this patch but as another patch on the same theme?
That would be nice. I don't put lots of git info on my shell prompt
though, so it does not help me. And it's probably difficult to report
the right thing too. Sometimes in the middle of rebase I would switch
to another branch, look or do stuff, then "git checkout -" back. I
don't think we can make the prompt script clever enough to see my
intention.
> - Would it have helped if bash-prompt were in use? I am not saying
> that this patch becomes unnecessary if you use it; I am trying to
> see if it helps its users by reminding them what state they are
> in.
Since I don't use it because I want to keep shell prompt short and
light, it doe not help me. But it looks like git-prompt.sh does print
rebase in progress and others (only checked the code, didn't test it).
--
Duy
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Aaron Pelly @ 2016-10-27 10:33 UTC (permalink / raw)
Cc: git
In-Reply-To: <CALhvvbYqeWw+q=TPxTpve6JKoy0URYeWxj2vVOnzrA_g3Z3esA@mail.gmail.com>
On 27/10/16 21:19, Alexei Lozovsky wrote:
>> I'm thinking something like ". path/to/include/file" in an ignore file,
>> and/or creating .gitignore.d and/or allowing $HOME/.config/git/ignore
>> and $GIT_DIR/info/exclude to be directories. Or some sane and consistent
>> mixture of these things.
>
> I think the rc.d-like approach with directories is better as it
> does not add new magical filenames (what if I absolutely do need
> to name my directories ". path", with a space? :)
Yes. Another alternative I thought of was #include path/to/include/file.
That'd be backwards compatible, which is a good thing, but would involve
parsing comments, which obviously could start with #include. Or lines
like ^Include /path/file$ In the case of finding an invalid file,
passing it over and issuing a simple warning should surface any issues
with existing gitignores. Anyway, this conversation is why I bring it up
on the list.
Coming back to this, maybe :(include)/path/file might be more git-like
>> In the case of a directory the plan would be to add links to files
>> stored/sourced elsewhere. This does pose a precedence question which I
>> haven't thought about yet, but probably makes it too hard for the
>> limited value it brings.
>
> Now, if we consider the case of multiple .gitignore files, it
> could be unexpected and possibly annoying for negative patterns
> in one file to affect the patterns added by some other files.
That is a concern. It is non obvious; the worst kind of annoying.
> I would find it more conceptually simple to apply individual
> .gitignores one by one, as opposed to parsing them all and
> creating one giant exclusion rule. (In technical terms, this
> means keeping one struct exclude_list for each .gitignore,
> not merging them all into one single list.)
I agree. I haven't looked, but that sounds like touching significantly
more code though. Actually, thinking about it for 20 seconds more, it
shouldn't be too hard, should it?
> In this case there should be no precendence problems as applied
> gitignores only add new ignored files, without un-ignoring
> anything previously ignored by other files.
Again, I haven't looked yet, but there is still an issue of precedence
with other gitignore files in $HOME and the repo.
> However, if we allow textual inclusion, then it means that we
> can put a gitignore into our gitignore so that we can unignore
> while we ignore, which again brings us the question of whether
> it is actually needed and expected.
Gah! Yes. One way or the other.
>> I would like to know the desirability/practicality/stupidity of such a
>> feature as I believe it is within my skillset to implement it.
>
> However, I do not recall any precendent of git using rc.d-like
> configs.
Many things have adopted this technique recently. Well, the last 15
years. It is common, understood, and fairly simple. I see no issue with it.
> And some can argue that your goal can be achieved by
> generating the .gitignore by some external means and symlinking
> the result into .git/info/exclude, so this is not Git's problem
> and we should not be overcomplicating things with something as
> simple as a list exclude patterns. This line of argument also
> can be used to opposes any textual inclusion as well, because
> it can be expanded into 'why don't we add a Turing-complete
> programming language then to specify the patterns to ignore'.
I know. Another reason to bring the idea to the list. I sort of have
this attitude myself. My main objection to it is that I can't think of a
hook to automate it with.
But: What about some kind of :(exec) that executes a script and returns
a gitignore file? You write the script; you're responsible. And the
behaviour is obvious. I haven't thought that through. It just came to me
then, and might present security issues, but could greatly simplify things.
^ permalink raw reply
* Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files
From: Duy Nguyen @ 2016-10-27 10:25 UTC (permalink / raw)
To: Christian Couder
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8C4Qd76LSYxk9BJtN3n3Knv_RCDgBOO-ybkNHFZK61ahQ@mail.gmail.com>
On Tue, Oct 25, 2016 at 5:43 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> static int write_shared_index(struct index_state *istate,
>> @@ -2211,8 +2269,11 @@ static int write_shared_index(struct index_state *istate,
>> }
>> ret = rename_tempfile(&temporary_sharedindex,
>> git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
>> - if (!ret)
>> + if (!ret) {
>> hashcpy(si->base_sha1, si->base->sha1);
>> + clean_shared_index_files(sha1_to_hex(si->base->sha1));
>
> This operation is technically garbage collection and should belong to
> "git gc --auto", which is already called automatically in a few
> places. Is it not called often enough that we need to do the cleaning
> up right after a new shared index is created?
Christian, if we assume to go with Junio's suggestion to disable
split-index on temporary files, the only files left we have to take
care of are index and index.lock. I believe pruning here in this code
will have an advantage over in "git gc --auto" because when this is
executed, we know we're holding index.lock, so nobody else is updating
the index, it's race-free. All we need to do is peek in $GIT_DIR/index
to see what shared index file it requires and keep it alive too, the
remaining of shared index files can be deleted safely. We don't even
need to fall back to mtime.
git-gc just can't match this because while it's running, somebody else
may be updating $GIT_DIR/index. Handling races would be a lot harder.
--
Duy
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Aaron Pelly @ 2016-10-27 9:51 UTC (permalink / raw)
Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kasr6ubuOm01rLtdK7pfWqriZE1vioLyxAr2G2Hd1xGhg@mail.gmail.com>
On 27/10/16 15:22, Stefan Beller wrote:
>> The use case for this is where I did not write my own rules, but I want
>> to keep them updated. https://github.com/github/gitignore is a damn good
>> resource, but I want to pull it and include relevant bits project by
>> project and/or system wide. I don't want to have to update many projects
>> manually if that, or any other, repo changes.
>
> .git/info/exclude could be a (sym)link to an up to date version
> of the gitignore repo as a hack?
>
Using links isn't a bad idea, but you still end up at some stage
combining the contents of several files that already exist. Well, in my
example, anyway.
I accept that I'm being pretty trivial, and once it's set up there's
never any pressing need to change anything, but it still irks me.
Even with a linked .gitignore, or .git/info/exclude there will be
sections that are project, language, editor, machine, whatever,
specific. So I still need to copy stuff from one file to another by
hand. By allowing includes, I only have to have a link to each file
describing the data types for each component of the environment.
And they are community maintained, so I don't have to google every time
I try a new editor.
note to self: reply-to isn't the list.
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Jeff King @ 2016-10-27 10:50 UTC (permalink / raw)
To: Aaron Pelly; +Cc: git
In-Reply-To: <80919456-7563-2c16-ba23-ce4fcc2777de@pelly.co>
On Thu, Oct 27, 2016 at 01:22:43PM +1300, Aaron Pelly wrote:
> The use case for this is where I did not write my own rules, but I want
> to keep them updated. https://github.com/github/gitignore is a damn good
> resource, but I want to pull it and include relevant bits project by
> project and/or system wide. I don't want to have to update many projects
> manually if that, or any other, repo changes.
That seems like a reasonable thing to want.
> A very brief look at dir.c would indicate that a recursive call from
> add_excludes to itself when it parses some sort of include tag would do
> it within a file. I'm sure it'd be pretty straight forward to hook into
> something in dir.c to parse directories too.
>
> I'm thinking something like ". path/to/include/file" in an ignore file,
> and/or creating .gitignore.d and/or allowing $HOME/.config/git/ignore
> and $GIT_DIR/info/exclude to be directories. Or some sane and consistent
> mixture of these things.
I'd shy away from an actual include directive, as it raises a lot of
complications:
- as you noted, cycles in the include graph need to be detected and
broken
- we parse possibly-hostile .gitignore files from cloned repositories.
What happens when I include ask to include /etc/passwd? Probably
nothing, but there are setups where it might matter (e.g., something
like Travis that auto-builds untrusted repositories, and you could
potentially leak the contents of files via error messages). It's
nice to avoid the issue entirely.
- finding a backwards-compatible syntax
Whereas letting any of the user- or repo-level exclude files be a
directory, and simply reading all of the files inside, seems simple and
obvious. If you go that route, it probably makes sense to teach
gitattributes the same trick.
> In the case of a directory the plan would be to add links to files
> stored/sourced elsewhere. This does pose a precedence question which I
> haven't thought about yet, but probably makes it too hard for the
> limited value it brings.
I think the normal behavior in such "foo.d" directory is to just sort
the contents lexically and read them in order, as if they were all
concatenated together, and with no recursion. I.e., behave "as if" the
user had run "cat $dir/*".
That lets you handle precedence via the filenames (or symlink names). It
can't handle all cases (some items in "00foo" want precedence over "01bar"
and vice versa), but I don't think there's an easy solution. That's a
good sign that one or more of the files should be broken up.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #06; Mon, 24)
From: Johannes Schindelin @ 2016-10-27 15:47 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Stefan Beller, Junio C Hamano, git@vger.kernel.org
In-Reply-To: <23f15627-4964-7ba9-45c3-899366d621bd@kdbg.org>
Hi Hannes,
On Tue, 25 Oct 2016, Johannes Sixt wrote:
> Am 25.10.2016 um 20:13 schrieb Stefan Beller:
> > On Tue, Oct 25, 2016 at 10:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > > - the "off-by-one fix" part of sb/submodule-ignore-trailing-slash
> > > needs to be in the upcoming release but the "trailing /. in base
> > > should not affect the resolution of ../relative/path" part that
> > > is still under discussion can wait. Which means we'd need a few
> > > more !MINGW prerequisites in the tests by -rc0.
> > >[...]
> >
> > So maybe instead of adding !MINGW we rather want to apply
> > https://public-inbox.org/git/2908451e-4273-8826-8989-5572263cc283@kdbg.org/
> > instead for now?
>
> I was about to submit this very patch again, and only then saw your message.
> So, yes, that's what I propose, too.
>
> Dscho, does this patch fix the test failures that you observed, too?
> Unfortunately, it goes against our endeavor to reduce subshells.
I am fine with the patch, even if I did not have a chance to test it yet
(ran out of time today).
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] reset: --unmerge
From: Andreas Schwab @ 2016-10-27 16:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, Git Mailing List
In-Reply-To: <xmqqmvhsc8kn.fsf@gitster.mtv.corp.google.com>
On Okt 25 2016, Junio C Hamano <gitster@pobox.com> wrote:
> Somebody with a bright idea decided that vc-git-resolve-conflicts
> variable should be on by default in Emacs 25.1 X-<
This is consistent with the behaviour of the other VC backends, where it
isn't even customizable.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files
From: Junio C Hamano @ 2016-10-27 16:13 UTC (permalink / raw)
To: Duy Nguyen
Cc: Christian Couder, Git Mailing List,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8BPe085Qu4GpJ3MLTPeZM_7pbVrX31enQrGQn0oyjSrdg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> Christian, if we assume to go with Junio's suggestion to disable
> split-index on temporary files, the only files left we have to take
> care of are index and index.lock. I believe pruning here in this code
> will have an advantage over in "git gc --auto" because when this is
> executed, we know we're holding index.lock, so nobody else is updating
> the index, it's race-free.
>
> All we need to do is peek in $GIT_DIR/index
> to see what shared index file it requires and keep it alive too, the
> remaining of shared index files can be deleted safely. We don't even
> need to fall back to mtime.
Yes, that exactly was why I wondered if we can afford to limit
splitting only to the primary index, because it makes things a
lot simpler.
But I suspect that temporary index is where split-index shines most,
e.g. while creating a partial commit. The mechanism penalizes the
read performance by making the format more complex in order to favor
the write performance, which is very much suited for temporary one
that is read only once after it is written before it gets discarded
(on the other hand, splitting the primary index will penalize reads
that happen a lot more than writes).
While I still find it attractive at the conceptual level to limit
splitting only to the primary index for the resulting simplicity,
I doubt it is a good way to go, as I meant to say in
<xmqqeg33ccjj.fsf@gitster.mtv.corp.google.com>
> git-gc just can't match this because while it's running, somebody else
> may be updating $GIT_DIR/index. Handling races would be a lot harder.
It could attempt to take a lock on the primary index while it runs,
and refrain to do anything if it can't take the lock ("gc --auto"
may want to silently retry), and then the race is no longer an
issue, no?
^ permalink raw reply
* Re: [PATCH] reset: --unmerge
From: Junio C Hamano @ 2016-10-27 16:23 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Duy Nguyen, Git Mailing List
In-Reply-To: <87d1il3hhs.fsf@linux-m68k.org>
Andreas Schwab <schwab@linux-m68k.org> writes:
> On Okt 25 2016, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Somebody with a bright idea decided that vc-git-resolve-conflicts
>> variable should be on by default in Emacs 25.1 X-<
>
> This is consistent with the behaviour of the other VC backends, where it
> isn't even customizable.
The problem I had was "M-x save-buffer" (after resolving the
conflicts in it manually) running "git add" on the resulting file,
robbing from "git diff" an opportunity to help the user to see how
the result looks relative to both branches.
Do you mean that VC mode broke the same feature equally other SCMs,
too? Do other SCM supported by VC backends take advantage of
unmerged stages in the index (until you say "$scm add") by allowing
you to verify the combined diff with "$scm diff"?
^ permalink raw reply
* Re: [PATCH v15 01/27] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
From: Junio C Hamano @ 2016-10-27 16:59 UTC (permalink / raw)
To: git
Cc: Pranit Bauva, Christian Couder, Matthieu Moy, Alex Henrie,
Antoine Delaite
In-Reply-To: <01020157c38b19e0-81123fa5-5d9d-4f64-8f1b-ff336e83ebe4-000000@eu-west-1.amazonses.com>
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?
Thanks.
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Stefan Beller @ 2016-10-27 17:01 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org,
Simon Ruderich, Jeff King
In-Reply-To: <xmqqd1im8foi.fsf@gitster.mtv.corp.google.com>
> Johannes Sixt <j6t@kdbg.org> writes:
> This is the pessimization that I am talking about. I would not mind at all if
> it were only for the attribute subsystem, but the proposed patch would
> pessimize *all* uses of pthread_mutex_lock.
It would only pessimize *uninitialized* mutexes? For initialized mutexes
the added burden is super cheap (one additional condition).
The positive aspect of this way the patch proposes would be that any
future contributor not knowing the details of how to do mutexes right
on Windows, would not totally break the whole system, i.e. this seems
to be more maintainable in the future as it reduces the friction between
pthreads mutexes and the way we can do things in Git in a platform
independent way
On Wed, Oct 26, 2016 at 11:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Lazy on-demand initialization as needed, perhaps? The on-demand
>> initialization mechanism may become no-op on some platforms that can
>> do static initialization.
>
> Ah, I think I misunderstood your "please rewrite". Did you mean to
> add "void attr_start(void)" helper function to attr.c that does
> series of pthread_mutex_init() calls as needed?
Well one init for now.
> That function can
> be called from main() of platforms that cannot statically initialize
> mutices,
By main you mean the main() in common-main.c or cmd_main in git.c ?
Those both look like the wrong place. Of course it would work adding it
there, but it smells like a maintenance nightmare.
And then we would modify the attr_start command depending on the
platform, i.e.
#ifdef WIN32
void attr_start(void)
{
pthread_mutex_init(..);
}
#else
void attr_start(void)
{
/* nothing as it is statically init'd */
}
#endif
> while on other platforms it can be a no-op as long as the
> variables are statically initialized? If so, that would not pessimize
> any platform, I would think.
I would think this pessimizes ALL platforms from a maintenance perspective
(but what do I know about maintaining stuff as an eager young contributor ;)
So I am willing to go that route, though I do not quite understand where exactly
you'd expect me to put the initializer as all places I can think of
are not the right
place.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v2 0/2] Stream and fast search
From: Junio C Hamano @ 2016-10-27 17:02 UTC (permalink / raw)
To: git; +Cc: tboegi, Lars Schneider, Duy Nguyen
In-Reply-To: <20161012134724.28287-1-tboegi@web.de>
Cc'ed those who touched convert.c or read-cache.c in our relatively
recent past with a change that affects the eol conversion codepath.
Does any of you (and others on the list) have time and inclination
to review this series?
Thanks.
^ permalink raw reply
* Re: [PATCH v4 00/14] Mark strings in Perl scripts for translation
From: Junio C Hamano @ 2016-10-27 17:08 UTC (permalink / raw)
To: git
Cc: Vasco Almeida, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, Jakub Narębski, David Aguilar
In-Reply-To: <20161010125449.7929-1-vascomalmeida@sapo.pt>
> Vasco Almeida (14):
> i18n: add--interactive: mark strings for translation
> i18n: add--interactive: mark simple here-documents for translation
> i18n: add--interactive: mark strings with interpolation for
> translation
> i18n: clean.c: match string with git-add--interactive.perl
> i18n: add--interactive: mark plural strings
> i18n: add--interactive: mark patch prompt for translation
> i18n: add--interactive: i18n of help_patch_cmd
> i18n: add--interactive: mark edit_hunk_manually message for
> translation
> i18n: add--interactive: remove %patch_modes entries
> i18n: add--interactive: mark status words for translation
> i18n: send-email: mark strings for translation
> i18n: send-email: mark warnings and errors for translation
> i18n: send-email: mark string with interpolation for translation
> i18n: difftool: mark warnings for translation
It seems that nobody has anything to say on this series, either
positive or negative?
The topic has been in "Waiting for review" pile but I'll mark it as
"Will merge to 'next'" unless I hear otherwise within a few days
(positives may make it faster, negatives may require a reroll).
Thanks.
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Jeff King @ 2016-10-27 17:11 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Johannes Sixt, Johannes Schindelin,
git@vger.kernel.org, Simon Ruderich
In-Reply-To: <CAGZ79kbP3pgPHgv-x1Q-Q1QwmXc=gOyxWhXh2SngO8WSZc3PFA@mail.gmail.com>
On Thu, Oct 27, 2016 at 10:01:02AM -0700, Stefan Beller wrote:
> > That function can
> > be called from main() of platforms that cannot statically initialize
> > mutices,
>
> By main you mean the main() in common-main.c or cmd_main in git.c ?
Any setup or initialization for library functions needs to go in main()
of common-main.c. Otherwise, only builtins get it, and external
programs are left to segfault (or whatever).
> Those both look like the wrong place. Of course it would work adding it
> there, but it smells like a maintenance nightmare.
I agree it's ugly, but I suspect it would work OK in practice.
We don't have that many mutexes and initializing them is cheap.
Languages like C++ have non-constant initializers, and the compiler
takes care of running the startup code before main() begins. There's no
portable way to do that in C, but our cmd_main() is roughly the same
thing.
I still prefer the lazy initialization if it can work without incurring
measurable overhead. That's the normal way to do things in C; the only
complication here is the thread safety.
-Peff
^ permalink raw reply
* [PATCH] git-compat-util: move content inside ifdef/endif guards
From: Jeff King @ 2016-10-27 17:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Commit 3f2e2297b9 (add an extra level of indirection to
main(), 2016-07-01) added a declaration to git-compat-util.h,
but it was accidentally placed after the final #endif that
guards against multiple inclusions.
This doesn't have any actual impact on the code, since it's
not incorrect to repeat a function declaration in C. But
it's a bad habit, and makes it more likely for somebody else
to make the same mistake. It also defeats gcc's optimization
to avoid opening header files whose contents are completely
guarded.
Signed-off-by: Jeff King <peff@peff.net>
---
I just happened to notice this today while doing something unrelated in
the file.
git-compat-util.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 91e366d1dd..771ea29f31 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1042,6 +1042,6 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
#define getc_unlocked(fh) getc(fh)
#endif
-#endif
-
extern int cmd_main(int, const char **);
+
+#endif
--
2.10.1.916.g0d2035c
^ permalink raw reply related
* Re: [PATCH] git-compat-util: move content inside ifdef/endif guards
From: Junio C Hamano @ 2016-10-27 17:37 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161027173029.bu233oiekpfoh6lw@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Commit 3f2e2297b9 (add an extra level of indirection to
> main(), 2016-07-01) added a declaration to git-compat-util.h,
> but it was accidentally placed after the final #endif that
> guards against multiple inclusions.
>
> This doesn't have any actual impact on the code, since it's
> not incorrect to repeat a function declaration in C. But
> it's a bad habit, and makes it more likely for somebody else
> to make the same mistake. It also defeats gcc's optimization
> to avoid opening header files whose contents are completely
> guarded.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I just happened to notice this today while doing something unrelated in
> the file.
Thanks.
>
> git-compat-util.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 91e366d1dd..771ea29f31 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1042,6 +1042,6 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
> #define getc_unlocked(fh) getc(fh)
> #endif
>
> -#endif
> -
> extern int cmd_main(int, const char **);
> +
> +#endif
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Johannes Sixt @ 2016-10-27 18:22 UTC (permalink / raw)
To: Stefan Beller, Junio C Hamano
Cc: Johannes Schindelin, git@vger.kernel.org, Simon Ruderich,
Jeff King
In-Reply-To: <CAGZ79kbP3pgPHgv-x1Q-Q1QwmXc=gOyxWhXh2SngO8WSZc3PFA@mail.gmail.com>
Am 27.10.2016 um 19:01 schrieb Stefan Beller:
>> Johannes Sixt <j6t@kdbg.org> writes:
>> This is the pessimization that I am talking about. I would not mind at all if
>> it were only for the attribute subsystem, but the proposed patch would
>> pessimize *all* uses of pthread_mutex_lock.
>
> It would only pessimize *uninitialized* mutexes? For initialized mutexes
> the added burden is super cheap (one additional condition).
It is not possible to mark a mutex uninitialized on Windows without an
extra piece of data. A solution would become quite complicated quite
quickly, and at the cost of additional operations that are in the same
ballpark as an uncontended mutex. I'm not enthused.
> The positive aspect of this way the patch proposes would be that any
> future contributor not knowing the details of how to do mutexes right
> on Windows, would not totally break the whole system, i.e. this seems
> to be more maintainable in the future as it reduces the friction between
> pthreads mutexes and the way we can do things in Git in a platform
> independent way
This is a non-argument. Coders have to know their tools.
-- Hannes
^ permalink raw reply
* Re: [PATCH 32/36] pathspec: allow querying for attributes
From: Junio C Hamano @ 2016-10-27 18:29 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, pclouds
In-Reply-To: <20161022233225.8883-33-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> The pathspec mechanism is extended via the new
> ":(attr:eol=input)pattern/to/match" syntax to filter paths so that it
> requires paths to not just match the given pattern but also have the
> specified attrs attached for them to be chosen.
I was looking at preload-index.c and its history again, because I
wanted to ensure that Lars's process filter stuff introduces no
unexpected interactions with getting multi-threaded, similar to the
problem we had in the earlier incarnations of this step, which we
worked around with <xmqqshwvvaxq.fsf@gitster.mtv.corp.google.com>
"pathspec: disable preload-index when attribute pathspec magic is in
use".
I think that Lars's series is safe, because the only thing among
what preload-index.c::preload_thread() does that can go outside the
simple stat data and pattern matching with the pathname is the
racy-git check in ie_match_stat(), and the object store access in
that call was explicitly disabled by 7c4ea599b0 ("Fix index
preloading for racy dirty case", 2008-11-17) long time ago.
By passing CE_MATCH_RACY_IS_DIRTY option to the call, this caller
effectively says "A cache entry whose stat data matches may be
actually dirty when the timestamp is racy, in which case we usually
compare data to determine if it really is clean, but it is OK to err
on the safe and lazy side by declaring it dirty and not marking it
up-to-date while we are preloading. Do not bother to go to the
object store".
The reason why I am bringing this up in this discussion thread on
this patch is because I wonder if we would benefit by a similar
"let's not do too involved things and be cheap by erring on the safe
and lazy side" strategy in the call to ce_path_match() call made in
this function to avoid making calls to the attr subsystem.
In other words, would it help the system by either simplifying the
processing done or reducing the cycle spent in preload_thread() if
we could tell ce_path_match() "A pathspec we are checking may
require not just the pattern to match but also attributes given to
the path to satisfy the criteria, but for the purpose of preloading,
pretend that the attribute satisfies the match criteria" (or
"pretend that it does not match"), thereby not having to make any
call into the attribute subsystem at all from this codepath?
The strategy this round takes to make it unnecessary to punt
preloading (i.e. dropping "pathspec: disable preload-index when
attribute pathspec magic is in use" patch the old series had) is to
make the attribute subsystem thread-safe. But that thread-safety in
the initial round is based on a single Big Attribute Lock, so it may
turn out that the end result performs better for this codepath if we
did not to make any call into the attribute subsystem.
I am probably being two step ahead of ourselves by saying the above,
which is just something to keep in mind as a possible solution if
performance in this preload codepath becomes an issue when the
pathspec has attributes match specified.
^ permalink raw reply
* Re: [PATCH] Update git rebase documentation to clarify HEAD behavior
From: Philip Oakley @ 2016-10-27 14:06 UTC (permalink / raw)
To: Junio C Hamano, Cody Sehl; +Cc: git
In-Reply-To: <xmqq8tta8f0x.fsf@gitster.mtv.corp.google.com>
From: "Junio C Hamano" <gitster@pobox.com>
> Cody Sehl <cody.sehl@gmail.com> writes:
>
>> The first few paragraphs in the git-rebase.txt documentation lay out the
>> steps git takes during a rebase:
>> 1. everything from `<upstream>..HEAD` is saved to a temporary area
>> 2. `HEAD` is set to `<upstream>`
>> 3. the changes held in the temporary area are applied one by one in order
>> on top of the new `HEAD`
>>
>> The second step was described using the phrase `The current branch is
>> reset to <upstream>`, which is true (because `HEAD` == current branch),
>> but not clear.
>> ---
>
> Please wrap your lines to reasonable lengths like 70 columns or so.
> Please sign off your patch.
>
>> Documentation/git-rebase.txt | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index de222c8..c47ca11 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -33,7 +33,7 @@ of commits that would be shown by `git log
>> <upstream>..HEAD`; or by
>> description on `--fork-point` below); or by `git log HEAD`, if the
>> `--root` option is specified.
>>
>> -The current branch is reset to <upstream>, or <newbase> if the
>> +HEAD is reset to <upstream>, or <newbase> if the
>> --onto option was supplied. This has the exact same effect as
>> `git reset --hard <upstream>` (or <newbase>). ORIG_HEAD is set
>> to point at the tip of the branch before the reset.
>
> This is describing an ancient behaviour before 6fd2f5e60d ("rebase:
> operate on a detached HEAD", 2007-11-08) in v1.5.4 timeframe. We
> apparently failed to update the description.
>
> This depends on the desired technical detail of the description, but
> a correct rewrite would be "HEAD is detached at <upstream>, or
> <newbase>, and ORIG_HEAD is set to point at the tip of the branch
> before this happens". Detaching the HEAD at <upstream> no longer
> has the same effect as "git reset --hard <upstream>" (or <newbase>),
> so that sentence must go. It was the primary point of the ancient
> change at 6fd2f5e60d after all.
>
> And then there is a new step (to be numbered 4. in your description
> in the proposed log message), which updates the tip of the branch to
> the resulting HEAD (after replaying all these changes) and check the
> branch out, which needs to be added. Perhaps after "one by one, in
> order." Oh, the mention of "reapplied to the current branch" also
> needs to be updated to "reapplied to the detached HEAD", too.
>
> On the other hand, if we do not aim for that deep level of technical
> correctness, but want to tell a white lie to make it easier to
> understand at the conceptual level to new readers who haven't
> grasped the detached HEAD, then the current description is fine. By
> bringing up "HEAD", you seem to be aiming for techincal correctness
> (which I tend to agree is a good direction to go in this part of the
> documentation), so the existing text needs a bit more work than your
> patch to be brought to the modern world.
>
A third option is to start with a short "Conceptually, .." description which
gives that summary overview, and then have a few "In detail, .." paragraphs
that cover the ORIG_HEAD and Detatched HEAD, etc., that users should at
least be aware of (so they know they can come back and read the gory details
when they need it;-)
--
Philip
^ permalink raw reply
* Re: [PATCH] Documentation/git-diff: document git diff with 3+ commits
From: Junio C Hamano @ 2016-10-27 18:40 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Jacob Keller
In-Reply-To: <e74d4d84-d848-00be-8ca5-75204cf47262@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> It did not exist, but even at that point in time, "git log A..B" listed
> only commits between the merge base and B, not those which are only in A
> and not in B. Whereas "git diff A B" shows the differences between the
> endpoints A and B.
I think you are speaking with 20/20 hindsight at this point.
Do you genuinely think you would have thought that way when there
weren't any concept of "merge-base" or "git log A...B" known to you?
>>> @@ -12,6 +12,7 @@ SYNOPSIS
>>> 'git diff' [options] [<commit>] [--] [<path>...]
>>> 'git diff' [options] --cached [<commit>] [--] [<path>...]
>>> 'git diff' [options] <commit> <commit> [--] [<path>...]
>>> +'git diff' [options] <commit> <commit> <commit> [<commit>...]
>>
>> Made me wonder "is [<A>...] 0-or-more As or 1-or-more As?".
>
> 0-or-more, at least that's the way it is used in all lines here.
>
>> Don't we allow pathspecs in this case?
>
> Yes, the combinded diff mode kicks in only with no blobs (not: no
> pathspec, which I had misread) and N>=3 commits. Maybe I should update
> the code comments in builtin/diff.c to describe this, too.
I don't know about the code comments, but the above SYNOPSIS needs
an update, I would think. That is why I wondered if [<A>...] were
0-or-more or 1-or-moer.
You can replace
>>> 'git diff' [options] <commit> <commit> [--] [<path>...]
with
>>> 'git diff' [options] <commit> <commit> [<commit>...] [--] [<path>...]
without adding a new line to cover the new(ly discovered) case.
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Junio C Hamano @ 2016-10-27 18:49 UTC (permalink / raw)
To: Johannes Sixt
Cc: Stefan Beller, Johannes Schindelin, git@vger.kernel.org,
Simon Ruderich, Jeff King
In-Reply-To: <34c88c40-2088-fd74-5d26-56c0599b7eb9@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 27.10.2016 um 19:01 schrieb Stefan Beller:
> ...
> It is not possible to mark a mutex uninitialized on Windows without an
> extra piece of data. A solution would become quite complicated quite
> quickly, and at the cost of additional operations that are in the same
> ballpark as an uncontended mutex. I'm not enthused.
>
>> The positive aspect of this way the patch proposes would be that any
>> future contributor not knowing the details of how to do mutexes right
>> on Windows, would not totally break the whole system, i.e. this seems
>> to be more maintainable in the future as it reduces the friction between
>> pthreads mutexes and the way we can do things in Git in a platform
>> independent way
>
> This is a non-argument. Coders have to know their tools.
The codebase should strive to give coders a coherent abstraction
that can be implemented efficiently on platforms, so that coders do
not have to care too deeply about quirks that exist on individual
platforms.
It is OK to argue that the particular solution Stefan lifted from
somewhere (perhaps msdn article he cited???) does not qualify as
such an abstraction. But I do not agree with your "Coders have to
know" as a blanket statement.
^ permalink raw reply
* Re: [PATCH] compat: Allow static initializer for pthreads on Windows
From: Stefan Beller @ 2016-10-27 19:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org,
Simon Ruderich, Jeff King
In-Reply-To: <xmqqr3716301.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 27, 2016 at 11:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Am 27.10.2016 um 19:01 schrieb Stefan Beller:
>> ...
>> It is not possible to mark a mutex uninitialized on Windows without an
>> extra piece of data. A solution would become quite complicated quite
>> quickly, and at the cost of additional operations that are in the same
>> ballpark as an uncontended mutex. I'm not enthused.
>>
>>> The positive aspect of this way the patch proposes would be that any
>>> future contributor not knowing the details of how to do mutexes right
>>> on Windows, would not totally break the whole system, i.e. this seems
>>> to be more maintainable in the future as it reduces the friction between
>>> pthreads mutexes and the way we can do things in Git in a platform
>>> independent way
>>
>> This is a non-argument. Coders have to know their tools.
Windows is not my tool.
>
> The codebase should strive to give coders a coherent abstraction
> that can be implemented efficiently on platforms, so that coders do
> not have to care too deeply about quirks that exist on individual
> platforms.
Currently working as a coder I care about "submodules, that work on
linux." I do not care about Windows in the big picture. I am however
willing to go an extra step to not break Windows. However that requires
a little bit of effort from both me and you:
* I need to be aware of what I cannot do with "not-my-tools". (So please
somebody tell me, also in the future when I break obscure platforms. Mind
that I don't equate obscure with not widely used or in any other way negative.
It's just that people working on linux find some quirks on Windows
not "obvious")
* the workaround should not be too time consuming in the bigger picture,
which is why I propose to make the API a bit clearer by emulating posix
mutexes harder. (From a Windows POV this might sound like making it
more obscure because posix mutexes itself are obscure.)
>
> It is OK to argue that the particular solution Stefan lifted from
> somewhere (perhaps msdn article he cited???)
A stack overflow article that I found with my search engine of choice, because
I could not believe that Windows cannot have statically initialized mutexes.
> does not qualify as
> such an abstraction.
The implementation under discussion (well we did not discuss the
implementation a
whole lot yet) may even contain an error as the first memory barrier
needs to be in front
of the first condition.
> But I do not agree with your "Coders have to
> know" as a blanket statement.
Well I do to some extent, I just disagree what my tools are.
^ permalink raw reply
* Re: [PATCH] git-svn: do not reuse caches memoized for a different architecture
From: Junio C Hamano @ 2016-10-27 19:44 UTC (permalink / raw)
To: Eric Wong; +Cc: Johannes Schindelin, git, Gavin Lambert
In-Reply-To: <20161025212357.GA8683@starla>
Eric Wong <e@80x24.org> writes:
> Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
>> +++ b/perl/Git/SVN.pm
>> @@ -1658,6 +1658,11 @@ sub tie_for_persistent_memoization {
>> if ($memo_backend > 0) {
>> tie %$hash => 'Git::SVN::Memoize::YAML', "$path.yaml";
>> } else {
>> + # first verify that any existing file can actually be loaded
>> + # (it may have been saved by an incompatible version)
>> + if (-e "$path.db") {
>> + unlink "$path.db" unless eval { retrieve("$path.db"); 1 };
>> + }
>
> That retrieve() call is unlikely to work without "use Storable"
> to import it into the current package.
>
> I also favor setting "$path.db" once to detect typos and avoid
> going over 80 columns. Additionally, having error-checking for
> unlink might be useful.
>
> So perhaps squashing this on top:
>
> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
> index 025c894..b3c1460 100644
> --- a/perl/Git/SVN.pm
> +++ b/perl/Git/SVN.pm
> @@ -1660,10 +1660,15 @@ sub tie_for_persistent_memoization {
> } else {
> # first verify that any existing file can actually be loaded
> # (it may have been saved by an incompatible version)
> - if (-e "$path.db") {
> - unlink "$path.db" unless eval { retrieve("$path.db"); 1 };
> + my $db = "$path.db";
> + if (-e $db) {
> + use Storable qw(retrieve);
> +
> + if (!eval { retrieve($db); 1 }) {
> + unlink $db or die "unlink $db failed: $!";
> + }
> }
> - tie %$hash => 'Memoize::Storable', "$path.db", 'nstore';
> + tie %$hash => 'Memoize::Storable', $db, 'nstore';
> }
> }
>
>
> Thoughts? Thanks.
Just peeking from the sideline, but the your squash looks like an
improvement to me.
Hopefully the final version after your interaction with Dscho can
come to me via another "pull this now"?
Thanks.
^ 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