Re: [PATCH v2 08/15] scalar: implement the `clone` subcommand

Hi Ævar,

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

On Fri, Sep 03 2021, Johannes Schindelin via GitGitGadget wrote:

quoted

From: Johannes Schindelin <redacted>

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.

...

quoted

+	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