Re: [PATCH] submodule: add verbose mode for add/update

2014-03-12 Thread Eric Sunshine
On Mar 12, 2014, at 2:38 AM, Orgad Shaneh org...@gmail.com wrote:
 Executes checkout without -q
 —

Missing sign-off. See Documentation/SubmittingPatches.

Your patch is badly whitespace-damaged, as if it was pasted into your email 
client. “git send-email” can avoid this problem.

As I’m not a submodule user, I won’t review the content of the patch other than 
to say that such a change should be accompanied by documentation update 
(Documentation/git-submodule.txt) and additional tests.

 git-submodule.sh | 24 +++-
 1 file changed, 19 insertions(+), 5 deletions(-)
 
 diff --git a/git-submodule.sh b/git-submodule.sh
 index a33f68d..5c4e057 100755
 --- a/git-submodule.sh
 +++ b/git-submodule.sh
 @@ -5,11 +5,11 @@
 # Copyright (c) 2007 Lars Hjemli
 
 dashless=$(basename $0 | sed -e 's/-/ /')
 -USAGE=[--quiet] add [-b branch] [-f|--force] [--name name]
 [--reference repository] [--] repository [path]
 +USAGE=[--quiet] add [-b branch] [-f|--force] [--name name]
 [--reference repository] [-v|--verbose] [--] repository [path]
 or: $dashless [--quiet] status [--cached] [--recursive] [--] [path...]
 or: $dashless [--quiet] init [--] [path...]
 or: $dashless [--quiet] deinit [-f|--force] [--] path...
 -   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
 [-f|--force] [--rebase] [--reference repository] [--merge]
 [--recursive] [--] [path...]
 +   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
 [-f|--force] [--rebase] [--reference repository] [--merge]
 [--recursive] [-v|--verbose] [--] [path...]
 or: $dashless [--quiet] summary [--cached|--files]
 [--summary-limit n] [commit] [--] [path...]
 or: $dashless [--quiet] foreach [--recursive] command
 or: $dashless [--quiet] sync [--recursive] [--] [path...]
 @@ -319,12 +319,16 @@ module_clone()
 rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
 (
 clear_local_git_env
 + if test -z $verbose
 + then
 + subquiet=-q
 + fi
 cd $sm_path 
 GIT_WORK_TREE=. git config core.worktree $rel/$b 
 # ash fails to wordsplit ${local_branch:+-B $local_branch...}
 case $local_branch in
 - '') git checkout -f -q ${start_point:+$start_point} ;;
 - ?*) git checkout -f -q -B $local_branch ${start_point:+$start_point} ;;
 + '') git checkout -f $subquiet ${start_point:+$start_point} ;;
 + ?*) git checkout -f $subquiet -B $local_branch
 ${start_point:+$start_point} ;;
 esac
 ) || die $(eval_gettext Unable to setup cloned submodule '\$sm_path')
 }
 @@ -380,6 +384,9 @@ cmd_add()
 --depth=*)
 depth=$1
 ;;
 + -v|--verbose)
 + verbose=1
 + ;;
 --)
 shift
 break
 @@ -786,6 +793,9 @@ cmd_update()
 --depth=*)
 depth=$1
 ;;
 + -v|--verbose)
 + verbose=1
 + ;;
 --)
 shift
 break
 @@ -913,7 +923,11 @@ Maybe you want to use 'update --init'?)
 must_die_on_failure=
 case $update_module in
 checkout)
 - command=git checkout $subforce -q
 + if test -z $verbose
 + then
 + subquiet=-q
 + fi
 + command=git checkout $subforce $subquiet
 die_msg=$(eval_gettext Unable to checkout '\$sha1' in submodule
 path '\$displaypath')
 say_msg=$(eval_gettext Submodule path '\$displaypath': checked out
 '\$sha1')
 ;;
 -- 
 1.9.0.msysgit.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] submodule: add verbose mode for add/update

2014-03-12 Thread Jens Lehmann
Am 12.03.2014 14:42, schrieb Orgad Shaneh:
 From: Orgad Shaneh org...@gmail.com

You don't need the line above when you are the sender ;-)

 Executes checkout without -q

That's a bit terse. What about:

