* Re: [PATCH v4 05/12] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
From: Junio C Hamano @ 2013-02-12 19:58 UTC (permalink / raw)
To: Brandon Casey
Cc: Brandon Casey, git@vger.kernel.org, pclouds@gmail.com,
jrnieder@gmail.com
In-Reply-To: <511A9CDB.9060008@nvidia.com>
Brandon Casey <bcasey@nvidia.com> writes:
> On 2/12/2013 11:36 AM, Junio C Hamano wrote:
>> Brandon Casey <bcasey@nvidia.com> writes:
>>
>>>>> + return len > strlen(cherry_picked_prefix) + 1 &&
>>>>> + !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
>>>>> +}
>>>>
>>>> Does the first "is it longer than the prefix?" check matter? If it
>>>> is not, prefixcmp() would not match anyway, no?
>>>
>>> Probably not in practice, but technically we should only be accessing
>>> len characters in buf even though buf may be longer than len. So the
>>> check is just making sure the function doesn't access chars it's not
>>> supposed to.
>>
>> Sorry, I do not follow. Isn't caller's buf terminated with LF at buf[len],
>> which would never match cherry_picked_prefix even if len is shorter
>> than the prefix?
>
> Heh, I almost pointed that out in my reply. Yes, buf will be terminated
> with LF at buf[len]. And yes, that means that we will never get a false
> positive from prefixcmp even if the comparison overruns buf+len while
> doing its comparison. That's why the check doesn't matter in practice,
> i.e. based on the way that is_cherry_picked_from_line is being called
> right now and the content of cherry_picked_prefix.
>
> But, hasn't is_cherry_picked_from_line entered into a contract with the
> caller and said "I will not access more than len characters"?
>
> It's ok with me if you think it reads better without the check.
As Jonathan says, if you rewrite it to
return buf[len - 1] == ')' && !prefixcmp(buf, cherry_picked_prefix);
then the code can keep its promise without the length check, because
it knows there is no ')' in cherry-picked-prefix, and it also knows
prefixcmp() stops at the first difference.
It is not a huge deal; I was primarily reacting to the ugly multi-line
boolean expresion that is not inside a pair of parentheses (and because
this is a "return" statement, there is no good reason to have parentheses
except that this is a multi-line expression), which looked odd.
^ permalink raw reply
* [PATCH 0/2] Add bash.showUntrackedFiles config option
From: Martin Erik Werner @ 2013-02-12 20:12 UTC (permalink / raw)
To: git; +Cc: trsten, Martin Erik Werner
Hi,
Here is a patch adding a config option for showing untracked files in
the shell prompt, I've noticed having it enabled tends to make the
prompt act very sluggish in some cases (large repos / unfriendly
filesystems). So it would be nice to have a more fine-grained control
over it, similar to what exists for bash.showDirtyState.
Martin Erik Werner (2):
bash completion: add bash.showUntrackedFiles option
t9903: add test case for bash.showUntrackedFiles
contrib/completion/git-prompt.sh | 11 ++++++++---
t/t9903-bash-prompt.sh | 11 +++++++++++
2 files changed, 19 insertions(+), 3 deletions(-)
--
1.7.10.4
^ permalink raw reply
* [PATCH 1/2] bash completion: add bash.showUntrackedFiles option
From: Martin Erik Werner @ 2013-02-12 20:12 UTC (permalink / raw)
To: git; +Cc: trsten, Martin Erik Werner
In-Reply-To: <1360699936-28688-1-git-send-email-martinerikwerner@gmail.com>
Add a config option 'bash.showUntrackedFiles' which allows enabling
the prompt showing untracked files on a per-repository basis. This is
useful for some repositories where the 'git ls-files ...' command may
take a long time.
Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
contrib/completion/git-prompt.sh | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 9bef053..9b2eec2 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -43,7 +43,10 @@
#
# If you would like to see if there're untracked files, then you can set
# GIT_PS1_SHOWUNTRACKEDFILES to a nonempty value. If there're untracked
-# files, then a '%' will be shown next to the branch name.
+# files, then a '%' will be shown next to the branch name. You can
+# configure this per-repository with the bash.showUntrackedFiles
+# variable, which defaults to true once GIT_PS1_SHOWUNTRACKEDFILES is
+# enabled.
#
# If you would like to see the difference between HEAD and its upstream,
# set GIT_PS1_SHOWUPSTREAM="auto". A "<" indicates you are behind, ">"
@@ -332,8 +335,10 @@ __git_ps1 ()
fi
if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
- if [ -n "$(git ls-files --others --exclude-standard)" ]; then
- u="%"
+ if [ "$(git config --bool bash.showUntrackedFiles)" != "false" ]; then
+ if [ -n "$(git ls-files --others --exclude-standard)" ]; then
+ u="%"
+ fi
fi
fi
--
1.7.10.4
^ permalink raw reply related
* [PATCH 2/2] t9903: add test case for bash.showUntrackedFiles
From: Martin Erik Werner @ 2013-02-12 20:12 UTC (permalink / raw)
To: git; +Cc: trsten, Martin Erik Werner
In-Reply-To: <1360699936-28688-1-git-send-email-martinerikwerner@gmail.com>
Add a test case for the bash.showUntrackedFiles config option, which
checks that the config option can disable the global effect of the
GIT_PS1_SHOWUNTRACKEDFILES environmant variable.
Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
t/t9903-bash-prompt.sh | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index f17c1f8..c9417b9 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -447,6 +447,17 @@ test_expect_success 'prompt - untracked files status indicator - not shown insid
test_cmp expected "$actual"
'
+test_expect_success 'prompt - untracked files status indicator - disabled by config' '
+ printf " (master)" > expected &&
+ echo "untracked" > file_untracked &&
+ test_config bash.showUntrackedFiles false &&
+ (
+ GIT_PS1_SHOWUNTRACKEDFILES=y &&
+ __git_ps1 > "$actual"
+ ) &&
+ test_cmp expected "$actual"
+'
+
test_expect_success 'prompt - format string starting with dash' '
printf -- "-master" > expected &&
__git_ps1 "-%s" > "$actual" &&
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH v4 00/12] unify appending of sob
From: Jonathan Nieder @ 2013-02-12 20:16 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, gitster, pclouds
In-Reply-To: <1360664260-11803-1-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> Round 4.
Yay. I think this is cooked now and a good foundation for later
changes on top.
For what it's worth, with or without the two tweaks Junio suggested
(simplifying "(cherry picked from" detection, deferring introduction
of no_dup_sob variable until it is used),
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH/FYI v4 13/12] fixup! t/t3511: add some tests of 'cherry-pick -s' functionality
From: Junio C Hamano @ 2013-02-12 20:20 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Brandon Casey, git, pclouds
In-Reply-To: <20130212195620.GB12240@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Brandon Casey wrote:
>
>> I'm not sure we should apply this though. I'm leaning towards saying that
>> the 'cherry-pick -s' behavior with respect to a commit with an empty message
>> body should be undefined. If we want it to be undefined then we probably
>> shouldn't introduce a test which would have the effect of defining it.
>
> Maybe it would make sense to just check that cherry-pick doesn't
> segfault in this case?
;-)
>
> That is, compute the output but don't compare it to expected output, as
> in:
>
> test_expect_success 'adding signoff to empty message does something sane' '
> git reset --hard HEAD^ &&
> git cherry-pick --allow-empty-message -s empty-branch &&
> git show --pretty=format:%B -s empty-branch >actual &&
>
> # sign-off is included *somewhere*
> grep "^Signed-off-by:.*>\$" actual
> '
Isn't what the current code happens to do is the best we could do?
We would end up showing one entry whose title appears to be
"Signed-off-by: ..." in the shortlog output if we did so. If we
added an empty line, then the shortlog output will have a single
empty line that is equally unsightly.
We could force a message like this:
tree d7f87518a26e9f00714675706f165b94f3625177
parent f459a4b602c0f4d371e1717572de6d0c4d39c6b1
author Junio C Hamano <gitster@pobox.com> 1360699963 -0800
committer Junio C Hamano <gitster@pobox.com> 1360699980 -0800
!!cherry-picked from a commit without any message!!
Signed-off-by: Junio C Hamano <gitster@pobox.com>
but I do not think that buys us much; it only replaces a totally
uninformative empty line with another totally uninformative junk.
That ugliness is a price the insane person, who is cherry picking a
commit without any justification made by another insane person,
indicates that he is willing to pay by doing so. At that point I do
not think we should care.
^ permalink raw reply
* [PATCH 3/3] Documentation/Makefile: fix inherited {html,info,man}dir
From: John Keeping @ 2013-02-12 20:17 UTC (permalink / raw)
To: git; +Cc: Steffen Prohaska, Jakub Narebski, John Keeping
In-Reply-To: <cover.1360700102.git.john@keeping.me.uk>
Commit e14421b (Allow INSTALL, bindir, mandir to be set in main Makefile
- 2006-06-29) changed Documentation/Makefile to inherit the value of
mandir from the top-level Makefile when invoked as "make install-doc" at
the top-level. This was inherited by infodir and htmldir when they were
added.
This was broken by commit 026fa0d (Move computation of absolute paths
from Makefile to runtime (in preparation for RUNTIME_PREFIX) -
2009-01-18) which changed these variables to have relative paths in the
top-level Makefile, causing the documentation to be installed into the
path without $(prefix) prepended.
Fix this by changing the defaults to be paths relative to $(prefix) and
introducing new variables {html,info,man}_instdir which contain the full
installation paths.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
I'm not sure if this is the best approach - the alternative would be to
change the top-level Makefile to use {html,info,man}dir_relative and
derive the {html,info,man}dir variables from that.
The top-level Makefile is inconsistent in the approach it takes - bindir
is derived from bindir_relative but gitexecdir and template_dir have
gitexec_instdir and template_instdir derived from them.
Documentation/Makefile | 56 +++++++++++++++++++++++++++++++++++---------------
1 file changed, 40 insertions(+), 16 deletions(-)
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 0cfdc36..34cd9f2 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -78,15 +78,21 @@ DOC_MAN1 = $(patsubst %.txt,%.1,$(MAN1_TXT))
DOC_MAN5 = $(patsubst %.txt,%.5,$(MAN5_TXT))
DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
+# The following variables can be relative paths due to the way they can be
+# inherited from the top-level Makefile:
+# htmldir
+# infodir
+# mandir
+# Note that pdfdir is an exception to this since it is not used by git-help.
prefix ?= $(HOME)
bindir ?= $(prefix)/bin
-htmldir ?= $(prefix)/share/doc/git-doc
-infodir ?= $(prefix)/share/info
pdfdir ?= $(prefix)/share/doc/git-doc
-mandir ?= $(prefix)/share/man
-man1dir = $(mandir)/man1
-man5dir = $(mandir)/man5
-man7dir = $(mandir)/man7
+htmldir ?= share/doc/git-doc
+infodir ?= share/info
+mandir ?= share/man
+man1dir = $(man_instdir)/man1
+man5dir = $(man_instdir)/man5
+man7dir = $(man_instdir)/man7
# DESTDIR =
ASCIIDOC = asciidoc
@@ -110,6 +116,24 @@ endif
-include ../config.mak.autogen
-include ../config.mak
+ifneq ($(filter /%,$(firstword $(htmldir))),)
+html_instdir = $(htmldir)
+else
+html_instdir = $(prefix)/$(htmldir)
+endif
+
+ifneq ($(filter /%,$(firstword $(infodir))),)
+info_instdir = $(infodir)
+else
+info_instdir = $(prefix)/$(infodir)
+endif
+
+ifneq ($(filter /%,$(firstword $(mandir))),)
+man_instdir = $(mandir)
+else
+man_instdir = $(prefix)/$(mandir)
+endif
+
#
# For docbook-xsl ...
# -1.68.1, no extra settings are needed?
@@ -144,7 +168,7 @@ endif
# Distros may want to use MAN_BASE_URL=file:///path/to/git/docs/
# or similar.
ifndef MAN_BASE_URL
-MAN_BASE_URL = file://$(htmldir)/
+MAN_BASE_URL = file://$(html_instdir)/
endif
XMLTO_EXTRA += -m manpage-base-url.xsl
@@ -220,13 +244,13 @@ install-man: man
$(INSTALL) -m 644 $(DOC_MAN7) $(DESTDIR)$(man7dir)
install-info: info
- $(INSTALL) -d -m 755 $(DESTDIR)$(infodir)
- $(INSTALL) -m 644 git.info gitman.info $(DESTDIR)$(infodir)
- if test -r $(DESTDIR)$(infodir)/dir; then \
- $(INSTALL_INFO) --info-dir=$(DESTDIR)$(infodir) git.info ;\
- $(INSTALL_INFO) --info-dir=$(DESTDIR)$(infodir) gitman.info ;\
+ $(INSTALL) -d -m 755 $(DESTDIR)$(info_instdir)
+ $(INSTALL) -m 644 git.info gitman.info $(DESTDIR)$(info_instdir)
+ if test -r $(DESTDIR)$(info_instdir)/dir; then \
+ $(INSTALL_INFO) --info-dir=$(DESTDIR)$(info_instdir) git.info ;\
+ $(INSTALL_INFO) --info-dir=$(DESTDIR)$(info_instdir) gitman.info ;\
else \
- echo "No directory found in $(DESTDIR)$(infodir)" >&2 ; \
+ echo "No directory found in $(DESTDIR)$(info_instdir)" >&2 ; \
fi
install-pdf: pdf
@@ -234,7 +258,7 @@ install-pdf: pdf
$(INSTALL) -m 644 user-manual.pdf $(DESTDIR)$(pdfdir)
install-html: html
- '$(SHELL_PATH_SQ)' ./install-webdoc.sh $(DESTDIR)$(htmldir)
+ '$(SHELL_PATH_SQ)' ./install-webdoc.sh $(DESTDIR)$(html_instdir)
../GIT-VERSION-FILE: FORCE
$(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE
@@ -402,14 +426,14 @@ require-manrepo::
then echo "git-manpages repository must exist at $(MAN_REPO)"; exit 1; fi
quick-install-man: require-manrepo
- '$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(MAN_REPO) $(DESTDIR)$(mandir)
+ '$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(MAN_REPO) $(DESTDIR)$(man_instdir)
require-htmlrepo::
@if test ! -d $(HTML_REPO); \
then echo "git-htmldocs repository must exist at $(HTML_REPO)"; exit 1; fi
quick-install-html: require-htmlrepo
- '$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(HTML_REPO) $(DESTDIR)$(htmldir)
+ '$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(HTML_REPO) $(DESTDIR)$(html_instdir)
print-man1:
@for i in $(MAN1_TXT); do echo $$i; done
--
1.8.1.2
^ permalink raw reply related
* [PATCH 0/3] Fix installation paths with "make install-doc"
From: John Keeping @ 2013-02-12 20:17 UTC (permalink / raw)
To: git; +Cc: Steffen Prohaska, Jakub Narebski, John Keeping
When using the top-level install-doc target the html, info and man
target directories are inherited from the top-level Makefile by the
documentation Makefile as relative paths, which is not expected and
results in the files being installed in an unexpected location.
The first two patches are simple style fixes. The third one fixes the
issue described above.
John Keeping (3):
Documentation/Makefile: fix spaces around assignments
Documentation/Makefile: move infodir to be with other '*dir's
Documentation/Makefile: fix inherited {html,info,man}dir
Documentation/Makefile | 88 ++++++++++++++++++++++++++++++++------------------
1 file changed, 56 insertions(+), 32 deletions(-)
--
1.8.1.2
^ permalink raw reply
* [PATCH 1/3] Documentation/Makefile: fix spaces around assignments
From: John Keeping @ 2013-02-12 20:17 UTC (permalink / raw)
To: git; +Cc: Steffen Prohaska, Jakub Narebski, John Keeping
In-Reply-To: <cover.1360700102.git.john@keeping.me.uk>
A simple style fix; no functional change.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
Documentation/Makefile | 42 +++++++++++++++++++++---------------------
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 62dbd9a..af3d8a4 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -31,11 +31,11 @@ MAN7_TXT += gittutorial.txt
MAN7_TXT += gitworkflows.txt
MAN_TXT = $(MAN1_TXT) $(MAN5_TXT) $(MAN7_TXT)
-MAN_XML=$(patsubst %.txt,%.xml,$(MAN_TXT))
-MAN_HTML=$(patsubst %.txt,%.html,$(MAN_TXT))
+MAN_XML = $(patsubst %.txt,%.xml,$(MAN_TXT))
+MAN_HTML = $(patsubst %.txt,%.html,$(MAN_TXT))
OBSOLETE_HTML = git-remote-helpers.html
-DOC_HTML=$(MAN_HTML) $(OBSOLETE_HTML)
+DOC_HTML = $(MAN_HTML) $(OBSOLETE_HTML)
ARTICLES = howto-index
ARTICLES += everyday
@@ -74,35 +74,35 @@ SP_ARTICLES += technical/api-index
DOC_HTML += $(patsubst %,%.html,$(ARTICLES) $(SP_ARTICLES))
-DOC_MAN1=$(patsubst %.txt,%.1,$(MAN1_TXT))
-DOC_MAN5=$(patsubst %.txt,%.5,$(MAN5_TXT))
-DOC_MAN7=$(patsubst %.txt,%.7,$(MAN7_TXT))
+DOC_MAN1 = $(patsubst %.txt,%.1,$(MAN1_TXT))
+DOC_MAN5 = $(patsubst %.txt,%.5,$(MAN5_TXT))
+DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
-prefix?=$(HOME)
-bindir?=$(prefix)/bin
-htmldir?=$(prefix)/share/doc/git-doc
-pdfdir?=$(prefix)/share/doc/git-doc
-mandir?=$(prefix)/share/man
-man1dir=$(mandir)/man1
-man5dir=$(mandir)/man5
-man7dir=$(mandir)/man7
-# DESTDIR=
+prefix ?= $(HOME)
+bindir ?= $(prefix)/bin
+htmldir ?= $(prefix)/share/doc/git-doc
+pdfdir ?= $(prefix)/share/doc/git-doc
+mandir ?= $(prefix)/share/man
+man1dir = $(mandir)/man1
+man5dir = $(mandir)/man5
+man7dir = $(mandir)/man7
+# DESTDIR =
ASCIIDOC = asciidoc
ASCIIDOC_EXTRA =
MANPAGE_XSL = manpage-normal.xsl
XMLTO = xmlto
XMLTO_EXTRA =
-INSTALL?=install
+INSTALL ?= install
RM ?= rm -f
MAN_REPO = ../../git-manpages
HTML_REPO = ../../git-htmldocs
-infodir?=$(prefix)/share/info
-MAKEINFO=makeinfo
-INSTALL_INFO=install-info
-DOCBOOK2X_TEXI=docbook2x-texi
-DBLATEX=dblatex
+infodir ?= $(prefix)/share/info
+MAKEINFO = makeinfo
+INSTALL_INFO = install-info
+DOCBOOK2X_TEXI = docbook2x-texi
+DBLATEX = dblatex
ifndef PERL_PATH
PERL_PATH = /usr/bin/perl
endif
--
1.8.1.2
^ permalink raw reply related
* [PATCH 2/3] Documentation/Makefile: move infodir to be with other '*dir's
From: John Keeping @ 2013-02-12 20:17 UTC (permalink / raw)
To: git; +Cc: Steffen Prohaska, Jakub Narebski, John Keeping
In-Reply-To: <cover.1360700102.git.john@keeping.me.uk>
Signed-off-by: John Keeping <john@keeping.me.uk>
---
Documentation/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/Makefile b/Documentation/Makefile
index af3d8a4..0cfdc36 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -81,6 +81,7 @@ DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
prefix ?= $(HOME)
bindir ?= $(prefix)/bin
htmldir ?= $(prefix)/share/doc/git-doc
+infodir ?= $(prefix)/share/info
pdfdir ?= $(prefix)/share/doc/git-doc
mandir ?= $(prefix)/share/man
man1dir = $(mandir)/man1
@@ -98,7 +99,6 @@ RM ?= rm -f
MAN_REPO = ../../git-manpages
HTML_REPO = ../../git-htmldocs
-infodir ?= $(prefix)/share/info
MAKEINFO = makeinfo
INSTALL_INFO = install-info
DOCBOOK2X_TEXI = docbook2x-texi
--
1.8.1.2
^ permalink raw reply related
* Re: Pushing a git repository to a new server
From: Jeff King @ 2013-02-12 20:42 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Ethan Reesor, Konstantin Khomoutov, git
In-Reply-To: <511A2775.9050209@drmicha.warpmail.net>
On Tue, Feb 12, 2013 at 12:28:53PM +0100, Michael J Gruber wrote:
> I'm not sure providers like GitHub would fancy an interface which allows
> the programmatic creation of repos (giving a new meaning to "fork
> bomb"). But I bet you know better ;-)
You can already do that:
http://developer.github.com/v3/repos/#create
We rate-limit API requests, and I imagine we might do something similar
with create-over-git. But that is exactly the kind of implementation
detail that can go into a custom create-repo script.
> An alternative would be to teach git (the client) about repo types and
> how to create them. After all, a repo URL "ssh://host/path" gives a
> clear indication that "ssh host git init path" will create a repo.
But that's the point of a microformat. It _doesn't_ always work, because
the server may not allow arbitrary commands, or may have special
requirements on top of the "init". You can make the microformat be "git
init path", and servers can intercept calls to "git init" and translate
them into custom magic. But I think the world is a little simpler if we
define a new service type (alongside git-upload-pack, git-receive-pack,
etc), and let clients request it. Then it's clear what the client is
trying to do, it's easy for servers to hook into it, we can request it
over http, etc. And it can be extended over time to take more fields
(like repo description, etc).
I'm really not suggesting anything drastic. The wrapper case for ssh
would be as simple as a 3-line shell script which calls "git init" under
the hood, but it provides one level of indirection that makes
replacing/hooking it much simpler for servers. So the parts that are in
stock git would not be much work (most of the work would be on _calling_
it, but that is the same for adding a call to "git init").
I think the main reason the idea hasn't gone anywhere is that nobody
really cares _that_ much. People just don't create repositories that
often. I feel like this is one of those topics that comes up once a
year, and then nothing happens on it, because people just make their
repo manually and then stop caring about it.
Just my two cents, of course. :)
-Peff
^ permalink raw reply
* Re: [PATCH v4 00/12] unify appending of sob
From: Junio C Hamano @ 2013-02-12 20:45 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Brandon Casey, git, pclouds
In-Reply-To: <20130212201613.GC12240@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Brandon Casey wrote:
>
>> Round 4.
>
> Yay. I think this is cooked now and a good foundation for later
> changes on top.
>
> For what it's worth, with or without the two tweaks Junio suggested
> (simplifying "(cherry picked from" detection, deferring introduction
> of no_dup_sob variable until it is used),
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Yeah, I am inclined to merge this to 'next' without any tweak, and
let it cook and get polished incrementally. I am not sure if we
have enough time to graduate it to 'master' for the upcoming
release, though.
Thanks.
^ permalink raw reply
* Re: [PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close
From: Jeff King @ 2013-02-12 20:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michal Nazarewicz, git
In-Reply-To: <7va9r9gy5y.fsf@alter.siamese.dyndns.org>
On Tue, Feb 12, 2013 at 10:55:05AM -0800, Junio C Hamano wrote:
> Michal Nazarewicz <mpn@google.com> writes:
>
> > From: Michal Nazarewicz <mina86@mina86.com>
> >
> > The body of the loop in command_close_bidi_pipe function is identical to
> > what _cmd_close function does so instead of duplicating, refactor change
> > _cmd_close so that it accepts list of file handlers to be closed, which
>
> s/file handlers/file handles/, I think.
And s/refactor change/refactor/.
Other than that, I think the series looks OK. I have one style micro-nit
on patch 4 which I'll reply in-line. But it is either "fix while
applying" or "ignore", I don't think it will be worth a re-roll.
-Peff
^ permalink raw reply
* Re: inotify to minimize stat() calls
From: Karsten Blees @ 2013-02-12 20:48 UTC (permalink / raw)
To: Duy Nguyen
Cc: kusmabite, Ramkumar Ramachandra, Robert Zeh, Junio C Hamano,
Git List, finnag
In-Reply-To: <CACsJy8AWyJ=dW5f44huWyPPe4X62xyi+R9CNM5Tg6u6TYf+thQ@mail.gmail.com>
Am 11.02.2013 04:53, schrieb Duy Nguyen:
> On Sun, Feb 10, 2013 at 11:58 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> Karsten Blees has done something similar-ish on Windows, and he posted
>> the results here:
>>
>> https://groups.google.com/forum/#!topic/msysgit/fL_jykUmUNE/discussion
>>
The new hashtable implementation in fscache [1] supports O(1) removal and has no mingw dependencies - might come in handy for anyone trying to implement an inotify daemon.
[1] https://github.com/kblees/git/commit/f7eb85c2
>> I also seem to remember he doing a ReadDirectoryChangesW version, but
>> I don't remember what happened with that.
>
> Thanks. I came across that but did not remember. For one thing, we
> know the inotify alternative for Windows: ReadDirectoryChangesW.
>
I dropped ReadDirectoryChangesW because maintaining a 'live' file system cache became more and more complicated. For example, according to MSDN docs, ReadDirectoryChangesW *may* report short DOS 8.3 names (i.e. "PROGRA~1" instead of "Program Files"), so a correct and fast cache implementation would have to be indexed by long *and* short names...
Another problem was that the 'live' cache had quite negative performance impact on mutating git commands (checkout, reset...). An inotify daemon running as a background process (not in-process as fscache) will probably affect everyone that modifies the working copy, e.g. running 'make' or the test-suite. This should be considered in the design.
> I copy "git status"'s (impressive) numbers from fscache-v0 for those
> who are interested in:
>
> preload | -u | normal | cached | gain
> --------+-----+--------+--------+------
> false | all | 25.144 | 3.055 | 8.2
> false | no | 22.822 | 1.748 | 12.8
> true | all | 9.234 | 2.179 | 4.2
> true | no | 6.833 | 0.955 | 7.2
>
Note that I wasn't able to reproduce such bad 'normal' values in later tests, I guess disk fragmentation and/or virus scanner must have tricked me on that day...gain factors of 2.5 - 5 are more appropriate.
However, the difference between git status -uall and -uno was always about 1.3 s in all fscache versions, even though opendir/readdir/closedir was served entirely from the cache. I added a bit of performance tracing to find the cause, and I think most of the time spent in wt_status_collect_untracked can be eliminated:
1.) 0.939 s is spent in dir.c/excluded (i.e. checking .gitignore). This check is done for *every* file in the working copy, including files in the index. Checking the index first could eliminate most of that, i.e.:
(Note: patches are for discussion only, I'm aware that they may have unintended side effects...)
@@ -1097,6 +1097,8 @@ static enum path_treatment treat_path(struct dir_struct *dir,
return path_ignored;
strbuf_setlen(path, baselen);
strbuf_addstr(path, de->d_name);
+ if (cache_name_exists(path->buf, path->len, ignore_case))
+ return path_ignored;
if (simplify_away(path->buf, path->len, simplify))
return path_ignored;
---
2.) 0.135 s is spent in name-hash.c/hash_index_entry_directories, reindexing the same directories over and over again. In the end, the hashtable contains 939k directory entries, even though the WebKit test repo only has 7k directories. Checking if a directory entry already exists could reduce that, i.e.:
@@ -53,14 +55,23 @@ static void hash_index_entry_directories(struct index_state *istate, struct cach
unsigned int hash;
void **pos;
double t = ticks();
+ struct cache_entry *ce2;
+ int len = ce_namelen(ce);
- const char *ptr = ce->name;
- while (*ptr) {
- while (*ptr && *ptr != '/')
- ++ptr;
- if (*ptr == '/') {
- ++ptr;
- hash = hash_name(ce->name, ptr - ce->name);
+ while (len > 0) {
+ while (len > 0 && ce->name[len - 1] != '/')
+ len--;
+ if (len > 0) {
+ hash = hash_name(ce->name, len);
+ ce2 = lookup_hash(hash, &istate->name_hash);
+ while (ce2) {
+ if (same_name(ce2, ce->name, len, ignore_case)) {
+ add_since(t, &hash_dirs);
+ return;
+ }
+ ce2 = ce2->dir_next;
+ }
+ len--;
pos = insert_hash(hash, ce, &istate->name_hash);
if (pos) {
ce->dir_next = *pos;
---
Tests were done with the WebKit repo (~200k files, ~7k directories, 15 .gitignore files, ~100 entries in root .gitignore). Instrumented code can be found here: https://github.com/kblees/git/tree/kb/git-status-performance-tracing
Here's the performance traces of 'git status -s -uall'
Before patches:
trace: at builtin/commit.c:1221, time: 0.523429 s: cmd_status/read_cache_preload
trace: at builtin/commit.c:1223, time: 0.00403477 s: cmd_status/refresh_index
trace: at builtin/commit.c:1231, time: 0.00318494 s: cmd_status/hold_locked_index
trace: at wt-status.c:539, time: 0.00527396 s: wt_status_collect_changes_worktree
trace: at wt-status.c:544, time: 0.00545771 s: wt_status_collect_changes
trace: at wt-status.c:546, time: 1.286 s: wt_status_collect_untracked
trace: at builtin/commit.c:1233, time: 1.29852 s: cmd_status/wt_status_collect
trace: at dir.c:1540, time: 0.00170986 s: read_directory_recursive/strbuf_add
trace: at dir.c:1541, time: 0.00623972 s: read_directory_recursive/opendir
trace: at dir.c:1542, time: 0.00517881 s: read_directory_recursive/readdir
trace: at dir.c:1543, time: 0.992936 s: read_directory_recursive/treat_path
trace: at dir.c:1544, time: 0.277942 s: read_directory_recursive/dir_add_name
trace: at dir.c:1545, time: 0.0014594 s: read_directory_recursive/close
trace: at dir.c:1546, time: 0.939349 s: treat_one_path/excluded
trace: at dir.c:1547, time: 0.0050811 s: treat_one_path/dir_add_ignored
trace: at dir.c:1548, time: 0.00515875 s: treat_one_path/get_dtype
trace: at dir.c:1549, time: 0.00329322 s: treat_one_path/treat_directory
trace: at dir.c:1550, time: 0.222969 s: excluded/prep_exclude
trace: at dir.c:1551, time: 0.00443398 s: excluded/excluded_from_list[EXC_CMDL]
trace: at dir.c:1552, time: 0.699602 s: excluded/excluded_from_list[EXC_DIRS]
trace: at dir.c:1553, time: 0.00475736 s: excluded/excluded_from_list[EXC_FILE]
trace: at read-cache.c:460, time: 0.00967987 s: index_name_pos
trace: at name-hash.c:213, time: 0.190481 s: lazy_init_name_hash
trace: at name-hash.c:216, time: 0.135248 s: hash_index_entry_directories (938865 entries)
trace: at name-hash.c:217, time: 0.0806647 s: index_name_exists
trace: at compat/mingw.c:2137, time: 1.97424 s: command: c:\git\msysgit\git\git-status.exe -s -uall
After patches:
trace: at builtin/commit.c:1221, time: 0.517511 s: cmd_status/read_cache_preload
trace: at builtin/commit.c:1223, time: 0.00405227 s: cmd_status/refresh_index
trace: at builtin/commit.c:1231, time: 0.00322796 s: cmd_status/hold_locked_index
trace: at wt-status.c:539, time: 0.00530057 s: wt_status_collect_changes_worktree
trace: at wt-status.c:544, time: 0.00546062 s: wt_status_collect_changes
trace: at wt-status.c:546, time: 0.322799 s: wt_status_collect_untracked
trace: at builtin/commit.c:1233, time: 0.33536 s: cmd_status/wt_status_collect
trace: at dir.c:1542, time: 0.00120529 s: read_directory_recursive/strbuf_add
trace: at dir.c:1543, time: 0.00476647 s: read_directory_recursive/opendir
trace: at dir.c:1544, time: 0.00502022 s: read_directory_recursive/readdir
trace: at dir.c:1545, time: 0.310515 s: read_directory_recursive/treat_path
trace: at dir.c:1546, time: 0 s: read_directory_recursive/dir_add_name
trace: at dir.c:1547, time: 0.000831234 s: read_directory_recursive/close
trace: at dir.c:1548, time: 0.0668582 s: treat_one_path/excluded
trace: at dir.c:1549, time: 0.000173174 s: treat_one_path/dir_add_ignored
trace: at dir.c:1550, time: 0.000174267 s: treat_one_path/get_dtype
trace: at dir.c:1551, time: 0.00315468 s: treat_one_path/treat_directory
trace: at dir.c:1552, time: 0.039733 s: excluded/prep_exclude
trace: at dir.c:1553, time: 0.000185205 s: excluded/excluded_from_list[EXC_CMDL]
trace: at dir.c:1554, time: 0.0264496 s: excluded/excluded_from_list[EXC_DIRS]
trace: at dir.c:1555, time: 0.000170622 s: excluded/excluded_from_list[EXC_FILE]
trace: at read-cache.c:460, time: 0.00260636 s: index_name_pos
trace: at name-hash.c:224, time: 0.126637 s: lazy_init_name_hash
trace: at name-hash.c:227, time: 0.0500866 s: hash_index_entry_directories (7152 entries)
trace: at name-hash.c:228, time: 0.0790143 s: index_name_exists
trace: at compat/mingw.c:2137, time: 1.00595 s: command: c:\git\msysgit\git\git-status.exe -s -uall
^ permalink raw reply
* Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe
From: Jeff King @ 2013-02-12 20:51 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: gitster, git
In-Reply-To: <3bb6b7736eb4b0a958469be13d8c646faec1208a.1360677646.git.mina86@mina86.com>
On Tue, Feb 12, 2013 at 03:02:31PM +0100, Michal Nazarewicz wrote:
> sub command_close_bidi_pipe {
> local $?;
> my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);
> - _cmd_close($ctx, $in, $out);
> + _cmd_close($ctx, grep defined, $in, $out);
Maybe it is just me, but I find the "grep EXPR" form a little subtle
inside an argument list. Either:
_cmd_close($ctx, grep { defined } $in, $out);
or
_cmd_close($ctx, grep(defined, $in, $out));
is a little more obvious to me.
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] Documentation/Makefile: fix spaces around assignments
From: Jonathan Nieder @ 2013-02-12 21:00 UTC (permalink / raw)
To: John Keeping; +Cc: git, Steffen Prohaska, Jakub Narebski
In-Reply-To: <48645e424b932536e8cd877962a29b9cfa8bdebd.1360700102.git.john@keeping.me.uk>
Hi,
John Keeping wrote:
> [Subject: [PATCH 1/3] Documentation/Makefile: fix spaces around assignments]
It's not so much "fix spaces" as "use consistent spacing", no?
Aside from that nit, looks like a sensible no-op to me, so
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
^ permalink raw reply
* Re: [PATCH 2/3] Documentation/Makefile: move infodir to be with other '*dir's
From: Jonathan Nieder @ 2013-02-12 21:01 UTC (permalink / raw)
To: John Keeping; +Cc: git, Steffen Prohaska, Jakub Narebski
In-Reply-To: <dcc4f597f26531b79bd9f097c73f6f186b73c81d.1360700102.git.john@keeping.me.uk>
John Keeping wrote:
> Signed-off-by: John Keeping <john@keeping.me.uk>
[...]
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -81,6 +81,7 @@ DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
> prefix ?= $(HOME)
> bindir ?= $(prefix)/bin
> htmldir ?= $(prefix)/share/doc/git-doc
> +infodir ?= $(prefix)/share/info
> pdfdir ?= $(prefix)/share/doc/git-doc
> mandir ?= $(prefix)/share/man
> man1dir = $(mandir)/man1
> @@ -98,7 +99,6 @@ RM ?= rm -f
> MAN_REPO = ../../git-manpages
> HTML_REPO = ../../git-htmldocs
>
> -infodir ?= $(prefix)/share/info
> MAKEINFO = makeinfo
Is this another stylefix or is there a functional reason for this
change?
^ permalink raw reply
* Re: [PATCH] completion: support 'git config --local'
From: Jeff King @ 2013-02-12 21:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matthieu Moy, git, Dasa Paddock
In-Reply-To: <7vzjz9h1w0.fsf@alter.siamese.dyndns.org>
On Tue, Feb 12, 2013 at 09:34:39AM -0800, Junio C Hamano wrote:
> I see the second hunk is new. Comments?
> [...]
> > @@ -1676,7 +1676,7 @@ _git_config ()
> > case "$cur" in
> > --*)
> > __gitcomp "
> > - --global --system --file=
> > + --system --global --local --file=
> > --list --replace-all
> > --get --get-all --get-regexp
> > --add --unset --unset-all
It makes sense to me. It just means that "--local" itself gets completed
(while the other hunk is about using the presence of "--local" impacting
other completion). It's an orthogonal issue, but I don't mind them in
the same patch.
> How would this interract with the writing side of "git config"?
> "git config --local foo.bar value" and "git config foo.bar value"
> are the same, no?
>
> Is it "yes they are the same but it does not hurt?"
It doesn't affect writing at all. The change is in
__git_config_get_set_variables, which is used only here:
--get|--get-all|--unset|--unset-all)
__gitcomp_nl "$(__git_config_get_set_variables)"
So it is purely about completing existing variables, and it's right to
limit itself to a particular file if we know that is what has been
given.
I'm not sure I understand the original poster's point about "git config
-l --local". "-l" does not take a limiter, does it?
-Peff
^ permalink raw reply
* Re: [PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close
From: Michal Nazarewicz @ 2013-02-12 21:12 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
In-Reply-To: <20130212204807.GB25330@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]
>> Michal Nazarewicz <mpn@google.com> writes:
>> > The body of the loop in command_close_bidi_pipe function is identical to
>> > what _cmd_close function does so instead of duplicating, refactor change
>> > _cmd_close so that it accepts list of file handlers to be closed, which
> On Tue, Feb 12, 2013 at 10:55:05AM -0800, Junio C Hamano wrote:
>> s/file handlers/file handles/, I think.
On Tue, Feb 12 2013, Jeff King wrote:
> And s/refactor change/refactor/.
>
> Other than that, I think the series looks OK. I have one style micro-nit
> on patch 4 which I'll reply in-line. But it is either "fix while
> applying" or "ignore", I don't think it will be worth a re-roll.
All fixed.
Junio, do you want me to resend or would you be fine with just pulling:
git://github.com/mina86/git.git master
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--
[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]
[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply
* Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe
From: Michal Nazarewicz @ 2013-02-12 21:13 UTC (permalink / raw)
To: Jeff King; +Cc: gitster, git
In-Reply-To: <20130212205141.GC25330@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 960 bytes --]
On Tue, Feb 12 2013, Jeff King wrote:
> On Tue, Feb 12, 2013 at 03:02:31PM +0100, Michal Nazarewicz wrote:
>
>> sub command_close_bidi_pipe {
>> local $?;
>> my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);
>> - _cmd_close($ctx, $in, $out);
>> + _cmd_close($ctx, grep defined, $in, $out);
>
> Maybe it is just me, but I find the "grep EXPR" form a little subtle
> inside an argument list. Either:
>
> _cmd_close($ctx, grep { defined } $in, $out);
>
> or
>
> _cmd_close($ctx, grep(defined, $in, $out));
>
> is a little more obvious to me.
I personally avoid parens whenever possible in Perl, but Git.pm seem to
favour them so I went with the second option.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--
[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]
[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply
* Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe
From: Junio C Hamano @ 2013-02-12 21:14 UTC (permalink / raw)
To: Jeff King; +Cc: Michal Nazarewicz, git
In-Reply-To: <20130212205141.GC25330@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Feb 12, 2013 at 03:02:31PM +0100, Michal Nazarewicz wrote:
>
>> sub command_close_bidi_pipe {
>> local $?;
>> my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);
>> - _cmd_close($ctx, $in, $out);
>> + _cmd_close($ctx, grep defined, $in, $out);
>
> Maybe it is just me, but I find the "grep EXPR" form a little subtle
> inside an argument list. Either:
>
> _cmd_close($ctx, grep { defined } $in, $out);
>
> or
>
> _cmd_close($ctx, grep(defined, $in, $out));
>
> is a little more obvious to me.
I would actually vote for the most explicit:
_cmd_close($ctx, (grep { defined } ($in, $out)));
^ permalink raw reply
* Re: [PATCHv4 3/6] Git.pm: refactor command_close_bidi_pipe to use _cmd_close
From: Junio C Hamano @ 2013-02-12 21:17 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: Jeff King, git
In-Reply-To: <xa1tk3qd9qza.fsf@mina86.com>
Michal Nazarewicz <mina86@mina86.com> writes:
>>> Michal Nazarewicz <mpn@google.com> writes:
>>> > The body of the loop in command_close_bidi_pipe function is identical to
>>> > what _cmd_close function does so instead of duplicating, refactor change
>>> > _cmd_close so that it accepts list of file handlers to be closed, which
>
>> On Tue, Feb 12, 2013 at 10:55:05AM -0800, Junio C Hamano wrote:
>>> s/file handlers/file handles/, I think.
>
> On Tue, Feb 12 2013, Jeff King wrote:
>> And s/refactor change/refactor/.
>>
>> Other than that, I think the series looks OK. I have one style micro-nit
>> on patch 4 which I'll reply in-line. But it is either "fix while
>> applying" or "ignore", I don't think it will be worth a re-roll.
>
> All fixed.
>
> Junio, do you want me to resend or would you be fine with just pulling:
>
> git://github.com/mina86/git.git master
Neither. I agree with Peff that these micronits are not enough
reason for the trouble of rerolling the series, so I'll just amend
them at my end. Please double-check what you see on the 'pu' branch
when I push today's integration result out later.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/3] Documentation/Makefile: move infodir to be with other '*dir's
From: John Keeping @ 2013-02-12 21:18 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Steffen Prohaska, Jakub Narebski
In-Reply-To: <20130212210138.GE12240@google.com>
On Tue, Feb 12, 2013 at 01:01:38PM -0800, Jonathan Nieder wrote:
> John Keeping wrote:
>
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> [...]
> > --- a/Documentation/Makefile
> > +++ b/Documentation/Makefile
> > @@ -81,6 +81,7 @@ DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
> > prefix ?= $(HOME)
> > bindir ?= $(prefix)/bin
> > htmldir ?= $(prefix)/share/doc/git-doc
> > +infodir ?= $(prefix)/share/info
> > pdfdir ?= $(prefix)/share/doc/git-doc
> > mandir ?= $(prefix)/share/man
> > man1dir = $(mandir)/man1
> > @@ -98,7 +99,6 @@ RM ?= rm -f
> > MAN_REPO = ../../git-manpages
> > HTML_REPO = ../../git-htmldocs
> >
> > -infodir ?= $(prefix)/share/info
> > MAKEINFO = makeinfo
>
> Is this another stylefix or is there a functional reason for this
> change?
Another stylefix - this arrangement seems more logical to me and makes
the comment in the next patch simpler.
^ permalink raw reply
* Re: [PATCHv4 4/6] Git.pm: allow pipes to be closed prior to calling command_close_bidi_pipe
From: Jeff King @ 2013-02-12 21:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michal Nazarewicz, git
In-Reply-To: <7va9r9fd4e.fsf@alter.siamese.dyndns.org>
On Tue, Feb 12, 2013 at 01:14:57PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Tue, Feb 12, 2013 at 03:02:31PM +0100, Michal Nazarewicz wrote:
> >
> >> sub command_close_bidi_pipe {
> >> local $?;
> >> my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);
> >> - _cmd_close($ctx, $in, $out);
> >> + _cmd_close($ctx, grep defined, $in, $out);
> >
> > Maybe it is just me, but I find the "grep EXPR" form a little subtle
> > inside an argument list. Either:
> >
> > _cmd_close($ctx, grep { defined } $in, $out);
> >
> > or
> >
> > _cmd_close($ctx, grep(defined, $in, $out));
> >
> > is a little more obvious to me.
>
> I would actually vote for the most explicit:
>
> _cmd_close($ctx, (grep { defined } ($in, $out)));
Gross. My perl spider-sense tingles at seeing that many optional
punctuation characters, but it should at least be obvious to a casual or
new perl programmer what is going on. I'm fine with it.
-Peff
^ permalink raw reply
* Re: [PATCH] completion: support 'git config --local'
From: Junio C Hamano @ 2013-02-12 22:13 UTC (permalink / raw)
To: Jeff King; +Cc: Matthieu Moy, git, Dasa Paddock
In-Reply-To: <20130212211140.GA29358@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I'm not sure I understand the original poster's point about "git config
> -l --local". "-l" does not take a limiter, does it?
"git config -l core.\*" will just die without limiting the output to
everything under core. hierarchy, so you are right---the combination
does not make any sense. You have to say
git config -l | grep ^core\\.
or something like that.
Completing "git config -l --lo<TAB>" still may help, though.
^ 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