mbox series

[v3,0/3] Teach submodule set-branch subcommand

Message ID cover.1549534460.git.liu.denton@gmail.com (mailing list archive)
Headers show
Series Teach submodule set-branch subcommand | expand

Message

Denton Liu Feb. 7, 2019, 10:18 a.m. UTC
I rebased the changes onto the latest 'next' because if this branch gets
merged into 'next', there'll be merge conflicts from
'dl/complete-submodule-absorbgitdirs'.

Currently, there is no way to set the branch of a submodule without
manually manipulating the .gitmodules file. This patchset introduces a
porcelain command that enables this.

Changes since v1:
	* Fixed incorrect usage of OPT_CMDMODE

Changes since v2:
	* Corrected missing argument for -b/--branch in git-submodule.txt
	* Rebased onto latest next


Denton Liu (3):
  git-submodule.txt: "--branch <branch>" option defaults to 'master'
  submodule--helper: teach config subcommand --unset
  submodule: teach set-branch subcommand

 Documentation/git-submodule.txt        | 14 +++-
 builtin/submodule--helper.c            | 18 +++--
 contrib/completion/git-completion.bash |  5 +-
 git-submodule.sh                       | 75 +++++++++++++++++++--
 t/t7411-submodule-config.sh            |  9 +++
 t/t7419-submodule-set-branch.sh        | 93 ++++++++++++++++++++++++++
 6 files changed, 200 insertions(+), 14 deletions(-)
 create mode 100755 t/t7419-submodule-set-branch.sh

Comments

Junio C Hamano Feb. 7, 2019, 6:01 p.m. UTC | #1
Denton Liu <liu.denton@gmail.com> writes:

> I rebased the changes onto the latest 'next' because if this branch gets
> merged into 'next', there'll be merge conflicts from
> 'dl/complete-submodule-absorbgitdirs'.

Please don't do that.

A topic that depends on everything in 'next' cannot graduate to
'master' until everything that is cooking in 'next' does.

When

 - the "conflict" that would arise is so trivial to resolve,

 - there is no semantic crashes between the new topic and existing
    ones, and

 - the topic is usable even before other topics graduate (or even
   when they get discarded)

please make it a habit to avoid making your topic (i.e. this one)
hostage to another topic (i.e. the absorbgitdirs one), and certainly
not hostage to the whole of 'next'.  A trivial conflict resolution
in this case, if you revert the rebasing and then attempt to merge
the result to 'next', would look like this.

