[PATCH v3 0/4] completion: couple of cleanups

[PATCH v3 0/4] completion: couple of cleanups

[Felipe Contreras]

And an improvement for zsh.

Junio: I see you already picked most of them for 'pu', but I've made further changes based on the feedback:

 * completion: be nicer with zsh
	Improved the code-style

  * completion: simplify __gitcomp*
	Fix
	Improved commit message

Cheers.

Felipe Contreras (4):
  completion: be nicer with zsh
  completion: simplify __git_remotes
  completion: remove unused code
  completion: simplify __gitcomp*

 contrib/completion/git-completion.bash |   66 +++++---------------------------
 1 files changed, 10 insertions(+), 56 deletions(-)

-- 
1.7.9

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Jonathan Nieder]

Felipe Contreras wrote:

> And yet another bug in zsh[1] causes a mismatch; zsh seems to have
> problem emulating wordspliting, but only on the command substitution.

Patches didn't hit the list again.  Any idea why?

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Felipe Contreras]

On Thu, Feb 2, 2012 at 10:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> And yet another bug in zsh[1] causes a mismatch; zsh seems to have
>> problem emulating wordspliting, but only on the command substitution.
>
> Patches didn't hit the list again.  Any idea why?

No. A bug in list software?

I didn't get any warning or error.

-- 
Felipe Contreras

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Jonathan Nieder]

Hi,

Felipe Contreras wrote:

> Felipe Contreras (4):
>   completion: be nicer with zsh

Since I can't find this patch in the mail archive, I'll reply here.
Luckily the most important bit is above already.

I think I mentioned before that this subject line is what will appear
in the shortlog and the shortlog is all that some people will see of
the changelog, so it should include a self-contained description of
the impact of the patch.

However, clearly I did not say it clearly enough. :)  I guess it's
better to take a cue from storytellers and show rather than tell.
(Please don't take this as a precedent --- I will not always be doing
the style fixes myself, and sometimes will consider a patch to scratch
someone else's itch not worth the trouble and work on something else.)

-- >8 --
From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Thu, 2 Feb 2012 03:15:17 +0200
Subject: completion: avoid default value assignment on : true command

zsh versions from 4.3.0 to present (4.3.15) do not correctly propagate
the SH_WORD_SPLIT option into the subshell in ${foo:=$(bar)}
expressions.  For example, after running

	emulate sh
	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.  This fixes a bug tht caused all commands to be
treated as porcelain and show up in "git <TAB><TAB>" completion,
because the list of all commands was treated as a single word in
__git_list_porcelain_commands and did not match any of the patterns
that would usually cause plumbing to be excluded.

[jn: clarified commit message, indentation style fix]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 contrib/completion/git-completion.bash |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 78be1958..d7965daf 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -676,7 +676,8 @@ __git_merge_strategies=
 # is needed.
 __git_compute_merge_strategies ()
 {
-	: ${__git_merge_strategies:=$(__git_list_merge_strategies)}
+	[ -n "$__git_merge_strategies" ] ||
+	__git_merge_strategies=$(__git_list_merge_strategies)
 }
 
 __git_complete_revlist_file ()
@@ -854,7 +855,8 @@ __git_list_all_commands ()
 __git_all_commands=
 __git_compute_all_commands ()
 {
-	: ${__git_all_commands:=$(__git_list_all_commands)}
+	[ -n "$__git_all_commands" ] ||
+	__git_all_commands=$(__git_list_all_commands)
 }
 
 __git_list_porcelain_commands ()
@@ -947,7 +949,8 @@ __git_porcelain_commands=
 __git_compute_porcelain_commands ()
 {
 	__git_compute_all_commands
-	: ${__git_porcelain_commands:=$(__git_list_porcelain_commands)}
+	[ -n "$__git_porcelain_commands" ] ||
+	__git_porcelain_commands=$(__git_list_porcelain_commands)
 }
 
 __git_pretty_aliases ()
-- 
1.7.9

Re: [PATCH v3 2/4] completion: simplify __git_remotes

[Jonathan Nieder]

Felipe Contreras wrote:

>   completion: simplify __git_remotes
>   completion: remove unused code

In the same spirit of stealing potential shortlog lines from Junio:

-- >8 --
From: Felipe Contreras <felipe.contreras@gmail.com>
Subject: completion: use ls -1 instead of rolling a loop to do that ourselves

This simplifies the code a great deal.  In particular, it allows us to
get rid of __git_shopt, which is used only in this fuction to enable
'nullglob' in zsh.

[jn: squashed with a patch that actually gets rid of __git_shopt]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
The diffstat says it all.

 contrib/completion/git-completion.bash |   37 +-------------------------------
 1 files changed, 1 insertions(+), 36 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 78be1958..4612dde9 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)"
-	__git_shopt -q nullglob || ngoff=1
-	__git_shopt -s nullglob
-	for i in "$d/remotes"/*; do
-		echo ${i#$d/remotes/}
-	done
-	[ "$ngoff" ] && __git_shopt -u nullglob
+	test -d "$d/remotes" && ls -1 "$d/remotes"
 	for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do
 		i="${i#remote.}"
 		echo "${i/.url*/}"
@@ -2733,33 +2728,3 @@ if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
 complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \
 	|| complete -o default -o nospace -F _git git.exe
 fi
-
-if [[ -n ${ZSH_VERSION-} ]]; then
-	__git_shopt () {
-		local option
-		if [ $# -ne 2 ]; then
-			echo "USAGE: $0 (-q|-s|-u) <option>" >&2
-			return 1
-		fi
-		case "$2" in
-		nullglob)
-			option="$2"
-			;;
-		*)
-			echo "$0: invalid option: $2" >&2
-			return 1
-		esac
-		case "$1" in
-		-q)	setopt | grep -q "$option" ;;
-		-u)	unsetopt "$option" ;;
-		-s)	setopt "$option" ;;
-		*)
-			echo "$0: invalid flag: $1" >&2
-			return 1
-		esac
-	}
-else
-	__git_shopt () {
-		shopt "$@"
-	}
-fi
-- 
1.7.9

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Jonathan Nieder]

(dropping Shawn from cc list, since I don't think he's touched the
 completion code for years)
Felipe Contreras wrote:
> On Thu, Feb 2, 2012 at 10:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> Patches didn't hit the list again.  Any idea why?
>
> No. A bug in list software?
>
> I didn't get any warning or error.

Except in response to HTML attachments, I've never seen vger return any
explanation when it decides a message is spam.  It just discards them.

See [1] for details.  If there's no obvious explanation, I'd suggest
contacting the postmaster.

