git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] submodule: Add --force option for git submodule update
@ 2011-03-30  7:56 Nicolas Morey-Chaisemartin
  2011-03-30 18:32 ` Jens Lehmann
  0 siblings, 1 reply; 9+ messages in thread
From: Nicolas Morey-Chaisemartin @ 2011-03-30  7:56 UTC (permalink / raw)
  To: git

By default git submodule update runs a simple checkout on submodules
that are not up-to-date.
If the submodules contains modified or untracked files, the command may
exit sanely with an error:

$ git submodule update
error: Your local changes to the following files would be overwritten by
checkout:
	file
Please, commit your changes or stash them before you can switch branches.
Aborting
Unable to checkout '1b69c6e55606b48d3284a3a9efe4b58bfb7e8c9e' in
submodule path 'test1'

This implies that to reset a whole git submodule tree, a user has to run
first 'git submodule foreach --recursive git checkout -f' to then be
able to run git submodule update.

This patch adds a --force option for the update command (only used for
submodules without --rebase or --merge options). It passes the --force
option to git checkout which will throw away the local changes.
Also when --force is specified, git checkout -f is always called on
submodules whether their HEAD matches the reference or not.

Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 Documentation/git-submodule.txt |    5 ++-
 git-submodule.sh                |   68 ++++++++++++++++++++------------------
 t/t7406-submodule-update.sh     |   23 +++++++++++++
 3 files changed, 62 insertions(+), 34 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 3a5aa01..6482a84 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -185,8 +185,9 @@ OPTIONS
 
 -f::
 --force::
-	This option is only valid for the add command.
-	Allow adding an otherwise ignored submodule path.
+	This option is only valid for add and update commands.
+	When running add, allow adding an otherwise ignored submodule path.
+	When running update, throw away local changes in submodules.
 
 --cached::
 	This option is only valid for status and summary commands.  These
diff --git a/git-submodule.sh b/git-submodule.sh
index 3a13397..a195879 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -8,7 +8,7 @@ dashless=$(basename "$0" | sed -e 's/-/ /')
 USAGE="[--quiet] add [-b branch] [-f|--force] [--reference <repository>] [--] <repository> [<path>]
    or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] init [--] [<path>...]
-   or: $dashless [--quiet] update [--init] [-N|--no-fetch] [--rebase] [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
+   or: $dashless [--quiet] update [--init] [-N|--no-fetch] [-f|--force] [--rebase] [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
    or: $dashless [--quiet] foreach [--recursive] <command>
    or: $dashless [--quiet] sync [--] [<path>...]"
@@ -385,6 +385,9 @@ cmd_update()
 		-N|--no-fetch)
 			nofetch=1
 			;;
+		-f | --force)
+			force=$1
+			;;
 		-r|--rebase)
 			update="rebase"
 			;;
@@ -430,6 +433,7 @@ cmd_update()
 		name=$(module_name "$path") || exit
 		url=$(git config submodule."$name".url)
 		update_module=$(git config submodule."$name".update)
+		force_checkout=
 		if test -z "$url"
 		then
 			# Only mention uninitialized submodules when its
@@ -456,13 +460,38 @@ cmd_update()
 			update_module=$update
 		fi
 
-		if test "$subsha1" != "$sha1"
+		if test -z "$subsha1" -a -z "$force"
+		then
+		    force_checkout="-f"
+		fi
+
+		# Is this something we just cloned?
+		case ";$cloned_modules;" in
+		    *";$name;"*)
+			 # then there is no local change to integrate
+			 update_module= ;;
+		esac
+
+		case "$update_module" in
+		    rebase)
+			 command="git rebase"
+			 action="rebase"
+			 msg="rebased onto"
+			 ;;
+		    merge)
+			 command="git merge"
+			 action="merge"
+			 msg="merged in"
+			 ;;
+		    *)
+			 command="git checkout $force $force_checkout -q"
+			 action="checkout"
+			 msg="checked out"
+			 ;;
+		esac
+
+		if test "$subsha1" != "$sha1" || test -n "$force" -a "$action" = "checkout"
 		then
-			force=
-			if test -z "$subsha1"
-			then
-				force="-f"
-			fi
 
 			if test -z "$nofetch"
 			then
@@ -471,31 +500,6 @@ cmd_update()
 				die "Unable to fetch in submodule path '$path'"
 			fi
 
-			# Is this something we just cloned?
-			case ";$cloned_modules;" in
-			*";$name;"*)
-				# then there is no local change to integrate
-				update_module= ;;
-			esac
-
-			case "$update_module" in
-			rebase)
-				command="git rebase"
-				action="rebase"
-				msg="rebased onto"
-				;;
-			merge)
-				command="git merge"
-				action="merge"
-				msg="merged in"
-				;;
-			*)
-				command="git checkout $force -q"
-				action="checkout"
-				msg="checked out"
-				;;
-			esac
-
 			(clear_local_git_env; cd "$path" && $command "$sha1") ||
 			die "Unable to $action '$sha1' in submodule path '$path'"
 			say "Submodule path '$path': $msg '$sha1'"
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index fa9d23a..5d24d9f 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -74,6 +74,29 @@ test_expect_success 'submodule update detaching the HEAD ' '
 	)
 '
 
+test_expect_success 'submodule update should fail due to local changes' '
+	(cd super/submodule &&
+	 git reset --hard HEAD~1 &&
+	 echo "local change" > file
+	) &&
+	(cd super &&
+	 (cd submodule &&
+	  compare_head
+	 ) &&
+	 test_must_fail git submodule update submodule
+	)
+'
+test_expect_success 'submodule update should throw away changes with --force ' '
+	(cd super &&
+	 (cd submodule &&
+	  compare_head
+	 ) &&
+	 git submodule update --force submodule &&
+	 cd submodule &&
+	 ! compare_head
+	)
+'
+
 test_expect_success 'submodule update --rebase staying on master' '
 	(cd super/submodule &&
 	  git checkout master

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] submodule: Add --force option for git submodule update
  2011-03-30  7:56 [PATCH] submodule: Add --force option for git submodule update Nicolas Morey-Chaisemartin
@ 2011-03-30 18:32 ` Jens Lehmann
  2011-03-30 18:50   ` Nicolas Morey-Chaisemartin
  0 siblings, 1 reply; 9+ messages in thread
From: Jens Lehmann @ 2011-03-30 18:32 UTC (permalink / raw)
  To: devel-git; +Cc: git

Am 30.03.2011 09:56, schrieb Nicolas Morey-Chaisemartin:
> By default git submodule update runs a simple checkout on submodules
> that are not up-to-date.
> If the submodules contains modified or untracked files, the command may
> exit sanely with an error:
> 
> $ git submodule update
> error: Your local changes to the following files would be overwritten by
> checkout:
> 	file
> Please, commit your changes or stash them before you can switch branches.
> Aborting
> Unable to checkout '1b69c6e55606b48d3284a3a9efe4b58bfb7e8c9e' in
> submodule path 'test1'
> 
> This implies that to reset a whole git submodule tree, a user has to run
> first 'git submodule foreach --recursive git checkout -f' to then be
> able to run git submodule update.
> 
> This patch adds a --force option for the update command (only used for
> submodules without --rebase or --merge options). It passes the --force
> option to git checkout which will throw away the local changes.
> Also when --force is specified, git checkout -f is always called on
> submodules whether their HEAD matches the reference or not.

I like where this is going (and it has tests too :-).

Maybe we can simplify the patch and simplify one of the tests (see below)
but apart from that I'm all for it.

> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
> ---
>  Documentation/git-submodule.txt |    5 ++-
>  git-submodule.sh                |   68 ++++++++++++++++++++------------------
>  t/t7406-submodule-update.sh     |   23 +++++++++++++
>  3 files changed, 62 insertions(+), 34 deletions(-)
> 
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index 3a5aa01..6482a84 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -185,8 +185,9 @@ OPTIONS
>  
>  -f::
>  --force::
> -	This option is only valid for the add command.
> -	Allow adding an otherwise ignored submodule path.
> +	This option is only valid for add and update commands.
> +	When running add, allow adding an otherwise ignored submodule path.
> +	When running update, throw away local changes in submodules.
>  
>  --cached::
>  	This option is only valid for status and summary commands.  These
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 3a13397..a195879 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -8,7 +8,7 @@ dashless=$(basename "$0" | sed -e 's/-/ /')
>  USAGE="[--quiet] add [-b branch] [-f|--force] [--reference <repository>] [--] <repository> [<path>]
>     or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
>     or: $dashless [--quiet] init [--] [<path>...]
> -   or: $dashless [--quiet] update [--init] [-N|--no-fetch] [--rebase] [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
> +   or: $dashless [--quiet] update [--init] [-N|--no-fetch] [-f|--force] [--rebase] [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
>     or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
>     or: $dashless [--quiet] foreach [--recursive] <command>
>     or: $dashless [--quiet] sync [--] [<path>...]"
> @@ -385,6 +385,9 @@ cmd_update()
>  		-N|--no-fetch)
>  			nofetch=1
>  			;;
> +		-f | --force)
> +			force=$1
> +			;;
>  		-r|--rebase)
>  			update="rebase"
>  			;;

All looking good up to here. But I wonder if the rest of git-submodule.sh
could be changed a bit less invasive ... maybe as simple as this?

@@ -458,7 +461,6 @@ cmd_update()

 		if test "$subsha1" != "$sha1"
 		then
-			force=
 			if test -z "$subsha1"
 			then
 				force="-f"

Now force will not be cleared and thus contain "-f" if the user provided
it on the command line. All tests (including your new ones) are running
fine with this simplification ... am I missing something?

> @@ -430,6 +433,7 @@ cmd_update()
>  		name=$(module_name "$path") || exit
>  		url=$(git config submodule."$name".url)
>  		update_module=$(git config submodule."$name".update)
> +		force_checkout=
>  		if test -z "$url"
>  		then
>  			# Only mention uninitialized submodules when its
> @@ -456,13 +460,38 @@ cmd_update()
>  			update_module=$update
>  		fi
>  
> -		if test "$subsha1" != "$sha1"
> +		if test -z "$subsha1" -a -z "$force"
> +		then
> +		    force_checkout="-f"
> +		fi
> +
> +		# Is this something we just cloned?
> +		case ";$cloned_modules;" in
> +		    *";$name;"*)
> +			 # then there is no local change to integrate
> +			 update_module= ;;
> +		esac
> +
> +		case "$update_module" in
> +		    rebase)
> +			 command="git rebase"
> +			 action="rebase"
> +			 msg="rebased onto"
> +			 ;;
> +		    merge)
> +			 command="git merge"
> +			 action="merge"
> +			 msg="merged in"
> +			 ;;
> +		    *)
> +			 command="git checkout $force $force_checkout -q"
> +			 action="checkout"
> +			 msg="checked out"
> +			 ;;
> +		esac
> +
> +		if test "$subsha1" != "$sha1" || test -n "$force" -a "$action" = "checkout"
>  		then
> -			force=
> -			if test -z "$subsha1"
> -			then
> -				force="-f"
> -			fi
>  
>  			if test -z "$nofetch"
>  			then
> @@ -471,31 +500,6 @@ cmd_update()
>  				die "Unable to fetch in submodule path '$path'"
>  			fi
>  
> -			# Is this something we just cloned?
> -			case ";$cloned_modules;" in
> -			*";$name;"*)
> -				# then there is no local change to integrate
> -				update_module= ;;
> -			esac
> -
> -			case "$update_module" in
> -			rebase)
> -				command="git rebase"
> -				action="rebase"
> -				msg="rebased onto"
> -				;;
> -			merge)
> -				command="git merge"
> -				action="merge"
> -				msg="merged in"
> -				;;
> -			*)
> -				command="git checkout $force -q"
> -				action="checkout"
> -				msg="checked out"
> -				;;
> -			esac
> -
>  			(clear_local_git_env; cd "$path" && $command "$sha1") ||
>  			die "Unable to $action '$sha1' in submodule path '$path'"
>  			say "Submodule path '$path': $msg '$sha1'"

And if I'm right all stuff up to here can go.

> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index fa9d23a..5d24d9f 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -74,6 +74,29 @@ test_expect_success 'submodule update detaching the HEAD ' '
>  	)
>  '
>  
> +test_expect_success 'submodule update should fail due to local changes' '
> +	(cd super/submodule &&
> +	 git reset --hard HEAD~1 &&
> +	 echo "local change" > file
> +	) &&
> +	(cd super &&
> +	 (cd submodule &&
> +	  compare_head
> +	 ) &&
> +	 test_must_fail git submodule update submodule
> +	)
> +'

This test is shorter and might be easier to understand rewritten as:

+test_expect_success 'submodule update should fail due to local changes' '
+	(cd super &&
+	 (cd submodule &&
+	  git reset --hard HEAD~1 &&
+	  echo "local change" > file
+	  compare_head
+	 ) &&
+	 test_must_fail git submodule update submodule
+	)
+'

> +test_expect_success 'submodule update should throw away changes with --force ' '
> +	(cd super &&
> +	 (cd submodule &&
> +	  compare_head
> +	 ) &&
> +	 git submodule update --force submodule &&
> +	 cd submodule &&
> +	 ! compare_head
> +	)
> +'
> +
>  test_expect_success 'submodule update --rebase staying on master' '
>  	(cd super/submodule &&
>  	  git checkout master

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] submodule: Add --force option for git submodule update
  2011-03-30 18:32 ` Jens Lehmann
