# [PATCH] submodule: no [--merge|--rebase] when newly cloned 9 messages from 2011-02-16 to 2011-02-18. Participants: Spencer E. Olson, Junio C Hamano, Jens Lehmann, Johannes Sixt, Martin von Zweigbergk. Thread: https://gitlist.dev/t/26522 ## Spencer E. Olson, 2011-02-16 12:46 Subject: [PATCH] submodule: no [--merge|--rebase] when newly cloned Message-ID: <1297860417-21895-1-git-send-email-olsonse@umich.edu> URL: https://gitlist.dev/e/1297860417-21895-1-git-send-email-olsonse%40umich.edu ``` Previously, when a submodule was cloned in the same command execution, --rebase or --merge would attempt to run (instead of and) before a checkout, resulting in an empty working directory. This patch ignores --rebase or --merge for a particular submodule when that submodule is newly cloned and instead simply checks out the appropriate revision. Signed-off-by: Spencer E. Olson --- git-submodule.sh | 28 ++++++++++++++++++++++++++++ 1 files changed, 28 insertions(+), 0 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 8b90589..44d8741 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -365,6 +365,27 @@ cmd_init() } # +# Test whether an element of the $3(=IFS) separated list $2 matches $1 +# +str_contains() +{ + if test -n "$3" + then + local IFS="$3" + fi + for i in $2 + do + if test "$i" = "$1" + then + echo "yes" + return + fi + done + echo "no" + return +} + +# # Update each submodule path to correct revision, using clone and checkout as needed # # $@ = requested paths (default to all) @@ -423,6 +444,7 @@ cmd_update() cmd_init "--" "$@" || return fi + cloned_modules= module_list "$@" | while read mode sha1 stage path do @@ -442,6 +464,7 @@ cmd_update() if ! test -d "$path"/.git -o -f "$path"/.git then module_clone "$path" "$url" "$reference"|| exit + cloned_modules="$cloned_modules;$name" subsha1= else subsha1=$(clear_local_git_env; cd "$path" && @@ -469,6 +492,11 @@ cmd_update() die "Unable to fetch in submodule path '$path'" fi + if test "$(str_contains "$name" "$cloned_modules" '\;')" = "yes" + then + update_module= + fi + case "$update_module" in rebase) command="git rebase" -- 1.7.4.1.42.g43f9f ``` ## Junio C Hamano, 2011-02-16 19:51 Subject: Re: [PATCH] submodule: no [--merge|--rebase] when newly cloned Message-ID: <7v62sjkbbi.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v62sjkbbi.fsf%40alter.siamese.dyndns.org In-Reply-To: <1297860417-21895-1-git-send-email-olsonse@umich.edu> ``` "Spencer E. Olson" writes: > Previously, when a submodule was cloned in the same command execution, --rebase > or --merge would attempt to run (instead of and) before a checkout, resulting in > an empty working directory. This patch ignores --rebase or --merge for a > particular submodule when that submodule is newly cloned and instead simply > checks out the appropriate revision. Sorry, but I cannot parse the problem description, "(instead of and)" part. Why is it a better thing to do to ignore these options, instead of detecting the situation and error it out, saying "you are initially cloning, don't say --rebase"? > +# Test whether an element of the $3(=IFS) separated list $2 matches $1 > +# > +str_contains() > +{ > + if test -n "$3" > + then > + local IFS="$3" > + fi > + for i in $2 > + do > + if test "$i" = "$1" > + then > + echo "yes" > + return > + fi > + done > + echo "no" > + return > +} We don't allow bash-ism outside POSIX shell in our primary Porcelain set (the rule is looser for stuff in contrib/), so "local" is out. Here is how to write the above more concisely, efficiently and portably. case "$2;" in *";$1;"*) echo yes ;; *) echo no ;; esac The trailing ';' takes care of the case where cloned_modules has only one element, in which case you have ";name" in "$2". No need for a loop. > + cloned_modules= > ... > while read mode sha1 stage path > do > @@ -442,6 +464,7 @@ cmd_update() > if ! test -d "$path"/.git -o -f "$path"/.git > then > module_clone "$path" "$url" "$reference"|| exit > + cloned_modules="$cloned_modules;$name" > subsha1= ``` ## Jens Lehmann, 2011-02-16 20:10 Subject: Re: [PATCH] submodule: no [--merge|--rebase] when newly cloned Message-ID: <4D5C2F33.8020309@web.de> URL: https://gitlist.dev/e/4D5C2F33.8020309%40web.de In-Reply-To: <7v62sjkbbi.fsf@alter.siamese.dyndns.org> ``` Am 16.02.2011 20:51, schrieb Junio C Hamano: > "Spencer E. Olson" writes: > >> Previously, when a submodule was cloned in the same command execution, --rebase >> or --merge would attempt to run (instead of and) before a checkout, resulting in >> an empty working directory. This patch ignores --rebase or --merge for a >> particular submodule when that submodule is newly cloned and instead simply >> checks out the appropriate revision. > > Sorry, but I cannot parse the problem description, "(instead of and)" part. I think what he wanted to say was that after a new submodule is cloned by running "git submodule update" it will attempt to do a rebase or merge without having checked out the submodule at all, which obviously can't work. A plain checkout looks like the right thing to do, as there aren't any local commits to rebase or merge yet. > Why is it a better thing to do to ignore these options, instead of > detecting the situation and error it out, saying "you are initially > cloning, don't say --rebase"? It should be fine to use these options when new submodules appear. (And even without explicitly specifing these command line options this bug can also be triggered by having the "submodule..update" option set in .gitmodules to either "rebase" or "merge", which is easy to miss) So this looks like a worthwhile fix. The commit message and POSIX issue need to be addressed, tests would be a good thing to add too, but apart from that it looks sane. ``` ## Junio C Hamano, 2011-02-16 21:41 Subject: Re: [PATCH] submodule: no [--merge|--rebase] when newly cloned Message-ID: <7v7hczirnz.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v7hczirnz.fsf%40alter.siamese.dyndns.org In-Reply-To: <4D5C2F33.8020309@web.de> ``` Jens Lehmann writes: > So this looks like a worthwhile fix. The commit message and POSIX issue > need to be addressed, tests would be a good thing to add too, but apart > from that it looks sane. Thanks for a translation ;-). I agree everthing you said above. ``` ## Spencer E. Olson, 2011-02-17 00:21 Subject: Re: [PATCH] submodule: no [--merge|--rebase] when newly cloned Message-ID: <201102161721.37389.olsonse@umich.edu> URL: https://gitlist.dev/e/201102161721.37389.olsonse%40umich.edu In-Reply-To: <7v7hczirnz.fsf@alter.siamese.dyndns.org> ``` I'll fix the POSIX issue and commit message and re-submit. (I wasn't aware that for loops were from a bash-ism). On Wednesday 16 February 2011 14:41, Junio C Hamano wrote: > Jens Lehmann writes: > > So this looks like a worthwhile fix. The commit message and POSIX issue > > need to be addressed, tests would be a good thing to add too, but apart > > from that it looks sane. > > Thanks for a translation ;-). I agree everthing you said above. ``` ## Johannes Sixt, 2011-02-17 07:10 Subject: Re: [PATCH] submodule: no [--merge|--rebase] when newly cloned Message-ID: <4D5CC9C9.60705@viscovery.net> URL: https://gitlist.dev/e/4D5CC9C9.60705%40viscovery.net In-Reply-To: <7v62sjkbbi.fsf@alter.siamese.dyndns.org> ``` Am 2/16/2011 20:51, schrieb Junio C Hamano: > Here is how to write the above more concisely, efficiently and portably. > > case "$2;" in > *";$1;"*) > echo yes ;; > *) > echo no ;; > esac > > The trailing ';' takes care of the case where cloned_modules has only one > element, in which case you have ";name" in "$2". No need for a loop. And while you are here, you could make this: list_contains() { case "$2;" in *";$1;"*) : yes ;; *) ! : no ;; esac } and test for the exit code of this function rather than its output at the call site. -- Hannes ``` ## Martin von Zweigbergk, 2011-02-18 00:34 Subject: Re: [PATCH] submodule: no [--merge|--rebase] when newly cloned Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.00.1102171929200.14950%40debian In-Reply-To: <4D5CC9C9.60705@viscovery.net> ``` On Thu, 17 Feb 2011, Johannes Sixt wrote: > Am 2/16/2011 20:51, schrieb Junio C Hamano: > > Here is how to write the above more concisely, efficiently and portably. > > > > case "$2;" in > > *";$1;"*) > > echo yes ;; > > *) > > echo no ;; > > esac > > > > The trailing ';' takes care of the case where cloned_modules has only one > > element, in which case you have ";name" in "$2". No need for a loop. > > And while you are here, you could make this: > > list_contains() > { > case "$2;" in > *";$1;"*) > : yes ;; > *) > ! : no ;; > esac > } > > and test for the exit code of this function rather than its output at the > call site. According to Brandon Casey: "Some platforms (IRIX 6.5, Solaris 7) do not provide the 'yes' utility." See 8648732 (t/test-lib.sh: provide a shell implementation of the 'yes' utility, 2009-08-28). /Martin ``` ## Spencer E. Olson, 2011-02-18 00:48 Subject: Re: [PATCH] submodule: no [--merge|--rebase] when newly cloned Message-ID: <201102171748.15516.olsonse@umich.edu> URL: https://gitlist.dev/e/201102171748.15516.olsonse%40umich.edu In-Reply-To: ``` We are not actually using the yes utility (think y\ny\ny\ny\ny\ny\n...) As far as I understand, in this particular use, "yes" and "no" are just comments. We could also write : I like banannas ;; and ! : I do not like anchovies ;; On Thursday 17 February 2011 17:34, Martin von Zweigbergk wrote: > On Thu, 17 Feb 2011, Johannes Sixt wrote: > > Am 2/16/2011 20:51, schrieb Junio C Hamano: > > > Here is how to write the above more concisely, efficiently and > > > portably. > > > > > > case "$2;" in > > > *";$1;"*) > > > echo yes ;; > > > *) > > > echo no ;; > > > esac > > > > > > The trailing ';' takes care of the case where cloned_modules has only > > > one element, in which case you have ";name" in "$2". No need for a > > > loop. > > > > And while you are here, you could make this: > > > > list_contains() > > { > > case "$2;" in > > *";$1;"*) > > > > : yes ;; > > > > *) > > ! : no ;; > > esac > > } > > > > and test for the exit code of this function rather than its output at the > > call site. > > According to Brandon Casey: "Some platforms (IRIX 6.5, Solaris 7) do > not provide the 'yes' utility." See 8648732 (t/test-lib.sh: provide a > shell implementation of the 'yes' utility, 2009-08-28). > > > /Martin ``` ## Martin von Zweigbergk, 2011-02-18 00:59 Subject: Re: [PATCH] submodule: no [--merge|--rebase] when newly cloned Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.00.1102171957350.14950%40debian In-Reply-To: <201102171748.15516.olsonse@umich.edu> ``` On Thu, 17 Feb 2011, Spencer E. Olson wrote: > We are not actually using the yes utility (think y\ny\ny\ny\ny\ny\n...) > > As far as I understand, in this particular use, "yes" and "no" are just > comments. We could also write > : I like banannas ;; > and > ! : I do not like anchovies ;; Ah, I'm sure you're right. Need to read up on that colon operator. Thanks. > > On Thursday 17 February 2011 17:34, Martin von Zweigbergk wrote: > > On Thu, 17 Feb 2011, Johannes Sixt wrote: > > > Am 2/16/2011 20:51, schrieb Junio C Hamano: > > > > Here is how to write the above more concisely, efficiently and > > > > portably. > > > > > > > > case "$2;" in > > > > *";$1;"*) > > > > echo yes ;; > > > > *) > > > > echo no ;; > > > > esac > > > > > > > > The trailing ';' takes care of the case where cloned_modules has only > > > > one element, in which case you have ";name" in "$2". No need for a > > > > loop. > > > > > > And while you are here, you could make this: > > > > > > list_contains() > > > { > > > case "$2;" in > > > *";$1;"*) > > > > > > : yes ;; > > > > > > *) > > > ! : no ;; > > > esac > > > } > > > > > > and test for the exit code of this function rather than its output at the > > > call site. > > > > According to Brandon Casey: "Some platforms (IRIX 6.5, Solaris 7) do > > not provide the 'yes' utility." See 8648732 (t/test-lib.sh: provide a > > shell implementation of the 'yes' utility, 2009-08-28). > > > > > > /Martin > ```