Hope that helps, and sorry for the fuss,
Jonathan

[1] http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Felipe Contreras]

On Thu, Feb 2, 2012 at 11:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> (dropping Shawn from cc list, since I don't think he's touched the
>  completion code for years)
> Felipe Contreras wrote:
>> On Thu, Feb 2, 2012 at 10:16 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> Patches didn't hit the list again.  Any idea why?
>>
>> No. A bug in list software?
>>
>> I didn't get any warning or error.
>
> Except in response to HTML attachments, I've never seen vger return any
> explanation when it decides a message is spam.  It just discards them.

Which is wrong, because when it discards them wrongly nobody knows why.

> See [1] for details.  If there's no obvious explanation, I'd suggest
> contacting the postmaster.

But there's nothing like the taboo words in the mail:
http://vger.kernel.org/majordomo-taboos.txt

-- 
Felipe Contreras

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Jonathan Nieder]

Felipe Contreras wrote:
> On Thu, Feb 2, 2012 at 11:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> See [1] for details.  If there's no obvious explanation, I'd suggest
>> contacting the postmaster.
>
> But there's nothing like the taboo words in the mail:
> http://vger.kernel.org/majordomo-taboos.txt

Why are you telling me?  I am not the postmaster.  I can't do anything
to investigate it or fix it.

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Felipe Contreras]

On Thu, Feb 2, 2012 at 10:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> Felipe Contreras (4):
>>   completion: be nicer with zsh
>
> Since I can't find this patch in the mail archive, I'll reply here.
> Luckily the most important bit is above already.
>
> I think I mentioned before that this subject line is what will appear
> in the shortlog and the shortlog is all that some people will see of
> the changelog, so it should include a self-contained description of
> the impact of the patch.

Exactly, and "completion: avoid default value assignment on : true
command" tells *nothing* to most people. Why is this patch needed? Do
I care about it?

OTOH "completion: be nicer with zsh" explains the purpose of the patch
and people that don't care about zsh can happily ignore it if they
want, and the ones that care about zsh might want to back port it, or
whatever.

Also, if you are looking at a shortlog and looking for patches related
to zsh, you would easily see this.

"completion: avoid default value assignment on : true command" almost
ensures that the people reading that summary would need to read
further to see *why*.

> -- >8 --
> From: Felipe Contreras <felipe.contreras@gmail.com>
> Date: Thu, 2 Feb 2012 03:15:17 +0200
> Subject: completion: avoid default value assignment on : true command

No useful information provided yet. Should I care about this? I guess
I need to open the mail and see.

> zsh versions from 4.3.0 to present (4.3.15) do not correctly propagate
> the SH_WORD_SPLIT option into the subshell in ${foo:=$(bar)}
> expressions.  For example, after running

Ok, a bug in zsh, I can skip this. Or if I care about zsh, then I
still have to read further, because I still don't know what's the
problem.

And BTW, this is extremely detailed. Where's the evidence? Is there a
bug report? How can I follow this to find out when it's fixed. (hint:
missing link)

>        emulate sh
>        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.

Ok, still reading.

> So avoid the construct, and use an explicit 'test -n "$foo" ||
> foo=$(bar)' instead.

Yeah...

> This fixes a bug tht caused all commands to be
> treated as porcelain and show up in "git <TAB><TAB>" completion,
> because the list of all commands was treated as a single word in
> __git_list_porcelain_commands and did not match any of the patterns
> that would usually cause plumbing to be excluded.

Aha! Finally we arrive to the important part.

I guess we have different styles of writing. Personally, I don't see
the point of forcing the readers to go through the whole thing.
Compare this to mine:

| Subject: [PATCH v3 1/4] completion: be nicer with zsh

At this point most people can skip this.

| And yet another bug in zsh[1] causes a mismatch; zsh seems to have
| problem emulating wordspliting, but only on the command substitution.
|
| Let's avoid it.

A bug in zsh in certain situations... got it.

| I found this issue because __git_compute_porcelain_commands listed all
| commands (not only porcelain).

At this point it might have been better to mention "git <TAB><TAB>"
instead, as you did. But not a big deal, the problem is clear; all
commands are listed.

Almost all people would stop at this point; either they don't care
about zsh, or they care, and they want the patch.

| This is because in zsh the following code:
|
|  for i in $__git_all_commands
|
| would evaluate $__git_all_commands as a single word (with spaces),
| ${=__git_all_commands} should be used to do word splitting expansion
| (unless SH_WORD_SPLIT is used).
|
| sh emulation should take care of that, but the command expantion is
| messing up with that.

And more details for the skeptics, or the ones that want to learn more.

| tl;dr: $__git_porcelain_commands = $__git_all_commands

Wrapping it up, to make clear what happens.

| [1] http://article.gmane.org/gmane.comp.shells.zsh.devel/24296

And then a link to further details, discussion, and possible fixes.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Felipe Contreras]

On Thu, Feb 2, 2012 at 11:46 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Thu, Feb 2, 2012 at 11:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> See [1] for details.  If there's no obvious explanation, I'd suggest
>>> contacting the postmaster.
>>
>> But there's nothing like the taboo words in the mail:
>> http://vger.kernel.org/majordomo-taboos.txt
>
> Why are you telling me?  I am not the postmaster.  I can't do anything
> to investigate it or fix it.

I'm not telling you, this is a conversation on a public mailing list.
Other people might know something about, or they might hit the same
issue in the future. It's good to state things on record.

And FTR, I had already contacted the postmaster, which has been not
exactly helpful in previous occasions, but lets see.

-- 
Felipe Contreras

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Thomas Rast]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Thu, Feb 2, 2012 at 10:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Exactly, and "completion: avoid default value assignment on : true
> command" tells *nothing* to most people. Why is this patch needed? Do
> I care about it?
>
> OTOH "completion: be nicer with zsh" explains the purpose of the patch
> and people that don't care about zsh can happily ignore it if they
> want, and the ones that care about zsh might want to back port it, or
> whatever.

Perhaps you could compromise on

  completion: work around zsh word splitting bug in : ${foo:=$(bar)}

?


> | tl;dr: $__git_porcelain_commands = $__git_all_commands
>
> Wrapping it up, to make clear what happens.

I think this is not good style for a commit message.  Apart from the
very trendy use of tl;dr, it doesn't even properly summarize the cause
*or* the user-visible symptom.  It just states how the confusion
propagates somewhere in the middle of the code.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Jonathan Nieder]

Thomas Rast wrote:

> Perhaps you could compromise on
>
>   completion: work around zsh word splitting bug in : ${foo:=$(bar)}