@ 2011-03-30 18:50   ` Nicolas Morey-Chaisemartin
  2011-03-30 19:05     ` Jens Lehmann
  0 siblings, 1 reply; 9+ messages in thread
From: Nicolas Morey-Chaisemartin @ 2011-03-30 18:50 UTC (permalink / raw)
  To: Jens Lehmann; +Cc: git

On 03/30/2011 08:32 PM, Jens Lehmann wrote:
> Am 30.03.2011 09:56, schrieb Nicolas Morey-Chaisemartin:

> All looking good up to here. But I wonder if the rest of git-submodule.sh
> could be changed a bit less invasive ... maybe as simple as this?
> 
> @@ -458,7 +461,6 @@ cmd_update()
> 
>  		if test "$subsha1" != "$sha1"
>  		then
> -			force=
>  			if test -z "$subsha1"
>  			then
>  				force="-f"
> 
> Now force will not be cleared and thus contain "-f" if the user provided
> it on the command line. All tests (including your new ones) are running
> fine with this simplification ... am I missing something?

Actually, I don't think this work.
By doing that, if you run git submodule update without -f, it will set -f when you reached the first submodule not yet checked out ( -z $subsha1 ),
and the following submodules will be checkout using --force which may throw away changes the user wanted to keep.

I know it is very intrusive. The main reason for that is I wanted the -f option to always behave the same (meaning throw away changes),
whether the submodule is already on the right commit or not.

