* Re: Performance issue exposed by git-filter-branch
From: Ken Brownfield @ 2010-12-17 2:31 UTC (permalink / raw)
To: git; +Cc: David Barr
In-Reply-To: <20101217014539.GA6775@burratino>
The thread titled "git and larger trees, not so fast?". Some of the history is lost, but here's the earliest post I can find:
http://lists-archives.org/git/627040-git-and-larger-trees-not-so-fast.html
On GMANE:
http://article.gmane.org/gmane.comp.version-control.git/55460/match=git+larger+trees+not+so+fast
But I can't figure out how to show the whole thread.
Sorry, that paragraph of my email disappeared. :-(
Ken
On Dec 16, 2010, at 5:45 PM, Jonathan Nieder wrote:
> Hi Ken,
>
> Ken Brownfield wrote:
>
>> Is there a way to apply the optimizations mentioned in that old
>> thread to the code paths used by git-filter-branch (mainly git-read
>> and git-rm, seemingly), or is there another way to investigate and
>> improve the performance of the filter?
>
> Which old thread?
>
> You might be able to get faster results using the approach of [1]
> (using "git cat-file --batch-check" to collect the trees you want
> and "git fast-import" to paste them together), which avoids unpacking
> trees when not needed.
>
> Hope that helps,
> Jonathan
>
> [1] http://repo.or.cz/w/git/barrbrain/github.git/commitdiff/db-svn-filter-root
^ permalink raw reply
* Re: [PATCH 07/14] t7800-difftool.sh: Fix a test failure on Cygwin
From: David Aguilar @ 2010-12-17 2:33 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <4D07B724.8000901@ramsay1.demon.co.uk>
On Tue, Dec 14, 2010 at 06:27:48PM +0000, Ramsay Jones wrote:
>
> In particular, test 14 'difftool last flag wins' fails. This is
> caused by git-difftool.perl passing both GIT_DIFFTOOL_NO_PROMPT
> (='true') and GIT_DIFFTOOL_PROMPT (='true') to the difftool helper
> script. Despite the appropriate key being deleted from the ENV
> hash, it seems that once a key has been set in the hash, it gets
> passed along to the system() call. (ie deleting the key does not
> do the equivalent of unsetenv()).
>
> In order to fix the problem, we keep track of the required prompt
> state while processing the arguments, and then set the relevant
> ENV hash key only once at the end.
>
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
> git-difftool.perl | 12 ++++++++----
> 1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index e95e4ad..ced1615 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -52,6 +52,7 @@ sub generate_command
> my @command = (exe('git'), 'diff');
> my $skip_next = 0;
> my $idx = -1;
> + my $prompt = '';
Would it be simpler to set $prompt = 1 and then
flip it to 0 when -y | or --no-prompt is supplied?
> for my $arg (@ARGV) {
> $idx++;
> if ($skip_next) {
> @@ -89,13 +90,11 @@ sub generate_command
> next;
> }
> if ($arg eq '-y' || $arg eq '--no-prompt') {
> - $ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
> - delete $ENV{GIT_DIFFTOOL_PROMPT};
> + $prompt = 'no';
> next;
> }
> if ($arg eq '--prompt') {
> - $ENV{GIT_DIFFTOOL_PROMPT} = 'true';
> - delete $ENV{GIT_DIFFTOOL_NO_PROMPT};
> + $prompt = 'yes';
> next;
> }
> if ($arg eq '-h' || $arg eq '--help') {
> @@ -103,6 +102,11 @@ sub generate_command
> }
> push @command, $arg;
> }
> + if ($prompt eq 'yes') {
This would become:
if ($prompt) {
...
}
else {
...
}
> + $ENV{GIT_DIFFTOOL_PROMPT} = 'true';
> + } elsif ($prompt eq 'no') {
> + $ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
> + }
> return @command
> }
>
> --
> 1.7.3
>
>
--
David
^ permalink raw reply
* Re: Performance issue exposed by git-filter-branch
From: Thomas Rast @ 2010-12-17 1:54 UTC (permalink / raw)
To: Ken Brownfield; +Cc: git
In-Reply-To: <41C1B4AC-8427-4D62-BEB6-689A4BE4EE5B@irridia.com>
Ken Brownfield wrote:
> git filter-branch --index-filter 'git rm -r --cached --ignore-unmatch -- bigdirtree stuff/a stuff/b stuff/c stuff/dir/{a,b,c}' --prune-empty --tag-name-filter cat -- --all
[...]
> Now that the same repository has grown, this same filter-branch
> process now takes 6.5 *days* at 100% CPU on the same machine (2x4
> Xeon, x86_64) on git-1.7.3.2. There's no I/O, memory, or other
> resource contention.
If all you do is an index-filter for deletion, I think it should be
rather easy to achieve good results by filtering the fast-export
stream to remove these files, and then piping that back to
fast-import.
(It's just that AFAIK nobody has written that code yet.)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: Performance issue exposed by git-filter-branch
From: Jonathan Nieder @ 2010-12-17 1:45 UTC (permalink / raw)
To: Ken Brownfield; +Cc: git, David Barr
In-Reply-To: <41C1B4AC-8427-4D62-BEB6-689A4BE4EE5B@irridia.com>
Hi Ken,
Ken Brownfield wrote:
> Is there a way to apply the optimizations mentioned in that old
> thread to the code paths used by git-filter-branch (mainly git-read
> and git-rm, seemingly), or is there another way to investigate and
> improve the performance of the filter?
Which old thread?
You might be able to get faster results using the approach of [1]
(using "git cat-file --batch-check" to collect the trees you want
and "git fast-import" to paste them together), which avoids unpacking
trees when not needed.
Hope that helps,
Jonathan
[1] http://repo.or.cz/w/git/barrbrain/github.git/commitdiff/db-svn-filter-root
^ permalink raw reply
* Performance issue exposed by git-filter-branch
From: Ken Brownfield @ 2010-12-17 1:07 UTC (permalink / raw)
To: git
I have a large git repository (1,757,784 objects, 209,282 commits) from which I have been planning to filter large tree portions (~36,000 of ~132,000 files). When I first ran git-filter-branch on this repository about a year ago:
git filter-branch --index-filter 'git rm -r --cached --ignore-unmatch -- bigdirtree stuff/a stuff/b stuff/c stuff/dir/{a,b,c}' --prune-empty --tag-name-filter cat -- --all
The process took around 25 hours for the repository when it was at ~101k commits. This wasn't ideal, but could be completed over a weekend maintenance. There are 50 daily active committers to this repository, so the window has to be short.
However, we didn't have time to implement this newly filtered repo (it involves everyone recloning, etc) until now.
Now that the same repository has grown, this same filter-branch process now takes 6.5 *days* at 100% CPU on the same machine (2x4 Xeon, x86_64) on git-1.7.3.2. There's no I/O, memory, or other resource contention.
I tend to doubt there are any multi-processing opportunities with this process, so at this point git-filter-branch is no longer feasible.
This is an oprofile sample (all samples >1%) at roughly one day into the 6.5 day Rewrite process:
[...]
11594 1.0208 git git add_index_entry
11616 1.0228 vmlinux-debug-2.6.24-28-server vmlinux-debug-2.6.24-28-server find_lock_page
12624 1.1115 git git decode_tree_entry
13065 1.1504 git git refresh_index
13757 1.2113 git git match_pathspec
14041 1.2363 git git read_packed_refs
18309 1.6121 vmlinux-debug-2.6.24-28-server vmlinux-debug-2.6.24-28-server unmap_vmas
20014 1.7622 libc-2.7.so libc-2.7.so _int_malloc
24248 2.1350 git git find_cache_pos
24560 2.1625 git git find_pack_entry_one
29042 2.5571 vmlinux-debug-2.6.24-28-server vmlinux-debug-2.6.24-28-server debug
31202 2.7473 libz.so.1.2.3.3 libz.so.1.2.3.3 inflate
34941 3.0765 git git df_name_compare
36749 3.2357 libz.so.1.2.3.3 libz.so.1.2.3.3 inflate_fast
41704 3.6720 git git index_name_pos
46908 4.1302 vmlinux-debug-2.6.24-28-server vmlinux-debug-2.6.24-28-server clear_page
92554 8.1493 libc-2.7.so libc-2.7.so memcpy
127439 11.2208 libcrypto.so.0.9.8 libcrypto.so.0.9.8 sha1_block_data_order
188373 16.5860 git git cache_name_compare
cache_name_compare (and the presumed follow-ons of memcpy/sha/malloc/etc) is the major consumer.
Sampling the filter only 2k commits into the Rewrite stage shows:
[...]
12058 1.0135 vmlinux-debug-2.6.24-28-server vmlinux-debug-2.6.24-28-server do_path_lookup
13532 1.1374 vmlinux-debug-2.6.24-28-server vmlinux-debug-2.6.24-28-server copy_user_generic_string
13934 1.1712 vmlinux-debug-2.6.24-28-server vmlinux-debug-2.6.24-28-server clear_page
16565 1.3924 git git cache_name_compare
16948 1.4246 libc-2.7.so libc-2.7.so memcpy
16969 1.4263 vmlinux-debug-2.6.24-28-server vmlinux-debug-2.6.24-28-server system_call
19189 1.6129 vmlinux-debug-2.6.24-28-server vmlinux-debug-2.6.24-28-server debug
22697 1.9078 git git add_ref
31112 2.6151 ext3 ext3 (no symbols)
33925 2.8516 git git sort_ref_list
34026 2.8600 vmlinux-debug-2.6.24-28-server vmlinux-debug-2.6.24-28-server _atomic_dec_and_lock
39304 3.3037 git git read_packed_refs
43920 3.6917 vmlinux-debug-2.6.24-28-server vmlinux-debug-2.6.24-28-server __link_path_walk
58504 4.9175 libc-2.7.so libc-2.7.so strcmp
79957 6.7208 vmlinux-debug-2.6.24-28-server vmlinux-debug-2.6.24-28-server __d_lookup
168696 14.1797 git git prepare_packed_git_one
The process is still pretty slow (1-2 commits per second) but cache_name_compare is in the background.
Is there a way to apply the optimizations mentioned in that old thread to the code paths used by git-filter-branch (mainly git-read and git-rm, seemingly), or is there another way to investigate and improve the performance of the filter?
Outside of this specific issue, it might be worth taking a look at the overall performance of git-filter-branch: bash loops iterating over core executables probably isn't ideal, but there are lower-hanging fruits. Running the filter in a single process may allow some better caching and reduce duplication of work (maybe parallelization?), but I'm just guessing.
Our tree is quite large, but the O(n^2) nature of this process is pretty crippling for the larger repositories that are bound to be in the wild. And while filter-branch isn't an everyday thing, when I /do/ need to use it, I won't be able to wait a week. :-)
I'd appreciate any feedback or suggestions anyone might have!
Thanks,
Ken
PS: On an unrelated note, I would recommend that the following code in git-filter-branch:
277:rev_args=$(git rev-parse --revs-only "$@")
be changed to write out to a temporary file, then piped into the "git rev-list" at line 289 with "--stdin". For larger trees, the use of $rev_args on the "git rev-list" command-line exceeds the size of some shells' command-line buffers (from direct experience).
^ permalink raw reply
* Re: [ANNOUNCE] Git in the SFC
From: Erik Faye-Lund @ 2010-12-17 1:02 UTC (permalink / raw)
To: Jeff King; +Cc: git, bkuhn
In-Reply-To: <20101217002034.GA18648@sigill.intra.peff.net>
On Fri, Dec 17, 2010 at 1:20 AM, Jeff King <peff@peff.net> wrote:
> I'm pleased to announce that Git is now a member project of the Software
> Freedom Conservancy (SFC). The SFC is a not-for-profit organization that
> provides financial and administrative assistance to open source
> projects.
>
> Among other things, the SFC will now handle any project money that Git
> receives (e.g., Google Summer of Code money). They will also accept
> tax-deductible donations on behalf of git that will go to git's project
> fund.
>
> For more information, see Git's SFC page:
>
> http://git-scm.com/sfc
>
I see there's donation-buttons for google checkout and paypal. Would
it be possible to donate through flattr also?
^ permalink raw reply
* Re: [ANNOUNCE] Git in the SFC
From: Jeff King @ 2010-12-17 0:33 UTC (permalink / raw)
To: git; +Cc: bkuhn
In-Reply-To: <20101217002034.GA18648@sigill.intra.peff.net>
I kept the announcement to the minimum, but a few follow-on points for
the community:
On Thu, Dec 16, 2010 at 07:20:35PM -0500, Jeff King wrote:
> I'm pleased to announce that Git is now a member project of the Software
> Freedom Conservancy (SFC). The SFC is a not-for-profit organization that
> provides financial and administrative assistance to open source
> projects.
In case you are wondering, this has basically no bearing on the
development of git code. We are not assigning copyrights to the SFC
(though we could if we so chose), and the only real requirement it makes
on the code is that we continue to develop as an open-source project.
This just gives us a legal entity for doing any monetary things, or
handling contracts or license enforcement should it ever be necessary.
There is a committee of liaisons to the Conservancy consisting of Junio,
Shawn Pearce, and myself. Doing anything (e.g., spending money we have
in our account) requires a majority vote of the committee. The committee
can be expanded with new members by a vote of the existing committee.
All of that is in our contract with the SFC. There's no official
procedure for doing something like holding a periodic community-wide
election. If somebody wants to organize such a thing, I'm sure the
committee would be happy to recognize the results by adding in whoever
wins the election. For starters, we went with the simplest thing.
> Among other things, the SFC will now handle any project money that Git
> receives (e.g., Google Summer of Code money). They will also accept
> tax-deductible donations on behalf of git that will go to git's project
> fund.
Note that we don't really have a planned use for any money. In the past
it has mostly gone to helping developers (especially students with no
money) make it to the GitTogether. If you do feel like donating money,
you might also consider making a suggestion of what to do with it. :)
You might also consider donating directly to the SFC. Their mission is
good, and git indirectly benefits by having access to their services.
For donations earmarked for git, we give the SFC 10%.
If you have any other questions about what this means for git, or
suggestions on how it could or should impact the community, feel free to
ask. I probably won't have good answers, but Bradley Kuhn from the
Conservancy (cc'd) will.
-Peff
^ permalink raw reply
* [ANNOUNCE] Git in the SFC
From: Jeff King @ 2010-12-17 0:20 UTC (permalink / raw)
To: git; +Cc: bkuhn
I'm pleased to announce that Git is now a member project of the Software
Freedom Conservancy (SFC). The SFC is a not-for-profit organization that
provides financial and administrative assistance to open source
projects.
Among other things, the SFC will now handle any project money that Git
receives (e.g., Google Summer of Code money). They will also accept
tax-deductible donations on behalf of git that will go to git's project
fund.
For more information, see Git's SFC page:
http://git-scm.com/sfc
or the SFC's website:
http://sfconservancy.org
-Peff
^ permalink raw reply
* Re: [PATCH 19/21] grep: use writable strbuf from caller in grep_tree()
From: Junio C Hamano @ 2010-12-17 0:15 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1292425376-14550-20-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> + hit = grep_tree(opt, pathspec, &tree, &base, base.neglen);
If you are going to let the users of strbuf API to refer directly to the
field, I think "neglen" should be renamed to something more reasonable,
say, "offset".
I am still debating myself if this strbuf_offset is anugly hack merely to
allow the implementation of "grep" not to carry one extra offset around
throughout its callchain, or if it is generic enough that other/future
callers would benefit from. I am leaning toward to think this is an ugly
hack, as a new caller that wants to carry _two_ offsets into a strbuf
wouldn't get much benefit from this new API. But I may be missreading
your code.
^ permalink raw reply
* Re: [PATCH 14/21] Convert ce_path_match() to use struct pathspec
From: Junio C Hamano @ 2010-12-17 0:02 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1292425376-14550-15-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> diff --git a/diff-lib.c b/diff-lib.c
> index 3b809f2..63db7f4 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -89,9 +89,11 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
> int silent_on_removed = option & DIFF_SILENT_ON_REMOVED;
> unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
> ? CE_MATCH_RACY_IS_DIRTY : 0);
> + struct pathspec pathspec;
>
> diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
>
> + init_pathspec(&pathspec, revs->prune_data);
I wonder if it makes more sense to change the type of revs->prune_data
from an array of pointers to strings to a pointer to struct pathspec.
Is there a downside?
> diff --git a/preload-index.c b/preload-index.c
> index e3d0bda..49cb08d 100644
> --- a/preload-index.c
> +++ b/preload-index.c
> @@ -35,7 +35,9 @@ static void *preload_thread(void *_data)
> struct index_state *index = p->index;
> struct cache_entry **cep = index->cache + p->offset;
> struct cache_def cache;
> + struct pathspec pathspec;
>
> + init_pathspec(&pathspec, p->pathspec);
Likewise; would it make the API cleaner to make read_cache_preload() and
read_index_preload() take "const struct pathspec *" instead of the
traditional "const char **"?
^ permalink raw reply
* Re: why the html and man versions of git-diff are different?
From: Leo @ 2010-12-16 23:52 UTC (permalink / raw)
To: git
In-Reply-To: <20101216215506.GB10480@sigill.intra.peff.net>
On 2010-12-16 21:55 +0000, Jeff King wrote:
> On Thu, Dec 16, 2010 at 09:43:45PM +0000, Leo wrote:
>
>> I compared the html version of git-diff found in
>> http://www.kernel.org/pub/software/scm/git/docs/git-diff.html and the
>> man version from
>> http://kernel.org/pub/software/scm/git/git-manpages-1.7.3.4.tar.bz2
>>
>> The man version says gitrevisions(1), which is incorrect since it is in
>> man7.
>
> Because it was fixed recently, and the html documention on kernel.org
> uses the "master" branch, whereas that commit did not make it into the
> maintenance release 1.7.3.4. It will be fixed in the manpages for
> git-1.7.4.
>
> -Peff
Sorry my bad it was fixed in 1.7.3.4.
Leo
^ permalink raw reply
* Re: [PATCH] trace.c: mark file-local function static
From: Thiago Farina @ 2010-12-16 23:43 UTC (permalink / raw)
To: Vasyl'; +Cc: git
In-Reply-To: <AANLkTinxJdASW6mQVU50grA2mUz6gt+gUND30VRK=BCN@mail.gmail.com>
On Thu, Dec 16, 2010 at 8:38 PM, Vasyl' <vvavrychuk@gmail.com> wrote:
> Signed-off-by: Vasyl' Vavrychuk <vvavrychuk@gmail.com>
> ---
> trace.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/trace.c b/trace.c
> index 1e560cb..62586fa 100644
> --- a/trace.c
> +++ b/trace.c
> @@ -25,7 +25,7 @@
> #include "cache.h"
> #include "quote.h"
>
> -void do_nothing(size_t unused)
> +static void do_nothing(size_t unused)
> {
> }
>
If it means something, this looks sane to me.
Acked-by: Thiago Farina <tfransosi@gmail.com>
> --
> 1.7.1
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH 10/21] tree_entry_interesting(): fix depth limit with overlapping pathspecs
From: Junio C Hamano @ 2010-12-16 23:31 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1292425376-14550-11-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Suppose we have two pathspecs 'a' and 'a/b' (both are dirs) and depth
> limit 1. In current code, pathspecs are checked in input order. When
> 'a/b' is checked against pathspec 'a', it fails depth limit and
> therefore is excluded, although it should match 'a/b' pathspec.
>
> This patch reorders all pathspecs alphabetically, then teaches
> tree_entry_interesting() to check against the deepest pathspec first,
> so depth limit of a shallower pathspec won't affect a deeper one.
I am quite happy to see where this new round of this series is going so
far. I however doubt this patch is a right approach for the problem you
are solving, especially because your longer-term (i.e. toward the rest of
the series to patch 21) plan is to allow wildcards [*1*].
One thing I am not clear is what it means to limit the recursion level
when you have wildcards.
One possible definition of interaction between limit and wildcard may be
to count the number of slashes in the part of the path that matches the
wildcarded part of the pathspec, add the number of path components
appended due to the leading directory match, and then subtract the number
of literal slashes in the wildcarded part of the pattern from the above,
and declare that a match is found if the difference is less than the
limit.
E.g. a pathspec element "a/*/x" would match "a/b/c/x", "a/b/c/d/e/x",
"a/b/x/y" and "a/b/x/y/z" without limit, and with the limit of 1:
a/b/c/x matches ('*' expands to "b/c")
a/b/c/d/e/x no ('*' has to expand to "c/d/e" and needs 2 levels)
a/b/x/y matches ('*' expands to "b" costing zero, "/y" needs 1)
a/b/x/y/z does not match
Another definition could be to count _only_ the part that is appended by
recursion (i.e. we do not count how many slashes has to match '*' in the
above examples), and as the option is called --depth, it might make more
sense.
In either case, I am not sure if "if it matches the longest pathspec, we
have the answer without looking at shorter ones" would be a good rule to
use.
[Footnote]
*1* In addition, perhaps you may later want to introduce some "negative"
match operators to pathspecs; while I am not particularly fond of that
direction at this moment, I would like to leave the door open for that
possibility, in case it turns out to be a good thing to have.
^ permalink raw reply
* Re: How to unpack recent objects?
From: Nicolas Pitre @ 2010-12-16 23:12 UTC (permalink / raw)
To: Phillip Susi; +Cc: git
In-Reply-To: <4D0A8D83.9080705@cfl.rr.com>
On Thu, 16 Dec 2010, Phillip Susi wrote:
> On 12/16/2010 4:19 PM, Nicolas Pitre wrote:
> > What makes you think that unpacking them will actually make the access
> > to them faster? Instead, you should consider _repacking_ them,
> > ultimately using the --aggressive parameter with the gc command, if you
> > want faster accesses.
>
> Because decompressing and undeltifying the objects in the pack file
> takes a fair amount of cpu time. It seems a waste to do this for the
> same set of objects repeatedly rather than just keeping them loose.
Well, here are a couple implementation details you might not know about:
1) Loose objects are compressed too. So you gain nothing on that front
by keeping objects loose.
2) Delta ordering is so that recent objects, i.e. those belonging to
most recent commits, are not delta compressed but rather used as base
objects for "older" objects to delta against. So in practice, the
cost of undeltifying objects is pushed towards objects that you're
most unlikely to access frequently.
3) Object placement within the pack is also optimized so that
objects belonging to recent commits are close together, and walking
them creates a linear IO access pattern which is much faster than
accessing random individual files as loose objects are.
4) Packed objects take considerably less space than loose ones which
makes for much better usage of the file system cache in the operating
system. This largely outweights the cost of undeltifying objects.
5) Git also keeps a cache of most frequently referenced objects when
replaying delta chains so deep deltas don't bring exponential costs.
And, in some cases, Git does even pick up the content of an object by
using its checked out form in the working directory directly instead of
locating and decompressing the object data.
So you shouldn't have to worry on that front. Git is not the fastest
SCM out there just by luck.
Nicolas
^ permalink raw reply
* Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin
From: Erik Faye-Lund @ 2010-12-16 23:06 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list, j6t, jrnieder, vmiklos
In-Reply-To: <4D0A7E87.3020203@ramsay1.demon.co.uk>
On Thu, Dec 16, 2010 at 10:03 PM, Ramsay Jones
<ramsay@ramsay1.demon.co.uk> wrote:
> Erik Faye-Lund wrote:
>>> diff --git a/help.c b/help.c
>>> index 7f4928e..eabadc9 100644
>>> --- a/help.c
>>> +++ b/help.c
>>> @@ -126,7 +126,10 @@ static int is_executable(const char *name)
>>> !S_ISREG(st.st_mode))
>>> return 0;
>>>
>>> -#ifdef WIN32
>>> +#if defined(WIN32) || defined(__CYGWIN__)
>>> +#if defined(__CYGWIN__)
>>> +if ((st.st_mode & S_IXUSR) == 0)
>>> +#endif
>>
>> Perhaps the first check should simply be changed to check for _WIN32
>> instead of WIN32? IIRC _WIN32 is set on Cygwin, but I could be
>> mistaken...
>
> No, neither WIN32 or _WIN32 will be defined here (and they should not be).
> It's actually quite tricky, particularly when #including <windows.h>, viz:
>
> $ cat -n test.c
> 1 #include <stdio.h>
> 2
> 3 #ifdef IW
> 4 # include <windows.h>
> 5 #endif
> 6
> 7 int main(int argc, char *argv[])
> 8 {
> 9 #ifdef WIN32
> 10 printf("WIN32 ");
> 11 #endif
> 12 #ifdef _WIN32
> 13 printf("_WIN32 ");
> 14 #endif
> 15 #ifdef __CYGWIN__
> 16 printf("__CYGWIN__ ");
> 17 #endif
> 18 #ifdef __MINGW32__
> 19 printf("__MINGW32__ ");
> 20 #endif
> 21 printf("\n");
> 22 return 0;
> 23 }
>
> $ gcc -o test test.c
> $ ./test
> __CYGWIN__
>
> $ gcc -o test -DIW test.c
> $ ./test
> WIN32 _WIN32 __CYGWIN__
>
> $ gcc -o test -mno-cygwin test.c
> $ ./test
> WIN32 _WIN32 __MINGW32__
>
Hmm, I thought _WIN32 was always defined when targeting Windows, and
that WIN32 was defined when windows.h was included (or usually in the
preprocessor flags when compiling GUI programs) - that's what MSVC
does anyway. MinGW seems to always define both _WIN32 and WIN32, but
as you've shown Cygwin doesn't define either until windows.h is
included.
So sorry for leading you onto the wrong path, a check for __CYGWIN__
seems to be necessary indeed.
^ permalink raw reply
* Re: [PATCH 3/3] gitweb: remove test when closing file descriptor
From: Jakub Narebski @ 2010-11-16 23:02 UTC (permalink / raw)
To: Sylvain Rabot; +Cc: git
In-Reply-To: <1292539020.2511.6.camel@kheops>
On Thu, 16 Dec 2010, Sylvain Rabot wrote:
> On Thu, 2010-12-16 at 14:30 -0800, Jakub Narebski wrote:
> > Sylvain Rabot <sylvain@abstraction.fr> writes:
> >
> > > it happens that closing file descriptor fails whereas
> > > the blob is perfectly readable.
> > >
> > > Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
> > > ---
[...]
> > > - close $fd
> > > - or die_error(404, "Reading blob failed");
> > > + close $fd;
[...]
> > In this place we close read end of pipe after at most reading a few
> > bytes (what -T test does), so what might happen is that writer got
> > SIGPIPE and "failed". But we are not interested in this, so we can
> > safely ignore return from 'close'.
> >
> > I think that some from above explanation should make it to commit
> > message.
>
> Thanks for explaining it, I did not have any idea why it was failing.
> Can I use your explanation as commit message ?
Of course.
Though I am not sure if it is really the reason, it looks reasonable.
--
Jakub Narebski
Poland
^ permalink raw reply
* [PATCH] trace.c: mark file-local function static
From: Vasyl' @ 2010-12-16 22:38 UTC (permalink / raw)
To: git
Signed-off-by: Vasyl' Vavrychuk <vvavrychuk@gmail.com>
---
trace.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/trace.c b/trace.c
index 1e560cb..62586fa 100644
--- a/trace.c
+++ b/trace.c
@@ -25,7 +25,7 @@
#include "cache.h"
#include "quote.h"
-void do_nothing(size_t unused)
+static void do_nothing(size_t unused)
{
}
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 13/14] t4135-*.sh: Skip the "backslash" tests on cygwin
From: Ramsay Jones @ 2010-12-16 22:38 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, GIT Mailing-list, jrnieder
In-Reply-To: <201012142149.33725.j6t@kdbg.org>
Johannes Sixt wrote:
> On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:
>> Note t3700-*.sh has a test protected by BSLASHSPEC which
>> previously passed on cygwin and will now be (unnecessarily)
>> skipped. This test needs to be skipped on MinGW, given how
>> it is written; if you remove the single quotes around the
>> filename, however, it will pass even on MinGW.
>
> That is suspicious. It would mean that git add does not do file globbing
> anymore. Should it or should it not do file globbing?
Hmm, something like "git add 'a.[ch]'" works just fine. The problems
occur when you back-quote the metachars like "git add 'a.\[ch\]'".
The test is skipped on MinGW, because of BSLASHSPEC. The test is now
skipped on cygwin, after this patch, even though it passes on cygwin.
BSLASHSPEC is, apparently, used for both a '\' in a filename and for
a "\-quoting". (Perhaps it should be split into two prerequisites)
The difference in behaviour between cygwin and MinGW (& msvc) is easy
to trace, thus:
cmd_add()
=>validate_pathspec() builtin/add.c:435
=>get_pathspec() builtin/add.c:216
=>prefix_path() setup.c:147
=>normalize_path_copy() setup.c:18
=>is_dir_sep()
on cygwin is_dir_sep() is defined thus:
git-compat-util.h:208:#define is_dir_sep(c) ((c) == '/')
where on MinGW it is defined thus:
compat/mingw.h:291:#define is_dir_sep(c) ((c) == '/' || (c) == '\\')
So, on entry to git-add the pathspec (in argv[1]) is
fo\[ou\]bar
On return from validate_pathspec(), on cygwin it is *still*
fo\[ou\]bar
but on MinGW (and msvc), it is now
fo/[ou/]bar
and everything follows from there. (So for example, on cygwin, match_one()
matches fo\[ou\]bar with fo[ou]bar, but not with foobar.)
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 11/14] t3032-*.sh: Pass the -b (--binary) option to sed on cygwin
From: Ramsay Jones @ 2010-12-16 21:19 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Johannes Sixt, Junio C Hamano, GIT Mailing-list, Pat Thoyts
In-Reply-To: <4D088AB6.5090501@sunshineco.com>
Eric Sunshine wrote:
> On 12/15/2010 3:22 AM, Johannes Sixt wrote:
>> Am 12/15/2010 0:32, schrieb Eric Sunshine:
>>> On 12/14/2010 2:24 PM, Junio C Hamano wrote:
>>>> Ramsay Jones<ramsay@ramsay1.demon.co.uk> writes:
>>>>> The test using the conflict_hunks helper function (test 9) fails
>>>>> on cygwin, since sed (by default) throws away the CR from CRLF
>>>>> line endings. This behaviour is undesirable, since the validation
>>>>> code expects the CRLF line-ending to be present. In order to fix
>>>>> the problem we pass the -b (--binary) option to sed, using the
>>>>> SED_OPTIONS variable. We use the SED_STRIPS_CR prerequisite in the
>>>>> conditional initialisation of SED_OPTIONS.
>>>>>
>>>>> Signed-off-by: Ramsay Jones<ramsay@ramsay1.demon.co.uk>
>>>>> ---
>>>>>
>>>>> Note that this test does not fail on MinGW, but I don't
>>>>> really know why, given commit ca02ad3... ahem ;-)
>>>> Ahem, indeed. Why?
>>> t3032 does indeed fail on MinGW, and was fixed in the msysgit port by [1],
>>> but was subsequently "lost" when msysgit was rebased onto junio/next [2]
>>> which did not have that test. Consequently, the fix never made it into the
>>> mainline git source.
>> Sorry, but on MinGW, I only need the GREP_OPTIONS part of that fix, but
>> not the SED_OPTIONS. It's also mysterious for me.
>>
>> OTOH, the fix in ca02ad3 that applies to t6038, does not work for me as is
>> because my sed does not understand -b; it needs --nocr. Maybe it is the
>> sed version that makes the difference?
>>
>> D:\Src\mingw-git\t>sed --version
>> GNU sed version 3.02
>
> Failure of t3032 was reported by Pat Thoyts [1] when preparing for the
> v1.7.3 release. The problem was diagnosed and patched via [2] under the
> standard msysgit netinstall [3] environment. From commit message [2],
> GREP_OPTIONS and SED_OPTIONS were applied to resolve distinct cases of
> line-terminator "corruption" (t3032.4-t3032.8 and t3032.9, respectively)
> within that environment at the time the patch was prepared.
>
> Your tool versions may indeed not be compatible with those of the
> netinstall environment [3]:
>
> $ sed --version
> GNU sed version 4.2.1
>
> Unfortunately, the old --nocr is not recognized by modern GNU sed:
>
> $ sed --nocr
> sed: unrecognized option `--nocr'
Yes. Like Johannes, I have sed version 3.02 on MinGW, but on cygwin
I have sed version 4.1.5. See patch #14, where I introduce the
SED_BIN_OPT variable to allow me to run the tests with SED_OPTIONS
set to -c instead of -b.
[I thought I was unusual in having such an old sed version, but
apparently not... ;-) ]
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin
From: Ramsay Jones @ 2010-12-16 21:12 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, GIT Mailing-list, jrnieder, vmiklos
In-Reply-To: <201012142138.37679.j6t@kdbg.org>
Johannes Sixt wrote:
> On Dienstag, 14. Dezember 2010, Ramsay Jones wrote:
>> @@ -126,7 +126,10 @@ static int is_executable(const char *name)
>> !S_ISREG(st.st_mode))
>> return 0;
>>
>> -#ifdef WIN32
>> +#if defined(WIN32) || defined(__CYGWIN__)
>> +#if defined(__CYGWIN__)
>> +if ((st.st_mode & S_IXUSR) == 0)
>> +#endif
>> { /* cannot trust the executable bit, peek into the file instead */
>> char buf[3] = { 0 };
>> int n;
>
> Do you gain a lot by this extra condition? Wouldn't
>
> -#ifdef WIN32
> +#if defined(WIN32) || defined(__CYGWIN__)
>
> be sufficient?
Yes, that would be sufficient. No, I probably don't gain a great deal
(but I have *not* timed it), since the number of files that are tested
by is_executable() is fairly low anyway since they are already filtered
by a filename prefix (eg. git-merge-).
However, if the executable bit is set, then executing the WIN32 code
block is wasted effort (we already know the answer), so why bother?
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 08/14] help.c: Fix detection of custom merge strategy on cygwin
From: Ramsay Jones @ 2010-12-16 21:03 UTC (permalink / raw)
To: kusmabite; +Cc: Junio C Hamano, GIT Mailing-list, j6t, jrnieder, vmiklos
In-Reply-To: <AANLkTimt3w9GVCXa_n1_HXivyRmnRyUhhdSArrHOT6fs@mail.gmail.com>
Erik Faye-Lund wrote:
>> diff --git a/help.c b/help.c
>> index 7f4928e..eabadc9 100644
>> --- a/help.c
>> +++ b/help.c
>> @@ -126,7 +126,10 @@ static int is_executable(const char *name)
>> !S_ISREG(st.st_mode))
>> return 0;
>>
>> -#ifdef WIN32
>> +#if defined(WIN32) || defined(__CYGWIN__)
>> +#if defined(__CYGWIN__)
>> +if ((st.st_mode & S_IXUSR) == 0)
>> +#endif
>
> Perhaps the first check should simply be changed to check for _WIN32
> instead of WIN32? IIRC _WIN32 is set on Cygwin, but I could be
> mistaken...
No, neither WIN32 or _WIN32 will be defined here (and they should not be).
It's actually quite tricky, particularly when #including <windows.h>, viz:
$ cat -n test.c
1 #include <stdio.h>
2
3 #ifdef IW
4 # include <windows.h>
5 #endif
6
7 int main(int argc, char *argv[])
8 {
9 #ifdef WIN32
10 printf("WIN32 ");
11 #endif
12 #ifdef _WIN32
13 printf("_WIN32 ");
14 #endif
15 #ifdef __CYGWIN__
16 printf("__CYGWIN__ ");
17 #endif
18 #ifdef __MINGW32__
19 printf("__MINGW32__ ");
20 #endif
21 printf("\n");
22 return 0;
23 }
$ gcc -o test test.c
$ ./test
__CYGWIN__
$ gcc -o test -DIW test.c
$ ./test
WIN32 _WIN32 __CYGWIN__
$ gcc -o test -mno-cygwin test.c
$ ./test
WIN32 _WIN32 __MINGW32__
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 14/14] t{3032,6038}-*.sh: Allow SED_BIN_OPT to override the -b sed option
From: Ramsay Jones @ 2010-12-16 20:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: GIT Mailing-list
In-Reply-To: <7vlj3stamh.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
>>
>> -test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b
>> +test_have_prereq SED_STRIPS_CR && SED_OPTIONS=${SED_BIN_OPT--b}
>
> It is unclear who is supposed to feed you SED_BIN_OPT.
>
> Perhaps a patch to add an insn like "If you have an ancient sed, export
> SED_BIN_OPT=-c before running tests" to t/README is also necessary to go
> together with this change.
Yes, you are right. I had such a change to t/README but it was mixed in with
another patch to document other such variables which was incomplete. (see my
previous mail in response to your comments on patch 4/14).
Also, I was half expecting you to drop the patch, because I thought I was in
the minority with an old sed; however, it seems I'm not that unusual after
all ... ;-)
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 04/14] lib-git-svn.sh: Add check for missing mod_dav_svn module
From: Ramsay Jones @ 2010-12-16 20:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: GIT Mailing-list, normalperson, avarab
In-Reply-To: <7v8vzsuq48.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
>> diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
>> index 919d45a..154f3d3 100644
>> --- a/t/lib-git-svn.sh
>> +++ b/t/lib-git-svn.sh
>> @@ -101,6 +101,11 @@ start_httpd () {
>> echo >&2 'SVN_HTTPD_PORT is not defined!'
>> return
>> fi
>> + if test ! -e "$SVN_HTTPD_MODULE_PATH/mod_dav_svn.so"
>> + then
>> + echo >&2 'Apache module "mod_dav_svn.so" not found'
>> + return 1
>> + fi
>
> Others seem to check with "test -f" for things like this.
OK, do you want me to send an updated patch?
> Also why "return 1" only on this codepath?
This test, along with t9115-*.sh, t9118-*.sh and t9120-*.sh, have
been written in such a way that it can access the svn repo using
either file or http urls. By default (SVN_HTTPD_PORT not set), it
uses file urls so that, despite it noisily printing to stderr that
"SVN_HTTPD_PORT is not defined!", lack of an Apache installation is
not a problem. (In fact, I suspect very few people run it with
SVN_HTTPD_PORT defined).
So, the initial test (of SVN_HTTPD_PORT) and return is not particularly
noteworthy, let alone an error. (Yes I have a patch to remove the
message, see below). However, *if* the user requests the test be run
with http urls and the mod_sav_svn.so module is missing, then we want
to fail the test noisily; thus we return 1.
Note that we could easily reset SVN_HTTPD_PORT and return, thus silently
converting to the "use file urls" case, but I don't think that would be
acceptable.
The reason I didn't submit the patch to remove the above message is
because I felt I should add a note about how/why you would use the
SVN_HTTPD_PORT variable to the t/README file. However, that started
me thinking about other similar variables like:
SVN_HTTPD_{PATH,MODULE_PATH}, LIB_HTTPD_{PATH,PORT,SSL,DAV,SVN},
GIT_SVN_NO_OPTIMIZE_COMMITS, GIT_TEST_HTTPD, .....
So, that patch never got finished! :-P
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 07/14] t7800-difftool.sh: Fix a test failure on Cygwin
From: Ramsay Jones @ 2010-12-16 20:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: GIT Mailing-list, davvid
In-Reply-To: <7v39q0upw7.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> This is not just "test failure" but can cause a failure in the real life
> for people who want "the last flag wins" to work, no?
Yes, indeed. I was concentrating on fixing test-suite failures (and that
can mean you have to modify the test, the application or both) so...
> I'll retitle the patch and queue it separately from other test fixes.
Thanks!
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 06/14] t9501-*.sh: Fix a test failure on Cygwin
From: Ramsay Jones @ 2010-12-16 19:25 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, GIT Mailing-list, marada
In-Reply-To: <201012142004.30322.jnareb@gmail.com>
Jakub Narebski wrote:
> Ramsay Jones wrote:
>> The first (setup) test attempts to create a file, using the
>> test_commit function, called 'i can has snapshot?'. On cygwin
>> (and MinGW) this fails with a "No such file or directory" error.
>> In order to fix the tests, we simply remove the '?' wildcard
>> from the name, since the purpose of these tests is not about
>> creating funny filenames.
>>
>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>
> All right.
>
> Acked-by: Jakub Narebski <jnareb@gmail.com>
Thanks.
> BTW. if the test was about handling funny filenames (containing
> leading, embedded and trailing space, +&@=<>"' characters), what
> should we do instead?
Hmm... dunno; take a look at t3600-rm.sh and t4135-apply-weird-filenames.sh
for inspiration? ;-)
Just FYI the "illegal" pathname characters in windows include:
< > : " / \ | ? *
along with control chars from 0 - 31.
Also, you can't have any trailing spaces or periods.
ATB,
Ramsay Jones
^ 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