[PATCH 0/3] completion: trivial cleanups

[PATCH 0/3] completion: trivial cleanups

[Felipe Contreras]

And an improvement for zsh.

Felipe Contreras (3):
  completion: be nicer with zsh
  completion: remove old code
  completion: remove unused code

 contrib/completion/git-completion.bash |   47 +++++---------------------------
 1 files changed, 7 insertions(+), 40 deletions(-)

-- 
1.7.8.3

[PATCH 1/3] completion: be nicer with zsh

[Felipe Contreras]

And yet another bug in zsh[1] causes a mismatch; zsh seems to have
problem emulating wordspliting, but only when the ':' command is
involved.

Let's avoid it. This has the advantage that the code is now actually
understandable (at least to me), while before it looked like voodoo.

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

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

Signed-off-by: Felipe Contreras <felipe.contreras@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 1496c6d..7051c7a 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 "$__git_merge_strategies" && return
+	__git_merge_strategies=$(__git_list_merge_strategies 2> /dev/null)
 }
 
 __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 "$__git_all_commands" && return
+	__git_all_commands=$(__git_list_all_commands 2> /dev/null)
 }
 
 __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 "$__git_porcelain_commands" && return
+	__git_porcelain_commands=$(__git_list_porcelain_commands 2> /dev/null)
 }
 
 __git_pretty_aliases ()
-- 
1.7.8.3

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

[Junio C Hamano]

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

> Let's avoid it. This has the advantage that the code is now actually
> understandable (at least to me), while before it looked like voodoo.

I am somewhat hesitant to accept a patch to shell scripts on the basis
that the patch author does not understand the existing constructs that
are standard parts of shell idioms.

Avoiding zsh's bug that cannot use conditional assignment on the no-op
colon command (if the bug is really that; it is somewhat hard to imagine
if the bug exists only for colon command, though) *is* by itself a good
justification for this change, even though the resulting code is harder to
read for people who are used to read shell scripts.

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

[Junio C Hamano]

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

> Avoiding zsh's bug that cannot use conditional assignment on the no-op
> colon command (if the bug is really that; it is somewhat hard to imagine
> if the bug exists only for colon command, though) *is* by itself a good
> justification for this change, even though the resulting code is harder to
> read for people who are used to read shell scripts.

Just from my curiosity, I am wondering what zsh does when given these:

	bar () { echo "frotz nitfol xyzzy" }

	unset foo; : ${foo:=$(bar)}; echo "<$?,$foo>"
        unset foo; true ${foo:=$(bar)}; echo "<$?,$foo>"
        unset foo; echo >/dev/null ${foo:=$(bar)}; echo "<$?,$foo>"

The first one is exactly your "And yet another bug in zsh[1] causes a
mismatch; zsh seems to have problem emulating wordspliting, but only when
the ':' command is involved.", so we already know it "seems to have
problem emulating word-splitting" (by the way, can we replace that with
exact description of faulty symptom? e.g. "does not split words at $IFS"
might be what you meant but still when we are assigning the result to a
single variable, it is unclear how that matters).

Note that I am not suggesting to rewrite the existing ": ${var:=val}" with
"echo ${var:val} >/dev/null" at all. Even if "echo >/dev/null" makes it
work as expected, your rewrite to protect it with an explicit conditional
e.g. "test -n ${foo:-} || foo=$(bar)" would be a lot better than funny
construct like "echo >/dev/null ${foo:=$(bar)", because it is not an
established shell idiom to use default assignment with anything but ":".

Thanks.

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

[Felipe Contreras]

On Mon, Jan 30, 2012 at 7:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Avoiding zsh's bug that cannot use conditional assignment on the no-op
>> colon command (if the bug is really that; it is somewhat hard to imagine
>> if the bug exists only for colon command, though) *is* by itself a good
>> justification for this change, even though the resulting code is harder to
>> read for people who are used to read shell scripts.
>
> Just from my curiosity, I am wondering what zsh does when given these:
>
>        bar () { echo "frotz nitfol xyzzy" }
>
>        unset foo; : ${foo:=$(bar)}; echo "<$?,$foo>"
>        unset foo; true ${foo:=$(bar)}; echo "<$?,$foo>"
>        unset foo; echo >/dev/null ${foo:=$(bar)}; echo "<$?,$foo>"