If we accept to drop this and only drop the changes when subsha1 != sha1, the patch can be much sorter by simply keeping the force flags I used and without modifying all the case/while thing.


>>  
>> +test_expect_success 'submodule update should fail due to local changes' '
>> +	(cd super/submodule &&
>> +	 git reset --hard HEAD~1 &&
>> +	 echo "local change" > file
>> +	) &&
>> +	(cd super &&
>> +	 (cd submodule &&
>> +	  compare_head
>> +	 ) &&
>> +	 test_must_fail git submodule update submodule
>> +	)
>> +'
> 
> This test is shorter and might be easier to understand rewritten as:
> 
> +test_expect_success 'submodule update should fail due to local changes' '
> +	(cd super &&
> +	 (cd submodule &&
> +	  git reset --hard HEAD~1 &&
> +	  echo "local change" > file
> +	  compare_head
> +	 ) &&
> +	 test_must_fail git submodule update submodule
> +	)
> +'
> 

Agreed.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] submodule: Add --force option for git submodule update
  2011-03-30 18:50   ` Nicolas Morey-Chaisemartin
@ 2011-03-30 19:05     ` Jens Lehmann
  2011-03-30 20:19       ` Nicolas Morey-Chaisemartin
  0 siblings, 1 reply; 9+ messages in thread
From: Jens Lehmann @ 2011-03-30 19:05 UTC (permalink / raw)
  To: Nicolas Morey-Chaisemartin; +Cc: git

Am 30.03.2011 20:50, schrieb Nicolas Morey-Chaisemartin:
> On 03/30/2011 08:32 PM, Jens Lehmann wrote:
>> Am 30.03.2011 09:56, schrieb Nicolas Morey-Chaisemartin:
> 
>> All looking good up to here. But I wonder if the rest of git-submodule.sh
>> could be changed a bit less invasive ... maybe as simple as this?
>>
>> @@ -458,7 +461,6 @@ cmd_update()
>>
>>  		if test "$subsha1" != "$sha1"
>>  		then
>> -			force=
>>  			if test -z "$subsha1"
>>  			then
>>  				force="-f"
>>
>> Now force will not be cleared and thus contain "-f" if the user provided
>> it on the command line. All tests (including your new ones) are running
>> fine with this simplification ... am I missing something?
> 
> Actually, I don't think this work.
> By doing that, if you run git submodule update without -f, it will set -f when you reached the first submodule not yet checked out ( -z $subsha1 ),
> and the following submodules will be checkout using --force which may throw away changes the user wanted to keep.

You are right, I just came to that conclusion myself ... but with a loop
local variable initialized from force on every iteration it should work.

> I know it is very intrusive. The main reason for that is I wanted the -f option to always behave the same (meaning throw away changes),
> whether the submodule is already on the right commit or not.

Hmm, I don't know if that is a good thing to do. People are used to
"git submodule update" to only touch those submodule where the HEAD
differs from the commit recorded in the superproject (And I often
find myself using "-f" if the command didn't succeed without it).
But when using "-f" touches other submodules than not using it the
user might experience a rather unpleasant surprise, I'm not sure we
want to go that way.

> If we accept to drop this and only drop the changes when subsha1 != sha1, the patch can be much sorter by simply keeping the force flags I used and without modifying all the case/while thing.

Yes.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] submodule: Add --force option for git submodule update
  2011-03-30 19:05     ` Jens Lehmann
