* [PATCH 0/3] completion: Convention updates and DIRTYSTATE fix
@ 2009-02-11 15:37 Ted Pavlic
2009-02-11 15:37 ` [PATCH 1/3] completion: For consistency, changed "git rev-parse" to __gitdir calls Ted Pavlic
0 siblings, 1 reply; 16+ messages in thread
From: Ted Pavlic @ 2009-02-11 15:37 UTC (permalink / raw)
To: spearce; +Cc: git, gitster, Ted Pavlic
Three more patches to git's bash completion script.
The first two are non-critical:
* The first patch changes two "git rev-parse" calls to "__gitdir" to
match the convention used in the rest of the script.
* The second patch changes "[...]" to "test ..." to match git
convention. In the one case of "[...] || [...]", a "test || test" call
is used. Alternatively, a "test ... -o ..." call could be used, but
that might not be as readable.
The third fixes an ugly error in the new GIT_PS1_DIRTYSTATE. In order to
determine whether the branch name needs a "*" following it, the
DIRTYSTATE implementation uses a "git diff." Because "git diff" is
illegal when not in a working tree, this command gives an ugly error
when changing directory to ".git". This patch detects this case and sets
"--work-tree=..". Is there a better fix?
Ted Pavlic (3):
completion: For consistency, changed "git rev-parse" to __gitdir
calls.
completion: Change "if [...]" to "if test ..." to match git
convention
completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is
.git
contrib/completion/git-completion.bash | 101 +++++++++++++++-----------------
1 files changed, 48 insertions(+), 53 deletions(-)
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/3] completion: For consistency, changed "git rev-parse" to __gitdir calls. 2009-02-11 15:37 [PATCH 0/3] completion: Convention updates and DIRTYSTATE fix Ted Pavlic @ 2009-02-11 15:37 ` Ted Pavlic 2009-02-11 15:37 ` [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention Ted Pavlic 2009-02-11 16:22 ` [PATCH 1/3] completion: For consistency, changed "git rev-parse" to __gitdir calls Shawn O. Pearce 0 siblings, 2 replies; 16+ messages in thread From: Ted Pavlic @ 2009-02-11 15:37 UTC (permalink / raw) To: spearce; +Cc: git, gitster, Ted Pavlic Signed-off-by: Ted Pavlic <ted@tedpavlic.com> --- contrib/completion/git-completion.bash | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index f44f63c..6bbe09a 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -80,7 +80,7 @@ __gitdir () # returns text to add to bash PS1 prompt (includes branch name) __git_ps1 () { - local g="$(git rev-parse --git-dir 2>/dev/null)" + local g="$(__gitdir)" if [ -n "$g" ]; then local r local b @@ -1797,7 +1797,7 @@ _gitk () __git_has_doubledash && return local cur="${COMP_WORDS[COMP_CWORD]}" - local g="$(git rev-parse --git-dir 2>/dev/null)" + local g="$(__gitdir)" local merge="" if [ -f $g/MERGE_HEAD ]; then merge="--merge" -- 1.6.1.2.390.gba743 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention 2009-02-11 15:37 ` [PATCH 1/3] completion: For consistency, changed "git rev-parse" to __gitdir calls Ted Pavlic @ 2009-02-11 15:37 ` Ted Pavlic 2009-02-11 15:37 ` [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Ted Pavlic ` (2 more replies) 2009-02-11 16:22 ` [PATCH 1/3] completion: For consistency, changed "git rev-parse" to __gitdir calls Shawn O. Pearce 1 sibling, 3 replies; 16+ messages in thread From: Ted Pavlic @ 2009-02-11 15:37 UTC (permalink / raw) To: spearce; +Cc: git, gitster, Ted Pavlic In the single case of: if [...] || [...] changed to: if test ... || test ... However, if test ... -o ... might be favorable (although arguably less readable). Also changed: if test ... then to: if test ...; then Signed-off-by: Ted Pavlic <ted@tedpavlic.com> --- contrib/completion/git-completion.bash | 91 ++++++++++++++----------------- 1 files changed, 41 insertions(+), 50 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 6bbe09a..6772be7 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -61,15 +61,15 @@ esac # returns location of .git repo __gitdir () { - if [ -z "${1-}" ]; then - if [ -n "$__git_dir" ]; then + if test -z "${1-}"; then + if test -n "$__git_dir"; then echo "$__git_dir" - elif [ -d .git ]; then + elif test -d .git; then echo .git else git rev-parse --git-dir 2>/dev/null fi - elif [ -d "$1/.git" ]; then + elif test -d "$1/.git"; then echo "$1/.git" else echo "$1" @@ -81,42 +81,33 @@ __gitdir () __git_ps1 () { local g="$(__gitdir)" - if [ -n "$g" ]; then + if test -n "$g"; then local r local b - if [ -d "$g/rebase-apply" ] - then - if test -f "$g/rebase-apply/rebasing" - then + if test -d "$g/rebase-apply"; then + if test -f "$g/rebase-apply/rebasing"; then r="|REBASE" - elif test -f "$g/rebase-apply/applying" - then + elif test -f "$g/rebase-apply/applying"; then r="|AM" else r="|AM/REBASE" fi b="$(git symbolic-ref HEAD 2>/dev/null)" - elif [ -f "$g/rebase-merge/interactive" ] - then + elif test -f "$g/rebase-merge/interactive"; then r="|REBASE-i" b="$(cat "$g/rebase-merge/head-name")" - elif [ -d "$g/rebase-merge" ] - then + elif test -d "$g/rebase-merge"; then r="|REBASE-m" b="$(cat "$g/rebase-merge/head-name")" - elif [ -f "$g/MERGE_HEAD" ] - then + elif test -f "$g/MERGE_HEAD"; then r="|MERGING" b="$(git symbolic-ref HEAD 2>/dev/null)" else - if [ -f "$g/BISECT_LOG" ] - then + if test -f "$g/BISECT_LOG"; then r="|BISECTING" fi - if ! b="$(git symbolic-ref HEAD 2>/dev/null)" - then - if ! b="$(git describe --exact-match HEAD 2>/dev/null)" - then + if ! b="$(git symbolic-ref HEAD 2>/dev/null)"; then + if ! b="$(git describe --exact-match HEAD 2>/dev/null)"; then b="$(cut -c1-7 "$g/HEAD")..." fi fi @@ -138,7 +129,7 @@ __git_ps1 () fi fi - if [ -n "${1-}" ]; then + if test -n "${1-}"; then printf "$1" "${b##refs/heads/}$w$i$r" else printf " (%s)" "${b##refs/heads/}$w$i$r" @@ -164,7 +155,7 @@ __gitcomp_1 () __gitcomp () { local cur="${COMP_WORDS[COMP_CWORD]}" - if [ $# -gt 2 ]; then + if test $# -gt 2; then cur="$3" fi case "$cur" in @@ -184,7 +175,7 @@ __gitcomp () __git_heads () { local cmd i is_hash=y dir="$(__gitdir "${1-}")" - if [ -d "$dir" ]; then + if test -d "$dir"; then git --git-dir="$dir" for-each-ref --format='%(refname:short)' \ refs/heads return @@ -203,7 +194,7 @@ __git_heads () __git_tags () { local cmd i is_hash=y dir="$(__gitdir "${1-}")" - if [ -d "$dir" ]; then + if test -d "$dir"; then git --git-dir="$dir" for-each-ref --format='%(refname:short)' \ refs/tags return @@ -223,14 +214,14 @@ __git_refs () { local i is_hash=y dir="$(__gitdir "${1-}")" local cur="${COMP_WORDS[COMP_CWORD]}" format refs - if [ -d "$dir" ]; then + if test -d "$dir"; then case "$cur" in refs|refs/*) format="refname" refs="${cur%/*}" ;; *) - if [ -e "$dir/HEAD" ]; then echo HEAD; fi + if test -e "$dir/HEAD"; then echo HEAD; fi format="refname:short" refs="refs/tags refs/heads refs/remotes" ;; @@ -299,7 +290,7 @@ __git_remotes () __git_merge_strategies () { - if [ -n "$__git_merge_strategylist" ]; then + if test -n "$__git_merge_strategylist"; then echo "$__git_merge_strategylist" return fi @@ -385,7 +376,7 @@ __git_complete_revlist () __git_all_commands () { - if [ -n "$__git_all_commandlist" ]; then + if test -n "$__git_all_commandlist"; then echo "$__git_all_commandlist" return fi @@ -403,7 +394,7 @@ __git_all_commandlist="$(__git_all_commands 2>/dev/null)" __git_porcelain_commands () { - if [ -n "$__git_porcelain_commandlist" ]; then + if test -n "$__git_porcelain_commandlist"; then echo "$__git_porcelain_commandlist" return fi @@ -511,7 +502,7 @@ __git_aliased_command () local word cmdline=$(git --git-dir="$(__gitdir)" \ config --get "alias.$1") for word in $cmdline; do - if [ "${word##-*}" ]; then + if test "${word##-*}"; then echo $word return fi @@ -526,7 +517,7 @@ __git_find_subcommand () while [ $c -lt $COMP_CWORD ]; do word="${COMP_WORDS[c]}" for subcommand in $1; do - if [ "$subcommand" = "$word" ]; then + if test "$subcommand" = "$word"; then echo "$subcommand" return fi @@ -539,7 +530,7 @@ __git_has_doubledash () { local c=1 while [ $c -lt $COMP_CWORD ]; do - if [ "--" = "${COMP_WORDS[c]}" ]; then + if test "--" = "${COMP_WORDS[c]}"; then return 0 fi c=$((++c)) @@ -552,7 +543,7 @@ __git_whitespacelist="nowarn warn error error-all fix" _git_am () { local cur="${COMP_WORDS[COMP_CWORD]}" dir="$(__gitdir)" - if [ -d "$dir"/rebase-apply ]; then + if test -d "$dir"/rebase-apply; then __gitcomp "--skip --resolved --abort" return fi @@ -636,7 +627,7 @@ _git_bisect () local subcommands="start bad good skip reset visualize replay log run" local subcommand="$(__git_find_subcommand "$subcommands")" - if [ -z "$subcommand" ]; then + if test -z "$subcommand"; then __gitcomp "$subcommands" return fi @@ -672,7 +663,7 @@ _git_branch () " ;; *) - if [ $only_local_ref = "y" -a $has_r = "n" ]; then + if test $only_local_ref = "y" -a $has_r = "n"; then __gitcomp "$(__git_heads)" else __gitcomp "$(__git_refs)" @@ -830,7 +821,7 @@ _git_fetch () { local cur="${COMP_WORDS[COMP_CWORD]}" - if [ "$COMP_CWORD" = 2 ]; then + if test "$COMP_CWORD" = 2; then __gitcomp "$(__git_remotes)" else case "$cur" in @@ -1088,7 +1079,7 @@ _git_pull () { local cur="${COMP_WORDS[COMP_CWORD]}" - if [ "$COMP_CWORD" = 2 ]; then + if test "$COMP_CWORD" = 2; then __gitcomp "$(__git_remotes)" else __gitcomp "$(__git_refs "${COMP_WORDS[2]}")" @@ -1099,7 +1090,7 @@ _git_push () { local cur="${COMP_WORDS[COMP_CWORD]}" - if [ "$COMP_CWORD" = 2 ]; then + if test "$COMP_CWORD" = 2; then __gitcomp "$(__git_remotes)" else case "$cur" in @@ -1125,7 +1116,7 @@ _git_push () _git_rebase () { local cur="${COMP_WORDS[COMP_CWORD]}" dir="$(__gitdir)" - if [ -d "$dir"/rebase-apply ] || [ -d "$dir"/rebase-merge ]; then + if test -d "$dir"/rebase-apply || test -d "$dir"/rebase-merge; then __gitcomp "--continue --skip --abort" return fi @@ -1416,7 +1407,7 @@ _git_remote () { local subcommands="add rename rm show prune update" local subcommand="$(__git_find_subcommand "$subcommands")" - if [ -z "$subcommand" ]; then + if test -z "$subcommand"; then __gitcomp "$subcommands" return fi @@ -1546,7 +1537,7 @@ _git_stash () { local subcommands='save list show apply clear drop pop create branch' local subcommand="$(__git_find_subcommand "$subcommands")" - if [ -z "$subcommand" ]; then + if test -z "$subcommand"; then __gitcomp "$subcommands" else local cur="${COMP_WORDS[COMP_CWORD]}" @@ -1576,7 +1567,7 @@ _git_submodule () __git_has_doubledash && return local subcommands="add status init update summary foreach sync" - if [ -z "$(__git_find_subcommand "$subcommands")" ]; then + if test -z "$(__git_find_subcommand "$subcommands")"; then local cur="${COMP_WORDS[COMP_CWORD]}" case "$cur" in --*) @@ -1598,7 +1589,7 @@ _git_svn () proplist show-ignore show-externals " local subcommand="$(__git_find_subcommand "$subcommands")" - if [ -z "$subcommand" ]; then + if test -z "$subcommand"; then __gitcomp "$subcommands" else local remote_opts="--username= --config-dir= --no-auth-cache" @@ -1690,7 +1681,7 @@ _git_tag () COMPREPLY=() ;; -*|tag) - if [ $f = 1 ]; then + if test $f = 1; then __gitcomp "$(__git_tags)" else COMPREPLY=() @@ -1718,7 +1709,7 @@ _git () c=$((++c)) done - if [ -z "$command" ]; then + if test -z "$command"; then case "${COMP_WORDS[COMP_CWORD]}" in --*) __gitcomp " --paginate @@ -1799,7 +1790,7 @@ _gitk () local cur="${COMP_WORDS[COMP_CWORD]}" local g="$(__gitdir)" local merge="" - if [ -f $g/MERGE_HEAD ]; then + if test -f $g/MERGE_HEAD; then merge="--merge" fi case "$cur" in @@ -1820,7 +1811,7 @@ complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \ # when the user has tab-completed the executable name and consequently # included the '.exe' suffix. # -if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then +if test Cygwin = "$(uname -o 2>/dev/null)"; then complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \ || complete -o default -o nospace -F _git git.exe fi -- 1.6.1.2.390.gba743 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git 2009-02-11 15:37 ` [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention Ted Pavlic @ 2009-02-11 15:37 ` Ted Pavlic 2009-02-11 16:18 ` [PATCH 4/3] completion: More fixes to prevent unbound variable errors Ted Pavlic ` (2 more replies) 2009-02-11 16:24 ` [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention Shawn O. Pearce 2009-02-11 16:46 ` Junio C Hamano 2 siblings, 3 replies; 16+ messages in thread From: Ted Pavlic @ 2009-02-11 15:37 UTC (permalink / raw) To: spearce; +Cc: git, gitster, Ted Pavlic The GIT_PS1_DIRTYSTATE support uses a "git diff" to see if a "*" should be placed after the branch name. The "git diff" fails with an ugly error if the user has just changed directory into GIT_DIR. This patch uses "git rev-parse --is-inside-work-tree" to determine whether a "--work-tree=.." should be added to the "git diff". Signed-off-by: Ted Pavlic <ted@tedpavlic.com> --- contrib/completion/git-completion.bash | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 6772be7..ffde82a 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -115,10 +115,14 @@ __git_ps1 () local w local i + local worktreestr if test -n "${GIT_PS1_SHOWDIRTYSTATE-}"; then if test "$(git config --bool bash.showDirtyState)" != "false"; then - git diff --no-ext-diff --ignore-submodules \ + if test "false" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)"; then + worktreestr="--work-tree=.." + fi + git ${worktreestr} diff --no-ext-diff --ignore-submodules \ --quiet --exit-code || w="*" if git rev-parse --quiet --verify HEAD >/dev/null; then git diff-index --cached --quiet \ -- 1.6.1.2.390.gba743 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 4/3] completion: More fixes to prevent unbound variable errors. 2009-02-11 15:37 ` [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Ted Pavlic @ 2009-02-11 16:18 ` Ted Pavlic 2009-02-11 16:28 ` Shawn O. Pearce 2009-02-11 16:26 ` [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Shawn O. Pearce 2009-02-11 16:56 ` Junio C Hamano 2 siblings, 1 reply; 16+ messages in thread From: Ted Pavlic @ 2009-02-11 16:18 UTC (permalink / raw) To: spearce; +Cc: git, gitster, Ted Pavlic Several functions make use of "test -n" and "test -z". In many cases, the variables being tested were declared with "local." However, several __variables are not, and so they must be replaced with their ${__-} equivalents. Signed-off-by: Ted Pavlic <ted@tedpavlic.com> --- This patch depends on: <1234366634-17900-3-git-send-email-ted@tedpavlic.com> "[PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention" If that patch is not applied, I can submit a new patch. contrib/completion/git-completion.bash | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index ffde82a..055e4ac 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -62,7 +62,7 @@ esac __gitdir () { if test -z "${1-}"; then - if test -n "$__git_dir"; then + if test -n "${__git_dir-}"; then echo "$__git_dir" elif test -d .git; then echo .git @@ -294,7 +294,7 @@ __git_remotes () __git_merge_strategies () { - if test -n "$__git_merge_strategylist"; then + if test -n "${__git_merge_strategylist-}"; then echo "$__git_merge_strategylist" return fi @@ -380,7 +380,7 @@ __git_complete_revlist () __git_all_commands () { - if test -n "$__git_all_commandlist"; then + if test -n "${__git_all_commandlist-}"; then echo "$__git_all_commandlist" return fi @@ -398,7 +398,7 @@ __git_all_commandlist="$(__git_all_commands 2>/dev/null)" __git_porcelain_commands () { - if test -n "$__git_porcelain_commandlist"; then + if test -n "${__git_porcelain_commandlist-}"; then echo "$__git_porcelain_commandlist" return fi -- 1.6.1.2.390.gba743 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 4/3] completion: More fixes to prevent unbound variable errors. 2009-02-11 16:18 ` [PATCH 4/3] completion: More fixes to prevent unbound variable errors Ted Pavlic @ 2009-02-11 16:28 ` Shawn O. Pearce 0 siblings, 0 replies; 16+ messages in thread From: Shawn O. Pearce @ 2009-02-11 16:28 UTC (permalink / raw) To: Ted Pavlic; +Cc: git, gitster Ted Pavlic <ted@tedpavlic.com> wrote: > Several functions make use of "test -n" and "test -z". In many cases, > the variables being tested were declared with "local." However, several > __variables are not, and so they must be replaced with their ${__-} > equivalents. > > Signed-off-by: Ted Pavlic <ted@tedpavlic.com> > --- > > This patch depends on: > > <1234366634-17900-3-git-send-email-ted@tedpavlic.com> > "[PATCH 2/3] completion: Change "if [...]" to "if test ..." to > match git convention" > > If that patch is not applied, I can submit a new patch. Looks OK to me, but I NAK'd the dependency, so you'll have to rebase it without the dependency in there. Or talk me into why that much churn is a good thing. > contrib/completion/git-completion.bash | 8 ++++---- > 1 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index ffde82a..055e4ac 100755 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -62,7 +62,7 @@ esac > __gitdir () > { > if test -z "${1-}"; then > - if test -n "$__git_dir"; then > + if test -n "${__git_dir-}"; then > echo "$__git_dir" > elif test -d .git; then > echo .git > @@ -294,7 +294,7 @@ __git_remotes () > > __git_merge_strategies () > { > - if test -n "$__git_merge_strategylist"; then > + if test -n "${__git_merge_strategylist-}"; then > echo "$__git_merge_strategylist" > return > fi > @@ -380,7 +380,7 @@ __git_complete_revlist () > > __git_all_commands () > { > - if test -n "$__git_all_commandlist"; then > + if test -n "${__git_all_commandlist-}"; then > echo "$__git_all_commandlist" > return > fi > @@ -398,7 +398,7 @@ __git_all_commandlist="$(__git_all_commands 2>/dev/null)" > > __git_porcelain_commands () > { > - if test -n "$__git_porcelain_commandlist"; then > + if test -n "${__git_porcelain_commandlist-}"; then > echo "$__git_porcelain_commandlist" > return > fi > -- > 1.6.1.2.390.gba743 > -- Shawn. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git 2009-02-11 15:37 ` [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Ted Pavlic 2009-02-11 16:18 ` [PATCH 4/3] completion: More fixes to prevent unbound variable errors Ted Pavlic @ 2009-02-11 16:26 ` Shawn O. Pearce 2009-02-11 16:53 ` Ted Pavlic 2009-02-11 16:56 ` Junio C Hamano 2 siblings, 1 reply; 16+ messages in thread From: Shawn O. Pearce @ 2009-02-11 16:26 UTC (permalink / raw) To: Ted Pavlic; +Cc: git, gitster Ted Pavlic <ted@tedpavlic.com> wrote: > The GIT_PS1_DIRTYSTATE support uses a "git diff" to see if a "*" should > be placed after the branch name. The "git diff" fails with an ugly error > if the user has just changed directory into GIT_DIR. > > This patch uses "git rev-parse --is-inside-work-tree" to determine > whether a "--work-tree=.." should be added to the "git diff". I think it makes more sense to just drop the work tree stuff from the prompt if we aren't inside the work tree anymore. Meaning, we should behave as though bash.showDirtyState is false. > Signed-off-by: Ted Pavlic <ted@tedpavlic.com> > --- > contrib/completion/git-completion.bash | 6 +++++- > 1 files changed, 5 insertions(+), 1 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index 6772be7..ffde82a 100755 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -115,10 +115,14 @@ __git_ps1 () > > local w > local i > + local worktreestr > > if test -n "${GIT_PS1_SHOWDIRTYSTATE-}"; then > if test "$(git config --bool bash.showDirtyState)" != "false"; then > - git diff --no-ext-diff --ignore-submodules \ > + if test "false" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)"; then > + worktreestr="--work-tree=.." > + fi > + git ${worktreestr} diff --no-ext-diff --ignore-submodules \ > --quiet --exit-code || w="*" > if git rev-parse --quiet --verify HEAD >/dev/null; then > git diff-index --cached --quiet \ > -- > 1.6.1.2.390.gba743 > -- Shawn. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git 2009-02-11 16:26 ` [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Shawn O. Pearce @ 2009-02-11 16:53 ` Ted Pavlic 0 siblings, 0 replies; 16+ messages in thread From: Ted Pavlic @ 2009-02-11 16:53 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: git, gitster >> This patch uses "git rev-parse --is-inside-work-tree" to determine >> whether a "--work-tree=.." should be added to the "git diff". > > I think it makes more sense to just drop the work tree stuff from > the prompt if we aren't inside the work tree anymore. Meaning, > we should behave as though bash.showDirtyState is false. I see. At first, it seemed like it would be useful to know if the working directory was dirty even when you're within .git. However, I guess that's problematic when your working tree is in some completely unpredictable location. It's probably a bad idea to assume that work-tree=.. So you're right... there's a logical problem with having showDirtyState turned on when within .git. It should be disabled there. So I'll check for "git rev-parse --is-inside-git-dir" and disable showDirtyState appropriately. --Ted -- Ted Pavlic <ted@tedpavlic.com> Please visit my ALS association page: http://web.alsa.org/goto/tedpavlic My family appreciates your support in the fight to defeat ALS. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git 2009-02-11 15:37 ` [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Ted Pavlic 2009-02-11 16:18 ` [PATCH 4/3] completion: More fixes to prevent unbound variable errors Ted Pavlic 2009-02-11 16:26 ` [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Shawn O. Pearce @ 2009-02-11 16:56 ` Junio C Hamano 2009-02-11 17:20 ` Ted Pavlic 2 siblings, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2009-02-11 16:56 UTC (permalink / raw) To: Ted Pavlic; +Cc: spearce, git Ted Pavlic <ted@tedpavlic.com> writes: > The GIT_PS1_DIRTYSTATE support uses a "git diff" to see if a "*" should > be placed after the branch name. The "git diff" fails with an ugly error > if the user has just changed directory into GIT_DIR. > > This patch uses "git rev-parse --is-inside-work-tree" to determine > whether a "--work-tree=.." should be added to the "git diff". Why ".."? What prevents you from "cd .git/refs/heads"? Your "is-inside-work-tree" might be a good change, but if you were to spend a letter to notify the users, "Warning: You are inside GIT_DIR! This is something unusual, proceed with caution." is a lot more important notice to give them than "You seem to have unstaged changes" notice. You have at least three possible states: * You are not in git repository at all; * You are somewhere in $GIT_DIR, perhaps in a bare repository, perhaps a repository with a work tree. * You are inside a work tree. The first should be quiet, the second should say "Proceed with caution, any 'rm -f file' or 'edit file' you do here should be to recover from unusual repository corruptoin only; you are welcome to look but don't touch.", and the last one is Ok to say "You have unstaged changes." ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git 2009-02-11 16:56 ` Junio C Hamano @ 2009-02-11 17:20 ` Ted Pavlic 2009-02-11 18:01 ` Shawn O. Pearce 0 siblings, 1 reply; 16+ messages in thread From: Ted Pavlic @ 2009-02-11 17:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: spearce, git >> This patch uses "git rev-parse --is-inside-work-tree" to determine >> whether a "--work-tree=.." should be added to the "git diff". > > Why ".."? What prevents you from "cd .git/refs/heads"? It was my impression that --work-tree=.. was relative to the GIT_DIR. My tests seem to confirm that. Within the git work tree I do: echo "test">>COPYING then cd .git/refs/heads git --work=tree=.. diff and that produces a diff of COPYING showing the new line. ("man git" confused me about how WORK_TREE was supposed to work) > Your "is-inside-work-tree" might be a good change, but if you were to > spend a letter to notify the users, "Warning: You are inside GIT_DIR! This > is something unusual, proceed with caution." is a lot more important > notice to give them than "You seem to have unstaged changes" notice. Ok, so if "is-inside-git-dir" then send a special "!" flag (and no dirtyState flags)? Does that seem reasonable? Additionally, is it a good idea to echo the branch name when inside the git dir? That is, what does it "mean" to be on "master" when you're inside .git? > You have at least three possible states: > * You are not in git repository at all; > * You are somewhere in $GIT_DIR, perhaps in a bare repository, perhaps a > repository with a work tree. > * You are inside a work tree. It seems like (psuedocode)... if git rev-parse --is-inside-git-dir; then use '!' flag to indicate caution elif git rev-parse --is-inside-work-tree; then proceed as before (with '*' and '+' flags) else do nothing I think that handles those cases. No? Thanks -- --Ted -- Ted Pavlic <ted@tedpavlic.com> Please visit my ALS association page: http://web.alsa.org/goto/tedpavlic My family appreciates your support in the fight to defeat ALS. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git 2009-02-11 17:20 ` Ted Pavlic @ 2009-02-11 18:01 ` Shawn O. Pearce 0 siblings, 0 replies; 16+ messages in thread From: Shawn O. Pearce @ 2009-02-11 18:01 UTC (permalink / raw) To: Ted Pavlic; +Cc: Junio C Hamano, git Ted Pavlic <ted@tedpavlic.com> wrote: > >> You have at least three possible states: >> * You are not in git repository at all; >> * You are somewhere in $GIT_DIR, perhaps in a bare repository, perhaps a >> repository with a work tree. >> * You are inside a work tree. > > It seems like (psuedocode)... > > if git rev-parse --is-inside-git-dir; then > use '!' flag to indicate caution > > elif git rev-parse --is-inside-work-tree; then > proceed as before (with '*' and '+' flags) > > else > do nothing > > I think that handles those cases. No? Yes, that looks right to me. -- Shawn. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention 2009-02-11 15:37 ` [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention Ted Pavlic 2009-02-11 15:37 ` [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Ted Pavlic @ 2009-02-11 16:24 ` Shawn O. Pearce 2009-02-11 16:36 ` Ted Pavlic 2009-02-11 16:46 ` Junio C Hamano 2 siblings, 1 reply; 16+ messages in thread From: Shawn O. Pearce @ 2009-02-11 16:24 UTC (permalink / raw) To: Ted Pavlic; +Cc: git, gitster Ted Pavlic <ted@tedpavlic.com> wrote: > In the single case of: > > if [...] || [...] > > changed to: > > if test ... || test ... NAK. This script only runs in bash. bash supports [...]. The prevailing convention in the script is to use [...]. Only 4 tests inside of __git_ps1 use "test", the rest of the code is using [...]. I would agree to a test->[...] conversion patch as its fairly small, but not this one. Too large, too much code churn, no benefit. -- Shawn. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention 2009-02-11 16:24 ` [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention Shawn O. Pearce @ 2009-02-11 16:36 ` Ted Pavlic 2009-02-11 17:14 ` Junio C Hamano 0 siblings, 1 reply; 16+ messages in thread From: Ted Pavlic @ 2009-02-11 16:36 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: git, gitster > NAK. > > This script only runs in bash. bash supports [...]. The > prevailing convention in the script is to use [...]. Only > 4 tests inside of __git_ps1 use "test", the rest of the code > is using [...]. So this trumps Documentation/CodingGuidelines, which says: - We prefer "test" over "[ ... ]". ? Thanks -- Ted -- Ted Pavlic <ted@tedpavlic.com> Please visit my ALS association page: http://web.alsa.org/goto/tedpavlic My family appreciates your support in the fight to defeat ALS. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention 2009-02-11 16:36 ` Ted Pavlic @ 2009-02-11 17:14 ` Junio C Hamano 0 siblings, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2009-02-11 17:14 UTC (permalink / raw) To: Ted Pavlic; +Cc: Shawn O. Pearce, git Ted Pavlic <ted@tedpavlic.com> writes: >> NAK. >> >> This script only runs in bash. bash supports [...]. The >> prevailing convention in the script is to use [...]. Only >> 4 tests inside of __git_ps1 use "test", the rest of the code >> is using [...]. > > So this trumps Documentation/CodingGuidelines, which says: > > - We prefer "test" over "[ ... ]". > > ? No, this paragraph from Documentation/CodingGuidelines trumps you. As for more concrete guidelines, just imitate the existing code (this is a good guideline, no matter which project you are contributing to). It is always preferable to match the _local_ convention. New code added to git suite is expected to match the overall style of existing code. Modifications to existing code is expected to match the style the surrounding code already uses (even if it doesn't match the overall style of existing code). ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention 2009-02-11 15:37 ` [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention Ted Pavlic 2009-02-11 15:37 ` [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Ted Pavlic 2009-02-11 16:24 ` [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention Shawn O. Pearce @ 2009-02-11 16:46 ` Junio C Hamano 2 siblings, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2009-02-11 16:46 UTC (permalink / raw) To: Ted Pavlic; +Cc: spearce, git Ted Pavlic <ted@tedpavlic.com> writes: > In the single case of: > > if [...] || [...] > > changed to: > > if test ... || test ... In [0/3] you talked about "git convention", but please match the local convention, especially inside contrib/ area. That is, consistency of the style within the same file (and files in vicinity), is more important. > Also changed: > > if test ... > then > > to: > > if test ...; then The prevailing style in bash completion script is to write "then" on the same line as "if", so I think this is a good example of matching the local convention (if you are trying to match "git convention", "then" is written on the same column as "if" on a line by itself for readability, so this change is going backwards). ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] completion: For consistency, changed "git rev-parse" to __gitdir calls. 2009-02-11 15:37 ` [PATCH 1/3] completion: For consistency, changed "git rev-parse" to __gitdir calls Ted Pavlic 2009-02-11 15:37 ` [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention Ted Pavlic @ 2009-02-11 16:22 ` Shawn O. Pearce 1 sibling, 0 replies; 16+ messages in thread From: Shawn O. Pearce @ 2009-02-11 16:22 UTC (permalink / raw) To: Ted Pavlic; +Cc: git, gitster Ted Pavlic <ted@tedpavlic.com> wrote: > Signed-off-by: Ted Pavlic <ted@tedpavlic.com> > --- Acked-by: Shawn O. Pearce <spearce@spearce.org> > contrib/completion/git-completion.bash | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index f44f63c..6bbe09a 100755 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -80,7 +80,7 @@ __gitdir () > # returns text to add to bash PS1 prompt (includes branch name) > __git_ps1 () > { > - local g="$(git rev-parse --git-dir 2>/dev/null)" > + local g="$(__gitdir)" > if [ -n "$g" ]; then > local r > local b > @@ -1797,7 +1797,7 @@ _gitk () > __git_has_doubledash && return > > local cur="${COMP_WORDS[COMP_CWORD]}" > - local g="$(git rev-parse --git-dir 2>/dev/null)" > + local g="$(__gitdir)" > local merge="" > if [ -f $g/MERGE_HEAD ]; then > merge="--merge" > -- > 1.6.1.2.390.gba743 > -- Shawn. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2009-02-11 18:02 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-02-11 15:37 [PATCH 0/3] completion: Convention updates and DIRTYSTATE fix Ted Pavlic 2009-02-11 15:37 ` [PATCH 1/3] completion: For consistency, changed "git rev-parse" to __gitdir calls Ted Pavlic 2009-02-11 15:37 ` [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention Ted Pavlic 2009-02-11 15:37 ` [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Ted Pavlic 2009-02-11 16:18 ` [PATCH 4/3] completion: More fixes to prevent unbound variable errors Ted Pavlic 2009-02-11 16:28 ` Shawn O. Pearce 2009-02-11 16:26 ` [PATCH 3/3] completion: Prevents GIT_PS1_DIRTYSTATE from breaking when CWD is .git Shawn O. Pearce 2009-02-11 16:53 ` Ted Pavlic 2009-02-11 16:56 ` Junio C Hamano 2009-02-11 17:20 ` Ted Pavlic 2009-02-11 18:01 ` Shawn O. Pearce 2009-02-11 16:24 ` [PATCH 2/3] completion: Change "if [...]" to "if test ..." to match git convention Shawn O. Pearce 2009-02-11 16:36 ` Ted Pavlic 2009-02-11 17:14 ` Junio C Hamano 2009-02-11 16:46 ` Junio C Hamano 2009-02-11 16:22 ` [PATCH 1/3] completion: For consistency, changed "git rev-parse" to __gitdir calls Shawn O. Pearce
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).