Thanks, that looks like a good subject line to me.  It gives a hint of
what the patch is trying to do, and it does not try to fool me into
thinking that if I use bash the patch does not affect me.

Felipe, I'm not going to respond to the rest of your message.  Perhaps
someone more patient than I am will, or if someone has specific
questions for me, I'll be glad to help.

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Jonathan Nieder]

Jonathan Nieder wrote:
> Thomas Rast wrote:

>> Perhaps you could compromise on
>>
>>   completion: work around zsh word splitting bug in : ${foo:=$(bar)}
>
> Thanks, that looks like a good subject line to me.

Ah, except it's not a word splitting bug.  It's an option propagation
bug.

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Felipe Contreras]

On Thu, Feb 2, 2012 at 12:35 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, Feb 2, 2012 at 10:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>
>> Exactly, and "completion: avoid default value assignment on : true
>> command" tells *nothing* to most people. Why is this patch needed? Do
>> I care about it?
>>
>> OTOH "completion: be nicer with zsh" explains the purpose of the patch
>> and people that don't care about zsh can happily ignore it if they
>> want, and the ones that care about zsh might want to back port it, or
>> whatever.
>
> Perhaps you could compromise on
>
>  completion: work around zsh word splitting bug in : ${foo:=$(bar)}

Yes, that sounds better, perhaps:

completion: work around zsh option propagation bug

>> | tl;dr: $__git_porcelain_commands = $__git_all_commands
>>
>> Wrapping it up, to make clear what happens.
>
> I think this is not good style for a commit message.  Apart from the
> very trendy use of tl;dr, it doesn't even properly summarize the cause
> *or* the user-visible symptom.  It just states how the confusion
> propagates somewhere in the middle of the code.

I don't think the cause or the user-visible symptom need any summary.

I was trying to summarize this:
---
This is because in zsh the following code:

 for i in $__git_all_commands

would evaluate $__git_all_commands as a single word (with spaces),
${=__git_all_commands} should be used to do word splitting expansion
(unless SH_WORD_SPLIT is used).

sh emulation should take care of that, but the command expantion is
messing up with that.
---

-- 
Felipe Contreras

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Felipe Contreras wrote:
>
>> Felipe Contreras (4):
>>   completion: be nicer with zsh
>
> Since I can't find this patch in the mail archive, I'll reply here.
> Luckily the most important bit is above already.

Thanks for playing a mail-relay.  Except for the much more readable log
message you have here, the result matches what I have at 06357ef (modulo
test vs '[').  I'll replace what I queued.

> I think I mentioned before that this subject line is what will appear
> in the shortlog and the shortlog is all that some people will see of
> the changelog, so it should include a self-contained description of
> the impact of the patch.
>
> However, clearly I did not say it clearly enough. :)  I guess it's
> better to take a cue from storytellers and show rather than tell.

Very big thanks for this ;-)

Re: [PATCH v3 0/4] completion: couple of cleanups

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Felipe Contreras (4):
>   completion: be nicer with zsh
>   completion: simplify __git_remotes
>   completion: remove unused code
>   completion: simplify __gitcomp*

My understanding is that the two patches relayed by Jonathan cover the
first three entries, so here is my attempt to reconstruct the last one
without seeing a single bit from the v3 series ;-)


-- >8 --
From: Felipe Contreras <felipe.contreras@gmail.com>
Subject: [PATCH] completion: simplify __gitcomp and __gitcomp_nl implementations

These shell functions are written in an unnecessarily verbose way;
simplify their "conditionally use $<number> after checking $# against
<number>" logic by using shell's built-in conditional substitution
facilities.

Also remove the first of the two assignments to IFS in __gitcomp_nl
that does not have any effect.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 contrib/completion/git-completion.bash |   19 +++----------------
 1 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8dd4e44..1ce1d6e 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -495,11 +495,8 @@ fi
 # 4: A suffix to be appended to each possible completion word (optional).
 __gitcomp ()
 {
-	local cur_="$cur"
+	local cur_="${3-$cur}"
 
-	if [ $# -gt 2 ]; then
-		cur_="$3"
-	fi
 	case "$cur_" in
 	--*=)
 		COMPREPLY=()
@@ -524,18 +521,8 @@ __gitcomp ()
 #    appended.
 __gitcomp_nl ()
 {
-	local s=$'\n' IFS=' '$'\t'$'\n'
-	local cur_="$cur" suffix=" "
-
-	if [ $# -gt 2 ]; then
-		cur_="$3"
-		if [ $# -gt 3 ]; then
-			suffix="$4"
-		fi
-	fi
-
-	IFS=$s
-	COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_"))
+	local IFS=$'\n'
+	COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
 }
 
 __git_heads ()
-- 
1.7.9.172.ge26ae

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Felipe Contreras]

On Thu, Feb 2, 2012 at 9:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> However, clearly I did not say it clearly enough. :)  I guess it's
>> better to take a cue from storytellers and show rather than tell.
>
> Very big thanks for this ;-)

Not a single comment regarding what I said? I don't see what's not
sensible about a commit message with this order:

 1) short description of the *purpose* of the patch
 2) Summary of the problem.
 3) Proposed solution
 4) Extra information, useful for future references, etc.

This ensures the most relevant information for most people is at the top.

-- 
Felipe Contreras

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Thu, Feb 2, 2012 at 9:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>> However, clearly I did not say it clearly enough. :) I guess it's
>>> better to take a cue from storytellers and show rather than tell.
>>
>> Very big thanks for this ;-)
>
> Not a single comment regarding what I said?

What entitles you to force me to refraining from commenting at all until I
read everything in my mailbox and after waiting for a while to make sure
there is no more to come to the thread?

In any case, "be nicer with zsh" conveys no more meaningful information
than "this is some patch about zsh".  Let's try to avoid warm and fuzzy
words that imply "goodness", e.g. "improve" and "be nicer with" because
nobody sends a patch to purposefully make Git worse and expects it to be
applied.

I found Jonathan's alternative "avoid default value assignment on : true
command" at least a bit better for the purpose of jogging the short-term
memory in the "'git shortlog v1.7.9.. contrib/completion/' tells us that
we have applied several patches, and I remember that : ${var=word} one!"
sense.  It is not super-useful for the longer term, though.