<0,frotz nitfol xyzzy>
<0,frotz nitfol xyzzy>
<0,frotz nitfol xyzzy>

And that's _without_ bash emulation.

BTW. That code didn't work for me in bash (though it did in zsh), I
had to add a semicolon:

 bar () { echo "frotz nitfol xyzzy" ;}

> The first one is exactly your "And yet another bug in zsh[1] causes a
> mismatch; zsh seems to have problem emulating wordspliting, but only when
> the ':' command is involved.", so we already know it "seems to have
> problem emulating word-splitting" (by the way, can we replace that with
> exact description of faulty symptom? e.g. "does not split words at $IFS"
> might be what you meant but still when we are assigning the result to a
> single variable, it is unclear how that matters).

That's not the problem, the problem is that this doesn't work in zsh:

array="a b c"
for i in $array; do
 echo $i
done

The result is "a b c". Unless sh emulation is on. This is the correct
way in zsh:

array="a b c"
for i in ${=array}; do
 echo $i
done

But this behavior can be controlled with SH_WORD_SPLIT.

Anyway, as I said, the problem is that the ':' have some problems, and
sh emulation seems to be turned off inside such command, or at least
SH_WORD_SPLIT was reset in my tests.

-- 
Felipe Contreras

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

[Felipe Contreras]

On Mon, Jan 30, 2012 at 6:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Let's avoid it. This has the advantage that the code is now actually
>> understandable (at least to me), while before it looked like voodoo.
>
> I am somewhat hesitant to accept a patch to shell scripts on the basis
> that the patch author does not understand the existing constructs that
> are standard parts of shell idioms.

I have been writing shell scripts for years[1], and I have *never* had
an encounter with ':'. vim's sh syntax doesn't seem to be prepared for
it, and zsh's sh emulation has problems only when ':' is involved, so
I still think ':' is quite obscure.

Plus, I haven't seen ${foo:=bar} that often.

In any case, there's no need for ad hominem arguments; there is a
problem when using zsh, that's a fact.

[1] https://www.ohloh.net/accounts/felipec/positions/total

-- 
Felipe Contreras

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

[Junio C Hamano]

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

> In any case, there's no need for ad hominem arguments; there is a
> problem when using zsh, that's a fact.

There was no ad-hominem argument at all.

Read your two lines I quoted "... the code is now actually understandable
(at least to me), while before it looked like voodoo", which was your
words.  What does it tell the reader?  The patch author (1) did not
understand existing code (voodoo) and (2) the change is a good thing as a
style/readability improvement.

I was saying that I did not want to see that in the justification, because
(2) is not true, while (1) may be.

The patch as-is is a good change that works around issues with zsh's POSIX
emulation, and that is sufficient-enough justification. IOW, we are in
agreement on the later half of your sentence.

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

[Felipe Contreras]

On Mon, Jan 30, 2012 at 8:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> In any case, there's no need for ad hominem arguments; there is a
>> problem when using zsh, that's a fact.
>
> There was no ad-hominem argument at all.
>
> Read your two lines I quoted "... the code is now actually understandable
> (at least to me), while before it looked like voodoo", which was your
> words.  What does it tell the reader?  The patch author (1) did not
> understand existing code (voodoo) and (2) the change is a good thing as a
> style/readability improvement.

I disagree. Another possibility is that the code actually looked like
voodoo (it was obfuscated). You might disagree, but the fact that one
of the main editors (the most used?) doesn't even recognize the syntax
of this code I think is pretty telling.

> I was saying that I did not want to see that in the justification, because
> (2) is not true, while (1) may be.

That's not true: (2) might be true; at least it's debatable.

> The patch as-is is a good change that works around issues with zsh's POSIX
> emulation, and that is sufficient-enough justification. IOW, we are in
> agreement on the later half of your sentence.

So, I shall just remove that part of the explanation?

-- 
Felipe Contreras

