regression in output of git-pull --rebase --recurse-submodules=yes --quiet

regression in output of git-pull --rebase --recurse-submodules=yes --quiet

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 1155 bytes --]

Somewhere between 2.13.6 & 2.14.1 there's an output regression. I
haven't done a bisect to trace it down further yet.

Specifically, --rebase --recurse-submodules=yes seems to cause --quiet
to not be effective anymore.

Full commandline:
$ git pull --rebase --recurse-submodules --quiet

In 2.13.6, there is no output, it's quiet as expect.

In 2.14.1, you get:
HEAD is up to date.
Submodule path '_data/news': rebased into 'a50b763c338161b4621d23e9fa5cd6e11455d6ca'
HEAD is up to date.
Submodule path 'glep': rebased into 'e1f100ec3ba44ab1672d61cabf4690b355e46158'

Steps to reproduction:
1. git clone --recurse-submodules \
 https://anongit.gentoo.org/git/sites/www.git
2. cd www
3. git submodule foreach --quiet git pull --quiet origin master
4. git pull --rebase --recurse-submodules=yes --quiet

Repeat step 4 for repeated bug output.
If you drop the --rebase, then you need to re-run step 3 first.

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 1113 bytes --]

[PATCH] builtin/pull: respect verbosity settings in submodules

[Stefan Beller]

In a6d7eb2c7a (pull: optionally rebase submodules (remote submodule
changes only), 2017-06-23), we taught Git how to rebase submodules in
a pull. However we missed to pass on the verbosity settings.

Reported-by: Robin H. Johnson <robbat2@gentoo.org>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/pull.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index 511dbbe0f6..1876271af9 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -574,6 +574,7 @@ static int rebase_submodules(void)
 	cp.no_stdin = 1;
 	argv_array_pushl(&cp.args, "submodule", "update",
 				   "--recursive", "--rebase", NULL);
+	argv_push_verbosity(&cp.args);
 
 	return run_command(&cp);
 }
@@ -586,6 +587,7 @@ static int update_submodules(void)
 	cp.no_stdin = 1;
 	argv_array_pushl(&cp.args, "submodule", "update",
 				   "--recursive", "--checkout", NULL);
+	argv_push_verbosity(&cp.args);
 
 	return run_command(&cp);
 }
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH] builtin/pull: respect verbosity settings in submodules

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> In a6d7eb2c7a (pull: optionally rebase submodules (remote submodule
> changes only), 2017-06-23), we taught Git how to rebase submodules in
> a pull. However we missed to pass on the verbosity settings.

Makes sense.  Thanks.

>
> Reported-by: Robin H. Johnson <robbat2@gentoo.org>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/pull.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 511dbbe0f6..1876271af9 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -574,6 +574,7 @@ static int rebase_submodules(void)
>  	cp.no_stdin = 1;
>  	argv_array_pushl(&cp.args, "submodule", "update",
>  				   "--recursive", "--rebase", NULL);
> +	argv_push_verbosity(&cp.args);
>  
>  	return run_command(&cp);
>  }
> @@ -586,6 +587,7 @@ static int update_submodules(void)
>  	cp.no_stdin = 1;
>  	argv_array_pushl(&cp.args, "submodule", "update",
>  				   "--recursive", "--checkout", NULL);
> +	argv_push_verbosity(&cp.args);
>  
>  	return run_command(&cp);
>  }

regression AGAIN in output of git-pull --rebase --recurse-submodules=yes --quiet

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 2334 bytes --]

A year ago, I raised <robbat2-20190410T062730-540884809Z@orbis-terrarum.net> as an issue,
which lead to commit commit a56771a668dd4963675914bc5da0e1e015952dae.

The exact same workload somewhere between 2.18.0 and 2.19.0 has caused
the message to come back. I noticed it first in a 2.18.0->2.21.0
upgrade, and did a partial bisect based on tags to trace it to 2.19.0.

===
$ git submodule foreach --quiet git pull origin master --quiet >/dev/null
From git://anongit.gentoo.org/data/gentoo-news
 * branch            master     -> FETCH_HEAD
From git://anongit.gentoo.org/data/glep
 * branch            master     -> FETCH_HEAD
===

I suspect it was a result of:
ea27893a65cc41cad2710466aa6a58866ff22f1e Merge branch 'pc/submodule-helper-foreach'