Here is what I ended up in preparation for queuing the series.  I still
haven't seen any version of 4/4, but please check $gmane/189683 and see if
that matches what you intended.  Also I am assuming $gmane/189606 relayed
by Jonathan is a squash between your 2 and 3 (which didn't reach me), so
please advise if that does not match what you want to have.

Thanks.

-- >8 --
From: Felipe Contreras <felipe.contreras@gmail.com>
Subject: [PATCH] completion: work around zsh option propagation bug

zsh versions from 4.3.0 to present (4.3.15) do not correctly propagate the
SH_WORD_SPLIT option into the subshell in ${foo:=$(bar)} expressions.  For
example, after running

	emulate sh
	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.  This fixes a bug tht caused all commands to be treated as
porcelain and show up in "git <TAB><TAB>" completion, because the list of
all commands was treated as a single word in __git_list_porcelain_commands
and did not match any of the patterns that would usually cause plumbing to
be excluded.

[jn: clarified commit message, indentation style fix]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 contrib/completion/git-completion.bash |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1496c6d..c636166 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -676,7 +676,8 @@ __git_merge_strategies=
 # is needed.
 __git_compute_merge_strategies ()
 {
-	: ${__git_merge_strategies:=$(__git_list_merge_strategies)}
+	test -n "$__git_merge_strategies" ||
+	__git_merge_strategies=$(__git_list_merge_strategies)
 }
 
 __git_complete_revlist_file ()
@@ -854,7 +855,8 @@ __git_list_all_commands ()
 __git_all_commands=
 __git_compute_all_commands ()
 {
-	: ${__git_all_commands:=$(__git_list_all_commands)}
+	test -n "$__git_all_commands" ||
+	__git_all_commands=$(__git_list_all_commands)
 }
 
 __git_list_porcelain_commands ()
@@ -947,7 +949,8 @@ __git_porcelain_commands=
 __git_compute_porcelain_commands ()
 {
 	__git_compute_all_commands
-	: ${__git_porcelain_commands:=$(__git_list_porcelain_commands)}
+	test -n "$__git_porcelain_commands" ||
+	__git_porcelain_commands=$(__git_list_porcelain_commands)
 }
 
 __git_pretty_aliases ()
-- 
1.7.9.172.ge26ae

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Felipe Contreras]

On Fri, Feb 3, 2012 at 2:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, Feb 2, 2012 at 9:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>>> However, clearly I did not say it clearly enough. :) I guess it's
>>>> better to take a cue from storytellers and show rather than tell.
>>>
>>> Very big thanks for this ;-)
>>
>> Not a single comment regarding what I said?
>
> What entitles you to force me to refraining from commenting at all until I
> read everything in my mailbox and after waiting for a while to make sure
> there is no more to come to the thread?

Fair enough. Just wondering.

> In any case, "be nicer with zsh" conveys no more meaningful information
> than "this is some patch about zsh".

And that already tells you a lot more than other alternatives.

> Let's try to avoid warm and fuzzy
> words that imply "goodness", e.g. "improve" and "be nicer with" because
> nobody sends a patch to purposefully make Git worse and expects it to be
> applied.

True. Which why I listened to the suggestion from Thomas Rast and
didn't use that, but "completion: work around zsh option propagation
bug" instead.

