* Re: [PATCH] git-for-each-ref: move GPG sigs off %(body) to %(signature).
From: Jeff King @ 2011-08-30 16:27 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Michał Górny, git
In-Reply-To: <4E5CB0D0.7000905@drmicha.warpmail.net>
On Tue, Aug 30, 2011 at 11:43:44AM +0200, Michael J Gruber wrote:
> > When grabbing a %(body) or %(contents) off a tag, one doesn't really
> > expect to get the GPG signature as well (as it's basically useless
> > without the complete signed text). Thus, strip it off those two tags,
> > and make available via %(signature) if anyone needs it.
>
> No, please do not change %(contents). It is the complete content which
> (together with the header) enters into the sha1 calculation.
>
> You will probably also face opposition as regards to %(body), changing
> existing behaviour.
Yeah. If it were 2005, I think it might make sense to have
non-overlapping individual keys to get each part of the tag. But it's
not worth breaking backwards compatibility of "%(body)" today. So it has
to remain as-is, and we have to introduce a new key for "the body
without the signature".
> In fact, I wish we didn't have %(body) but %(contents:body) just like
> other modifiers such as :short.
>
> I think I'd go for
>
> %(contents:signature)
That makes some sense to me, though it is a little weird that the
":signature" modifier works only for tags, and not commits. In other
cases, whole keys either work or don't work (e.g., "taggerdate"). But I
guess it is not that big a deal.
> %(contents:subject) the same as %(subject)
> %(contents:body) as contents minus subject minus signature
>
> and slowly deprecate %(subject) and %(body) (simply un-document for now).
That leaves no way to get what "%(body)" provides now, right? I wonder
if anyone cares. You can always ask for:
%(contents:body)%(contents:signature)
I guess.
> > + /* look for GPG signature */
>
> Again I have to say no. Please look at
>
> 3d5854e (tag: recognize rfc1991 signatures, 2010-11-10)
>
> which uses the factored out signature detection as introduced in the
> previous commits. Thanks!
Yeah. More correct, and it's less code, too. :)
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] am: preliminary support for hg patches
From: Junio C Hamano @ 2011-08-30 17:02 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git
In-Reply-To: <CAOxFTczyNtyLWyXppj=0UW_zeD3t+rDtzt-vwqXkwvWOTdxi2g@mail.gmail.com>
Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
> ... This is
> correctly detected by -3, with
>
> Applying: Threeway test
> fatal: sha1 information is lacking or useless (dir.h).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 Threeway test
>
> The message is a bit misleading (it's not the repo lacking the blobs,
> it's the patch missing the information), but the process fails as
> expected.
>
>> What about renaming patches?
>
> They lack similarity indices, but they seem to be properly formated
> (and the simple cases I tested apply correctly).
These were exactly what I wanted to know. Thanks for experimenting.
> So I think that keeping the --git is the right choice.
Yeah, sounds like we are safe and better off keeping it.
^ permalink raw reply
* Re: [PATCH 0/3] Un-pessimize "diff-index $commit -- $pathspec"
From: Junio C Hamano @ 2011-08-30 17:03 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Linus Torvalds, Nguyen Thai Ngoc Duy
In-Reply-To: <4E5CB5C6.6020403@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> Seems my analysis wasn't that far off (though, admittedly, unspecific):
Yes, the "diff-index" implementation was updated over time a few times and
I was confused. Sorry.
Thanks for helping diagnosing the issue.
^ permalink raw reply
* Re: [PATCH 2/2] Add a remote helper to interact with mediawiki (fetch & push)
From: Junio C Hamano @ 2011-08-30 17:13 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Sverre Rabbelier, Matthieu Moy, git, Jeremie Nikaes,
Arnaud Lacurie, Claire Fousse, David Amouyal, Matthieu Moy,
Sylvain Boulmé, Ramkumar Ramachandra, Dmitry Ivankov
In-Reply-To: <20110830035608.GB6647@elie.gateway.2wire.net>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Sverre Rabbelier wrote:
>> On Mon, Aug 29, 2011 at 08:05, Junio C Hamano <gitster@pobox.com> wrote:
>
>>> Does this exchange suggest that at least we would need an update to
>>> documentation around "done", as Matthieu's "why is 'done' needed even
>>> though I am not calling with --done?" sounds like a very fair question.
>>
>> No I think the documentation for fast-import is correct.
>
> However, the documentation for git-remote-helpers is hopelessly outdated.
Good point. If somebody writes a working remote-helper using "import" to
trigger fast-import as the back-end, and if it breaks because we changed
the way fast-import is invoked and the helper needs to be updated, we at
least need to give them a useful documentation that clearly states how the
helper is expected to behave.
> It does _not_ document recent changes; patches for that would
> presumably come on top.
Will queue this version for now, if only as a reminder that we would need
more updates.
Thanks.
^ permalink raw reply
* Re: [PATCH v3] Makefile: Improve compiler header dependency check
From: Junio C Hamano @ 2011-08-30 17:17 UTC (permalink / raw)
To: David Aguilar; +Cc: Jonathan Nieder, Fredrik Kuivinen, git
In-Reply-To: <1314692855-75113-1-git-send-email-davvid@gmail.com>
Thanks, both; will queue with the final paragraph updated to Jonathan's
suggestion.
^ permalink raw reply
* [PATCH] stash: Don't paginate by default with list command
From: Ingo Brückl @ 2011-08-30 17:21 UTC (permalink / raw)
To: git
The output of "stash list" is such that piping into a pager
normally isn't necessary but annoying, so disable it by default.
Signed-off-by: Ingo Brückl <ib@wupperonline.de>
---
git-stash.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index 31dec0a..b92d986 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -264,7 +264,7 @@ have_stash () {
list_stash () {
have_stash || return 0
- git log --format="%gd: %gs" -g "$@" $ref_stash --
+ git --no-pager log --format="%gd: %gs" -g "$@" $ref_stash --
}
show_stash () {
--
1.7.6
^ permalink raw reply related
* Re: [PATCH 2/3] unpack-trees: allow pruning with pathspec
From: Junio C Hamano @ 2011-08-30 17:32 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Linus Torvalds
In-Reply-To: <CACsJy8CBJEimG5am8+aWTGP7kzuZ79WfZ3KQKh7pyjKTx-mR9A@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> On Tue, Aug 30, 2011 at 4:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> and this codepath probably should not be enabled while running a merge
>
> But do you think if it works with merge (ie. a partial merge)?
I highly doubt it.
The primary reason why I doubt it would work is actually the same as the
very reason why a merge would work with unpack_trees() machinery. It walks
all the entries in the trees involved, picking matching entries that would
go in the result, while also picking the corresponding entry from the index,
and compute their merge and depositing the result in the final index. If
you "optimize" it by not walking some parts of trees, you don't just omit
merging damages made to these parts by other trees, but you also omit
recording the contributions by the current HEAD and the index. You would
need to compensate for that by doing something that copies the contents
of the index for paths outside the area covered by pathspec.
And whole point of this series is not doing that. We don't want to say
"because they are outside pathspec, we pretend that these paths in index
do not have corresponding entries in the trees we are merging into" and
end up producing creation filepairs for them.
But I honestly am not interested giving it unnecessary deep thought at
this point in the cycle. I haven't found a need for a low-level partial
merge machinery implementation to support any higher level workflows in
git, and the path-level 3-way merge machinery already exists in the form
of ll_merge().
^ permalink raw reply
* Re: Idea: "git format-patch" should get more information out of git
From: Johan Herland @ 2011-08-30 17:39 UTC (permalink / raw)
To: Michael J Gruber
Cc: Jeff King, Junio C Hamano, Michael Haggerty, git, Jonathan Nieder
In-Reply-To: <4E5D048D.4010902@drmicha.warpmail.net>
On Tuesday 30. August 2011, Michael J Gruber wrote:
> Reminds me of the ref namespace restructuring which could help
> sharing notes... Oh, lots to do before git 3.0!
Indeed. I am very sorry to not have been able to properly follow up on
that proposal (or a couple of other patches for that matter), but I'm
currently in the middle of changing jobs and moving to a new apartment,
which unfortunately leaves little if any spare time for git work. I
don't believe I'll find the time to pick this up in the next month or
two, so if someone wants to run with it, you are very welcome to do so.
I will try to participate in discussions where I'm CCed, but the rest of
the mailing list is mostly a blur, these days.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: [PATCH] stash: Don't paginate by default with list command
From: Ben Walton @ 2011-08-30 17:43 UTC (permalink / raw)
To: Ingo Brückl; +Cc: git
In-Reply-To: <4e5d1e5c.438d9c87.bm000@wupperonline.de>
Excerpts from Ingo Brückl's message of Tue Aug 30 13:21:18 -0400 2011:
> The output of "stash list" is such that piping into a pager
> normally isn't necessary but annoying, so disable it by default.
If you $PAGER is less and you use the default LESS environment value
FRXS, this shouldn't be annoying at all. Are you using either a
different pager or a different value for LESS?
Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302
^ permalink raw reply
* Re: [PATCH 1/3] traverse_trees(): allow pruning with pathspec
From: Junio C Hamano @ 2011-08-30 17:44 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Linus Torvalds
In-Reply-To: <CACsJy8CmZv7Fuhw+m6X2CVO4vWLYFQNmStu1jJmhVXSi4mPxJQ@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>> @@ -376,16 +396,22 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
>> mask |= 1ul << i;
>> if (S_ISDIR(entry[i].mode))
>> dirmask |= 1ul << i;
>> + e = &entry[i];
>> }
>
> Why? "e" is not used in that loop or anywhere after that.
This is trying to find _a_ surviving entry to be fed to prune_traversal()
which in turn uses tree_entry_interesting(). At this point in the code, we
are stuffing the entries of the same name from the input trees (and if one
tree is missing an entry of the chosen name, it will have NULL there), so
any non-empty entry would do. It corresponds to "first" but that is just a
simple string and not a name_entry tree_entry_interesting() wants.
>> if (!mask)
>> break;
>> - ret = info->fn(n, mask, dirmask, entry, info);
>> - if (ret < 0) {
>> - error = ret;
>> - if (!info->show_all_errors)
>> - break;
>> + interesting = prune_traversal(e, info, &base, interesting);
>> + if (interesting < 0)
>> + break;
>
> I don't really understand this function to comment. But I guess when
> interesting < 0, we only skip info->fn() and assume it returns "mask"
> (its user unpack_callback() only returns either "mask" or -1).
We consume the entries we have used in merging (which is actually
"everything in entry[] array" as info->fn() returns "mask" itself) by
saying "update_extended_entry()" and the purpose of doing so is to prepare
to process the next entry of the tree we are traversing.
When tree_entry_interesting() returns negative, it tells us "no, and no
subsequent entries will be either", meaning "we are done with this tree".
As we are done, there is nothing to prepare for the next round; we are not
walking the remaining entries in the trees we are looking at. Is there any
point in calling update_extended_entry() I am missing?
^ permalink raw reply
* Re: [PATCH v2] Documentation: clarify effects of -- <path> arguments
From: Junio C Hamano @ 2011-08-30 18:03 UTC (permalink / raw)
To: Thomas Rast; +Cc: git
In-Reply-To: <73058a4801ea2707f5ffa12150682ee0522385eb.1314695984.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> Or both?
Certainly; I didn't mean to reject the "see also". It just didn't look
right not to update the original sentence that seemed to have caused
confusion to new people.
Applied.
^ permalink raw reply
* Re: [PATCH] t3903: fix misquoted rev-parse invocation
From: Junio C Hamano @ 2011-08-30 18:03 UTC (permalink / raw)
To: Jon Seymour; +Cc: Thomas Rast, git
In-Reply-To: <CAH3AnrophV=bi+70B8EnfZpBQXVbEAJ9nSo9UbZ2svDNre9W=A@mail.gmail.com>
Thanks, both.
^ permalink raw reply
* Re: [PATCH] stash: Don't paginate by default with list command
From: Raphael Zimmerer @ 2011-08-30 17:57 UTC (permalink / raw)
To: Ingo Brückl; +Cc: git
In-Reply-To: <4e5d1e5c.438d9c87.bm000@wupperonline.de>
On Tue, Aug 30, 2011 at 07:21:18PM +0200, Ingo Brückl wrote:
> The output of "stash list" is such that piping into a pager
> normally isn't necessary but annoying, so disable it by default.
Why is the current behaviour annoying?
Here on my (default) setup the pager automatically exits if the entire
output can be displayed on the screen, as with any other git
command. Is there any chance you have set the LESS environment
variable somewhere? To disable the pager for some commands, cou can
have a look at the *.pager config variables (man git-confg). However,
'pager.stash' is not recognized... The 'alias.*' config variables may
be interesting, too.
Raphael
^ permalink raw reply
* Re: [PATCH] Symlink mergetools scriptlets into valgrind wrappers
From: Jeff King @ 2011-08-30 18:16 UTC (permalink / raw)
To: Thomas Rast; +Cc: Junio C Hamano, git, David Aguilar
In-Reply-To: <e11fd4c98faf1fc4f970bd1570fe931e078f436e.1314665234.git.trast@student.ethz.ch>
On Tue, Aug 30, 2011 at 02:47:36AM +0200, Thomas Rast wrote:
> Since bc7a96a (mergetool--lib: Refactor tools into separate files,
> 2011-08-18) the mergetools and difftools related tests fail under
> --valgrind because the mergetools/* scriptlets are not in the exec
> path.
>
> For now, symlink the mergetools subdir into the t/valgrind/bin
> directory as a whole, since it does not contain anything of interest
> to the valgrind wrappers.
I'm not super-excited about special-casing like this, but I don't think
there's a way to make it much cleaner without a lot of fuss. And the
valgrind stuff is pretty special-cased as it is (it would probably be
cleaner if it were integrated with the bin-wrappers rules in the
Makefile). But it's probably not worth spending effort on it. What's
there works, and your patch is a sane and straightforward fix for this
new issue. So:
Acked-by: Jeff King <peff@peff.net>
-Peff
^ permalink raw reply
* Re: [PATCH] stash: Don't paginate by default with list command
From: Ingo Brückl @ 2011-08-30 18:24 UTC (permalink / raw)
To: git
In-Reply-To: <1314726104-sup-5174@pinkfloyd.chass.utoronto.ca>
Ben Walton wrote on Tue, 30 Aug 2011 13:43:46 -0400:
> Excerpts from Ingo Brückl's message of Tue Aug 30 13:21:18 -0400 2011:
>> The output of "stash list" is such that piping into a pager
>> normally isn't necessary but annoying, so disable it by default.
> If you $PAGER is less and you use the default LESS environment value
> FRXS, this shouldn't be annoying at all. Are you using either a
> different pager or a different value for LESS?
For some reason I have '-c' in LESS which must be convenient for a case
I currently don't remember.
Ingo
^ permalink raw reply
* Re: Pointers to setup an internal git repository with access control
From: Magnus Bäck @ 2011-08-30 18:33 UTC (permalink / raw)
To: robert mena; +Cc: git
In-Reply-To: <CAAZ43xZPsw2CnqOtb6wO7dRC1eMd0eweQ30w-wfzUHL5cqw=Yw@mail.gmail.com>
On Tuesday, August 30, 2011 at 14:01 CEST,
robert mena <robert.mena@gmail.com> wrote:
> I am trying to migrate from SVN to git and so I am looking for tools
> that help me create an interal repository. I currently use SVN over
> HTTPS/Dav using the access.conf to be able to control which uses can
> read/write to the repos or given branches.
>
> Ideally I am looking for tools that enable me to do the same with git.
Gerrit Code Review (http://code.google.com/p/gerrit/) provides
branch-level ACLs for both reading and writing.
--
Magnus Bäck Opinions are my own and do not necessarily
SW Configuration Manager represent the ones of my employer, etc.
Sony Ericsson
^ permalink raw reply
* Re: Overriding ~/.gitconfig using GIT_CONFIG
From: Jeff King @ 2011-08-30 18:49 UTC (permalink / raw)
To: Richard Purdie
Cc: David Aguilar, Junio C Hamano, Daniel Barkalow,
Nguy?? n Thái Ngọc Duy, GIT Mailing-list
In-Reply-To: <1314729545.5939.391.camel@rex>
On Tue, Aug 30, 2011 at 07:39:05PM +0100, Richard Purdie wrote:
> On Tue, 2011-08-30 at 11:56 -0400, Jeff King wrote:
> > On Tue, Aug 30, 2011 at 01:13:01PM +0100, Richard Purdie wrote:
> >
> > > We've gone through several iterations of this but as things stand now,
> > > to initially clone things we're doing:
> > >
> > > git clone --bare --mirror <url> <dir>
> > >
> > > but if we already have some existing clone we'd update with:
> > >
> > > git remote prune origin
> > > git remote rm origin
> > > git remote add --mirror origin <url>
> > > git fetch --all -t
> > >
> > > [...]
> > >
> > > So if a user has an origin remote in their .gitconfig, can we ignore it?
> >
> > Wouldn't:
> >
> > git fetch --prune <url> refs/heads/*:refs/remotes/origin/*
> >
> > do what you want, and not look at config at all?
>
> Since this is a bare/mirror clone, wouldn't that need to be:
>
> git fetch --prune <url> refs/heads/*:refs/heads/*
Sorry, yes, I forgot about the mirroring bit.
> That also wouldn't fetch tags?
It would only do autofollowing. You could use "-t", but if you really
want a straight mirror, you could do:
git fetch --prune refs/*:refs/*
to get everything.
-Peff
^ permalink raw reply
* Re: [PATCH 2/5] tree-walk: drop unused parameter from match_dir_prefix
From: Dan McGee @ 2011-08-30 18:55 UTC (permalink / raw)
To: git; +Cc: Dan McGee
In-Reply-To: <1301535481-1085-2-git-send-email-dpmcgee@gmail.com>
On Wed, Mar 30, 2011 at 8:37 PM, Dan McGee <dpmcgee@gmail.com> wrote:
> Signed-off-by: Dan McGee <dpmcgee@gmail.com>
> ---
This still seems to apply to the current code, ping?
> tree-walk.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tree-walk.c b/tree-walk.c
> index 322becc..9be8007 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -522,7 +522,7 @@ static int match_entry(const struct name_entry *entry, int pathlen,
> return 0;
> }
>
> -static int match_dir_prefix(const char *base, int baselen,
> +static int match_dir_prefix(const char *base,
> const char *match, int matchlen)
> {
> if (strncmp(base, match, matchlen))
> @@ -579,7 +579,7 @@ int tree_entry_interesting(const struct name_entry *entry,
>
> if (baselen >= matchlen) {
> /* If it doesn't match, move along... */
> - if (!match_dir_prefix(base_str, baselen, match, matchlen))
> + if (!match_dir_prefix(base_str, match, matchlen))
> goto match_wildcards;
>
> if (!ps->recursive || ps->max_depth == -1)
> --
> 1.7.4.2
^ permalink raw reply
* Re: [PATCH] stash: Don't paginate by default with list command
From: Raphael Zimmerer @ 2011-08-30 19:08 UTC (permalink / raw)
To: Ingo Brückl; +Cc: git
In-Reply-To: <4e5d2ac6.64676448.bm000@wupperonline.de>
On Tue, Aug 30, 2011 at 08:24:04PM +0200, Ingo Brückl wrote:
> Ben Walton wrote on Tue, 30 Aug 2011 13:43:46 -0400:
>
> > Excerpts from Ingo Brückl's message of Tue Aug 30 13:21:18 -0400 2011:
>
> >> The output of "stash list" is such that piping into a pager
> >> normally isn't necessary but annoying, so disable it by default.
>
> > If you $PAGER is less and you use the default LESS environment value
> > FRXS, this shouldn't be annoying at all. Are you using either a
> > different pager or a different value for LESS?
>
> For some reason I have '-c' in LESS which must be convenient for a case
> I currently don't remember.
When you don't require the '-c' option for paging in git, please try
'core.pager = less -FRXS'. That forces git's default behaviour
regardless of the LESS environment variable.
Raphael
^ permalink raw reply
* Re: Overriding ~/.gitconfig using GIT_CONFIG
From: Richard Purdie @ 2011-08-30 18:39 UTC (permalink / raw)
To: Jeff King
Cc: David Aguilar, Junio C Hamano, Daniel Barkalow,
Nguy?? n Thái Ngọc Duy, GIT Mailing-list
In-Reply-To: <20110830155649.GA15790@sigill.intra.peff.net>
On Tue, 2011-08-30 at 11:56 -0400, Jeff King wrote:
> On Tue, Aug 30, 2011 at 01:13:01PM +0100, Richard Purdie wrote:
>
> > We've gone through several iterations of this but as things stand now,
> > to initially clone things we're doing:
> >
> > git clone --bare --mirror <url> <dir>
> >
> > but if we already have some existing clone we'd update with:
> >
> > git remote prune origin
> > git remote rm origin
> > git remote add --mirror origin <url>
> > git fetch --all -t
> >
> > [...]
> >
> > So if a user has an origin remote in their .gitconfig, can we ignore it?
>
> Wouldn't:
>
> git fetch --prune <url> refs/heads/*:refs/remotes/origin/*
>
> do what you want, and not look at config at all?
Since this is a bare/mirror clone, wouldn't that need to be:
git fetch --prune <url> refs/heads/*:refs/heads/*
?
That also wouldn't fetch tags?
Cheers,
Richard
^ permalink raw reply
* Re: [PATCH] stash: Don't paginate by default with list command
From: Junio C Hamano @ 2011-08-30 19:26 UTC (permalink / raw)
To: Raphael Zimmerer; +Cc: Ingo Brückl, git
In-Reply-To: <20110830175748.GA19715@rdrz.de>
Raphael Zimmerer <killekulla@rdrz.de> writes:
> On Tue, Aug 30, 2011 at 07:21:18PM +0200, Ingo Brückl wrote:
>> The output of "stash list" is such that piping into a pager
>> normally isn't necessary but annoying, so disable it by default.
>
> Why is the current behaviour annoying?
>
> Here on my (default) setup the pager automatically exits if the entire
> output can be displayed on the screen, as with any other git
> command.
Ben Walton pointed out exactly the same thing, but there needs one bit of
caution/consideration. If you tend to use longer and more descriptive
branch name (which is encouraged) and create a lazy stash without giving
any message, the default description of the stash would need a fairly wide
terminal window to fit on one line, e.g.
"stash@{N}: WIP on some-long-ish-branch-name:" followed by
"09997df a one line description of the commit your changes are made on"
concatenated on the same line. "S" in "FRSX" (the default value of "LESS"
environment variable we give to those who do not have any) is a good thing
to have while reviewing long-ish patches via "git log -p", but having a
long line that "S" causes to be chopped counts as not being able to show
the entire file on the first screen for the purpose of "F", and introduces
a minor irritation of having to exit explicitly with a 'q'.
^ permalink raw reply
* Re: [PATCH 3/5] tree-walk: micro-optimization in tree_entry_interesting
From: Dan McGee @ 2011-08-30 19:51 UTC (permalink / raw)
To: git
In-Reply-To: <CAEik5nOKrpFycZYVnSu4_5LYWxn0JS_hVXyiQH-80Bu-C4k8VQ@mail.gmail.com>
On Mon, Apr 4, 2011 at 7:22 PM, Dan McGee <dpmcgee@gmail.com> wrote:
> On Sun, Apr 3, 2011 at 1:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Dan McGee <dpmcgee@gmail.com> writes:
>>
>>> In the case of a wide breadth top-level tree (~2400 entries, all trees
>>> in this case), we can see a noticeable cost in the profiler calling
>>> strncmp() here. Most of the time we are at the base level of the
>>> repository, so base is "" and baselen == 0, which means we will always
>>> test true. Break out this one tiny case so we can short circuit the
>>> strncmp() call.
>>
>> This sounds as if the patch helps only when you have a superfat tree at
>> the "top-level" of the project, but wouldn't this benefit any superfat
>> tree at _any_ level while we recursively descend into it?
>
> Correct. I looked at the fact that more often than not, we wouldn't
> have to descend into subtrees unless searching for a path underneath
> it, so that is why I phrased it that way. So the "in the case of" was
> quite literally the case I was testing, but didn't mean to exclude
> other potential test cases.
>
>>> This resulted in an ~11% improvement (43 to 38 secs) for a reasonable
>>> log operation on the Arch Linux Packages SVN clone repository, which
>>> contained 117220 commits and the aforementioned 2400 top-level objects:
>>> git log -- autogen/trunk pacman/trunk/ wget/trunk/
>>>
>>> Negligible slowdown was noted with other repositories (e.g. linux-2.6).
>>
>> It would have been easier to swallow if the last sentence were "This could
>> lead to a slowdown in repositories without directories that are too wide,
>> but in practice it was not even measurable." "Negligible" sounds as if it
>> had still measurable downside, and as if you decided that the slowdown can
>> be ignored---but obviously you are not an unbiased judge.
>
> Perhaps I was too cautious with my words- but I was also trying to not
> be biased. Considering this same operation takes < 1 second in
> linux-2.6, I only wanted to mention it could have a slight effect. In
> reality I saw nothing more than an extra 0.01s or so, and definitely
> nothing significant. Let me know if you see otherwise.
>
> dmcgee@galway ~/projects/linux-2.6 (master)
> $ time ../git/git-log -- zzzzz_not_exist > /dev/null
>
> real 0m0.945s
> user 0m0.857s
> sys 0m0.083s
>
>> There is nothing wrong in the patch per-se, but I really wish we didn't
>> have to do this; it feels like the compiler should be helping us in this
>> case.
If I resurrect this with an updated commit message reflecting concerns
raised, can it be merged? Given that it is a noticeable performance
boost on real-life repositories and I can show it has little (<1%) to
no impact on most repos, it is a definite win.
>>> Signed-off-by: Dan McGee <dpmcgee@gmail.com>
>>> ---
>>> tree-walk.c | 4 ++--
>>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tree-walk.c b/tree-walk.c
>>> index 9be8007..f386151 100644
>>> --- a/tree-walk.c
>>> +++ b/tree-walk.c
>>> @@ -591,8 +591,8 @@ int tree_entry_interesting(const struct name_entry *entry,
>>> ps->max_depth);
>>> }
>>>
>>> - /* Does the base match? */
>>> - if (!strncmp(base_str, match, baselen)) {
>>> + /* Either there must be no base, or the base must match. */
>>> + if (baselen == 0 || !strncmp(base_str, match, baselen)) {
>>> if (match_entry(entry, pathlen,
>>> match + baselen, matchlen - baselen,
>>> &never_interesting))
>>
>
^ permalink raw reply
* Re: [PATCH] t6030: use $SHELL_PATH to invoke user's preferred shell instead of bare sh
From: Junio C Hamano @ 2011-08-30 20:15 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, Brandon Casey
In-Reply-To: <Sn_8tnPB8FBe4C5QxoGkIRcdzJ2sLq_-h0oZ37fkrq42HJgWSlGgty_hMznS2ckiKKOg66yY7R2mRQqhJAzedr762dMKb_s6ELjELyGXyU-tkJzir-FnxhCkAIb3q1028d5FrBBy6EW3SXEPBMTYTw@cipher.nrlssc.navy.mil>
Brandon Casey <casey@nrlssc.navy.mil> writes:
> From: Brandon Casey <drafnel@gmail.com>
>
> Some platforms (IRIX, Solaris) provide an ancient /bin/sh which chokes on
> modern shell syntax like $(). SHELL_PATH is provided to allow the user to
> specify a working sh, let's use it here.
>
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
> t/t6030-bisect-porcelain.sh | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index 62125ec..c53af62 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -732,7 +732,7 @@ test_expect_success 'bisect: demonstrate identification of damage boundary' "
> git bisect reset &&
> git checkout broken &&
> git bisect start broken master --no-checkout &&
> - git bisect run sh -c '
> + git bisect run \"$SHELL_PATH\" -c '
> GOOD=\$(git for-each-ref \"--format=%(objectname)\" refs/bisect/good-*) &&
> git rev-list --objects BISECT_HEAD --not \$GOOD >tmp.\$\$ &&
> git pack-objects --stdout >/dev/null < tmp.\$\$
Hmm, shouldn't we also be quoting '$' in front of SHELL_PATH to make the
test_expect_success interpolate the value of SHELL_PATH, instead of hoping
that SHELL_PATH does not have double-quote in it when the shell that forms
parameters given to test_expect_success interpolates it?
^ permalink raw reply
* Re: [PATCH] stash: Don't paginate by default with list command
From: Raphael Zimmerer @ 2011-08-30 20:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ingo Brückl, git
In-Reply-To: <7vk49u1z1n.fsf@alter.siamese.dyndns.org>
On Tue, Aug 30, 2011 at 12:26:12PM -0700, Junio C Hamano wrote:
[...]
> to have while reviewing long-ish patches via "git log -p", but having a
> long line that "S" causes to be chopped counts as not being able to show
> the entire file on the first screen for the purpose of "F", and introduces
> a minor irritation of having to exit explicitly with a 'q'.
Yes, you are right. That can be annoying. Same issue when showing the
reflog, but as it has lots of entries, the automatic pager is really
handy. I seldom use 'stash clear' (stash is a nice way for storing
working copy history somewhere...), so I don't like to miss the
pager in stash list. Maybe 'pager.stash.list' as mentioned before is
the way to go.
But as Ingo has set the LESS environment variable to some value
('-c'), he always has to press 'q', what may be his problem (beside of
the annoyance that his less now displays '~' on each empty line).
Raphael
^ permalink raw reply
* Re: [PATCH] t6030: use $SHELL_PATH to invoke user's preferred shell instead of bare sh
From: Junio C Hamano @ 2011-08-30 20:27 UTC (permalink / raw)
To: Brandon Casey, Brandon Casey; +Cc: git
In-Reply-To: <7vfwki1wqo.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> + git bisect run \"$SHELL_PATH\" -c '
>> GOOD=\$(git for-each-ref \"--format=%(objectname)\" refs/bisect/good-*) &&
>> git rev-list --objects BISECT_HEAD --not \$GOOD >tmp.\$\$ &&
>> git pack-objects --stdout >/dev/null < tmp.\$\$
>
> Hmm, shouldn't we also be quoting '$' in front of SHELL_PATH to make the
> test_expect_success interpolate the value of SHELL_PATH, instead of hoping
> that SHELL_PATH does not have double-quote in it when the shell that forms
> parameters given to test_expect_success interpolates it?
By doing this silly thing:
$ mkdir '/var/tmp/a"b c/'
$ ln -s /bin/sh '/var/tmp/a"b c/shell'
and then adding
SHELL_PATH='/var/tmp/a"b c/shell'
after we source "test-lib.sh" in t6030, I see this breaks when the shell
parses to figure out what parameters to call test_expect_success with.
t/t6030-bisect-porcelain.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 68c0e97..cd9de63 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -701,7 +701,7 @@ test_expect_success 'bisect: demonstrate identification of damage boundary' "
git bisect reset &&
git checkout broken &&
git bisect start broken master --no-checkout &&
- git bisect run \"$SHELL_PATH\" -c '
+ git bisect run \"\$SHELL_PATH\" -c '
GOOD=\$(git for-each-ref \"--format=%(objectname)\" refs/bisect/good-*) &&
git rev-list --objects BISECT_HEAD --not \$GOOD >tmp.\$\$ &&
git pack-objects --stdout >/dev/null < tmp.\$\$
^ 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