mbox series

[RFC,0/4] git: remove --super-prefix

Message ID 20221109004708.97668-1-chooglen@google.com (mailing list archive)
Headers show
Series git: remove --super-prefix | expand

Message

Glen Choo Nov. 9, 2022, 12:47 a.m. UTC 
*Note to maintainer*: See "Interactions with other series".

Given the work on ab/submodule-helper-prep-only, I'm sending this early
for feedback, and to see if I can avoid blocking Ævar's follow-up work.
This is pretty much done, but I haven't scrutinized the patches too
closely to see if all of the relevant paths are tested.

The main motivation behind this is the partial clone bug demonstrated by
the test in Patch 3/4, but instead of just fixing that one issue, I'd
prefer to get rid of this class of error once and for all.

= Description

When we introduced the internal-only "--super-prefix" in 74866d7579
(git: make super-prefix option, 2016-10-07), we specified that commands
must prefix paths by it, and pathspecs must be parsed relative to it.
That commit also includes safeguards to ensure that "--super-prefix" is
used correctly, namely:

- Only commands marked SUPPORT_SUPER_PREFIX can be invoked with
  "--super-prefix".
- The super prefix is propagated via the GIT_INTERNAL_SUPER_PREFIX env
  var, so a non-SUPPORT_SUPER_PREFIX command cannot be invoked by a
  SUPPORT_SUPER_PREFIX one.

However, its use is inconsistent, which is a strong reason to consider
using better-scoped flags instead. For example..

- Of the 4 commands that are SUPPORT_SUPER_PREFIX, only "read-tree" and
  "submodule--helper" do anything useful with it. "fsmonitor--daemon"
  has it to avoid the SUPPORT_SUPER_PREFIX warning [1].
  "checkout--worker" doesn't have a documented reason, but since it can
  be invoked by "read-tree", it's presumably also a workaround.
- "read-tree" and "submodule--helper" use different values for the super
  prefix; "read-tree" passes the path from the root of the
  superproject's tree to the submodule's gitlink, while
  "submodule--helper" passes the relative path of the original CWD to
  the submodule.
- "submodule--helper" doesn't use "--super-prefix" to parse pathspecs,
  only to display paths.

This series replaces the top-level "--super-prefix" with better-scoped,
per-command flags.

= Interactions with other series

As noted in the conversation starting at [2], the submodule--helper bits
in this series have substantial overlap with
ab/submodule-helper-prep-only and the work that series is prep for. I've
based this series off ab/submodule-helper-prep-only, since the
conversion of builtin/submodule--helper.c to use options parsing makes
patch 1 much easier to reason about.

Ævar: The patch of interest is 1/4, which removes the super prefix check
from submodule--helper. Hopefully you find this useful, I strongly
suspect that it will save time and churn for us to move this series
forward and to base your follow up work on this. But if this does end up
stalling, however, I'm happy to rebase this on top of your follow up
work.

= Patch summary

- Patch 1/4 introduces a "--toplevel-cwd-prefix" flag for "git
  submodule--helper" (replacing "--super-prefix" for "git
  submodule--helper").
- Patches 2-3/4 refactors "--submodule-prefix" for "git fetch" and
  reuses that implementation for "git read-tree" (replacing
  "--super-prefix" for "git read-tree").
- Patch 4 drops "--super-prefix" and a now-defunct fsmonitor test.

[1]: 53fcfbc84f (fsmonitor--daemon: allow --super-prefix argument, 2022-05-26)
[2]: https://lore.kernel.org/git/221104.86wn8bzeus.gmgdl@evledraar.gmail.com

Glen Choo (4):
  submodule--helper: teach --toplevel-cwd-prefix
  fetch: refactor --submodule-prefix
  read-tree: teach --submodule-prefix
  git: remove --super-prefix

 Documentation/fetch-options.txt    |  5 --
 Documentation/git.txt              |  7 +--
 builtin.h                          |  4 --
 builtin/fetch.c                    |  7 ++-
 builtin/read-tree.c                |  4 ++
 builtin/submodule--helper.c        | 52 ++++++++-----------
 cache.h                            |  2 -
 environment.c                      | 13 -----
 git.c                              | 40 +++-----------
 repository.c                       |  1 +
 repository.h                       |  9 +++-
 submodule.c                        | 83 ++++++++++++++++++------------
 submodule.h                        | 20 ++++++-
 t/t1001-read-tree-m-2way.sh        |  4 +-
 t/t5616-partial-clone.sh           | 43 ++++++++++++++++
 t/t7412-submodule-absorbgitdirs.sh | 13 ++++-
 t/t7527-builtin-fsmonitor.sh       | 50 ------------------
 unpack-trees.c                     | 32 ++++++------
 18 files changed, 187 insertions(+), 202 deletions(-)


base-commit: 69d94464e14de859ff56bcde7ebe0132201eceb9

Comments