[PATCH 2/3] completion: remove old code

[Felipe Contreras]

We don't need to check for GIT_DIR/remotes, right? This was removed long
time ago.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7051c7a..f7278b5 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -643,13 +643,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
+	local i IFS=$'\n' d="$(__gitdir)"
 	for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do
 		i="${i#remote.}"
 		echo "${i/.url*/}"
-- 
1.7.8.3

Re: [PATCH 2/3] completion: remove old code

[Jonathan Nieder]

Hi,

Felipe Contreras wrote:

> We don't need to check for GIT_DIR/remotes, right? This was removed long
> time ago.

I don't follow.  fetch, push, and remote still look in .git/remotes
like they always did, last time I checked.

Perhaps you mean that /usr/share/git-core/templates/ no longer
contains a remotes/ directory?  That's true but not particularly
relevant.  A more relevant detail would be that very few people _use_
the .git/remotes feature, though it is not obvious to me whether that
justifies removing this code from the git-completion script that
already works.

Re: [PATCH 2/3] completion: remove old code

[Felipe Contreras]

On Mon, Jan 30, 2012 at 4:36 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> We don't need to check for GIT_DIR/remotes, right? This was removed long
>> time ago.
>
> I don't follow.  fetch, push, and remote still look in .git/remotes
> like they always did, last time I checked.
>
> Perhaps you mean that /usr/share/git-core/templates/ no longer
> contains a remotes/ directory?  That's true but not particularly
> relevant.  A more relevant detail would be that very few people _use_
> the .git/remotes feature, though it is not obvious to me whether that
> justifies removing this code from the git-completion script that
> already works.

The problem is all the 'nullglob' stuff. It's *a lot* of code for this
feature that nobody uses.

OK, maybe some people use it, but most likely they are using an old
version of git, and thus an old version of the completion script.

Anyway, aren't there easier ways to get this? Perhaps first checking
if the directory exists, to avoid wasting cycles.

Something like:
  test -d "$d/remotes" && ls -1 "$d/remotes"

-- 
Felipe Contreras

Re: [PATCH 2/3] completion: remove old code

[Jonathan Nieder]

Felipe Contreras wrote:

> Anyway, aren't there easier ways to get this? Perhaps first checking
> if the directory exists, to avoid wasting cycles.
>
> Something like:
>   test -d "$d/remotes" && ls -1 "$d/remotes"

Yeah, that sounds like a good idea.  Could you send a patch that does
that?

Thanks,
Jonathan

Re: [PATCH 2/3] completion: remove old code

[Junio C Hamano]

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

> OK, maybe some people use it, but most likely they are using an old
> version of git, and thus an old version of the completion script.

Please adjust your attitude about backward compatibility to match the
standard used for other parts of Git.

Most likely they are using repositories that they started using with an
old version, but at the same time, most likely they are happily using more
modern version exactly because the rest of Git still support it, except
for the completion script after _this_ patch breaks the support.

Re: [PATCH 2/3] completion: remove old code

[Felipe Contreras]

On Mon, Jan 30, 2012 at 6:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> OK, maybe some people use it, but most likely they are using an old
>> version of git, and thus an old version of the completion script.
>
> Please adjust your attitude about backward compatibility to match the
> standard used for other parts of Git.

What attitude? I am simply stating a fact. How much percentage of
people do you think still have .git/remotes around? How many people do
you think have clones more than 3 years old? And how many of these
people would complain if remotes were not properly completed for these
repos?

I doubt anybody would have complained, but I guess we would never
know, because I already proposed a solution that would work for them
and only uses a *single* line of code, unlike the current 40 ones.

I don't see what is the problem with the attitude of sending a patch
to remove code that most likely nobody cares about (neither you or I
have numbers on this), and then finding an alternative when people do
care about it.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 2/3] completion: remove old code

[Frans Klaver]

Hi,

On Mon, Jan 30, 2012 at 11:51 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Mon, Jan 30, 2012 at 6:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> OK, maybe some people use it, but most likely they are using an old
>>> version of git, and thus an old version of the completion script.
>>
>> Please adjust your attitude about backward compatibility to match the
>> standard used for other parts of Git.
>
> What attitude?

