threads / discuss / 37913

Feedback on submitting patches / here submodule

Subject: Feedback on submitting patches / here submodule

## tl;dr

3 messages between Nov 9, 2014 and Nov 9, 2014.

replies: 2people: 3as markdown or json

Tom Freudenberg· Nov 9, 2014, 11:39 UTC · lore

Hi, a few weeks ago I send out attached message and patch to gitster-at-pobox.com tried to follow the guidelines.

Have not get any feedback until now, neither acceptance nor decline … just no feedback.

I am not sure if the message was received or this is some kind of intended procedure. From my personal point of view it would be nice and polite to get an answer for postings.

Hopefully I got some feedback from this list to whom I should send the patch for review or denial.

Thanks Tom

=========================================================================== =

Subject: FEATURE: submodule: checkout and update from remote stay tracked
to upstream

Hi, hopefully I added everything that you will check this feature if it could be merged to git-submodule.sh

If have not yet added tests nor documentation. If you agree on this feature in general, I will make the additional work for tests and documentation.

Patch is for working on maint as well as on master.
Diff is available also on my git fork on github at:

https://github.com/TomFreudenberg/git/compare/git:master...TomFreudenberg:m aster-added-feature-submodule-track

Thanks for some feedback. Tom

=====================================================================
submodule: checkout and update from remote stay tracked to upstream

With this change, cloning submodules for the first time or running updates afterwards via:

$ git submodule update ‹remote ‹track ...

will get a local branch instead of a detached HEAD. Following updates check track to upstream branch and merge if matching. Checkout to detached head can be forced afterwards. Checkout will test and warn if checkout modes are not compatible.

The motivation for the change is that developers doing local work inside the submodule are likely to select a non-checkout-mode for updates so their local work is integrated with upstream work. Developers who are not doing local submodule work stick with checkout-mode updates so any apparently local work is blown away during updates. For example, if upstream rolls back the remote branch or gitlinked commit to an earlier version, the checkout-mode developer wants their old submodule checkout to be rolled back as well, instead of getting a no-op merge/rebase with the rolled-back reference.

By using the update mode to distinguish submodule developers from black-box submodule consumers, we can setup local branches for the developers who will want local branches, and stick with detached HEADs for the developers that don't care.

Testing =======

Not yet updated

Documentation =============

Not yet updated
Signed-off-by: Tom Freudenberg <tom.freudenberg@4commerce.de>
=====================================================================
diff --git a/git-submodule.sh b/git-submodule.sh
index 9245abf..60a2145 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -9,7 +9,7 @@ USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name
<name>] [--reference <re
    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] [--checkout|--merge|--rebase] [--reference <repository>]
[--recursive] [--] [<path>...]
+   or: $dashless [--quiet] update [--init] [--remote [--track]]
[-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference
<repository>] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit
<n>] [commit] [--] [<path>...]
    or: $dashless [--quiet] foreach [--recursive] <command>
    or: $dashless [--quiet] sync [--recursive] [--] [<path>...]"
@@ -31,6 +31,7 @@ recursive=
init=
files=
remote=
+track=
nofetch=
update=
prefix=
@@ -736,6 +737,9 @@ cmd_update()
		--remote)
			remote=1
			;;
+		--track)
+			test -n "$remote" && track="--track" || usage
+			;;
		-N|--no-fetch)
			nofetch=1
			;;
@@ -837,10 +841,14 @@ Maybe you want to use 'update --init'?")"
			module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit
			cloned_modules="$cloned_modules;$name"
			subsha1=
+			subupstream=
		else
			subsha1=$(clear_local_git_env; cd "$sm_path" &&
				git rev-parse --verify HEAD) ||
			die "$(eval_gettext "Unable to find current revision in submodule path
'\$displaypath'")"
+			# get optional upstream info
+			subupstream=$(clear_local_git_env; cd "$sm_path" &&
+				git rev-parse --symbolic-full-name --abbrev-ref '@{u}' 2>/dev/null)
		fi
		if test -n "$remote"
@@ -855,6 +863,21 @@ Maybe you want to use 'update --init'?")"
			sha1=$(clear_local_git_env; cd "$sm_path" &&
				git rev-parse --verify "${remote_name}/${branch}") ||
			die "$(eval_gettext "Unable to find current ${remote_name}/${branch}
