[0/2] Fix git subtree on Windows

Message

Kazuhiro Kato via GitGitGadget June 10, 2021, 9:13 a.m. UTC

In the topic branch ls/subtree, we saw a lot of improvements of the git
subtree command, and some cleaning up, too. For example, 22d550749361
(subtree: don't fuss with PATH, 2021-04-27) carefully laid out a history of
changes intended to work around issues where the git-subtree script was not
in the intended location, and removed a statement modifying PATH in favor of
a conditional warning (contingent on the PATH being in an unexpected shape).

This particular condition, however, was never tested on Windows, and it
broke git subtree in Git for Windows v2.32.0, as reported here
[https://github.com/git-for-windows/git/issues/3260]. Now, every invocation
of git subtree, with or without command-line arguments, results in output
like this:

It looks like either your git installation or your
git-subtree installation is broken.

Tips:
 - If `git --exec-path` does not print the correct path to
   your git install directory, then set the GIT_EXEC_PATH
   environment variable to the correct directory.
 - Make sure that your `git-core\git-subtree` file is either in your
   PATH or in your git exec path (`C:/Users/harry/Downloads/PortableGit/mingw64/libexec/git-core`).
 - You should run git-subtree as `git core\git-subtree`,
   not as `git-core\git-subtree`.


This patch series provides a band-aid to that symptom, and is based on
ls/subtree.

Johannes Schindelin (2):
  subtree: fix the GIT_EXEC_PATH sanity check to work on Windows
  subtree: fix assumption about the directory separator

 contrib/subtree/git-subtree.sh | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)


base-commit: 9a3e3ca2ba869f9fef9f8be390ed45457565ccd1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-978%2Fdscho%2Ffix-subtree-on-windows-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-978/dscho/fix-subtree-on-windows-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/978

Comments

Luke Shumaker June 11, 2021, 12:58 a.m. UTC | #1

On Thu, 10 Jun 2021 03:13:29 -0600,
Johannes Schindelin via GitGitGadget wrote:
> This particular condition, however, was never tested on Windows,

I was going to say that I do have CI set up[0] to run the subtree
tests on Windows, and was going to ask for assistance in figuring out
how to set up CI's Windows to match the behavior that you're seeing
with Cygwin.

However, upon closer inspection: Because of how run-test-slice.sh
works, it wasn't actually running the subtree test on Windows.  Oops.

Now, that CI setup was just on my personal copies and hadn't been
submitted upstream, since I wasn't sure if it was welcome[1].  I never
really got a reply to that part, but but now that we've got buy-in
from Ævar for running the tests in contrib/,[2] I might spend some
more time on it and try to get that submitted.  That is, if Felipe
doesn't beat me to it[3].

[0]: https://github.com/LukeShu/git/commit/359c71be69749621125181b9d8c109baba6bf745
[1]: https://lore.kernel.org/git/[e-mail]/
[2]: https://lore.kernel.org/git/[e-mail]/
[3]: https://lore.kernel.org/git/[e-mail]/#t

Johannes Schindelin June 11, 2021, 10:30 a.m. UTC | #2

Hi Luke,

On Thu, 10 Jun 2021, Luke Shumaker wrote:

> On Thu, 10 Jun 2021 03:13:29 -0600,
> Johannes Schindelin via GitGitGadget wrote:
> > This particular condition, however, was never tested on Windows,
>
> I was going to say that I do have CI set up[0] to run the subtree
> tests on Windows, and was going to ask for assistance in figuring out
> how to set up CI's Windows to match the behavior that you're seeing
> with Cygwin.
>
> However, upon closer inspection: Because of how run-test-slice.sh
> works, it wasn't actually running the subtree test on Windows.  Oops.

Right, I am sorry about that, it did bite me recently, too.

> Now, that CI setup was just on my personal copies and hadn't been
> submitted upstream, since I wasn't sure if it was welcome[1].  I never
> really got a reply to that part, but but now that we've got buy-in
> from Ævar for running the tests in contrib/,[2] I might spend some
> more time on it and try to get that submitted.

I am not so sure about running tests in contrib/ as part of the CI. Those
bits and pieces are in contrib/ to indicate that they should _not_ be
considered as adding to Junio's maintenance burden, and adding them to CI
would contradict that.

Having said that, in _Git for Windows_, we do bundle `git subtree`.
Therefore, I would be interested in running its tests as part of CI
because it is part of _my_ maintenance burden.

As to your 30-strong patch series: Sorry, I really cannot afford even
looking at such a large patch series. There are only 24h in a day, I need
to sleep from time to time, and there is a pandemic raging on, and the Git
mailing list's current atmosphere requires more self-care time from me
that I need to spend elsewhere.

But _if_ I had time to even look at it, I would suggest to first spend
time to turn it into a full-fledged built-in in Git. I.e. move it from
contrib/ to builtin/, rewriting it in C in the process.

Please take that suggestion with a large rock of salt, though: as I said,
I don't have time to assist in that endeavor, unfortunately not even as a
reviewer.

Ciao,
Dscho

>
> [0]: https://github.com/LukeShu/git/commit/359c71be69749621125181b9d8c109baba6bf745
> [1]: https://lore.kernel.org/git/[e-mail]/
> [2]: https://lore.kernel.org/git/[e-mail]/
>
> --
> Happy hacking,
> ~ Luke Shumaker
>

Luke Shumaker June 11, 2021, 1:46 p.m. UTC | #3

On Fri, 11 Jun 2021 04:30:21 -0600,
Johannes Schindelin wrote:
> But _if_ I had time to even look at it, I would suggest to first spend
> time to turn it into a full-fledged built-in in Git. I.e. move it from
> contrib/ to builtin/, rewriting it in C in the process.

That's definitely the direction I want to go, yes.

Felipe Contreras June 11, 2021, 3:50 p.m. UTC | #4

Johannes Schindelin wrote:
> On Thu, 10 Jun 2021, Luke Shumaker wrote:
> 
> > On Thu, 10 Jun 2021 03:13:29 -0600,
> > Johannes Schindelin via GitGitGadget wrote:
> > > This particular condition, however, was never tested on Windows,
> >
> > I was going to say that I do have CI set up[0] to run the subtree
> > tests on Windows, and was going to ask for assistance in figuring out
> > how to set up CI's Windows to match the behavior that you're seeing
> > with Cygwin.
> >
> > However, upon closer inspection: Because of how run-test-slice.sh
> > works, it wasn't actually running the subtree test on Windows.  Oops.
> 
> Right, I am sorry about that, it did bite me recently, too.
> 
> > Now, that CI setup was just on my personal copies and hadn't been
> > submitted upstream, since I wasn't sure if it was welcome[1].  I never
> > really got a reply to that part, but but now that we've got buy-in
> > from Ævar for running the tests in contrib/,[2] I might spend some
> > more time on it and try to get that submitted.
> 
> I am not so sure about running tests in contrib/ as part of the CI.

But we already do run contrib tests:

  t1021-rerere-in-workdir.sh
  t3000-ls-files-others.sh
  t9902-completion.sh
  t9903-bash-prompt.sh

> Those bits and pieces are in contrib/ to indicate that they should
> _not_ be considered as adding to Junio's maintenance burden, and
> adding them to CI would contradict that.

People rely on contrib and distributions build packages enabling what is
in contrib.

Part of a successful release is contrib not being broken, so Junio can't
just ignore the status of contrib.

I agree this is not goood, but that's the current situation we are in,
and that's because we have allowed perfectly widely used and stable
software on the same cohort as unmaintained waste nobody cares about.

Luke Shumaker June 12, 2021, 2:58 a.m. UTC | #5

On Fri, 11 Jun 2021 09:50:18 -0600,
Felipe Contreras wrote:
> Johannes Schindelin wrote:
> > On Thu, 10 Jun 2021, Luke Shumaker wrote:
> > > Now, that CI setup was just on my personal copies and hadn't been
> > > submitted upstream, since I wasn't sure if it was welcome[1].  I never
> > > really got a reply to that part, but but now that we've got buy-in
> > > from Ævar for running the tests in contrib/,[2] I might spend some
> > > more time on it and try to get that submitted.
> > 
> > I am not so sure about running tests in contrib/ as part of the CI.
> 
> But we already do run contrib tests:
> 
>   t1021-rerere-in-workdir.sh
>   t3000-ls-files-others.sh
>   t9902-completion.sh
>   t9903-bash-prompt.sh

Lovely, I didn't realize that it would be OK to move t7900-subtree.sh
out of contrib/ without moving all of subtree out of contrib/.
That'll make things easier, I think.