This attitude:

> I am simply stating a fact. How much percentage of
> people do you think still have .git/remotes around? How many people do
> you think have clones more than 3 years old? And how many of these
> people would complain if remotes were not properly completed for these
> repos?
>
> I doubt anybody would have complained, but I guess we would never
> know, because I already proposed a solution that would work for them
> and only uses a *single* line of code, unlike the current 40 ones.
>
> I don't see what is the problem with the attitude of sending a patch
> to remove code that most likely nobody cares about (neither you or I
> have numbers on this), and then finding an alternative when people do
> care about it.

I don't think Junio actually meant an "attitude", but just your angle
of approach (== attitude) on backwards compatibility.

Maybe numbers for this could be generated from the next git user
survey. If numbers justify this change, maybe this or something like
it could be scheduled for a major release of git.

Cheers,
Frans

Re: [PATCH 2/3] completion: remove old code

[Felipe Contreras]

On Mon, Jan 30, 2012 at 1:19 PM, Frans Klaver <fransklaver@gmail.com> wrote:
> On Mon, Jan 30, 2012 at 11:51 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Mon, Jan 30, 2012 at 6:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> OK, maybe some people use it, but most likely they are using an old
>>>> version of git, and thus an old version of the completion script.
>>>
>>> Please adjust your attitude about backward compatibility to match the
>>> standard used for other parts of Git.
>>
>> What attitude?
>
> This attitude:
>
>> I am simply stating a fact. How much percentage of
>> people do you think still have .git/remotes around? How many people do
>> you think have clones more than 3 years old? And how many of these
>> people would complain if remotes were not properly completed for these
>> repos?
>>
>> I doubt anybody would have complained, but I guess we would never
>> know, because I already proposed a solution that would work for them
>> and only uses a *single* line of code, unlike the current 40 ones.
>>
>> I don't see what is the problem with the attitude of sending a patch
>> to remove code that most likely nobody cares about (neither you or I
>> have numbers on this), and then finding an alternative when people do
>> care about it.
>
> I don't think Junio actually meant an "attitude", but just your angle
> of approach (== attitude) on backwards compatibility.

We are not talking about backwards compatibility; we are talking about
compatibility of remotes completion of the bash completion script of
repositories more than 3 years old with remotes that haven't been
migrated.

This barely resembles the git-foo -> 'git foo', which truly broke
backwards compatibility, and at the time I proposed many different
approaches to deal with these type of problems, which seem to be
followed now (although probably not because of my recommendations).

But this has nothing to do with _attitude_; I am merely stating fact.
I have never expressed any opinion or attitude with respect to how
backwards compatibility should be handled in this thread, have I?

> Maybe numbers for this could be generated from the next git user
> survey. If numbers justify this change, maybe this or something like
> it could be scheduled for a major release of git.

Maybe, but I doubt this issue hardly deserves much discussion.

Nobody is proposing to break backwards compatibility--as you can see,
I already proposed a simple solution that should work.

And FTR, when I wrote 'We don't need to check for GIT_DIR/remotes,
right? This was removed long time ago." I clearly wasn't sure if
.git/remotes was still used or not, after Jonathan Nieder replied, I
checked the source code of remotes.c, and I found that it was still
supported, so I wrote the proposed alternative.

-- 
Felipe Contreras

Re: [PATCH 2/3] completion: remove old code

[Frans Klaver]

On Mon, Jan 30, 2012 at 12:55 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:

> We are not talking about backwards compatibility; we are talking about
> compatibility of remotes completion of the bash completion script of
> repositories more than 3 years old with remotes that haven't been
> migrated.

What's not backward about that?


> This barely resembles the git-foo -> 'git foo', which truly broke
> backwards compatibility, and at the time I proposed many different
> approaches to deal with these type of problems, which seem to be
> followed now (although probably not because of my recommendations).
>
> But this has nothing to do with _attitude_; I am merely stating fact.
> I have never expressed any opinion or attitude with respect to how
> backwards compatibility should be handled in this thread, have I?