revision in submodule path '\$sm_path'")"
+			# track upsteam
+			if test -n "$track"
+			then
+				if test -z "$subsha1"
+				then
+					# first time checkout use symbol name to track
+					sha1="${remote_name}/${branch}"
+				else
+					# test that already on the same upstream
+					test "${remote_name}/${branch}" = "$subupstream" ||
+					die "$(eval_gettext "Unable to merge. Current does not track
upstream ${remote_name}/${branch} in submodule path '\$sm_path'")"
+					# just merge with upstream
+					update_module="merge"
+				fi
+			fi
		fi
		if test "$subsha1" != "$sha1" || test -n "$force"
@@ -883,10 +906,17 @@ Maybe you want to use 'update --init'?")"
				update_module=checkout ;;
			esac
+			if test "$update_module" = "checkout"
+			then
+				# warn and abort if on upstream and not forced
+				test -n "$subupstream" -a -z "$subforce" &&
+					die "$(eval_gettext "Use --force to checkout detached HEAD.
Currently on upstream ${remote_name}/${branch} in submodule path
'\$sm_path'")"
+			fi
+
			must_die_on_failure=
			case "$update_module" in
			checkout)
-				command="git checkout $subforce -q"
+				command="git checkout $subforce $track -q"
				die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule
path '\$displaypath'")"
				say_msg="$(eval_gettext "Submodule path '\$displaypath': checked out
'\$sha1'")"
				;;
Jens Lehmann· Nov 9, 2014, 16:24 UTC · re: Tom Freudenberg · lore

Re: Feedback on submitting patches / here submodule

Am 09.11.2014 um 12:39 schrieb Tom Freudenberg:
Show 13 quoted lines
> Hi, a few weeks ago I send out attached message and patch to
> gitster-at-pobox.com tried to follow the guidelines.
>
> Have not get any feedback until now, neither acceptance nor decline … just
> no feedback.
>
> I am not sure if the message was received or this is some kind of intended
> procedure.
>  From my personal point of view it would be nice and polite to get an
> answer for postings.
>
> Hopefully I got some feedback from this list to whom I should send the
> patch for review or denial.

A patch should go to: gitster@pobox.com /and/ cc: git@vger.kernel.org (and optinally any people relevant to the area you are touching). Just sending it to Junio will most probably result in it being overlooked in his overflowing inbox (please see Documentation/SubmittingPatches for a more detailed description).

Show 17 quoted lines
> Thanks
> Tom
>
>
> ===========================================================================
> =
>
> Subject: FEATURE: submodule: checkout and update from remote stay tracked
> to upstream
>
>
> Hi, hopefully I added everything that you will check this feature
> if it could be merged to git-submodule.sh
>
> If have not yet added tests nor documentation. If you agree on
> this feature in general, I will make the additional work for
> tests and documentation.

The same goal was pursued by Trevor some time ago in 23d25e48f ("submodule: explicit local branch creation in module_clone"), and was considered a worthwhile change. You can find the whole discussion here:

   http://thread.gmane.org/gmane.comp.version-control.git/239799

But it had to be reverted shortly after because it broke detached HEAD checkouts quite badly, details under:

   http://thread.gmane.org/gmane.comp.version-control.git/245283
