* Re: [PATCH] t: don't redefine test_config() in various places
From: Jonathan Nieder @ 2013-03-20 18:40 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1363767703-12936-1-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
> Thanks, Junio.
>
> t/t4018-diff-funcname.sh | 5 -----
> t/t7810-grep.sh | 5 -----
> t/t7811-grep-open.sh | 5 -----
> 3 files changed, 15 deletions(-)
Yeah, that looks like all of them. FWIW,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [RFC] Add posibility to preload stat information.
From: Fredrik Gustafsson @ 2013-03-20 18:40 UTC (permalink / raw)
To: Jeff King; +Cc: Fredrik Gustafsson, spearce, git, pclouds
In-Reply-To: <20130320164806.GA10752@sigill.intra.peff.net>
On Wed, Mar 20, 2013 at 12:48:06PM -0400, Jeff King wrote:
> Kind of gross, but I guess it is useful to some people.
Yes it is. The questions is if it's gross enough to never
leave my computer, or if someone else can find this useful.
>
> > +__git_recursive_stat ()
> > +{
> > + if test ! -e /tmp/gitbash.lock
> > + then
> > + touch /tmp/gitbash.lock
>
> This is a tmp-race security hole. E.g., do:
>
> ln -s /etc/nologin /tmp/gitbash.lock
>
> as a user; when root runs __git_recursive_stat, it will create
> /etc/nologin. It's not quite as bad as some other holes, because we only
> "touch" the file, not overwrite its contents, but you can see that it's
> possible to do some mischief.
>
> Should this maybe just be ~/.gitbash.lock or something?
Thank you! I totally missed that.
I guess a new solution would be to keep an access time-stamp in each
repository and with certain intervall run git status on that repository.
--
Med vänliga hälsningar
Fredrik Gustafsson
tel: 0733-608274
e-post: iveqy@iveqy.com
^ permalink raw reply
* Re: [PATCH 3/6] t5516 (fetch-push): introduce mk_test_with_name()
From: Ramkumar Ramachandra @ 2013-03-20 18:38 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano, Jeff King, Eric Sunshine
In-Reply-To: <20130320182830.GJ3655@google.com>
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> mk_test() creates a repository with the constant name "testrepo", and
>> this may be limiting for tests that need to create more than one
>> repository for testing. To fix this, create a new mk_test_with_name()
>> which accepts the repository name as $1. Reimplement mk_test() as a
>> special case of this function, making sure that no tests need to be
>> rewritten.
>
> Why not give mk_test an optional parameter?
>
> repo_name=${1:-testrepo}
>
> Oh, it is because mk_test already takes arguments naming refs to push.
> I suppose the change description could make this clearer.
Isn't it obvious?
> Why not use mk_test and then rename, like so?
>
> mk_test ...refs... &&
> mv testrepo testrepo-a &&
>
> mk_test ...refs... &&
> mv testrepo testrepo-b &&
> ...
No. This is ugly. mk_test() should not hardcode "testrepo".
> I dunno. The helper functions at the top of this test are already
> intimidating, so I guess I am looking for a way to avoid making that
> problem worse. One way would be to add an opening comment before
> the function definition explaining how it is meant to be used. See
> t/test-lib-functions.sh for examples, such as test_cmp.
My patch does not make the situation worse in any way: it just adds
one line that passes $1 as a parameter to existing code. Yes, the
functions and tests can be improved greatly, but I refrained from
doing so because of your series. We can save it for later.
^ permalink raw reply
* Re: [PATCH 2/6] t5516 (fetch-push): update test description
From: Jonathan Nieder @ 2013-03-20 18:35 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano, Jeff King, Eric Sunshine
In-Reply-To: <CALkWK0nu5oMw-+uh8mLRUOyAFoETUpKZfYuO2LLJAgF0+XHpRA@mail.gmail.com>
Ramkumar Ramachandra wrote:
> When I want to add a test for branch.<name>.pushremote, I grep
> for branch.*.pushurl, and open files with sensible names; I'm not
> going to open up the file and read a long description of what tests it
> already contains.
Huh? The test_description is output for "./t5516-* --help" and is
supposed to help people hacking on the test to understand its setup
and its purpose.
^ permalink raw reply
* Re: [RFC] Add posibility to preload stat information.
From: Fredrik Gustafsson @ 2013-03-20 18:36 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Fredrik Gustafsson, spearce, git, pclouds, Jeff King
In-Reply-To: <CALkWK0nCY3o_KW8ykq9TkzfHeVj6NKDkOFuym9UmSWLH0ZFxBw@mail.gmail.com>
On Wed, Mar 20, 2013 at 11:19:38PM +0530, Ramkumar Ramachandra wrote:
> I think it should be a separate script in contrib/ that people can
> just `eval` in their shell configs; zsh has a chpwd() function for
> example, which seems like the right place to put such a thing.
I was trying to spare the number of calls to "git rev-parse
--is-inside-work-tree". But maybe that is to fast to care about.
> No, I meant in the commit message.
Okay, thanks.
> >> That said, this feature is extremely gross; it thrashes my filesystem
> >> and hard drive. Modern software is written to minimize IO, not
> >> maximize it! I'm completely against the inclusion of this patch.
> >
> > It's extremly gross. I don't like this, _but_ it does speed up my work.
> > I'm unsure if it should be included in git though (hence the RFC-tag).
>
> Yes, I would certainly like my git startup time to be improved. But I
> don't want to trade my hard drive's life for it.
Does this really increase disk-reads? The fs-cache would make sure that
the disk reads is almost the same, we only do them before we usually do
them.
> What I meant by "first time" is "chpwd() into the git repository, not
> further chpwd()s when already inside the git repository".
That's a good point. I'm not sure how to solve that though. Because it's
not a fact that you always go to the root git-dir first.
The only way I see this is with a lock-file that's kept and we only run
git status every 5 minutes when doing something inside a work dir. That
would add a lot of meta-data (the lock files), to store. (I hope I
successfully explained that).
--
Med vänliga hälsningar
Fredrik Gustafsson
tel: 0733-608274
e-post: iveqy@iveqy.com
^ permalink raw reply
* Re: [PATCH 2/6] t5516 (fetch-push): update test description
From: Ramkumar Ramachandra @ 2013-03-20 18:33 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano, Jeff King, Eric Sunshine
In-Reply-To: <20130320182213.GI3655@google.com>
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> --- a/t/t5516-fetch-push.sh
>> +++ b/t/t5516-fetch-push.sh
>> @@ -1,6 +1,6 @@
>> #!/bin/sh
>>
>> -test_description='fetching and pushing, with or without wildcard'
>> +test_description='fetching and pushing'
>
> I'm not thrilled with the description before or after. Would it make
> sense to do something like the following?
>
> test_description='Tests of basic fetch/push functionality.
>
> These tests create small test repositories and fetch from and
> push to them, testing:
>
> * commandline syntax
> * refspecs and default refspecs
> * fast-forward detection and overriding fast-forward detection
> * configuration (insteadOf, pushInsteadOf, [remote "name"] push,
> etc)
> * hooks
> * --porcelain output format
> * hiderefs
> '
No. When I want to add a test for branch.<name>.pushremote, I grep
for branch.*.pushurl, and open files with sensible names; I'm not
going to open up the file and read a long description of what tests it
already contains. The filename and test headlines are sufficient.
Our test suite is bad enough as it is (inconsistent style, missing &&,
false positives)- I'm against adding to the maintenance burden.
^ permalink raw reply
* Re: [PATCH 5/6] remote.c: introduce remote.pushdefault
From: Jonathan Nieder @ 2013-03-20 18:32 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano, Jeff King, Eric Sunshine
In-Reply-To: <1363783501-27981-6-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> This new configuration variable defines the default remote to push to,
> and overrides `branch.<name>.remote` for all branches.
Micronit: I think this would be easier to explain if it came after
patch 6, since then you could say "In other words, it is a default for
branch.<name>.pushremote for all branches" and readers would not have
to wonder "Why does the more generic configuration override the more
specific one?".
My two cents,
Jonathan
^ permalink raw reply
* Re: USE_NSEC bug?
From: Jeff King @ 2013-03-20 18:30 UTC (permalink / raw)
To: Andrew Rodland; +Cc: Junio C Hamano, git
In-Reply-To: <CABFQKmONh2dQ+dkt3MsSXtFyKh5mnfK1fQVyw6Ez6aWajo=BDg@mail.gmail.com>
On Wed, Mar 20, 2013 at 02:22:27PM -0400, Andrew Rodland wrote:
> I think it would be good if NO_NSEC and USE_ST_TIMESPEC were
> controlled by configure instead of config.mak, and it doesn't seem
> like too tall of an order.
There is no "instead of" here. The Makefile provides knobs which you can
tweak on the command-line or via config.mak, and the configure script is
expected to set any of those knobs it wants. But as most of the regular
developers do not use the configure script themselves, it is often
missing support for many of the knobs.
So I think your real complaint is "configure does not set NO_NSEC
properly", which is something worth fixing.
-Peff
^ permalink raw reply
* Re: [PATCH 4/6] remote.c: introduce a way to have different remotes for fetch/push
From: Jonathan Nieder @ 2013-03-20 18:30 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano, Jeff King, Eric Sunshine
In-Reply-To: <1363783501-27981-5-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Currently, do_push() in push.c calls remote_get(), which gets the
> configured remote for fetching and pushing. Replace this call with a
> call to pushremote_get() instead, a new function that will return the
> remote configured specifically for pushing. This function tries to
> work with the string pushremote_name, before falling back to the
> codepath of remote_get(). This patch has no visible impact, but
> serves to enable future patches to introduce configuration variables
> to set this variable.
The above description does not make the impact of this change clear to
me. Could you give a before-and-after example? How will this
internal API change make my life easier as a developer?
Hope that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH 3/6] t5516 (fetch-push): introduce mk_test_with_name()
From: Jonathan Nieder @ 2013-03-20 18:28 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano, Jeff King, Eric Sunshine
In-Reply-To: <1363783501-27981-4-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> mk_test() creates a repository with the constant name "testrepo", and
> this may be limiting for tests that need to create more than one
> repository for testing. To fix this, create a new mk_test_with_name()
> which accepts the repository name as $1. Reimplement mk_test() as a
> special case of this function, making sure that no tests need to be
> rewritten.
Why not give mk_test an optional parameter?
repo_name=${1:-testrepo}
Oh, it is because mk_test already takes arguments naming refs to push.
I suppose the change description could make this clearer.
Why not use mk_test and then rename, like so?
mk_test ...refs... &&
mv testrepo testrepo-a &&
mk_test ...refs... &&
mv testrepo testrepo-b &&
...
I dunno. The helper functions at the top of this test are already
intimidating, so I guess I am looking for a way to avoid making that
problem worse. One way would be to add an opening comment before
the function definition explaining how it is meant to be used. See
t/test-lib-functions.sh for examples, such as test_cmp.
Hope that helps,
Jonathan
^ permalink raw reply
* Re: USE_NSEC bug?
From: Andrew Rodland @ 2013-03-20 18:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7v620mf05l.fsf@alter.siamese.dyndns.org>
On Wed, Mar 20, 2013 at 1:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> And the cost is that we have another Makefile knob people need to tweak
>> that would not otherwise need to be there. Which can be annoying, but is
>> also not that huge a cost to deal with (we might want to improve the
>> configure script or something, though).
>>
>> I admit I don't care too much either way.
>
> I don't, either ;-)
>
For what it's worth, I'm not really invested either; I just figured
that raising the issue was better than not, since I bumped into it.
I think it would be good if NO_NSEC and USE_ST_TIMESPEC were
controlled by configure instead of config.mak, and it doesn't seem
like too tall of an order.
Andrew
^ permalink raw reply
* Re: [PATCH 2/6] t5516 (fetch-push): update test description
From: Jonathan Nieder @ 2013-03-20 18:22 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano, Jeff King, Eric Sunshine
In-Reply-To: <1363783501-27981-3-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1,6 +1,6 @@
> #!/bin/sh
>
> -test_description='fetching and pushing, with or without wildcard'
> +test_description='fetching and pushing'
I'm not thrilled with the description before or after. Would it make
sense to do something like the following?
test_description='Tests of basic fetch/push functionality.
These tests create small test repositories and fetch from and
push to them, testing:
* commandline syntax
* refspecs and default refspecs
* fast-forward detection and overriding fast-forward detection
* configuration (insteadOf, pushInsteadOf, [remote "name"] push,
etc)
* hooks
* --porcelain output format
* hiderefs
'
^ permalink raw reply
* Re: git merge <tag> behavior
From: Yann Droneaud @ 2013-03-20 18:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git
In-Reply-To: <1363802682.6289.46.camel@test.quest-ce.net>
Hi,
Le mercredi 20 mars 2013 à 19:04 +0100, Yann Droneaud a écrit :
> > > 2) git merge <tag> VS git merge <object-id>
> > >
> > > If <tag> is an object (not a lightweight/reference tag), git merge <tag>
> > > ...
> > > But, if you use the tag object-id instead of its name, for example using
> > > git merge `git parse-rev <tag>`,
> [EDIT]
> > > signature is not checked. Git still create a merge commit, but doesn't
> > > prepare a commit message with the tag message and the signature:
> > >
> > > It would be great to have Git using the tag message and check the
> > > signature.
> >
> > Perhaps, but if you feed the $(git rev-parse v1.12.2) to merge, your
> > subject will not be able to say "Merge tag 'v1.12.2'" in the first
> > place, so I do not think you would want to encourage such usage in
> > the first place.
>
> I think if someone want to merge the tag object-id instead of the tag,
> the commit subject/message should probably not make a reference to the
> tag.
>
> The only use case for such tag merging by commit-id would be to get
> consistent behavior in case of tag deletion. The named tag could be
> recreated to point to another point in time. So when looking at the
> merge commit message and searching for the tag (by name) could be
> misleading.
>
But but do not take those remarks as a feature request.
I was just asking for clarification/comment on the behavior difference
between merging tag/tag object-id.
Regards
--
Yann Droneaud
OPTEYA
^ permalink raw reply
* Re: [PATCH 1/6] remote.c: simplify a bit of code using git_config_string()
From: Ramkumar Ramachandra @ 2013-03-20 18:12 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano, Jeff King, Eric Sunshine
In-Reply-To: <20130320180707.GH3655@google.com>
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -356,9 +356,7 @@ static int handle_config(const char *key, const char *value, void *cb)
>> return 0;
>> branch = make_branch(name, subkey - name);
>> if (!strcmp(subkey, ".remote")) {
>> - if (!value)
>> - return config_error_nonbool(key);
>> - branch->remote_name = xstrdup(value);
>> + git_config_string(&branch->remote_name, key, value);
>
> Shouldn't this say
>
> if (git_config_string(&branch->remote_name, key, value))
> return -1;
>
> or something?
Yes, and so should the instances in [5/6] and [6/6]. Thanks for catching it.
^ permalink raw reply
* Re: [PATCH 1/6] remote.c: simplify a bit of code using git_config_string()
From: Jonathan Nieder @ 2013-03-20 18:07 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano, Jeff King, Eric Sunshine
In-Reply-To: <1363783501-27981-2-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> --- a/remote.c
> +++ b/remote.c
> @@ -356,9 +356,7 @@ static int handle_config(const char *key, const char *value, void *cb)
> return 0;
> branch = make_branch(name, subkey - name);
> if (!strcmp(subkey, ".remote")) {
> - if (!value)
> - return config_error_nonbool(key);
> - branch->remote_name = xstrdup(value);
> + git_config_string(&branch->remote_name, key, value);
Shouldn't this say
if (git_config_string(&branch->remote_name, key, value))
return -1;
or something?
Thanks,
Jonathan
^ permalink raw reply
* Re: git merge <tag> behavior
From: Yann Droneaud @ 2013-03-20 18:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git
In-Reply-To: <7vfvzrjrad.fsf@alter.siamese.dyndns.org>
Hi,
Le mardi 19 mars 2013 à 09:19 -0700, Junio C Hamano a écrit :
> Yann Droneaud <ydroneaud@opteya.com> writes:
>
> > 1) there's no mention of the git merge <tag> behavior in git-merge.1
> Yes; we welcome documentation patches.
>
Sent.
> > 2) git merge <tag> VS git merge <object-id>
> >
> > If <tag> is an object (not a lightweight/reference tag), git merge <tag>
> > ...
> > But, if you use the tag object-id instead of its name, for example using
> > git merge `git parse-rev <tag>`,
[EDIT]
> > signature is not checked. Git still create a merge commit, but doesn't
> > prepare a commit message with the tag message and the signature:
> >
> > It would be great to have Git using the tag message and check the
> > signature.
>
> Perhaps, but if you feed the $(git rev-parse v1.12.2) to merge, your
> subject will not be able to say "Merge tag 'v1.12.2'" in the first
> place, so I do not think you would want to encourage such usage in
> the first place.
I think if someone want to merge the tag object-id instead of the tag,
the commit subject/message should probably not make a reference to the
tag.
The only use case for such tag merging by commit-id would be to get
consistent behavior in case of tag deletion. The named tag could be
recreated to point to another point in time. So when looking at the
merge commit message and searching for the tag (by name) could be
misleading.
PS: and what about my third issue ?
Regards.
--
Yann Droneaud
OPTEYA
^ permalink raw reply
* Re: [PATCH v2 00/45] parse_pathspec and :(glob) magic
From: Junio C Hamano @ 2013-03-20 18:02 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1363781779-14947-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Junio please pull the series from github [1]....
> [1] https://github.com/pclouds/git/commits/parse-pathspec
Please write it like this:
https://github.com/pclouds/git parse-pathspec
so that I can just say
git fetch <<that thing>>
git diff nd/magic-pathspecs FETCH_HEAD
I am still getting this out of 7300, though.
expecting success:
mkdir -p build docs &&
touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
would_clean=$(
cd docs &&
git clean -n "$(pwd)/../src" |
sed -n -e "s|^Would remove ||p"
) &&
test "$would_clean" = ../src/part3.c || {
echo "OOps <$would_clean>"
false
}
OOps <>
not ok 8 - git clean with absolute path
#
#
# mkdir -p build docs &&
# touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
# would_clean=$(
# cd docs &&
# git clean -n "$(pwd)/../src" |
# sed -n -e "s|^Would remove ||p"
# ) &&
# test "$would_clean" = ../src/part3.c || {
# echo "OOps <$would_clean>"
# false
# }
#
^ permalink raw reply
* [PATCH] Documentation: merging a tag is a special case
From: Yann Droneaud @ 2013-03-20 17:53 UTC (permalink / raw)
To: Git; +Cc: Yann Droneaud, Junio C Hamano
In-Reply-To: <7vfvzrjrad.fsf@alter.siamese.dyndns.org>
When asking Git to merge a tag (such as a signed tag or annotated tag),
it will always create a merge commit even if fast-forward was possible.
It's like having --no-ff present on the command line.
It's a difference from the default behavior described in git-merge.txt.
It should be documented as an exception of "FAST-FORWARD MERGE" section
and "--ff" option description.
Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
Documentation/git-merge.txt | 9 +++++++++
Documentation/merge-options.txt | 2 +-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index c852a26..84bc873 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -170,6 +170,15 @@ happens:
If you tried a merge which resulted in complex conflicts and
want to start over, you can recover with `git merge --abort`.
+MERGING TAG
+-----------
+
+When merging a tag (annotated or signed), Git will create a merge commit
+even if a fast-forward merge is possible (see above).
+The commit message template will be created from the tag message.
+Additionally, the signature check will be reported as a comment
+if the tag was signed. See also linkgit:git-tag[1].
+
HOW CONFLICTS ARE PRESENTED
---------------------------
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 0bcbe0a..70d1ec0 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -26,7 +26,7 @@ set to `no` at the beginning of them.
--ff::
When the merge resolves as a fast-forward, only update the branch
pointer, without creating a merge commit. This is the default
- behavior.
+ behavior (except when merging a tag).
--no-ff::
Create a merge commit even when the merge resolves as a
--
1.7.11.7
^ permalink raw reply related
* Re: [RFC] Add posibility to preload stat information.
From: Ramkumar Ramachandra @ 2013-03-20 17:49 UTC (permalink / raw)
To: Fredrik Gustafsson; +Cc: spearce, git, pclouds, Jeff King
In-Reply-To: <20130320174120.GA32426@paksenarrion.iveqy.com>
Fredrik Gustafsson wrote:
> On Wed, Mar 20, 2013 at 10:45:22PM +0530, Ramkumar Ramachandra wrote:
>> Fredrik Gustafsson wrote:
>> > When entering a git working dir, optionally run a forked process that
>> > stat all files in the whole workdir and therefore loads stat information
>> > to RAM which will speedup things like git status and so on.
>>
>> This is misleading. You just execute the equivalent of `git status`
>> everytime I request a prompt inside a git working directory. And this
>> is if I'm using __git_ps1() to augment my prompt, which I'm not- I use
>> ZSH's vcs_info, which is arguably better. Also, you forgot to say how
>> to turn on the feature.
>
> The invokation place is quesionable (Junio also had some thoughts about
> that). I don't find vcs_info in the contrib/completition/. Do you have
> any suggestion about where the best way is to inwoke this kind of thing?
I think it should be a separate script in contrib/ that people can
just `eval` in their shell configs; zsh has a chpwd() function for
example, which seems like the right place to put such a thing.
> I added documentation about how to turn the feature on, in the same way
> the other features is documented. (Is there an other way/better way I
> should do this?)
No, I meant in the commit message.
>> That said, this feature is extremely gross; it thrashes my filesystem
>> and hard drive. Modern software is written to minimize IO, not
>> maximize it! I'm completely against the inclusion of this patch.
>
> It's extremly gross. I don't like this, _but_ it does speed up my work.
> I'm unsure if it should be included in git though (hence the RFC-tag).
Yes, I would certainly like my git startup time to be improved. But I
don't want to trade my hard drive's life for it.
>> However, I would not mind a feature that runs `git status` the very
>> first time I enter a git working directory: when I enter my clone of
>> linux.git, it takes my first `git status` invocation a good ten
>> seconds to complete, and we can fix this pretty easily.
>
> That's the problem I try to solve. However "the first time" is
> irrelevant. We will run git status a bit before we need it. If we enter
> linux.git, do other work (in an other project) for an hour and go back
> to linux.git our cache will probably be empty. We will need to run this
> more than "the first time". But still, we don't want it to run too
> often. (Which is does now).
What I meant by "first time" is "chpwd() into the git repository, not
further chpwd()s when already inside the git repository".
^ permalink raw reply
* Re: [RFC] Add posibility to preload stat information.
From: Jeff King @ 2013-03-20 17:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Fredrik Gustafsson, spearce, git, pclouds
In-Reply-To: <7vhak6f0w4.fsf@alter.siamese.dyndns.org>
On Wed, Mar 20, 2013 at 10:15:39AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > So maybe just run "git status >/dev/null"?
>
> In the background? How often would it run? I do not think a single
> lockfile solves anything. It may prevent simultaneous runs of two
> such "prime the well" processes, but the same user may be working in
> two separate repositories.
Yes, in the background (he invokes __git_recursive_stat already in the
background). I'd think you would want to run it whenever you enter a
repository.
> I do not see anything that prevents it from running in the same
> repository over and over again, either. "prompt" is a bad place to
> do this kind of thing.
Yeah, I did not look closely at that. The commit message claims "When
entering a git working dir", but the implementation runs it on each
prompt invocation, which is awful. I think you'd want to check to use
rev-parse to see if you have changed into a new git repo, and only run
it once then.
Which is still gross, and I have no interest whatsoever in this feature.
I was just trying to be positive and constructive to the original
submission. :)
-Peff
^ permalink raw reply
* [PATCH] do not use GIT_TRACE_PACKET=3 in tests
From: Jeff King @ 2013-03-20 17:43 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <20130320172640.GC10752@sigill.intra.peff.net>
Some test scripts use the GIT_TRACE mechanism to dump
debugging information to descriptor 3 (and point it to a
file using the shell). On Windows, however, bash is unable
to set up descriptor 3. We do not write our trace to the
file, and worse, we may interfere with other operations
happening on descriptor 3, causing tests to fail or other
even behave inconsistently.
Prior to commit 97a83fa (upload-pack: remove packet debugging
harness), these tests used GIT_DEBUG_SEND_PACK, which only
supported output to a descriptor. The tests in t5503 were
always broken on Windows, and were marked to be skipped via
the NOT_MINGW prerequisite. In t5700, the tests used to pass
prior to 97a83fa, but only because they were not careful
enough; because we only grepped the trace file, an empty
file looked successful to us. But post-97a83fa, the writing
to descriptor 3 causes "git fetch" to hang (presumably
because we are throwing random bytes into the middle of the
protocol).
Now that we are using the GIT_TRACE mechanism, we can
improve both scripts by asking git to write directly to a
file rather than a descriptor. That fixes the hang in t5700,
and should allow t5503 to successfully run on Windows.
In both cases we now also use "test -s" to double-check that
our trace file actually contains output, which should reduce
the possibility of an erroneously passing test.
Signed-off-by: Jeff King <peff@peff.net>
---
On top of jk/pkt-line-cleanup.
This works for me on Linux, but I do not have a mingw system to test
t5503 on. If my reasoning is right, it should, but one never knows. :)
Johannes, can you report whether it fixes the problem?
t/t5503-tagfollow.sh | 38 ++++++++++++++++++--------------------
t/t5700-clone-reference.sh | 14 +++++++++-----
2 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index d181c96..f30c038 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -4,10 +4,6 @@ test_description='test automatic tag following'
. ./test-lib.sh
-if ! test_have_prereq NOT_MINGW; then
- say "GIT_TRACE_PACKET not supported - skipping tests"
-fi
-
# End state of the repository:
#
# T - tag1 S - tag2
@@ -17,7 +13,7 @@ fi
# \ C - origin/cat \
# origin/master master
-test_expect_success NOT_MINGW setup '
+test_expect_success setup '
test_tick &&
echo ichi >file &&
git add file &&
@@ -39,33 +35,35 @@ test_expect_success NOT_MINGW 'fetch A (new commit : 1 connection)' '
'
U=UPLOAD_LOG
+UPATH="$(pwd)/$U"
-test_expect_success NOT_MINGW 'setup expect' '
+test_expect_success 'setup expect' '
cat - <<EOF >expect
want $A
EOF
'
get_needs () {
+ test -s "$1" &&
perl -alne '
next unless $F[1] eq "upload-pack<";
last if $F[2] eq "0000";
print $F[2], " ", $F[3];
- ' "$@"
+ ' "$1"
}
-test_expect_success NOT_MINGW 'fetch A (new commit : 1 connection)' '
+test_expect_success 'fetch A (new commit : 1 connection)' '
rm -f $U &&
(
cd cloned &&
- GIT_TRACE_PACKET=3 git fetch 3>../$U &&
+ GIT_TRACE_PACKET=$UPATH git fetch &&
test $A = $(git rev-parse --verify origin/master)
) &&
get_needs $U >actual &&
test_cmp expect actual
'
-test_expect_success NOT_MINGW "create tag T on A, create C on branch cat" '
+test_expect_success "create tag T on A, create C on branch cat" '
git tag -a -m tag1 tag1 $A &&
T=$(git rev-parse --verify tag1) &&
@@ -77,18 +75,18 @@ test_expect_success NOT_MINGW 'fetch C, T (new branch, tag : 1 connection)' '
git checkout master
'
-test_expect_success NOT_MINGW 'setup expect' '
+test_expect_success 'setup expect' '
cat - <<EOF >expect
want $C
want $T
EOF
'
-test_expect_success NOT_MINGW 'fetch C, T (new branch, tag : 1 connection)' '
+test_expect_success 'fetch C, T (new branch, tag : 1 connection)' '
rm -f $U &&
(
cd cloned &&
- GIT_TRACE_PACKET=3 git fetch 3>../$U &&
+ GIT_TRACE_PACKET=$UPATH git fetch &&
test $C = $(git rev-parse --verify origin/cat) &&
test $T = $(git rev-parse --verify tag1) &&
test $A = $(git rev-parse --verify tag1^0)
@@ -97,7 +95,7 @@ test_expect_success NOT_MINGW 'fetch C, T (new branch, tag : 1 connection)' '
test_cmp expect actual
'
-test_expect_success NOT_MINGW "create commits O, B, tag S on B" '
+test_expect_success "create commits O, B, tag S on B" '
test_tick &&
echo O >file &&
git add file &&
@@ -113,18 +111,18 @@ test_expect_success NOT_MINGW 'fetch B, S (commit and tag : 1 connection)' '
S=$(git rev-parse --verify tag2)
'
-test_expect_success NOT_MINGW 'setup expect' '
+test_expect_success 'setup expect' '
cat - <<EOF >expect
want $B
want $S
EOF
'
-test_expect_success NOT_MINGW 'fetch B, S (commit and tag : 1 connection)' '
+test_expect_success 'fetch B, S (commit and tag : 1 connection)' '
rm -f $U &&
(
cd cloned &&
- GIT_TRACE_PACKET=3 git fetch 3>../$U &&
+ GIT_TRACE_PACKET=$UPATH git fetch &&
test $B = $(git rev-parse --verify origin/master) &&
test $B = $(git rev-parse --verify tag2^0) &&
test $S = $(git rev-parse --verify tag2)
@@ -133,14 +131,14 @@ EOF
test_cmp expect actual
'
-test_expect_success NOT_MINGW 'setup expect' '
+test_expect_success 'setup expect' '
cat - <<EOF >expect
want $B
want $S
EOF
'
-test_expect_success NOT_MINGW 'new clone fetch master and tags' '
+test_expect_success 'new clone fetch master and tags' '
git branch -D cat
rm -f $U
(
@@ -148,7 +146,7 @@ test_expect_success NOT_MINGW 'new clone fetch master and tags' '
cd clone2 &&
git init &&
git remote add origin .. &&
- GIT_TRACE_PACKET=3 git fetch 3>../$U &&
+ GIT_TRACE_PACKET=$UPATH git fetch &&
test $B = $(git rev-parse --verify origin/master) &&
test $S = $(git rev-parse --verify tag2) &&
test $B = $(git rev-parse --verify tag2^0) &&
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index 9cd3b4d..60f1552 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -54,11 +54,14 @@ test_expect_success 'cloning with reference (no -l -s)' \
rm -f "$U.D"
-test_expect_success 'cloning with reference (no -l -s)' \
-'GIT_TRACE_PACKET=3 git clone --reference B "file://$(pwd)/A" D 3>"$U.D"'
+test_expect_success 'cloning with reference (no -l -s)' '
+ GIT_TRACE_PACKET=$U.D git clone --reference B "file://$(pwd)/A" D
+'
-test_expect_success 'fetched no objects' \
-'! grep " want" "$U.D"'
+test_expect_success 'fetched no objects' '
+ test -s "$U.D" &&
+ ! grep " want" "$U.D"
+'
cd "$base_dir"
@@ -173,9 +176,10 @@ test_expect_success 'fetch with incomplete alternates' '
(
cd K &&
git remote add J "file://$base_dir/J" &&
- GIT_TRACE_PACKET=3 git fetch J 3>"$U.K"
+ GIT_TRACE_PACKET=$U.K git fetch J
) &&
master_object=$(cd A && git for-each-ref --format="%(objectname)" refs/heads/master) &&
+ test -s "$U.K" &&
! grep " want $master_object" "$U.K" &&
tag_object=$(cd A && git for-each-ref --format="%(objectname)" refs/tags/HEAD) &&
! grep " want $tag_object" "$U.K"
--
1.8.2.22.g1efe1a3
^ permalink raw reply related
* Re: [RFC] Add posibility to preload stat information.
From: Fredrik Gustafsson @ 2013-03-20 17:41 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Fredrik Gustafsson, spearce, git, pclouds, Jeff King
In-Reply-To: <CALkWK0=b80U5dxGMpwKwL+jFURisEuSapWuuNRFcP+5R2f3+GA@mail.gmail.com>
On Wed, Mar 20, 2013 at 10:45:22PM +0530, Ramkumar Ramachandra wrote:
> Fredrik Gustafsson wrote:
> > When entering a git working dir, optionally run a forked process that
> > stat all files in the whole workdir and therefore loads stat information
> > to RAM which will speedup things like git status and so on.
>
> This is misleading. You just execute the equivalent of `git status`
> everytime I request a prompt inside a git working directory. And this
> is if I'm using __git_ps1() to augment my prompt, which I'm not- I use
> ZSH's vcs_info, which is arguably better. Also, you forgot to say how
> to turn on the feature.
The invokation place is quesionable (Junio also had some thoughts about
that). I don't find vcs_info in the contrib/completition/. Do you have
any suggestion about where the best way is to inwoke this kind of thing?
I added documentation about how to turn the feature on, in the same way
the other features is documented. (Is there an other way/better way I
should do this?)
>
> That said, this feature is extremely gross; it thrashes my filesystem
> and hard drive. Modern software is written to minimize IO, not
> maximize it! I'm completely against the inclusion of this patch.
It's extremly gross. I don't like this, _but_ it does speed up my work.
I'm unsure if it should be included in git though (hence the RFC-tag).
>
> However, I would not mind a feature that runs `git status` the very
> first time I enter a git working directory: when I enter my clone of
> linux.git, it takes my first `git status` invocation a good ten
> seconds to complete, and we can fix this pretty easily.
That's the problem I try to solve. However "the first time" is
irrelevant. We will run git status a bit before we need it. If we enter
linux.git, do other work (in an other project) for an hour and go back
to linux.git our cache will probably be empty. We will need to run this
more than "the first time". But still, we don't want it to run too
often. (Which is does now).
--
Med vänliga hälsningar
Fredrik Gustafsson
tel: 0733-608274
e-post: iveqy@iveqy.com
^ permalink raw reply
* Re: USE_NSEC bug?
From: Junio C Hamano @ 2013-03-20 17:31 UTC (permalink / raw)
To: Jeff King; +Cc: Andrew Rodland, git
In-Reply-To: <20130320170957.GA21350@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> And the cost is that we have another Makefile knob people need to tweak
> that would not otherwise need to be there. Which can be annoying, but is
> also not that huge a cost to deal with (we might want to improve the
> configure script or something, though).
>
> I admit I don't care too much either way.
I don't, either ;-)
^ permalink raw reply
* Re: [PATCH jk/pkt-line-cleanup] t5700-clone-reference: send trace to fd 2, not 3, to please Windows git
From: Junio C Hamano @ 2013-03-20 17:30 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Git Mailing List
In-Reply-To: <20130320170607.GB10752@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Also, though I think your use of stderr is probably OK, might it be a
> little more robust against future output changes to use:
>
> GIT_TRACE_PACKET=$PWD/$U.D git clone ...
>
> to write directly to the file. The original GIT_DEBUG_SEND_PACK did not
> support such niceties, but GIT_TRACE gives them to us for free.
Sounds very sensible.
^ permalink raw reply
* Re: [PATCH jk/pkt-line-cleanup] t5700-clone-reference: send trace to fd 2, not 3, to please Windows git
From: Jeff King @ 2013-03-20 17:26 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Git Mailing List
In-Reply-To: <20130320170607.GB10752@sigill.intra.peff.net>
On Wed, Mar 20, 2013 at 01:06:07PM -0400, Jeff King wrote:
> On Wed, Mar 20, 2013 at 12:30:47PM +0100, Johannes Sixt wrote:
>
> > > I think that is OK, but I'm curious why this is a problem _now_, and not
> > > with the code prior to 97a83fa8. The old GIT_DEBUG_SEND_PACK was also
> > > just calling write() to descriptor 3.
> >
> > Before this change, both affected commands completed and the trace file
> > was empty. Notice that in both test cases we only check for the absence of
> > certain lines, which is naturally true for an empty file, so that the
> > tests pass.
>
> Hmm. The code in t5503 is similar, but before my patch used to actually
> use "test -s" to make sure that some trace output was written. Did it
> fail before 97a83fa8 (and does it pass now)?
Ah, I see. It did fail, and it was marked with NOT_MINGW. I think we can
fix that now. Patch coming in a moment.
-Peff
^ 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