* [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 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
* 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 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 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 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 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 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 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 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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.