[0/5] submodule: remove "--recursive-prefix"

Message

Phillip Wood via GitGitGadget June 27, 2022, 11:20 p.m. UTC

= Description

This series is a refactor of "git submodule--helper update" that replaces
"--recursive-prefix" with "--super-prefix" (see Background). This was
initially motivated by:

 * Junio's suggestion to simplify the code [1] (in response to a memory leak
   found by Johannes Schindelin [2]).
 * A desire to use the module_list API + get_submodule_displaypath() outside
   of builtin/submodule--helper.c (I expect to use this to check out
   branches in each submodule).

But it also happens to remove some overly complicated/duplicated code that
was literally converted from shell :)

= Background

When recursing into nested submodules, Git commands keep track of the path
from the superproject to the submodule in order to print a "display path" to
the user, e.g.

Submodule path '../super/sub/nested-sub': checked out 'abcdef'

For historical reasons, "git submodule--helper update" uses
"--recursive-prefix" for this purpose, but it should use "--super-prefix"
instead because:

 * That's what every other command uses (not just submodule--helper
   subcommands).
 * Using the "--super-prefix" helper functions makes the "git
   submodule--helper update" code simpler

= Patch organization

 * Patch 1/5 makes sure that display paths are only computed using display
   path helper functions ([do_]get_submodule_displaypath()) and fixes some
   display paths that we never realized were broken.
 * Patches 2-3/5 simplify and deduplicate some display path computations.
 * Patch 4/5 replaces "--recursive-prefix" with "--super-prefix", making
   do_get_submodule_displaypath() obsolete.
 * Patch 5/5 removes do_get_submodule_displaypath().

= Interactions with other series

This would have been rebased onto ab/submodule-cleanup, but it's not yet
clear to me if that series will be merged first. That series is almost done,
but Ævar is still doing some digging on SUPPORT_SUPER_PREFIX [3] and may
come back with findings that affect this series.

Fortunately, this series is only tangentially related to
ab/submodule-cleanup, and the conflicts are quite simple:

| this | ab/submodule-cleanup | resolution |
|-----------------------------+-------------------------------+---------------------------|
| push --super-prefix arg | add new "ud" var | keep both |
|-----------------------------+-------------------------------+---------------------------|
| remove "--recursive-prefix" | add "--checkout", "--rebase", | keep both |
| | "--merge" | |
|-----------------------------+-------------------------------+---------------------------|
| add SUPPORT_SUPER_PREFIX | remove SUPPORT_SUPER_PREFIX | keep
ab/submodule-cleanup | | to "git submodule--helper | from "git
submodule--helper" | | | update" | | |

= Future work

At the end of this series, get_submodule_displaypath() can be moved to
submodule.h, which would make submodule.c:get_super_prefix_or_empty()
obsolete. I have a patch that demonstrates this (CI run: [4]), but I decided
to keep this series more focused on "git submodule--helper update" so that
it's easier to review.

[1] https://lore.kernel.org/git/xmqq35g5xmmv.fsf@gitster.g [2]
https://lore.kernel.org/git/877a[e-mail]
[3]
https://lore.kernel.org/git/[e-mail]
[4] https://github.com/chooglen/git/actions/runs/2572557584

Glen Choo (5):
  submodule--helper update: use display path helper
  submodule--helper: don't recreate recursive prefix
  submodule--helper: use correct display path helper
  submodule--helper update: use --super-prefix
  submodule--helper: remove display path helper

 builtin/submodule--helper.c | 82 +++++++++----------------------------
 t/t7406-submodule-update.sh | 59 ++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 62 deletions(-)


base-commit: 39c15e485575089eb77c769f6da02f98a55905e0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1282%2Fchooglen%2Fsubmodule%2Fremove-recursive-prefix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1282/chooglen/submodule/remove-recursive-prefix-v1
Pull-Request: https://github.com/git/git/pull/1282

Comments

Glen Choo June 28, 2022, 4:34 p.m. UTC | #1

"Glen Choo via GitGitGadget" <[e-mail]> writes:

> = Interactions with other series
>
> This would have been rebased onto ab/submodule-cleanup, but it's not yet
> clear to me if that series will be merged first. That series is almost done,
> but Ævar is still doing some digging on SUPPORT_SUPER_PREFIX [3] and may
> come back with findings that affect this series.
>
> Fortunately, this series is only tangentially related to
> ab/submodule-cleanup, and the conflicts are quite simple:
>
> | this | ab/submodule-cleanup | resolution |
> |-----------------------------+-------------------------------+---------------------------|
> | push --super-prefix arg | add new "ud" var | keep both |
> |-----------------------------+-------------------------------+---------------------------|
> | remove "--recursive-prefix" | add "--checkout", "--rebase", | keep both |
> | | "--merge" | |
> |-----------------------------+-------------------------------+---------------------------|
> | add SUPPORT_SUPER_PREFIX | remove SUPPORT_SUPER_PREFIX | keep
> ab/submodule-cleanup | | to "git submodule--helper | from "git
> submodule--helper" | | | update" | | |

Hm, maybe I'm not using GGG correctly, but these whitespace issues seem
pretty common (Markdown collapsing the whitespace maybe?). Maybe I need
to blockquote the monospaced stuff.

Here's the original table:

| this                        | ab/submodule-cleanup          | resolution                |
|-----------------------------+-------------------------------+---------------------------|
| push --super-prefix arg     | add new "ud" var              | keep both                 |
|-----------------------------+-------------------------------+---------------------------|
| remove "--recursive-prefix" | add "--checkout", "--rebase", | keep both                 |
|                             | "--merge"                     |                           |
|-----------------------------+-------------------------------+---------------------------|
| add SUPPORT_SUPER_PREFIX    | remove SUPPORT_SUPER_PREFIX   | keep ab/submodule-cleanup |
| to "git submodule--helper   | from "git submodule--helper"  |                           |
| update"                     |                               |                           |

(For the org-mode users: org-table-align fixes everything except the
last row, presumably because of the rogue line breaks.)