From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:03
Jeff King [off-list ref] writes:
On Thu, Oct 18, 2012 at 07:31:35AM +0200, Johannes Sixt wrote:
quoted
Right. But we should really be doing something like this instead to save a
few subprocesses.
[...]
- eval "$(set_ident AUTHOR <../commit)" ||
+ eval "$(set_ident AUTHOR author <../commit)" ||
I cringe a little at losing DRY-ness to avoid processes.
Well, the header field token "author" and the middle word of the
variable GIT_AUTHOR_NAME _happen_ to be the same modulo case, but
they did not have to be, so you could argue the updated set_ident
implementation is more generally useful (you could even argue that
we should spell the first parameter out as "GIT_AUTHOR_NAME" and
"GIT_AUTHOR_EMAIL", two separate parameters).
Speaking of repetition, this seems like almost the exact same parsing
that happens in git-sh-setup's get_author_ident_from_commit. Maybe it's
worth merging them. I suspect you could also avoid another process
by parsing out both author and committer information in the same sed
invocation.
I cringe a little at losing DRY-ness to avoid processes.
Well, the header field token "author" and the middle word of the
variable GIT_AUTHOR_NAME _happen_ to be the same modulo case, but
they did not have to be, so you could argue the updated set_ident
implementation is more generally useful (you could even argue that
we should spell the first parameter out as "GIT_AUTHOR_NAME" and
"GIT_AUTHOR_EMAIL", two separate parameters).
True, though that is even more work for the caller (and *_DATE, too). We
could make it "GIT_AUTHOR", but I don't think there is much point in
being that level of half-way general. The caller can always pick it out
of the variables if they really want to do something tricky.
quoted
Speaking of repetition, this seems like almost the exact same parsing
that happens in git-sh-setup's get_author_ident_from_commit. Maybe it's
worth merging them. I suspect you could also avoid another process
by parsing out both author and committer information in the same sed
invocation.
Yes, yes and yes.
Working on it now. git-sh-setup works, but chasing an annoying bug in
filter-branch. I'm sure it's something silly and stupid.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:55:03
On Thu, Oct 18, 2012 at 02:08:47AM -0400, Jeff King wrote:
Working on it now. git-sh-setup works, but chasing an annoying bug in
filter-branch. I'm sure it's something silly and stupid.
Ugh. I was being caught by crazy dash-versus-bash stuff.
Try this:
$ bash -c 'echo "\\\\"'
\\
$ dash -c 'echo "\\\\"'
\
It's the "echo will automatically do backslash escaping" magic we have
so often enjoyed. I solved it with printf.
Patches to follow.
My primary motivation was cleanup, but it also has a net reduction of 5
fork+execs for each commit that filter-branch processes. This dropped
the run-time of "git filter-branch HEAD -1000" on my linux box from 62s
to 47s. A real filter-branch would do more work in the filters, of
course, but that translates to 7.5 minutes of time saved if you are
filtering all 30,000 commits of git.git.
[1/2]: git-sh-setup: refactor ident-parsing functions
[2/2]: filter-branch: use git-sh-setup's ident parsing functions
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:55:03
The only ident-parsing function we currently provide is
get_author_ident_from_commit. This is not very
flexible for two reasons:
1. It takes a commit as an argument, and can't read from
commit headers saved on disk.
2. It will only parse authors, not committers.
This patch provides a more flexible interface which will
parse multiple idents from a commit provide on stdin. We can
easily use it as a building block for the current function
to retain compatibility.
Signed-off-by: Jeff King <redacted>
---
Since we are counting processes in this series, I should note that this
actually adds a subshell invocation for each call, since it went from:
script='...'
sed $script
to:
sed "$(make_script)"
For filter-branch, which is really the only high-performance caller we
have, this is negated by the fact that it will do author and committer
at the same time, saving us an extra subshell (in addition to an extra
sed invocation).
git-sh-setup.sh | 62 +++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 43 insertions(+), 19 deletions(-)
@@ -191,28 +191,52 @@ get_author_ident_from_commit () {fi}+# Generate a sed script to parse identities from a commit.+#+# Reads the commit from stdin, which should be in raw format (e.g., from+# cat-file or "--pretty=raw").+#+# The first argument specifies the ident line to parse (e.g., "author"), and+# the second specifies the environment variable to put it in (e.g., "AUTHOR"+# for "GIT_AUTHOR_*"). Multiple pairs can be given to parse author and+# committer.+pick_ident_script(){+whiletest$#-gt0+do+lid=$1;shift+uid=$1;shift+printf'%s'"+/^$lid/{+s/'/'\\\\''/g+h+s/^$lid"'\([^<]*\) <[^>]*> .*$/\1/'"+s/.*/GIT_${uid}_NAME='&'/p++g+s/^$lid"'[^<]* <\([^>]*\)> .*$/\1/'"+s/.*/GIT_${uid}_EMAIL='&'/p++g+s/^$lid"'[^<]* <[^>]*> \(.*\)$/@\1/'"+s/.*/GIT_${uid}_DATE='&'/p+}+"+done+echo'/^$/q'+}++# Create a pick-script as above and feed it to sed. Stdout is suitable for+# feeding to eval.+parse_ident_from_commit(){+LANG=CLC_ALL=Csed-ne"$(pick_ident_script"$@")"+}++# Parse the author from a commit given as an argument. Stdout is suitable for+# feeding to eval to set the usual GIT_* ident variables. get_author_ident_from_commit(){-pick_author_script='-/^author/{-s/'\''/'\''\\'\'\''/g-h-s/^author\([^<]*\)<[^>]*>.*$/\1/-s/.*/GIT_AUTHOR_NAME='\''&'\''/p--g-s/^author[^<]*<\([^>]*\)>.*$/\1/-s/.*/GIT_AUTHOR_EMAIL='\''&'\''/p--g-s/^author[^<]*<[^>]*>\(.*\)$/@\1/-s/.*/GIT_AUTHOR_DATE='\''&'\''/p--q-}-'encoding=$(gitconfigi18n.commitencoding||echoUTF-8)gitshow-s--pretty=raw--encoding="$encoding""$1"--|-LANG=CLC_ALL=Csed-ne"$pick_author_script"+parse_ident_from_commitauthorAUTHOR}# Clear repo-local GIT_* environment variables. Useful when switching to
From: Jeff King <hidden> Date: 2016-06-15 22:55:03
This saves us some code, but it also reduces the number of
processes we start for each filtered commit. Since we can
parse both author and committer in the same sed invocation,
we save one process. And since the new interface avoids tr,
we save 4 processes.
It also avoids using "tr", which has had some odd
portability problems reported with from Solaris's xpg6
version.
Signed-off-by: Jeff King <redacted>
---
git-filter-branch.sh | 42 +++++++++---------------------------------
1 file changed, 9 insertions(+), 33 deletions(-)
@@ -64,37 +64,15 @@ set_ident () {eval"$functions"-# When piped a commit, output a script to set the ident of either-# "author" or "committer+# Ensure non-empty id name.+fallback_name(){+echo"case \"\$GIT_$1_NAME\" in \"\") GIT_$1_NAME=\"\${GIT_$1_EMAIL%%@*}\" && export GIT_$1_NAME;; esac"+} set_ident(){-lid="$(echo"$1"|tr"[A-Z]""[a-z]")"-uid="$(echo"$1"|tr"[a-z]""[A-Z]")"-pick_id_script='-/^'$lid'/{-s/'\''/'\''\\'\'\''/g-h-s/^'$lid'\([^<]*\)<[^>]*>.*$/\1/-s/'\''/'\''\'\'\''/g-s/.*/GIT_'$uid'_NAME='\''&'\''; export GIT_'$uid'_NAME/p--g-s/^'$lid'[^<]*<\([^>]*\)>.*$/\1/-s/'\''/'\''\'\'\''/g-s/.*/GIT_'$uid'_EMAIL='\''&'\''; export GIT_'$uid'_EMAIL/p--g-s/^'$lid'[^<]*<[^>]*>\(.*\)$/@\1/-s/'\''/'\''\'\'\''/g-s/.*/GIT_'$uid'_DATE='\''&'\''; export GIT_'$uid'_DATE/p--q-}-'--LANG=CLC_ALL=Csed-ne"$pick_id_script"-# Ensure non-empty id name.-echo"case \"\$GIT_${uid}_NAME\" in \"\") GIT_${uid}_NAME=\"\${GIT_${uid}_EMAIL%%@*}\" && export GIT_${uid}_NAME;; esac"+parse_ident_from_commitauthorAUTHORcommitterCOMMITTER+fallback_nameAUTHOR+fallback_nameCOMMITTER}USAGE="[--env-filter <command>] [--tree-filter <command>]
@@ -320,10 +298,8 @@ while read commit parents; dogitcat-filecommit"$commit">../commit||die"Cannot read commit $commit"-eval"$(set_identAUTHOR<../commit)"||-die"setting author failed for commit $commit"-eval"$(set_identCOMMITTER<../commit)"||-die"setting committer failed for commit $commit"+eval"$(set_ident<../commit)"||+die"setting author/committer failed for commit $commit"eval"$filter_env"</dev/null||die"env filter failed: $filter_env"
Didn't you lose the export GIT_$uid_{NAME,EMAIL,DATE} parts somewhere on
the way?
Yikes, you're right. I didn't even notice, as the test suite still
passes. I can see how the env filter would still be able to see the
variables, but the commit-tree call wouldn't. I guess it happens to work
because we do not test alternate idents in our filter branch tests (IOW,
we are silently rewriting each commit during the filter-branch, but it
happens to have the same identities).
I'll investigate.
-Peff
Didn't you lose the export GIT_$uid_{NAME,EMAIL,DATE} parts somewhere on
the way?
Yikes, you're right. I didn't even notice, as the test suite still
passes. I can see how the env filter would still be able to see the
variables, but the commit-tree call wouldn't. I guess it happens to work
because we do not test alternate idents in our filter branch tests (IOW,
we are silently rewriting each commit during the filter-branch, but it
happens to have the same identities).
Hrm. We _do_ test this in t7003. Weirder, if I instrument filter-branch
like this:
@@ -174,6 +174,8 @@ test_expect_success 'author information is preserved' 'test1=$(gitrev-list--author="B V Uips"preserved-author|wc-l)'+test_done+ test_expect_success"remove a certain author's commits"'echoi>i&&test_tick&&
and run t7003, it shows that the variable is properly exported to the
sub-process! But I can't seem to figure out why. Confused...
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:55:04
On Thu, Oct 18, 2012 at 06:22:17AM -0400, Jeff King wrote:
Hrm. We _do_ test this in t7003. Weirder, if I instrument filter-branch
like this:
[...]
and run t7003, it shows that the variable is properly exported to the
sub-process! But I can't seem to figure out why. Confused...
Oh, I see. The variables are already exported by test-lib.sh. You can
see the breakage with:
@@ -167,10 +167,11 @@ test_expect_success 'author information is preserved' 'test_tick&&GIT_AUTHOR_NAME="B V Uips"gitcommit-mbvuips&&gitbranchpreserved-author&&-gitfilter-branch-f--msg-filter"cat; \+(sane_unsetGIT_AUTHOR_NAME&&+gitfilter-branch-f--msg-filter"cat; \test\$GIT_COMMIT!=$(gitrev-parsemaster)||\echoHallo" \-preserved-author&&+preserved-author)&&test1=$(gitrev-list--author="B V Uips"preserved-author|wc-l)'-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:55:04
This saves us some code, but it also reduces the number of
processes we start for each filtered commit. Since we can
parse both author and committer in the same sed invocation,
we save one process. And since the new interface avoids tr,
we save 4 processes.
It also avoids using "tr", which has had some odd
portability problems reported with from Solaris's xpg6
version.
We also tweak one of the tests in t7003 to double-check that
we are properly exporting the variables (because test-lib.sh
exports GIT_AUTHOR_NAME, it will be automatically exported
in subprograms. We override this to make sure that
filter-branch handles it properly itself).
Signed-off-by: Jeff King <redacted>
---
This fixes the missing exports from v1. There's no changes needed to
patch 1.
git-filter-branch.sh | 46 +++++++++++++---------------------------------
t/t7003-filter-branch.sh | 5 +++--
2 files changed, 16 insertions(+), 35 deletions(-)
@@ -64,37 +64,19 @@ set_ident () {eval"$functions"-# When piped a commit, output a script to set the ident of either-# "author" or "committer+finish_ident(){+# Ensure non-empty id name.+echo"case \"\$GIT_$1_NAME\" in \"\") GIT_$1_NAME=\"\${GIT_$1_EMAIL%%@*}\" && export GIT_$1_NAME;; esac"+# And make sure everything is exported.+echo"export GIT_$1_NAME"+echo"export GIT_$1_EMAIL"+echo"export GIT_$1_DATE"+} set_ident(){-lid="$(echo"$1"|tr"[A-Z]""[a-z]")"-uid="$(echo"$1"|tr"[a-z]""[A-Z]")"-pick_id_script='-/^'$lid'/{-s/'\''/'\''\\'\'\''/g-h-s/^'$lid'\([^<]*\)<[^>]*>.*$/\1/-s/'\''/'\''\'\'\''/g-s/.*/GIT_'$uid'_NAME='\''&'\''; export GIT_'$uid'_NAME/p--g-s/^'$lid'[^<]*<\([^>]*\)>.*$/\1/-s/'\''/'\''\'\'\''/g-s/.*/GIT_'$uid'_EMAIL='\''&'\''; export GIT_'$uid'_EMAIL/p--g-s/^'$lid'[^<]*<[^>]*>\(.*\)$/@\1/-s/'\''/'\''\'\'\''/g-s/.*/GIT_'$uid'_DATE='\''&'\''; export GIT_'$uid'_DATE/p--q-}-'--LANG=CLC_ALL=Csed-ne"$pick_id_script"-# Ensure non-empty id name.-echo"case \"\$GIT_${uid}_NAME\" in \"\") GIT_${uid}_NAME=\"\${GIT_${uid}_EMAIL%%@*}\" && export GIT_${uid}_NAME;; esac"+parse_ident_from_commitauthorAUTHORcommitterCOMMITTER+finish_identAUTHOR+finish_identCOMMITTER}USAGE="[--env-filter <command>] [--tree-filter <command>]
@@ -320,10 +302,8 @@ while read commit parents; dogitcat-filecommit"$commit">../commit||die"Cannot read commit $commit"-eval"$(set_identAUTHOR<../commit)"||-die"setting author failed for commit $commit"-eval"$(set_identCOMMITTER<../commit)"||-die"setting committer failed for commit $commit"+eval"$(set_ident<../commit)"||+die"setting author/committer failed for commit $commit"eval"$filter_env"</dev/null||die"env filter failed: $filter_env"
@@ -167,10 +167,11 @@ test_expect_success 'author information is preserved' 'test_tick&&GIT_AUTHOR_NAME="B V Uips"gitcommit-mbvuips&&gitbranchpreserved-author&&-gitfilter-branch-f--msg-filter"cat; \+(sane_unsetGIT_AUTHOR_NAME&&+gitfilter-branch-f--msg-filter"cat; \test\$GIT_COMMIT!=$(gitrev-parsemaster)||\echoHallo" \-preserved-author&&+preserved-author)&&test1=$(gitrev-list--author="B V Uips"preserved-author|wc-l)'
From: Junio C Hamano <hidden> Date: 2016-06-15 22:55:15
Jeff King [off-list ref] writes:
The only ident-parsing function we currently provide is
get_author_ident_from_commit. This is not very
flexible for two reasons:
1. It takes a commit as an argument, and can't read from
commit headers saved on disk.
2. It will only parse authors, not committers.
This patch provides a more flexible interface which will
parse multiple idents from a commit provide on stdin. We can
easily use it as a building block for the current function
to retain compatibility.
Signed-off-by: Jeff King <redacted>
---
Since we are counting processes in this series, I should note that this
actually adds a subshell invocation for each call, since it went from:
script='...'
sed $script
to:
sed "$(make_script)"
For filter-branch, which is really the only high-performance caller we
have, this is negated by the fact that it will do author and committer
at the same time, saving us an extra subshell (in addition to an extra
sed invocation).
Given that pick-ident-script is a const function, a caller that
repeatedly call is could call it once and use it in a variable, no?
@@ -191,28 +191,52 @@ get_author_ident_from_commit () {fi}+# Generate a sed script to parse identities from a commit.+#+# Reads the commit from stdin, which should be in raw format (e.g., from+# cat-file or "--pretty=raw").+#+# The first argument specifies the ident line to parse (e.g., "author"), and+# the second specifies the environment variable to put it in (e.g., "AUTHOR"+# for "GIT_AUTHOR_*"). Multiple pairs can be given to parse author and+# committer.+pick_ident_script(){+whiletest$#-gt0+do+lid=$1;shift+uid=$1;shift+printf'%s'"+/^$lid/{+s/'/'\\\\''/g+h+s/^$lid"'\([^<]*\) <[^>]*> .*$/\1/'"+s/.*/GIT_${uid}_NAME='&'/p++g+s/^$lid"'[^<]* <\([^>]*\)> .*$/\1/'"+s/.*/GIT_${uid}_EMAIL='&'/p++g+s/^$lid"'[^<]* <[^>]*> \(.*\)$/@\1/'"+s/.*/GIT_${uid}_DATE='&'/p+}+"+done+echo'/^$/q'+}++# Create a pick-script as above and feed it to sed. Stdout is suitable for+# feeding to eval.+parse_ident_from_commit(){+LANG=CLC_ALL=Csed-ne"$(pick_ident_script"$@")"+}++# Parse the author from a commit given as an argument. Stdout is suitable for+# feeding to eval to set the usual GIT_* ident variables. get_author_ident_from_commit(){-pick_author_script='-/^author/{-s/'\''/'\''\\'\'\''/g-h-s/^author\([^<]*\)<[^>]*>.*$/\1/-s/.*/GIT_AUTHOR_NAME='\''&'\''/p--g-s/^author[^<]*<\([^>]*\)>.*$/\1/-s/.*/GIT_AUTHOR_EMAIL='\''&'\''/p--g-s/^author[^<]*<[^>]*>\(.*\)$/@\1/-s/.*/GIT_AUTHOR_DATE='\''&'\''/p--q-}-'encoding=$(gitconfigi18n.commitencoding||echoUTF-8)gitshow-s--pretty=raw--encoding="$encoding""$1"--|-LANG=CLC_ALL=Csed-ne"$pick_author_script"+parse_ident_from_commitauthorAUTHOR}# Clear repo-local GIT_* environment variables. Useful when switching to
From: Jeff King <hidden> Date: 2016-06-15 22:55:15
On Mon, Nov 12, 2012 at 09:44:01AM -0800, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
The only ident-parsing function we currently provide is
get_author_ident_from_commit. This is not very
flexible for two reasons:
1. It takes a commit as an argument, and can't read from
commit headers saved on disk.
2. It will only parse authors, not committers.
This patch provides a more flexible interface which will
parse multiple idents from a commit provide on stdin. We can
easily use it as a building block for the current function
to retain compatibility.
Signed-off-by: Jeff King <redacted>
---
Since we are counting processes in this series, I should note that this
actually adds a subshell invocation for each call, since it went from:
script='...'
sed $script
to:
sed "$(make_script)"
For filter-branch, which is really the only high-performance caller we
have, this is negated by the fact that it will do author and committer
at the same time, saving us an extra subshell (in addition to an extra
sed invocation).
Given that pick-ident-script is a const function, a caller that
repeatedly call is could call it once and use it in a variable, no?
The problem is that it is a helper called from parse_ident_from_commit.
And that function just passes along its arguments, so it does not know
that it is being called repeatedly with the same arguments. So you'd
have to either change the interface or memoize internally.
I don't think memoization is a good option for two reasons:
1. Storing the arguments to compare to later is complex. You don't
want to just store "$*" from the last run and see if we got the
same arguments. You'd have to quote your delimiter (e.g., you would
not want to confuse ("foo", "bar") with ("foo bar"). Though in this
instance, we know that our args do not have spaces, so we could get
away with that.
2. If you are in a subshell or even a while loop, your memoized
variable will not be retained.
So unless somebody has some clever scheme for memoizing shell functions
without any process overhead, it is probably not worth it.
Changing the interface for get_author_ident_from_commit would be a pain,
but if we just wanted to help filter-branch, we could do something like
this:
@@ -225,10 +225,17 @@ pick_ident_script () {echo'/^$/q'}+# Feed a pick_ident_script return value to sed. Use this instead of+# parse_ident_from_commit below if you are going to be parsing commits in a+# tight loop and want to save a process.+parse_ident_from_commit_via_script(){+LANG=CLC_ALL=Csed-ne"$1"+}+# Create a pick-script as above and feed it to sed. Stdout is suitable for# feeding to eval. parse_ident_from_commit(){-LANG=CLC_ALL=Csed-ne"$(pick_ident_script"$@")"+parse_ident_from_commit_via_script"$(pick_ident_script"$@")"}# Parse the author from a commit given as an argument. Stdout is suitable for