Add the verbose flag to add and update which displays the
 progress of the actual submodule checkout when given. This
 is useful for checkouts that take a long time, as the user
 can then see the progress.

 Signed-off-by: Orgad Shaneh org...@gmail.com
 ---
  Documentation/git-submodule.txt |  7 +--
  git-submodule.sh| 24 +++-
  t/t7406-submodule-update.sh |  9 +
  3 files changed, 33 insertions(+), 7 deletions(-)
 
 diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
 index 21cb59a..1867e94 100644
 --- a/Documentation/git-submodule.txt
 +++ b/Documentation/git-submodule.txt
 @@ -10,13 +10,13 @@ SYNOPSIS
  
  [verse]
  'git submodule' [--quiet] add [-b branch] [-f|--force] [--name name]
 -   [--reference repository] [--depth depth] [--] repository 
 [path]
 +   [--reference repository] [--depth depth] [-v|--verbose] [--] 
 repository [path]
  'git submodule' [--quiet] status [--cached] [--recursive] [--] [path...]
  'git submodule' [--quiet] init [--] [path...]
  'git submodule' [--quiet] deinit [-f|--force] [--] path...
  'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
 [-f|--force] [--rebase|--merge|--checkout] [--reference 
 repository]
 -   [--depth depth] [--recursive] [--] [path...]
 +   [--depth depth] [--recursive] [-v|--verbose] [--] [path...]
  'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) 
 n]
 [commit] [--] [path...]
  'git submodule' [--quiet] foreach [--recursive] command
 @@ -363,6 +363,9 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` 
 options carefully.
   clone with a history truncated to the specified number of revisions.
   See linkgit:git-clone[1]
  
 +--verbose::
 +  This option is valid for add and update commands. Show output of
 +  checkout.

The above looks whitespace-damaged, you should use TABs here to
indent (just like the other options do).

  path...::
   Paths to submodule(s). When specified this will restrict the command
 diff --git a/git-submodule.sh b/git-submodule.sh
 index a33f68d..5c4e057 100755
 --- a/git-submodule.sh
 +++ b/git-submodule.sh
 @@ -5,11 +5,11 @@
  # Copyright (c) 2007 Lars Hjemli
  
  dashless=$(basename $0 | sed -e 's/-/ /')
 -USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] [--reference 
 repository] [--] repository [path]
 +USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] [--reference 
 repository] [-v|--verbose] [--] repository [path]
 or: $dashless [--quiet] status [--cached] [--recursive] [--] [path...]
 or: $dashless [--quiet] init [--] [path...]
 or: $dashless [--quiet] deinit [-f|--force] [--] path...
 -   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
 [-f|--force] [--rebase] [--reference repository] [--merge] [--recursive] 
 [--] [path...]
 +   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
 [-f|--force] [--rebase] [--reference repository] [--merge] [--recursive] 
 [-v|--verbose] [--] [path...]
 or: $dashless [--quiet] summary [--cached|--files] [--summary-limit n] 
 [commit] [--] [path...]
 or: $dashless [--quiet] foreach [--recursive] command
 or: $dashless [--quiet] sync [--recursive] [--] [path...]
 @@ -319,12 +319,16 @@ module_clone()
   rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
   (
   clear_local_git_env
 + if test -z $verbose
 + then
 + subquiet=-q
 + fi
   cd $sm_path 
   GIT_WORK_TREE=. git config core.worktree $rel/$b 
   # ash fails to wordsplit ${local_branch:+-B $local_branch...}
   case $local_branch in
 - '') git checkout -f -q ${start_point:+$start_point} ;;
 - ?*) git checkout -f -q -B $local_branch 
 ${start_point:+$start_point} ;;
 + '') git checkout -f $subquiet ${start_point:+$start_point} ;;
 + ?*) git checkout -f $subquiet -B $local_branch 
 ${start_point:+$start_point} ;;

Wouldn't it be better to use the ${subquiet:+$subquiet} notation
here like the other optional arguments do? After all the subquiet
isn't always set.

   esac
   ) || die $(eval_gettext Unable to setup cloned submodule 
 '\$sm_path')
  }
 @@ -380,6 +384,9 @@ cmd_add()
   --depth=*)
   depth=$1
   ;;
 + -v|--verbose)
 + verbose=1
 + ;;
   --)
   shift
   break
 @@ -786,6 +793,9 @@ cmd_update()
   --depth=*)
   depth=$1
   ;;
 + -v|--verbose)
 + 

Re: [PATCH] submodule: add verbose mode for add/update

2014-03-12 Thread Orgad Shaneh
On Wed, Mar 12, 2014 at 6:15 PM, Jens Lehmann jens.lehm...@web.de wrote:

 Am 12.03.2014 14:42, schrieb Orgad Shaneh:
  From: Orgad Shaneh org...@gmail.com

 You don't need the line above when you are the sender ;-)

  Executes checkout without -q

 That's a bit terse. What about:

 Add the verbose flag to add and update which displays the
  progress of the actual submodule checkout when given. This
  is useful for checkouts that take a long time, as the user
  can then see the progress.

  Signed-off-by: Orgad Shaneh org...@gmail.com
  ---
   Documentation/git-submodule.txt |  7 +--
   git-submodule.sh| 24 +++-
   t/t7406-submodule-update.sh |  9 +
   3 files changed, 33 insertions(+), 7 deletions(-)
 
  diff --git a/Documentation/git-submodule.txt 
  b/Documentation/git-submodule.txt
  index 21cb59a..1867e94 100644
  --- a/Documentation/git-submodule.txt
  +++ b/Documentation/git-submodule.txt
  @@ -10,13 +10,13 @@ SYNOPSIS
   
   [verse]
   'git submodule' [--quiet] add [-b branch] [-f|--force] [--name name]
  -   [--reference repository] [--depth depth] [--] repository 
  [path]
  +   [--reference repository] [--depth depth] [-v|--verbose] 
  [--] repository [path]
   'git submodule' [--quiet] status [--cached] [--recursive] [--] [path...]
   'git submodule' [--quiet] init [--] [path...]
   'git submodule' [--quiet] deinit [-f|--force] [--] path...
   'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
  [-f|--force] [--rebase|--merge|--checkout] [--reference 
  repository]
  -   [--depth depth] [--recursive] [--] [path...]
  +   [--depth depth] [--recursive] [-v|--verbose] [--] [path...]
   'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) 
  n]
  [commit] [--] [path...]
   'git submodule' [--quiet] foreach [--recursive] command
  @@ -363,6 +363,9 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` 
  options carefully.