> I found Jonathan's alternative "avoid default value assignment on : true
> command" at least a bit better for the purpose of jogging the short-term
> memory in the "'git shortlog v1.7.9.. contrib/completion/' tells us that
> we have applied several patches, and I remember that : ${var=word} one!"
> sense.  It is not super-useful for the longer term, though.
>
> Here is what I ended up in preparation for queuing the series.  I still
> haven't seen any version of 4/4, but please check $gmane/189683 and see if
> that matches what you intended.  Also I am assuming $gmane/189606 relayed
> by Jonathan is a squash between your 2 and 3 (which didn't reach me), so
> please advise if that does not match what you want to have.

This is getting ridiculous, now I sent the patches directly to you, is
your pobox.com server also silently dropping them for no reason? I
think this is totally counter-productive. I haven't received any reply
from the vger postmaster, but I guess you should be able to find out
why your host is dropping mails. Am I the only one that has such
issues?

Anyway. I have uploaded all the mails to here:

http://people.freedesktop.org/~felipec/git-patches/

As for $gmane/189683, the changes seem to be correct, but I still
prefer my commit message[1]--which I have written and rewritten many
times now to improve it.

Regarding $gmane/189606, I still prefer my commit message[2], because
it starts with the *purpose* of the patch. As for the changes, they
are correct, and I don't mind squashing them, but they are *two*
logically independent changes; imagine in the future somebody
wants/need to re-enable __git_shopt, well, all they have to do is
revert the second patch. But that's up to you.

Cheers.

[1] http://people.freedesktop.org/~felipec/git-patches/4
[2] http://people.freedesktop.org/~felipec/git-patches/2

-- 
Felipe Contreras

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Fri, Feb 3, 2012 at 2:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> Here is what I ended up in preparation for queuing the series. I still
>> haven't seen any version of 4/4, but please check $gmane/189683 and see if
>> that matches what you intended. Also I am assuming $gmane/189606 relayed
>> by Jonathan is a squash between your 2 and 3 (which didn't reach me), so
>> please advise if that does not match what you want to have.
>
> This is getting ridiculous, now I sent the patches directly to you, is
> your pobox.com server also silently dropping them for no reason?

Do not blame pobox.com; they have nothing to do with the corruption of
your headers.

The volume of messages on the git mailing list I read on gmane's nntp
interface is much larger than that of my personal git-mail mailbox.
Patches and ideas in their early rounds do not come to my personal
git-mail mailbox (because I asked people not to cc: me such messages
unless I am an area expert, and they have been good to me), but I try to
point people not to go in a wrong direction as early as possible to avoid
wasting time for contributors and reviewers when I can.

So I almost always am on the mailing list "newsgroup" when dealing with
git related mail traffic (which is not 100% of my git time to begin with),
and only after I ran out of messages to process I check my personal
git-mail mailbox "newsgroup". I usually only find "help me" messages and
questions that are addressed directly to me in private, and my standard
response to them is to ask the sender to conduct the business in public on
the list instead. In other words, my personal git-mail mailbox "newsgroup"
is primariliy a back-up mailbox for my purpose.  I do not even run
fetchmail that often to poll it, because the latency for it is large due
to the way I work (described above).

You just caught me at the wrong moment when there were much more important
messages on the list (more refers to the volume, not all of them are more
important) and I was working on them (not limited to your issue) from top
to bottom in the mailing list "newsgroup".  I however wanted to get the
zsh issue resolved sooner, and because you seemed to have been having so
much trouble with your MUA (I only so 0/4 even for v4), I tried to help
out by sending what I thought is already good, hoping that a message that
only has to say "that looks good, thanks" would be easier to make it to
the list.

All that happened before I got to the back-up git mailbox where you sent
the rest of your v4.

People say "Oops, our mails crossed." and go on without making too much
fuss about it.  E-mail communications are asynchronous.  Get used to it.

I think your mail breakage, from looking at your mail header, is this:

  From: Felipe Contreras <felipe.contreras@gmail.com>
  To: git@vger.kernel.org
  Cc: Junio C Hamano <gitster@pobox.com>, SZEDER Gábor <szeder@ira.uka.de>, Jonathan Nieder <jrnieder@gmail.com>, Thomas Rast <trast@inf.ethz.ch>, Felipe Contreras <felipe.contreras@gmail.com>, "Shawn O. Pearce" <spearce@spearce.org>>
  Subject: [PATCH v4 1/4] completion: work around zsh option propagation bug

Notice the excess '>' after the last address on Cc:?

It's not like this is your first serious submission to the list, so it is
curious why only this time you have been having so much trouble. Perhaps
you have changed your mail set-up lately?

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Felipe Contreras]

On Fri, Feb 3, 2012 at 10:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Fri, Feb 3, 2012 at 2:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>>> Here is what I ended up in preparation for queuing the series. I still
>>> haven't seen any version of 4/4, but please check $gmane/189683 and see if
>>> that matches what you intended. Also I am assuming $gmane/189606 relayed
>>> by Jonathan is a squash between your 2 and 3 (which didn't reach me), so
>>> please advise if that does not match what you want to have.
>>
>> This is getting ridiculous, now I sent the patches directly to you, is
>> your pobox.com server also silently dropping them for no reason?
>
> Do not blame pobox.com; they have nothing to do with the corruption of
> your headers.

No, but they have everything to do with *silently* dropping it. Why
couldn't they _at least_ return an error saying that the headers are
wrong? Note that other servers didn't even complain, they processed
the mail happily.

In any case, the one to blame for the header corruption is git:

% git blame -e -L 947,+7 contrib/completion/git-completion.bash v1.7.9
eaa4e6ee (<jrnieder@gmail.com>   2009-11-17 18:49:10 -0600 947)
__git_compute_porcelain_commands ()
eaa4e6ee (<jrnieder@gmail.com>   2009-11-17 18:49:10 -0600 948) {
eaa4e6ee (<jrnieder@gmail.com>   2009-11-17 18:49:10 -0600 949)
 __git_compute_all_commands
eaa4e6ee (<jrnieder@gmail.com>   2009-11-17 18:49:10 -0600 950)
 : ${__git_porcelain_commands:=$(__git_list_porcelain_commands)}
eaa4e6ee (<jrnieder@gmail.com>   2009-11-17 18:49:10 -0600 951) }
f2bb9f88 (<spearce@spearce.org>> 2006-11-27 03:41:01 -0500 952)
c3898111 (<szeder@ira.uka.de>    2010-10-11 00:06:22 +0200 953)
__git_pretty_aliases ()

Notice the mail is wrong.

And then, 'git send-email' is happy to send such headers. I sent a
patch to the list to improve that situation, but looks like
sanitize_address() could be improved a lot.

You can blame it on 'git send-email', or 'git blame', or my cccmd
script[1], but as I discussed before, this is *precisely* the reason
why it would be nice to have an official cccmd script.

> You just caught me at the wrong moment when there were much more important
> messages on the list (more refers to the volume, not all of them are more
> important) and I was working on them (not limited to your issue) from top
> to bottom in the mailing list "newsgroup".  I however wanted to get the
> zsh issue resolved sooner, and because you seemed to have been having so
> much trouble with your MUA (I only so 0/4 even for v4), I tried to help
> out by sending what I thought is already good, hoping that a message that
> only has to say "that looks good, thanks" would be easier to make it to
> the list.

I was not nor am I blaming you, I am just saying the situation is ridiculous.

And my MUA is 'git send-email'.

> People say "Oops, our mails crossed." and go on without making too much
> fuss about it.  E-mail communications are asynchronous.  Get used to it.

That's not the problem, but problem is these sanctimonious mail
servers that drop mails without warning. And sure, also the code that
sends malformed mail.

In any case, fixing 'git blame', and/or 'git send-email', and/or have
an official cccmd script, would solve the problem.

> I think your mail breakage, from looking at your mail header, is this:
>
>  From: Felipe Contreras <felipe.contreras@gmail.com>
>  To: git@vger.kernel.org
>  Cc: Junio C Hamano <gitster@pobox.com>, SZEDER Gábor <szeder@ira.uka.de>, Jonathan Nieder <jrnieder@gmail.com>, Thomas Rast <trast@inf.ethz.ch>, Felipe Contreras <felipe.contreras@gmail.com>, "Shawn O. Pearce" <spearce@spearce.org>>
>  Subject: [PATCH v4 1/4] completion: work around zsh option propagation bug
>
> Notice the excess '>' after the last address on Cc:?

Thanks for pointing that out :)

> It's not like this is your first serious submission to the list, so it is
> curious why only this time you have been having so much trouble. Perhaps
> you have changed your mail set-up lately?

Yes, people asked me to CC more relevant people, so I enabled by
cc-cmd script that I sent to the list long time ago, but didn't
receive any more replies, and I also asked if it wouldn't make sense
to have an official one [1] -- no replies.

Cheers.

[1] http://article.gmane.org/gmane.comp.version-control.git/189360

-- 
Felipe Contreras

[bug] blame duplicates trailing ">" in mailmapped emails

[Jeff King]

On Sat, Feb 04, 2012 at 05:46:05PM +0200, Felipe Contreras wrote:

> In any case, the one to blame for the header corruption is git:
> [...]
> f2bb9f88 (<spearce@spearce.org>> 2006-11-27 03:41:01 -0500 952)
> 
> Notice the mail is wrong.

Ugh. The fault lies in this code:

  $ sed -n 1405,1414p builtin/blame.c 
          if (map_user(&mailmap, mail+1, mail_len-1, person, tmp-person-1)) {
                  /* Add a trailing '>' to email, since map_user returns plain emails
                     Note: It already has '<', since we replace from mail+1 */
                  mailpos = memchr(mail, '\0', mail_len);
                  if (mailpos && mailpos-mail < mail_len - 1) {
                          *mailpos = '>';
                          *(mailpos+1) = '\0';
                  }
          }
  }

But that comment is wrong. If there's no email mapping needed, map_user
will leave the "mail" buffer intact, in which case it will have the
trailing ">" (because we feed the address with enclosing angle
brackets).  So while map_user tries to accept either "foo@example.com\0"
and "foo@example.com>", it is up to the contents of the mailmap whether
you get back something with the closing angle bracket or not. Which is a
pretty error-prone interface.