@ 2011-03-30 20:19       ` Nicolas Morey-Chaisemartin
  2011-03-30 21:08         ` Junio C Hamano
  0 siblings, 1 reply; 9+ messages in thread
From: Nicolas Morey-Chaisemartin @ 2011-03-30 20:19 UTC (permalink / raw)
  To: Jens Lehmann; +Cc: git

On 03/30/2011 09:05 PM, Jens Lehmann wrote:
>> I know it is very intrusive. The main reason for that is I wanted the -f option to always behave the same (meaning throw away changes),
>> whether the submodule is already on the right commit or not.
> 
> Hmm, I don't know if that is a good thing to do. People are used to
> "git submodule update" to only touch those submodule where the HEAD
> differs from the commit recorded in the superproject (And I often
> find myself using "-f" if the command didn't succeed without it).
> But when using "-f" touches other submodules than not using it the
> user might experience a rather unpleasant surprise, I'm not sure we
> want to go that way.
> 

Well I was'nt sure about it either.
The idea for me was to be able to put the repo and submodules in the cloned state (except for ignored files)
In the current version the right thing to do is a bit of a mess:
$ git submodule foreach --recursive 'git checkout -f HEAD'
$ git submodule foreach --recursive 'git clean -f' # An untracked file on HEAD may be overwrittent by the new HEAD so checkout may fail if you don't do that
$ git submodule update --recursive

Hopefully with --force we can simplify to:
$ git submodule foreach --recursive 'git checkout -f HEAD'
$ git submodule update --recursive --force

However the first step is not that trivial for most people (I sadly have too many first hand examples with my git users).
If the --force option only works on submodule where HEAD differs from the reference, the first step will still be necessary !
I find this a bit redundant.

Maybe another way would be to add another option.
Keep the --force as you proposed. And add a --all option of some sort that let the user decides if he wants to hit all the submodules or not.
This way we keep the regular behaviour with -f but provide a way to bypass the first step above.

Nicolas Morey-Chaisemartin

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] submodule: Add --force option for git submodule update
  2011-03-30 20:19       ` Nicolas Morey-Chaisemartin
@ 2011-03-30 21:08         ` Junio C Hamano
  2011-03-31  2:20           ` Nicolas Morey-Chaisemartin
  0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2011-03-30 21:08 UTC (permalink / raw)
  To: Nicolas Morey-Chaisemartin; +Cc: Jens Lehmann, git

Nicolas Morey-Chaisemartin <devel-git@morey-chaisemartin.com> writes:

> On 03/30/2011 09:05 PM, Jens Lehmann wrote:
>>> I know it is very intrusive. The main reason for that is I wanted the -f option to always behave the same (meaning throw away changes),
>>> whether the submodule is already on the right commit or not.
>> 
>> Hmm, I don't know if that is a good thing to do. People are used to
>> "git submodule update" to only touch those submodule where the HEAD
>> differs from the commit recorded in the superproject (And I often
>> find myself using "-f" if the command didn't succeed without it).
>> But when using "-f" touches other submodules than not using it the
>> user might experience a rather unpleasant surprise, I'm not sure we
>> want to go that way.
>
> Well I was'nt sure about it either.
> The idea for me was to be able to put the repo and submodules in the cloned state (except for ignored files)
> In the current version the right thing to do is a bit of a mess:
> $ git submodule foreach --recursive 'git checkout -f HEAD'
> $ git submodule foreach --recursive 'git clean -f' # An untracked file on HEAD may be overwrittent by the new HEAD so checkout may fail if you don't do that
> $ git submodule update --recursive

Shouldn't you be questioning _why_ your users have such changes that
require them to run "checkout -f" everywhere in the submodule forests and
still want to run "submodule update" in the first place?  If it happens
very often and your users are constantly discarding what they have half
accomplished, there is something wrong with the way your project works.

If we read your motivation section in your original,

  > This implies that to reset a whole git submodule tree, a user has to run
  > first 'git submodule foreach --recursive git checkout -f' to then be
  > able to run git submodule update.

this is about "resetting", i.e. throwing away the work.  I think that is a
sensible thing to do, but it is a very different purpose than "updating
submodules so that I can work on top of what other people did", which
would happen a lot more often than that.

Wouldn't it be both safer and easier to understand for the users if this
"obliterate all my uncommitted work" were a separate subcommand, e.g. "git
submodule reset --recursive" or something?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] submodule: Add --force option for git submodule update
  2011-03-30 21:08         ` Junio C Hamano
