Re: [PATCH v3 3/4] get rid of git submodule summary --for-status

2013-09-03 Thread Jens Lehmann 
Am 29.08.2013 21:54, schrieb Jens Lehmann:
 Am 29.08.2013 15:05, schrieb Matthieu Moy:
 The --for-status option was an undocumented option used only by
 wt-status.c, which inserted a header and commented out the output. We can
 achieve the same result within wt-status.c, without polluting the
 submodule command-line options.

 This will make it easier to disable the comments from wt-status.c later.
 
 Cool, thanks for implementing this!
 
 But unfortunately this change collides with bc/submodule-status-ignored
 (I added Brian to the CC) which is currently on its way to next. Your
 patch will break the fix in the second commit, because that's only
 enabled when the submodule script sees the --for-status option.
 
 A solution for that would be to rebase your patches on top of pu, drop
 the first two hunks of the change to git-submodule.sh and still pass
 the --for-status option to git-submodule.sh. This would move adding the
 comment characters into wt-status.c but will still enable the script to
 honor the ignore=all setting when called by status.

I think we should go that route, --for-status is an internal option and
nobody should rely on its behavior.

 Signed-off-by: Matthieu Moy matthieu@imag.fr
 ---
  git-submodule.sh | 17 +
  t/t7401-submodule-summary.sh | 13 -
  wt-status.c  | 29 +++--
  3 files changed, 28 insertions(+), 31 deletions(-)

 diff --git a/git-submodule.sh b/git-submodule.sh
 index 2979197..fccdec9 100755
 --- a/git-submodule.sh
 +++ b/git-submodule.sh
 @@ -965,7 +965,6 @@ set_name_rev () {
  #
  cmd_summary() {
  summary_limit=-1
 -for_status=
  diff_cmd=diff-index
  
  # parse $args after submodule ... summary.
 @@ -978,9 +977,6 @@ cmd_summary() {
  --files)
  files=$1
  ;;
 ---for-status)
 -for_status=$1
 -;;
  -n|--summary-limit)
  summary_limit=$2
  isnumber $summary_limit || usage

Please drop the two hunks above ...

 @@ -1149,18 +1145,7 @@ cmd_summary() {
  echo
  fi
  echo
 -done |
 -if test -n $for_status; then
 -if [ -n $files ]; then
 -gettextln Submodules changed but not updated: | git 
 stripspace -c
 -else
 -gettextln Submodule changes to be committed: | git 
 stripspace -c
 -fi
 -printf \n | git stripspace -c
 -git stripspace -c
 -else
 -cat
 -fi
 +done
  }
  #
  # List all submodules, prefixed with:
 diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
 index ac2434c..b435d03 100755
 --- a/t/t7401-submodule-summary.sh
 +++ b/t/t7401-submodule-summary.sh
 @@ -262,19 +262,6 @@ EOF
  test_cmp expected actual
  
  
 -test_expect_success '--for-status' 
 -git submodule summary --for-status HEAD^ actual 
 -test_i18ncmp actual - EOF
 -# Submodule changes to be committed:
 -#
 -# * sm1 $head6...000:
 -#
 -# * sm2 000...$head7 (2):
 -#Add foo9
 -#
 -EOF
 -
 -