But I haven't done a full bisect to prove it yet.

On Sat, Jan 20, 2018 at 05:57:29AM +0000, Robin H. Johnson wrote:
> Somewhere between 2.13.6 & 2.14.1 there's an output regression. I
> haven't done a bisect to trace it down further yet.
> 
> Specifically, --rebase --recurse-submodules=yes seems to cause --quiet
> to not be effective anymore.
> 
> Full commandline:
> $ git pull --rebase --recurse-submodules --quiet
> 
> In 2.13.6, there is no output, it's quiet as expect.
> 
> In 2.14.1, you get:
> HEAD is up to date.
> Submodule path '_data/news': rebased into 'a50b763c338161b4621d23e9fa5cd6e11455d6ca'
> HEAD is up to date.
> Submodule path 'glep': rebased into 'e1f100ec3ba44ab1672d61cabf4690b355e46158'
> 
> Steps to reproduction:
> 1. git clone --recurse-submodules \
>  https://anongit.gentoo.org/git/sites/www.git
> 2. cd www
> 3. git submodule foreach --quiet git pull --quiet origin master
> 4. git pull --rebase --recurse-submodules=yes --quiet
> 
> Repeat step 4 for repeated bug output.
> If you drop the --rebase, then you need to re-run step 3 first.
> 
> -- 
> Robin Hugh Johnson
> Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
> E-Mail   : robbat2@gentoo.org
> GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
> GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136



-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1113 bytes --]

Re: regression AGAIN in output of git-pull --rebase --recurse-submodules=yes --quiet

[Duy Nguyen]

On Wed, Apr 10, 2019 at 06:41:05AM +0000, Robin H. Johnson wrote:
> A year ago, I raised <robbat2-20190410T062730-540884809Z@orbis-terrarum.net> as an issue,
> which lead to commit commit a56771a668dd4963675914bc5da0e1e015952dae.
> 
> The exact same workload somewhere between 2.18.0 and 2.19.0 has caused
> the message to come back. I noticed it first in a 2.18.0->2.21.0
> upgrade, and did a partial bisect based on tags to trace it to 2.19.0.
> 
> ===
> $ git submodule foreach --quiet git pull origin master --quiet >/dev/null
> From git://anongit.gentoo.org/data/gentoo-news
>  * branch            master     -> FETCH_HEAD
> From git://anongit.gentoo.org/data/glep
>  * branch            master     -> FETCH_HEAD
> ===

If you run this with GIT_TRACE=1, you can see that --quiet is passed
to submodule--helper correctly.

trace: built-in: git submodule--helper foreach --quiet git pull --quiet origin master

The problem here is the option parser of this command would try to
parse all options, so it considers both --quiet the same thing and are
to tell "submodule--foreach" to be quiet, the second --quiet is not
part of the "git pull" command anymore.

So the fix would be to pass "--" to stop option parsing.
submodule--helper should not parse options it does not understand
anyway. Something like this should work.

-- 8< --
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6bcc4f1bd7..6394222628 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -571,7 +571,7 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
 	};
 
 	argc = parse_options(argc, argv, prefix, module_foreach_options,
-			     git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);
+			     git_submodule_helper_usage, 0);
 
 	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
 		return 1;
diff --git a/git-submodule.sh b/git-submodule.sh
index 2c0fb6d723..a967b2890d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -346,7 +346,7 @@ cmd_foreach()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@"
 }
 
 #
-- 8< --

I'm a bit reluctant to follow up with a proper patch because I can't
digest the t5572-submodule-pull.sh tests. And we definitely need to
add a test case about --quiet to make sure it won't happen again.
--
Duy