@ 2011-03-31  2:20           ` Nicolas Morey-Chaisemartin
  2011-03-31 17:41             ` Jens Lehmann
  0 siblings, 1 reply; 9+ messages in thread
From: Nicolas Morey-Chaisemartin @ 2011-03-31  2:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jens Lehmann, git

On 03/30/2011 11:08 PM, Junio C Hamano wrote:
> Nicolas Morey-Chaisemartin <devel-git@morey-chaisemartin.com> writes:
>> Well I was'nt sure about it either.
>> The idea for me was to be able to put the repo and submodules in the cloned state (except for ignored files)
>> In the current version the right thing to do is a bit of a mess:
>> $ git submodule foreach --recursive 'git checkout -f HEAD'
>> $ git submodule foreach --recursive 'git clean -f' # An untracked file on HEAD may be overwrittent by the new HEAD so checkout may fail if you don't do that
>> $ git submodule update --recursive
> 
> Shouldn't you be questioning _why_ your users have such changes that
> require them to run "checkout -f" everywhere in the submodule forests and
> still want to run "submodule update" in the first place?  If it happens
> very often and your users are constantly discarding what they have half
> accomplished, there is something wrong with the way your project works.
> 

I did. Thee reason is we commit file generated by compilation.
Whether it's docs, references (for automated integration) or simply result files that take 2 days to build,
we have a strong need to commit generated files.
And in this case, we need to discard a lot of things very often.