You're welcome to revive this topic.
Show 173 quoted lines
> Patch is for working on maint as well as on master.
>
> Diff is available also on my git fork on github at:
>
> https://github.com/TomFreudenberg/git/compare/git:master...TomFreudenberg:m
> aster-added-feature-submodule-track
>
>
> Thanks for some feedback.
> Tom
>
>
> =====================================================================
>
>
> submodule: checkout and update from remote stay tracked to upstream
>
> With this change, cloning submodules for the first time or running
> updates afterwards via:
>
> $ git submodule update ‹remote ‹track ...
>
> will get a local branch instead of a detached HEAD. Following updates
> check track to upstream branch and merge if matching. Checkout to
> detached head can be forced afterwards. Checkout will test and warn
> if checkout modes are not compatible.
>
> The motivation for the change is that developers doing local work
> inside the submodule are likely to select a non-checkout-mode for
> updates so their local work is integrated with upstream work.
> Developers who are not doing local submodule work stick with
> checkout-mode updates so any apparently local work is blown away
> during updates.  For example, if upstream rolls back the remote branch
> or gitlinked commit to an earlier version, the checkout-mode developer
> wants their old submodule checkout to be rolled back as well, instead
> of getting a no-op merge/rebase with the rolled-back reference.
>
> By using the update mode to distinguish submodule developers from
> black-box submodule consumers, we can setup local branches for the
> developers who will want local branches, and stick with detached HEADs
> for the developers that don't care.
>
>
>
> Testing
> =======
>
> Not yet updated
>
>
>
> Documentation
> =============
>
> Not yet updated
>
>
>
> Signed-off-by: Tom Freudenberg <tom.freudenberg@4commerce.de>
>
>
>
>
> =====================================================================
>
>
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 9245abf..60a2145 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -9,7 +9,7 @@ USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name
> <name>] [--reference <re
>      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] [--checkout|--merge|--rebase] [--reference <repository>]
> [--recursive] [--] [<path>...]
> +   or: $dashless [--quiet] update [--init] [--remote [--track]]
> [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference
> <repository>] [--recursive] [--] [<path>...]
>      or: $dashless [--quiet] summary [--cached|--files] [--summary-limit
> <n>] [commit] [--] [<path>...]
>      or: $dashless [--quiet] foreach [--recursive] <command>
>      or: $dashless [--quiet] sync [--recursive] [--] [<path>...]"
> @@ -31,6 +31,7 @@ recursive=
> init=
> files=
> remote=
> +track=
> nofetch=
> update=
> prefix=
> @@ -736,6 +737,9 @@ cmd_update()
> 		--remote)
> 			remote=1
> 			;;
> +		--track)
> +			test -n "$remote" && track="--track" || usage
> +			;;
> 		-N|--no-fetch)
> 			nofetch=1
> 			;;
> @@ -837,10 +841,14 @@ Maybe you want to use 'update --init'?")"
> 			module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit
> 			cloned_modules="$cloned_modules;$name"
> 			subsha1=
> +			subupstream=
> 		else
> 			subsha1=$(clear_local_git_env; cd "$sm_path" &&
> 				git rev-parse --verify HEAD) ||
> 			die "$(eval_gettext "Unable to find current revision in submodule path
> '\$displaypath'")"
> +			# get optional upstream info
> +			subupstream=$(clear_local_git_env; cd "$sm_path" &&
> +				git rev-parse --symbolic-full-name --abbrev-ref '@{u}' 2>/dev/null)
> 		fi
> 		if test -n "$remote"
> @@ -855,6 +863,21 @@ Maybe you want to use 'update --init'?")"
> 			sha1=$(clear_local_git_env; cd "$sm_path" &&
> 				git rev-parse --verify "${remote_name}/${branch}") ||
> 			die "$(eval_gettext "Unable to find current ${remote_name}/${branch}
> revision in submodule path '\$sm_path'")"
> +			# track upsteam
> +			if test -n "$track"
> +			then
> +				if test -z "$subsha1"
> +				then
> +					# first time checkout use symbol name to track
> +					sha1="${remote_name}/${branch}"
> +				else
> +					# test that already on the same upstream
> +					test "${remote_name}/${branch}" = "$subupstream" ||
> +					die "$(eval_gettext "Unable to merge. Current does not track
> upstream ${remote_name}/${branch} in submodule path '\$sm_path'")"
> +					# just merge with upstream
> +					update_module="merge"
> +				fi
> +			fi
> 		fi
> 		if test "$subsha1" != "$sha1" || test -n "$force"
> @@ -883,10 +906,17 @@ Maybe you want to use 'update --init'?")"
> 				update_module=checkout ;;
> 			esac
> +			if test "$update_module" = "checkout"
> +			then
> +				# warn and abort if on upstream and not forced
> +				test -n "$subupstream" -a -z "$subforce" &&
> +					die "$(eval_gettext "Use --force to checkout detached HEAD.
> Currently on upstream ${remote_name}/${branch} in submodule path
> '\$sm_path'")"
> +			fi
> +
> 			must_die_on_failure=
> 			case "$update_module" in
> 			checkout)
> -				command="git checkout $subforce -q"
> +				command="git checkout $subforce $track -q"
> 				die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule
> path '\$displaypath'")"
> 				say_msg="$(eval_gettext "Submodule path '\$displaypath': checked out
> '\$sha1'")"
> 				;;
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Junio C Hamano· Nov 9, 2014, 17:38 UTC · re: Jens Lehmann · lore

Re: Feedback on submitting patches / here submodule

Jens Lehmann <Jens.Lehmann@web.de> writes:
> A patch should go to: gitster@pobox.com /and/ cc: git@vger.kernel.org
> (and optinally any people relevant to the area you are touching).

No. A patch should go To: git@vger.kernel.org and Cc: those who are involved in the area, and after those involved in the area agree that it is a good change, please Cc: me with acks from them.

For submodule patches, I am unlikely to be on the To: or Cc: list of the initial proposal.

← back to recent threads