As far as I know you haven't explicitly said anything about that.
There may still be a possibility that the sentence Junio quoted in his
reply could have implied a certain attitude.

>> Maybe numbers for this could be generated from the next git user
>> survey. If numbers justify this change, maybe this or something like
>> it could be scheduled for a major release of git.
>
> Maybe, but I doubt this issue hardly deserves much discussion.

I wouldn't know about that. Apparently not everybody is happy with
applying it without further discussion.

Cheers,
Frans

Re: [PATCH 2/3] completion: remove old code

[Felipe Contreras]

On Mon, Jan 30, 2012 at 2:21 PM, Frans Klaver <fransklaver@gmail.com> wrote:
> On Mon, Jan 30, 2012 at 12:55 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>
>> We are not talking about backwards compatibility; we are talking about
>> compatibility of remotes completion of the bash completion script of
>> repositories more than 3 years old with remotes that haven't been
>> migrated.
>
> What's not backward about that?

Not all backwards compatibility issues are the same.

>> This barely resembles the git-foo -> 'git foo', which truly broke
>> backwards compatibility, and at the time I proposed many different
>> approaches to deal with these type of problems, which seem to be
>> followed now (although probably not because of my recommendations).
>>
>> But this has nothing to do with _attitude_; I am merely stating fact.
>> I have never expressed any opinion or attitude with respect to how
>> backwards compatibility should be handled in this thread, have I?
>
> As far as I know you haven't explicitly said anything about that.
> There may still be a possibility that the sentence Junio quoted in his
> reply could have implied a certain attitude.

I already asked, but I ask again; what would be that attitude? Not
caring about backwards compatibility? Then that implication would have
been wrong.

If you look a few lines below, you would see a change that doesn't
break backwards compatibility, which proves the previous implication
wrong... Not to mention previous discussions.

>>> Maybe numbers for this could be generated from the next git user
>>> survey. If numbers justify this change, maybe this or something like
>>> it could be scheduled for a major release of git.
>>
>> Maybe, but I doubt this issue hardly deserves much discussion.
>
> I wouldn't know about that. Apparently not everybody is happy with
> applying it without further discussion.

Jonathan Nieder is happy with the 'ls -1 "$d/remotes"' change, and I
haven't seen anybody object it.

Either way. I'm not going to discuss in this thread any more. I'll
resend the patches, feel free to comment there.

-- 
Felipe Contreras

Re: [PATCH 2/3] completion: remove old code

[Jonathan Nieder]

Felipe Contreras wrote:

> Either way. I'm not going to discuss in this thread any more. I'll
> resend the patches, feel free to comment there.

Good idea.  Just for the record, I'm not happy with any patch until
I've seen the code. ;-)

Thanks,
Jonathan

[PATCH 3/3] completion: remove unused code

[Felipe Contreras]

No need for thus rather complicated piece of code :)

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |   30 ------------------------------
 1 files changed, 0 insertions(+), 30 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f7278b5..59a4650 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2730,33 +2730,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.8.3

Re: [PATCH 3/3] completion: remove unused code

[Jonathan Nieder]

Felipe Contreras wrote:

> No need for thus rather complicated piece of code :)
[...]
>  contrib/completion/git-completion.bash |   30 ------------------------------
>  1 files changed, 0 insertions(+), 30 deletions(-)
[...]
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2730,33 +2730,3 @@ if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
[...]
> -if [[ -n ${ZSH_VERSION-} ]]; then
> -	__git_shopt () {
[...]
> -else
> -	__git_shopt () {
> -		shopt "$@"
> -	}
> -fi

What codebase does this apply to?  My copy of git-completion.bash
contains a number of calls to __git_shopt, which will fail after this
change.

By the way, is there any reason you did not cc this series to Gábor or
others who also know the completion code well?  The patches are not
marked with RFC/ so I assume they are intended for direct application,
which seems somewhat odd to me.

Thanks and hope that helps,
Jonathan

Re: [PATCH 3/3] completion: remove unused code

[Jonathan Nieder]

Jonathan Nieder wrote:

> What codebase does this apply to?  My copy of git-completion.bash
> contains a number of calls to __git_shopt

Ah, now I get it.  This would have been easier to understand if
squashed in with patch 2/3.

And it certainly looks like a good change, yes. :)