> If we read your motivation section in your original,
> 
>   > This implies that to reset a whole git submodule tree, a user has to run
>   > first 'git submodule foreach --recursive git checkout -f' to then be
>   > able to run git submodule update.
> 
> this is about "resetting", i.e. throwing away the work.  I think that is a
> sensible thing to do, but it is a very different purpose than "updating
> submodules so that I can work on top of what other people did", which
> would happen a lot more often than that.
> 
> Wouldn't it be both safer and easier to understand for the users if this
> "obliterate all my uncommitted work" were a separate subcommand, e.g. "git
> submodule reset --recursive" or something?
> 

I agree. A git submodule reset command makes a lot of sense.
But I also still think having a --force option to update does too, in the way Jens proposed it (only on submodule that actually needed a checkout), don't you?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] submodule: Add --force option for git submodule update
  2011-03-31  2:20           ` Nicolas Morey-Chaisemartin
@ 2011-03-31 17:41             ` Jens Lehmann
  2011-03-31 18:13               ` Nicolas Morey-Chaisemartin
  0 siblings, 1 reply; 9+ messages in thread
From: Jens Lehmann @ 2011-03-31 17:41 UTC (permalink / raw)
  To: Nicolas Morey-Chaisemartin; +Cc: Junio C Hamano, git

Am 31.03.2011 04:20, schrieb Nicolas Morey-Chaisemartin:
> On 03/30/2011 11:08 PM, Junio C Hamano wrote:
>> Shouldn't you be questioning _why_ your users have such changes that
>> require them to run "checkout -f" everywhere in the submodule forests and
>> still want to run "submodule update" in the first place?  If it happens
>> very often and your users are constantly discarding what they have half
>> accomplished, there is something wrong with the way your project works.
>
> I did. Thee reason is we commit file generated by compilation.
> Whether it's docs, references (for automated integration) or simply result files that take 2 days to build,
> we have a strong need to commit generated files.

We are in a similar situation at my dayjob, but we handled the issue in
a different way. We were reluctant to check in generated files into our
repository, so we decided to distribute them via zip files. We have a
beefy build machine which builds and tests master over night and if
everything went ok it zips the whole work tree including .git directory,
build artifacts and submodules into one file. Then each developer just
has to unpack this file, fetch from the git server and he is all set.

>> If we read your motivation section in your original,
>>
>>   > This implies that to reset a whole git submodule tree, a user has to run
>>   > first 'git submodule foreach --recursive git checkout -f' to then be
>>   > able to run git submodule update.
>>
>> this is about "resetting", i.e. throwing away the work.  I think that is a
>> sensible thing to do, but it is a very different purpose than "updating
>> submodules so that I can work on top of what other people did", which
>> would happen a lot more often than that.
>>
>> Wouldn't it be both safer and easier to understand for the users if this
>> "obliterate all my uncommitted work" were a separate subcommand, e.g. "git
>> submodule reset --recursive" or something?
> 
> I agree. A git submodule reset command makes a lot of sense.

Me too agrees that being able to reset a whole work tree including all
submodules recursively makes a lot of sense. But I would not be so happy
seeing this functionality being added to the git-submodule.sh script, as
I believe it belongs into "git reset", so you'll just have to do a "git
reset --hard --recurse-submodules" and you are done.

In the branch "git-checkout-recurse-submodules" in my github repo (which
is at https://github.com/jlehmann/git-submod-enhancements) you can see a
preliminary implementation of that (it works and I use it every day at my
job. It has recursion enabled by default at the moment yet still might
overwrite local changes even if it shouldn't, so use with care ;-).

> But I also still think having a --force option to update does too, in the way Jens proposed it (only on submodule that actually needed a checkout), don't you?

Yes, I still think it makes sense to add the "-f" flag to submodule update.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] submodule: Add --force option for git submodule update
  2011-03-31 17:41             ` Jens Lehmann
