Re: [PATCH] Rename V15_MINGW_HEADERS into CYGWIN_OLD_WINSOCK_HEADERS
Mark Levedahl writes: >> ... >> -V15_MINGW_HEADERS = YesPlease >> +CYGWIN_OLD_WINSOCK_HEADERS = YesPlease >> > WINSOCK is certainly a part of the win32 api implementation, but it is > is the entire win32api that changed, not just the tiny bit dealing > with sockets. > Basically, WINDOWS.h, and everything it includes, and all of the dlls > it touches, and the .o files, changed. > ... So my suggestion in the bike shedding > category is to > > s/V15_MINGW_HEADERS/CYGWIN_V15_WIN32API/ Sounds sensible. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: t5801-remote-helpers.sh fails
On 10.11.12 23:05, Felipe Contreras wrote: > On Sat, Nov 10, 2012 at 8:20 PM, Torsten Bögershausen wrote: >> On 11/10/2012 08:15 PM, Felipe Contreras wrote: >>> >>> Hi, >>> >>> On Sat, Nov 10, 2012 at 2:48 PM, Torsten Bögershausen >>> wrote: >>> on peff/pu t5801 fails, the error is in git-remote-testgit, please see below. That's on my Mac OS X box. I haven't digged further into the test case, but it looks as if "[-+]A make NAMEs associative arrays" is not supported on this version of bash. /Torsten /Users/tb/projects/git/git.peff/git-remote-testgit: line 64: declare: -A: invalid option declare: usage: declare [-afFirtx] [-p] [name[=value] ...] /Users/tb/projects/git/git.peff/git-remote-testgit: line 66: refs/heads/master: division by 0 (error token is "/master") error: fast-export died of signal 13 fatal: Error while running fast-export >>> >>> >>> What is your bash --version? >>> >> bash --version >> GNU bash, version 3.2.48(1)-release (x86_64-apple-darwin10.0) >> Copyright (C) 2007 Free Software Foundation, Inc. > > I see, that version indeed doesn't have associative arrays. > >> On the other hand, Documentation/CodingGuidelines says: >> - No shell arrays. > > Yeah, for shell code I guess, but this is bash code. > >> Could we use perl to have arrays? > > I think the code in perl would be harder to follow, and this is meant > not only as a test, but also as a reference. > > I'm not exactly sure what we should do here: > > a) remove the code (would not be so good as a reference) > b) enable the code conditionally based on the version of bash (harder to read) > c) replace the code without associative arrays (will be much more > complicated and ugly) > d) add a check for the bash version to the top of the test in t/ > > I'm leaning towards d), followed by b). > > If only there was a clean way to do this, so c) would not be so ugly. > > After investigating different optins this seems to be the best: > > join -e empty -o '0 1.2 2.2' -a 2 <(echo "$before") <(echo "$after") > | while read e a b; do > test "$a" == "$b" && continue > echo "changed $e" > done > > But to me seems a bit harder to grasp. Not sure. > > Cheers. > Hi again, I managed to have a working solution for "d) add a check for the bash version to the top of the test in t/" Please see diff below. This unbreaks the test suite here. Is this a good way forward? Filipe, does the code line you mention above work for you? If yes: I can test it here, if you send it as a patch. /Torsten diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh old mode 100644 new mode 100755 index 6e4e078..ea3d0f3 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -13,6 +13,15 @@ compare_refs() { test_cmp expect actual } +cat >"testbashArray" <<-EOF + declare -A assa +EOF + +/bin/bash testbashArray || { + skip_all='t5801. /bin/bash does not handle associative arrays' + test_done +} + test_expect_success 'setup repository' ' git init server && (cd server && -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()
On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote: > __gitcomp_nl () > { > local IFS=$'\n' > - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) > + COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" ' > + BEGIN { > + FS="\n"; > + len=length(cur); > + } > + { > + if (cur == substr($1, 1, len)) > + print pfx$1sfx; > + }' <<< "$1" )) > } This version is simpler and faster: local IFS=$'\n' COMPREPLY=($(awk -v cur="${3-$cur}" -v pre="${2-}" -v suf="${4- }" '$0 ~ cur { print pre$0suf }' <<< "$1" )) == 1 == awk 1: real0m0.067s user0m0.066s sys 0m0.001s awk 2: real0m0.057s user0m0.055s sys 0m0.002s -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()
On Sat, Nov 17, 2012 at 8:28 PM, Felipe Contreras wrote: > On Sat, Nov 17, 2012 at 8:08 PM, Felipe Contreras > wrote: >> On Sat, Nov 17, 2012 at 3:14 PM, SZEDER Gábor wrote: >>> On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote: On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote: > __gitcomp_nl () > { > local IFS=$'\n' > - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- > "${3-$cur}")) > + COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v > cur="${3-$cur}" ' > + BEGIN { > + FS="\n"; > + len=length(cur); > + } > + { > + if (cur == substr($1, 1, len)) > + print pfx$1sfx; > + }' <<< "$1" )) > } Does this really perform better than my alternative? + for x in $1; do + if [[ "$x" = "$3"* ]]; then + COMPREPLY+=("$2$x$4") + fi + done >>> >>> It does: >>> >>> My version: >>> >>> $ refs="$(for i in {0..} ; do echo branch$i ; done)" >>> $ time __gitcomp_nl "$refs" >>> >>> real0m0.109s >>> user0m0.096s >>> sys 0m0.012s >>> >>> Yours: >>> >>> $ time __gitcomp_nl "$refs" >>> >>> real0m0.321s >>> user0m0.312s >>> sys 0m0.008s >> >> Yeah, for 1 refs, which is not the common case: > > And this beats both in every case: > > while read -r x; do > if [[ "$x" == "$3"* ]]; then > COMPREPLY+=("$2$x$4") > fi > done <<< $1 Nevermind that. Here's another: local IFS=$'\n' COMPREPLY=($(printf -- "$2%s$4\n" $1 | grep "^$2$3")) -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 4/5] completion: get rid of compgen
On Sat, Nov 17, 2012 at 8:33 PM, Felipe Contreras wrote: > On Sat, Nov 17, 2012 at 3:12 PM, SZEDER Gábor wrote: >> On Sat, Nov 17, 2012 at 12:42:38PM +0100, Felipe Contreras wrote: >>> On Sat, Nov 17, 2012 at 12:00 PM, SZEDER Gábor wrote: >>> > On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote: >>> >> The functionality we use is very simple, plus, this fixes a known >>> >> breakage 'complete tree filename with metacharacters'. >>> >> >>> >> Signed-off-by: Felipe Contreras >>> >> --- >>> >> contrib/completion/git-completion.bash | 6 +- >>> >> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >> >>> >> diff --git a/contrib/completion/git-completion.bash >>> >> b/contrib/completion/git-completion.bash >>> >> index 975ae13..ad3e1fe 100644 >>> >> --- a/contrib/completion/git-completion.bash >>> >> +++ b/contrib/completion/git-completion.bash >>> >> @@ -227,7 +227,11 @@ fi >>> >> >>> >> __gitcompadd () >>> >> { >>> >> - COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3")) >>> >> + for x in $1; do >>> >> + if [[ "$x" = "$3"* ]]; then >>> >> + COMPREPLY+=("$2$x$4") >>> >> + fi >>> >> + done >>> > >>> > The whole point of creating __gitcomp_nl() back then was to fill >>> > COMPREPLY without iterating through all words in the wordlist, making >>> > completion faster for large number of words, e.g. a lot of refs, or >>> > later a lot of symbols for 'git grep' in a larger project. >>> > >>> > The loop here kills that optimization. >>> >>> So your solution is to move the loop to awk? I fail to see how that >>> could bring more optimization, specially since it includes an extra >>> fork now. >> >> This patch didn't aim for more optimization, but it was definitely a >> goal not to waste what we gained by creating __gitcomp_nl() in >> a31e6262 (completion: optimize refs completion, 2011-10-15). However, >> as it turns out the new version with awk is actually faster than >> current master with compgen: >> >> Before: >> >> $ refs="$(for i in {0..} ; do echo branch$i ; done)" >> $ time __gitcomp_nl "$refs" >> >> real0m0.242s >> user0m0.220s >> sys 0m0.028s >> >> After: >> >> $ time __gitcomp_nl "$refs" >> >> real0m0.109s >> user0m0.096s >> sys 0m0.012s > > This one is even faster: > > while read -r x; do > if [[ "$x" == "$3"* ]]; then > COMPREPLY+=("$2$x$4") > fi > done <<< $1 > > == 1 == > one: > real0m0.148s > user0m0.134s > sys 0m0.025s > two: > real0m0.055s > user0m0.054s > sys 0m0.000s Ah, nevermind, I didn't quote the $1. However, this one is quite close and much simpler: local IFS=$'\n' COMPREPLY=($(printf -- "$2%s$4\n" $1 | grep "^$2$3")) == 1 == awk 1: real0m0.064s user0m0.062s sys 0m0.003s printf: real0m0.069s user0m0.064s sys 0m0.020s -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/7] completion: add tests for invalid variable name among completion words
SZEDER Gábor wrote: > The breakage can > be simply bogus possible completion words, but it can also be a > failure: > > $ git branch '${foo.bar}' > $ git checkout > bash: ${foo.bar}: bad substitution Or arbitrary code execution: $ git branch '$(>kilroy-was-here)' $ git checkout $ ls -l kilroy-was-here -rw-rw-r-- 1 jrn jrn 0 nov 17 15:40 kilroy-was-here The final version of this patch should go to maint. Thanks for catching it. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] completion: add tests for the __gitcomp_nl() completion helper function
SZEDER Gábor wrote: > --- a/t/t9902-completion.sh > +++ b/t/t9902-completion.sh > @@ -155,6 +155,90 @@ test_expect_success '__gitcomp - suffix' ' > test_cmp expected out > ' > > +test_expect_success '__gitcomp_nl - trailing space' ' > + sed -e "s/Z$//" >expected <<-\EOF && '$' is usually a shell metacharacter, so it would be more comfortable to read a version that escapes it: sed -e "s/Z\$//" >expected <<-\EOF Since '$/' is not a valid parameter expansion, if I understand correctly then POSIX leaves the meaning of the quoted string "s/Z$//" undefined (XCU §2.2.3). Luckily every shell I've tried, including bash, keeps the $ unmolested. Other parts of the file already use the same style, so I wouldn't suggest changing it in this patch. Thanks for some nice tests. Reviewed-by: Jonathan Nieder -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] completion: fix args of run_completion() test helper
SZEDER Gábor wrote: > Fix this by passing the command line to run_completion() as separate > words. Good catch. The change makes sense, the code looks saner after the fix, and since this is test code any breakage should be caught quickly, so Reviewed-by: Jonathan Nieder Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] completion: make the 'basic' test more tester-friendly
SZEDER Gábor wrote: > The 'basic' test uses 'grep -q' to filter the resulting possible > completion words while looking for the presence or absence of certain > git commands, and relies on grep's exit status to indicate a failure. [...] > To make testers' life easier provide some output about the failed > condition: store the results of the filtering in a file and compare > its contents to the expected results by the good old test_cmp helper. Looks good. I wonder if this points to the need for a test_grep helper more generally. [...] > run_completion "git f" && > - ! grep -q -v "^f" out > + >expected && > + sed -n "/^[^f]/p" out >out2 && > + test_cmp expected out2 Functional change: before, this would fail if "out" contained a blank line, while afterward it does not. I doubt that matters, though. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Re: [PATCH v3 1/3] git-submodule add: Add -r/--record option
On Sat, Nov 17, 2012 at 10:31:30PM +0100, Heiko Voigt wrote: > On Sat, Nov 17, 2012 at 02:20:27PM -0500, W. Trevor King wrote: > > On Sat, Nov 17, 2012 at 04:30:07PM +0100, Heiko Voigt wrote: > > > > > (2) "git diff [$path]" and friends in the superproject compares the > > > > > HEAD of thecheckout of the submodule at $path with the tip of > > > > > the branch named by submodule.$name.branch in .gitmodules of > > > > > the superproject, instead of the commit that is recorded in the > > > > > index of the superproject. > > > > > > > > > > > > > Hmm. ???git diff??? compares the working tree with the local HEAD > > > > (just a > > > > SHA for submodules), so I don't think it should care about the status > > > > of a remote branch. This sounds like you want something like: > > > > > > > > $ git submodule foreach 'git diff origin/$submodule_branch' > > > > > > > > Perhaps this is enough motivation for keeping $submodule_* exports? > > > > > > > > > and the option were called something like "--follow-branch=$branch", > > > > > ??? > > > > > > I am not sure if hiding changes to the recorded SHA1 from the user is > > > such a useful thing. In the first step I would like it if it was kept > > > simple and only the submodule update machinery learned to follow a > > > branch. If that results in local changes that should be shown. The user > > > is still in charge of recording the updated SHA1 in his commit. > > > > I understand what you're warning against here, or what it has to do > > with "git diff". > > Is there a not missing here? Thanks. I'd meant to say "I don't understand…". > What I am talking about is the suggestion of Junio. Instead of > showing a diff if the SHA1 is different we show a diff if the > checkout in the worktree is different from the tip of the configured > branch. That would hide the fact that a submodule has changed during > a submodule update operation. Ahh, now I understand. I agree that comparing to the remote tip is a bad idea. -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
[PATCH v4.1 5/5] push: update remote tags only with force
References are allowed to update from one commit-ish to another if the former is a ancestor of the latter. This behavior is oriented to branches which are expected to move with commits. Tag references are expected to be static in a repository, though, thus an update to a tag (lightweight and annotated) should be rejected unless the update is forced. To enable this functionality, the following checks have been added to set_ref_status_for_push() for updating refs (i.e, not new or deletion) to restrict fast-forwarding in pushes: 1) The old and new references must be commits. If this fails, it is not a valid update for a branch. 2) The reference name cannot start with "refs/tags/". This catches lightweight tags which (usually) point to commits and therefore would not be caught by (1). If either of these checks fails, then it is flagged (by default) with a status indicating the update is being rejected due to the reference already existing in the remote. This can be overridden by passing --force to git push. Signed-off-by: Chris Rorvick --- Fix C99 comment. Documentation/git-push.txt | 10 +- builtin/push.c | 2 +- builtin/send-pack.c| 5 + cache.h| 3 ++- remote.c | 24 send-pack.c| 1 + t/t5516-fetch-push.sh | 42 +- transport-helper.c | 6 ++ transport.c| 8 ++-- 9 files changed, 87 insertions(+), 14 deletions(-) diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index fe46c42..479e25f 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -51,11 +51,11 @@ be named. If `:` is omitted, the same ref as will be updated. + The object referenced by is used to update the reference -on the remote side, but by default this is only allowed if the -update can fast-forward . By having the optional leading `+`, -you can tell git to update the ref even when the update is not a -fast-forward. This does *not* attempt to merge into . See -EXAMPLES below for details. +on the remote side. By default this is only allowed if the update is +a branch, and then only if it can fast-forward . By having the +optional leading `+`, you can tell git to update the ref even when +the update is not a branch or it is not a fast-forward. This does *not* +attempt to merge into . See EXAMPLES below for details. + `tag ` means the same as `refs/tags/:refs/tags/`. + diff --git a/builtin/push.c b/builtin/push.c index 0e13e11..cd7aa3f 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -222,7 +222,7 @@ static const char message_advice_checkout_pull_push[] = static const char message_advice_ref_already_exists[] = N_("Updates were rejected because a matching reference already exists in\n" - "the remote and the update is not a fast-forward."); + "the remote."); static void advise_pull_before_push(void) { diff --git a/builtin/send-pack.c b/builtin/send-pack.c index fda28bc..1eabf42 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -44,6 +44,11 @@ static void print_helper_status(struct ref *ref) msg = "non-fast forward"; break; + case REF_STATUS_REJECT_ALREADY_EXISTS: + res = "error"; + msg = "already exists"; + break; + case REF_STATUS_REJECT_NODELETE: case REF_STATUS_REMOTE_REJECT: res = "error"; diff --git a/cache.h b/cache.h index f410d94..127e504 100644 --- a/cache.h +++ b/cache.h @@ -1004,13 +1004,14 @@ struct ref { requires_force:1, merge:1, nonfastforward:1, - is_a_tag:1, + forwardable:1, update:1, deletion:1; enum { REF_STATUS_NONE = 0, REF_STATUS_OK, REF_STATUS_REJECT_NONFASTFORWARD, + REF_STATUS_REJECT_ALREADY_EXISTS, REF_STATUS_REJECT_NODELETE, REF_STATUS_UPTODATE, REF_STATUS_REMOTE_REJECT, diff --git a/remote.c b/remote.c index 44e72d6..a723f7a 100644 --- a/remote.c +++ b/remote.c @@ -1311,14 +1311,24 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, * to overwrite it; you would not know what you are losing * otherwise. * -* (3) if both new and old are commit-ish, and new is a -* descendant of old, it is OK. +* (3) if new is commit-ish and old is a commit, new is a +* descendant of old, and the reference is not of the +* refs/tags/ hierarchy it is OK. * * (4) regardless of all of the above, removing :B
Re: Re: Re: [PATCH v3 1/3] git-submodule add: Add -r/--record option
On Sat, Nov 17, 2012 at 02:20:27PM -0500, W. Trevor King wrote: > On Sat, Nov 17, 2012 at 04:30:07PM +0100, Heiko Voigt wrote: > > > > (2) "git diff [$path]" and friends in the superproject compares the > > > > HEAD of thecheckout of the submodule at $path with the tip of > > > > the branch named by submodule.$name.branch in .gitmodules of > > > > the superproject, instead of the commit that is recorded in the > > > > index of the superproject. > > > > > > > > > > Hmm. ???git diff??? compares the working tree with the local HEAD (just a > > > SHA for submodules), so I don't think it should care about the status > > > of a remote branch. This sounds like you want something like: > > > > > > $ git submodule foreach 'git diff origin/$submodule_branch' > > > > > > Perhaps this is enough motivation for keeping $submodule_* exports? > > > > > > > and the option were called something like "--follow-branch=$branch", > > > > ??? > > > > I am not sure if hiding changes to the recorded SHA1 from the user is > > such a useful thing. In the first step I would like it if it was kept > > simple and only the submodule update machinery learned to follow a > > branch. If that results in local changes that should be shown. The user > > is still in charge of recording the updated SHA1 in his commit. > > I understand what you're warning against here, or what it has to do > with "git diff". Is there a not missing here? Reads somehow like that. What I am talking about is the suggestion of Junio. Instead of showing a diff if the SHA1 is different we show a diff if the checkout in the worktree is different from the tip of the configured branch. That would hide the fact that a submodule has changed during a submodule update operation. > > From what I have heard of projects using this: They usually still have > > something that records the SHA1s on a regular basis. Thinking further, > > why not record them in git? We could add an option to update which > > creates such a commit. > > I think it's best to have users craft their own commit messages > explaining why the branch was updated. That said, an auto-generated > hint (a la "git merge") would probably be a useful extra feature. I have the same opinion. Commits should always be created by humans so you have someone to blame/ask why. But I guess there are people that expect this to be automatic. One argument somehow goes along the lines: "I already created a commit in the submodule why do I need to create another one in the superproject? Just follow the HEAD revision!" They think in subversions "submodules" which are merely pointers to other svn repositories without any revision information. I am unsure if its good to support this the same way. Another use case is big projects that have so many submodules that creating superproject commits would create to much maintenance work. They want to have their integration server make those commits. That would already be supported with update checking out the branch tips and the commit is just one extra thing to do by the integration server. So I think it should be fine just to teach update to checkout the configured branch tips (or forward them to their tracking branch tips) and leave the rest to the user. Cheers Heiko -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/6] completion: consolidate test_completion*() tests
On Sat, Nov 17, 2012 at 12:41 AM, Junio C Hamano wrote: > Felipe Contreras writes: > >> No need to have two versions; if a second argument is specified, use >> that, otherwise use stdin. >> >> Signed-off-by: Felipe Contreras >> --- >> t/t9902-completion.sh | 30 +- >> 1 file changed, 13 insertions(+), 17 deletions(-) >> >> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh >> index 204c92a..59cdbfd 100755 >> --- a/t/t9902-completion.sh >> +++ b/t/t9902-completion.sh >> @@ -60,19 +60,15 @@ run_completion () >> # 2: expected completion >> test_completion () >> { >> - test $# -gt 1 && echo "$2" > expected >> + if [ $# -gt 1 ]; then >> + echo "$2" > expected >> + else >> + sed -e 's/Z$//' > expected >> + fi && > > As "$2" could begin with dash, end with \c, etc. that possibly can > be misinterpred by echo, I'd rewrite this as > > printf '%s\n' "$2" >expected > > Otherwise looked fine; thanks. But that was the case before. I would do that in a separate patch. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/5] push: flag updates
If the reference exists on the remote and the the update is not a delete, then mark as an update. This is in preparation for handling tags and branches differently when pushing. Signed-off-by: Chris Rorvick --- cache.h | 1 + remote.c | 18 +++--- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/cache.h b/cache.h index 82dead1..0f53d11 100644 --- a/cache.h +++ b/cache.h @@ -1003,6 +1003,7 @@ struct ref { merge:1, nonfastforward:1, is_a_tag:1, + update:1, deletion:1; enum { REF_STATUS_NONE = 0, diff --git a/remote.c b/remote.c index 186814d..fe89869 100644 --- a/remote.c +++ b/remote.c @@ -1318,15 +1318,19 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, ref->is_a_tag = !prefixcmp(ref->name, "refs/tags/"); - ref->nonfastforward = + ref->update = !ref->deletion && - !is_null_sha1(ref->old_sha1) && - (!has_sha1_file(ref->old_sha1) - || !ref_newer(ref->new_sha1, ref->old_sha1)); + !is_null_sha1(ref->old_sha1); - if (ref->nonfastforward && !ref->force && !force_update) { - ref->status = REF_STATUS_REJECT_NONFASTFORWARD; - continue; + if (ref->update) { + ref->nonfastforward = + !has_sha1_file(ref->old_sha1) + || !ref_newer(ref->new_sha1, ref->old_sha1); + + if (ref->nonfastforward && !ref->force && !force_update) { + ref->status = REF_STATUS_REJECT_NONFASTFORWARD; + continue; + } } } } -- 1.8.0.155.g3a063ad.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 5/5] push: update remote tags only with force
References are allowed to update from one commit-ish to another if the former is a ancestor of the latter. This behavior is oriented to branches which are expected to move with commits. Tag references are expected to be static in a repository, though, thus an update to a tag (lightweight and annotated) should be rejected unless the update is forced. To enable this functionality, the following checks have been added to set_ref_status_for_push() for updating refs (i.e, not new or deletion) to restrict fast-forwarding in pushes: 1) The old and new references must be commits. If this fails, it is not a valid update for a branch. 2) The reference name cannot start with "refs/tags/". This catches lightweight tags which (usually) point to commits and therefore would not be caught by (1). If either of these checks fails, then it is flagged (by default) with a status indicating the update is being rejected due to the reference already existing in the remote. This can be overridden by passing --force to git push. Signed-off-by: Chris Rorvick --- Documentation/git-push.txt | 10 +- builtin/push.c | 2 +- builtin/send-pack.c| 5 + cache.h| 3 ++- remote.c | 23 +++ send-pack.c| 1 + t/t5516-fetch-push.sh | 30 +- transport-helper.c | 6 ++ transport.c| 8 ++-- 9 files changed, 74 insertions(+), 14 deletions(-) diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index fe46c42..479e25f 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -51,11 +51,11 @@ be named. If `:` is omitted, the same ref as will be updated. + The object referenced by is used to update the reference -on the remote side, but by default this is only allowed if the -update can fast-forward . By having the optional leading `+`, -you can tell git to update the ref even when the update is not a -fast-forward. This does *not* attempt to merge into . See -EXAMPLES below for details. +on the remote side. By default this is only allowed if the update is +a branch, and then only if it can fast-forward . By having the +optional leading `+`, you can tell git to update the ref even when +the update is not a branch or it is not a fast-forward. This does *not* +attempt to merge into . See EXAMPLES below for details. + `tag ` means the same as `refs/tags/:refs/tags/`. + diff --git a/builtin/push.c b/builtin/push.c index 0e13e11..cd7aa3f 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -222,7 +222,7 @@ static const char message_advice_checkout_pull_push[] = static const char message_advice_ref_already_exists[] = N_("Updates were rejected because a matching reference already exists in\n" - "the remote and the update is not a fast-forward."); + "the remote."); static void advise_pull_before_push(void) { diff --git a/builtin/send-pack.c b/builtin/send-pack.c index fda28bc..1eabf42 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -44,6 +44,11 @@ static void print_helper_status(struct ref *ref) msg = "non-fast forward"; break; + case REF_STATUS_REJECT_ALREADY_EXISTS: + res = "error"; + msg = "already exists"; + break; + case REF_STATUS_REJECT_NODELETE: case REF_STATUS_REMOTE_REJECT: res = "error"; diff --git a/cache.h b/cache.h index f410d94..127e504 100644 --- a/cache.h +++ b/cache.h @@ -1004,13 +1004,14 @@ struct ref { requires_force:1, merge:1, nonfastforward:1, - is_a_tag:1, + forwardable:1, update:1, deletion:1; enum { REF_STATUS_NONE = 0, REF_STATUS_OK, REF_STATUS_REJECT_NONFASTFORWARD, + REF_STATUS_REJECT_ALREADY_EXISTS, REF_STATUS_REJECT_NODELETE, REF_STATUS_UPTODATE, REF_STATUS_REMOTE_REJECT, diff --git a/remote.c b/remote.c index 1e263b0..4fcd22c 100644 --- a/remote.c +++ b/remote.c @@ -1311,14 +1311,23 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, * to overwrite it; you would not know what you are losing * otherwise. * -* (3) if both new and old are commit-ish, and new is a -* descendant of old, it is OK. +* (3) if new is commit-ish and old is a commit, new is a +* descendant of old, and the reference is not of the +* refs/tags/ hierarchy it is OK. * * (4) regardless of all of the above, removing :B is * alway
[PATCH v4 4/5] push: flag updates that require force
Add a flag for indicating an update to a reference requires force. Currently the nonfastforward flag of a ref is used for this when generating status the status message. A separate flag insulates the status logic from the details of set_ref_status_for_push(). Signed-off-by: Chris Rorvick --- cache.h | 4 +++- remote.c| 11 --- transport.c | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/cache.h b/cache.h index 0f53d11..f410d94 100644 --- a/cache.h +++ b/cache.h @@ -999,7 +999,9 @@ struct ref { unsigned char old_sha1[20]; unsigned char new_sha1[20]; char *symref; - unsigned int force:1, + unsigned int + force:1, + requires_force:1, merge:1, nonfastforward:1, is_a_tag:1, diff --git a/remote.c b/remote.c index fe89869..1e263b0 100644 --- a/remote.c +++ b/remote.c @@ -1285,6 +1285,8 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, struct ref *ref; for (ref = remote_refs; ref; ref = ref->next) { + int force_ref_update = ref->force || force_update; + if (ref->peer_ref) hashcpy(ref->new_sha1, ref->peer_ref->new_sha1); else if (!send_mirror) @@ -1327,9 +1329,12 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, !has_sha1_file(ref->old_sha1) || !ref_newer(ref->new_sha1, ref->old_sha1); - if (ref->nonfastforward && !ref->force && !force_update) { - ref->status = REF_STATUS_REJECT_NONFASTFORWARD; - continue; + if (ref->nonfastforward) { + ref->requires_force = 1; + if (!force_ref_update) { + ref->status = REF_STATUS_REJECT_NONFASTFORWARD; + continue; + } } } } diff --git a/transport.c b/transport.c index 5fcaea8..60a7421 100644 --- a/transport.c +++ b/transport.c @@ -659,7 +659,7 @@ static void print_ok_ref_status(struct ref *ref, int porcelain) const char *msg; strcpy(quickref, status_abbrev(ref->old_sha1)); - if (ref->nonfastforward) { + if (ref->requires_force) { strcat(quickref, "..."); type = '+'; msg = "forced update"; -- 1.8.0.155.g3a063ad.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/5] push: add advice for rejected tag reference
Advising the user to fetch and merge only makes sense if the rejected reference is a branch. If none of the rejections were for branches, tell the user they need to force the update(s). Signed-off-by: Chris Rorvick --- builtin/push.c | 15 +-- cache.h| 1 + remote.c | 2 ++ transport.c| 6 -- transport.h| 5 +++-- 5 files changed, 23 insertions(+), 6 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index eaeaf7e..0e13e11 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -220,6 +220,10 @@ static const char message_advice_checkout_pull_push[] = "(e.g. 'git pull') before pushing again.\n" "See the 'Note about fast-forwards' in 'git push --help' for details."); +static const char message_advice_ref_already_exists[] = + N_("Updates were rejected because a matching reference already exists in\n" + "the remote and the update is not a fast-forward."); + static void advise_pull_before_push(void) { if (!advice_push_non_ff_current || !advice_push_nonfastforward) @@ -241,6 +245,11 @@ static void advise_checkout_pull_push(void) advise(_(message_advice_checkout_pull_push)); } +static void advise_ref_already_exists(void) +{ + advise(_(message_advice_ref_already_exists)); +} + static int push_with_options(struct transport *transport, int flags) { int err; @@ -265,13 +274,15 @@ static int push_with_options(struct transport *transport, int flags) if (!err) return 0; - if (reject_mask & NON_FF_HEAD) { + if (reject_mask & REJECT_NON_FF_HEAD) { advise_pull_before_push(); - } else if (reject_mask & NON_FF_OTHER) { + } else if (reject_mask & REJECT_NON_FF_OTHER) { if (default_matching_used) advise_use_upstream(); else advise_checkout_pull_push(); + } else if (reject_mask & REJECT_ALREADY_EXISTS) { + advise_ref_already_exists(); } return 1; diff --git a/cache.h b/cache.h index dbd8018..82dead1 100644 --- a/cache.h +++ b/cache.h @@ -1002,6 +1002,7 @@ struct ref { unsigned int force:1, merge:1, nonfastforward:1, + is_a_tag:1, deletion:1; enum { REF_STATUS_NONE = 0, diff --git a/remote.c b/remote.c index 04fd9ea..186814d 100644 --- a/remote.c +++ b/remote.c @@ -1316,6 +1316,8 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, * always allowed. */ + ref->is_a_tag = !prefixcmp(ref->name, "refs/tags/"); + ref->nonfastforward = !ref->deletion && !is_null_sha1(ref->old_sha1) && diff --git a/transport.c b/transport.c index ae9fda8..5fcaea8 100644 --- a/transport.c +++ b/transport.c @@ -740,10 +740,12 @@ void transport_print_push_status(const char *dest, struct ref *refs, ref->status != REF_STATUS_OK) n += print_one_push_status(ref, dest, n, porcelain); if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) { + if (!ref->is_a_tag) + *reject_mask |= REJECT_ALREADY_EXISTS; if (!strcmp(head, ref->name)) - *reject_mask |= NON_FF_HEAD; + *reject_mask |= REJECT_NON_FF_HEAD; else - *reject_mask |= NON_FF_OTHER; + *reject_mask |= REJECT_NON_FF_OTHER; } } } diff --git a/transport.h b/transport.h index 1f9699c..7e86352 100644 --- a/transport.h +++ b/transport.h @@ -140,8 +140,9 @@ int transport_set_option(struct transport *transport, const char *name, void transport_set_verbosity(struct transport *transport, int verbosity, int force_progress); -#define NON_FF_HEAD 0x01 -#define NON_FF_OTHER0x02 +#define REJECT_NON_FF_HEAD 0x01 +#define REJECT_NON_FF_OTHER0x02 +#define REJECT_ALREADY_EXISTS 0x04 int transport_push(struct transport *connection, int refspec_nr, const char **refspec, int flags, -- 1.8.0.155.g3a063ad.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 1/5] push: return reject reasons via a mask
Pass all rejection reasons back from transport_push(). The logic is simpler and more flexible with regard to providing useful feedback. Signed-off-by: Chris Rorvick --- builtin/push.c | 13 - builtin/send-pack.c | 4 ++-- transport.c | 17 - transport.h | 9 + 4 files changed, 19 insertions(+), 24 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index db9ba30..eaeaf7e 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -244,7 +244,7 @@ static void advise_checkout_pull_push(void) static int push_with_options(struct transport *transport, int flags) { int err; - int nonfastforward; + unsigned int reject_mask; transport_set_verbosity(transport, verbosity, progress); @@ -257,7 +257,7 @@ static int push_with_options(struct transport *transport, int flags) if (verbosity > 0) fprintf(stderr, _("Pushing to %s\n"), transport->url); err = transport_push(transport, refspec_nr, refspec, flags, -&nonfastforward); +&reject_mask); if (err != 0) error(_("failed to push some refs to '%s'"), transport->url); @@ -265,18 +265,13 @@ static int push_with_options(struct transport *transport, int flags) if (!err) return 0; - switch (nonfastforward) { - default: - break; - case NON_FF_HEAD: + if (reject_mask & NON_FF_HEAD) { advise_pull_before_push(); - break; - case NON_FF_OTHER: + } else if (reject_mask & NON_FF_OTHER) { if (default_matching_used) advise_use_upstream(); else advise_checkout_pull_push(); - break; } return 1; diff --git a/builtin/send-pack.c b/builtin/send-pack.c index d342013..fda28bc 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -85,7 +85,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) int send_all = 0; const char *receivepack = "git-receive-pack"; int flags; - int nonfastforward = 0; + unsigned int reject_mask; int progress = -1; argv++; @@ -223,7 +223,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) ret |= finish_connect(conn); if (!helper_status) - transport_print_push_status(dest, remote_refs, args.verbose, 0, &nonfastforward); + transport_print_push_status(dest, remote_refs, args.verbose, 0, &reject_mask); if (!args.dry_run && remote) { struct ref *ref; diff --git a/transport.c b/transport.c index 9932f40..ae9fda8 100644 --- a/transport.c +++ b/transport.c @@ -714,7 +714,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i } void transport_print_push_status(const char *dest, struct ref *refs, - int verbose, int porcelain, int *nonfastforward) + int verbose, int porcelain, unsigned int *reject_mask) { struct ref *ref; int n = 0; @@ -733,18 +733,17 @@ void transport_print_push_status(const char *dest, struct ref *refs, if (ref->status == REF_STATUS_OK) n += print_one_push_status(ref, dest, n, porcelain); - *nonfastforward = 0; + *reject_mask = 0; for (ref = refs; ref; ref = ref->next) { if (ref->status != REF_STATUS_NONE && ref->status != REF_STATUS_UPTODATE && ref->status != REF_STATUS_OK) n += print_one_push_status(ref, dest, n, porcelain); - if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD && - *nonfastforward != NON_FF_HEAD) { + if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) { if (!strcmp(head, ref->name)) - *nonfastforward = NON_FF_HEAD; + *reject_mask |= NON_FF_HEAD; else - *nonfastforward = NON_FF_OTHER; + *reject_mask |= NON_FF_OTHER; } } } @@ -1031,9 +1030,9 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing) int transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags, - int *nonfastforward) + unsigned int *reject_mask) { - *nonfastforward = 0; + *reject_mask = 0; transport_verify_remote_names(refspec_nr, refspec); if (transport->push) { @@ -1099,7 +1098,7 @@ int transport_push(struct transport *transport, if (!quiet || err) transport_print_push_status(transport->url, remote_refs,
[PATCH v4 0/5] push: update remote tags only with force
This patch set can be divided into two sets: 1. Provide useful advice for rejected tag references. push: return reject reasons via a mask push: add advice for rejected tag reference Recommending a merge to resolve a rejected tag update seems nonsensical since the tag does not come along for the ride. These patches change the advice for rejected tags to suggest using "push -f". 2. Require force when updating tag references, even on a fast-forward. push: flag updates push: flag updates that require force push: update remote tags only with force This is in response to the following thread: http://thread.gmane.org/gmane.comp.version-control.git/208354 This solution prevents fast-forwards if the reference is of the refs/tags/* hierarchy or if the old object is not a commit. These patches contain the following updates since the v3 set: * builtin/push.c: Remove "push --force" suggestion from advice. * remote.c: Only require old object to be a commit to be forwardable. I added the check for object types based comments from Peff in original thread, and I think this implementation is actually what he intended. If the new object is a tag, the operation is not destructive so there is no reason to block it (at least within the scope of these changes) as was done in the previous iteration. * t/t5516-fetch-push.sh: Create separate tests for the lightweight and annotated cases. Do the annotated tests outside of refs/tags/ so that it actually tests different functionality. Chris Rorvick (5): push: return reject reasons via a mask push: add advice for rejected tag reference push: flag updates push: flag updates that require force push: update remote tags only with force Documentation/git-push.txt | 10 +- builtin/push.c | 24 +++- builtin/send-pack.c| 9 +++-- cache.h| 7 ++- remote.c | 46 -- send-pack.c| 1 + t/t5516-fetch-push.sh | 30 +- transport-helper.c | 6 ++ transport.c| 25 +++-- transport.h| 10 ++ 10 files changed, 126 insertions(+), 42 deletions(-) -- 1.8.0.155.g3a063ad.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: `git mv` has ambiguous error message for non-existing target
Patrick Lehner writes: > But just because mv's error essage isnt very good, does that mean git > mv's error message mustn't be better? Did I say the error message from 'mv' was not very good in the message you are responding to (by the way, this is why you should never top-post when you are responding to a message on this list)? I meant to say that the message from 'mv' is good enough, so is the one given by 'git mv'. I wouldn't reject a patch that updates our message to something more informative without looking at it, though. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 4/5] completion: get rid of compgen
On Sat, Nov 17, 2012 at 3:12 PM, SZEDER Gábor wrote: > On Sat, Nov 17, 2012 at 12:42:38PM +0100, Felipe Contreras wrote: >> On Sat, Nov 17, 2012 at 12:00 PM, SZEDER Gábor wrote: >> > On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote: >> >> The functionality we use is very simple, plus, this fixes a known >> >> breakage 'complete tree filename with metacharacters'. >> >> >> >> Signed-off-by: Felipe Contreras >> >> --- >> >> contrib/completion/git-completion.bash | 6 +- >> >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/contrib/completion/git-completion.bash >> >> b/contrib/completion/git-completion.bash >> >> index 975ae13..ad3e1fe 100644 >> >> --- a/contrib/completion/git-completion.bash >> >> +++ b/contrib/completion/git-completion.bash >> >> @@ -227,7 +227,11 @@ fi >> >> >> >> __gitcompadd () >> >> { >> >> - COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3")) >> >> + for x in $1; do >> >> + if [[ "$x" = "$3"* ]]; then >> >> + COMPREPLY+=("$2$x$4") >> >> + fi >> >> + done >> > >> > The whole point of creating __gitcomp_nl() back then was to fill >> > COMPREPLY without iterating through all words in the wordlist, making >> > completion faster for large number of words, e.g. a lot of refs, or >> > later a lot of symbols for 'git grep' in a larger project. >> > >> > The loop here kills that optimization. >> >> So your solution is to move the loop to awk? I fail to see how that >> could bring more optimization, specially since it includes an extra >> fork now. > > This patch didn't aim for more optimization, but it was definitely a > goal not to waste what we gained by creating __gitcomp_nl() in > a31e6262 (completion: optimize refs completion, 2011-10-15). However, > as it turns out the new version with awk is actually faster than > current master with compgen: > > Before: > > $ refs="$(for i in {0..} ; do echo branch$i ; done)" > $ time __gitcomp_nl "$refs" > > real0m0.242s > user0m0.220s > sys 0m0.028s > > After: > > $ time __gitcomp_nl "$refs" > > real0m0.109s > user0m0.096s > sys 0m0.012s This one is even faster: while read -r x; do if [[ "$x" == "$3"* ]]; then COMPREPLY+=("$2$x$4") fi done <<< $1 == 1 == one: real0m0.148s user0m0.134s sys 0m0.025s two: real0m0.055s user0m0.054s sys 0m0.000s -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()
On Sat, Nov 17, 2012 at 8:08 PM, Felipe Contreras wrote: > On Sat, Nov 17, 2012 at 3:14 PM, SZEDER Gábor wrote: >> On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote: >>> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote: >>> >>> > __gitcomp_nl () >>> > { >>> > local IFS=$'\n' >>> > - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- >>> > "${3-$cur}")) >>> > + COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v >>> > cur="${3-$cur}" ' >>> > + BEGIN { >>> > + FS="\n"; >>> > + len=length(cur); >>> > + } >>> > + { >>> > + if (cur == substr($1, 1, len)) >>> > + print pfx$1sfx; >>> > + }' <<< "$1" )) >>> > } >>> >>> Does this really perform better than my alternative? >>> >>> + for x in $1; do >>> + if [[ "$x" = "$3"* ]]; then >>> + COMPREPLY+=("$2$x$4") >>> + fi >>> + done >> >> It does: >> >> My version: >> >> $ refs="$(for i in {0..} ; do echo branch$i ; done)" >> $ time __gitcomp_nl "$refs" >> >> real0m0.109s >> user0m0.096s >> sys 0m0.012s >> >> Yours: >> >> $ time __gitcomp_nl "$refs" >> >> real0m0.321s >> user0m0.312s >> sys 0m0.008s > > Yeah, for 1 refs, which is not the common case: And this beats both in every case: while read -r x; do if [[ "$x" == "$3"* ]]; then COMPREPLY+=("$2$x$4") fi done <<< $1 == 1 == one: real0m0.004s user0m0.003s sys 0m0.000s two: real0m0.000s user0m0.000s sys 0m0.000s == 10 == one: real0m0.005s user0m0.002s sys 0m0.002s two: real0m0.000s user0m0.000s sys 0m0.000s == 100 == one: real0m0.005s user0m0.004s sys 0m0.000s two: real0m0.001s user0m0.000s sys 0m0.000s == 1000 == one: real0m0.010s user0m0.008s sys 0m0.001s two: real0m0.004s user0m0.004s sys 0m0.000s == 1 == one: real0m0.061s user0m0.057s sys 0m0.005s two: real0m0.045s user0m0.044s sys 0m0.000s == 10 == one: real0m0.582s user0m0.575s sys 0m0.022s two: real0m0.487s user0m0.482s sys 0m0.004s == 100 == one: real0m6.305s user0m6.284s sys 0m0.216s two: real0m5.268s user0m5.194s sys 0m0.065s -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [PATCH v3 1/3] git-submodule add: Add -r/--record option
On Sat, Nov 17, 2012 at 04:04:42PM +0100, Heiko Voigt wrote: > > On Sat, Nov 10, 2012 at 01:44:37PM -0500, W. Trevor King wrote: > > > $ git submodule pull-branch > > > > I think "floating submodules" is a misleading name for this feature > > though, since the checkout SHA is explicitly specified. We're just > > making it more convenient to explicitly update the SHA. How about > > "tracking submodules"? > > Until now we have always called this workflow floating submodules. I > imaging since the submodule floats to the newest revision (whatever the > user chooses that to be) instead of staying at the recorded sha1. > > "tracking submodules" sounds strange to me since the term tracked in git > is mainly used in combination with exact recorded history (e.g. tracking > branch). Since it is about *not* checking out the recorded sha1 but > something that can change I think that could cause confusion. > > I think floating is a more unambiguous term and already known on the > list. I had been getting the impression that floating submodules would automatically update without explicit user intervention. After re-reading your initial floating submodules post, it looks like we do match up after the mapping: GitHeiko Trevor - - - update update --checkout update update update --pull So I'll go back to "floating" ;). On Sat, Nov 17, 2012 at 04:30:07PM +0100, Heiko Voigt wrote: > > > (2) "git diff [$path]" and friends in the superproject compares the > > > HEAD of the checkout of the submodule at $path with the tip of > > > the branch named by submodule.$name.branch in .gitmodules of > > > the superproject, instead of the commit that is recorded in the > > > index of the superproject. > > > > > > > Hmm. ???git diff??? compares the working tree with the local HEAD (just a > > SHA for submodules), so I don't think it should care about the status > > of a remote branch. This sounds like you want something like: > > > > $ git submodule foreach 'git diff origin/$submodule_branch' > > > > Perhaps this is enough motivation for keeping $submodule_* exports? > > > > > and the option were called something like "--follow-branch=$branch", > > > ??? > > I am not sure if hiding changes to the recorded SHA1 from the user is > such a useful thing. In the first step I would like it if it was kept > simple and only the submodule update machinery learned to follow a > branch. If that results in local changes that should be shown. The user > is still in charge of recording the updated SHA1 in his commit. I understand what you're warning against here, or what it has to do with "git diff". > From what I have heard of projects using this: They usually still have > something that records the SHA1s on a regular basis. Thinking further, > why not record them in git? We could add an option to update which > creates such a commit. I think it's best to have users craft their own commit messages explaining why the branch was updated. That said, an auto-generated hint (a la "git merge") would probably be a useful extra feature. -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()
On Sat, Nov 17, 2012 at 3:14 PM, SZEDER Gábor wrote: > On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote: >> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote: >> >> > __gitcomp_nl () >> > { >> > local IFS=$'\n' >> > - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- >> > "${3-$cur}")) >> > + COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" >> > ' >> > + BEGIN { >> > + FS="\n"; >> > + len=length(cur); >> > + } >> > + { >> > + if (cur == substr($1, 1, len)) >> > + print pfx$1sfx; >> > + }' <<< "$1" )) >> > } >> >> Does this really perform better than my alternative? >> >> + for x in $1; do >> + if [[ "$x" = "$3"* ]]; then >> + COMPREPLY+=("$2$x$4") >> + fi >> + done > > It does: > > My version: > > $ refs="$(for i in {0..} ; do echo branch$i ; done)" > $ time __gitcomp_nl "$refs" > > real0m0.109s > user0m0.096s > sys 0m0.012s > > Yours: > > $ time __gitcomp_nl "$refs" > > real0m0.321s > user0m0.312s > sys 0m0.008s Yeah, for 1 refs, which is not the common case: == 1 == SZEDER: real0m0.007s user0m0.003s sys 0m0.000s felipec: real0m0.000s user0m0.000s sys 0m0.000s == 10 == SZEDER: real0m0.004s user0m0.003s sys 0m0.001s felipec: real0m0.000s user0m0.000s sys 0m0.000s == 100 == SZEDER: real0m0.005s user0m0.002s sys 0m0.002s felipec: real0m0.002s user0m0.002s sys 0m0.000s == 1000 == SZEDER: real0m0.010s user0m0.008s sys 0m0.001s felipec: real0m0.018s user0m0.017s sys 0m0.001s == 1 == SZEDER: real0m0.062s user0m0.060s sys 0m0.003s felipec: real0m0.175s user0m0.174s sys 0m0.000s == 10 == SZEDER: real0m0.595s user0m0.593s sys 0m0.021s felipec: real0m1.848s user0m1.843s sys 0m0.003s == 100 == SZEDER: real0m6.258s user0m6.241s sys 0m0.215s felipec: real0m18.191s user0m18.115s sys 0m0.045s -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tcsh-completion re-using git-completion.bash
On Sat, Nov 17, 2012 at 6:15 PM, Marc Khouzam wrote: > On Fri, Nov 16, 2012 at 4:56 PM, Felipe Contreras > wrote: >> On Fri, Nov 16, 2012 at 10:20 PM, Junio C Hamano wrote: >> >>> The point is not about the quality of zsh's emulation >>> of (k)sh when it is run under that mode, but is about not having to >>> have that logic in bash-only part in the first place. >> >> As I said, that logic can be moved away _if_ my wrapper is merged. But >> then again, that would cause regressions to existing users. > > Please forgive me as I don't know the background of the efforts for > zsh git-completion or > the syntax for zsh completion, but I thought I'd mention another > approach I tried for tcsh > which may work for zsh. > > I gather that using a wrapper for zsh causes concerns about > backwards-compatibility. I don't see any concerns. > if [[ -n ${ZSH_VERSION-} ]]; then > # replace below by zsh completion commands calling `bash > ${HOME}/.git-completion.bash` > complete git 'p/*/`bash ${HOME}/.git-completion.bash ${COMMAND_LINE}`/' > complete gitk 'p/*/`bash ${HOME}/.git-completion.bash ${COMMAND_LINE}`/' That doesn't work in zsh. It might be possible to do something similar, but it would probably require many more lines. And we can achieve the same by essentially moving the relevant code of my wrapper: --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -23,10 +23,6 @@ #3) Consider changing your PS1 to also show the current branch, # see git-prompt.sh for details. -if [[ -n ${ZSH_VERSION-} ]]; then - autoload -U +X bashcompinit && bashcompinit -fi - case "$COMP_WORDBREAKS" in *:*) : great ;; *) COMP_WORDBREAKS="$COMP_WORDBREAKS:" @@ -2404,6 +2400,32 @@ __gitk_main () __git_complete_revlist } +if [[ -n ${ZSH_VERSION-} ]]; then + emulate -L zsh + + __gitcompadd () + { + compadd -Q -S "$4" -P "${(M)cur#*[=:]}" -p "$2" -- ${=1} && _ret=0 + } + + _git () + { + local _ret=1 + () { + emulate -L ksh + local cur cword prev + cur=${words[CURRENT-1]} + prev=${words[CURRENT-2]} + let cword=CURRENT-1 + __${service}_main + } + let _ret && _default -S '' && _ret=0 + return _ret + } + compdef _git git gitk + return +fi + __git_func_wrap () { if [[ -n ${ZSH_VERSION-} ]]; then -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tcsh-completion re-using git-completion.bash
On Fri, Nov 16, 2012 at 4:56 PM, Felipe Contreras wrote: > On Fri, Nov 16, 2012 at 10:20 PM, Junio C Hamano wrote: > >> The point is not about the quality of zsh's emulation >> of (k)sh when it is run under that mode, but is about not having to >> have that logic in bash-only part in the first place. > > As I said, that logic can be moved away _if_ my wrapper is merged. But > then again, that would cause regressions to existing users. Please forgive me as I don't know the background of the efforts for zsh git-completion or the syntax for zsh completion, but I thought I'd mention another approach I tried for tcsh which may work for zsh. I gather that using a wrapper for zsh causes concerns about backwards-compatibility. So, what could be done is have the bash script do both jobs: setup the zsh completion commands, and output the git completion using bash itself. At the top of git-completion.bash (or it could be even pushed at the bottom using if/else) we could use: if [[ -n ${ZSH_VERSION-} ]]; then # replace below by zsh completion commands calling `bash ${HOME}/.git-completion.bash` complete git 'p/*/`bash ${HOME}/.git-completion.bash ${COMMAND_LINE}`/' complete gitk 'p/*/`bash ${HOME}/.git-completion.bash ${COMMAND_LINE}`/' exit fi That way the zsh user would still simply do 'source ~/.git-completion.bash' which would only execute the two zsh completion setup commands. Then, when completion is triggered, it calls `bash ${HOME}/.git-completion.bash ${COMMAND_LINE}` and processes the output like tcsh does. This limits the zsh-specific code to 2 lines for the entire script. I got this to work for tcsh (solution (B)) adding the following a the top of git-completion.bash: test "$tcsh" != "" && \ complete git 'p,*,`${HOME}/.git-completion.sh "${COMMAND_LINE}"|\sort|\uniq`,' && \ complete gitk 'p,*,`${HOME}/.git-completion.sh "${COMMAND_LINE}"|\sort|\uniq`,' && \ exit but I didn't think people would go for that since those lines have to work in both bash and tcsh syntax. I thought this made the script a bit brittle. Just a thought. Marc -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: using multiple version of git simultaneously
On Sat, Nov 17, 2012 at 02:50:31PM +, Tomas Carnecky wrote: > On Sat, 17 Nov 2012 20:25:21 +0600, arif wrote: > > I'm trying to use different version of git simultaneously. So how can i > > append some suffix (like "--program-suffix=git1.8) so that i can > > distinguish between different versions. > > Install each version into its own prefix (~/git/1.8.0/, ~/git/1.7.0/ etc). Once you have done that, you can also symlink the binary from each into your regular PATH (e.g., ln -s ~/git/1.8.0/bin/git ~/bin/git.v1.8) to make it easy to switch between them. The installed exec-path is baked in at compile-time, so it finds the correct git sub-programs properly. I keep a couple dozen built versions of git around like this for quick regression testing of bugs we see on the list. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [PATCH v3 1/3] git-submodule add: Add -r/--record option
Hi, On Sun, Nov 11, 2012 at 10:00:48AM -0500, W. Trevor King wrote: > On Sun, Nov 11, 2012 at 02:33:45AM -0800, Junio C Hamano wrote: > In order to avoid losing (or creating) local-only submodule commits, > I'll probably bail (with an error) on non-fast-forward pulls. Can > anyone else think of other safety concerns? That sounds like a good thing to do. We can allow more flexibility later if people come up with usecases. > This means that I'll probably drop Phil's $submodule_* export in v4, > because the only explicit use we have for it is this branch tracking. > I still think it is a useful idea, but it may not be useful enough to > be worth the complexity. Yes lets concentrate on the branch following first. > > (2) "git diff [$path]" and friends in the superproject compares the > > HEAD of the checkout of the submodule at $path with the tip of > > the branch named by submodule.$name.branch in .gitmodules of > > the superproject, instead of the commit that is recorded in the > > index of the superproject. > > > > Hmm. ???git diff??? compares the working tree with the local HEAD (just a > SHA for submodules), so I don't think it should care about the status > of a remote branch. This sounds like you want something like: > > $ git submodule foreach 'git diff origin/$submodule_branch' > > Perhaps this is enough motivation for keeping $submodule_* exports? > > > and the option were called something like "--follow-branch=$branch", > > ??? I am not sure if hiding changes to the recorded SHA1 from the user is such a useful thing. In the first step I would like it if it was kept simple and only the submodule update machinery learned to follow a branch. If that results in local changes that should be shown. The user is still in charge of recording the updated SHA1 in his commit. >From what I have heard of projects using this: They usually still have something that records the SHA1s on a regular basis. Thinking further, why not record them in git? We could add an option to update which creates such a commit. Since git is all about changes I am hesitant to hide them from the user. > I'll replace -r/--record with --follow-branch in v4. Sounds good. Cheers Heiko -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [PATCH v3 1/3] git-submodule add: Add -r/--record option
Hi, sorry for the late reply but my git time is limited. On Sat, Nov 10, 2012 at 02:02:32PM -0500, W. Trevor King wrote: > On Fri, Nov 09, 2012 at 05:29:27PM +0100, Heiko Voigt wrote: > > I think we should agree on a behavior for this option and implement it > > the same time when add learns about it. When we were discussing floating > > submodules as an important option for the gerrit people I already started > > to implement a proof of concept. Please have a look here: > > > > https://github.com/hvoigt/git/commits/hv/floating_submodules > > After skimming through this, something like > > $ git submodule update --pull > > would probably be better than introducing a new command: Yeah along the lines of that, but one thing to keep in mind: We already have --rebase and --merge which do slightly different things (I think). Adding --pull here should behave similar to them. Like fetch and merge is the same to pull without submodules. If I am understanding your goal correctly your --pull would be different. On the other hand: A --pull makes no sense if we apply it to the existing --merge option since it merges the recorded sha1 into the current HEAD. Just a fetch would not really make a difference. Thinking along the existing options I would probably still expect --pull to merge something into the current HEAD. So maybe we have to iron out where this command/option should go. But changing that once we have a patch to discuss should not be that much work. So please proceed with --pull and once we know exactly what it does we can polish that. > On Sat, Nov 10, 2012 at 01:44:37PM -0500, W. Trevor King wrote: > > $ git submodule pull-branch > > I think "floating submodules" is a misleading name for this feature > though, since the checkout SHA is explicitly specified. We're just > making it more convenient to explicitly update the SHA. How about > "tracking submodules"? Until now we have always called this workflow floating submodules. I imaging since the submodule floats to the newest revision (whatever the user chooses that to be) instead of staying at the recorded sha1. "tracking submodules" sounds strange to me since the term tracked in git is mainly used in combination with exact recorded history (e.g. tracking branch). Since it is about *not* checking out the recorded sha1 but something that can change I think that could cause confusion. I think floating is a more unambiguous term and already known on the list. Cheers Heiko -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: using multiple version of git simultaneously
On Sat, 17 Nov 2012 20:25:21 +0600, arif wrote: > I'm trying to use different version of git simultaneously. So how can i > append some suffix (like "--program-suffix=git1.8) so that i can > distinguish between different versions. Install each version into its own prefix (~/git/1.8.0/, ~/git/1.7.0/ etc). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Rename V15_MINGW_HEADERS into CYGWIN_OLD_WINSOCK_HEADERS
On 11/17/2012 02:09 AM, Torsten Bögershausen wrote: See commit 380a4d927bff693c42fc6b22c3547bdcaac4bdc3: "Update cygwin.c for new mingw-64 win32 api headers" Cygwin up to 1.7.16 uses some header file from the WINE project Cygwin 1.7.17 uses some header file from the mingw-64 project As the old cygwin (like 1.5) never used mingw, the name V15_MINGW_HEADERS is confusing. Rename it into CYGWIN_OLD_WINSOCK_HEADERS diff --git a/Makefile b/Makefile index c3edf8c..c2ea735 100644 --- a/Makefile +++ b/Makefile @@ -1089,7 +1089,7 @@ ifeq ($(uname_O),Cygwin) NO_SYMLINK_HEAD = YesPlease NO_IPV6 = YesPlease OLD_ICONV = UnfortunatelyYes - V15_MINGW_HEADERS = YesPlease + CYGWIN_OLD_WINSOCK_HEADERS = YesPlease WINSOCK is certainly a part of the win32 api implementation, but it is is the entire win32api that changed, not just the tiny bit dealing with sockets. Basically, WINDOWS.h, and everything it includes, and all of the dlls it touches, and the .o files, changed. Calling it "OLD" is not helpful, what happens in the future with the next change? The only version info we really have is the dll version. We are switching between the win32 api implementation shipped with cygwin dll version 1.5.x and the one that is now current. And, the shift is not tied to any particular cygwin 1.7.x dll version either (there are no cross dependencies between the win32api implementation and any particular dll in the 1.7.x series, just a coincidence in time as to what packages got updated when). So my suggestion in the bike shedding category is to s/V15_MINGW_HEADERS/CYGWIN_V15_WIN32API/ /end of bike shedding. If this is really worth a second patch, I'll be happy to send one :^) Mark -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
using multiple version of git simultaneously
Hi, I'm trying to use different version of git simultaneously. So how can i append some suffix (like "--program-suffix=git1.8) so that i can distinguish between different versions. -- Cheers arif -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tcsh-completion re-using git-completion.bash
On Sat, Nov 17, 2012 at 12:46:27PM +0100, Felipe Contreras wrote: > On Sat, Nov 17, 2012 at 11:56 AM, SZEDER Gábor wrote: > > On Fri, Nov 16, 2012 at 10:46:16PM +0100, Felipe Contreras wrote: > > >> But even in that case, if push comes to shoves, this zsh wrapper can > >> ultimately read COMPREPLY and figure things backwards, as even more > >> previous versions did: > >> > >> http://article.gmane.org/gmane.comp.version-control.git/189310 > > > > Even better. I was just going to propose that zsh's completion could > > just read the contents of COMPREPLY at the end of _git() and _gitk(), > > because this way no zsh-induced helper functions and changes would be > > needed to the completion script at all. > > I would rather modify the __gitcomp function. Parsing COMPREPLY is too > cumbersome. Each element of COMPREPLY contains a possible completion word. What parsing is needed to use that, that is so cumbersome? > > However, running the completion script with Bash would also prevent > > possible issues caused by incompatibilities between the two shells > > mentioned below. > > It could, but it doesn't now. > > >> >> This is the equivalent of what Marc is doing, except that zsh has no > >> >> problems running bash's code. Note there's a difference with zsh's > >> >> emulation bash (or rather bourne shell, or k shell), and zsh's > >> >> emulation of bash's _completion_. The former is fine, the later is > >> >> not. > >> > > >> > There are a couple of constructs supported by Bash but not by zsh, > >> > which we usually try to avoid. > >> > >> Yes, and is that a big deal? > > > > Not that big, but I wanted to point out that it's not "fine" either. > > Just a slight maintenance burden, because we have to pay attention not > > to use such constructs. > > Do we have to pay attention? Unless you don't mind possible breakages of zsh completion, yes. > I say when we encounter one of such maintenance burden issues _then_ > we think about it. In the meantime for all we know sourcing bash's > script from zsh is fine. That's a cool argument, will remember it when it again comes to refactoring the __gitcomp() tests. For now those tests work just fine. When we encounter maintenance burden issues, like fixing a bug requiring the same modification to all of those tests, then we'll think about it. ;) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()
On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote: > On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote: > > > __gitcomp_nl () > > { > > local IFS=$'\n' > > - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) > > + COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" ' > > + BEGIN { > > + FS="\n"; > > + len=length(cur); > > + } > > + { > > + if (cur == substr($1, 1, len)) > > + print pfx$1sfx; > > + }' <<< "$1" )) > > } > > Does this really perform better than my alternative? > > + for x in $1; do > + if [[ "$x" = "$3"* ]]; then > + COMPREPLY+=("$2$x$4") > + fi > + done It does: My version: $ refs="$(for i in {0..} ; do echo branch$i ; done)" $ time __gitcomp_nl "$refs" real0m0.109s user0m0.096s sys 0m0.012s Yours: $ time __gitcomp_nl "$refs" real0m0.321s user0m0.312s sys 0m0.008s -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 5/5] completion: refactor __gitcomp_1
On Sat, Nov 17, 2012 at 12:27:40PM +0100, Felipe Contreras wrote: > On Sat, Nov 17, 2012 at 11:58 AM, SZEDER Gábor wrote: > > >> # The following function is based on code from: > >> @@ -249,10 +246,16 @@ __gitcomp () > >> --*=) > >> ;; > >> *) > >> - local IFS=$'\n' > >> - __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" > >> "$cur_" "" > >> + local c IFS=$' \t\n' > >> + for c in ${1-}; do > >> + c=`__gitcomp_1 "$c${4-}"` > > > > 1. Backticks. > > 2. A subshell for every word in the wordlist? > > Fine, lets make it hard for zsh then: No, it's about keeping it usable. With this change offering the approximately 170 commands for 'git help ' would take more than 4 seconds on Windows. > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -56,19 +56,6 @@ __gitdir () > fi > } > > -__gitcomp_1 () > -{ > - local c IFS=$' \t\n' > - for c in $1; do > - c="$c$2" > - case $c in > - --*=*|*.) ;; > - *) c="$c " ;; > - esac > - printf '%s\n' "$c" > - done > -} > - > # The following function is based on code from: > # > # bash_completion - programmable completion functions for bash 3.2+ > @@ -241,12 +228,22 @@ __gitcomp () > COMPREPLY=() > ;; > *) > - local IFS=$'\n' > - COMPREPLY=($(compgen -P "${2-}" \ > - -W "$(__gitcomp_1 "${1-}" "${4-}")" \ > - -- "$cur_")) > + local c i IFS=$' \t\n' > + i=0 > + for c in ${1-}; do > + c="$c${4-}" > + case $c in > + --*=*|*.) ;; > + *) c="$c " ;; > + esac > + if [[ "$c" = "$cur_"* ]]; then > + (( i++ )) > + COMPREPLY[$i]="${2-}$c" > + fi > + done > ;; > esac > + > } > > # Generates completion reply with compgen from newline-separated possible > > -- > Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 4/5] completion: get rid of compgen
On Sat, Nov 17, 2012 at 12:42:38PM +0100, Felipe Contreras wrote: > On Sat, Nov 17, 2012 at 12:00 PM, SZEDER Gábor wrote: > > On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote: > >> The functionality we use is very simple, plus, this fixes a known > >> breakage 'complete tree filename with metacharacters'. > >> > >> Signed-off-by: Felipe Contreras > >> --- > >> contrib/completion/git-completion.bash | 6 +- > >> 1 file changed, 5 insertions(+), 1 deletion(-) > >> > >> diff --git a/contrib/completion/git-completion.bash > >> b/contrib/completion/git-completion.bash > >> index 975ae13..ad3e1fe 100644 > >> --- a/contrib/completion/git-completion.bash > >> +++ b/contrib/completion/git-completion.bash > >> @@ -227,7 +227,11 @@ fi > >> > >> __gitcompadd () > >> { > >> - COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3")) > >> + for x in $1; do > >> + if [[ "$x" = "$3"* ]]; then > >> + COMPREPLY+=("$2$x$4") > >> + fi > >> + done > > > > The whole point of creating __gitcomp_nl() back then was to fill > > COMPREPLY without iterating through all words in the wordlist, making > > completion faster for large number of words, e.g. a lot of refs, or > > later a lot of symbols for 'git grep' in a larger project. > > > > The loop here kills that optimization. > > So your solution is to move the loop to awk? I fail to see how that > could bring more optimization, specially since it includes an extra > fork now. This patch didn't aim for more optimization, but it was definitely a goal not to waste what we gained by creating __gitcomp_nl() in a31e6262 (completion: optimize refs completion, 2011-10-15). However, as it turns out the new version with awk is actually faster than current master with compgen: Before: $ refs="$(for i in {0..} ; do echo branch$i ; done)" $ time __gitcomp_nl "$refs" real0m0.242s user0m0.220s sys 0m0.028s After: $ time __gitcomp_nl "$refs" real0m0.109s user0m0.096s sys 0m0.012s -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] completion: fix expansion issue in __gitcomp()
On Sat, Nov 17, 2012 at 12:39:24PM +0100, Felipe Contreras wrote: > On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote: > > > -# Generates completion reply with compgen, appending a space to possible > > -# completion words, if necessary. > > +# Generates completion reply for the word in $cur, appending a space to > > +# possible completion words, if necessary. > > # It accepts 1 to 4 arguments: > > # 1: List of possible completion words. > > # 2: A prefix to be added to each possible completion word (optional). > > -# 3: Generate possible completion matches for this word (optional). > > +# 3: Generate possible completion matches for this word instead of $cur > > +#(optional). > > # 4: A suffix to be appended to each possible completion word (optional). > > __gitcomp () > > { > > @@ -241,10 +242,22 @@ __gitcomp () > > COMPREPLY=() > > ;; > > *) > > - local IFS=$'\n' > > - COMPREPLY=($(compgen -P "${2-}" \ > > - -W "$(__gitcomp_1 "${1-}" "${4-}")" \ > > - -- "$cur_")) > > + local i=0 c IFS=$' \t\n' > > + for c in $1; do > > + case $c in > > + "$cur_"*) > > + c="$c${4-}" > > + case $c in > > + --*=*|*.) ;; > > + *) c="$c " ;; > > + esac > > + COMPREPLY[$i]="${2-}$c" > > + i=$((++i)) > > + ;; > > + *) > > + ;; > > + esac > > + done > > This is not quite the same as before, is it? Before the suffix would > be taken into consideration for the comparison with $cur_, but not any > more. That's a good catch, thanks. I remember it puzzled me that the suffix is considered in the comparison (and that a trailing space would be appended even after a given suffix, too, so there seems to be no way to disable the trailing space). However, currently it doesn't make a difference for us, because afaics we never specify a suffix for __gitcomp(). There were two instances in _git_config() where we specified "." as suffix, but those two were converted to __gitcomp_nl(). I changed those callsites back to __gitcomp() and tried to provoke wrong behavior with the above patch, but couldn't. Anyway, it's better to err on the safe side, so here's the fixup. -- >8 -- Subject: [PATCH] fixup! completion: fix expansion issue in __gitcomp() --- contrib/completion/git-completion.bash | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index a1bf732f..29818fb5 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -231,9 +231,9 @@ __gitcomp () *) local i=0 c IFS=$' \t\n' for c in $1; do + c="$c${4-}" case $c in "$cur_"*) - c="$c${4-}" case $c in --*=*|*.) ;; *) c="$c " ;; -- 1.8.0.220.g4d14ece -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
git-credential-gnome-keyring fails at multilib
Downstream bug report: https://bugs.gentoo.org/443634 The git-credential-gnome-keyring Makefile doesn't allow overriding its variables, making for spectacular link failure if you use CFLAGS for aught but decoration. gcc -g -O2 -Wall -I/usr/include/gnome-keyring-1 -I/usr/include/glib-2.0 -I/usr/lib32/glib-2.0/include -o git-credential-gnome-keyring.o -c git-credential-gnome-keyring.c gcc -o git-credential-gnome-keyring -Wl,--as-needed -Wl,--hash-style=gnu -m32 git-credential-gnome-keyring.o -lgnome-keyring -lglib-2.0 /usr/lib/gcc/x86_64-pc-linux-gnu/4.7.2/../../../../x86_64-pc-linux-gnu/bin/ld: i386:x86-64 architecture of input file `git-credential-gnome-keyring.o' is incompatible with i386 output /usr/lib/gcc/x86_64-pc-linux-gnu/4.7.2/../../../../x86_64-pc-linux-gnu/bin/ld: git-credential-gnome-keyring.o: file class ELFCLASS64 incompatible with ELFCLASS32 /usr/lib/gcc/x86_64-pc-linux-gnu/4.7.2/../../../../x86_64-pc-linux-gnu/bin/ld: final link failed: File in wrong format collect2: error: ld returned 1 exit status Attached patch fixes it. /Peter git-1.8.0-gnome-keyring-multilib.patch Description: Binary data
Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()
On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote: > __gitcomp_nl () > { > local IFS=$'\n' > - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) > + COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" ' > + BEGIN { > + FS="\n"; > + len=length(cur); > + } > + { > + if (cur == substr($1, 1, len)) > + print pfx$1sfx; > + }' <<< "$1" )) > } Does this really perform better than my alternative? + for x in $1; do + if [[ "$x" = "$3"* ]]; then + COMPREPLY+=("$2$x$4") + fi + done -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tcsh-completion re-using git-completion.bash
On Sat, Nov 17, 2012 at 11:56 AM, SZEDER Gábor wrote: > On Fri, Nov 16, 2012 at 10:46:16PM +0100, Felipe Contreras wrote: >> But even in that case, if push comes to shoves, this zsh wrapper can >> ultimately read COMPREPLY and figure things backwards, as even more >> previous versions did: >> >> http://article.gmane.org/gmane.comp.version-control.git/189310 > > Even better. I was just going to propose that zsh's completion could > just read the contents of COMPREPLY at the end of _git() and _gitk(), > because this way no zsh-induced helper functions and changes would be > needed to the completion script at all. I would rather modify the __gitcomp function. Parsing COMPREPLY is too cumbersome. > However, running the completion script with Bash would also prevent > possible issues caused by incompatibilities between the two shells > mentioned below. It could, but it doesn't now. >> >> This is the equivalent of what Marc is doing, except that zsh has no >> >> problems running bash's code. Note there's a difference with zsh's >> >> emulation bash (or rather bourne shell, or k shell), and zsh's >> >> emulation of bash's _completion_. The former is fine, the later is >> >> not. >> > >> > There are a couple of constructs supported by Bash but not by zsh, >> > which we usually try to avoid. >> >> Yes, and is that a big deal? > > Not that big, but I wanted to point out that it's not "fine" either. > Just a slight maintenance burden, because we have to pay attention not > to use such constructs. Do we have to pay attention? I say when we encounter one of such maintenance burden issues _then_ we think about it. In the meantime for all we know sourcing bash's script from zsh is fine. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 4/5] completion: get rid of compgen
On Sat, Nov 17, 2012 at 12:00 PM, SZEDER Gábor wrote: > On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote: >> The functionality we use is very simple, plus, this fixes a known >> breakage 'complete tree filename with metacharacters'. >> >> Signed-off-by: Felipe Contreras >> --- >> contrib/completion/git-completion.bash | 6 +- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/contrib/completion/git-completion.bash >> b/contrib/completion/git-completion.bash >> index 975ae13..ad3e1fe 100644 >> --- a/contrib/completion/git-completion.bash >> +++ b/contrib/completion/git-completion.bash >> @@ -227,7 +227,11 @@ fi >> >> __gitcompadd () >> { >> - COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3")) >> + for x in $1; do >> + if [[ "$x" = "$3"* ]]; then >> + COMPREPLY+=("$2$x$4") >> + fi >> + done > > The whole point of creating __gitcomp_nl() back then was to fill > COMPREPLY without iterating through all words in the wordlist, making > completion faster for large number of words, e.g. a lot of refs, or > later a lot of symbols for 'git grep' in a larger project. > > The loop here kills that optimization. So your solution is to move the loop to awk? I fail to see how that could bring more optimization, specially since it includes an extra fork now. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] completion: fix expansion issue in __gitcomp()
On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor wrote: > -# Generates completion reply with compgen, appending a space to possible > -# completion words, if necessary. > +# Generates completion reply for the word in $cur, appending a space to > +# possible completion words, if necessary. > # It accepts 1 to 4 arguments: > # 1: List of possible completion words. > # 2: A prefix to be added to each possible completion word (optional). > -# 3: Generate possible completion matches for this word (optional). > +# 3: Generate possible completion matches for this word instead of $cur > +#(optional). > # 4: A suffix to be appended to each possible completion word (optional). > __gitcomp () > { > @@ -241,10 +242,22 @@ __gitcomp () > COMPREPLY=() > ;; > *) > - local IFS=$'\n' > - COMPREPLY=($(compgen -P "${2-}" \ > - -W "$(__gitcomp_1 "${1-}" "${4-}")" \ > - -- "$cur_")) > + local i=0 c IFS=$' \t\n' > + for c in $1; do > + case $c in > + "$cur_"*) > + c="$c${4-}" > + case $c in > + --*=*|*.) ;; > + *) c="$c " ;; > + esac > + COMPREPLY[$i]="${2-}$c" > + i=$((++i)) > + ;; > + *) > + ;; > + esac > + done This is not quite the same as before, is it? Before the suffix would be taken into consideration for the comparison with $cur_, but not any more. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 5/5] completion: refactor __gitcomp_1
On Sat, Nov 17, 2012 at 11:58 AM, SZEDER Gábor wrote: >> # The following function is based on code from: >> @@ -249,10 +246,16 @@ __gitcomp () >> --*=) >> ;; >> *) >> - local IFS=$'\n' >> - __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" >> "" >> + local c IFS=$' \t\n' >> + for c in ${1-}; do >> + c=`__gitcomp_1 "$c${4-}"` > > 1. Backticks. > 2. A subshell for every word in the wordlist? Fine, lets make it hard for zsh then: --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -56,19 +56,6 @@ __gitdir () fi } -__gitcomp_1 () -{ - local c IFS=$' \t\n' - for c in $1; do - c="$c$2" - case $c in - --*=*|*.) ;; - *) c="$c " ;; - esac - printf '%s\n' "$c" - done -} - # The following function is based on code from: # # bash_completion - programmable completion functions for bash 3.2+ @@ -241,12 +228,22 @@ __gitcomp () COMPREPLY=() ;; *) - local IFS=$'\n' - COMPREPLY=($(compgen -P "${2-}" \ - -W "$(__gitcomp_1 "${1-}" "${4-}")" \ - -- "$cur_")) + local c i IFS=$' \t\n' + i=0 + for c in ${1-}; do + c="$c${4-}" + case $c in + --*=*|*.) ;; + *) c="$c " ;; + esac + if [[ "$c" = "$cur_"* ]]; then + (( i++ )) + COMPREPLY[$i]="${2-}$c" + fi + done ;; esac + } # Generates completion reply with compgen from newline-separated possible -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()
The compgen Bash-builtin performs expansion on all words in the wordlist given to its -W option, breaking Git's completion script when refs or filenames passed to __gitcomp_nl() contain expandable substrings. At least one user can't use ref completion at all in a repository, which contains tags with metacharacters like Build%20V%20${bamboo.custom.jiraversion.name}%20Build%20721 Such a ref causes a failure in compgen as it tries to expand the variable with invalid name. Unfortunately, compgen has no option to turn off this expansion. However, in __gitcomp_nl() we use only a small subset of compgen's functionality, namely the filtering of matching possible completion words and adding a prefix and/or suffix to each of those words. So, to avoid compgen's problematic expansion we get rid of compgen, and implement the equivalent functionality in a small awk script. The reason for using awk instead of sed is that awk allows plain (i.e. non-regexp) string comparison, making quoting of regexp characters in the current word unnecessary. Note, that such expandable words need quoting to be of any use on the command line. This patch doesn't perform that quoting, but it could be implemented later on top by extending the awk script to unquote $cur and to quote matching words. Nonetheless, this patch still a step forward, because at least refs can be completed even in the presence of one with metacharacters. Also update the function's description a bit. Reported-by: Jeroen Meijer Signed-off-by: SZEDER Gábor --- awk doesn't have a prefixcmp(). I'm no awk wizard, so I'm not sure this is the right way to do it. contrib/completion/git-completion.bash | 17 + t/t9902-completion.sh | 4 ++-- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index bc0657a2..65196ddd 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -249,19 +249,28 @@ __gitcomp () esac } -# Generates completion reply with compgen from newline-separated possible -# completion words by appending a space to all of them. +# Generates completion reply for the word in $cur from newline-separated +# possible completion words, appending a space to all of them. # It accepts 1 to 4 arguments: # 1: List of possible completion words, separated by a single newline. # 2: A prefix to be added to each possible completion word (optional). -# 3: Generate possible completion matches for this word (optional). +# 3: Generate possible completion matches for this word instead of $cur +#(optional). # 4: A suffix to be appended to each possible completion word instead of #the default space (optional). If specified but empty, nothing is #appended. __gitcomp_nl () { local IFS=$'\n' - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}")) + COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" ' + BEGIN { + FS="\n"; + len=length(cur); + } + { + if (cur == substr($1, 1, len)) + print pfx$1sfx; + }' <<< "$1" )) } __git_heads () diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index a108ec1c..fa289324 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -246,7 +246,7 @@ test_expect_success '__gitcomp_nl - no suffix' ' test_cmp expected out ' -test_expect_failure '__gitcomp_nl - doesnt fail because of invalid variable name' ' +test_expect_success '__gitcomp_nl - doesnt fail because of invalid variable name' ' ( __gitcomp_nl "$invalid_variable_name" ) @@ -383,7 +383,7 @@ test_expect_success 'complete tree filename with spaces' ' EOF ' -test_expect_failure 'complete tree filename with metacharacters' ' +test_expect_success 'complete tree filename with metacharacters' ' echo content >"name with \${meta}" && git add . && git commit -m meta && -- 1.8.0.220.g4d14ece -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/7] completion: remove the now unused __gitcomp_1() internal helper function
Since the __gitcomp_1() helper is basically only an implementation detail of __gitcomp() that exists solely for performance reasons (see ab02dfe5 (bash completion: Improve responsiveness of git-log completion, 2008-07-13)), it's quite unlikely that anyone uses or ever used it besides __gitcomp(). And now __gitcomp() doesn't need it anymore either, so delete it. Signed-off-by: SZEDER Gábor --- contrib/completion/git-completion.bash | 13 - 1 file changed, 13 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 283ef99b..a281b28d 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -56,19 +56,6 @@ __gitdir () fi } -__gitcomp_1 () -{ - local c IFS=$' \t\n' - for c in $1; do - c="$c$2" - case $c in - --*=*|*.) ;; - *) c="$c " ;; - esac - printf '%s\n' "$c" - done -} - # The following function is based on code from: # # bash_completion - programmable completion functions for bash 3.2+ -- 1.8.0.220.g4d14ece -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/7] completion: fix expansion issue in __gitcomp()
The compgen Bash-builtin performs expansion on all words in the wordlist given to its -W option, breaking Git's completion script when words passed to __gitcomp() contain expandable substrings. In __gitcomp() we only use a small subset ot compgen's functionality, namely the filtering of matching possible completion words and adding a prefix to each of those words; suffix is added by the __gitcomp_1() helper function instead. Now, since we already have to iterate over all words in the wordlist to append a trailing space if necessary, we can check in the same loop whether a word matches the word to be completed and store matching words with possible prefix and/or suffix in the COMPREPLY array. This way we achieve the same functionality without the compgen builtin and, more importantly, without it's problematic expansions. An additional benefit, especially for Windows/MSysGit users, is the loss of two subshells, one to run __gitcomp_1() and the other to run the compgen builtin. Note, that like the previous patch for __gitcomp_nl(), this patch doesn't quote expandable words either, but that could be implemented later on top by unquoting $cur and then quoting what get stored in COMPREPLY. Also update the function's description a bit. Signed-off-by: SZEDER Gábor --- contrib/completion/git-completion.bash | 27 --- t/t9902-completion.sh | 2 +- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 65196ddd..283ef99b 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -225,12 +225,13 @@ _get_comp_words_by_ref () fi fi -# Generates completion reply with compgen, appending a space to possible -# completion words, if necessary. +# Generates completion reply for the word in $cur, appending a space to +# possible completion words, if necessary. # It accepts 1 to 4 arguments: # 1: List of possible completion words. # 2: A prefix to be added to each possible completion word (optional). -# 3: Generate possible completion matches for this word (optional). +# 3: Generate possible completion matches for this word instead of $cur +#(optional). # 4: A suffix to be appended to each possible completion word (optional). __gitcomp () { @@ -241,10 +242,22 @@ __gitcomp () COMPREPLY=() ;; *) - local IFS=$'\n' - COMPREPLY=($(compgen -P "${2-}" \ - -W "$(__gitcomp_1 "${1-}" "${4-}")" \ - -- "$cur_")) + local i=0 c IFS=$' \t\n' + for c in $1; do + case $c in + "$cur_"*) + c="$c${4-}" + case $c in + --*=*|*.) ;; + *) c="$c " ;; + esac + COMPREPLY[$i]="${2-}$c" + i=$((++i)) + ;; + *) + ;; + esac + done ;; esac } diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index fa289324..d08f4259 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -156,7 +156,7 @@ test_expect_success '__gitcomp - suffix' ' test_cmp expected out ' -test_expect_failure '__gitcomp - doesnt fail because of invalid variable name' ' +test_expect_success '__gitcomp - doesnt fail because of invalid variable name' ' ( __gitcomp "$invalid_variable_name" ) -- 1.8.0.220.g4d14ece -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/7] completion: add tests for invalid variable name among completion words
The compgen Bash-builtin performs expansion on all words in the wordlist given to its -W option, breaking Git's completion script in various ways if one of those words can be expanded. The breakage can be simply bogus possible completion words, but it can also be a failure: $ git branch '${foo.bar}' $ git checkout bash: ${foo.bar}: bad substitution ${foo.bar} is an invalid variable name, which triggers the failure when compgen attempts to expand it, completely breaking refs completion. The same applies to e.g. completing the : notation when a filename contains a similar expandable substring. Since both __gitcomp() and __gitcomp_nl() rely on compgen, both are affected by this issue. So add a simple test for each of them to check that such a word doesn't cause failures (but don't check the resulting possible completion words for now, because that should be quoted properly, and that's a separate topic). Reported-by: Jeroen Meijer Signed-off-by: SZEDER Gábor --- t/t9902-completion.sh | 13 + 1 file changed, 13 insertions(+) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 32b3e8c4..a108ec1c 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -71,6 +71,7 @@ test_completion_long () } newline=$'\n' +invalid_variable_name="${foo.bar}" test_expect_success '__gitcomp - trailing space - options' ' sed -e "s/Z$//" >expected <<-\EOF && @@ -155,6 +156,12 @@ test_expect_success '__gitcomp - suffix' ' test_cmp expected out ' +test_expect_failure '__gitcomp - doesnt fail because of invalid variable name' ' + ( + __gitcomp "$invalid_variable_name" + ) +' + test_expect_success '__gitcomp_nl - trailing space' ' sed -e "s/Z$//" >expected <<-\EOF && maint Z @@ -239,6 +246,12 @@ test_expect_success '__gitcomp_nl - no suffix' ' test_cmp expected out ' +test_expect_failure '__gitcomp_nl - doesnt fail because of invalid variable name' ' + ( + __gitcomp_nl "$invalid_variable_name" + ) +' + test_expect_success 'basic' ' run_completion git "" && # built-in -- 1.8.0.220.g4d14ece -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/7] completion: add tests for the __gitcomp_nl() completion helper function
Test __gitcomp_nl()'s basic functionality, i.e. that trailing space, prefix, and suffix are added correctly. Signed-off-by: SZEDER Gábor --- t/t9902-completion.sh | 84 +++ 1 file changed, 84 insertions(+) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 3af75872..32b3e8c4 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -155,6 +155,90 @@ test_expect_success '__gitcomp - suffix' ' test_cmp expected out ' +test_expect_success '__gitcomp_nl - trailing space' ' + sed -e "s/Z$//" >expected <<-\EOF && + maint Z + master Z + EOF + ( + declare -a COMPREPLY && + refs="$(cat <<-\EOF + maint + master + next + pu + EOF + )" && + cur="m" && + __gitcomp_nl "$refs" && + print_comp + ) && + test_cmp expected out +' + +test_expect_success '__gitcomp_nl - prefix' ' + sed -e "s/Z$//" >expected <<-\EOF && + --fixup=maint Z + --fixup=master Z + EOF + ( + declare -a COMPREPLY && + refs="$(cat <<-\EOF + maint + master + next + pu + EOF + )" && + cur="--fixup=m" && + __gitcomp_nl "$refs" "--fixup=" "m" && + print_comp + ) && + test_cmp expected out +' + +test_expect_success '__gitcomp_nl - suffix' ' + sed -e "s/Z$//" >expected <<-\EOF && + branch.maint.Z + branch.master.Z + EOF + ( + declare -a COMPREPLY && + refs="$(cat <<-\EOF + maint + master + next + pu + EOF + )" && + cur="branch.ma" && + __gitcomp_nl "$refs" "branch." "ma" "." && + print_comp + ) && + test_cmp expected out +' + +test_expect_success '__gitcomp_nl - no suffix' ' + sed -e "s/Z$//" >expected <<-\EOF && + maintZ + masterZ + EOF + ( + declare -a COMPREPLY && + refs="$(cat <<-\EOF + maint + master + next + pu + EOF + )" && + cur="ma" && + __gitcomp_nl "$refs" "" "ma" "" && + print_comp + ) && + test_cmp expected out +' + test_expect_success 'basic' ' run_completion git "" && # built-in -- 1.8.0.220.g4d14ece -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/7] completion: make the 'basic' test more tester-friendly
The 'basic' test uses 'grep -q' to filter the resulting possible completion words while looking for the presence or absence of certain git commands, and relies on grep's exit status to indicate a failure. This works fine as long as there are no errors. However, in case of a failure it doesn't give any indication whatsoever about the reason of the failure, i.e. which condition failed. To make testers' life easier provide some output about the failed condition: store the results of the filtering in a file and compare its contents to the expected results by the good old test_cmp helper. However, to actually get output from test_cmp in case of an error we must make sure that test_cmp is always executed. Since in case of an error grep's exit code aborts the test immediately, before running the subsequent test_cmp, do the filtering using sed instead of grep. Signed-off-by: SZEDER Gábor --- t/t9902-completion.sh | 16 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 8fa025f9..b56759f7 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -158,14 +158,22 @@ test_expect_success '__gitcomp - suffix' ' test_expect_success 'basic' ' run_completion "git \"\"" && # built-in - grep -q "^add \$" out && + echo "add " >expected && + sed -n "/^add \$/p" out >out2 && + test_cmp expected out2 && # script - grep -q "^filter-branch \$" out && + echo "filter-branch " >expected && + sed -n "/^filter-branch \$/p" out >out2 && + test_cmp expected out2 && # plumbing - ! grep -q "^ls-files \$" out && + >expected && + sed -n "/^ls-files \$/p" out >out2 && + test_cmp expected out2 && run_completion "git f" && - ! grep -q -v "^f" out + >expected && + sed -n "/^[^f]/p" out >out2 && + test_cmp expected out2 ' test_expect_success 'double dash "git" itself' ' -- 1.8.0.220.g4d14ece -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/7] completion: fix args of run_completion() test helper
To simulate that the user hit 'git , the 'basic' test sets up the rather strange command line containing the two words git "" i.e. the second word on the command line consists of two double quotes. This is not what happens for real, however, because after 'git ' the second word on the command line is just an empty string. Luckily, the test works nevertheless. Fix this by passing the command line to run_completion() as separate words. Signed-off-by: SZEDER Gábor --- t/t9902-completion.sh | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index b56759f7..3af75872 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -49,7 +49,7 @@ run_completion () { local -a COMPREPLY _words local _cword - _words=( $1 ) + _words=( "$@" ) (( _cword = ${#_words[@]} - 1 )) __git_wrap__git_main && print_comp } @@ -57,7 +57,7 @@ run_completion () test_completion () { test $# -gt 1 && echo "$2" > expected - run_completion "$@" && + run_completion $1 && test_cmp expected out } @@ -156,7 +156,7 @@ test_expect_success '__gitcomp - suffix' ' ' test_expect_success 'basic' ' - run_completion "git \"\"" && + run_completion git "" && # built-in echo "add " >expected && sed -n "/^add \$/p" out >out2 && @@ -170,7 +170,7 @@ test_expect_success 'basic' ' sed -n "/^ls-files \$/p" out >out2 && test_cmp expected out2 && - run_completion "git f" && + run_completion git f && >expected && sed -n "/^[^f]/p" out >out2 && test_cmp expected out2 -- 1.8.0.220.g4d14ece -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/7] completion: fix expansion issues with compgen
This series fixes the expansion issues with compgen reported by Jeroen Meijer a while ago in http://article.gmane.org/gmane.comp.version-control.git/201596 The problem is that the compgen Bash-builtin performs expansion on all words in the wordlist given to its -W option, breaking Git's completion script when e.g. refs or filenames contain expandable substrings. Since compgen has no option to turn off this expansion and we only use a small subset of compgen's functionality, this series replaces compgen in __gitcomp() and __gitcomp_nl() by some shell logic and a small awk script, respectively. Patches 1 and 2 are test enhancements and fixes, while 3 and 4 add new tests to make sure I don't break anything and to demonstrate the issues, respectively. The actual bugfixes are in patches 5 and 6. Finally, patch 7 is a cleanup made possible by patch 6 (could be squashed into 6). In the future we might want/need to fill COMPREPLY directly when completing a path in the : notation instead using __gitcomp_nl() there, because paths can contain newlines, too. Compared to Felipe's series from yesterday, this series has more tests, more descriptive commit messages, and it's faster. However, it doesn't include his 1/5 (completion: get rid of empty COMPREPLY assignments). Footnote: check the patchlist below, and notice how format-patch put patches 4 and 5 into the same line. SZEDER Gábor (7): completion: make the 'basic' test more tester-friendly completion: fix args of run_completion() test helper completion: add tests for the __gitcomp_nl() completion helper function completion: add tests for invalid variable name among completion words completion: fix expansion issues in __gitcomp_nl() completion: fix expansion issue in __gitcomp() completion: remove the now unused __gitcomp_1() internal helper function contrib/completion/git-completion.bash | 57 --- t/t9902-completion.sh | 123 ++--- 2 files changed, 147 insertions(+), 33 deletions(-) -- 1.8.0.220.g4d14ece -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 4/5] completion: get rid of compgen
On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote: > The functionality we use is very simple, plus, this fixes a known > breakage 'complete tree filename with metacharacters'. > > Signed-off-by: Felipe Contreras > --- > contrib/completion/git-completion.bash | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/contrib/completion/git-completion.bash > b/contrib/completion/git-completion.bash > index 975ae13..ad3e1fe 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -227,7 +227,11 @@ fi > > __gitcompadd () > { > - COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3")) > + for x in $1; do > + if [[ "$x" = "$3"* ]]; then > + COMPREPLY+=("$2$x$4") > + fi > + done The whole point of creating __gitcomp_nl() back then was to fill COMPREPLY without iterating through all words in the wordlist, making completion faster for large number of words, e.g. a lot of refs, or later a lot of symbols for 'git grep' in a larger project. The loop here kills that optimization. > } > > # Generates completion reply with compgen, appending a space to possible > -- > 1.8.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 3/5] completion: trivial test improvement
On Sat, Nov 17, 2012 at 02:38:16AM +0100, Felipe Contreras wrote: > Instead of passing a dummy "", let's check if the last character is a > space, and then move the _cword accordingly. > > Apparently we were passing "" all the way to compgen, which fortunately > expanded it to nothing. Glad you noticed it, too. I posted an alternative fix (without any new conditions in the code path) a while ago: http://article.gmane.org/gmane.comp.version-control.git/206525 Will repost it as part of my series shortly. > Lets do the right thing though. > > Signed-off-by: Felipe Contreras > --- > t/t9902-completion.sh | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > index cbd0fb6..0b5e1f5 100755 > --- a/t/t9902-completion.sh > +++ b/t/t9902-completion.sh > @@ -51,6 +51,7 @@ run_completion () > local _cword > _words=( $1 ) > (( _cword = ${#_words[@]} - 1 )) > + test "${1: -1}" == ' ' && (( _cword += 1 )) > __git_wrap__git_main && print_comp > } > > @@ -156,7 +157,7 @@ test_expect_success '__gitcomp - suffix' ' > ' > > test_expect_success 'basic' ' > - run_completion "git \"\"" && > + run_completion "git " && > # built-in > grep -q "^add \$" out && > # script > -- > 1.8.0 > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 5/5] completion: refactor __gitcomp_1
[Wow, that's quite the Cc-list above. I wonder why e.g. Robert ended up there, when all his "sins" were to add a couple of 'git svn' options back in 2009.] On Sat, Nov 17, 2012 at 02:38:18AM +0100, Felipe Contreras wrote: > It's only used by __gitcomp, so move some code there and avoid going > through the loop again. > > We could get rid of it altogether, but it's useful for zsh's completion > wrapper. > > Signed-off-by: Felipe Contreras > --- > contrib/completion/git-completion.bash | 25 ++--- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/contrib/completion/git-completion.bash > b/contrib/completion/git-completion.bash > index ad3e1fe..d92d11e 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -58,15 +58,12 @@ __gitdir () > > __gitcomp_1 () > { > - local c IFS=$' \t\n' > - for c in $1; do > - c="$c$2" > - case $c in > - --*=*|*.) ;; > - *) c="$c " ;; > - esac > - printf '%s\n' "$c" > - done > + local c=$1 > + case $c in > + --*=*|*.) ;; > + *) c="$c " ;; > + esac > + printf '%s\n' "$c" > } > > # The following function is based on code from: > @@ -249,10 +246,16 @@ __gitcomp () > --*=) > ;; > *) > - local IFS=$'\n' > - __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" "" > + local c IFS=$' \t\n' > + for c in ${1-}; do > + c=`__gitcomp_1 "$c${4-}"` 1. Backticks. 2. A subshell for every word in the wordlist? > + if [[ "$c" = "$cur_"* ]]; then > + COMPREPLY+=("${2-}$c") This is the first time we use the append operator in the completion script. When it came up last time the question was whether the benefit of using it is large enough for worrying about supported Bash versions. http://article.gmane.org/gmane.comp.version-control.git/206525 > + fi > + done > ;; > esac > + > } > > # Generates completion reply with compgen from newline-separated possible > -- > 1.8.0 > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tcsh-completion re-using git-completion.bash
On Fri, Nov 16, 2012 at 10:46:16PM +0100, Felipe Contreras wrote: > On Fri, Nov 16, 2012 at 10:22 PM, SZEDER Gábor wrote: > > On Fri, Nov 16, 2012 at 10:03:41PM +0100, Felipe Contreras wrote: > > >> > As I understand the main issues with using the completion script with > >> > zsh are the various little incompatibilities between the two shells > >> > and bugs in zsh's emulation of Bash's completion-related builtins. > >> > Running the completion script under Bash and using its results in zsh > >> > would solve these issues at the root. And would allow as to remove > >> > some if [[ -n ${ZSH_VERSION-} ]] code. > >> > >> We can remove that code already, because we now have code that is > >> superior than zsh's bash completion emulation: > >> > >> http://article.gmane.org/gmane.comp.version-control.git/208173 > > > > Which depends on the completion script having a wrapper function > > around compgen filling COMPREPLY. > > No, it does not. Previous incarnations didn't have this dependency: > > http://article.gmane.org/gmane.comp.version-control.git/196720 Good. > > However, COMPREPLY will be soon > > filled by hand-rolled code to prevent expansion issues with compgen, > > and there will be no such wrapper. > > I'm still waiting to see a resemblance of that code, but my bet would > be that there will be a way to fill both COMPREPLY, and call zsh's > compadd. But it's hard to figure that out without any code. Which is > why I'm thinking on doing it myself. > > But even in that case, if push comes to shoves, this zsh wrapper can > ultimately read COMPREPLY and figure things backwards, as even more > previous versions did: > > http://article.gmane.org/gmane.comp.version-control.git/189310 Even better. I was just going to propose that zsh's completion could just read the contents of COMPREPLY at the end of _git() and _gitk(), because this way no zsh-induced helper functions and changes would be needed to the completion script at all. However, running the completion script with Bash would also prevent possible issues caused by incompatibilities between the two shells mentioned below. > >> This is the equivalent of what Marc is doing, except that zsh has no > >> problems running bash's code. Note there's a difference with zsh's > >> emulation bash (or rather bourne shell, or k shell), and zsh's > >> emulation of bash's _completion_. The former is fine, the later is > >> not. > > > > There are a couple of constructs supported by Bash but not by zsh, > > which we usually try to avoid. > > Yes, and is that a big deal? Not that big, but I wanted to point out that it's not "fine" either. Just a slight maintenance burden, because we have to pay attention not to use such constructs. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Auto-repo-repair
Hi, > You can't reliably just grab the broken objects, because most > transports > don't support grabbing arbitrary objects (you can do it if you have > shell access to a known-good repository, but it's not automated). can we introduce a new or extend existing transports to support that ? cu -- Mit freundlichen Grüßen / Kind regards Enrico Weigelt VNC - Virtual Network Consult GmbH Head Of Development Pariser Platz 4a, D-10117 Berlin Tel.: +49 (30) 3464615-20 Fax: +49 (30) 3464615-59 enrico.weig...@vnc.biz; www.vnc.de -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add support for a 'pre-push' hook
On Fri, Nov 16, 2012 at 9:25 PM, Matthieu Moy wrote: > Aske Olsson writes: > >> +--no-verify:: >> + This option bypasses the pre-push hook. >> + See also linkgit:githooks[5]. >> + >> -q:: >> --quiet:: >> Suppress all output, including the listing of updated refs, > > Here, and below: you seem to have whitespace damage. Somebody replaced > tabs with spaces I guess. Using git send-email helps avoiding this. I had some firewall problems at work, so ended up sending from gmail. Will fix ;) >> +D=`pwd` > > Unused variable, left from previous hacking I guess. Yep! > I'd add a "touch hook-ran" in the script, a "rm -f hook-ran" before > launching git-push, and test the file existance after the hook to make > sure it was ran. Yea' that would probably be a good idea! > I don't think you need to re-create the repos for each tests. Tests are > good, but they're better when they're fast so avoiding useless > operations is good. > > We try to write tests so that one test failure does not imply failures > of the next tests (i.e. stopping in the middle should not not leave > garbage that would prevent the next test from running), but do not > necessarily write 100% self-contained tests. Ok, I'll speed it up. >> + echo one >foo && git add foo && git commit -m one && > > test_commit ? Cool, I'll use that! > It would be cool to actually check that the push was not performed > (verify that the target repo is still pointing to the old revision). > Otherwise, an implementation that would call run_hook after pushing > would pass the tests. [...] > > These two tests are not testing much. The hook is not executable, so it > shouldn't be ran, but you do not check whether the hook is ran or not. > At least make the "exit 0" an "exit 1" in the hook. All very good points, thanks. I'll get back to hacking. -Aske -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add support for a 'pre-push' hook
On Sat, Nov 17, 2012 at 7:39 AM, Michael Haggerty wrote: > > On 11/16/2012 09:30 PM, Junio C Hamano wrote: > > Aske Olsson writes: > > > >> If the script .git/hooks/pre-push exists and is executable it will be > >> called before a `git push` command, and when the script exits with a > >> non-zero status the push will be aborted. > >> The hook can be overridden by passing the '--no-verify' option to > >> `git push`. > >> > >> The pre-push hook is usefull to run tests etc. before push. Or to make > >> sure that if a binary solution like git-media, git-annex or git-bin is > >> used the binaries are uploaded before the push, so when others do a > >> fetch the binaries will be available already. This also reduces the > >> need for introducing extra (git) commands to e.g. sync binaries. > >> > >> Signed-off-by: Aske Olsson > >> --- > >> ... > >> +[[pre-push]] > >> +pre-push > >> + > >> + > >> +This hook is invoked by 'git push' and can be bypassed with the > >> +`--no-verify` option. It takes no parameter, and is invoked before > >> +the push happens. > >> +Exiting with a non-zero status from this script causes 'git push' > >> +to abort. > >> ... > >> + if (!no_verify && run_hook(NULL, "pre-push")) { > >> + die(_("pre-push hook failed: exiting\n")); > >> + } > > > > NAK, at least in the current form. At least the system should tell > > the hook where it is pushing and what is being pushed. > > I agree. Yes, I also agree that is a nice piece of information to pass to the hook. Will work on that. > > Besides, there are five valid reasons to add a new hook to the > > system, but your version of pre-push does not satisfy any of them: > > > > > > http://thread.gmane.org/gmane.comp.version-control.git/94111/focus=71069 > > Here I disagree. I think it satisfies (1): > > > (1) A hook that countermands the normal decision made by the > > underlying command. Examples of this class are the update > > hook and the pre-commit hook. > > pre-push would be very similar in spirit to pre-commit: pre-commit is a > filter that can prevent a "bad" commit from getting into the local > repository; pre-push is similarly a filter between the local repo and > remote repositories. Yes, I was thinking of a pre-push hook in the same way, preventing bad commits from leaving the repo, but I guess you could argue that a bad commit shouldn't happen in the first place due to the pre-commit hook. > I also think it satisfies (2) and/or (5b): > > > (2) A hook that operates on data generated after the command > > starts to run. [...] > > > (5) [...] Another example is the post-checkout > > hook that gets information that is otherwise harder to get > > (namely, if it was a branch checkout or file checkout -- > > you can figure it out by examining the command line but > > that already is part of the processing git-checkout does > > anyway, so no need to force duplicating that code in the > > userland). > > It would not be trivial for a wrapper script to figure out what branches > and commits are about to be pushed. But "git push" could tell the hook > script what branches are to be pushed. And if the pre-push hook is run > after negotiation between client and server of what commits need to be > transfered, the hook could also be provided that information and use it > to figure out which commits it should vet. > > > Although a pre-receive script on the remote repository could do most of > the same things as a pre-push script, the latter would sometimes have > advantages because it is run "client-side": > > * When the user doesn't have the ability to change the pre-receive > script on the server (think public git hosting services). Yeah, and for my case I would like to sync some binaries to a remote storage before pushing the commits that contains references to these. I know a wrapper script could easily do this, but I'm not to sure that it will work in an organization with a lot of developers. Sooner or later one of them will have some git problem and search the net, find an answer which (of course) doesn't include the commands in the wrapper script. Then he might end up pushing something that none of the other devs can get hold of and that might cause builds to break etc. And we can't currently run pre-receive hooks to deny the commit. > * For user-specific actions that are not wanted by all users pushing to > the same server (for example, maybe I add the string "WIP" to commit > messages for commits that are not ready to be pushed; my pre-push script > could verify that I don't push such a commit by accident). > > * Preventing "secret" info (password files, proprietary branches) from > being pushed. Even if the remote repo were taught to reject them, they > would have already traversed the internet. Yes we have also seen a couple of these, would be nice to have a way do deny them. -Aske -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a m