* Re: Fwd: Gitweb error
From: Jakub Narebski @ 2012-01-30 19:08 UTC (permalink / raw)
To: rajesh boyapati, git
In-Reply-To: <CA+EqV8xB6vcDrqM3EY7uRfu0c7sOj6FbMXci+5w2qgi5RSWrbw@mail.gmail.com>
On Mon, 30 Jan 2012, rajesh boyapati wrote:
> 2012/1/28 Jakub Narebski <jnareb@gmail.com>
>> On Fri, 27 Jan 2012, rajesh boyapati wrote:
>>> my $alternate = 1;
>>> for (my $i = $from; $i <= $to; $i++) {
>>> my $entry = $headlist->[$i];
>>> my %ref = %$entry;
>>> my $curr = $ref{'id'} eq $head;
>>> if ($alternate) {
>>> print "<tr class=\"dark\">\n";
>>> } else {
>>> print "<tr class=\"light\">\n";
>>> }
>>> $alternate ^= 1;
>>
>> Hmmmm... I see that we do not check if $head is defined here before using
>> it. This can happen legitimately if we are on yet to be born orphan branch
>> (so $head, which should be named $head_at, is undefined) but there exist
>> other branches (so $headlist is not empty).
>>
>> But I don't think it is what happened in your case, is it?
tldr; It did happen.
> For my git projects on gerrit, our main branch name is "base".
> We don't have any code on "master" branch.
> May be the $HEAD is looking for master branch(or checked out branch in git
> project).--> In our case, "master" is an empty branch.
> Also, In the git projects, the HEAD file is pointing to "ref:
> refs/heads/master".
> So, I think that's the reason for errors.
>
> How can I make $HEAD to point to a branch other than "master"?.
> a) I can do this by pointing HEAD file in git projects to other branch
[...]
> b) Is there any way, other than doing above step (a) ?.
> I mean I don't want to have a code on "master" branch and also I
> don't want to point HEAD file in git projects to some other branch.
> Do I need to make any modifications to "gitweb.cgi" for this?
Now that I know the source of this error, I can write test case
for it, and fix it. I'll try to do it soon.
So finally what you would need for (b) is just upgrade gitweb.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH v2 3/4] completion: cleanup __gitcomp*
From: Junio C Hamano @ 2012-01-30 19:03 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Felipe Contreras, git, Felipe Contreras, Ted Pavlic,
SZEDER Gábor, Shawn O. Pearce
In-Reply-To: <20120130175004.GG10618@burratino>
Jonathan Nieder <jrnieder@gmail.com> writes:
> I imagine it would have been enough to say something along the lines of
> "The __gitcomp and __gitcomp_nl functions are unnecessarily verbose.
> __gitcomp_nl sets IFS to " \t\n" unnecessarily before setting it to "\n"
> by mistake. Both functions use 'if' statements to read parameters
> with defaults, where the ${parameter:-default} idiom would be just as
> clear. By fixing these, we can make each function almost a one-liner."
>
> By the way, the subject ("clean up __gitcomp*") tells me almost as
> little as something like "fix __gitcomp*". A person reading the
> shortlog would like to know _how_ you are fixing it, or what the
> impact of the change will be --- e.g., something like "simplify
> __gitcomp and __gitcomp_nl" would be clearer.
I love both of the above two paragraphs. Thanks.
> [...]
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
> [...]
>> @@ -524,18 +520,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}"))
>
> This loses the nice name $suffix for the -S argument. Not a problem,
> just noticing.
The patch looks good, including the localness that is kept for IFS.
^ permalink raw reply
* Re: [PATCH v2 4/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-01-30 19:03 UTC (permalink / raw)
To: Felipe Contreras
Cc: Felipe Contreras, git, Lee Marlow, Shawn O. Pearce,
SZEDER Gábor
In-Reply-To: <CAMP44s1SzE1h+4Eoebr2LrnLNgFX1UE2+O8z6yYDuukmaijf7Q@mail.gmail.com>
Felipe Contreras wrote:
> On Mon, Jan 30, 2012 at 8:25 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> : is a no-op command. It does not redirect stderr automatically or
>> do any other magical thing.
>
> Why don't you go ahead and try it?
>
> bash -c ': echo "err" > /dev/stderr'
: is a no-op command. If you have any questions after reading about
it in your manual or online help system of choice, I'll be happy to
answer them.
[...]
> Maybe '[' then.
Honestly, I don't care. :)
(If I had to choose a convention for scripts specific to ksh-style
shells, in order of preference, I would rank them:
1. Always use [[.
2. Use "test", spelled out, like the portable shell code in git does.
3. Use [.
If you have arguments for one convention or another that are
compelling enough that the codebase won't be flipping back and forth
and a patch to go along with them, I imagine no one will mind.)
By the way, since I forget to say enough: thanks for taking care about
this code. Simpler code is definitely a good thing.
Regards,
Jonathan
^ permalink raw reply
* Re: i18n: Avoid sentence puzzles
From: Junio C Hamano @ 2012-01-30 18:57 UTC (permalink / raw)
To: Frederik Schwarzer; +Cc: git
In-Reply-To: <201201301231.21090.schwarzerf@gmail.com>
Frederik Schwarzer <schwarzerf@gmail.com> writes:
> So my question would be: Is it considered worth it to extend the code
> for translators' and translations' sake? If so, I would be glad to
> help with that.
The sample you supplied for 'branch' vs 'remote branch' is a good change
in the logic to choose what translatable message to use, and I do suspect
that there are other instances of problems of this kind.
I am expecting that we may see some activity to the logic in that file
during this cycle, and we would need to be careful not make people step on
other people's toes, though.
^ permalink raw reply
* Re: [PATCH v2 4/4] completion: be nicer with zsh
From: Felipe Contreras @ 2012-01-30 18:56 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Felipe Contreras, git, Lee Marlow, Shawn O. Pearce,
SZEDER Gábor
In-Reply-To: <20120130182547.GA22549@burratino>
On Mon, Jan 30, 2012 at 8:25 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> The commands might fail, that's why '2> /dev/null' was used before,
>> and ':' is used right now.
>
> Wait, what?
>
> : is a no-op command. It does not redirect stderr automatically or
> do any other magical thing.
Why don't you go ahead and try it?
bash -c ': echo "err" > /dev/stderr'
I don't see anything here.
But actually, if I use $(echo "err" > /dev/stderr); _then_ I get
something. Smells a lot like a bug to me.
In any case, if you expected ':' to print errors, now I understand why
you removed 2>/dev/null in eaa4e6e.
> [...]
>> And IMO harder to read. But you are correct that most of the code uses
>> [[]], which I think is a shame. But I guess people want to keep using
>> that.
>
> [[ has simpler syntax wrt quoting and other details. But now that I
> check, the code uses [ a lot, too (which, like "test", is a plain
> built-in command), so I suppose consistency is the only reason to
> prefer one over another. "git log --grep='if \['" tells me the use of
> '[' instead of 'test' here is deliberate.
Maybe '[' then.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] completion: add new zsh completion
From: Junio C Hamano @ 2012-01-30 18:52 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Matthieu Moy, Felipe Contreras, git
In-Reply-To: <4F267AC9.1080407@viscovery.net>
Johannes Sixt <j.sixt@viscovery.net> writes:
>> Something along the lines of
>>
>> ZSH_VERSION='' . $(dirname ${funcsourcetrace[1]%:*})/git-completion.bash
>>
>> should do it (mostly untested, and written by a non-ZSH expert).
>
> Moreover, if zsh is POSIX compliant, the value of ZSH_VERSION will be an
> empty string after this statement. That may or (more likely) may not be
> what you want.
As this split-file is about only zsh completion, it can be as non-POSIX as
it wants, and it appears that zsh is not striving to be POSIX compliant,
judging from Felipe's other messages.
Whatever works for zsh is just fine.
^ permalink raw reply
* Re: Bug: "git checkout -b" should be allowed in empty repo
From: Junio C Hamano @ 2012-01-30 18:48 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
In-Reply-To: <4F263AEE.4080409@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> [3] If commit 0000000 were treated specially, then there would be no
> unborn branches but only branches pointing at the empty commit. In that
> case, my expectation would change--the old branch should be left
> pointing at 0000000. But currently git has no concept of an unborn
> branch that is not HEAD.
And it probably is not a good thing to add such. Under that constraints,
HEAD that says refs/heads/foo where foo does not exist yet needs to be
special cased at places where it matters.
For that matter, even if we artificially created refs/heads/foo before any
commit is made and made it point at 0{40}, you would need to add special
cases to other parts of the system (e.g. "commit" needs to notice that the
result should be a root, not a child of 0{40}; "checkout other_branch"
needs to notice that it should refrain from running the equivalent of
"read-tree -m HEAD other_branch" because HEAD does not point at a real
tree; etc.), so it does not change the fact that the unborn branch is case
is special.
Note that I am not saying that we shouldn't add support for special cases
with special case codepaths.
Perhaps we would need to sprinkle more special case magic like this (this
is for the special case that arises from the same cause)?
builtin/branch.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 7095718..0997e75 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -640,6 +640,13 @@ static int edit_branch_description(const char *branch_name)
struct strbuf buf = STRBUF_INIT;
struct strbuf name = STRBUF_INIT;
+ strbuf_addf(&name, "refs/heads/%s", branch_name);
+ if (!ref_exists(name.buf)) {
+ strbuf_reset(&name);
+ return error("No such branch '%s'.", branch_name);
+ }
+ strbuf_reset(&name);
+
read_branch_desc(&buf, branch_name);
if (!buf.len || buf.buf[buf.len-1] != '\n')
strbuf_addch(&buf, '\n');
^ permalink raw reply related
* Re: [PATCH v2 1/4] completion: simplify __git_remotes
From: Junio C Hamano @ 2012-01-30 18:27 UTC (permalink / raw)
To: Felipe Contreras
Cc: Jonathan Nieder, git, Felipe Contreras, Shawn O. Pearce,
SZEDER Gábor
In-Reply-To: <20120130173446.GF10618@burratino>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Felipe Contreras wrote:
>
>> From: Felipe Contreras <felipe.contreras@gmail.com>
>>
>> There's no need for all that complicated code that requires nullglob,
>> and the complexities related to such option.
>>
>> As an advantage, this would allow us to get rid of __git_shopt, which is
>> used only in this fuction to enable 'nullglob' in zsh.
>
> That is all a longwinded way to say "zsh doesn't support the same
> interface as bash for setting the nullglob option, so let's avoid
> it and use 'ls' which is simpler", right?
;-)
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 1496c6d..086e38d 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"
Yeah, very nice reduction of unnecessary code.
The original loop might have been justifiable if it were doing something
more meaningful inside (e.g. making sure the file really describes a
remote), but as far as I can tell, it merely is a poor-man's emulation of
"ls -1".
You updated it to make the code say what it wanted to say in the way it
should have said from day one ;-).
^ permalink raw reply
* Re: [PATCH v2 4/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-01-30 18:25 UTC (permalink / raw)
To: Felipe Contreras
Cc: Felipe Contreras, git, Lee Marlow, Shawn O. Pearce,
SZEDER Gábor
In-Reply-To: <CAMP44s0ACC+AnwHGtBLe8C1S_sxWj6SbMbawDThvLQAA0pKMYQ@mail.gmail.com>
Felipe Contreras wrote:
> The commands might fail, that's why '2> /dev/null' was used before,
> and ':' is used right now.
Wait, what?
: is a no-op command. It does not redirect stderr automatically or
do any other magical thing.
[...]
> And IMO harder to read. But you are correct that most of the code uses
> [[]], which I think is a shame. But I guess people want to keep using
> that.
[[ has simpler syntax wrt quoting and other details. But now that I
check, the code uses [ a lot, too (which, like "test", is a plain
built-in command), so I suppose consistency is the only reason to
prefer one over another. "git log --grep='if \['" tells me the use of
'[' instead of 'test' here is deliberate.
^ permalink raw reply
* Re: Why does git stash create two commits?
From: Seth Robertson @ 2012-01-30 18:25 UTC (permalink / raw)
To: Phillip Susi; +Cc: git
In-Reply-To: <4F26DF7B.7050109@ubuntu.com>
In message <4F26DF7B.7050109@ubuntu.com>, Phillip Susi writes:
git stash makes two commits, one that has no changes from the base,
and one to stash your changes in. Why?
git-stash can make up to three commits:
working directory (of files git knows about)
index
working directory (of files git does not know about)
The third commit only appears if you use --all or --include-untracked.
Most likely you do not use -u/-a and your index has not been touched
yet, so you only see two commits, of which one is empty.
-Seth Robertson
^ permalink raw reply
* Re: [PATCH 1/3] completion: be nicer with zsh
From: Felipe Contreras @ 2012-01-30 18:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vpqe1cbds.fsf@alter.siamese.dyndns.org>
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
^ permalink raw reply
* Why does git stash create two commits?
From: Phillip Susi @ 2012-01-30 18:20 UTC (permalink / raw)
To: git
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
git stash makes two commits, one that has no changes from the base,
and one to stash your changes in. Why?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQEcBAEBAgAGBQJPJt97AAoJEJrBOlT6nu75G1cH/1qXXMFBPr5vGZbPXCCgG4eG
nmTVlVjxa9Wk52rY/o3NfrgDrftuKIeMdjrFxe/hOsuU37rfMUpWlGa6wtwOIgHL
Cm2s/+PssrOwpajb+v3h+0vqZGNULaxPNpApDjB4JWTxNs86aCV/+zkzoqNjO8y4
OfAZnzqe03BJmPjldvygswD0RMvRvBuHyGRVhTgATcc89yZmihFxCNyekHdVUSvD
zVIUgyUj1vlSRDnSJrtARbKR3JZQeTtN6rMLIOxN0T3iOkltMWFGMoVbJ23kiUyD
LuMSNMrBvvkEU6g6czqQNNyreSut/b1r1ZTt/pvF0qDWUE5CZ9Qhgs6Xz7prtWs=
=Yp0K
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH] merge: use editor by default in interactive sessions
From: Junio C Hamano @ 2012-01-30 18:17 UTC (permalink / raw)
To: Thomas Rast; +Cc: git
In-Reply-To: <87hazdazmb.fsf@thomas.inf.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> I would suggest something like
>
> # Please enter the commit message for your merge commit. Lines starting
> # with '#' will be ignored, and an empty message aborts the commit.
>
> or if you feel comfortable with educating the user in a
> workflow-specific way, even
>
> # Please enter the commit message for your merge commit. You should
> # justify it especially if it merges an updated upstream into a topic
> # branch.
> #
> # Lines starting with '#' will be ignored, and an empty message aborts
> # the commit.
Sounds like a good thing to do.
Please make it so.
^ permalink raw reply
* Re: [PATCH] completion: --edit option for git-merge
From: Junio C Hamano @ 2012-01-30 18:15 UTC (permalink / raw)
To: Adrian Weimann; +Cc: git
In-Reply-To: <1327930670-32396-1-git-send-email-adrian.weimann@googlemail.com>
Thanks.
Once we start suggesting --edit, don't we also need to suggest --no-edit,
which I think is more important for two reasons.
^ permalink raw reply
* Re: Autocompletion - commands no longer work as stand alone
From: Junio C Hamano @ 2012-01-30 18:14 UTC (permalink / raw)
To: Nathan Bullock; +Cc: SZEDER Gábor, git
In-Reply-To: <CAPx=Vfo20wWKOTeeKc=WM9dOpm85enTZHJsTnBL5uh8v0r6PRQ@mail.gmail.com>
Nathan Bullock <nathanbullock@gmail.com> writes:
> ... There was a question if those could be included
> in the main git code base. Do you know if that is likely to happen?
It entirely is up to the author of the patch.
"I have this random code on Github so people can just copy and paste it in
their .bashrc" may be a good starting point to give hint to people who are
interested to come up with a good patch with a use example on a handful of
comment lines and a readable commit log message.
I didn't see it happen in that thread, so perhaps nobody was interested
back then.
^ permalink raw reply
* Re: [PATCH v2 4/4] completion: be nicer with zsh
From: Felipe Contreras @ 2012-01-30 18:10 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Felipe Contreras, git, Lee Marlow, Shawn O. Pearce,
SZEDER Gábor
In-Reply-To: <20120130175324.GH10618@burratino>
On Mon, Jan 30, 2012 at 7:53 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -657,7 +657,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)
>
> Why the new redirect?
It's not new, it was in the original code that your change to the ':'
stuff (eaa4e6e) replaced.
And the reason is explained right above, in the comment:
# 'git merge -s help' (and thus detection of the merge strategy
# list) fails, unfortunately, if run outside of any git working
# tree. __git_merge_strategies is set to the empty string in
# that case, and the detection will be repeated the next time it
# is needed.
The commands might fail, that's why '2> /dev/null' was used before,
and ':' is used right now.
> If I add debugging output to __git_list_merge_strategies that writes to stderr, I want to see it.
Well, you wouldn't see it right now, so that out of scope of this patch.
> Why the 'test "$foo"' form instead of [[ -n which is more common in
> this completion script? Why use "return" instead of
>
> [[ -n $var ]] || var=$(...)
>
> which feels a little simpler?
Because this is _huge_:
[[ "$__git_merge_strategies" ]] ||
__git_merge_strategies=$(__git_list_merge_strategies 2> /dev/null)
And IMO harder to read. But you are correct that most of the code uses
[[]], which I think is a shame. But I guess people want to keep using
that.
So, how about?
[[ "$__git_merge_strategies" ]] && return
__git_merge_strategies=$(__git_list_merge_strategies 2> /dev/null)
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH 1/3] completion: be nicer with zsh
From: Junio C Hamano @ 2012-01-30 18:07 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Junio C Hamano, git
In-Reply-To: <CAMP44s0rp1EwruAwMpntcUzKS=Pbe44t7Eq0OcHdH8WF7OoUhQ@mail.gmail.com>
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.
^ permalink raw reply
* Re: [PATCH v2 4/4] completion: be nicer with zsh
From: Jonathan Nieder @ 2012-01-30 17:53 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Felipe Contreras, Lee Marlow, Shawn O. Pearce,
SZEDER Gábor
In-Reply-To: <1327944197-6379-5-git-send-email-felipec@infradead.org>
Felipe Contreras wrote:
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -657,7 +657,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)
Why the new redirect? If I add debugging output to
__git_list_merge_strategies that writes to stderr, I want to see it.
Why the 'test "$foo"' form instead of [[ -n which is more common in
this completion script? Why use "return" instead of
[[ -n $var ]] || var=$(...)
which feels a little simpler?
^ permalink raw reply
* Re: [PATCH v2 3/4] completion: cleanup __gitcomp*
From: Jonathan Nieder @ 2012-01-30 17:50 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Felipe Contreras, Ted Pavlic, SZEDER Gábor,
Shawn O. Pearce
In-Reply-To: <1327944197-6379-4-git-send-email-felipec@infradead.org>
Felipe Contreras wrote:
> I don't know why there's so much code; these functions don't seem to be
> doing much:
Unless you mean "This patch has had inadequate review and I don't
understand the code I'm patching, so do not trust it", please drop
this commentary or place it after the three dashes.
> * no need to check $#, ${3:-$cur} is much easier
> * __gitcomp_nl doesn't seem to using the initial IFS
>
> This makes the code much simpler.
>
> Eventually it would be nice to wrap everything that touches compgen and
> COMPREPLY in one function for the zsh wrapper.
>
> Comments by Jonathan Nieder.
I don't want this acknowledgement. Who should care that I commented
on something?
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> contrib/completion/git-completion.bash | 20 +++-----------------
> 1 files changed, 3 insertions(+), 17 deletions(-)
This diffstat tells me more of what I wanted to know about the patch
than the description did.
I imagine it would have been enough to say something along the lines of
"The __gitcomp and __gitcomp_nl functions are unnecessarily verbose.
__gitcomp_nl sets IFS to " \t\n" unnecessarily before setting it to "\n"
by mistake. Both functions use 'if' statements to read parameters
with defaults, where the ${parameter:-default} idiom would be just as
clear. By fixing these, we can make each function almost a one-liner."
By the way, the subject ("clean up __gitcomp*") tells me almost as
little as something like "fix __gitcomp*". A person reading the
shortlog would like to know _how_ you are fixing it, or what the
impact of the change will be --- e.g., something like "simplify
__gitcomp and __gitcomp_nl" would be clearer.
[...]
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
[...]
> @@ -524,18 +520,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}"))
This loses the nice name $suffix for the -S argument. Not a problem,
just noticing.
^ permalink raw reply
* [PATCH v2] completion: add new zsh completion
From: Felipe Contreras @ 2012-01-30 17:40 UTC (permalink / raw)
To: git; +Cc: Felipe Contreras
From: Felipe Contreras <felipe.contreras@gmail.com>
It seems there's always issues with zsh's bash completion emulation,
after I took a deep look at the code, I found many issues[1].
So, here is a kind of wrapper that does the same, but properly :)
This would also allow us to make fixes if necessary, since the code is
simple enough, and extend functionality.
[1] http://article.gmane.org/gmane.comp.shells.zsh.devel/24290
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
v2:
* Fix completion of aliased commands
* Add check for wrong commands
* Properly set return codes
* Add default actiooon; for 'git foo -- <tab>'
contrib/completion/git-completion.zsh | 181 +++++++++++++++++++++++++++++++++
1 files changed, 181 insertions(+), 0 deletions(-)
create mode 100644 contrib/completion/git-completion.zsh
diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh
new file mode 100644
index 0000000..bca08f2
--- /dev/null
+++ b/contrib/completion/git-completion.zsh
@@ -0,0 +1,181 @@
+#compdef git gitk
+
+# zsh completion wrapper for git
+#
+# You need git's bash completion script installed somewhere, by default on the
+# same directory as this script.
+#
+# If your script is on ~/.git-completion.sh instead, you can configure it on
+# your ~/.zshrc:
+#
+# zstyle ':completion:*:*:git:*' script ~/.git-completion.sh
+#
+# The recommended way to install this script is to copy to
+# '~/.zsh/completion/_git', and then add the following to your ~/.zshrc file:
+#
+# fpath=(~/.zsh/completion $fpath)
+
+compgen () {
+ local prefix suffix
+ local -a results
+ while getopts "W:P:S:" opt
+ do
+ case $opt in
+ W)
+ results=( ${(Q)~=OPTARG} )
+ ;;
+ P)
+ prefix="$OPTARG"
+ ;;
+ S)
+ suffix="$OPTARG"
+ ;;
+ esac
+ done
+ print -l -r -- "$prefix${^results[@]}$suffix"
+}
+
+complete () {
+ # do nothing
+ return 0
+}
+
+zstyle -s ":completion:$curcontext:" script script
+test -z "$script" && script="$(dirname ${funcsourcetrace[1]%:*})"/git-completion.bash
+ZSH_VERSION='' . "$script"
+
+_get_comp_words_by_ref ()
+{
+ while [ $# -gt 0 ]; do
+ case "$1" in
+ cur)
+ cur=${_words[CURRENT]}
+ ;;
+ prev)
+ cur=${_words[CURRENT-1]}
+ ;;
+ words)
+ words=("${_words[@]}")
+ ;;
+ cword)
+ ((cword = CURRENT - 1))
+ ;;
+ esac
+ shift
+ done
+}
+
+_bash_wrap ()
+{
+ local -a COMPREPLY results _words
+ _words=( $words )
+ () {
+ emulate -L sh
+ setopt KSH_TYPESET
+ setopt kshglob noshglob braceexpand nokshautoload
+ typeset -h words
+ local cur words cword prev
+ _get_comp_words_by_ref -n =: cur words cword prev
+ $1
+ } $1
+ results=( "${^COMPREPLY[@]}" )
+ if [[ -n $results ]]; then
+ local COMP_WORDBREAKS="\"'@><=;|&(:"
+ local i start
+ local cur="${words[CURRENT]}"
+ i=$(expr index "$cur" "$COMP_WORDBREAKS")
+ start="${cur:0:$i}"
+ compadd -Q -S '' -p "$start" -a results && ret=0
+ fi
+ if (( ret )); then
+ _default -S '' && ret=0
+ fi
+}
+
+_gitk ()
+{
+ __git_has_doubledash && return
+
+ local g="$(__gitdir)"
+ local merge=""
+ if [ -f "$g/MERGE_HEAD" ]; then
+ merge="--merge"
+ fi
+ case "$cur" in
+ --*)
+ __gitcomp "
+ $__git_log_common_options
+ $__git_log_gitk_options
+ $merge
+ "
+ return
+ ;;
+ esac
+ __git_complete_revlist
+}
+
+_get_completion_func ()
+{
+ emulate -L sh
+ setopt KSH_TYPESET
+ local command="$1"
+
+ completion_func="_git_${command//-/_}"
+ declare -f $completion_func >/dev/null && return
+
+ local expansion=$(__git_aliased_command "$command")
+ if [ -n "$expansion" ]; then
+ completion_func="_git_${expansion//-/_}"
+ declare -f $completion_func >/dev/null && return
+ fi
+
+ completion_func=
+}
+
+_git ()
+{
+ local ret=1
+
+ if [[ $service != git ]]
+ then
+ _bash_wrap _$service
+ return ret
+ fi
+
+ local curcontext="$curcontext" state state_descr line
+ typeset -A opt_args
+
+ _arguments -C \
+ '(-p --paginate --no-pager)'{-p,--paginate}'[Pipe all output into ''less'']' \
+ '(-p --paginate)--no-pager[Do not pipe git output into a pager]' \
+ '--git-dir=-[Set the path to the repository]: :_directories' \
+ '--bare[Treat the repository as a bare repository]' \
+ '(- :)--version[Prints the git suite version]' \
+ '--exec-path=-[Path to where your core git programs are installed]:: :_directories' \
+ '--html-path[Print the path where git''s HTML documentation is installed]' \
+ '--work-tree=-[Set the path to the working tree]: :_directories' \
+ '--namespace=-[Set the git namespace]: :_directories' \
+ '(- :)--help[Prints the synopsis and a list of the most commonly used commands]' \
+ '(-): :->command' \
+ '(-)*:: :->option-or-argument' && return 0
+
+ case $state in
+ (command)
+ emulate sh -c __git_compute_porcelain_commands
+ local -a porcelain aliases
+ porcelain=( ${=__git_porcelain_commands} )
+ aliases=( $(__git_aliases) )
+ _describe -t porcelain-commands 'porcelain commands' porcelain && ret=0
+ _describe -t aliases 'aliases' aliases && ret=0
+ ;;
+ (option-or-argument)
+ local completion_func
+ _get_completion_func "${words[1]}"
+ test "$completion_func" && _bash_wrap $completion_func
+ ;;
+ esac
+
+ return ret
+}
+
+_git
--
1.7.8
^ permalink raw reply related
* Re: [PATCH v2 1/4] completion: simplify __git_remotes
From: Jonathan Nieder @ 2012-01-30 17:34 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Felipe Contreras, Junio C Hamano, Shawn O. Pearce,
SZEDER Gábor
In-Reply-To: <1327944197-6379-2-git-send-email-felipec@infradead.org>
Felipe Contreras wrote:
> From: Felipe Contreras <felipe.contreras@gmail.com>
>
> There's no need for all that complicated code that requires nullglob,
> and the complexities related to such option.
>
> As an advantage, this would allow us to get rid of __git_shopt, which is
> used only in this fuction to enable 'nullglob' in zsh.
That is all a longwinded way to say "zsh doesn't support the same
interface as bash for setting the nullglob option, so let's avoid
it and use 'ls' which is simpler", right?
There's a potential speed regression involved --- using "ls" involves
an extra fork/exec. I believe you have thought about this and done a
little to mitigate it; could you explain this in the commit message so
future coders know what features of your code need to be preserved?
Please consider squashing this with patch 2/4. It will make both
patches way easier to understand on their own.
Cc-ing Gábor, who I imagine is more familiar with this code than I am.
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> contrib/completion/git-completion.bash | 7 +------
> 1 files changed, 1 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 1496c6d..086e38d 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*/}"
> --
> 1.7.8
>
^ permalink raw reply
* [PATCH v2 2/4] completion: remove unused code
From: Felipe Contreras @ 2012-01-30 17:23 UTC (permalink / raw)
To: git; +Cc: Jonathan Nieder, Felipe Contreras
In-Reply-To: <1327944197-6379-1-git-send-email-felipec@infradead.org>
From: Felipe Contreras <felipe.contreras@gmail.com>
No need for this rather complicated piece of code anymore :)
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 086e38d..4f68f0a 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2728,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.8
^ permalink raw reply related
* [PATCH 0/4] completion: trivial cleanups
From: Felipe Contreras @ 2012-01-30 17:23 UTC (permalink / raw)
To: git; +Cc: Jonathan Nieder, Felipe Contreras
From: Felipe Contreras <felipe.contreras@gmail.com>
And an improvement for zsh.
Felipe Contreras (4):
completion: simplify __git_remotes
completion: remove unused code
completion: cleanup __gitcomp*
completion: be nicer with zsh
contrib/completion/git-completion.bash | 66 +++++---------------------------
1 files changed, 10 insertions(+), 56 deletions(-)
--
1.7.8
^ permalink raw reply
* Re: [PATCH v5 5/5] gitweb: place links to parent directories in page header
From: Jakub Narebski @ 2012-01-30 17:10 UTC (permalink / raw)
To: Bernhard R. Link; +Cc: Junio C Hamano, git
In-Reply-To: <20120130115046.GE9267@server.brlink.eu>
On Mon, 30 Jan 2012, Bernhard R. Link wrote:
> Change html page headers to not only link the project root and the
> currently selected project but also the directories in between using
> project_filter. (Allowing to jump to a list of all projects within
> that intermediate directory directly and making the project_filter
> feature visible to users).
Nice idea, nice description.
> Signed-off-by: Bernhard R. Link <brlink@debian.org>
> ---
> gitweb/gitweb.perl | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index dfc79df..b54ddb9 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3853,7 +3853,10 @@ sub print_nav_breadcrumbs {
>
> print $cgi->a({-href => esc_url($home_link)}, $home_link_str) . " / ";
> if (defined $project) {
> - print $cgi->a({-href => href(action=>"summary")}, esc_html($project));
> + my @dirname = split '/', $project;
> + my $projectbasename = pop @dirname;
> + print_nav_breadcrumbs_path(@dirname);
> + print $cgi->a({-href => href(action=>"summary")}, esc_html($projectbasename));
> if (defined $action) {
> my $action_print = $action ;
> if (defined $opts{-action_extra}) {
> --
Nice code.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH] merge: use editor by default in interactive sessions
From: Thomas Rast @ 2012-01-30 17:06 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <7vipk26p1b.fsf@alter.siamese.dyndns.org>
On Mon, 23 Jan 2012 14:18:40 -0800, Junio C Hamano <gitster@pobox.com> wrote:
> Traditionally, a cleanly resolved merge was committed by "git merge" using
> the auto-generated merge commit log message with invoking the editor.
>
> After 5 years of use in the field, it turns out that people perform too
> many unjustified merges of the upstream history into their topic branches.
> These merges are not just useless, but they are often not explained well,
> and making the end result unreadable when it gets time for merging their
> history back to their upstream.
Ok, so I'm late to the party and perhaps I missed the discussion about
this, but...
I think the proposed commit message should have a comment, just like for
an ordinary commit, that explains why we are showing the user an editor.
(I'm too lazy to check, but I suspect we *always* give a comment about
what is going on when we fire up an editor.)
I would suggest something like
# Please enter the commit message for your merge commit. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
or if you feel comfortable with educating the user in a
workflow-specific way, even
# Please enter the commit message for your merge commit. You should
# justify it especially if it merges an updated upstream into a topic
# branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox