* 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: 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] 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 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 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] 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 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: [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: 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: [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: [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 1/2] docs: add a basic description of the config API
From: Junio C Hamano @ 2012-02-06 22:31 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120206095346.GA4300@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This wasn't documented at all; this is pretty bare-bones,
> but it should at least give new git hackers a basic idea of
> how the reading side works.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Documentation/technical/api-config.txt | 101 ++++++++++++++++++++++++++++++++
> 1 files changed, 101 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/technical/api-config.txt
>
> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
> new file mode 100644
> index 0000000..f428c5c
> --- /dev/null
> +++ b/Documentation/technical/api-config.txt
> @@ -0,0 +1,101 @@
> +config API
> +==========
> +
> +The config API gives callers a way to access git configuration files
> +(and files which have the same syntax). See linkgit:git-config[1] for a
> +discussion of the config file syntax.
> +
> +General Usage
> +-------------
> +
> +Config files are parsed linearly, and each variable found is passed to a
> +caller-provided callback function. The callback function is responsible
> +for any actions to be taken on the config option, and is free to ignore
> +some options (it is not uncommon for the configuration to be parsed
> +several times during the run of a git program, with different callbacks
> +picking out different variables useful to themselves).
It woud be easeier to read if you stopped the sentence after "some
options" and made the "It is not uncommon..." a first-class sentence
outside the parentheses.
> +A config callback function takes three parameters:
> +
> +- the name of the parsed variable. This is in canonical "flat" form: the
> + section, subsection, and variable segments will be separated by dots,
> + and the section and variable segments will be all lowercase. E.g.,
> + `core.ignorecase`, `diff.SomeType.textconv`.
> +
> +- the value of the found variable, as a string. If the variable had no
> + value specified, the value will be NULL (typically this means it
> + should be interpreted as boolean true).
> +
> +- a void pointer passed in by the caller of the config API; this can
> + contain callback-specific data
> +
> +A config callback should return 0 for success, or -1 if the variable
> +could not be parsed properly.
This matches what I have always thought, but I think I recently saw a
series that adds callbacks that return 1 to mean "I have understood this
variable, so callers should not look at it any more". It felt wrong, but
I did not find anything in the config.c API framework to prvent such a
local calling convention.
> +Basic Config Querying
> +---------------------
> +
> +Most programs will simply want to look up variables in all config files
> +that git knows about, using the normal precedence rules. To do this,
> +call `git_config` with a callback function and void data pointer.
> +
> +`git_config` will read all config sources in order of increasing
> +priority. Thus a callback should typically overwrite previously-seen
> +entries with new ones (e.g., if both the user-wide `~/.gitconfig` and
> +repo-specific `.git/config` contain `color.ui`, the config machinery
> +will first feed the user-wide one to the callback, and then the
> +repo-specific one; by overwriting, the higher-priority repo-specific
> +value is left at the end).
> +
> +There is a special version of `git_config` called `git_config_early`
> +that takes an additional parameter to specify the repository config.
> +This should be used early in a git program when the repository location
> +has not yet been determined (and calling the usual lazy-evaluation
> +lookup rules would yield an incorrect location).
Do you want to say somethink like "Ordinary programs should not have to
worry about git_config_early()"? Differently put, if you are learning the
config API by reading this document and cannot tell which one you should
be calling, you are way too inexperienced to call git_config_early() and
you would always want to call git_config()?
^ permalink raw reply
* Re: git describe relative to a given tag pattern
From: Adam Mercer @ 2012-02-06 22:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vhaz362b8.fsf@alter.siamese.dyndns.org>
On Mon, Feb 6, 2012 at 16:10, Junio C Hamano <gitster@pobox.com> wrote:
> Perhaps Git is correct and your HEAD is not a descendant of any tag whose
> name matches the pattern "lal-v*".
Thanks you're right, the tags I was trying to match are on a different
branch. Back to the drawing board for this script.
Cheers
Adam
^ permalink raw reply
* Re: git describe relative to a given tag pattern
From: Junio C Hamano @ 2012-02-06 22:10 UTC (permalink / raw)
To: Adam Mercer; +Cc: git
In-Reply-To: <CA+mfgz0VB9qMHHtoT76zCOiUaH=8egdMDrneQVRBug2waQsGAg@mail.gmail.com>
Adam Mercer <ramercer@gmail.com> writes:
> $ git describe --match=lal-v* HEAD
> fatal: No tags can describe 'cee13cbb25d0fa38f6e3bc6622bc751ebf35d2f0'.
Perhaps Git is correct and your HEAD is not a descendant of any tag whose
name matches the pattern "lal-v*".
Here is how people can mimic the situation:
$ git checkout v1.7.6~20
$ git describe --match='v1.7.*' HEAD
v1.7.6-rc0-3-g6c92972
$ git describe --match='v1.7.[7-9]*' HEAD
fatal: No tags can describe '6c92972d7f5ab247a8cab5e4b88cb281bf201970'.
Try --always, or create some tags.
^ permalink raw reply
* Re: [bug] blame duplicates trailing ">" in mailmapped emails
From: Junio C Hamano @ 2012-02-06 22:04 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Jeff King, Jonathan Nieder, git, SZEDER Gábor
In-Reply-To: <CAMP44s1RN+_UK9rAk_m9Z=YaJJtwHLyiCu2stMMDEWqZN9260g@mail.gmail.com>
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. 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. 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.
^ permalink raw reply
* git describe relative to a given tag pattern
From: Adam Mercer @ 2012-02-06 22:03 UTC (permalink / raw)
To: git
Hi
I'm trying to use git describe to describe the current status of my
tree with respect a given tag, or more specifically the latest tag
matching a specific pattern.
My repository has the following tags:
$ git tag -l lal-v*
lal-v6.6.0
lal-v6.6.0rc1
lal-v6.6.0rc2
lal-v6.6.1
lal-v6.6.2
$
And I want git describe to use the latest of these tags, so looking at
the git describe man page it seems that the --match option can allow
me to specify a pattern. This, however, doesn't seem to work:
$ git describe --match=lal-v* HEAD
fatal: No tags can describe 'cee13cbb25d0fa38f6e3bc6622bc751ebf35d2f0'.
Try --always, or create some tags.
$
quoting the pattern has no effect, what am I doing wrong here?
Cheers
Adam
PS: I'm using git-1.7.9
^ 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-06 21:48 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20120206211321.GA2949@burratino>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Junio C Hamano wrote:
>
>> 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."
>
> I agree with everything up to here. But I believe these people were
> _already_ paying attention to "git status" output from the commandline
> and in the editor window when they run "git commit", to notice other
> changes they forgot to add, too. I don't think this series would
> inconvenience them.
That means that you are willing to declare that nobody will ever need
"please remind me lest I forget". Not just the original requestor of the
"add -N" feature, but absolutely nobody else.
Then the deprecation sequence presented in this series is fine. The
wording to sell that "removal of misfeature" to the end user community
needs to be well thought out, though.
^ permalink raw reply
* Re: [PATCH 3/3] t: mailmap: add simple name translation test
From: Felipe Contreras @ 2012-02-06 21:32 UTC (permalink / raw)
To: Jeff King
Cc: Jonathan Nieder, git, Junio C Hamano, Marius Storm-Olsen,
Jim Meyering
In-Reply-To: <20120204234237.GB1366@sigill.intra.peff.net>
On Sun, Feb 5, 2012 at 1:42 AM, Jeff King <peff@peff.net> wrote:
> On Sun, Feb 05, 2012 at 12:19:53AM +0200, Felipe Contreras wrote:
>
>> > Thanks. I guess you think I'm stupid. I have no idea how I can
>> > correct that assumption and help you to actually work with me to make
>> > the code better. :/
>>
>> You mean the commit message, you haven't made any comment about the code.
>>
>> If you want to know why I had to modify those test assertions, you
>> really need to look at the code. In essence; all of them use the same
>> repo, and obviously adding a new commit message changes the output of
>> the commands.
>
> Then say that in the commit message.
I believe that's overkill. If somebody needs an explanation, it's
because they are not familiar with the code being modified, and
introducing people to some code in each and every patch that modifies
it definitely seems like overkill to me. There is not even such
introduction in the code itself for this test, and in most of git's
code, presumably because we are following the principle of having
self-documented code.
If you look at the code, it would become obvious why so many hunks are
introduced, in fact, if you look closely at the patch you can see it
as well: look for strings related to CTO <cto@company.xx>.
> Looking at this series, I wonder if the tests should simply be squashed
> into the bugfix patch, which might make what is going on more obvious.
Because it's a logically independent change?
There's _nothing_ that prevents this patch from being applied to
master *right now*. Of course, it would conflict, because it depends
on the 'git blame -e' tests, but if you solve that conflict by just
removing that hunk, it would apply and run just fine, and it would
detect regressions orthogonal from my other proposed patches.
> Keep in mind that as reviewers now, we read the whole series. But in a
> year, as "git log" users, we may see the commits in isolation.
Sure, and this patch by itself is good; it's adding a missing test
(even if you ignore the 'git blame -e' part).
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [RFC/PATCH] add update to branch support for "floating submodules"
From: Jens Lehmann @ 2012-02-06 21:32 UTC (permalink / raw)
To: Phil Hord; +Cc: Junio C Hamano, Leif Gruenwoldt, git
In-Reply-To: <CABURp0rt=LcjMfDU61m0de-gLpX1a3x3vhb0zVxCbceSvD9jFw@mail.gmail.com>
Am 06.02.2012 18:31, schrieb Phil Hord:
> On Wed, Feb 1, 2012 at 5:37 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>> Hmm, I really think the fact that submodules are unaware that they
>> are part of a superproject is a feature. I'd prefer seeing that kind
>> of problem being tackled by the CI server and/or user education. Or
>> maybe a pre-commit hook which issues a warning in that case?
>
> I agree that submodule isolation is a feature essential to the
> architecture of git and the submodules implementation. But it is also
> a limitation, not just of this example. A pre-commit hook is a nice
> idea, but it doesn't help 'git status' (which is the standard go-to
> answer point for "where am I").
Yes, this feature also is a limitation. To put it in other words: I want
each submodule to be a full fledged repo of its own. IMO it must always
be possible to just clone a submodule without any superproject and run
any git command in it. And the other way around: each superproject must
be usable as a submodule in another superproject. That makes adding some
kind of "superproject awareness" in a sane way rather difficult, as a
repo can't say "I live in a superproject" or "I am the topmost project".
> This has me thinking more about recursing siblings now, though. I find
> myself typing something like this quite a lot:
> git submodule foreach 'git grep "someFunction" || :'
There was an attempt to teach git grep the --recurse-submodules option,
but unfortunately it looks like this didn't lead anywhere so far.
> Or worse (in that the UI is more unwieldy):
> git submodule foreach 'git log --oneline "-SsomeFunction" || :'
This could also be done by teaching git log the --recurse-submodules
option. Me thinks in the long run a lot of git commands should learn
that option to make it easy to optionally include submodules in
whatever they are doing. But my focus is on recursive checkout for the
next time, so I have no idea when I find some time to do that.
> But what I want is this:
> git --git-dir=${TOP}/../.git grep --recurse-submodules "someFunction"
>
> But not really, because I am lazy and that is too much typing.
> git grep --include-siblings "someFunction"
>
> Maybe I can add a "sib" macro to get this:
> git sib grep "someFunction"
And from what you where saying earlier a "git sib status" would be nice
too? What about using alias commands for that functionality? They could
point to a script which searches the topmost repo and calls "git submodule
foreach 'git <command> "$@" || :'" from there ...
^ permalink raw reply
* Re: [PATCH 3/3] t: mailmap: add simple name translation test
From: Felipe Contreras @ 2012-02-06 21:18 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Junio C Hamano, Marius Storm-Olsen, Jim Meyering
In-Reply-To: <20120205061738.GB1870@burratino>
On Sun, Feb 5, 2012 at 8:17 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> You mean the commit message, you haven't made any comment about the code.
>
> No, for this patch, more important than the absence of any explanation
> in the commit message (which is also important) is the code change
> that seems unnecessarily invasive.
It only seems that way if you are not familiar with the code, and you
assume the worst.
Feel free propose an alternative change. You would find there's no
better way to simplify those changes.
Here's a tip: look at the previous change "CTO <cto@company.xx>", and
see how it's right next to each an every of the hunks introduced in
this patch.
> You've already demonstrated that I do not have the right communication
> style to explain such things to you and work towards a fix that
> addresses both our concerns. So I give up.
You are assuming your concerns are valid without actually looking at the code.
> I'll just give my
> feedback on patches that concern code I care about and an explanation
> for the sake of others on the list that are better able to interact
> with you. I am willing to work with or answer questions from anyone
> including you, though.
Your feedback is most certainly welcome, but you shouldn't assume you
are always right. There's nothing wrong with disagreeing, and in this
case I most definitely disagree, and there's nothing wrong with that.
--
Felipe Contreras
^ permalink raw reply
* Re: Git performance results on a large repository
From: Sam Vilain @ 2012-02-06 21:17 UTC (permalink / raw)
To: Joshua Redstone; +Cc: Nguyen Thai Ngoc Duy, git@vger.kernel.org
In-Reply-To: <243C23AF01622E49BEA3F28617DBF0AD5912CA85@SC-MBX02-5.TheFacebook.com>
> Sam Vilain: Thanks for the pointer, i didn't realize that
> fast-import was bi-directional. I used it for generating the
> synthetic repo. Will look into using it the other way around.
> Though that still won't speed up things like git-blame,
> presumably?
It could, because blame is an operation which primarily works on
the source history with little reference to the working copy. Of
course this will depend on the quality of the implementation
server-side. Blame should suit distribution over a cluster, as
it is mostly involved with scanning candidate revisions for
string matches which is the compute intensive part. Coming up
with candidate revisions has its own cost and can probably also
be distributed, but just working on the lowest loop level might
be a good place to start.
What it doesn't help with is local filesystem operations. For
this I think a different approach is required, if you can tie
into fam or a similar inode change notification system, then you
should be able to avoid the entire recursive stat on 'git
status'. I'm not sure --assume-unchanged on its own is a good
idea, you could easily miss things. Those stat's are useful.
Making the index able to hold just changes to the checked-out
tree, as others have mentioned, would also save the massive reads
and writes you've identified. Perhaps a more high performance
back-end could be developed.
> The sparse-checkout issue you mention is a good one.
It's actually been on the table since at least GitTogether 2008;
there's been some design discussion on it and I think it's just
one of those features which doesn't have enough demand yet for it
to be built. It keeps coming up but not from anyone with the
inclination or resources to make it happen. There is a protocol
issue, but this should be able to fit into the current extension
system.
> There is a good question of how to support quick checkout,
> branch switching, clone, push and so forth.
Sure. It will be much more network intensive as you are
replacing the part which normally has a very fast link through
the buffercache to pack files etc. A hybrid approach is also
possible, where objects are fetched individually via fast-import
and cached in a local .git repo. And I have a hunch that LZOP
compression of the stream may also be a win, but as with all of
these ideas, it would be after profiling identifies it as a choke point
than just because it sounds good.
> I'll look into the approaches you suggest. One consideration
> is coming up with a high-leverage approach - i.e. not doing
> heavy dev work if we can avoid it.
Right. You don't actually need to port the whole of git to Hadoop
initially, to begin with it can just pass through all commands to a
server-side git fast-import process. When you find specific operations
which are slow then these specific operations can be implemented using a
Hadoop back-end, and the rest backed to the standard git. If done using
a useful plug-in system, these systems could be accepted by the core
project as an enterprise scaling option.
This could let you get going with the knowledge that the scaling option
is there should it come out.
> On the other hand, it would be nice if we (including the entire
> community:) ) improve git in areas that others that share
> similar issues benefit from as well.
Like I say, a lot of people have run into this already...
HTH,
Sam
^ 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-06 21:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <7vwr7z653f.fsf@alter.siamese.dyndns.org>
Hi,
Junio C Hamano wrote:
> 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."
I agree with everything up to here. But I believe these people were
_already_ paying attention to "git status" output from the commandline
and in the editor window when they run "git commit", to notice other
changes they forgot to add, too. I don't think this series would
inconvenience them.
Hoping I can find time to look over the other changes soon.
Thanks,
Jonathan
^ 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-06 21:10 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jonathan Nieder
In-Reply-To: <1328525855-2547-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index 9c1d395..ec548ea 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -123,8 +123,16 @@ subdirectories.
> Record only the fact that the path will be added later. An entry
> for the path is placed in the index with no content. This is
> useful for, among other things, showing the unstaged content of
> - such files with `git diff` and committing them with `git commit
> - -a`.
> + such files with `git diff`.
> ++
> +Paths added with this option have intent-to-add flag in index. The
> +flag is removed once real content is added or updated. By default you
> +cannot commit the index as-is from until this flag is removed from all
> +entries (i.e. all entries have real content). See commit.ignoreIntentToAdd
> +regardless the flag.
> ++
> +Committing with `git commit -a` or with selected paths works
> +regardless the config key and the flag.
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).
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.
Without conclusion... Sigh...
^ permalink raw reply
* Re: Git performance results on a large repository
From: Greg Troxel @ 2012-02-06 21:07 UTC (permalink / raw)
To: Joshua Redstone; +Cc: git@vger.kernel.org
In-Reply-To: <CB55A6A4.40AFD%joshua.redstone@fb.com>
[-- Attachment #1: Type: text/plain, Size: 1742 bytes --]
Joshua Redstone <joshua.redstone@fb.com> writes:
> Greg, 'git commit' does some stat'ing of every file, even with all those
> flags - for example, I think one instance it does it is, just in case any
> pre-commit hooks touched any files, it re-stats everything.
That seems ripe for skipping. If I understand correctly, what's being
committed is the index, not the working dir contents, so it would follow
that a pre-commit hook changing a file is a bug.
> Regarding the perf numbers, I ran it on a beefy linux box. Have you
> tried doing your measurements with the drop_caches trick to make sure
> the file cache is totally cold?
On NetBSD, there should be a clear cache command for just this reason,
but I'm not sure there is. So I did
sysctl -w kern.maxvnodes=1000 # seemed to take a while
ls -lR # wait for those to be faulted in
sysctl -w kern.maxvnodes=500000
Then, git status on my repo churned the disk for a long time.
real 2m7.121s
user 0m3.086s
sys 0m7.577s
and then again right away
real 0m6.497s
user 0m2.533s
sys 0m3.010s
That repo has 217852 files (a real source tree with a few binaries, not
synthetic).
> Sorry for the dumb question, but how do I check the vnode cache size?
On BSD, sysctl kern.maxvnodes. I would aasume that on Linux there is
some max size for the the vnode cache, and that stat of a file in that
cache is faster than going to the filesystem (even if reading from
cached disk blocks). But I really don't know how that works in Linux.
I was going to say that if your vnode cache isn't big enough, then the
hot run won't be so much faster than the warm run, but that's not true,
because the fs blocks will be in the block cache and it will still help.
[-- Attachment #2: Type: application/pgp-signature, Size: 194 bytes --]
^ permalink raw reply
* Re: [PATCH v4 2/4] completion: simplify __git_remotes
From: Felipe Contreras @ 2012-02-06 21:04 UTC (permalink / raw)
To: SZEDER Gábor
Cc: git, Junio C Hamano, Jonathan Nieder, Thomas Rast, Todd Zullinger,
Shawn O. Pearce, Junio C Hamano
In-Reply-To: <20120206205315.GI16099@goldbirke>
2012/2/6 SZEDER Gábor <szeder@ira.uka.de>:
> On Thu, Feb 02, 2012 at 10:30:23PM +0200, Felipe Contreras wrote:
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index b435b6d..f86b734 100755
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -644,12 +644,7 @@ __git_refs_remotes ()
>> __git_remotes ()
>> {
>> local i ngoff IFS=$'\n' d="$(__gitdir)"
>
> You could also remove the ngoff variable, because with this patch it's
> not used anymore.
Right, I thought I did that... The change must have been lost in one
of the many revisions =/
--
Felipe Contreras
^ 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