clone with a history truncated to the specified number of revisions.
See linkgit:git-clone[1]
 
  +--verbose::
  +  This option is valid for add and update commands. Show output of
  +  checkout.

 The above looks whitespace-damaged, you should use TABs here to
 indent (just like the other options do).

   path...::
Paths to submodule(s). When specified this will restrict the command
  diff --git a/git-submodule.sh b/git-submodule.sh
  index a33f68d..5c4e057 100755
  --- a/git-submodule.sh
  +++ b/git-submodule.sh
  @@ -5,11 +5,11 @@
   # Copyright (c) 2007 Lars Hjemli
 
   dashless=$(basename $0 | sed -e 's/-/ /')
  -USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] 
  [--reference repository] [--] repository [path]
  +USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] 
  [--reference repository] [-v|--verbose] [--] repository [path]
  or: $dashless [--quiet] status [--cached] [--recursive] [--] [path...]
  or: $dashless [--quiet] init [--] [path...]
  or: $dashless [--quiet] deinit [-f|--force] [--] path...
  -   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
  [-f|--force] [--rebase] [--reference repository] [--merge] [--recursive] 
  [--] [path...]
  +   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
  [-f|--force] [--rebase] [--reference repository] [--merge] [--recursive] 
  [-v|--verbose] [--] [path...]
  or: $dashless [--quiet] summary [--cached|--files] [--summary-limit 
  n] [commit] [--] [path...]
  or: $dashless [--quiet] foreach [--recursive] command
  or: $dashless [--quiet] sync [--recursive] [--] [path...]
  @@ -319,12 +319,16 @@ module_clone()
rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
(
clear_local_git_env
  + if test -z $verbose
  + then
  + subquiet=-q
  + fi
cd $sm_path 
GIT_WORK_TREE=. git config core.worktree $rel/$b 
# ash fails to wordsplit ${local_branch:+-B 
  $local_branch...}