> 
> I suspect it was a result of:
> ea27893a65cc41cad2710466aa6a58866ff22f1e Merge branch 'pc/submodule-helper-foreach'
> 
> But I haven't done a full bisect to prove it yet.
> 
> On Sat, Jan 20, 2018 at 05:57:29AM +0000, Robin H. Johnson wrote:
> > Somewhere between 2.13.6 & 2.14.1 there's an output regression. I
> > haven't done a bisect to trace it down further yet.
> > 
> > Specifically, --rebase --recurse-submodules=yes seems to cause --quiet
> > to not be effective anymore.
> > 
> > Full commandline:
> > $ git pull --rebase --recurse-submodules --quiet
> > 
> > In 2.13.6, there is no output, it's quiet as expect.
> > 
> > In 2.14.1, you get:
> > HEAD is up to date.
> > Submodule path '_data/news': rebased into 'a50b763c338161b4621d23e9fa5cd6e11455d6ca'
> > HEAD is up to date.
> > Submodule path 'glep': rebased into 'e1f100ec3ba44ab1672d61cabf4690b355e46158'
> > 
> > Steps to reproduction:
> > 1. git clone --recurse-submodules \
> >  https://anongit.gentoo.org/git/sites/www.git
> > 2. cd www
> > 3. git submodule foreach --quiet git pull --quiet origin master
> > 4. git pull --rebase --recurse-submodules=yes --quiet
> > 
> > Repeat step 4 for repeated bug output.
> > If you drop the --rebase, then you need to re-run step 3 first.
> > 
> > -- 
> > Robin Hugh Johnson
> > Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
> > E-Mail   : robbat2@gentoo.org
> > GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
> > GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136
> 
> 
> 
> -- 
> Robin Hugh Johnson
> Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
> E-Mail   : robbat2@gentoo.org
> GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
> GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

Re: regression AGAIN in output of git-pull --rebase --recurse-submodules=yes --quiet

[Robin H. Johnson]

[-- Attachment #1.1: Type: text/plain, Size: 1260 bytes --]

On Wed, Apr 10, 2019 at 06:18:35PM +0700, Duy Nguyen wrote:
> ...

Thanks, I tested, and had good results in almost all of my tests.

Almost all: config setting of 'pull.rebase=preserve' 
===
$ git submodule foreach --quiet git pull --quiet origin master >/dev/null
Successfully rebased and updated detached HEAD.
Successfully rebased and updated detached HEAD.
$ git pull --rebase --recurse-submodules=yes --quiet >/dev/null
$
===
Looking at git-rebase--preserve-merges.sh for this message, I think that
should be a separate patch to make it respect --quiet.

> -- 8< --
(snip patch, please add my DCO signed-off-by)
Tested-by: Robin H. Johnson <robbat2@gentoo.org>
Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
> -- 8< --
> 
> I'm a bit reluctant to follow up with a proper patch because I can't
> digest the t5572-submodule-pull.sh tests. And we definitely need to
> add a test case about --quiet to make sure it won't happen again.
Find testcase attached. Please submit in a series with your patch

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

[-- Attachment #1.2: 0001-submodule-foreach-test-foreach-option-swallowing.patch --]
[-- Type: text/x-diff, Size: 1271 bytes --]

From a57994f2d78134936521375ba9798a1b7418e230 Mon Sep 17 00:00:00 2001
From: "Robin H. Johnson" <robbat2@gentoo.org>
Date: Fri, 12 Apr 2019 00:00:07 -0700
Subject: [PATCH] submodule foreach: test foreach option swallowing

Add a testcase for submodule foreach option parsing not knowing where to
stop taking options, and accidently removing options intended for
foreach target commands.

CC: Duy Nguyen <pclouds@gmail.com>
CC: Prathamesh Chavan <pc44800@gmail.com>
Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
---
 t/t7407-submodule-foreach.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 77729ac4aa..706ae762e0 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -411,4 +411,14 @@ test_expect_success 'multi-argument command passed to foreach is not shell-evalu
 	test_cmp expected actual
 '
 
+test_expect_success 'option-like arguments passed to foreach commands are not lost' '
+	(
+		cd super &&
+		git submodule foreach "echo be --quiet" > ../expected &&
+		git submodule foreach echo be --quiet > ../actual
+	) &&
+	grep -sq -e "--quiet" expected &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.21.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1113 bytes --]

Re: regression AGAIN in output of git-pull --rebase --recurse-submodules=yes --quiet

[Duy Nguyen]

On Fri, Apr 12, 2019 at 2:09 PM Robin H. Johnson <robbat2@gentoo.org> wrote:
> > -- 8< --
> (snip patch, please add my DCO signed-off-by)
> Tested-by: Robin H. Johnson <robbat2@gentoo.org>
> Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
> > -- 8< --
> >
> > I'm a bit reluctant to follow up with a proper patch because I can't
> > digest the t5572-submodule-pull.sh tests. And we definitely need to
> > add a test case about --quiet to make sure it won't happen again.
> Find testcase attached. Please submit in a series with your patch

Clever. I was stuck thinking about actually pulling things. But yeah
"echo --quiet" does the job just as well. Making patches (and maybe
trying to fix that pull --rebase --quiet thing as well)
-- 
Duy

[PATCH] submodule foreach: fix "<command> --quiet" not being respected

[Nguyễn Thái Ngọc Duy]

Robin reported that

    git submodule foreach --quiet git pull --quiet origin

is not really quiet anymore [1]. "git pull" behaves as if --quiet is not
given.

This happens because parseopt in submodule--helper will try to parse
both --quiet options as if they are foreach's options, not git-pull's.
The parsed options are removed from the command line. So when we do
pull later, we execute just this

    git pull origin

When calling submodule helper, adding "--" in front of "git pull" will
stop parseopt for parsing options that do not really belong to
submodule--helper foreach.

PARSE_OPT_KEEP_UNKNOWN is removed as a safety measure. parseopt should
never see unknown options or something has gone wrong. There are also
a couple usage string update while I'm looking at them.

While at it, I also add "--" to other subcommands that pass "$@" to
submodule--helper. "$@" in these cases are paths and less likely to be
--something-like-this. But the point still stands, git-submodule has
parsed and classified what are options, what are paths. submodule--helper
should never consider paths passed by git-submodule to be options even
if they look like one.

The test case is also contributed by Robin.

[1] it should be quiet before fc1b9243cd (submodule: port submodule
    subcommand 'foreach' from shell to C, 2018-05-10) because parseopt
    can't accidentally eat options then.

Reported-by: Robin H. Johnson <robbat2@gentoo.org>
Tested-by: Robin H. Johnson <robbat2@gentoo.org>
Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I'm not trying to fix "git pull --rebase --quiet" (or "git rebase
 --quiet" in general) in the end, since that looks like a whole other
 can of worms.
 
 Not only git-rebase--preserve-merges.sh needs to respect --quiet (but
 which case? I don't have enough experience to say) but sequencer.c
 may need to be scanned too.

 builtin/submodule--helper.c  |  8 ++++----
 git-submodule.sh             | 11 ++++++-----
 t/t7407-submodule-foreach.sh | 10 ++++++++++
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6bcc4f1bd7..59570b5e87 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -566,12 +566,12 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper foreach [--quiet] [--recursive] <command>"),
+		N_("git submodule--helper foreach [--quiet] [--recursive] [--] <command>"),
 		NULL
 	};
 
 	argc = parse_options(argc, argv, prefix, module_foreach_options,
-			     git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);
+			     git_submodule_helper_usage, 0);
 
 	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
 		return 1;