@ 2011-03-31 18:13               ` Nicolas Morey-Chaisemartin
  0 siblings, 0 replies; 9+ messages in thread
From: Nicolas Morey-Chaisemartin @ 2011-03-31 18:13 UTC (permalink / raw)
  To: Jens Lehmann; +Cc: git, Junio C Hamano

On 03/31/2011 07:41 PM, Jens Lehmann wrote:
> We are in a similar situation at my dayjob, but we handled the issue in
> a different way. We were reluctant to check in generated files into our
> repository, so we decided to distribute them via zip files. We have a
> beefy build machine which builds and tests master over night and if
> everything went ok it zips the whole work tree including .git directory,
> build artifacts and submodules into one file. Then each developer just
> has to unpack this file, fetch from the git server and he is all set.
> 

We have something a bit similar for cross project dependencies.
We just store in project a file with the SHA1 to another one and we have a server which stores all the packages ever generated
and a sqliteDB to have the  SHA1 <=> package relation.
It works but it requires to add DB files, CGI script to update/query the DB...
So we limited this only to a few dependencies between otherwise separated projects.
For the rest, it's just easier to commit generated small files.


> 
> Me too agrees that being able to reset a whole work tree including all
> submodules recursively makes a lot of sense. But I would not be so happy
> seeing this functionality being added to the git-submodule.sh script, as
> I believe it belongs into "git reset", so you'll just have to do a "git
> reset --hard --recurse-submodules" and you are done.
> 
Yes it is probably easier and clearer for everyone to put it into the dedicated command.
This way you know it does the same thing it usually does except on all submodules.
Thinking about "git submodule reset" I started wondering about what should be done for indexes, checked out files, ignored files and so on,
and whatever is chosen it will confuse pretty much everyone.
Also a git clean --recurse-submodules would be nice !

> In the branch "git-checkout-recurse-submodules" in my github repo (which
> is at https://github.com/jlehmann/git-submod-enhancements) you can see a
> preliminary implementation of that (it works and I use it every day at my
> job. It has recursion enabled by default at the moment yet still might
> overwrite local changes even if it shouldn't, so use with care ;-).
> 
I'll have a look at it. For the moment I use a bash script which recurse in all submodules doing git clean and git checkout all other the place
(it also works with old gits that lack all the foreach and --recursive features)

>> But I also still think having a --force option to update does too, in the way Jens proposed it (only on submodule that actually needed a checkout), don't you?
> 
> Yes, I still think it makes sense to add the "-f" flag to submodule update.
> 

If Junio agrees with the idea, I'll repost a much simpler version of the path that just add -f for the submodules that needs it.

Nicolas Morey-Chaisemartin

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2011-03-31 18:13 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-30  7:56 [PATCH] submodule: Add --force option for git submodule update Nicolas Morey-Chaisemartin
2011-03-30 18:32 ` Jens Lehmann
2011-03-30 18:50   ` Nicolas Morey-Chaisemartin
2011-03-30 19:05     ` Jens Lehmann
2011-03-30 20:19       ` Nicolas Morey-Chaisemartin
2011-03-30 21:08         ` Junio C Hamano
2011-03-31  2:20           ` Nicolas Morey-Chaisemartin
2011-03-31 17:41             ` Jens Lehmann
2011-03-31 18:13               ` Nicolas Morey-Chaisemartin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).