Thanks for explaining,
Jonathan

Re: [PATCH 3/3] completion: remove unused code

[Felipe Contreras]

On Mon, Jan 30, 2012 at 4:50 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> No need for thus rather complicated piece of code :)
> [...]
>>  contrib/completion/git-completion.bash |   30 ------------------------------
>>  1 files changed, 0 insertions(+), 30 deletions(-)
> [...]
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -2730,33 +2730,3 @@ if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
> [...]
>> -if [[ -n ${ZSH_VERSION-} ]]; then
>> -     __git_shopt () {
> [...]
>> -else
>> -     __git_shopt () {
>> -             shopt "$@"
>> -     }
>> -fi
>
> What codebase does this apply to?  My copy of git-completion.bash
> contains a number of calls to __git_shopt, which will fail after this
> change.

The latest and greatest of course:

http://git.kernel.org/?p=git/git.git;a=blob;f=contrib/completion/git-completion.bash

It's only used in __git_remotes.

> By the way, is there any reason you did not cc this series to Gábor or
> others who also know the completion code well?  The patches are not
> marked with RFC/ so I assume they are intended for direct application,
> which seems somewhat odd to me.

No reason. I hope they read the mailing list, otherwise I'll resend
and CC them. A get_maintainers script, or something like that would
make things easier.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 3/3] completion: remove unused code

[Thomas Rast]

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

> No reason. I hope they read the mailing list, otherwise I'll resend
> and CC them. A get_maintainers script, or something like that would
> make things easier.

I simply use

  git shortlog -sn --no-merges v1.7.0.. -- contrib/completion/

