Hi Ævar,

On Mon, 6 Sep 2021, Ævar Arnfjörð Bjarmason wrote:

>
> On Fri, Sep 03 2021, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > This implements Scalar's opinionated `clone` command: it tries to use a
> > partial clone and sets up a sparse checkout by default. In contrast to
> > `git clone`, `scalar clone` sets up the worktree in the `src/`
> > subdirectory, to encourage a separation between the source files and the
> > build output (which helps Git tremendously because it avoids untracked
> > files that have to be specifically ignored when refreshing the index).
>
> Perhaps it's simpler to just say about that /src/ injection:
>
>     `scalar clone` adds an implicit "/src" subdirectory to whatever
>     directory the user provides, with the added stricture on top of
>     doing that with "git clone" that the "src" cannot exist already.

It would not only be simpler, it would also skip an important point I
tried to make, namely how this _differs_ from `git clone`. Rather crucial,
really.

> ...
>
> > +	if (is_directory(enlistment))
> > +		die(_("directory '%s' exists already"), enlistment);
> > +
> > +	dir = xstrfmt("%s/src", enlistment);
>
> Which also seems to suggest a bug here. I.e. if I "git clone <repo>
> /tmp/xyz/abc" and Ctrl+C it we'll remove "abc", but leave "xyz"
> behind. Since we're creating that "xyz" (or "src") implicitly here an
> abort/ctrl+C followed by a retry is going to run into this error, isn't
> it?

Sure, it's just like calling `git clone <url> a/b/c` and upon failure
seeing only the `c` directory removed, while `a/b` is left behind.

> I.e. it seems what's missing in this state machine is checking if the
> directory was there already, and if it isn't add it to the existing
> atexit() removals.
>
> Which may be tricky seeing as this is shelling out to "init" then
> "fetch" etc, i.e. who removes it? But maybe not.

I would rather spend time (after this patch series landed, of course) on
teaching `git clone` to handle what Scalar needs, and upon Ctrl+C to
optionally remove _all_ the directories it created, not just the innermost
one.

Then we get that functionality "for free", without spending a lot of time
on code that will be obsolete soon enough anyway.

Ciao,
Johannes