case $local_branch in
  - '') git checkout -f -q ${start_point:+$start_point} ;;
  - ?*) git checkout -f -q -B $local_branch 
  ${start_point:+$start_point} ;;
  + '') git checkout -f $subquiet ${start_point:+$start_point} 
  ;;
  + ?*) git checkout -f $subquiet -B $local_branch 
  ${start_point:+$start_point} ;;

 Wouldn't it be better to use the ${subquiet:+$subquiet} notation
 here like the other optional arguments do? After all the subquiet
 isn't always set.

esac
) || die $(eval_gettext Unable to setup cloned submodule 
  '\$sm_path')
   }
  @@ -380,6 +384,9 @@ cmd_add()
--depth=*)
depth=$1
;;
  + -v|--verbose)
  + verbose=1
  + ;;
--)

Re: [PATCH] submodule: add verbose mode for add/update

2014-03-12 Thread Junio C Hamano
Orgad Shaneh org...@gmail.com writes:

 +--verbose::
 + This option is valid for add and update commands. Display the progress
 + of the actual submodule checkout.

Hmm, is the valid for add and update part we want to keep?  I do
not think it is a crime if some other subcommand accepted --verbose
option but its output under verbose mode and normal mode happened to
be the same.

I doubt it would take a lot of imagination to see that people would
want to see git submodule status --verbose to get richer output,
and at that point, progress of checkout as part of the description
of the --verbose option does not make any sense.  Perhaps the
second part that is specific to add and update subcommands
should move to the description of these two subcommands?

I dunno.

 diff --git a/git-submodule.sh b/git-submodule.sh
 index a33f68d..e1df2c8 100755
 --- a/git-submodule.sh
 +++ b/git-submodule.sh
 @@ -5,11 +5,11 @@
  # Copyright (c) 2007 Lars Hjemli
  
  dashless=$(basename $0 | sed -e 's/-/ /')
 -USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] [--reference 
 repository] [--] repository [path]
 +USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] [--reference 
 repository] [-v|--verbose] [--] repository [path]
 or: $dashless [--quiet] status [--cached] [--recursive] [--] [path...]
 or: $dashless [--quiet] init [--] [path...]
 or: $dashless [--quiet] deinit [-f|--force] [--] path...
 -   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
 [-f|--force] [--rebase] [--reference repository] [--merge] [--recursive] 
 [--] [path...]
 +   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
 [-f|--force] [--rebase] [--reference repository] [--merge] [--recursive] 
 [-v|--verbose] [--] [path...]
 or: $dashless [--quiet] summary [--cached|--files] [--summary-limit n] 
 [commit] [--] [path...]
 or: $dashless [--quiet] foreach [--recursive] command
 or: $dashless [--quiet] sync [--recursive] [--] [path...]
 @@ -319,12 +319,16 @@ module_clone()
   rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
   (
   clear_local_git_env
 + if test -z $verbose
 + then
 + subquiet=-q
 + fi
   cd $sm_path 
   GIT_WORK_TREE=. git config core.worktree $rel/$b 
   # ash fails to wordsplit ${local_branch:+-B $local_branch...}
   case $local_branch in
 - '') git checkout -f -q ${start_point:+$start_point} ;;
 - ?*) git checkout -f -q -B $local_branch 
 ${start_point:+$start_point} ;;
 + '') git checkout -f ${subquiet:+$subquiet} 
 ${start_point:+$start_point} ;;
 + ?*) git checkout -f ${subquiet:+$subquiet} -B $local_branch 
 ${start_point:+$start_point} ;;
   esac
   ) || die $(eval_gettext Unable to setup cloned submodule 
 '\$sm_path')
  }
 @@ -380,6 +384,9 @@ cmd_add()
   --depth=*)
   depth=$1
   ;;
 + -v|--verbose)
 + verbose=1
 + ;;

Compare $depth and $verbose, and think what would happen if the end
user had an environment variable whose name happened to be $depth or
$verbose.  Does this script misbehave under such a stray $verbose?
What does the existing script do to prevent it from misbehaving when
a stray $depth exists in the environment?
--
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] submodule: add verbose mode for add/update

