patch, 3 partscompletion: trivial cleanups

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(-)

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(-)

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

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.

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

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.

On Mon, Jan 30, 2012 at 7:50 AM, Junio C Hamano <gitster@pobox.com> wrote:

<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" ;}

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.

On Mon, Jan 30, 2012 at 6:34 AM, Junio C Hamano <gitster@pobox.com> wrote:

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 <felipe.contreras@gmail.com> writes:

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.

On Mon, Jan 30, 2012 at 8:07 PM, Junio C Hamano <gitster@pobox.com> wrote:

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.

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

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

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(-)

Hi,

Felipe Contreras wrote:

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.

On Mon, Jan 30, 2012 at 4:36 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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 wrote:

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

Thanks, Jonathan

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

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.

On Mon, Jan 30, 2012 at 6:27 AM, Junio C Hamano <gitster@pobox.com> wrote:

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.

Hi,

On Mon, Jan 30, 2012 at 11:51 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

This attitude:

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

On Mon, Jan 30, 2012 at 1:19 PM, Frans Klaver <fransklaver@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.

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, 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.

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

What's not backward about that?

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 wouldn't know about that. Apparently not everybody is happy with applying it without further discussion.

Cheers, Frans

On Mon, Jan 30, 2012 at 2:21 PM, Frans Klaver <fransklaver@gmail.com> wrote:

Not all backwards compatibility issues are the same.

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.

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 wrote:

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

Thanks, Jonathan

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(-)

Felipe Contreras wrote:

[...]

[...]

[...]

[...]

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

Jonathan Nieder wrote:

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

On Mon, Jan 30, 2012 at 4:50 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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.

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 <felipe.contreras@gmail.com> writes:

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.ethz.ch> wrote:

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

On Mon, Jan 30, 2012 at 10:22 AM, Junio C Hamano <jch2355@gmail.com> wrote:

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 <felipe.contreras@gmail.com> writes:

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

On Mon, Jan 30, 2012 at 3:19 PM, Thomas Rast <trast@inf.ethz.ch> wrote:

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.

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