@@ -709,7 +709,7 @@ static int module_init(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper init [<path>]"),
+		N_("git submodule--helper init [<options>] [<path>]"),
 		NULL
 	};
 
@@ -2096,7 +2096,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper embed-git-dir [<path>...]"),
+		N_("git submodule--helper asorb-git-dirs [<options>] [<path>...]"),
 		NULL
 	};
 
diff --git a/git-submodule.sh b/git-submodule.sh
index 2c0fb6d723..d33f5d8bb4 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -346,7 +346,7 @@ cmd_foreach()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@"
 }
 
 #
@@ -377,7 +377,7 @@ cmd_init()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper init ${GIT_QUIET:+--quiet}  "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper init ${GIT_QUIET:+--quiet} -- "$@"
 }
 
 #
@@ -413,7 +413,7 @@ cmd_deinit()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${prefix:+--prefix "$prefix"} ${force:+--force} ${deinit_all:+--all} "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${prefix:+--prefix "$prefix"} ${force:+--force} ${deinit_all:+--all} -- "$@"
 }
 
 is_tip_reachable () (
@@ -542,6 +542,7 @@ cmd_update()
 		${depth:+--depth "$depth"} \
 		$recommend_shallow \
 		$jobs \
+		-- \
 		"$@" || echo "#unmatched" $?
 	} | {
 	err=
@@ -934,7 +935,7 @@ cmd_status()
 		shift
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- "$@"
 }
 #
 # Sync remote urls for submodules