2013-04-10 Thread Jens Lehmann
Am 10.04.2013 20:24, schrieb Orgad Shaneh:
 Executes checkout without -q

Nice, looks like you picked the proposal I made last September:
  http://permalink.gmane.org/gmane.comp.version-control.git/204747

The change is looking good, but you still need to document the
new option in Documentation/git-submodule.txt too please.

And the commit message is still too short, as I said in that
other thread:

On Tue, Sep 4, 2012 at 6:28 PM, Jens Lehmann jens.lehm...@web.de wrote:
 Before the Signed-off-by is the place where you should have
 explained why this would be a worthwhile change ;-)

And you answered to that with something that would really make
sense as first part of the commit message, because you explain
*why* you do that change:

Am 05.09.2012 13:42, schrieb Orgad and Raizel Shaneh:
 When I run 'git submodule update' I don't expect to be in the dark
 until the submodule/s finishes checkout, this sometimes can take a
 significant amount of time and feedback is expected.

Another paragraph after that should explain *how* you do it.

So what about the following as commit message:
--
When 'git submodule add/update' is run there is no output during
checkout. This can take a significant amount of time and it would
be nice if user could enable some feedback to see what's going on.

Add the -v/--verbose option to both add and update which suppresses
the -q normally given to checkout so the user sees progress output
from the checkout command.

Your Signed-off-by goes here
--

I'm looking forward to your next iteration.

 ---
  git-submodule.sh |   24 +++-
  1 file changed, 19 insertions(+), 5 deletions(-)
 
 diff --git a/git-submodule.sh b/git-submodule.sh
 index 79bfaac..f7964ad 100755
 --- a/git-submodule.sh
 +++ b/git-submodule.sh
 @@ -5,11 +5,11 @@
  # Copyright (c) 2007 Lars Hjemli
  
  dashless=$(basename $0 | sed -e 's/-/ /')
 -USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] [--reference 
 repository] [--] repository [path]
 +USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] [--reference 
 repository] [-v|--verbose] [--] repository [path]
 or: $dashless [--quiet] status [--cached] [--recursive] [--] [path...]
 or: $dashless [--quiet] init [--] [path...]
 or: $dashless [--quiet] deinit [-f|--force] [--] path...
 -   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
 [-f|--force] [--rebase] [--reference repository] [--merge] [--recursive] 
 [--] [path...]
 +   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
 [-f|--force] [--rebase] [--reference repository] [--merge] [--recursive] 
 [-v|--verbose] [--] [path...]
 or: $dashless [--quiet] summary [--cached|--files] [--summary-limit n] 
 [commit] [--] [path...]
 or: $dashless [--quiet] foreach [--recursive] command
 or: $dashless [--quiet] sync [--recursive] [--] [path...]
 @@ -309,6 +309,9 @@ cmd_add()
   custom_name=$2
   shift
   ;;
 + -v|--verbose)
 + VERBOSE=1
 + ;;
   --)
   shift
   break
 @@ -408,11 +411,15 @@ Use -f if you really want to add it. 2
   module_clone $sm_path $sm_name $realrepo $reference || 
 exit
   (
   clear_local_git_env
 + if test -z $VERBOSE
 + then
 + subquiet=-q
 + fi
   cd $sm_path 
   # ash fails to wordsplit ${branch:+-b $branch...}
   case $branch in
 - '') git checkout -f -q ;;
 - ?*) git checkout -f -q -B $branch origin/$branch ;;
 + '') git checkout -f $subquiet ;;
 + ?*) git checkout -f $subquiet -B $branch 
 origin/$branch ;;
   esac
   ) || die $(eval_gettext Unable to checkout submodule 
 '\$sm_path')
   fi
 @@ -676,6 +683,9 @@ cmd_update()
   --checkout)
   update=checkout
   ;;
 + -v|--verbose)
 + VERBOSE=1
 + ;;
   --)
   shift
   break
 @@ -799,7 +809,11 @@ Maybe you want to use 'update --init'?)
   must_die_on_failure=yes
   ;;
   *)
 - command=git checkout $subforce -q
 + if test -z $VERBOSE
 + then
 + subquiet=-q
 + fi
 + command=git checkout $subforce $subquiet
   die_msg=$(eval_gettext Unable to