diff --cc contrib/completion/git-completion.bash
index 8b3b5a9d34,de56879960..0000000000
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@@ -2573,7 -2573,7 +2573,7 @@@ _git_submodule (
  {
  	__git_has_doubledash && return
  
- 	local subcommands="add status init deinit update set-branch summary foreach sync"
 -	local subcommands="add status init deinit update summary foreach sync absorbgitdirs"
++	local subcommands="add status init deinit update set-branch summary foreach sync absorbgitdirs"
  	local subcommand="$(__git_find_on_cmdline "$subcommands")"
  	if [ -z "$subcommand" ]; then
  		case "$cur" in


By the way, if conflicts worry you so much, one thing you could do
is this.  Instead of maintaining the ever-growing list of
subcommands and having to worry about textual conflicts, the
completion script could use a clean-up to reduce the need of textual
conflict resolution, when it is quiescent, perhaps like this (I am
assuming that in some future, there is a quiescent period _after_
both of these two topics landed, and this illustration patch is to
be used in such a future).

Merging two topics, each of which adds a new element by inserting a
new line with the element (and the element alone) is on it, is
certainly a lot easier and simpler than having to see what word is
getting inserted by each topic on a single long string on a line.


 contrib/completion/git-completion.bash | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 0fccadfc97..5d7d4ebacc 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2573,7 +2573,18 @@ _git_submodule ()
 {
 	__git_has_doubledash && return
 
-	local subcommands="add status init deinit update set-branch summary foreach sync absorbgitdirs"
+	local subcommands="
+		add
+		status
+		init
+		deinit
+		update
+		set-branch
+		summary
+		foreach
+		sync
+		absorbgitdirs
+	"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 	if [ -z "$subcommand" ]; then
 		case "$cur" in
Denton Liu Feb. 8, 2019, 5:31 a.m. UTC | #2
On Thu, Feb 07, 2019 at 10:01:40AM -0800, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > I rebased the changes onto the latest 'next' because if this branch gets
> > merged into 'next', there'll be merge conflicts from
> > 'dl/complete-submodule-absorbgitdirs'.
> 
> Please don't do that.
> 
> A topic that depends on everything in 'next' cannot graduate to
> 'master' until everything that is cooking in 'next' does.
> 
> When
> 
>  - the "conflict" that would arise is so trivial to resolve,
> 
>  - there is no semantic crashes between the new topic and existing
>     ones, and
> 
>  - the topic is usable even before other topics graduate (or even
>    when they get discarded)
> 
> please make it a habit to avoid making your topic (i.e. this one)
> hostage to another topic (i.e. the absorbgitdirs one), and certainly
> not hostage to the whole of 'next'.  A trivial conflict resolution
> in this case, if you revert the rebasing and then attempt to merge
> the result to 'next', would look like this.

My mistake, even though I've made a few contributions already, I'm still
getting used to the git.git workflow.

Thanks so much for helping me out and please don't hesitate to let me
know if I'm doing anything else wrong.

By the way, just for the record, how would you like me to handle
patchsets that cause merge conflicts?

Thanks,

Denton
Junio C Hamano Feb. 8, 2019, 6:43 p.m. UTC | #3
Denton Liu <liu.denton@gmail.com> writes:

> By the way, just for the record, how would you like me to handle
> patchsets that cause merge conflicts?

When it happens, I may ask you to rebase onto a specific commit.
https://public-inbox.org/git/xmqq5ztxstkh.fsf@gitster-ct.c.googlers.com/
is a recent example.

My preference (read: I'd be grateful if contributors tried to stick
to it, but it won't be the reason for patch rejection if they don't)
is:

 1. Choose the right base:

   a) For a fix to a bug that already exists in the last feature
      release (i.e. v2.20.0), build on 'maint' and make sure it
      merges cleanly to 'master', and the merge result builds and
      passes tests.  Depending on the severity of the bug, we might
      want to make sure that the fix applies maintenance track even
      older, but it would be something you would be sending patches
      to git-security mailing list---over there we'll figure out the
      right base on case-by-case basis.

   b) For a new feature, build on 'master', if you can.

   c) If you need to use something (i.e. a new helper function,
      updated type, etc.) in 'next' that is not yet in 'master',
      identify the topic(s) that introduces these things you need,
      merge them to 'master' and build on top of it.  If you did so,
      please note in the cover letter what topics you depend on.

   d) When updating a topic that is already in my tree
      (i.e. rerolling), stick to the same base as the previous
      round, if possible.  You can find out the commit the previous
      round was applied on top by fetching from me.  Your reroll may
      start using something the previous round did not use from
      'next', in which case you may end up doing c) above, which is
      OK.  Just make a note in the cover letter to let reviewers
      know.

 2. Make sure the result builds and passes tests.

 3. Make sure the result merges cleanly to 'next', and the merge
    result builds and passes tests.

 4. In any of the above steps where you are asked to "make sure it
    merges cleanly", you may see merge conflicts.

   a) If they are too much to resolve for you, for a change that is
      not a bugfix, you may have to depend on the other topic(s).
      Identify the topic(s) that touches these lines that heavily
      conflict with your changes, merge them to 'master' and base
      your topic on top of it (i.e. going back to step 1.c).

   b) If they are easy to resolve for you, do not worry too much
      about them.  It may be helpful to note in the cover letter
      "this may have minor textual conflict with these other
      topics".
   
   c) If you are left with huge conflict while working on a bugfix
      change, we need to resolve it on case-by-case basis, so send
      it out with the chosen base.  Noting that it conflicts heavily
      with 'master' or 'next' would be very much appreciated when
      this happens.

Thanks.