... and just remove the #  from the expected output here. This
test can be removed when we use

  test_expect_success 'fail when using --files together with --cached' 
  test_must_fail git submodule summary --files --cached
  
 diff --git a/wt-status.c b/wt-status.c
 index 958a53c..d91661d 100644
 --- a/wt-status.c
 +++ b/wt-status.c
 @@ -665,6 +665,10 @@ static void wt_status_print_submodule_summary(struct 
 wt_status *s, int uncommitt
  char index[PATH_MAX];
  const char *env[] = { NULL, NULL };
  struct argv_array argv = ARGV_ARRAY_INIT;
 +struct strbuf cmd_stdout = STRBUF_INIT;
 +struct strbuf summary = STRBUF_INIT;
 +char *summary_content;
 +size_t len;
  
  sprintf(summary_limit, %d, s-submodule_summary);
  snprintf(index, sizeof(index), GIT_INDEX_FILE=%s, s-index_file);
 @@ -673,7 +677,6 @@ static void wt_status_print_submodule_summary(struct 
 wt_status *s, int uncommitt
  argv_array_push(argv, submodule);
  argv_array_push(argv, summary);
  argv_array_push(argv, uncommitted ? --files : --cached);
 -argv_array_push(argv, --for-status);

And the line above has to stay.

  argv_array_push(argv, --summary-limit);
  argv_array_push(argv, summary_limit);
  if (!uncommitted)
 @@ -685,9 +688,31 @@ static void wt_status_print_submodule_summary(struct 
 wt_status *s, int uncommitt
  sm_summary.git_cmd = 1;
  sm_summary.no_stdin = 1;
  fflush(s-fp);
 -sm_summary.out = dup(fileno(s-fp));/* run_command closes it */
 +sm_summary.out = -1;
 +
  run_command(sm_summary);
  argv_array_clear(argv);
 +
 +len = strbuf_read(cmd_stdout, sm_summary.out, 1024);
 +
 +/* prepend header, only if there's an actual output */
 +if (len) {
 +if

Re: [PATCH v3 3/4] get rid of git submodule summary --for-status

2013-09-01 Thread Jens Lehmann 
Am 31.08.2013 19:08, schrieb brian m. carlson:
 On Fri, Aug 30, 2013 at 10:08:53PM +0200, Jens Lehmann wrote:
 Am 30.08.2013 21:51, schrieb Jens Lehmann:
 Am 30.08.2013 21:40, schrieb Jens Lehmann:
 Am 29.08.2013 23:23, schrieb Matthieu Moy:
 Jens Lehmann jens.lehm...@web.de writes:

 Am 29.08.2013 15:05, schrieb Matthieu Moy:
 Because of the missing quotes around $for_status, it seems the test is
 unconditionnaly true:

 $ test -n t ; echo $?
 0
 $ test -n   ; echo $?
 0

 Right you are, I did not notice the missing  in my review. Looks like
 we also should add one or more tests making sure that submodule summary
 and status never honor the ignore settings.
 
 How do we want to handle this?  I can send a reroll and include some
 new tests, but if this code is going away, then there's no point.

A reroll would be great, as I think your patch is a bugfix that should
go in rather soonish no matter how we continue with the comment signs.
Two new tests (one for submodule summary and one for submodule status)
with both the global ignore setting and a submodule specific one set
to all showing no impact on the output would suffice (and trigger the
then also fixed missing  bug ;-).
--
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 v3 3/4] get rid of git submodule summary --for-status

2013-08-31 Thread brian m. carlson 
On Fri, Aug 30, 2013 at 10:08:53PM +0200, Jens Lehmann wrote:
 Am 30.08.2013 21:51, schrieb Jens Lehmann:
  Am 30.08.2013 21:40, schrieb Jens Lehmann:
  Am 29.08.2013 23:23, schrieb Matthieu Moy:
  Jens Lehmann jens.lehm...@web.de writes:
 
  Am 29.08.2013 15:05, schrieb Matthieu Moy:
  Because of the missing quotes around $for_status, it seems the test is
  unconditionnaly true:
 
  $ test -n t ; echo $?
  0
  $ test -n   ; echo $?
  0
 
 Right you are, I did not notice the missing  in my review. Looks like
 we also should add one or more tests making sure that submodule summary
 and status never honor the ignore settings.

How do we want to handle this?  I can send a reroll and include some
new tests, but if this code is going away, then there's no point.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: Digital signature

Re: [PATCH v3 3/4] get rid of git submodule summary --for-status

2013-08-30 Thread Jens Lehmann 
Am 30.08.2013 21:51, schrieb Jens Lehmann:
 Am 30.08.2013 21:40, schrieb Jens Lehmann:
 Am 29.08.2013 23:23, schrieb Matthieu Moy:
 Jens Lehmann jens.lehm...@web.de writes:

 Am 29.08.2013 15:05, schrieb Matthieu Moy:
 The --for-status option was an undocumented option used only by
 wt-status.c, which inserted a header and commented out the output. We can
 achieve the same result within wt-status.c, without polluting the
 submodule command-line options.

 This will make it easier to disable the comments from wt-status.c later.

 Cool, thanks for implementing this!

 But unfortunately this change collides with bc/submodule-status-ignored
 (I added Brian to the CC) which is currently on its way to next.

 Thanks for pointing that out. The patch looks buggy:

 Ok, I'll tak
 
 Sorry, I accidentally hit Send ... :-(
 
 Ok, I'll take a look and will comment on that soon.
 
 --- a/git-submodule.sh
 +++ b/git-submodule.sh
 @@ -1036,6 +1036,13 @@ cmd_summary() {
 do
 # Always show modules deleted or type-changed 
 (blob-module)
 test $status = D -o $status = T  echo $sm_path 
  continue
 +   # Respect the ignore setting for --for-status.
 +   if test -n $for_status
 +   then
 +   name=$(module_name $sm_path)
 +   ignore_config=$(get_submodule_config 
 $name ignore none)
 +   test $status != A -a $ignore_config = all 
  continue
 +   fi

 Because of the missing quotes around $for_status, it seems the test is
 unconditionnaly true:

 $ test -n t ; echo $?
 0
 $ test -n   ; echo $?
 0

Right you are, I did not notice the missing  in my review. Looks like
we also should add one or more tests making sure that submodule summary
and status never honor the ignore settings.
--
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 v3 3/4] get rid of git submodule summary --for-status

2013-08-30 Thread Jens Lehmann 
Am 30.08.2013 21:40, schrieb Jens Lehmann:
 Am 29.08.2013 23:23, schrieb Matthieu Moy:
 Jens Lehmann jens.lehm...@web.de writes:

 Am 29.08.2013 15:05, schrieb Matthieu Moy:
 The --for-status option was an undocumented option used only by
 wt-status.c, which inserted a header and commented out the output. We can
 achieve the same result within wt-status.c, without polluting the
 submodule command-line options.

 This will make it easier to disable the comments from wt-status.c later.

 Cool, thanks for implementing this!

 But unfortunately this change collides with bc/submodule-status-ignored
 (I added Brian to the CC) which is currently on its way to next.

 Thanks for pointing that out. The patch looks buggy:
 
 Ok, I'll tak

Sorry, I accidentally hit Send ... :-(

Ok, I'll take a look and will comment on that soon.

 --- a/git-submodule.sh
 +++ b/git-submodule.sh
 @@ -1036,6 +1036,13 @@ cmd_summary() {
 do
 # Always show modules deleted or type-changed 
 (blob-module)
 test $status = D -o $status = T  echo $sm_path 
  continue
 +   # Respect the ignore setting for --for-status.
 +   if test -n $for_status
 +   then
 +   name=$(module_name $sm_path)
 +   ignore_config=$(get_submodule_config $name 
 ignore none)
 +   test $status != A -a $ignore_config = all  
 continue
 +   fi

 Because of the missing quotes around $for_status, it seems the test is
 unconditionnaly true:

 $ test -n t ; echo $?
 0
 $ test -n   ; echo $?
 0

 This makes me wonder why the ignore configuration should be considered
 only with --for-status. Why not turn that into

Please don't. That changes the default behavior of submodule summary,
which never ignores any submodules. The ignore logic was added to core
git after commands like diff and status learned to check submodules for
modifications too. That was bad for people who used submodules to store
many and/or huge files in a way that wouldn't slow down diff or status,
as it slowed them down again. The ignore option allowed them to continue
using submodules for that purpose. They still need to have the submodule
script ignore the ignore settings, because running them is the point in
time they want to take the extra effort to look into those submodules
they normally ignore. And that's why the submodule totally lacks any
option to control the ignore behavior, which we would also have to add
if we would follow your proposal.

So I think it's either changing the default behavior of --for-status or
adding another option (--for-status-wo-comment or such) which will honor
the ignore setting only when called from status.

 --- a/git-submodule.sh
 +++ b/git-submodule.sh
 @@ -1036,6 +1036,13 @@ cmd_summary() {
 do
 # Always show modules deleted or type-changed 
 (blob-module)
 test $status = D -o $status = T  echo $sm_path 
  continue
 +# Respect the ignore setting
 +name=$(module_name $sm_path)
 +ignore_config=$(get_submodule_config $name ignore 
 none)
 +test $status != A -a $ignore_config = all  continue

 ?

 

--
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 v3 3/4] get rid of git submodule summary --for-status

2013-08-30 Thread Jens Lehmann 
Am 29.08.2013 23:23, schrieb Matthieu Moy:
 Jens Lehmann jens.lehm...@web.de writes:
 
 Am 29.08.2013 15:05, schrieb Matthieu Moy:
 The --for-status option was an undocumented option used only by
 wt-status.c, which inserted a header and commented out the output. We can
 achieve the same result within wt-status.c, without polluting the
 submodule command-line options.

 This will make it easier to disable the comments from wt-status.c later.

 Cool, thanks for implementing this!

 But unfortunately this change collides with bc/submodule-status-ignored
 (I added Brian to the CC) which is currently on its way to next.
 
 Thanks for pointing that out. The patch looks buggy:

Ok, I'll tak

 --- a/git-submodule.sh
 +++ b/git-submodule.sh
 @@ -1036,6 +1036,13 @@ cmd_summary() {
 do
 # Always show modules deleted or type-changed 
 (blob-module)
 test $status = D -o $status = T  echo $sm_path  
 continue
 +   # Respect the ignore setting for --for-status.
 +   if test -n $for_status
 +   then
 +   name=$(module_name $sm_path)
 +   ignore_config=$(get_submodule_config $name 
 ignore none)
 +   test $status != A -a $ignore_config = all  
 continue
 +   fi
 
 Because of the missing quotes around $for_status, it seems the test is
 unconditionnaly true:
 
 $ test -n t ; echo $?
 0
 $ test -n   ; echo $?
 0
 
 This makes me wonder why the ignore configuration should be considered
 only with --for-status. Why not turn that into
 
 --- a/git-submodule.sh
 +++ b/git-submodule.sh
 @@ -1036,6 +1036,13 @@ cmd_summary() {
 do
 # Always show modules deleted or type-changed 
 (blob-module)
 test $status = D -o $status = T  echo $sm_path  
 continue
 + # Respect the ignore setting
 + name=$(module_name $sm_path)
 + ignore_config=$(get_submodule_config $name ignore 
 none)
 + test $status != A -a $ignore_config = all  continue
 
 ?
 

--
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 v3 3/4] get rid of git submodule summary --for-status

2013-08-29 Thread Junio C Hamano 
Matthieu Moy matthieu@imag.fr writes:

 + /* prepend header, only if there's an actual output */
 + if (len) {
 + if (uncommitted)
 + strbuf_addstr(summary, _(Submodules changed but not 
 updated:));
 + else
 + strbuf_addstr(summary, _(Submodule changes to be 
 committed:));
 + strbuf_addstr(summary, \n\n);
 + }
 + strbuf_addbuf(summary, cmd_stdout);
 + strbuf_release(cmd_stdout);
 +
 + summary_content = strbuf_detach(summary, len);
 + strbuf_add_commented_lines(summary, summary_content, len);
 + free(summary_content);
 +
 + summary_content = strbuf_detach(summary, len);
 + fprintf(s-fp, summary_content);
 + free(summary_content);

This fprintf() looks bogus to me.  How about adding this on top?

 wt-status.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index d91661d..1f17652 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -710,9 +710,8 @@ static void wt_status_print_submodule_summary(struct 
wt_status *s, int uncommitt
strbuf_add_commented_lines(summary, summary_content, len);
free(summary_content);
 
-   summary_content = strbuf_detach(summary, len);
-   fprintf(s-fp, summary_content);
-   free(summary_content);
+   fputs(summary.buf, s-fp);
+   strbuf_release(summary);
 }
 
 static void wt_status_print_other(struct wt_status *s,
--
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 v3 3/4] get rid of git submodule summary --for-status

2013-08-29 Thread Matthieu Moy 
Junio C Hamano gits...@pobox.com writes:

 Matthieu Moy matthieu@imag.fr writes:

 +/* prepend header, only if there's an actual output */
 +if (len) {
 +if (uncommitted)
 +strbuf_addstr(summary, _(Submodules changed but not 
 updated:));
 +else
 +strbuf_addstr(summary, _(Submodule changes to be 
 committed:));
 +strbuf_addstr(summary, \n\n);
 +}
 +strbuf_addbuf(summary, cmd_stdout);
 +strbuf_release(cmd_stdout);
 +
 +summary_content = strbuf_detach(summary, len);
 +strbuf_add_commented_lines(summary, summary_content, len);
 +free(summary_content);
 +
 +summary_content = strbuf_detach(summary, len);
 +fprintf(s-fp, summary_content);
 +free(summary_content);

 This fprintf() looks bogus to me.

Oops, indeed. I forgot the %s.

 How about adding this on top?

Your solution is better, yes.

  wt-status.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

 diff --git a/wt-status.c b/wt-status.c
 index d91661d..1f17652 100644
 --- a/wt-status.c
 +++ b/wt-status.c
 @@ -710,9 +710,8 @@ static void wt_status_print_submodule_summary(struct 
 wt_status *s, int uncommitt
   strbuf_add_commented_lines(summary, summary_content, len);
   free(summary_content);
  
 - summary_content = strbuf_detach(summary, len);
 - fprintf(s-fp, summary_content);
 - free(summary_content);
 + fputs(summary.buf, s-fp);
 + strbuf_release(summary);
  }
  
  static void wt_status_print_other(struct wt_status *s,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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