Re: [PATCH v2 0/2] extra: new concept of extra components

From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: Jul 12, 2021, 17:43 UTC
Message-ID: <c530dedb-8cad-2a73-5b56-a32173046382@gmail.com>
In-Reply-To: <20210710234629.17197-1-felipe.contreras@gmail.com>

Hi Felipe,

Le 2021-07-10 à 19:46, Felipe Contreras a écrit :

Show 15 quoted lines

> This patch series introduces the concept of extra components. These are
> components which are not yet part of the core but are good enough for
> distributions to ship, and in fact, they already do.
> 
> This benefits everyone:
> 
>   1. Distribution packagers that just want to do `make install`
>   2. People who download git's source code and just want to do
>      `make install`
>   3. Developers who have no idea what's production-level quality in
>      contrib/ and just want to do `make install`.
> 
> For now they'll have to do `make install install-extra`. But if the
> result is deemed correct, we might choose to add "install-extra" to the
> "install" target.

I agree with the end goal of this series. I myself carry a patch that adds an 'install-completion' target to the main Makefile that I apply before installing Git from 'master'.

Show 5 quoted lines

> 
> The measuring stick I'm using to gauge if a component in contrib belongs
> in extra is simple: are we already running tests for them with
> 'make test'? If the answer is "yes, we do run tests", then the answer is
> "yes, it belongs in contrib".

OK, this seems about right, it should cover prompt and completion, but...

Show 9 quoted lines

> 
> We might want to move more components from contrib to extra once their
> tests are being run reliably.
> 
> And we might move some components from the core which aren't really part
> of the core to extra, like gitk, git-gui, git-p4, and git-svn.
> 
> For now only contrib/completion and contrib/workdir are graduated to the
> new area.

... when I read this I went "what is this workdir thing, it must date from before 'git worktree' was added". And combing through the history, it does. The latest commit to the script is e32afab7b0 (git-new-workdir: don't fail if the target directory is empty, 2014-11-26), which describes as v2.3.0-rc0~60^2. And 'git worktree' was shipped in Git 2.5, 2015-07-27.

Looking at the tests, I see two uses of 'git-new-workdir': $ git grep -p 'new-workdir' t1021-rerere-in-workdir.sh=28=test_expect_success SYMLINKS 'rerere in workdir' ' t1021-rerere-in-workdir.sh:30:56: "$SHELL_PATH" "$TEST_DIRECTORY/../contrib/workdir/git-new-workdir" . work && t1021-rerere-in-workdir.sh:41:50:# For the purpose of helping contrib/workdir/git-new-workdir users, we do not t1021-rerere-in-workdir.sh=44=test_expect_failure SYMLINKS 'rerere in workdir (relative)' ' t1021-rerere-in-workdir.sh:46:56: "$SHELL_PATH" "$TEST_DIRECTORY/../contrib/workdir/git-new-workdir" . krow && t3000-ls-files-others.sh=75=test_expect_success SYMLINKS 'ls-files --others with symlinked submodule' ' t3000-ls-files-others.sh:87:57: "$SHELL_PATH" "$TEST_DIRECTORY/../contrib/workdir/git-new-workdir" ../sub sub &&

So they are not really testing this script per se, more like testing rerere and ls-files in a worktree created by 'git-new-workdir'. I do not think this enough justification to include 'git new-workdir' in 'extra/', since 'git worktree add' does the same thing and is a builtin command. Even if its "BUGS" section in the doc says it's "in general [...] still experimental", an experimental builtin is better than a 'contrib' script, no ?

Cheers,

Philippe.