(In many parts the revision limiter can be omitted without losing much,
but e.g. here this drops Shawn who hasn't worked on it since 2009.)

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

Re: [PATCH 3/3] completion: remove unused code

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> wrote:

>Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> No reason. I hope they read the mailing list, otherwise I'll resend
>> and CC them. A get_maintainers script, or something like that would
>> make things easier.
>
>I simply use
>
>  git shortlog -sn --no-merges v1.7.0.. -- contrib/completion/
>
>(In many parts the revision limiter can be omitted without losing much,
>but e.g. here this drops Shawn who hasn't worked on it since 2009.)

Or "--since=1.year", which you can keep using forever without adjusting.

Re: [PATCH 3/3] completion: remove unused code

[Felipe Contreras]

On Mon, Jan 30, 2012 at 10:22 AM, Junio C Hamano <jch2355@gmail.com> wrote:
> Thomas Rast <trast@inf.ethz.ch> wrote:
>>Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> No reason. I hope they read the mailing list, otherwise I'll resend
>>> and CC them. A get_maintainers script, or something like that would
>>> make things easier.
>>
>>I simply use
>>
>>  git shortlog -sn --no-merges v1.7.0.. -- contrib/completion/
>>
>>(In many parts the revision limiter can be omitted without losing much,
>>but e.g. here this drops Shawn who hasn't worked on it since 2009.)
>
> Or "--since=1.year", which you can keep using forever without adjusting.

Perhaps something like that can be stored in a script somewhere in
git's codebase so that people can set sendemail.cccmd to that.

-- 
Felipe Contreras

Re: [PATCH 3/3] completion: remove unused code

[Thomas Rast]

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

> On Mon, Jan 30, 2012 at 10:22 AM, Junio C Hamano <jch2355@gmail.com> wrote:
>> Thomas Rast <trast@inf.ethz.ch> wrote:
>>>Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> No reason. I hope they read the mailing list, otherwise I'll resend
>>>> and CC them. A get_maintainers script, or something like that would
>>>> make things easier.
>>>
>>>I simply use
>>>
>>>  git shortlog -sn --no-merges v1.7.0.. -- contrib/completion/
>>>
>>>(In many parts the revision limiter can be omitted without losing much,
>>>but e.g. here this drops Shawn who hasn't worked on it since 2009.)
>>
>> Or "--since=1.year", which you can keep using forever without adjusting.
>
> Perhaps something like that can be stored in a script somewhere in
> git's codebase so that people can set sendemail.cccmd to that.

Umm, that seems rather AI-complete.  You should always compile the list
by hand.

For example, the list in this case started

      25  SZEDER Gábor
       4  Michael J Gruber
       3  Teemu Matilainen
       3  Thomas Rast

Would you Cc Michael, Teemu and me?  Probably not.  What if it started

       5  SZEDER Gábor
       4  Michael J Gruber
       3  Teemu Matilainen
       3  Thomas Rast

Also, something I didn't mention so far was that you may be patching
squarely into the code of one contributor, even if he only had a single
patch in that area.  To catch this, you should blame the code you are
fixing (you already checked the message of the commit to verify whether
the bug/feature was intentional, right?).  On top of that, the patch may
have involved a large number of people not listed in the Author field.
As a random example,

  $ git shortlog -sn --no-merges v1.7.0..origin/next -- grep.[ch] builtin/grep.[ch]
      15  René Scharfe
       9  Junio C Hamano
       8  Nguyễn Thái Ngọc Duy
       5  Michał Kiedrowicz
       4  Johannes Schindelin
       3  Jeff King
       3  Thomas Rast

but if you were to submit a patch that disputes the case made by
53b8d931, you should probably cc René, Peff and me (see the Helped-by
lines).

Ok, this got rather long-winded.  But I think the bottom line is, trying
to put this in sendemail.cccmd is trying to script common sense.

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

Re: [PATCH 3/3] completion: remove unused code

[Felipe Contreras]

On Mon, Jan 30, 2012 at 3:19 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Mon, Jan 30, 2012 at 10:22 AM, Junio C Hamano <jch2355@gmail.com> wrote:
>>> Thomas Rast <trast@inf.ethz.ch> wrote:
>>>>Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>>
>>>>> No reason. I hope they read the mailing list, otherwise I'll resend
>>>>> and CC them. A get_maintainers script, or something like that would
>>>>> make things easier.
>>>>
>>>>I simply use
>>>>
>>>>  git shortlog -sn --no-merges v1.7.0.. -- contrib/completion/
>>>>
>>>>(In many parts the revision limiter can be omitted without losing much,
>>>>but e.g. here this drops Shawn who hasn't worked on it since 2009.)
>>>
>>> Or "--since=1.year", which you can keep using forever without adjusting.
>>
>> Perhaps something like that can be stored in a script somewhere in
>> git's codebase so that people can set sendemail.cccmd to that.
>
> Umm, that seems rather AI-complete.  You should always compile the list
> by hand.

Why? Take a look at the Linux kernel; having tons of contributors,
many still haven't learned the ropes, and looking at MAINTAIERS, plus
the output of 'git blame', for potentially dozens of patches is too
burdensome, which is why they have 'scripts/get_maintainer.pl' that
does a pretty good job of figuring out who to cc so you don't have to
think about it.

> Ok, this got rather long-winded.  But I think the bottom line is, trying
> to put this in sendemail.cccmd is trying to script common sense.

It's still better than nothing.

I once wrote a much smarter script[1], but it never go into the tree.

The output I get is this:
"Shawn O. Pearce" <spearce@spearce.org>>
"Jonathan Nieder" <jrnieder@gmail.com>
"Mark Lodato" <lodatom@gmail.com>
"Junio C Hamano" <junkio@cox.net>
"Ted Pavlic" <ted@tedpavlic.com>

Note: seems like there's a bug with git blame -P:
% g blame -p -L 2730,+33 contrib/completion/git-completion.bash | grep
author-mail

And if this script is such a bad idea; why do you think sendmail.cccmd exists?

I think we should have a simple script that at least does something
sensible, at least in contrib, but I hope we could even have a
standard git-cccmd that all projects could use.

It looks like my ruby script never had much of a chance getting
anywhere, so would it be accepted in another format? perl? python?
bash?

Cheers.

[1] http://thread.gmane.org/gmane.comp.version-control.git/130391

-- 
Felipe Contreras