@@ -967,7 +968,7 @@ cmd_sync()
 		esac
 	done
 
-	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- "$@"
 }
 
 cmd_absorbgitdirs()
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 77729ac4aa..706ae762e0 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -411,4 +411,14 @@ test_expect_success 'multi-argument command passed to foreach is not shell-evalu
 	test_cmp expected actual
 '
 
+test_expect_success 'option-like arguments passed to foreach commands are not lost' '
+	(
+		cd super &&
+		git submodule foreach "echo be --quiet" > ../expected &&
+		git submodule foreach echo be --quiet > ../actual
+	) &&
+	grep -sq -e "--quiet" expected &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.21.0.682.g30d2204636

Re: [PATCH] submodule foreach: fix "<command> --quiet" not being respected

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

On Fri, Apr 12, 2019 at 05:08:19PM +0700, Nguyễn Thái Ngọc Duy wrote:
> @@ -2096,7 +2096,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
>  	};
>  
>  	const char *const git_submodule_helper_usage[] = {
> -		N_("git submodule--helper embed-git-dir [<path>...]"),
> +		N_("git submodule--helper asorb-git-dirs [<options>] [<path>...]"),
Nit typo here: s/asorb/absorb/

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1113 bytes --]

Re: [PATCH] submodule foreach: fix "<command> --quiet" not being respected

[Junio C Hamano]

"Robin H. Johnson" <robbat2@gentoo.org> writes:

> On Fri, Apr 12, 2019 at 05:08:19PM +0700, Nguyễn Thái Ngọc Duy wrote:
>> @@ -2096,7 +2096,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
>>  	};
>>  
>>  	const char *const git_submodule_helper_usage[] = {
>> -		N_("git submodule--helper embed-git-dir [<path>...]"),
>> +		N_("git submodule--helper asorb-git-dirs [<options>] [<path>...]"),
> Nit typo here: s/asorb/absorb/

Will locally tweak while queueing.  Thanks, both.

Re: regression AGAIN in output of git-pull --rebase --recurse-submodules=yes --quiet

[Johannes Schindelin]

Hi Robin,

On Fri, 12 Apr 2019, Robin H. Johnson wrote:

> Looking at git-rebase--preserve-merges.sh for this message, I think that
> should be a separate patch to make it respect --quiet.

Please note that `git rebase --preserve-merges` will be deprecated as of
the next Git version (see https://github.com/gitgitgadget/git/pull/158 for
details).

So I don't think it is worth the bother to fix that mode with respect to
--quiet.

Ciao,
Johannes

Re: regression AGAIN in output of git-pull --rebase --recurse-submodules=yes --quiet

[Duy Nguyen]

On Tue, Apr 16, 2019 at 1:31 PM Paul Morelle <paul.morelle@gmail.com> wrote:
>> The problem here is the option parser of this command would try to
>> parse all options, so it considers both --quiet the same thing and are
>> to tell "submodule--foreach" to be quiet, the second --quiet is not
>> part of the "git pull" command anymore.
>>
>> So the fix would be to pass "--" to stop option parsing.
>> submodule--helper should not parse options it does not understand
>> anyway. Something like this should work.
>
>
> My expectation as a user (and probably Robin's too) would be that `git submodule foreach` stops parsing arguments at `--` or at the first not-recognized argument, whichever is encountered first. The rest of the arguments would then be considered as the command.

I don't think I change any visible behavior though (or at least trying
not to). There are two command line parsers, the "front" one is in
git-submodule.sh and should do what you describe (or whatever the
current behavior is) and there's an internal one for "git
submodule--helper" which is more like internal API than anything.

The change here is to stop the internal parser from accidentally
interpret options that belong to the foreach's command. The "--" and
first non-recognized argument should be handled correctly by the front
parser.

The exact behavior of this front parser, I can't tell (I'm nowhere
near expert level of submodules) but yeah it should stop at either
`--` or the first non-option argument (e.g. something that does not
start with '-'). An argument that looks like an option (i.e. starts
with '-') but not recognized should result in an error. This is pretty
much standard behavior for all other commands, but I have not tested
this with git-submodule.sh.

> This would slightly break the retrocompatibility, but would also avoid similar bugs in the future.
-- 
Duy