* Re: [PATCH 2/2] config: add include directive
From: Junio C Hamano @ 2012-02-06 22:39 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120206095404.GB4300@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> +Includes
> +~~~~~~~~
> +
> +You can include one config file from another by setting the special
> +`include.path` variable to the name of the file to be included. The
> +included file is expanded immediately, as if its contents had been
> +found at the location of the include directive. If the value of the
> +`include.path` variable is a relative path, the path is considered to be
> +relative to the configuration file in which the include directive was
> +found. See below for examples.
If the file referenced by this directive does not exist, what should
happen? Should it be signalled as an error? Should it stop the whole
calling process with die()?
I think "die() when we are honoring the include, ignore when we are not"
would be a good way to handle this, as it allows us to catch mistakes
while allowing the user to fix broken configuration files using "git
config --unset include.path", but I may be overlooking something.
^ permalink raw reply
* Re: [PATCH v4 1/4] completion: work around zsh option propagation bug
From: Felipe Contreras @ 2012-02-06 22:59 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, SZEDER Gábor, Jonathan Nieder, Thomas Rast,
Shawn O. Pearce
In-Reply-To: <7v1uqbpsyh.fsf@alter.siamese.dyndns.org>
On Fri, Feb 3, 2012 at 10:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Right now when listing commands in zsh (git <TAB><TAB>), all of them
>> will show up, instead of only porcelain ones.
>
> Jonathan's rewrite goes straight to the root cause instead, which is
> another way to describe the problem.
>
> Explaining user-visible symptoms at the beginning like you did is a good
> strategy that I would want to see more contributors follow, though.
>
>> Basically, in zsh, this:
>>
>> for i in $__git_all_commands
>>
>> Should be:
>>
>> for i in ${=__git_all_commands}
>>
>> Otherwise there's no word-splitting expansion (unless SH_WORD_SPLIT is
>> set). sh emulation should take care of that, but the subshell is messing
>> up with that. So __git_list_porcelain_commands does not do any
>> filtering.
>
> Let me step back a bit and see if we are on the same page wrt the root
> cause of the problem and for whom we are explaining the change.
>
> The adaptation of the bash completion script to zsh is done by asking zsh
> to obey POSIXy word splitting rules to honor $IFS that is in effect when
> the words are split. However zsh does not do a good job at it in some
> cases, and your patch works it around by avoiding a construct known to be
> troublesome to zsh.
Troublesome to zsh emulation, yeah.
> Am I correct so far? If so, especially if the first sentence of the above
> paragraph is correct, then how would it help others to teach "this is the
> right way to do a word-split if we were writing in native zsh" when we are
> not?
Because without that explanation it's quite difficult to know what
part of the code would behave differently in zsh, and how. Most people
are not familiar with shell features, and would have no idea what
"word splitting" means in a practical context.
> While it probably is a good description to have in a bug report given to
> zsh folks, it is useless for people who read the history of Git.
Of course it's not. It tells you that there is indeed an issue in zsh,
and not in the way we are using it, as it has been acknowledged by zsh
developers.
> The readers need to read the solution described in order to understand why
> the updated construct is written in an unnatural (to people who write to
> POSIXy shells) way, or to avoid reintroducing a similar problem elsewhere
> in the future.
Doesn't this explain that?
---
sh emulation should take care of that, but the subshell is messing
up with that.
---
Granted, for people not familiar with shell features "subshell" should
be accompanied with $(foo).
> I find that what Jonathan gave you helps them much better:
> ...
> fn () {
> var='one two'
> printf '%s\n' $var
> }
> x=$(fn)
> : ${y=$(fn)}
>
> printing "$x" results in two lines as expected, but printing "$y" results
> in a single line because $var is expanded as a single word when evaluating
> fn to compute y.
>
> So avoid the construct, and use an explicit 'test -n "$foo" || foo=$(bar)'
> instead.
>
> So I'll take the first two lines of the message (good bits), and simplify
> the "This fixes a bug tht caused..." from the last paragraph (or perhaps
> even drop it).
I'm not sure about it, because this relies on knowledge of how printf
works, and it's not used that often; an example with 'for' would be
much more clear IMO.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: git-svn: t9155 fails against subversion 1.7.0
From: Eric Wong @ 2012-02-06 22:59 UTC (permalink / raw)
To: Frans Klaver
Cc: Robin H. Johnson, Git Mailing List, Jonathan Nieder, Ben Walton
In-Reply-To: <CAH6sp9ORKvXt2_V4UgESTY7Tn2=9ysjWS3dO4eGgxCuZY1a5Yw@mail.gmail.com>
Frans Klaver <fransklaver@gmail.com> wrote:
> I haven't invested time in this, and the tests still seem to fail on
> subversion 1.7.x. Maybe one of the people involved in $gmane/184644
> knows more?
Not me. I haven't had time/interest in dealing with SVN in a while.
I've always hoped somebody else has the time+interest for git-svn, but
given the self-obsoleting nature of git-svn, it's hard to find
motivation.
^ permalink raw reply
* Re: [bug] blame duplicates trailing ">" in mailmapped emails
From: Felipe Contreras @ 2012-02-06 23:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Jonathan Nieder, git, SZEDER Gábor
In-Reply-To: <7vliof62ko.fsf@alter.siamese.dyndns.org>
On Tue, Feb 7, 2012 at 12:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> This subject doesn't explain the *purpose* of the patch: always return
>> a plain mail address from map_user()
>
> That would be a much better subject.
>
>> I think the immediate problem should be here:
>>
>> Currently 'git blame -e' would add an extra '>' if map_user() returns
>> true, which would end up as '<foo@bar.com>>'. This is because
>> map_user() sometimes modifies, the mail string, but sometimes not. So
>> let's always modify it.
>
> That is just a symptom.
That's a matter of semantics. Is the API broken? Then the API has a
problem, and you are fixing it, otherwise you are merely *improving*
it.
> People who reached this commit by digging the
> history of mailmap.c would need to see the *cause* of the symptom
> described in the light of how the API is designed to be used.
Well, when you have a summary as "always return a plain mail address
from map_user()", I think it's pretty clear what is the "problem"; the
API does not always return a plain mail address. But any case, that is
described below, in my suggestion.
Besides, even people digging the history would benefit from seeing the
"symptom" first.
> In other
> words, "the code after the update has to be this way because these are the
> i/o constraints this API has". "Otherwise you would see this breakage for
> example" is merely a supporting material.
I disagree. The reader of the current commit message will keep in
his/her head the question "Why?", and it would be answered only at the
very end. Besides, what more succinct way to describe a problem with
the API than with an example (that happens to be real).
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v4 1/4] completion: work around zsh option propagation bug
From: Junio C Hamano @ 2012-02-06 23:20 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, SZEDER Gábor, Jonathan Nieder, Thomas Rast,
Shawn O. Pearce
In-Reply-To: <CAMP44s3SruBpM74BjWuTLfS=_66p7r6rkjJ+ObLr4bLq0nERNA@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
>> I find that what Jonathan gave you helps them much better:
>> ...
>> fn () {
>> var='one two'
>> printf '%s\n' $var
>> }
>> x=$(fn)
>> : ${y=$(fn)}
>>
>> printing "$x" results in two lines as expected, but printing "$y" results
>> in a single line because $var is expanded as a single word when evaluating
>> fn to compute y.
>>
>> So avoid the construct, and use an explicit 'test -n "$foo" || foo=$(bar)'
>> instead.
>>
>> So I'll take the first two lines of the message (good bits), and simplify
>> the "This fixes a bug tht caused..." from the last paragraph (or perhaps
>> even drop it).
>
> I'm not sure about it, because this relies on knowledge of how printf
> works, and it's not used that often; an example with 'for' would be
> much more clear IMO.
Meaning, replace the fn() definition with something like:
fn () {
var='one two'
for v in $var
do
echo "$v"
done
}
I can see that may make the issue easier to see; as you pointed out, it
requires no implicit knowledge that printf "loops" over the arguments
and applies the format string as manu times as needed to eat them.
Let me update the log message before I merge it to 'next'.
My main point was to illustrate the problematic pattern for people who
write for bash, not for zsh, and that does not change with the above
improvement, though ;-).
^ permalink raw reply
* Re: [PATCH] bash-completion: add --edit-description to choices for branch
From: Thomas Rast @ 2012-02-06 23:25 UTC (permalink / raw)
To: Paul Gortmaker; +Cc: git
In-Reply-To: <1328547807-3374-1-git-send-email-paul.gortmaker@windriver.com>
Paul Gortmaker <paul.gortmaker@windriver.com> writes:
> Support was recently added to allow storing a branch description,
> so teach bash completion about it.
See 48c07d8 (completion: --edit-description option for git-branch,
2012-01-29) which is already in next.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 2/3] tag: die when listing missing or corrupt objects
From: Junio C Hamano @ 2012-02-06 23:34 UTC (permalink / raw)
To: Jeff King; +Cc: Tom Grennan, git, jasampler
In-Reply-To: <20120206201245.GA30776@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> Subject: tag: do not show non-tag contents with "-n"
>
> Looks perfect. Thanks.
>
> -Peff
I was an idiot and you were being too polite to point it out X-<.
+ if (type != OBJ_COMMIT || type != OBJ_TAG)
+ goto free_return;
When will I ever get any output from this crap? What kind of object
should I craft to pass through this stupid gate? ;-)
Fixed and requeued.
^ permalink raw reply
* Re: [PATCH v4 1/4] completion: work around zsh option propagation bug
From: Felipe Contreras @ 2012-02-06 23:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, SZEDER Gábor, Jonathan Nieder, Thomas Rast,
Shawn O. Pearce
In-Reply-To: <7vvcnj4kif.fsf@alter.siamese.dyndns.org>
On Tue, Feb 7, 2012 at 1:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> I find that what Jonathan gave you helps them much better:
>>> ...
>>> fn () {
>>> var='one two'
>>> printf '%s\n' $var
>>> }
>>> x=$(fn)
>>> : ${y=$(fn)}
>>>
>>> printing "$x" results in two lines as expected, but printing "$y" results
>>> in a single line because $var is expanded as a single word when evaluating
>>> fn to compute y.
>>>
>>> So avoid the construct, and use an explicit 'test -n "$foo" || foo=$(bar)'
>>> instead.
>>>
>>> So I'll take the first two lines of the message (good bits), and simplify
>>> the "This fixes a bug tht caused..." from the last paragraph (or perhaps
>>> even drop it).
>>
>> I'm not sure about it, because this relies on knowledge of how printf
>> works, and it's not used that often; an example with 'for' would be
>> much more clear IMO.
>
> Meaning, replace the fn() definition with something like:
>
> fn () {
> var='one two'
> for v in $var
> do
> echo "$v"
> done
> }
Yes.
> I can see that may make the issue easier to see; as you pointed out, it
> requires no implicit knowledge that printf "loops" over the arguments
> and applies the format string as manu times as needed to eat them.
> Let me update the log message before I merge it to 'next'.
>
> My main point was to illustrate the problematic pattern for people who
> write for bash, not for zsh, and that does not change with the above
> improvement, though ;-).
True, but I think that's an addendum. Most likely the people working
on this script will be thinking on bash terms, and would not notice if
they introduce code that is difficult for zsh, even if it's carefully
explained in this commit message; it would be zsh people that find the
bug, thus that's why I address the zsh side _first_.
So, IMO it should look like (looks like a fix has been submitted, so
I've updated accordingly):
---
completion: work around zsh option propagation bug
Right now when listing commands in zsh (git <TAB><TAB>), all of them
will show up, instead of only porcelain ones.
This is caused by a bug in zsh[1] that causes subshells to loose the
word splitting option (SH_WORD_SPLIT) since 4.3.0[2]. It will probably
be fixed in the next release (4.3.16).
Basically, in zsh, this:
for i in $__git_all_commands
Should be:
for i in ${=__git_all_commands}
Otherwise there's no word-splitting expansion (unless SH_WORD_SPLIT is
set). sh emulation should take care of that, but the subshell is
messing things up.
The visible result is that __git_list_porcelain_commands don't do any filtering.
Specifically, the issue is with subshells in parmeter expansion
(e.g. '${var=$(func)'):
fn () {
var='one two'
for v in $var; do
echo "$v"
done
}
x=$(fn)
: ${y=$(fn)}
Printing "$x" results in two lines as expected, but printing "$y"
results in a single line because $var is expanded as a single word
(there's no word-splitting expansion).
So avoid the construct, and use an explicit '[ -n "$foo" ] ||
foo=$(bar)' instead.
[1] http://article.gmane.org/gmane.comp.shells.zsh.devel/24296
[2] http://article.gmane.org/gmane.comp.shells.zsh.devel/24338
---
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] bash-completion: add --edit-description to choices for branch
From: Paul Gortmaker @ 2012-02-07 0:16 UTC (permalink / raw)
To: Thomas Rast; +Cc: git
In-Reply-To: <871uq7sfwb.fsf@thomas.inf.ethz.ch>
On 12-02-06 06:25 PM, Thomas Rast wrote:
> Paul Gortmaker <paul.gortmaker@windriver.com> writes:
>
>> Support was recently added to allow storing a branch description,
>> so teach bash completion about it.
>
> See 48c07d8 (completion: --edit-description option for git-branch,
> 2012-01-29) which is already in next.
OK thanks, I did a quick google, didn't get any obvious
clues that it was done already, but must have missed it.
P.
>
^ permalink raw reply
* Re: [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
From: Nguyen Thai Ngoc Duy @ 2012-02-07 0:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder
In-Reply-To: <7vhaz37nce.fsf@alter.siamese.dyndns.org>
2012/2/7 Junio C Hamano <gitster@pobox.com>:
>> + if (i < active_nr)
>> + warning(_("You are committing as-is with intent-to-add entries as the result of\n"
>> + "\"git add -N\". Git currently forbids this case.
>
> Can we phrase this a bit better?
>
> It is not like "forbids", but is "giving up because you didn't tell me
> what content to include in the commit, even though you said you will tell
> me later".
"rejects"? I would rather say "see `git add -N` man page for more
explanation" than putting it here. The warning is quite long as it is
right now.
--
Duy
^ permalink raw reply
* Re: [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
From: Nguyen Thai Ngoc Duy @ 2012-02-07 0:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder
In-Reply-To: <7vwr7z653f.fsf@alter.siamese.dyndns.org>
2012/2/7 Junio C Hamano <gitster@pobox.com>:
> It still is not clear to me how best to sell this change to the end-user
> community.
>
> IIRC, the original motivation of intent-to-add "add -N" was in response to
> users who curse Git because they often forget to add new files before
> committing, and they wanted to say "Here I have a new file, it is not
> complete yet, but I do not want it left out of the commit. While my memory
> is fresh, let me tell Git to keep an eye on it, so that it can remind me
> if I forget to add the final contents." For them, the current "did you
> forget to add them? If so tell me the final contents for at least the
> paths you will be changing with this commit" error was a perfect safety
> solution.
>
> It turned out that the benefits described we see above in the context,
> "This is useful, among other things, ...", were of more value, and for
> these use cases, i-t-a entries ceased to mean "I may forget, so I am
> telling you now, please remind me when I say commit." And "did you
> forget?" error is hinderance for them.
>
> But does that mean nobody will ever need "please remind me lest I forget"?
> Just the original requestor of the "add -N" feature may still be using
> git, but more importantly, isn't it the case that those who have been
> using it merely for the other side effect (e.g. 'git diff') sometimes want
> the "please remind me" safety?
>
> I suspect that some among 1 million Git users would want the "please
> remind me", so a solution with configuration variable without command line
> override is not ideal (command line without any configurability is fine as
> long as we have a good default).
Which is why I prefer adding a new configuration variable (and
optionally a command line option) instead of deprecating current
behavior, because (being lazy) I never be able to find "some among 1
million" so I'm fine with assuming there are some among 1 million that
favors safety over convenience.
> I am beginning to think "safety by default, which can be turned off by
> learned users, but still can be turned on on demand" may be a lot easier
> to sell this. That is:
>
> - commit.ignoreIntentToAdd defaults to `false`; the default will never
> change. The users can set it to `true`.
>
> - "commit --ignore-intent-to-add" can be used without setting the
> configuration or to defeat an explicit `false`, for a one-shot request.
>
> - "commit --honor-intent-to-add" can be used to defeat an explicit
> `true`, for a one-shot request.
>
> The third one is a bit funny, as it is a way to bring back safety when the
> user earlier decided that he does not need that kind of safety (i.e. "I
> only say 'add -N' for `diff` and stuff, I will never forget to add real
> contents before committing"), so it will almost never be used, because
> these users who set 'ignoreIntentToAdd = true' do _not_ expect Git to help
> them in remembering to add the real contents. And having to add a funny
> option just for the sake of completeness is often an indication that there
> is something fundamentally wrong in the system that the option tries to
> express an interface into it.
Well, that --honor-intent-to-add could be renamed as
--no-ignore-intent-to-add. The --[no-]ignore-intent-to-add pair
functions as a way to override default behavior/config var. No extra
could required. "git commit -h" just does not show it.
We need better option/config names though, --ignore-intent-to-add
looks way too long to type and it's not clear what it does without
looking up "git add -N".
--
Duy
^ permalink raw reply
* Re: [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
From: Jonathan Nieder @ 2012-02-07 0:59 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, git
In-Reply-To: <CACsJy8Be=WLv1Zz5CqQMYfi8dJN6M980kVuckAQKAujVo8xFkQ@mail.gmail.com>
Nguyen Thai Ngoc Duy wrote:
> 2012/2/7 Junio C Hamano <gitster@pobox.com>:
>>> + if (i < active_nr)
>>> + warning(_("You are committing as-is with intent-to-add entries as the result of\n"
>>> + "\"git add -N\". Git currently forbids this case.
>>
>> Can we phrase this a bit better?
>>
>> It is not like "forbids", but is "giving up because you didn't tell me
>> what content to include in the commit, even though you said you will tell
>> me later".
>
> "rejects"? I would rather say "see `git add -N` man page for more
> explanation" than putting it here. The warning is quite long as it is
> right now.
If I ruled the world, it would say something like this:
error: you intended to add "foo.c" but did not add it; not committing
hint: to commit all changes to tracked files, use "commit -a"
hint: to commit without adding "foo.c", use "commit --ignore-intent-to-add", which may become the default in future versions of git
But without the long line. ;-)
^ permalink raw reply
* Re: [PATCH 3/4] commit: turn commit.ignoreIntentToAdd to true by default
From: Nguyen Thai Ngoc Duy @ 2012-02-07 1:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder
In-Reply-To: <7v8vkf7mrn.fsf@alter.siamese.dyndns.org>
2012/2/7 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> - if (!set_commit_ignoreintenttoadd) {
>
> Now no warning() is associated with testing of this flag, which means that
> people who didn't get around to read the doc and react to the warnings in
> the earlier releases will get _nothing_ when the real change hits them?
>
> That sounds strangely bad. What am I missing?
Well if they stumble upon this case during the previous deprecation
phase, they ought to set commit.ignoreIntentToAdd. If it's not set,
they probably do not have prior experience with this feature. With
their fresh mind, they hopefully learn the new default behavior via
to-be-updated git-add.txt so no warnings for them.
--
Duy
^ permalink raw reply
* Re: [PATCH 2/4] commit: introduce a config key to allow as-is commit with i-t-a entries
From: Junio C Hamano @ 2012-02-07 1:13 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Jonathan Nieder
In-Reply-To: <CACsJy8C5=JKHsjyaSFsxxyScb1CCQZmh4gXSAgsyJ8DFkjfQ0g@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> 2012/2/7 Junio C Hamano <gitster@pobox.com>:
>
>> I suspect that some among 1 million Git users would want the "please
>> remind me", so a solution with configuration variable without command line
>> override is not ideal (command line without any configurability is fine as
>> long as we have a good default).
>
> Which is why I prefer adding a new configuration variable (and
> optionally a command line option) instead of deprecating current
> behavior, because (being lazy) I never be able to find "some among 1
> million" so I'm fine with assuming there are some among 1 million that
> favors safety over convenience.
If there is one thing I want to absolutely avoid, it is to split the
userbase into many pieces by giving sticky configuration variables, so
the above argument is not a good starting point.
>> The third one is a bit funny, as it is a way to bring back safety when the
>> .... And having to add a funny
>> option just for the sake of completeness is often an indication that there
>> is something fundamentally wrong in the system that the option tries to
>> express an interface into it.
>
> Well, that --honor-intent-to-add could be renamed as
> --no-ignore-intent-to-add.
I wasn't talking about the name at all. What is _funny_ is the semantics.
A "by default unsafe" configuration introduces a need for an option to be
extra careful only when matters, but "an option" to be extra careful by
definition is easy to forget, so it is no longer a safety at all, iow,
people who want "by default unsafe" will get "always unsafe". And it
probably is perfectly fine because to them, forgetting to add 'add -N'
entries is not a mistake at all, but always is a deliberate act.
Another thing I am somewhat worried about is if there are existing scripts
that create commits and relies on the current "we cannot commit because
the final contents is not known yet". I didn't check but for example how
well does "git stash" work when the default is flipped to "just ignore"?
^ permalink raw reply
* Re: Git performance results on a large repository
From: Nguyen Thai Ngoc Duy @ 2012-02-07 1:19 UTC (permalink / raw)
To: Sam Vilain
Cc: Joshua Redstone, Ævar Arnfjörð Bjarmason,
git@vger.kernel.org
In-Reply-To: <4F2C6276.1070100@vilain.net>
On Sat, Feb 4, 2012 at 5:40 AM, Sam Vilain <sam@vilain.net> wrote:
> There have also been designs at various times for sparse check–outs; ie
> check–outs where you don't check out the root of the repository but a
> sub–tree.
There is a sparse checkout feature in git (hopefully from one of the
designs you mentioned) and it can checkout subtrees. The only problem
in this case is it maintains full index. So it only solves half of the
problem (stat calls), reading/writing large index just slows
everything down.
--
Duy
^ permalink raw reply
* Re: Git performance results on a large repository
From: david @ 2012-02-07 1:28 UTC (permalink / raw)
To: Joshua Redstone
Cc: Nguyen Thai Ngoc Duy, Joey Hess, dgma@mohsinc.com, Matt Graham,
Tomas Carnecky, Greg Troxel, David Barr, git@vger.kernel.org
In-Reply-To: <CB55A6A4.40AFD%joshua.redstone@fb.com>
On Mon, 6 Feb 2012, Joshua Redstone wrote:
> David Lang and David Barr, I generated the pack files by doing a repack:
> "git repack -a -d -f --max-pack-size=10g --depth=100 --window=250" after
> generating the repo.
how many pack files does this end up creating?
I think that doing a full repack the way you did will group all revisions
of a given file into a pack.
while what I'm saying is that if you create the packs based on time,
rather than space efficiency of the resulting pack files, you may end up
not having to go through as much date when doing things like a git blame.
what you did was
initialize repo
4M commits
repack
what I'm saying is
initialize repo
loop
500K commits
repack (and set pack to .keep so it doesn't get overwritten)
so you will end up with ~8 sets of pack files, but time based so that when
you only need recent information you only look at the most recent pack
file. If you need to go back through all time, the multiple pack files
will be a little more expensive to process.
this has the added advantage that the 8 small repacks should be cheaper
than the one large repack as it isn't trying to cover all commits each
time.
David Lang
^ permalink raw reply
* Re: [PATCH 3/6] Stop producing index version 2
From: Shawn Pearce @ 2012-02-07 3:09 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc, git, Thomas Rast,
Joshua Redstone
In-Reply-To: <7v4nv4a131.fsf@alter.siamese.dyndns.org>
2012/2/5 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> read-cache.c learned to produce version 2 or 3 depending on whether
>> extended cache entries exist in 06aaaa0 (Extend index to save more flags
>> - 2008-10-01), first released in 1.6.1. The purpose is to keep
>> compatibility with older git. It's been more than three years since
>> then and git has reached 1.7.9. Drop support for older git.
>
> Cc'ing this, as I suspect this would surely raise eyebrows of some people
> who wanted to get rid of the version 3 format.
Version 3 was a mistake because of the variable length record sizes.
Saving 2 bytes on some records that don't use the extended flags makes
the index file *MUCH* harder to parse. So much so that we should take
version 3 and kill it, not encourage it as the default!
IMHO, when these extended flags were added to make version 3 the
following should have happened:
- All records use the larger structure format with 4 bytes for the
flags, not 2 bytes.
- Change the trailing padding after the name to be a *SINGLE* \0 byte,
and do not pad out to an 8 byte boundary.
Both make it really hard to process the file, and the latter happens
only for direct mmap usage, which we don't do anymore.
We also have to consider the EGit and JGit user base as part of the
ecosystem. We can't just kill a file format because git-core has been
capable of reading its alternative since some arbitrary YYYY-MM-DD
release date. We need to also consider when did some other major tools
catch up and also support this format?
FWIW JGit released index version 3 support in version 0.9.1, which
shipped Sep 15, 2010. JGit/EGit were more than 2 years behind here.
<thinking type="wishful" probability="never-happen"
probably-inflating-flame-from="linus">
I have long wanted to scrap the current index format. I unfortunately
don't have the time to do it myself. But I suspect there may be a lot
of gains by making the index format match the canonical tree format
better by keeping the tree structure within a single file stream,
nesting entries below their parent directory, and keeping tree SHA-1
data along with the directory entry. For one thing the index would be
able to register an empty subdirectory, rather than ignoring them. It
would also better line up with the filesystem's readdir() handling,
giving us more sane logic to compare what readdir() tells us exists
against what the index thinks should be in the same file. And the
overall index should be smaller, because we don't have to repeat the
same path/to/a/file/for/every/file/in/that/same/directory/tree.
Reconstructing the path strings at read time into a flat list should
be pretty trivial, and still keep the parallel lstat calls running off
a flat list working well for fast status operations.
</thinking>
^ permalink raw reply
* Re: [PATCH 5/6] Allow to use crc32 as a lighter checksum on index
From: Shawn Pearce @ 2012-02-07 3:17 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Thomas Rast, Joshua Redstone
In-Reply-To: <1328507319-24687-5-git-send-email-pclouds@gmail.com>
2012/2/5 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> if (hdr->hdr_signature != htonl(CACHE_SIGNATURE))
> return error("bad signature");
> - if (hdr->hdr_version != htonl(2) &&
> - hdr->hdr_version != htonl(3) &&
> - hdr->hdr_version != htonl(4))
> + if (hdr->hdr_version == htonl(2) ||
> + hdr->hdr_version == htonl(3))
> + do_crc = 0;
> + else if (hdr->hdr_version == htonl(4)) {
> + struct ext_cache_header *ehdr = (struct ext_cache_header *)hdr;
> + do_crc = ntohl(ehdr->hdr_flags) & CACHE_F_CRC;
> + }
> + else
Ick. Ick. Ick. Please $DEITY no.
When it comes to data integrity codes in Git... PICK ONE AND STICK WITH IT.
If CRC-32 is good enough to protect the index content such that disk
corruption is probably detectable with it, lets just switch to CRC-32
in index version 4. Don't make it optional with a new header field
that wasn't there in version 3 and is now only able to accept 32 bits
of flags before we have to go and create index version 5. We already
have a cache extension system available with extension codes in the
footer of the index file. We don't need YET ANOTHER EXTENSION SYSTEM.
If CRC-32 is not good enough, and we don't want to trust it (or
really, YOU don't want to trust it) please do not then go and propose
that a less knowledgeable user should switch to CRC-32 "because it is
faster". If we don't want to rely on the error detection of CRC-32,
then we should be using SHA-1. Or SHA-256.
I haven't really put a lot of thought into this. But I suspect CRC-32
is sufficient on the index file, until it gets so big that the
probability of a bit flip going undetected is too high due to the size
of the file, but then we are into the "huge" index size range that has
you trying to swap out SHA-1 for CRC-32 because SHA-1 is too slow. Uhm
no.
CRC-32 may be good enough, we use it inside of the pack-objects when
doing repacking locally and don't want to inflate objects to check
SHA-1, but do want to try and detect a random bit flip caused by a
broken file copier. Thus far its held up well there. Given the very
transient nature of the index file (and how it can be mostly rebuilt
from a tree object and the working directory), CRC-32 might be good
enough. But please pick one.
^ permalink raw reply
* Re: [PATCH 5/6] Allow to use crc32 as a lighter checksum on index
From: Dave Zarzycki @ 2012-02-07 4:04 UTC (permalink / raw)
To: Shawn Pearce
Cc: Nguyễn Thái Ngọc Duy, git, Thomas Rast,
Joshua Redstone
In-Reply-To: <CAJo=hJvSyhv8EUh=6ROotc3Q=zQo7vbww_ShQJP3tf1T7s889g@mail.gmail.com>
On Feb 6, 2012, at 7:17 PM, Shawn Pearce <spearce@spearce.org> wrote:
> I haven't really put a lot of thought into this. But I suspect CRC-32
> is sufficient on the index file, until it gets so big that the
> probability of a bit flip going undetected is too high due to the size
> of the file, but then we are into the "huge" index size range that has
> you trying to swap out SHA-1 for CRC-32 because SHA-1 is too slow. Uhm
> no.
CRCs are designed to be implemented in hardware and provide basic single-bit error checking for networking packets of disk blocks. With a good polynomial, they're reasonably effective at detecting a single-bit error within 8 or 16 kilobytes:
http://www.ece.cmu.edu/~koopman/networks/dsn02/dsn02_koopman.pdf
^ permalink raw reply
* [PATCH] add -e: ignore dirty submodules
From: Johannes Schindelin @ 2012-02-07 4:05 UTC (permalink / raw)
To: gitster; +Cc: git
We cannot add untracked/modified files in submodules anyway.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
This patch is actually from Oct 23, 2010.
builtin/add.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index 1c42900..b79336d 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -280,6 +280,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
argc = setup_revisions(argc, argv, &rev, NULL);
rev.diffopt.output_format = DIFF_FORMAT_PATCH;
+ DIFF_OPT_SET(&rev.diffopt, IGNORE_DIRTY_SUBMODULES);
out = open(file, O_CREAT | O_WRONLY, 0644);
if (out < 0)
die (_("Could not open '%s' for writing."), file);
--
1.7.9.msysgit.0.27.ge92cd
^ permalink raw reply related
* Re: [PATCH 5/6] Allow to use crc32 as a lighter checksum on index
From: Dave Zarzycki @ 2012-02-07 4:29 UTC (permalink / raw)
To: Shawn Pearce
Cc: Nguyễn Thái Ngọc Duy, git, Thomas Rast,
Joshua Redstone
In-Reply-To: <159A2D07-0B02-4E85-B7AA-C668FDA9F382@apple.com>
On Feb 6, 2012, at 8:04 PM, Dave Zarzycki <zarzycki@apple.com> wrote:
> On Feb 6, 2012, at 7:17 PM, Shawn Pearce <spearce@spearce.org> wrote:
>
>> I haven't really put a lot of thought into this. But I suspect CRC-32
>> is sufficient on the index file, until it gets so big that the
>> probability of a bit flip going undetected is too high due to the size
>> of the file, but then we are into the "huge" index size range that has
>> you trying to swap out SHA-1 for CRC-32 because SHA-1 is too slow. Uhm
>> no.
>
> CRCs are designed to be implemented in hardware and provide basic single-bit error checking for networking packets of disk blocks. With a good polynomial, they're reasonably effective at detecting a single-bit error within 8 or 16 kilobytes:
>
> http://www.ece.cmu.edu/~koopman/networks/dsn02/dsn02_koopman.pdf
s/packets of disk blocks/packets or disk blocks/g
^ permalink raw reply
* Re: [PATCH] git-add: allow --ignore-missing always, not just in dry run
From: Mike Gant @ 2012-02-07 4:39 UTC (permalink / raw)
To: git; +Cc: Dieter Plaetinck
In-Reply-To: <1326923544-8287-1-git-send-email-dieter@plaetinck.be>
On Wed, Jan 18, 2012 at 10:52:24PM +0100, Dieter Plaetinck wrote:
> There is no need to restrict use of --ignore-missing to dry runs,
> it can be useful to ignore missing files during normal operation as
> well.
FWIW I would be in favor of this change and I was going to submit a
patch, too. My use case is different, though. I create branches that
will never be merged to the mainline because they have files added that
I don't want in master. The files added to these branches can vary. In
my script to create the branch, I want the largest possible set of these
files as the argument to 'git add' but if not all exist it's okay. I
realize I can write my script to only add the files that exist but I'm
lazy ;) and the --ignore-missing option would be easier.
Mike
^ permalink raw reply
* Re: [PATCH 3/6] Stop producing index version 2
From: Nguyen Thai Ngoc Duy @ 2012-02-07 4:50 UTC (permalink / raw)
To: Shawn Pearce; +Cc: Junio C Hamano, git, Thomas Rast, Joshua Redstone
In-Reply-To: <CAJo=hJvtRnmvALcn3vKpYTr3j6ada8iboPjWN3cQnwwKzRvrDA@mail.gmail.com>
On Tue, Feb 7, 2012 at 10:09 AM, Shawn Pearce <spearce@spearce.org> wrote:
> 2012/2/5 Junio C Hamano <gitster@pobox.com>:
>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>
>>> read-cache.c learned to produce version 2 or 3 depending on whether
>>> extended cache entries exist in 06aaaa0 (Extend index to save more flags
>>> - 2008-10-01), first released in 1.6.1. The purpose is to keep
>>> compatibility with older git. It's been more than three years since
>>> then and git has reached 1.7.9. Drop support for older git.
>>
>> Cc'ing this, as I suspect this would surely raise eyebrows of some people
>> who wanted to get rid of the version 3 format.
>
> Version 3 was a mistake because of the variable length record sizes.
> Saving 2 bytes on some records that don't use the extended flags makes
> the index file *MUCH* harder to parse. So much so that we should take
> version 3 and kill it, not encourage it as the default!
Probably too late for that, but it's good to know there are strong
user base for v2.
> <thinking type="wishful" probability="never-happen"
> probably-inflating-flame-from="linus">
>
> I have long wanted to scrap the current index format. I unfortunately
> don't have the time to do it myself. But I suspect there may be a lot
> of gains by making the index format match the canonical tree format
> better by keeping the tree structure within a single file stream,
> nesting entries below their parent directory, and keeping tree SHA-1
> data along with the directory entry. For one thing the index would be
> able to register an empty subdirectory, rather than ignoring them. It
> would also better line up with the filesystem's readdir() handling,
> giving us more sane logic to compare what readdir() tells us exists
> against what the index thinks should be in the same file. And the
> overall index should be smaller, because we don't have to repeat the
> same path/to/a/file/for/every/file/in/that/same/directory/tree.
> Reconstructing the path strings at read time into a flat list should
> be pretty trivial, and still keep the parallel lstat calls running off
> a flat list working well for fast status operations.
>
> </thinking>
Haven't really thought through, but I suppose we could create extended
tree object format (there is info in cache entry that's not in tree
entry), store index in this format, then pack together and store the
pack as part of index file. Append-only access to index would be
possible by appending a new pack of new trees to index) I think with
tree-based index, we could kill a big chunk of code (merging trees and
index together) in unpack_trees(). With further efforts to remove
list-based index usage, we could even kill match_pathspec_depth(),
making tree_entry_interesting() the only function to match patchspec.
But dreams probably never come true.
--
Duy
^ permalink raw reply
* Re: [PATCH 0/2] config includes, take 2
From: David Aguilar @ 2012-02-07 5:01 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120206062713.GA9699@sigill.intra.peff.net>
On Sun, Feb 5, 2012 at 10:27 PM, Jeff King <peff@peff.net> wrote:
> That leaves the file-inclusion bits:
>
> [1/2]: imap-send: remove dead code
> [2/2]: config: add include directive
>
> The first patch is new in this round, and is a necessary cleanup for the
> second patch (though it might be worth applying regardless).
>
> The second patch corresponds to patch 1/4 from the previous round. Among
> the functional changes are:
>
> 1. do not use includes for "git config" in single-file mode
I have a questions about this. Let's say I have ~/foo1.gitconfig:
[foo]
bar = 1
...and ~/.gitconfig (I forgot the syntax):
[foo]
bar = 0
#include "~/foo1.gitconfig"
Does that mean that:
$ git config -f ~/.gitconfig foo.bar
...prints 0 and not 1?
I can see cases where this would be undesirable behavior.
For example, an application that uses "git config -z --list -f
~/.gitconfig" might expect that the result encompasses all of the
user-specific config bits.
Following this to its natural conclusion means "git config" might
learn some kind of --no-include flag for use with e.g. "git config
--no-include -f ~/.gitconfig". That said, I don't know where I would
ever actually use such a flag.
I do know where I would use an `--include` flag (if following includes
were not the default behavior when using '-f'), though, and that's why
I'm asking. The problem with not having it be the default means we
have to use a flag. This makes it harder to support multiple versions
of git.
Maybe I'm mis-interpreting what you mean by, 'do not use includes for
"git config" in single-file mode', though.
--
David
^ permalink raw reply
* Re: [PATCH 0/2] config includes, take 2
From: Jeff King @ 2012-02-07 5:17 UTC (permalink / raw)
To: David Aguilar; +Cc: git
In-Reply-To: <CAJDDKr6A2UvB3D-Dapw7WCEzWfzLoLd0E8MSDjT0RtdxFeWZAQ@mail.gmail.com>
On Mon, Feb 06, 2012 at 09:01:21PM -0800, David Aguilar wrote:
> I have a questions about this. Let's say I have ~/foo1.gitconfig:
>
> [foo]
> bar = 1
>
> ...and ~/.gitconfig (I forgot the syntax):
>
> [foo]
> bar = 0
>
> #include "~/foo1.gitconfig"
>
>
> Does that mean that:
>
> $ git config -f ~/.gitconfig foo.bar
>
> ...prints 0 and not 1?
Yes, though the syntax is:
[include]
path = foo1.gitconfig
(it doesn't respect tilde-expansion, but it probably should). Note that
the syntax was specifically selected for backwards compatibility, and to
allow manipulation with existing tools.
> I can see cases where this would be undesirable behavior.
>
> For example, an application that uses "git config -z --list -f
> ~/.gitconfig" might expect that the result encompasses all of the
> user-specific config bits.
The problem is that an application might also expect it _not_ to happen
(e.g., if the application is really interested in knowing what's in
~/.gitconfig). Because includes aren't respected now, the safe default
seems to be not to have includes (i.e., don't change behavior for this
case).
A bigger question for me is: if you are interested in getting an answer
from anywhere, why are you restricting with "-f"? IOW, is this a
real-world problem, or a hypothetical one? And if real-world, what is
the actual use case?
> Following this to its natural conclusion means "git config" might
> learn some kind of --no-include flag for use with e.g. "git config
> --no-include -f ~/.gitconfig". That said, I don't know where I would
> ever actually use such a flag.
It already learned it as part of my series (and --includes, as well).
> I do know where I would use an `--include` flag (if following includes
> were not the default behavior when using '-f'), though, and that's why
> I'm asking. The problem with not having it be the default means we
> have to use a flag. This makes it harder to support multiple versions
> of git.
Yes, that's a general problem of adding new command-line options to turn
features off or on. We could add an environment variable to control this
feature. But I'd really like to hear the compelling use case first
(i.e., both why it cannot simply be spelled "git config -z --list", and
why not following includes is not OK).
> Maybe I'm mis-interpreting what you mean by, 'do not use includes for
> "git config" in single-file mode', though.
No, I think you understand the described behavior.
-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