You can fix it with this:

diff --git a/builtin/blame.c b/builtin/blame.c
index 5a67c20..9b886fa 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1406,7 +1406,8 @@ static void get_ac_line(const char *inbuf, const char *what,
 		/* Add a trailing '>' to email, since map_user returns plain emails
 		   Note: It already has '<', since we replace from mail+1 */
 		mailpos = memchr(mail, '\0', mail_len);
-		if (mailpos && mailpos-mail < mail_len - 1) {
+		if (mailpos && mailpos-mail < mail_len - 1 &&
+		    mailpos > mail && *(mailpos-1) != '>') {
 			*mailpos = '>';
 			*(mailpos+1) = '\0';
 		}

but it feels like the fix should go into map_user.  I tried a few things,
like "git log -1 --format=%aE", and couldn't find other code paths with
this problem. So presumably they are all feeding email addresses without
the closing ">" (so one option is to just say "map_user needs to get
NUL-terminated strings).

-Peff

Re: [bug] blame duplicates trailing ">" in mailmapped emails

[Felipe Contreras]

On Sat, Feb 4, 2012 at 8:26 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Feb 04, 2012 at 05:46:05PM +0200, Felipe Contreras wrote:
>
>> In any case, the one to blame for the header corruption is git:
>> [...]
>> f2bb9f88 (<spearce@spearce.org>> 2006-11-27 03:41:01 -0500 952)
>>
>> Notice the mail is wrong.
>
> Ugh. The fault lies in this code:

Yes, I found that as well.

> But that comment is wrong. If there's no email mapping needed, map_user
> will leave the "mail" buffer intact, in which case it will have the
> trailing ">" (because we feed the address with enclosing angle
> brackets).  So while map_user tries to accept either "foo@example.com\0"
> and "foo@example.com>", it is up to the contents of the mailmap whether
> you get back something with the closing angle bracket or not. Which is a
> pretty error-prone interface.
>
> You can fix it with this:
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 5a67c20..9b886fa 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1406,7 +1406,8 @@ static void get_ac_line(const char *inbuf, const char *what,
>                /* Add a trailing '>' to email, since map_user returns plain emails
>                   Note: It already has '<', since we replace from mail+1 */
>                mailpos = memchr(mail, '\0', mail_len);
> -               if (mailpos && mailpos-mail < mail_len - 1) {
> +               if (mailpos && mailpos-mail < mail_len - 1 &&
> +                   mailpos > mail && *(mailpos-1) != '>') {
>                        *mailpos = '>';
>                        *(mailpos+1) = '\0';
>                }

Yes, I already did this. I'm writing tests for this right now, but I
think I found yet another bug...

> but it feels like the fix should go into map_user.  I tried a few things,
> like "git log -1 --format=%aE", and couldn't find other code paths with
> this problem. So presumably they are all feeding email addresses without
> the closing ">" (so one option is to just say "map_user needs to get
> NUL-terminated strings).

Perhaps, but I though the idea was to make it efficient. I think the
above fix should be ok.

We should have tests for this though, to make sure it doesn't get
broken again. I'm on that.

Cheers.

-- 
Felipe Contreras

Re: [bug] blame duplicates trailing ">" in mailmapped emails

[Jeff King]

On Sat, Feb 04, 2012 at 09:30:42PM +0200, Felipe Contreras wrote:

> > but it feels like the fix should go into map_user.  I tried a few things,
> > like "git log -1 --format=%aE", and couldn't find other code paths with
> > this problem. So presumably they are all feeding email addresses without
> > the closing ">" (so one option is to just say "map_user needs to get
> > NUL-terminated strings).
> 
> Perhaps, but I though the idea was to make it efficient. I think the
> above fix should be ok.

Because of the calling convention of map_user, the buffer with the input
must also be writable (since it holds the result). So there should be no
loss of efficiency to convert the ">" into a "\0" (and in fact, the
simplest fix is probably to just have map_user "tie off" any ">" it
detects).

> We should have tests for this though, to make sure it doesn't get
> broken again. I'm on that.

Definitely. Thanks for working on it.

-Peff

Re: [bug] blame duplicates trailing ">" in mailmapped emails

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Feb 04, 2012 at 05:46:05PM +0200, Felipe Contreras wrote:
>
>> In any case, the one to blame for the header corruption is git:
>> [...]
>> f2bb9f88 (<spearce@spearce.org>> 2006-11-27 03:41:01 -0500 952)
>> 
>> Notice the mail is wrong.
> ...
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 5a67c20..9b886fa 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1406,7 +1406,8 @@ static void get_ac_line(const char *inbuf, const char *what,
>  		/* Add a trailing '>' to email, since map_user returns plain emails
>  		   Note: It already has '<', since we replace from mail+1 */
>  		mailpos = memchr(mail, '\0', mail_len);
> -		if (mailpos && mailpos-mail < mail_len - 1) {
> +		if (mailpos && mailpos-mail < mail_len - 1 &&
> +		    mailpos > mail && *(mailpos-1) != '>') {
>  			*mailpos = '>';
>  			*(mailpos+1) = '\0';
>  		}
>
> but it feels like the fix should go into map_user.

Thanks.

The map_user() API takes an email address that is terminated by either NUL
or '>' to allow the caller to learn the corresponding up-to-date email
address that is NUL terminated, while indicating with its return value
that if the caller even needs to replace what it already has.  But the
function does not properly terminate email when it only touched the name
part. And I think that is the real bug.

So I agree that the real fix should go to map_user() so that when it says
"I've updated something, so pick up the updated result from the i/o
arguments you gave me, i.e. email and name", it makes sure what it claims
to be an e-mail address does not have the extra '>' in it.

Working around the current behaviour by forcing all callers that pass '>'
terminated e-mail address to have the code like the above quoted patch 
does not feel right.

Re: [PATCH v3 1/4] completion: be nicer with zsh

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> Do not blame pobox.com; they have nothing to do with the corruption of
>> your headers.
>
> No, but they have everything to do with *silently* dropping it. Why
> couldn't they _at least_ return an error saying that the headers are
> wrong? Note that other servers didn't even complain, they processed
> the mail happily.

Again, please do not blame pobox.com; they fall into your "other servers"
category.  You are probably talking about vger.kernel.org that is extra
picky and wants to avoid wasting their outgoing bandwidth because they get
so much spams.

> % git blame -e -L 947,+7 contrib/completion/git-completion.bash v1.7.9
> ...
> f2bb9f88 (<spearce@spearce.org>> 2006-11-27 03:41:01 -0500 952)

I am glad to see that something useful came out from your digging, and a
fix is being worked on it, while I was away from my machines.  Thanks for
getting the ball rolling.

Re: [bug] blame duplicates trailing ">" in mailmapped emails

[Felipe Contreras]

On Sun, Feb 5, 2012 at 1:20 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Feb 04, 2012 at 09:30:42PM +0200, Felipe Contreras wrote:
>
>> > but it feels like the fix should go into map_user.  I tried a few things,
>> > like "git log -1 --format=%aE", and couldn't find other code paths with
>> > this problem. So presumably they are all feeding email addresses without
>> > the closing ">" (so one option is to just say "map_user needs to get
>> > NUL-terminated strings).
>>
>> Perhaps, but I though the idea was to make it efficient. I think the
>> above fix should be ok.
>
> Because of the calling convention of map_user, the buffer with the input
> must also be writable (since it holds the result). So there should be no
> loss of efficiency to convert the ">" into a "\0" (and in fact, the
> simplest fix is probably to just have map_user "tie off" any ">" it
> detects).

Yes, but then the caller (git blame) would need to _always_ do that
conversion before (">" -> "\0"), and after ("\0" -> ">"), as opposed
to now, that it does the conversion only when map_user succeeds (or
checks if it has to do it).

-- 
Felipe Contreras

Re: [bug] blame duplicates trailing ">" in mailmapped emails

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
> ...
>> but it feels like the fix should go into map_user.
>
> Thanks.
>
> The map_user() API takes an email address that is terminated by either NUL
> or '>' to allow the caller to learn the corresponding up-to-date email
> address that is NUL terminated, while indicating with its return value
> that if the caller even needs to replace what it already has.  But the
> function does not properly terminate email when it only touched the name
> part. And I think that is the real bug.

And the gist of the patch to fix the bug would look like this two liner.
In the real fix, "p" should be renamed to "end_of_email" or something
descriptive like that.

I only made sure that this fixes the original case of the email address of
Shawn reported by Felipe, but other than that like everything else I send
here with "... would look like this", not tested beyond "it compiles".

But conceptually it looks correct (famous last words ;-).

Felipe, does it pass your test cases?


 mailmap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index 8c3196c..ce805fa 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -236,6 +236,8 @@ int map_user(struct string_list *map,
 		}
 		if (maxlen_email && mi->email)
 			strlcpy(email, mi->email, maxlen_email);
+		else
+			*p = '\0';
 		if (maxlen_name && mi->name)
 			strlcpy(name, mi->name, maxlen_name);
 		debug_mm("map_user:  to '%s' <%s>\n", name, mi->email ? mi->email : "");

Re: [bug] blame duplicates trailing ">" in mailmapped emails

[Jeff King]

On Sun, Feb 05, 2012 at 01:38:19PM -0800, Junio C Hamano wrote:

> > The map_user() API takes an email address that is terminated by either NUL
> > or '>' to allow the caller to learn the corresponding up-to-date email
> > address that is NUL terminated, while indicating with its return value
> > that if the caller even needs to replace what it already has.  But the
> > function does not properly terminate email when it only touched the name
> > part. And I think that is the real bug.
> 
> And the gist of the patch to fix the bug would look like this two liner.
> In the real fix, "p" should be renamed to "end_of_email" or something
> descriptive like that.

Exactly; this is much better.

We could also go as far as saying that map_user would _always_ terminate
in this way (i.e., the caller gets a munged result, whether we found
anything or not). Then internally, map_user could be simplified to stop
worrying about making a temporary copy in mailbuf. And callers could
simply call map_user without worrying about branching on whether it
found anything or not.

But maybe it is not worth that level of refactoring. From my reading,
your patch fixes the problem just fine.

-Peff

Re: [bug] blame duplicates trailing ">" in mailmapped emails

[Jeff King]

On Sun, Feb 05, 2012 at 11:11:20PM +0200, Felipe Contreras wrote:

> > Because of the calling convention of map_user, the buffer with the input
> > must also be writable (since it holds the result). So there should be no
> > loss of efficiency to convert the ">" into a "\0" (and in fact, the
> > simplest fix is probably to just have map_user "tie off" any ">" it
> > detects).
> 
> Yes, but then the caller (git blame) would need to _always_ do that
> conversion before (">" -> "\0"), and after ("\0" -> ">"), as opposed
> to now, that it does the conversion only when map_user succeeds (or
> checks if it has to do it).

Yes, I'm talking about changing the calling and return conventions of
map_user. I think the efficiency change is negligible, though, as we
are talking about character assignments (and in fact, it would probably
end up more efficient, as we could eliminate some copying inside
map_user). But Junio's patch is simple, and fixes the problem without
creating any complexity for the callers. So I think it's a good fix.

-Peff

Re: [bug] blame duplicates trailing ">" in mailmapped emails

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We could also go as far as saying that map_user would _always_ terminate
> in this way (i.e., the caller gets a munged result, whether we found
> anything or not). Then internally, map_user could be simplified to stop
> worrying about making a temporary copy in mailbuf. And callers could
> simply call map_user without worrying about branching on whether it
> found anything or not.

I thought about it, but such a change needs to audit all the call sites
that assumes the promise original map_user() used to make before it was
broken. If we return 0 to the caller, the caller does not have to worry
about map_user() munging the buffer it lent to it.

It might be a worthwhile thing to do. I dunno; I didn't look into it.

Re: [bug] blame duplicates trailing ">" in mailmapped emails

[Jeff King]

On Sun, Feb 05, 2012 at 04:39:35PM -0800, Junio C Hamano wrote:

> > We could also go as far as saying that map_user would _always_ terminate
> > in this way (i.e., the caller gets a munged result, whether we found
> > anything or not). Then internally, map_user could be simplified to stop
> > worrying about making a temporary copy in mailbuf. And callers could
> > simply call map_user without worrying about branching on whether it
> > found anything or not.
> 
> I thought about it, but such a change needs to audit all the call sites
> that assumes the promise original map_user() used to make before it was
> broken. If we return 0 to the caller, the caller does not have to worry
> about map_user() munging the buffer it lent to it.
> 
> It might be a worthwhile thing to do. I dunno; I didn't look into it.

Ugh, yeah. I was thinking about how it would improve this call site, but
I don't want to get into auditing the others. Let's drop it and go with
your patch.

-Peff

Re: [bug] blame duplicates trailing ">" in mailmapped emails

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> It might be a worthwhile thing to do. I dunno; I didn't look into it.
>
> Ugh, yeah. I was thinking about how it would improve this call site, but
> I don't want to get into auditing the others.

There aren't that many, though.  shortlog has one, pretty has another and
that is about it.

But both seems to care that map_user() is not a function that returns void,
so...

Re: [bug] blame duplicates trailing ">" in mailmapped emails

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Ugh, yeah. I was thinking about how it would improve this call site, but
> I don't want to get into auditing the others. Let's drop it and go with
> your patch.

In any case, here is what I queued for tonight.

-- >8 --
Subject: [PATCH] mailmap: do not leave '>' in the output when answering "we did something"

The callers of map_user() give email and name to it, and expect to get an
up-to-date versions of email and/or name to be used in their output. The
function rewrites the given buffers in place. To optimize the majority of
cases, the function returns 0 when it did not do anything, and it returns
1 when the caller should use the updated contents.

The 'email' input to the function is terminated by '>' or a NUL (whichever
comes first) for historical reasons, but when a rewrite happens, the value
is replaced with the mailbox inside the <> pair.  However, it failed to
meet this expectation when it only rewrote the name part without rewriting
the email part, and the email in the input was terminated by '>'.

This causes an extra '>' to appear in the output of "blame -e", because the
caller does send in '>'-terminated email, and when the function returned 1
to tell it that rewriting happened, it appends '>' that is necessary when
the email part was rewritten.

The patch looks bigger than it actually is, because this change makes a
variable that points at the end of the email part in the input 'p' live
much longer than it used to, deserving a more descriptive name.

Noticed and diagnosed by Felipe Contreras and Jeff King.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 mailmap.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index 8c3196c..47aa419 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -190,27 +190,27 @@ void clear_mailmap(struct string_list *map)
 int map_user(struct string_list *map,
 	     char *email, int maxlen_email, char *name, int maxlen_name)
 {
-	char *p;
+	char *end_of_email;
 	struct string_list_item *item;
 	struct mailmap_entry *me;
 	char buf[1024], *mailbuf;
 	int i;
 
 	/* figure out space requirement for email */
-	p = strchr(email, '>');
-	if (!p) {
+	end_of_email = strchr(email, '>');
+	if (!end_of_email) {
 		/* email passed in might not be wrapped in <>, but end with a \0 */
-		p = memchr(email, '\0', maxlen_email);
-		if (!p)
+		end_of_email = memchr(email, '\0', maxlen_email);
+		if (!end_of_email)
 			return 0;
 	}
-	if (p - email + 1 < sizeof(buf))
+	if (end_of_email - email + 1 < sizeof(buf))
 		mailbuf = buf;
 	else
-		mailbuf = xmalloc(p - email + 1);
+		mailbuf = xmalloc(end_of_email - email + 1);
 
 	/* downcase the email address */
-	for (i = 0; i < p - email; i++)
+	for (i = 0; i < end_of_email - email; i++)
 		mailbuf[i] = tolower(email[i]);
 	mailbuf[i] = 0;
 
@@ -236,6 +236,8 @@ int map_user(struct string_list *map,
 		}
 		if (maxlen_email && mi->email)
 			strlcpy(email, mi->email, maxlen_email);
+		else
+			*end_of_email = '\0';
 		if (maxlen_name && mi->name)
 			strlcpy(name, mi->name, maxlen_name);
 		debug_mm("map_user:  to '%s' <%s>\n", name, mi->email ? mi->email : "");
-- 
1.7.9.204.gdf845

Re: [bug] blame duplicates trailing ">" in mailmapped emails

[Jeff King]

On Sun, Feb 05, 2012 at 07:16:22PM -0800, Junio C Hamano wrote:

> In any case, here is what I queued for tonight.
> 
> -- >8 --
> Subject: [PATCH] mailmap: do not leave '>' in the output when answering "we did something"

Looks good to me.

> The callers of map_user() give email and name to it, and expect to get an
> up-to-date versions of email and/or name to be used in their output. The

Minor grammar error.

-Peff

Re: [bug] blame duplicates trailing ">" in mailmapped emails

[Felipe Contreras]

On Mon, Feb 6, 2012 at 5:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> Ugh, yeah. I was thinking about how it would improve this call site, but
>> I don't want to get into auditing the others. Let's drop it and go with
>> your patch.
>
> In any case, here is what I queued for tonight.
>
> -- >8 --
> Subject: [PATCH] mailmap: do not leave '>' in the output when answering "we did something"

This subject doesn't explain the *purpose* of the patch: always return
a plain mail address from map_user()

That would be enough for most readers.

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.

At this point a lot of readers can skip the rest of the explanation.

> The callers of map_user() give email and name to it, and expect to get an
> up-to-date versions of email and/or name to be used in their output. The
> function rewrites the given buffers in place. To optimize the majority of
> cases, the function returns 0 when it did not do anything, and it returns
> 1 when the caller should use the updated contents.
>
> The 'email' input to the function is terminated by '>' or a NUL (whichever
> comes first) for historical reasons, but when a rewrite happens, the value
> is replaced with the mailbox inside the <> pair.  However, it failed to
> meet this expectation when it only rewrote the name part without rewriting
> the email part, and the email in the input was terminated by '>'.

With the above explanation I suggested, I think this can be summarized as:

As of right now most of the callers of map_user() give a plan address,
and expect a plain address, however, 'git blame -e' passes along a
mail address that ends with >, and uses the return value to determine
if a transformation was made, and adds the missing '>'. This is
because when map_user() does the transformation, it returns a plan
address.

This might have worked in previous versions of map_user(), but now not
only does it transforms mail addresses, but also the name (phrase). So
now callers can't use the return value to know if they need to add an
extra '>', or not. So lets always remove the '>', so they can.

> This causes an extra '>' to appear in the output of "blame -e", because the
> caller does send in '>'-terminated email, and when the function returned 1
> to tell it that rewriting happened, it appends '>' that is necessary when
> the email part was rewritten.

It took too long to reach the problem. As a reader, that's the first
thing I would expect.

> The patch looks bigger than it actually is, because this change makes a
> variable that points at the end of the email part in the input 'p' live
> much longer than it used to, deserving a more descriptive name.

This is a *separate* logically independent patch. And you yourself
mention, makes the review of the patch more difficult, as of right now
it's difficult to spot the functional change, because it's mixed with
non-functional changes.

I find it peculiar how patches in the Linux kernel are truly logically
independent, but Git has a tendency to squash many things: cleanups,
fixes, documentation, tests, extra tests, remove unused code, etc.

Cheers.

-- 
Felipe Contreras

Re: [bug] blame duplicates trailing ">" in mailmapped emails

[Junio C Hamano]

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.

Re: [bug] blame duplicates trailing ">" in mailmapped emails

[Felipe Contreras]

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