* [RFC/WIP PATCH] Use config value rebase.editor as editor when starting git rebase -i
From: Peter Oberndorfer @ 2011-10-11 17:56 UTC (permalink / raw)
To: git
If rebase.editor is not set interactive rebase falls back
to the default editor.
With this change is it possible to have a separate
(possibly graphical) editor that helps the user
during a interactive rebase.
Using $GIT_EDITOR or core.editor config var for this is not possible
since it is also used to start the commit message editor for reword action.
Signed-off-by: Peter Oberndorfer <kumbayo84@arcor.de>
---
Hi,
i wrote a (not yet released) git rebase -i helper that allows to order commits
by drag/drop and allows to select the action from a combo box.
(written in Qt)
See http://i55.tinypic.com/2d94gg0.jpg for how it currently looks like. :-)
No more typos, no more lost commit by cutting without pasting...
To integrate this properly into git i need something like this patch.
Open questions/problems:
* GIT_EDITOR env var is not honored anymore after this change.
Help from somebody with more bash knowledge is highly appreciated!
* Should git_rebase_editor be in git-rebase--interactive.sh instead
(since it is only used there)
* How should the config be called?
It is not directly used during rebase, only during rebase -i
that might not be fully clear from the config name.
* Better config.txt description?
Thanks,
Greetings Peter
PS: My tool will hopefully be released soon.
Cleanup code, test(lin/ win), write some doc (how to use with git),
choose name :-), choose license...
Documentation/config.txt | 6 ++++++
git-rebase--interactive.sh | 2 +-
git-sh-setup.sh | 13 +++++++++++++
3 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 03296b7..1d9ae79 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1591,6 +1591,12 @@ rebase.stat::
Whether to show a diffstat of what changed upstream since the last
rebase. False by default.
+rebase.editor::
+ Text editor used by git rebase -i for editing the rebasse todo file.
+ The value is meant to be interpreted by the shell when it is used.
+ When not configured the default commit message editor is used instead.
+ See "core.editor"
+
rebase.autosquash::
If set to true enable '--autosquash' option by default.
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 94f36c2..0f3b569 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -832,7 +832,7 @@ has_action "$todo" ||
die_abort "Nothing to do"
cp "$todo" "$todo".backup
-git_editor "$todo" ||
+git_rebase_editor "$todo" ||
die_abort "Could not execute editor"
has_action "$todo" ||
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 8e427da..303fb96 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -113,6 +113,19 @@ git_editor() {
eval "$GIT_EDITOR" '"$@"'
}
+git_rebase_editor() {
+ if test -z "${GIT_REBASEI_EDITOR:+set}"
+ then
+ GIT_REBASEI_EDITOR="$(git config rebase.editor)"
+ if [ -z "$GIT_REBASEI_EDITOR" ]
+ then
+ GIT_REBASEI_EDITOR="$(git var GIT_EDITOR)" || return $?
+ fi
+ fi
+
+ eval "$GIT_REBASEI_EDITOR" '"$@"'
+}
+
git_pager() {
if test -t 1
then
--
1.7.7.215.gfef80
^ permalink raw reply related
* Re: [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references
From: Junio C Hamano @ 2011-10-11 17:53 UTC (permalink / raw)
To: Jeff King
Cc: Michael Haggerty, git, cmn, A Large Angry SCM, Daniel Barkalow,
Sverre Rabbelier
In-Reply-To: <20111011161652.GA15629@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I wonder if the right solution is for us to be more picky about what can
> be found in $GIT_DIR. Maybe matching all-uppercase, or starting with
> "refs/", which I think would match existing convention?
I think we've discussed tightening it a few years ago already.
HEAD, MERGE_HEAD, FETCH_HEAD, etc. all are "^[_A-Z]*$" and it may even be
a good idea to insist "^[_A-Z]*HEAD$" or even "^([A-Z][A-Z]*_)?HEAD$".
^ permalink raw reply
* Re: [PATCHv5/RFC 1/6] Documentation: Preparation for gitweb manpages
From: Junio C Hamano @ 2011-10-11 17:49 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, git, Drew Northup, Jonathan Nieder
In-Reply-To: <201110111739.49967.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> On Tue, 11 Oct 2011, Junio C Hamano wrote:
>
>> I probably do not have time to look into this, but just FYI my trial merge
>> to 'pu' of this topic is failing like this:
>>
>> asciidoc: ERROR: gitweb.conf.txt: line 484: illegal style name: Default: ()
>> asciidoc: ERROR: gitweb.conf.txt: line 494: illegal style name: Default: 300
>
> Damn, I thought I have fixed that. This probably depends on AsciiDoc
> version ("make doc" on 'master' generates a few _warnings_ for me related
> to similar situation), but the problem is with
>
> [Default: <value>]
>
> that was copied from gitweb/README. But [<sth>] is an attribute list
> (style name in simplest form), used more often in newer AsciiDoc.
>
> So either we have to escape '[' and ']', i.e. use {startsb} and {endsb},
> which would reduce human-friendliness, or move to different way of marking
> default values, e.g. _italic_.
>
> What do you think?
What do the other documents in the directory this file lives say? I think
we explain what the variables does, and add "defaults to false" or
somesuch in the text, without any funny mark-up.
^ permalink raw reply
* Re: Re: [PATCH 6/6] Retain caches of submodule refs
From: Heiko Voigt @ 2011-10-11 17:41 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, git, Jeff King, Drew Northup, Jakub Narebski
In-Reply-To: <4E93C232.9090400@alum.mit.edu>
Hi,
On Tue, Oct 11, 2011 at 06:12:34AM +0200, Michael Haggerty wrote:
> On 10/10/2011 09:53 PM, Heiko Voigt wrote:
> > On Sun, Oct 09, 2011 at 01:12:20PM +0200, Michael Haggerty wrote:
> > Since the setup_revision() api can currently not be used to safely
> > iterate twice over the same submodule my patch
> >
> > allow multiple calls to submodule merge search for the same path
> >
> > rewrites the search into using a child process. AFAIK the submodule ref
> > iteration api would then even be unused.
>
> If your patch is accepted, then we should check whether anything should
> be ripped out.
I would rather like to extend the setup_revision() api so that we can
iterate multiple times. Additionally I have a feeling that this API
might be useful for further extensions of the recursive-push support of
submodules which is currently under development.
So in summary: I would like to wait with ripping anything out until we
get to a final state with submodule support.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH 2/2] submodule::module_clone(): silence die() message from module_name()
From: Jens Lehmann @ 2011-10-11 17:38 UTC (permalink / raw)
To: Tay Ray Chuan; +Cc: git, Junio C Hamano, David Aguilar
In-Reply-To: <CALUzUxo6YEEpL_MhT=O9sJSUdwcpKBpeM2O8GkbbyxvqmWCFLQ@mail.gmail.com>
Am 11.10.2011 10:44, schrieb Tay Ray Chuan:
> On Tue, Oct 11, 2011 at 3:34 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>> When cmd_foreach() passes an empty "name" variable to the
>> spawned command that might still work (and even make sense), but using the
>> empty name in cmd_sync() to access the config is looking like an error to
>> me. It might make sense to add an "|| exit" at least to the callsite in
>> cmd_sync(). Or am I missing something here?
>
> Cc-ed David, who authored cmd_sync().
>
> David, what do you think of Jens' analysis?
>
> In the meantime, I'll probably reword the second paragraph to say that
> future work will be needed to analyze non- || exit callsites.
Yeah, me too thinks the missing "|| exit" should be subject of another
patch.
^ permalink raw reply
* Re: Submodule confusion when checking out branch
From: Jens Lehmann @ 2011-10-11 17:35 UTC (permalink / raw)
To: Howard Miller; +Cc: git
In-Reply-To: <CAHVO_90rHbqqUx6HCh7tLWO9aP9PyPMpYnZGszCDB2bfNzUXAQ@mail.gmail.com>
Am 11.10.2011 11:38, schrieb Howard Miller:
> I added a submodule to my project like this (all from the root of the project)
>
> git submodule add git@..... path/to/submodule
> git submodule init
> git add path/to/submodule
> git commit -m 'I added a submodule!'
> git push
>
> All looks good and 'git status' reports 'nothing to commit'
>
> However, I now cannot change branches. On checkout, I get...
>
> "error: The following untracked working tree files would be
> overwritten by checkout:"
> (followed by a big list of all the files in the submodule)
>
> Where did I go wrong and what can I do to sort it?
Hmm, as I don't know for what checkout you see this problem (do you
switch from a branch containing the submodule to one that doesn't
have it or the other way round?) and assuming you had some files
committed in the directory where the submodule lives I can take a
guess what happened:
Could it be the case that you converted an existing directory into a
submodule and then get this error when you want to switch back to a
branch where this directory is still filled with the original files?
Then this is a known shortcoming of submodules at the moment. I have
experimental code to make Git work in that case but it is not ready
for inclusion yet.
^ permalink raw reply
* Re: [PATCH v2 0/7] Provide API to invalidate refs cache
From: Junio C Hamano @ 2011-10-11 17:26 UTC (permalink / raw)
To: Michael Haggerty
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Martin Fick, Christian Couder,
Christian Couder, Thomas Rast
In-Reply-To: <4E93D932.6020001@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 10/11/2011 02:02 AM, Junio C Hamano wrote:
> ...
>> I could rebase your series, but it always is more error prone to have
>> somebody who is not the original author rebase a series than the original
>> author build for the intended base tree from the beginning.
>
> I don't mind rebasing this little series on jp/get-ref-dir-unsorted.
> ...
> Rebasing 78 patches is going to be a morass of clerical work.
I do not think it is "clerical" in the first place.
Realistically, I expect that a 50+ patch series that touch fairly an
important part of the system to take 2 cycles and a half before it hits a
released version, judging from our recent experience with the recursive
merge fix-up series.
When we already have a patch that has been discussed well enough on the
list to fix somebody's real world problem, can we afford to block it and
give an exclusive write lock to part of the codebase for 2 cycles to your
series, or anybody's for that matter?
> Is there any alternative?
I think an alternative is not to hold on to a series before it gets so
large to make you feel adjusting to the needs to other changes in the
codebase is "clerical". Commit often and early while developing the
initial pass, re-read often and throughout the whole process looking for
things you regret you would have done in early in the series that you
didn't (aka "oops, here is a fixup for the thinko in the early patch in
the series), and clean-up early while preparing to publish. Reorder the
parts that you are more confident that they do not need to change to come
early in the series, and unleash these early parts when you reach certain
confidence level.
I think your iterate-refs series was an example of good execution. It made
the codeflow a lot clearer by reducing the special casing of the submodule
parameter. In your grand scheme of things (e.g. read only parts of the ref
namespace as needed) you might consider it a mere side effect, but the
series by itself was a good thing to have.
Sometimes you may feel that a part of your series when taken out of
context would not justify itself like the iterate-refs series did, until
later parts of the series start taking advantage of the change. But that
is what commit log messages are for: e.g. "this change to encapsulate
these global variables into a single structure does not make a difference
in the current codebase, but in a later patch this and that callers will
need additional pieces of information passed aruond in the callchain, and
will add new members to the structure".
> ... So maybe
> I brought this whole mess down on my own head :-(
No, it is not anybody's fault in particular. That's life and open source.
^ permalink raw reply
* "trying out" gitolite with minimum impact on a system
From: Sitaram Chamarty @ 2011-10-11 17:19 UTC (permalink / raw)
To: Git Mailing List
(This is kind of a record for me, 2 gitolite related emails to the git
ML in less than a month)
If you were in the position of vaguely thinking gitolite may be useful
to you but lacked the time to actually install it to try things out,
this may help.
----
Some folks want to be able to try gitolite's access control features,
play with the configuration language, and so on, but to actually
*install* it seems like a "commitment" of some kind.
Gitolite has always had the ability to be installed *entirely* within
one Unix userid. Plus, the homegrown test suite I've been using for a
while now has always simulated different "gitolite users" with a bit
of ~/.ssh/config magic.
So I just combined the two, and made the client also be on the same
Unix userid, resulting in this:
http://sitaramc.github.com/gitolite/t/README.html#_playing_with_gitolite
Briefly, it boils down to this: create a user, log in as that user,
clone gitolite, run *one* command. Done.
After that, entirely within that user, you have an admin user and six
normal users, to do with as you please. You emulate different users
simply by using a different username in the URL, like "git clone
u1:reponame" versus "git clone u2:reponame".
This gives you a chance to play with the myriad access control
features gitolite has.
When done, delete that user and your system is back where it was.
--
Sitaram
^ permalink raw reply
* Re: [PATCHv5/RFC 3/6] gitweb: Add manpage for gitweb (APPLICATION!!!)
From: Drew Northup @ 2011-10-11 16:56 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, git, Jonathan Nieder
In-Reply-To: <201110111620.12788.jnareb@gmail.com>
On Tue, 2011-10-11 at 16:20 +0200, Jakub Narebski wrote:
> On Tue, 11 Oct 2011, Drew Northup wrote:
> > On Tue, 2011-10-11 at 15:51 +0200, Jakub Narebski wrote:
> > > On Tue, 11 Oct 2011, Drew Northup wrote:
> >
> > > > This would be why I included a synopsis with my original submission. As
> > > > this was supposed to be a description of the configuration files of said
> > > > application it does not make much sense to put the executable in the
> > > > synopsis. Please forgive me for attempting to make sense!
> > >
> > > But this manpage is about _gitweb itself_, not about gitweb config file(s).
> > > Gitweb itself is application, though it is not runnable directly (yet).
> > >
> > > Web apps either don't use manpages as documentation, and those that do
> > > that I found (SVN::Web for example) include runnable server-starting script.
> >
> > Hmm, Couldn't tell from the mail header _which_ we were talking about in
> > this subthread. I'll have to read the _whole_ patch apparently next
> > time.
>
> I'm sorry. I guess better subjects would be:
>
> gitweb: Add gitweb.conf(5) manpage
> gitweb: Add gitweb(1) manpage
>
> instead of current
>
> gitweb: Add manpage for gitweb configuration files
> gitweb: Add manpage for gitweb
>
> Or perhaps:
>
> gitweb: Add manpage for gitweb configuration files
> gitweb: Add manpage for gitweb itself
Jakub,
For the sake of clarity I'd go with redundancy (but I've noticed that's
not always enormously popular around here)
gitweb: Add manpage for gitweb configuration files--gitweb.conf(5)
gitweb: Add manpage for gitweb itself--gitweb(1)
Perhaps the first one is too long, but I think that it leaves little
question as to what the patch contains. Otherwise, your first or final
pair of updated subjects would probably be clear enough.
Granted, just as soon as you fix that somebody else will manage to top
my misreading of the subject line ;-).
--
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
^ permalink raw reply
* Re: [PATCH v4] attr.c: respect core.ignorecase when matching attribute patterns
From: Junio C Hamano @ 2011-10-11 16:54 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, mhagger, Brandon Casey
In-Reply-To: <HBn1XX5GLcGG9WPqS0RC9Uscll_6Kbd741mHOR7uc_IFxdOpGSDGF7qEBPF66SbtO3keG4GcnkbtEvKDQ5D3bCDNiV9EPgEh-CMLKgbfFJcpVD5Gcb69-QoqTpHG_J9pDQG844LtNZU@cipher.nrlssc.navy.mil>
Brandon Casey <casey@nrlssc.navy.mil> writes:
> ... Currently, git builds the attr stack
> based on the path supplied by the user, so we don't have to do anything
> special (like use strcmp_icase) to handle the parts of that path that don't
> match the filesystem with respect to case. If git instead built the attr
> stack by scanning the repository, then the paths in the origin field would
> not necessarily match the paths supplied by the user.
I find this description somewhat misleading. "check-attr" at the plumbing
level does take full path from the end user, but a common thing Git does
is to ask the system to learn the prefix to the current directory with
getcwd(3) append what fill_directory() enumerates as matching a pathspec
given by the user with readdir(3) to the prefix to form the full path, and
then feed that full path to git_check_attr().
Without anybody changing anything, we already do build the attr stack by
"scanning the repository" in that case, no?
^ permalink raw reply
* Re: [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references
From: Jeff King @ 2011-10-11 16:16 UTC (permalink / raw)
To: Michael Haggerty
Cc: git, Junio C Hamano, cmn, A Large Angry SCM, Daniel Barkalow,
Sverre Rabbelier
In-Reply-To: <1316121043-29367-20-git-send-email-mhagger@alum.mit.edu>
On Thu, Sep 15, 2011 at 11:10:40PM +0200, Michael Haggerty wrote:
> While resolving references, if a reference is found that is in an
> unrecognized format, emit a warning (and then fail, as before).
> Wouldn't *you* want to know?
Not necessarily. :)
I noticed this today, and bisected it to this patch:
$ git.v1.7.7 show config
fatal: ambiguous argument 'config': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions
$ git.mh.check-ref-format-3 show config
warning: reference in .git/config is formatted incorrectly
warning: reference in .git/config is formatted incorrectly
fatal: ambiguous argument 'config': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions
which is arguably not a big deal, as we eventually report failure
anyway. But it's even worse with:
$ git branch config v1.7.7
$ git show --oneline config
warning: reference in .git/config is formatted incorrectly
703f05a Git 1.7.7
As you probably guessed, this is due to the ref resolution rules first
looking directly in .git for refs, which catches things like "HEAD" and
fully-qualified refs like "refs/heads/foo".
Which means we have been considering ".git/config" as a possible ref for
a long time, which is arguably wrong. Your patch only makes it more
obvious that this is the case.
I wonder if the right solution is for us to be more picky about what can
be found in $GIT_DIR. Maybe matching all-uppercase, or starting with
"refs/", which I think would match existing convention?
-Peff
^ permalink raw reply
* Symmetric of rebase / more intelligent cherry-pick
From: Lionel Elie Mamane @ 2011-10-11 15:54 UTC (permalink / raw)
To: git
Hi,
I was thinking about a sort of symmetric to rebase, a kind of
"intelligent" mass cherry-pick. For the sake of example let's call it
"basere", although a better name would have to be found.
While
git rebase UPSTREAM
moves the *current* branch (and any commit in it, but not in UPSTREAM)
on top of UPSTREAM,
git basere UPSTREAM
would "cherry-pick" all commits in UPSTREAM that are not in HEAD to
the current branch.
git cherry-pick ..UPSTREAM
*nearly* does what I want, except that it lacks rebase's intelligence
of skipping commits that do the same textual changes as a commit
already in the current branch. So maybe it should be an option to
cherry-pick rather than a new command? E.g. --skip-same? What about
making it cherry-picks *default* behaviour?
Why do I think it is useful? Take the case of a developer following
two different origins A and B, and B uses rebase (not merge) to
integrate changes from A, but not as often as one would like. A could
for example be the "official" tree (e.g. linux Linus tree), and B a
feature branch of a developer that is meant to eventually go into A
"cleanly" at some future time and A's policy is that one has to rebase
before sending a merge request. Then another developer wants to keep
up with changes from *both* A and B. So, the situation could look
like:
A---A---A remotes/A/master
/
X---X---B0---B1---L master
\
B0---B1---B2--B3 remotes/B/master
So one does "git rebase remotes/A/master master" and one gets
A---A---A---B0---B1---L master
/
X---X
\
B0---B1---B2--B3 remotes/B/master
I'd like to go to the situation:
A---A---A---B0---B1---L---B2--B3 master
/
X---X
\
B0---B1---B2--B3 remotes/B/master
Without having to manually find the cutoff point "B2"
(or worse, if the history is more messy than that, as in:
A---A---A---B1---L master
/
X---X
\
B0---B1---B2--B3 remotes/B/master
)
The best way I have found to do that is:
git fetch B
git checkout --no-track -b TMP remotes/B/master
git rebase master
git checkout master
git merge --ff-only TMP
git branch -d TMP
or less commands but more messy/fragile (and more work for git):
git fetch B
git tag TMP
git rebase remotes/B/master
git rebase TMP
git tag -d TMP
but that is rather onerous for something that should be just one
command.
So what are your thoughts on that?
- Good idea? Would you take a patch that does something like that?
- Should it be cherry-picks's default behaviour?
- An option to cherry-pick?
- A new command? What name? Maybe "multi-cherry-pick"?
- An option to rebase, e.g. "--pull" or "--incoming" or "--here"?
("--pull" is probably too confusing wrt to "git pull"; people will
have trouble remembering which is which between "git pull
--rebase" and "git rebase --pull"...)
--
Lionel
^ permalink raw reply
* Release notes link on homepage 404
From: Lionel Elie Mamane @ 2011-10-11 15:58 UTC (permalink / raw)
To: git
The "Release Notes" link on http://git-scm.com/ is a 404
--
Lionel
^ permalink raw reply
* [PATCH v4] attr.c: respect core.ignorecase when matching attribute patterns
From: Brandon Casey @ 2011-10-11 15:53 UTC (permalink / raw)
To: gitster; +Cc: git, mhagger, Brandon Casey
In-Reply-To: <4E93BBA8.6080403@alum.mit.edu>
From: Brandon Casey <drafnel@gmail.com>
When core.ignorecase is true, the file globs configured in the
.gitattributes file should be matched case-insensitively against the paths
in the working directory. Let's do so.
Plus, add some tests.
The last set of tests is performed only on a case-insensitive filesystem.
Those tests make sure that git handles the case where the .gitignore file
resides in a subdirectory and the user supplies a path that does not match
the case in the filesystem. In that case^H^H^H^Hsituation, part of the
path supplied by the user is effectively interpreted case-insensitively,
and part of it is dependent on the setting of core.ignorecase. git will
currently only match the portion of the path below the directory holding
the .gitignore file according to the setting of core.ignorecase.
This is also partly future-proofing. Currently, git builds the attr stack
based on the path supplied by the user, so we don't have to do anything
special (like use strcmp_icase) to handle the parts of that path that don't
match the filesystem with respect to case. If git instead built the attr
stack by scanning the repository, then the paths in the origin field would
not necessarily match the paths supplied by the user. If someone makes a
change like that in the future, these tests will notice.
Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
On Mon, Oct 10, 2011 at 10:44 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 10/10/2011 08:01 PM, Brandon Casey wrote:
>> On Sun, Oct 9, 2011 at 10:16 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Maybe my commit message is not clear that it is describing the current
>> behavior and not defining it. Instead of
>>
>> git should only match the portion of the path below the directory
>> holding the .gitignore file according to the setting of
>> core.ignorecase.
>>
>> maybe I should say
>>
>> git will currently only match the portion of the path...
>>
>> I could also remove the following test from the CASE_INSENSITIVE_FS
>> tests since it is really a dontcare:
>>
>> attr_check A/b/h a/b/h "-c core.ignorecase=0"
>>
>> We don't care what happens when the user supplies A/b/h and a/b/h
>> exists on disk when core.ignorecase=0, we only care that A/b/h is
>> interpreted correctly when core.ignorecase=1.
>
> Sounds good to me.
Ok, here it is.
Minor tweak which, hopefully, adds clarity to the commit message and
removes an unnecessary test.
On top of bc/attr-ignore-case^ 64589a03a8ffb3eb4fb2ff8f416ff638a9aaa439.
-Brandon
attr.c | 5 ++-
t/t0003-attributes.sh | 59 ++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 61 insertions(+), 3 deletions(-)
diff --git a/attr.c b/attr.c
index 124337d..76b079f 100644
--- a/attr.c
+++ b/attr.c
@@ -11,6 +11,7 @@
#include "cache.h"
#include "exec_cmd.h"
#include "attr.h"
+#include "dir.h"
const char git_attr__true[] = "(builtin)true";
const char git_attr__false[] = "\0(builtin)false";
@@ -631,7 +632,7 @@ static int path_matches(const char *pathname, int pathlen,
/* match basename */
const char *basename = strrchr(pathname, '/');
basename = basename ? basename + 1 : pathname;
- return (fnmatch(pattern, basename, 0) == 0);
+ return (fnmatch_icase(pattern, basename, 0) == 0);
}
/*
* match with FNM_PATHNAME; the pattern has base implicitly
@@ -645,7 +646,7 @@ static int path_matches(const char *pathname, int pathlen,
return 0;
if (baselen != 0)
baselen++;
- return fnmatch(pattern, pathname + baselen, FNM_PATHNAME) == 0;
+ return fnmatch_icase(pattern, pathname + baselen, FNM_PATHNAME) == 0;
}
static int macroexpand_one(int attr_nr, int rem);
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index ae2f1da..6946c4b 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -9,7 +9,7 @@ attr_check () {
path="$1"
expect="$2"
- git check-attr test -- "$path" >actual 2>err &&
+ git $3 check-attr test -- "$path" >actual 2>err &&
echo "$path: test: $2" >expect &&
test_cmp expect actual &&
test_line_count = 0 err
@@ -27,6 +27,7 @@ test_expect_success 'setup' '
echo "onoff test -test"
echo "offon -test test"
echo "no notest"
+ echo "A/e/F test=A/e/F"
) >.gitattributes &&
(
echo "g test=a/g" &&
@@ -93,6 +94,62 @@ test_expect_success 'attribute test' '
'
+test_expect_success 'attribute matching is case sensitive when core.ignorecase=0' '
+
+ test_must_fail attr_check F f "-c core.ignorecase=0" &&
+ test_must_fail attr_check a/F f "-c core.ignorecase=0" &&
+ test_must_fail attr_check a/c/F f "-c core.ignorecase=0" &&
+ test_must_fail attr_check a/G a/g "-c core.ignorecase=0" &&
+ test_must_fail attr_check a/B/g a/b/g "-c core.ignorecase=0" &&
+ test_must_fail attr_check a/b/G a/b/g "-c core.ignorecase=0" &&
+ test_must_fail attr_check a/b/H a/b/h "-c core.ignorecase=0" &&
+ test_must_fail attr_check a/b/D/g "a/b/d/*" "-c core.ignorecase=0" &&
+ test_must_fail attr_check oNoFf unset "-c core.ignorecase=0" &&
+ test_must_fail attr_check oFfOn set "-c core.ignorecase=0" &&
+ attr_check NO unspecified "-c core.ignorecase=0" &&
+ test_must_fail attr_check a/b/D/NO "a/b/d/*" "-c core.ignorecase=0" &&
+ attr_check a/b/d/YES a/b/d/* "-c core.ignorecase=0" &&
+ test_must_fail attr_check a/E/f "A/e/F" "-c core.ignorecase=0"
+
+'
+
+test_expect_success 'attribute matching is case insensitive when core.ignorecase=1' '
+
+ attr_check F f "-c core.ignorecase=1" &&
+ attr_check a/F f "-c core.ignorecase=1" &&
+ attr_check a/c/F f "-c core.ignorecase=1" &&
+ attr_check a/G a/g "-c core.ignorecase=1" &&
+ attr_check a/B/g a/b/g "-c core.ignorecase=1" &&
+ attr_check a/b/G a/b/g "-c core.ignorecase=1" &&
+ attr_check a/b/H a/b/h "-c core.ignorecase=1" &&
+ attr_check a/b/D/g "a/b/d/*" "-c core.ignorecase=1" &&
+ attr_check oNoFf unset "-c core.ignorecase=1" &&
+ attr_check oFfOn set "-c core.ignorecase=1" &&
+ attr_check NO unspecified "-c core.ignorecase=1" &&
+ attr_check a/b/D/NO "a/b/d/*" "-c core.ignorecase=1" &&
+ attr_check a/b/d/YES unspecified "-c core.ignorecase=1" &&
+ attr_check a/E/f "A/e/F" "-c core.ignorecase=1"
+
+'
+
+test_expect_success 'check whether FS is case-insensitive' '
+ mkdir junk &&
+ echo good >junk/CamelCase &&
+ echo bad >junk/camelcase &&
+ if test "$(cat junk/CamelCase)" != good
+ then
+ test_set_prereq CASE_INSENSITIVE_FS
+ fi
+'
+
+test_expect_success CASE_INSENSITIVE_FS 'additional case insensitivity tests' '
+ test_must_fail attr_check a/B/D/g "a/b/d/*" "-c core.ignorecase=0" &&
+ test_must_fail attr_check A/B/D/NO "a/b/d/*" "-c core.ignorecase=0" &&
+ attr_check A/b/h a/b/h "-c core.ignorecase=1" &&
+ attr_check a/B/D/g "a/b/d/*" "-c core.ignorecase=1" &&
+ attr_check A/B/D/NO "a/b/d/*" "-c core.ignorecase=1"
+'
+
test_expect_success 'unnormalized paths' '
attr_check ./f f &&
--
1.7.7.138.g7f41b6
^ permalink raw reply related
* Re: [PATCHv5/RFC 1/6] Documentation: Preparation for gitweb manpages
From: Jakub Narebski @ 2011-10-11 15:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Drew Northup, Jonathan Nieder
In-Reply-To: <7v62jwjvdk.fsf@alter.siamese.dyndns.org>
On Tue, 11 Oct 2011, Junio C Hamano wrote:
> I probably do not have time to look into this, but just FYI my trial merge
> to 'pu' of this topic is failing like this:
>
> asciidoc: ERROR: gitweb.conf.txt: line 484: illegal style name: Default: ()
> asciidoc: ERROR: gitweb.conf.txt: line 494: illegal style name: Default: 300
Damn, I thought I have fixed that. This probably depends on AsciiDoc
version ("make doc" on 'master' generates a few _warnings_ for me related
to similar situation), but the problem is with
[Default: <value>]
that was copied from gitweb/README. But [<sth>] is an attribute list
(style name in simplest form), used more often in newer AsciiDoc.
So either we have to escape '[' and ']', i.e. use {startsb} and {endsb},
which would reduce human-friendliness, or move to different way of marking
default values, e.g. _italic_.
What do you think?
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCHv5/RFC 3/6] gitweb: Add manpage for gitweb (APPLICATION!!!)
From: Jakub Narebski @ 2011-10-11 14:20 UTC (permalink / raw)
To: Drew Northup; +Cc: Junio C Hamano, git, Jonathan Nieder
In-Reply-To: <1318341714.22324.46.camel@drew-northup.unet.maine.edu>
On Tue, 11 Oct 2011, Drew Northup wrote:
> On Tue, 2011-10-11 at 15:51 +0200, Jakub Narebski wrote:
> > On Tue, 11 Oct 2011, Drew Northup wrote:
>
> > > This would be why I included a synopsis with my original submission. As
> > > this was supposed to be a description of the configuration files of said
> > > application it does not make much sense to put the executable in the
> > > synopsis. Please forgive me for attempting to make sense!
> >
> > But this manpage is about _gitweb itself_, not about gitweb config file(s).
> > Gitweb itself is application, though it is not runnable directly (yet).
> >
> > Web apps either don't use manpages as documentation, and those that do
> > that I found (SVN::Web for example) include runnable server-starting script.
>
> Hmm, Couldn't tell from the mail header _which_ we were talking about in
> this subthread. I'll have to read the _whole_ patch apparently next
> time.
I'm sorry. I guess better subjects would be:
gitweb: Add gitweb.conf(5) manpage
gitweb: Add gitweb(1) manpage
instead of current
gitweb: Add manpage for gitweb configuration files
gitweb: Add manpage for gitweb
Or perhaps:
gitweb: Add manpage for gitweb configuration files
gitweb: Add manpage for gitweb itself
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: Display line numbers in gitk?
From: Sebastian Schuberth @ 2011-10-11 14:16 UTC (permalink / raw)
To: Pat Thoyts; +Cc: git
In-Reply-To: <CABNJ2GLaquXK7o_V_6KmOtnbcCGXMgbupbVJzXU_yK=2a=wKSg@mail.gmail.com>
On Tue, Oct 11, 2011 at 16:00, Pat Thoyts <patthoyts@gmail.com> wrote:
(putting the list back on CC)
> Not currently possible. It can be done though. Is this just for the
> file view or patch view as well?
> The following seems to work reasonably well for just the file view.
Thanks, from my tests it seems to work indeed very well. Although I
was initially thinking about the patch view.
A downside of line numbers in the file view could be that they'll be
copied to the clipboard, too, if you copy and paste code from there,
so they should probably be implemented as an option.
> From 0e18a9a2789838925f2ed50b05ce9d7e6c3a9a38 Mon Sep 17 00:00:00 2001
> From: Pat Thoyts <patthoyts@users.sourceforge.net>
> Date: Tue, 11 Oct 2011 14:57:24 +0100
> Subject: [PATCH] gitk: display line numbers for file view
>
> Suggested-by: Sebastian Schuberth <sschuberth@gmail.com>
> Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
> ---
> gitk | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/gitk b/gitk
> index 4cde0c4..70d8f57 100755
> --- a/gitk
> +++ b/gitk
> @@ -2277,6 +2277,9 @@ proc makewindow {} {
> if {$have_tk85} {
> $ctext conf -tabstyle wordprocessor
> }
> + catch {eval font create linenofont [font configure textfont] \
> + -size [expr {[font configure textfont -size] - 2}]}
> + $ctext tag configure lineno -foreground #808080 -font linenofont
> ${NS}::scrollbar .bleft.bottom.sb -command "$ctext yview"
> ${NS}::scrollbar .bleft.bottom.sbhorizontal -command "$ctext
> xview" -orient h
> pack .bleft.top -side top -fill x
> @@ -7316,7 +7319,7 @@ proc getblobline {bf id} {
> $ctext config -state normal
> set nl 0
> while {[incr nl] <= 1000 && [gets $bf line] >= 0} {
> - $ctext insert end "$line\n"
> + $ctext insert end $nl lineno "\t" {} "$line\n"
> }
> if {[eof $bf]} {
> global jump_to_here ctext_file_names commentend
> --
> 1.7.7.1.gbba15
^ permalink raw reply
* Re: [PATCHv5/RFC 3/6] gitweb: Add manpage for gitweb (APPLICATION!!!)
From: Drew Northup @ 2011-10-11 14:01 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, git, Jonathan Nieder
In-Reply-To: <201110111551.09621.jnareb@gmail.com>
On Tue, 2011-10-11 at 15:51 +0200, Jakub Narebski wrote:
> On Tue, 11 Oct 2011, Drew Northup wrote:
> > This would be why I included a synopsis with my original submission. As
> > this was supposed to be a description of the configuration files of said
> > application it does not make much sense to put the executable in the
> > synopsis. Please forgive me for attempting to make sense!
>
> But this manpage is about _gitweb itself_, not about gitweb config file(s).
> Gitweb itself is application, though it is not runnable directly (yet).
>
> Web apps either don't use manpages as documentation, and those that do
> that I found (SVN::Web for example) include runnable server-starting script.
Hmm, Couldn't tell from the mail header _which_ we were talking about in
this subthread. I'll have to read the _whole_ patch apparently next
time.
Crawling back into my cave...
--
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
^ permalink raw reply
* Re: [PATCH v3] Teach merge the '[-e|--edit]' option
From: Peter Krefting @ 2011-10-11 13:57 UTC (permalink / raw)
To: Jay Soffian; +Cc: Git Mailing List, Junio C Hamano, Todd A. Jacobs
In-Reply-To: <1318099192-60860-1-git-send-email-jaysoffian@gmail.com>
Jay Soffian:
> +--edit::
> +-e::
> ++
> + Invoke editor before committing successful merge to further
> + edit the default merge message.
I have a feature request, and that is to also add a configuration option to
make this the default behaviour.
--
\\// Peter - http://www.softwolves.pp.se/
^ permalink raw reply
* Re: [PATCHv5/RFC 3/6] gitweb: Add manpage for gitweb
From: Jakub Narebski @ 2011-10-11 13:51 UTC (permalink / raw)
To: Drew Northup; +Cc: Junio C Hamano, git, Jonathan Nieder
In-Reply-To: <1318338135.22324.33.camel@drew-northup.unet.maine.edu>
On Tue, 11 Oct 2011, Drew Northup wrote:
> On Mon, 2011-10-10 at 17:18 -0500, Jonathan Nieder wrote:
> > Jakub Narebski wrote:
> >
> > > The problem is that catering to old AsciiDoc (but still used by some of
> > > long-term-support Linux distributions) requires to have "SYNOPSIS"
> > > section... but there is no natural synopsis for non self-hostable web
> > > application, is there?
> >
> > I personally think something like
> >
> > SYNOPSIS
> > --------
> > /usr/share/gitweb/gitweb.cgi
> > git instaweb
> >
> > or perhaps something like
> >
> > SYNOPSIS
> > --------
> > http://<site>/?p=<project>.git;a=<action>;h=<object>;<parameters>
> > http://<site>/<project>/<action>/<object>?<parameters>
> >
> > would be best.
>
> This would be why I included a synopsis with my original submission. As
> this was supposed to be a description of the configuration files of said
> application it does not make much sense to put the executable in the
> synopsis. Please forgive me for attempting to make sense!
But this manpage is about _gitweb itself_, not about gitweb config file(s).
Gitweb itself is application, though it is not runnable directly (yet).
Web apps either don't use manpages as documentation, and those that do
that I found (SVN::Web for example) include runnable server-starting script.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCHv5/RFC 3/6] gitweb: Add manpage for gitweb
From: Drew Northup @ 2011-10-11 13:02 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, git, Jonathan Nieder
In-Reply-To: <20111010221811.GA21367@elie.hsd1.il.comcast.net>
On Mon, 2011-10-10 at 17:18 -0500, Jonathan Nieder wrote:
> Jakub Narebski wrote:
>
> > The problem is that catering to old AsciiDoc (but still used by some of
> > long-term-support Linux distributions) requires to have "SYNOPSIS"
> > section... but there is no natural synopsis for non self-hostable web
> > application, is there?
>
> I personally think something like
>
> SYNOPSIS
> --------
> /usr/share/gitweb/gitweb.cgi
> git instaweb
>
> or perhaps something like
>
> SYNOPSIS
> --------
> http://<site>/?p=<project>.git;a=<action>;h=<object>;<parameters>
> http://<site>/<project>/<action>/<object>?<parameters>
>
> would be best.
This would be why I included a synopsis with my original submission. As
this was supposed to be a description of the configuration files of said
application it does not make much sense to put the executable in the
synopsis. Please forgive me for attempting to make sense!
--
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
^ permalink raw reply
* Re: [PATCH] Improving performance with pthreads in refresh_index().
From: Michael J Gruber @ 2011-10-11 11:50 UTC (permalink / raw)
To: klinkert; +Cc: gitster, git
In-Reply-To: <1318325521-23262-1-git-send-email-klinkert@webgods.de>
klinkert@webgods.de venit, vidit, dixit 11.10.2011 11:32:
> Git performs for every file in a repository at least one (with a cold cache)
> lstat(). In larger repositories operations like git status take a
> long time. In case your local repository is located on a remote server
> (e. g. mounted via nfs) it ends up in an *incredible* slow git.
>
> With this patch you're able to determine a number of threads (maxthreads)
> in your config file to run these tons of lstats in threads. There
> won't be created any pthreads if you haven't set maxthreads. In my
> test cases a git status with this patch performs enormously faster (over
> two minutes before and approximately 25 seconds now). Of course, it
> has a positive impact on other git commands, too.
Can you specify under which circumstances one should get a speedup? Our
NFS isn't slow enough... but on a dead slow sshfs work tree I get the
following for "git status -s":
maxthreads: 0, preloadindex: false, time: 14.73
maxthreads: 1, preloadindex: false, time: 14.25
maxthreads: 2, preloadindex: false, time: 13.32
maxthreads: 3, preloadindex: false, time: 12.40
maxthreads: 4, preloadindex: false, time: 12.65
maxthreads: 5, preloadindex: false, time: 12.16
maxthreads: 8, preloadindex: false, time: 12.32
maxthreads: 10, preloadindex: false, time: 11.98
maxthreads: 15, preloadindex: false, time: 12.31
maxthreads: 20, preloadindex: false, time: 12.00
maxthreads: 0, preloadindex: true, time: 12.17
maxthreads: 1, preloadindex: true, time: 11.98
maxthreads: 2, preloadindex: true, time: 12.21
maxthreads: 3, preloadindex: true, time: 11.99
maxthreads: 4, preloadindex: true, time: 12.14
maxthreads: 5, preloadindex: true, time: 12.21
maxthreads: 8, preloadindex: true, time: 12.14
maxthreads: 10, preloadindex: true, time: 12.08
maxthreads: 15, preloadindex: true, time: 12.16
maxthreads: 20, preloadindex: true, time: 11.96
So it seams it gives me what preloadindex does, which is not much.
Note: I'm not saying the patch is bad. I'm just wondering whether that
is expected.
Michael
P.S.: It's actually sshfs with ssh to an NFSv3 client (server restricts
exports) :(
^ permalink raw reply
* Re: [PATCH] Improving performance with pthreads in refresh_index().
From: Nguyen Thai Ngoc Duy @ 2011-10-11 10:59 UTC (permalink / raw)
To: klinkert; +Cc: gitster, git
In-Reply-To: <1318325521-23262-1-git-send-email-klinkert@webgods.de>
On Tue, Oct 11, 2011 at 8:32 PM, <klinkert@webgods.de> wrote:
> Git performs for every file in a repository at least one (with a cold cache)
> lstat(). In larger repositories operations like git status take a
> long time. In case your local repository is located on a remote server
> (e. g. mounted via nfs) it ends up in an *incredible* slow git.
This sounds really similar to what Linus did with preload-index.c..
--
Duy
^ permalink raw reply
* Re: [PATCH] Improving performance with pthreads in refresh_index().
From: Johannes Sixt @ 2011-10-11 10:46 UTC (permalink / raw)
To: klinkert; +Cc: gitster, git
In-Reply-To: <1318325521-23262-1-git-send-email-klinkert@webgods.de>
First of all, thanks for your contribution!
When you submit a patch, you should make sure that the "From:" line of
your mail includes your full name. You included it in the Signed-off-by
line and that is a good start.
Am 10/11/2011 11:32, schrieb klinkert@webgods.de:
> Git performs for every file in a repository at least one (with a cold cache)
> lstat().
It doesn't do the lstat() when the cache is warm? I doubt it.
> In larger repositories operations like git status take a
> long time. In case your local repository is located on a remote server
> (e. g. mounted via nfs) it ends up in an *incredible* slow git.
"Incredible" is very subjective. You should back your claim with benchmarks.
> With this patch you're able to determine a number of threads (maxthreads)
> in your config file to run these tons of lstats in threads. There
> won't be created any pthreads if you haven't set maxthreads. In my
> test cases a git status with this patch performs enormously faster (over
> two minutes before and approximately 25 seconds now).
Ok, so here you have something that you can turn into a benchmark. Just
replace the hand-waving by hard facts.
You report an improvement by a factor 4. How many threads did you use? How
does the number of threads change the improvement. How does the number of
threads influence the performance on a repository that is not on a network
partition? Can I set it to 25 even on a dual core machine without
noticable degradation?
> Of course, it
> has a positive impact on other git commands, too.
What other commands? Benchmarks?
> diff --git a/attr.c b/attr.c
> index 33cb4e4..d296fe8 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -8,10 +8,14 @@
> */
>
> #define NO_THE_INDEX_COMPATIBILITY_MACROS
> +#include <pthread.h>
This and all pthread usages should be bracketed by NO_PTHREADS, or you
could use thread-utils.h instead. And it should be included *after* cache.h.
> +#undef _FILE_OFFSET_BITS
What is this good for?
> #include "cache.h"
> #include "exec_cmd.h"
> #include "attr.h"
>
> +pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
This should be static, no? Otherwise, the name is *WAY* too generic for a
global variable.
What does the mutex protect? Please write some comment. It is not obvious.
Or perhaps it is sufficient to move the variable near the data that it
protects.
PTHREAD_MUTEX_INITIALIZER does not work on Windows. Please call
pthread_mutex_init() from a suitable place.
> +
> const char git_attr__true[] = "(builtin)true";
> const char git_attr__false[] = "\0(builtin)false";
> static const char git_attr__unknown[] = "(builtin)unknown";
> @@ -748,6 +752,9 @@ int git_check_attr(const char *path, int num, struct git_attr_check *check)
> {
> int i;
>
> + if (max_threads)
> + pthread_mutex_lock(&mutex);
As I said, this should be bracketed by #ifndef NO_PTHREADS. But since you
also bracket all mutex uses by max_threads anyway, you should factor this
into a small helper function.
> --- a/cache.h
> +++ b/cache.h
> @@ -600,6 +600,7 @@ extern int read_replace_refs;
> extern int fsync_object_files;
> extern int core_preload_index;
> extern int core_apply_sparse_checkout;
> +extern int max_threads;
This name is *WAY* too generic for a global variable. Please choose a more
specific name.
> --- a/config.c
> +++ b/config.c
> @@ -466,6 +466,11 @@ int git_config_pathname(const char **dest, const char *var, const char *value)
>
> static int git_default_core_config(const char *var, const char *value)
> {
> + if (!strcmp(var, "core.maxthreads")) {
Same here. core.maxthreads is *WAY* too generic for its current use.
Please paint the bikeshed in a more distiguishing color. ;)
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -5,6 +5,8 @@
> */
> #define NO_THE_INDEX_COMPATIBILITY_MACROS
> #include "cache.h"
> +#undef _FILE_OFFSET_BITS
Again: why this?
> +#include <pthread.h>
> #include "cache-tree.h"
> #include "refs.h"
> #include "dir.h"
> @@ -13,6 +15,8 @@
> #include "blob.h"
> #include "resolve-undo.h"
>
> +pthread_mutex_t mutex_refresh_index = PTHREAD_MUTEX_INITIALIZER;
Should be static, no?
> static void show_file(const char * fmt, const char * name, int in_porcelain,
> - int * first, const char *header_msg)
> + int * first, char *header_msg)
Why?
> -int refresh_index(struct index_state *istate, unsigned int flags, const char **pathspec,
> - char *seen, const char *header_msg)
> +struct t_ctx {
> + int flags;
> + int has_errors;
> + struct index_state *istate;
> + const char **pathspec;
> + char *seen, header_msg;
> +
> + int tasks_done;
> +} ctx;
Should this be static? Is this the only global data that is protected by
the mutex?
> +
> +int
> +thread_manager(void)
Style: the type is on the same line as the function name.
> +void *
> +thread_refresh_index(void *p)
> {
> + struct cache_entry *ce, *new;
> + int cache_errno = 0;
> int i;
> - int has_errors = 0;
> +
> + while ((i = thread_manager()) != -1) {
> + struct index_state *istate = ctx.istate;
> + int flags = ctx.flags;
> +
> int really = (flags & REFRESH_REALLY) != 0;
> int allow_unmerged = (flags & REFRESH_UNMERGED) != 0;
> int quiet = (flags & REFRESH_QUIET) != 0;
> int not_new = (flags & REFRESH_IGNORE_MISSING) != 0;
> - int ignore_submodules = (flags & REFRESH_IGNORE_SUBMODULES) != 0;
> + int ignore_submodules =
> + (flags & REFRESH_IGNORE_SUBMODULES) != 0;
Indentation?
Perhaps it is worthwhile to factor the body of the loop into a helper
function in a preparatory patch to reduce churn in the "real" patch.
> ce = istate->cache[i];
> - if (ignore_submodules && S_ISGITLINK(ce->ce_mode))
> + if (ignore_submodules && S_ISGITLINK(ce->ce_mode)) {
> continue;
> + }
Style: do not add braces unnecessarily.
> +
> + if (max_threads)
> + pthread_mutex_lock(&mutex_refresh_index);
>
> if (ce_stage(ce)) {
> while ((i < istate->cache_nr) &&
> ! strcmp(istate->cache[i]->name, ce->name))
> i++;
> i--;
> - if (allow_unmerged)
> + if (allow_unmerged) {
> + if (max_threads)
> + pthread_mutex_unlock(&mutex_refresh_index);
> continue;
> - show_file(needs_merge_fmt, ce->name, in_porcelain, &first, header_msg);
> - has_errors = 1;
> + }
> + show_file(needs_merge_fmt, ce->name, in_porcelain,
> + &first, &ctx.header_msg);
You are calling show_file() from the thread. Does that mean that the
output is not in index order anymore? Are there guarantees that the output
of different threads does not overlap?
> + ctx.has_errors = 1;
> + if (max_threads)
> + pthread_mutex_unlock(&mutex_refresh_index);
So, at least the second worry is unfounded since output is produced while
the mutex is held.
BTW, would it be possible to use a different mutex for this purpose?
> + /* create threads */
> + for (i = 0; i < max_threads; i++) {
> + ret = pthread_create(&threads[created_threads], NULL,
> + thread_refresh_index, NULL);
> + if (ret) {
> + printf("pthread_create failed ret=%d\n", ret);
We have warning() to write warnings.
-- Hannes
^ permalink raw reply
* [PATCH] Improving performance with pthreads in refresh_index().
From: klinkert @ 2011-10-11 9:32 UTC (permalink / raw)
To: gitster; +Cc: git
Git performs for every file in a repository at least one (with a cold cache)
lstat(). In larger repositories operations like git status take a
long time. In case your local repository is located on a remote server
(e. g. mounted via nfs) it ends up in an *incredible* slow git.
With this patch you're able to determine a number of threads (maxthreads)
in your config file to run these tons of lstats in threads. There
won't be created any pthreads if you haven't set maxthreads. In my
test cases a git status with this patch performs enormously faster (over
two minutes before and approximately 25 seconds now). Of course, it
has a positive impact on other git commands, too.
Signed-off-by: Simon Klinkert <klinkert@webgods.de>
---
attr.c | 22 +++++++++
cache.h | 1 +
config.c | 5 ++
environment.c | 1 +
read-cache.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++-------
5 files changed, 151 insertions(+), 18 deletions(-)
diff --git a/attr.c b/attr.c
index 33cb4e4..d296fe8 100644
--- a/attr.c
+++ b/attr.c
@@ -8,10 +8,14 @@
*/
#define NO_THE_INDEX_COMPATIBILITY_MACROS
+#include <pthread.h>
+#undef _FILE_OFFSET_BITS
#include "cache.h"
#include "exec_cmd.h"
#include "attr.h"
+pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
+
const char git_attr__true[] = "(builtin)true";
const char git_attr__false[] = "\0(builtin)false";
static const char git_attr__unknown[] = "(builtin)unknown";
@@ -748,6 +752,9 @@ int git_check_attr(const char *path, int num, struct git_attr_check *check)
{
int i;
+ if (max_threads)
+ pthread_mutex_lock(&mutex);
+
collect_all_attrs(path);
for (i = 0; i < num; i++) {
@@ -757,6 +764,9 @@ int git_check_attr(const char *path, int num, struct git_attr_check *check)
check[i].value = value;
}
+ if (max_threads)
+ pthread_mutex_unlock(&mutex);
+
return 0;
}
@@ -764,6 +774,9 @@ int git_all_attrs(const char *path, int *num, struct git_attr_check **check)
{
int i, count, j;
+ if (max_threads)
+ pthread_mutex_lock(&mutex);
+
collect_all_attrs(path);
/* Count the number of attributes that are set. */
@@ -785,6 +798,9 @@ int git_all_attrs(const char *path, int *num, struct git_attr_check **check)
}
}
+ if (max_threads)
+ pthread_mutex_unlock(&mutex);
+
return 0;
}
@@ -795,8 +811,14 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
if (is_bare_repository() && new != GIT_ATTR_INDEX)
die("BUG: non-INDEX attr direction in a bare repo");
+ if (max_threads)
+ pthread_mutex_lock(&mutex);
+
direction = new;
if (new != old)
drop_attr_stack();
use_index = istate;
+
+ if (max_threads)
+ pthread_mutex_unlock(&mutex);
}
diff --git a/cache.h b/cache.h
index 607c2ea..ab1b3e4 100644
--- a/cache.h
+++ b/cache.h
@@ -600,6 +600,7 @@ extern int read_replace_refs;
extern int fsync_object_files;
extern int core_preload_index;
extern int core_apply_sparse_checkout;
+extern int max_threads;
enum branch_track {
BRANCH_TRACK_UNSPECIFIED = -1,
diff --git a/config.c b/config.c
index 4183f80..24de139 100644
--- a/config.c
+++ b/config.c
@@ -466,6 +466,11 @@ int git_config_pathname(const char **dest, const char *var, const char *value)
static int git_default_core_config(const char *var, const char *value)
{
+ if (!strcmp(var, "core.maxthreads")) {
+ max_threads = git_config_int(var, value);
+ return 0;
+ }
+
/* This needs a better name */
if (!strcmp(var, "core.filemode")) {
trust_executable_bit = git_config_bool(var, value);
diff --git a/environment.c b/environment.c
index e96edcf..8bca4d5 100644
--- a/environment.c
+++ b/environment.c
@@ -59,6 +59,7 @@ char *notes_ref_name;
int grafts_replace_parents = 1;
int core_apply_sparse_checkout;
struct startup_info *startup_info;
+int max_threads = 1;
/* Parallel index stat data preload? */
int core_preload_index = 0;
diff --git a/read-cache.c b/read-cache.c
index 01a0e25..350bf4b 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -5,6 +5,8 @@
*/
#define NO_THE_INDEX_COMPATIBILITY_MACROS
#include "cache.h"
+#undef _FILE_OFFSET_BITS
+#include <pthread.h>
#include "cache-tree.h"
#include "refs.h"
#include "dir.h"
@@ -13,6 +15,8 @@
#include "blob.h"
#include "resolve-undo.h"
+pthread_mutex_t mutex_refresh_index = PTHREAD_MUTEX_INITIALIZER;
+
static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really);
/* Index extensions.
@@ -1080,7 +1084,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
}
static void show_file(const char * fmt, const char * name, int in_porcelain,
- int * first, const char *header_msg)
+ int * first, char *header_msg)
{
if (in_porcelain && *first && header_msg) {
printf("%s\n", header_msg);
@@ -1089,45 +1093,96 @@ static void show_file(const char * fmt, const char * name, int in_porcelain,
printf(fmt, name);
}
-int refresh_index(struct index_state *istate, unsigned int flags, const char **pathspec,
- char *seen, const char *header_msg)
+struct t_ctx {
+ int flags;
+ int has_errors;
+ struct index_state *istate;
+ const char **pathspec;
+ char *seen, header_msg;
+
+ int tasks_done;
+} ctx;
+
+int
+thread_manager(void)
+{
+ if (max_threads)
+ pthread_mutex_lock(&mutex_refresh_index);
+
+ if (ctx.tasks_done >= ctx.istate->cache_nr) {
+ if (max_threads)
+ pthread_mutex_unlock(&mutex_refresh_index);
+ return -1;
+ }
+
+ int task = ctx.tasks_done++;
+
+ if (max_threads)
+ pthread_mutex_unlock(&mutex_refresh_index);
+
+ return task;
+}
+
+void *
+thread_refresh_index(void *p)
{
+ struct cache_entry *ce, *new;
+ int cache_errno = 0;
int i;
- int has_errors = 0;
+
+ while ((i = thread_manager()) != -1) {
+ struct index_state *istate = ctx.istate;
+ int flags = ctx.flags;
+
int really = (flags & REFRESH_REALLY) != 0;
int allow_unmerged = (flags & REFRESH_UNMERGED) != 0;
int quiet = (flags & REFRESH_QUIET) != 0;
int not_new = (flags & REFRESH_IGNORE_MISSING) != 0;
- int ignore_submodules = (flags & REFRESH_IGNORE_SUBMODULES) != 0;
+ int ignore_submodules =
+ (flags & REFRESH_IGNORE_SUBMODULES) != 0;
int first = 1;
int in_porcelain = (flags & REFRESH_IN_PORCELAIN);
unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
const char *needs_update_fmt;
const char *needs_merge_fmt;
- needs_update_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
- needs_merge_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
- for (i = 0; i < istate->cache_nr; i++) {
- struct cache_entry *ce, *new;
- int cache_errno = 0;
+ needs_update_fmt =
+ (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
+ needs_merge_fmt =
+ (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
ce = istate->cache[i];
- if (ignore_submodules && S_ISGITLINK(ce->ce_mode))
+ if (ignore_submodules && S_ISGITLINK(ce->ce_mode)) {
continue;
+ }
+
+ if (max_threads)
+ pthread_mutex_lock(&mutex_refresh_index);
if (ce_stage(ce)) {
while ((i < istate->cache_nr) &&
! strcmp(istate->cache[i]->name, ce->name))
i++;
i--;
- if (allow_unmerged)
+ if (allow_unmerged) {
+ if (max_threads)
+ pthread_mutex_unlock(&mutex_refresh_index);
continue;
- show_file(needs_merge_fmt, ce->name, in_porcelain, &first, header_msg);
- has_errors = 1;
+ }
+ show_file(needs_merge_fmt, ce->name, in_porcelain,
+ &first, &ctx.header_msg);
+ ctx.has_errors = 1;
+ if (max_threads)
+ pthread_mutex_unlock(&mutex_refresh_index);
continue;
}
- if (pathspec && !match_pathspec(pathspec, ce->name, strlen(ce->name), 0, seen))
+ if (max_threads)
+ pthread_mutex_unlock(&mutex_refresh_index);
+
+ if (ctx.pathspec && !match_pathspec(ctx.pathspec, ce->name,
+ strlen(ce->name), 0,
+ ctx.seen))
continue;
new = refresh_cache_ent(istate, ce, options, &cache_errno);
@@ -1145,14 +1200,63 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
}
if (quiet)
continue;
- show_file(needs_update_fmt, ce->name, in_porcelain, &first, header_msg);
- has_errors = 1;
+
+ if (max_threads)
+ pthread_mutex_lock(&mutex_refresh_index);
+
+ show_file(needs_update_fmt, ce->name, in_porcelain,
+ &first, &ctx.header_msg);
+
+ if (max_threads)
+ pthread_mutex_unlock(&mutex_refresh_index);
+
+ ctx.has_errors = 1;
continue;
}
replace_index_entry(istate, i, new);
}
- return has_errors;
+ return NULL;
+}
+
+int refresh_index(struct index_state *istate, unsigned int flags,
+ const char **pathspec, char *seen, const char *header_msg)
+{
+ int i;
+ int ret;
+ unsigned int created_threads = 0;
+
+ ctx.has_errors = 0;
+ ctx.tasks_done = 0;
+ ctx.istate = istate;
+ ctx.flags = flags;
+ ctx.pathspec = &pathspec[0];
+
+ if (istate->cache_nr < max_threads)
+ max_threads = istate->cache_nr;
+
+ if (max_threads > 1) {
+ pthread_t threads[max_threads];
+
+ /* create threads */
+ for (i = 0; i < max_threads; i++) {
+ ret = pthread_create(&threads[created_threads], NULL,
+ thread_refresh_index, NULL);
+ if (ret) {
+ printf("pthread_create failed ret=%d\n", ret);
+ break;
+ }
+ ++created_threads;
+ }
+
+ /* collect threads */
+ for (i = 0; i < created_threads; i++) {
+ ret = pthread_join(threads[i], NULL);
+ }
+ } else {
+ thread_refresh_index(NULL);
+ }
+ return ctx.has_errors;
}
static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really)
--
1.7.7
^ permalink raw reply related
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