Taylor Blau Nov. 9, 2022, 9:16 p.m. UTC | #1 
On Tue, Nov 08, 2022 at 04:47:04PM -0800, Glen Choo wrote:
> *Note to maintainer*: See "Interactions with other series".

Thanks for the heads up ;-).

It looks like this series is broken, at least in my application of it.
On the first patch, running t7401, for example, I get:

    BUG: builtin/submodule--helper.c:121: cannot have prefix 'sub/' and toplevel_cwd_prefix ''
    Aborted

after running the first test (-x shows that it happens after running
'git submodule summary', unsurprisingly).

I pushed out the result of what I have to the 'gc/remove--super-prefix'
branch of git@github.com:ttaylorr/git.git. As you'll see, the base is
'master' (as of 319605f8f0 (The eleventh batch, 2022-11-08)) with a
--no-ff merge of 'ab/submodule-helper-prep-only'.

Let me know if I'm holding it wrong.

Thanks,
Taylor
Glen Choo Nov. 9, 2022, 11:55 p.m. UTC | #2 
Taylor Blau <me@ttaylorr.com> writes:

> On Tue, Nov 08, 2022 at 04:47:04PM -0800, Glen Choo wrote:
>> *Note to maintainer*: See "Interactions with other series".
>
> Thanks for the heads up ;-).
>
> It looks like this series is broken, at least in my application of it.
> On the first patch, running t7401, for example, I get:
>
>     BUG: builtin/submodule--helper.c:121: cannot have prefix 'sub/' and toplevel_cwd_prefix ''
>     Aborted
>
> after running the first test (-x shows that it happens after running
> 'git submodule summary', unsurprisingly).
>
> I pushed out the result of what I have to the 'gc/remove--super-prefix'
> branch of git@github.com:ttaylorr/git.git. As you'll see, the base is
> 'master' (as of 319605f8f0 (The eleventh batch, 2022-11-08)) with a
> --no-ff merge of 'ab/submodule-helper-prep-only'.
>
> Let me know if I'm holding it wrong.

No, the problem seems to be entirely on my end. (I could've sworn I sent
this to CI..)

Given that we have another competing RFC, it doesn't seem like a great
use of time to reroll this just to unbreak "seen", but let me know if
I'm missing somthing.

>
> Thanks,
> Taylor
Taylor Blau Nov. 10, 2022, 2:14 a.m. UTC | #3 
On Wed, Nov 09, 2022 at 03:55:39PM -0800, Glen Choo wrote:
> > It looks like this series is broken, at least in my application of it.
> > On the first patch, running t7401, for example, I get:
> >
> >     BUG: builtin/submodule--helper.c:121: cannot have prefix 'sub/' and toplevel_cwd_prefix ''
> >     Aborted
> >
> > after running the first test (-x shows that it happens after running
> > 'git submodule summary', unsurprisingly).
> >
> > I pushed out the result of what I have to the 'gc/remove--super-prefix'
> > branch of git@github.com:ttaylorr/git.git. As you'll see, the base is
> > 'master' (as of 319605f8f0 (The eleventh batch, 2022-11-08)) with a
> > --no-ff merge of 'ab/submodule-helper-prep-only'.
> >
> > Let me know if I'm holding it wrong.
>
> No, the problem seems to be entirely on my end. (I could've sworn I sent
> this to CI..)
>
> Given that we have another competing RFC, it doesn't seem like a great
> use of time to reroll this just to unbreak "seen", but let me know if
> I'm missing somthing.

No problem. Let's drop this one for now, unless you have strong
objections.

Thanks,
Taylor
Glen Choo Nov. 10, 2022, 11:49 p.m. UTC | #4 
Taylor Blau <me@ttaylorr.com> writes:

> On Wed, Nov 09, 2022 at 03:55:39PM -0800, Glen Choo wrote:
>> > It looks like this series is broken, at least in my application of it.
>> > On the first patch, running t7401, for example, I get:
>> >
>> >     BUG: builtin/submodule--helper.c:121: cannot have prefix 'sub/' and toplevel_cwd_prefix ''
>> >     Aborted
>> >
>> > after running the first test (-x shows that it happens after running
>> > 'git submodule summary', unsurprisingly).
>> >
>> > I pushed out the result of what I have to the 'gc/remove--super-prefix'
>> > branch of git@github.com:ttaylorr/git.git. As you'll see, the base is
>> > 'master' (as of 319605f8f0 (The eleventh batch, 2022-11-08)) with a
>> > --no-ff merge of 'ab/submodule-helper-prep-only'.
>> >
>> > Let me know if I'm holding it wrong.
>>
>> No, the problem seems to be entirely on my end. (I could've sworn I sent
>> this to CI..)
>>
>> Given that we have another competing RFC, it doesn't seem like a great
>> use of time to reroll this just to unbreak "seen", but let me know if
>> I'm missing somthing.
>
> No problem. Let's drop this one for now, unless you have strong
> objections.

Yes let's drop my RFC for now, thanks.

>
> Thanks,
> Taylor