[PATCH v3 0/5] implement branch --recurse-submodules

[Glen Choo <chooglen@google.com>]

Submodule branching RFC:
https://lore.kernel.org/git/kl6lv912uvjv.fsf@chooglen-macbookpro.roam.corp.google.com/

Original Submodule UX RFC/Discussion:
https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com/

Contributor Summit submodules Notes:
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110211148060.56@tvgsbejvaqbjf.bet/

Submodule UX overhaul updates:
https://lore.kernel.org/git/?q=Submodule+UX+overhaul+update

This series implements branch --recurse-submodules as laid out in the
Submodule branching RFC (linked above). If there are concerns about the
UX/behavior, I would appreciate feedback on the RFC thread as well :)

This series is based off js/branch-track-inherit.

This version is functionally identical to v2. I've only addressed
feedback around code organization, i.e. the the merge conflict with
js/branch-track-inherit and making patch 1 easier to review. Thus, some
discussions on [1] are still unaddressed.

Patch 5 is an optional cleanup of the questionable exit codes that Junio
found [1]. I wasn't able to figure out the intent of the original
authors, so it is mostly a best-guess at the right exit code. It doesn't
cause any tests to fail, but this wasn't covered by tests to begin with.

Changes since v2
* Rebase onto js/branch-track-inherit. This series should continue to be
  the case going forward.
* Patch 1 has a smaller diff because the introduction of
  validate_branch_start() no longer changes the function order thanks to a
  forward declaration. This artificial forward declaration is removed in a
  patch 2 (which can just be squashed into patch 1).
* Optional cleanup: fix questionable exit codes in patch 5.

Changes since v1:
* Move the functionality of "git branch --dry-run" into "git submodule-helper create-branch --dry-run"
* Add more fields to the submodules_of_tree() struct to reduce the
  number of allocations made by the caller [2]. Move this functionality
  to patch 3 (formerly patch 4) and drop patch 1.
* Make submodules_of_tree() ignore inactive submodules [3]
* Structure the output of the submodules a bit better by adding prefixes
  to the child process' output (instead of inconsistently indenting the
  output).
** I wasn't able to find a good way to interleave stdout/stderr
   correctly, so a less-than-desirable workaround was to route the child
   process output to stdout/stderr depending on the exit code.
** Eventually, I would like to structure the output of submodules in a
   report, as Ævar suggested [4]. But at this stage, I think that it's
   better to spend time getting user feedback on the submodules
   branching UX and it'll be easier to standardize the output when we've
   implemented more of the UX :)

[1] https://lore.kernel.org/git/xmqqbl1tcptq.fsf@gitster.g
[2] https://lore.kernel.org/git/211123.86r1b7uoil.gmgdl@evledraar.gmail.com
[3] https://lore.kernel.org/git/3ad3941c-de18-41bf-2e44-4238ae868d79@gmail.com
[4] https://lore.kernel.org/git/211123.86v90juovj.gmgdl@evledraar.gmail.com

Glen Choo (5):
  branch: move --set-upstream-to behavior to setup_tracking()
  branch: remove forward declaration of validate_branch_start()
  builtin/branch: clean up action-picking logic in cmd_branch()
  branch: add --recurse-submodules option for branch creation
  branch.c: replace questionable exit() codes

 Documentation/config/advice.txt    |   3 +
 Documentation/config/submodule.txt |   8 +
 advice.c                           |   1 +
 advice.h                           |   1 +
 branch.c                           | 336 +++++++++++++++++++++--------
 branch.h                           |  44 +++-
 builtin/branch.c                   |  66 ++++--
 builtin/checkout.c                 |   3 +-
 builtin/submodule--helper.c        |  38 ++++
 submodule-config.c                 |  35 +++
 submodule-config.h                 |  35 +++
 submodule.c                        |  11 +-
 submodule.h                        |   3 +
 t/t3200-branch.sh                  |  17 ++
 t/t3207-branch-submodule.sh        | 284 ++++++++++++++++++++++++
 15 files changed, 765 insertions(+), 120 deletions(-)
 create mode 100755 t/t3207-branch-submodule.sh

Range-diff against v2:
1:  cc212dcd39 ! 1:  8241c0b51a branch: move --set-upstream-to behavior to setup_tracking()
    @@ Commit message
         Signed-off-by: Glen Choo <chooglen@google.com>
     
      ## branch.c ##
    -@@ branch.c: int install_branch_config(int flag, const char *local, const char *origin, const
    - 	return -1;
    +@@ branch.c: static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
    + 	return 0;
      }
      
     -/*
    @@ branch.c: int install_branch_config(int flag, const char *local, const char *ori
     - */
     -static void setup_tracking(const char *new_ref, const char *orig_ref,
     -			   enum branch_track track, int quiet)
    --{
    --	struct tracking tracking;
    --	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
    --
    --	memset(&tracking, 0, sizeof(tracking));
    ++static void validate_branch_start(struct repository *r, const char *start_name,
    ++				  enum branch_track track,
    ++				  struct object_id *oid, char **real_ref);
    ++
    ++void setup_tracking(const char *new_ref, const char *orig_ref,
    ++			   enum branch_track track, int quiet, int expand_orig)
    + {
    + 	struct tracking tracking;
    + 	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
    + 	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
    ++	char *full_orig_ref;
    ++	struct object_id unused_oid;
    + 
    + 	memset(&tracking, 0, sizeof(tracking));
     -	tracking.spec.dst = (char *)orig_ref;
    --	if (for_each_remote(find_tracked_branch, &tracking))
    --		return;
    --
    --	if (!tracking.matches)
    --		switch (track) {
    --		case BRANCH_TRACK_ALWAYS:
    --		case BRANCH_TRACK_EXPLICIT:
    --		case BRANCH_TRACK_OVERRIDE:
    --			break;
    --		default:
    ++	if (expand_orig)
    ++		validate_branch_start(the_repository, orig_ref, track, &unused_oid, &full_orig_ref);
    ++	else
    ++		full_orig_ref = xstrdup(orig_ref);
    ++
    ++	tracking.spec.dst = full_orig_ref;
    + 	tracking.srcs = &tracking_srcs;
    + 	if (track != BRANCH_TRACK_INHERIT)
    + 		for_each_remote(find_tracked_branch, &tracking);
    +@@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
    + 		case BRANCH_TRACK_OVERRIDE:
    + 			break;
    + 		default:
     -			return;
    --		}
    --
    --	if (tracking.matches > 1)
    --		die(_("Not tracking: ambiguous information for ref %s"),
    ++			goto cleanup;
    + 		}
    + 
    + 	if (tracking.matches > 1)
    + 		die(_("Not tracking: ambiguous information for ref %s"),
     -		    orig_ref);
    --
    --	if (install_branch_config(config_flags, new_ref, tracking.remote,
    --			      tracking.src ? tracking.src : orig_ref) < 0)
    --		exit(-1);
    --
    --	free(tracking.src);
    --}
    --
    ++		    full_orig_ref);
    + 
    + 	if (tracking.srcs->nr < 1)
    +-		string_list_append(tracking.srcs, orig_ref);
    ++		string_list_append(tracking.srcs, full_orig_ref);
    + 	if (install_branch_config_multiple_remotes(config_flags, new_ref, tracking.remote,
    + 			      tracking.srcs) < 0)
    + 		exit(-1);
    + 
    ++cleanup:
    + 	string_list_clear(tracking.srcs, 0);
    ++	free(full_orig_ref);
    + }
    + 
      int read_branch_desc(struct strbuf *buf, const char *branch_name)
    - {
    - 	char *v = NULL;
     @@ branch.c: N_("\n"
      "will track its remote counterpart, you may want to use\n"
      "\"git push -u\" to set the upstream config as you push.");
    @@ branch.c: void create_branch(struct repository *r,
     +	oidcpy(oid, &commit->object.oid);
     +}
     +
    -+void setup_tracking(const char *new_ref, const char *orig_ref,
    -+			   enum branch_track track, int quiet, int expand_orig)
    -+{
    -+	struct tracking tracking;
    -+	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
    -+	char *full_orig_ref;
    -+	struct object_id unused_oid;
    -+
    -+	memset(&tracking, 0, sizeof(tracking));
    -+	if (expand_orig)
    -+		validate_branch_start(the_repository, orig_ref, track, &unused_oid, &full_orig_ref);
    -+	else
    -+		full_orig_ref = xstrdup(orig_ref);
    -+
    -+	tracking.spec.dst = full_orig_ref;
    -+	if (for_each_remote(find_tracked_branch, &tracking))
    -+		goto cleanup;
    -+
    -+	if (!tracking.matches)
    -+		switch (track) {
    -+		case BRANCH_TRACK_ALWAYS:
    -+		case BRANCH_TRACK_EXPLICIT:
    -+		case BRANCH_TRACK_OVERRIDE:
    -+			break;
    -+		default:
    -+			goto cleanup;
    -+		}
    -+
    -+	if (tracking.matches > 1)
    -+		die(_("Not tracking: ambiguous information for ref %s"),
    -+		    full_orig_ref);
    -+
    -+	if (install_branch_config(config_flags, new_ref, tracking.remote,
    -+			      tracking.src ? tracking.src : full_orig_ref) < 0)
    -+		exit(-1);
    -+
    -+cleanup:
    -+	free(tracking.src);
    -+	free(full_orig_ref);
    -+}
    -+
     +void create_branch(struct repository *r, const char *name,
     +		   const char *start_name, int force, int clobber_head_ok,
     +		   int reflog, int quiet, enum branch_track track)
-:  ---------- > 2:  b74bcbaade branch: remove forward declaration of validate_branch_start()
2:  320749cc82 = 3:  235173efc9 builtin/branch: clean up action-picking logic in cmd_branch()
3:  c0441c6691 = 4:  3dabb8e2fa branch: add --recurse-submodules option for branch creation
-:  ---------- > 5:  70fb03f882 branch.c: replace questionable exit() codes
-- 
2.33.GIT