{"thread":{"id":"60111","subject":"[PATCH 1/3] scalar: add --[no-]src option","startedAt":"2023-08-14T15:13:14Z","lastAt":"2023-08-28T16:23:04Z","messageCount":25,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee","Oswald Buddenhagen"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"480648","messageId":"c1c7e2f049e762b9b60614a5732e4d41db1d0da5.1692025937.git.gitgitgadget@gmail.com","threadId":"60111","inReplyTo":"pull.1569.git.1692025937.gitgitgadget@gmail.com","subject":"[PATCH 1/3] scalar: add --[no-]src option","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-14T15:12:15Z","receivedAt":"2023-08-14T15:13:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSome users have strong aversions to Scalar's opinion that the repository\nshould be in a 'src' directory, even though it creates a clean slate for\nplacing build outputs in adjacent directories.\n\nThe --no-src option allows users to opt-out of the default behavior.\n\nWhile adding options, make sure the usage output by 'scalar clone -h'\nreports the same as the SYNOPSIS line in Documentation/scalar.txt.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/scalar.txt | 8 +++++++-\n scalar.c | 11 +++++++++--\n t/t9211-scalar-clone.sh | 8 ++++++++\n 3 files changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/scalar.txt b/Documentation/scalar.txt\nindex f33436c7f65..cd65b3e230d 100644\n--- a/Documentation/scalar.txt\n+++ b/Documentation/scalar.txt\n@@ -8,7 +8,8 @@ scalar - A tool for managing large Git repositories\n SYNOPSIS\n --------\n [verse]\n-scalar clone [--single-branch] [--branch ] [--full-clone] []\n+scalar clone [--single-branch] [--branch ] [--full-clone]\n+\t[--[no-]src] []\n scalar list\n scalar register []\n scalar unregister []\n@@ -80,6 +81,11 @@ remote-tracking branch for the branch this option was used for the initial\n cloning. If the HEAD at the remote did not point at any branch when\n `--single-branch` clone was made, no remote-tracking branch is created.\n \n+--[no-]src::\n+\tSpecify if the repository should be created within a `src` directory\n+\twithin ``. This is the default behavior, so use\n+\t`--no-src` to opt-out of the creation of the `src` directory.\n+\n --[no-]full-clone::\n \tA sparse-checkout is initialized by default. This behavior can be\n \tturned off via `--full-clone`.\ndiff --git a/scalar.c b/scalar.c\nindex df7358f481c..938bb73f3ce 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -409,6 +409,7 @@ static int cmd_clone(int argc, const char **argv)\n {\n \tconst char *branch = NULL;\n \tint full_clone = 0, single_branch = 0, show_progress = isatty(2);\n+\tint src = 1;\n \tstruct option clone_options[] = {\n \t\tOPT_STRING('b', \"branch\", &branch, N_(\"\"),\n \t\t\t N_(\"branch to checkout after clone\")),\n@@ -417,10 +418,13 @@ static int cmd_clone(int argc, const char **argv)\n \t\tOPT_BOOL(0, \"single-branch\", &single_branch,\n \t\t\t N_(\"only download metadata for the branch that will \"\n \t\t\t \"be checked out\")),\n+\t\tOPT_BOOL(0, \"src\", &src,\n+\t\t\t N_(\"create repository within 'src' directory\")),\n \t\tOPT_END(),\n \t};\n \tconst char * const clone_usage[] = {\n-\t\tN_(\"scalar clone [] [--] []\"),\n+\t\tN_(\"scalar clone [--single-branch] [--branch ] [--full-clone]\\n\"\n+\t\t \"\\t[--[no-]src] []\"),\n \t\tNULL\n \t};\n \tconst char *url;\n@@ -456,7 +460,10 @@ static int cmd_clone(int argc, const char **argv)\n \tif (is_directory(enlistment))\n \t\tdie(_(\"directory '%s' exists already\"), enlistment);\n \n-\tdir = xstrfmt(\"%s/src\", enlistment);\n+\tif (src)\n+\t\tdir = xstrfmt(\"%s/src\", enlistment);\n+\telse\n+\t\tdir = xstrdup(enlistment);\n \n \tstrbuf_reset(&buf);\n \tif (branch)\ndiff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh\nindex 872ad1c9c2b..7ee73aba092 100755\n--- a/t/t9211-scalar-clone.sh\n+++ b/t/t9211-scalar-clone.sh\n@@ -180,4 +180,12 @@ test_expect_success 'scalar clone warns when background maintenance fails' '\n \tgrep \"could not turn on maintenance\" err\n '\n \n+test_expect_success '`scalar clone --no-src`' '\n+\tscalar clone --src \"file://$(pwd)/to-clone\" with-src &&\n+\tscalar clone --no-src \"file://$(pwd)/to-clone\" without-src &&\n+\n+\ttest_path_is_dir with-src/src &&\n+\ttest_path_is_missing without-src/src\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"480649","messageId":"pull.1569.git.1692025937.gitgitgadget@gmail.com","threadId":"60111","inReplyTo":null,"subject":"[PATCH 0/3] scalar: two downstream improvements","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-14T15:12:14Z","receivedAt":"2023-08-14T15:13:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"While updating git-for-windows/git and microsoft/git for the 2.42.0 release\nwindow, a few patches that we've been running in those forks for a while\ncame to light as something that would be beneficial to the core Git project.\nHere are some that are focused on the 'scalar' command.\n\n * Patch 1 adds a --no-src option to scalar clone to appease users who want\n to use scalar but object to the creation of the src directory.\n * Patches 2 and 3 help when scalar reconfigure -a fails. Patch 2 is a\n possibly helpful change on its own for other uses in the future.\n\nThanks, -Stolee\n\nDerrick Stolee (3):\n scalar: add --[no-]src option\n setup: add discover_git_directory_reason()\n scalar reconfigure: help users remove buggy repos\n\n Documentation/scalar.txt | 8 ++++-\n scalar.c | 67 ++++++++++++++++++++++++++++------------\n setup.c | 32 +++++++------------\n setup.h | 32 +++++++++++++++++--\n t/t9211-scalar-clone.sh | 8 +++++\n 5 files changed, 104 insertions(+), 43 deletions(-)\n\n\nbase-commit: a82fb66fed250e16d3010c75404503bea3f0ab61\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1569%2Fderrickstolee%2Fscalar-no-src-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1569/derrickstolee/scalar-no-src-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1569\n-- \ngitgitgadget\n"},{"id":"480650","messageId":"907410f76c4a5d7ef325545f52696a9a0c00b6b3.1692025937.git.gitgitgadget@gmail.com","threadId":"60111","inReplyTo":"pull.1569.git.1692025937.gitgitgadget@gmail.com","subject":"[PATCH 3/3] scalar reconfigure: help users remove buggy repos","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-14T15:12:17Z","receivedAt":"2023-08-14T15:13:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen running 'scalar reconfigure -a', Scalar has warning messages about\nthe repository missing (or not containing a .git directory). Failures\ncan also happen while trying to modify the repository-local config for\nthat repository.\n\nThese warnings may seem confusing to users who don't understand what\nthey mean or how to stop them.\n\nAdd a warning that instructs the user how to remove the warning in\nfuture installations.\n\nSigned-off-by: Derrick Stolee \n---\n scalar.c | 56 +++++++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 39 insertions(+), 17 deletions(-)\n\ndiff --git a/scalar.c b/scalar.c\nindex 938bb73f3ce..7d87d7ea724 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -664,6 +664,7 @@ static int cmd_reconfigure(int argc, const char **argv)\n \tgit_config(get_scalar_repos, &scalar_repos);\n \n \tfor (i = 0; i < scalar_repos.nr; i++) {\n+\t\tint failed = 0;\n \t\tconst char *dir = scalar_repos.items[i].string;\n \n \t\tstrbuf_reset(&commondir);\n@@ -674,30 +675,51 @@ static int cmd_reconfigure(int argc, const char **argv)\n \n \t\t\tif (errno != ENOENT) {\n \t\t\t\twarning_errno(_(\"could not switch to '%s'\"), dir);\n-\t\t\t\tres = -1;\n-\t\t\t\tcontinue;\n+\t\t\t\tfailed = -1;\n+\t\t\t\tgoto loop_end;\n \t\t\t}\n \n \t\t\tstrbuf_addstr(&buf, dir);\n \t\t\tif (remove_deleted_enlistment(&buf))\n-\t\t\t\tres = error(_(\"could not remove stale \"\n-\t\t\t\t\t \"scalar.repo '%s'\"), dir);\n+\t\t\t\tfailed = error(_(\"could not remove stale \"\n+\t\t\t\t\t\t \"scalar.repo '%s'\"), dir);\n \t\t\telse\n-\t\t\t\twarning(_(\"removing stale scalar.repo '%s'\"),\n+\t\t\t\twarning(_(\"removed stale scalar.repo '%s'\"),\n \t\t\t\t\tdir);\n \t\t\tstrbuf_release(&buf);\n-\t\t} else if (discover_git_directory(&commondir, &gitdir) < 0) {\n-\t\t\twarning_errno(_(\"git repository gone in '%s'\"), dir);\n-\t\t\tres = -1;\n-\t\t} else {\n-\t\t\tgit_config_clear();\n-\n-\t\t\tthe_repository = &r;\n-\t\t\tr.commondir = commondir.buf;\n-\t\t\tr.gitdir = gitdir.buf;\n-\n-\t\t\tif (set_recommended_config(1) < 0)\n-\t\t\t\tres = -1;\n+\t\t\tgoto loop_end;\n+\t\t}\n+\n+\t\tswitch (discover_git_directory_reason(&commondir, &gitdir)) {\n+\t\tcase GIT_DIR_INVALID_OWNERSHIP:\n+\t\t\twarning(_(\"repository at '%s' has different owner\"), dir);\n+\t\t\tfailed = -1;\n+\t\t\tgoto loop_end;\n+\n+\t\tcase GIT_DIR_DISCOVERED:\n+\t\t\tbreak;\n+\n+\t\tdefault:\n+\t\t\twarning(_(\"repository not found in '%s'\"), dir);\n+\t\t\tfailed = -1;\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tgit_config_clear();\n+\n+\t\tthe_repository = &r;\n+\t\tr.commondir = commondir.buf;\n+\t\tr.gitdir = gitdir.buf;\n+\n+\t\tif (set_recommended_config(1) < 0)\n+\t\t\tfailed = -1;\n+\n+loop_end:\n+\t\tif (failed) {\n+\t\t\tres = failed;\n+\t\t\twarning(_(\"to unregister this repository from Scalar, run\\n\"\n+\t\t\t\t \"\\tgit config --global --unset --fixed-value scalar.repo \\\"%s\\\"\"),\n+\t\t\t\tdir);\n \t\t}\n \t}\n \n-- \ngitgitgadget\n"},{"id":"480651","messageId":"fbba6252aeabc9a77d8529e4725feb3ea995545f.1692025937.git.gitgitgadget@gmail.com","threadId":"60111","inReplyTo":"pull.1569.git.1692025937.gitgitgadget@gmail.com","subject":"[PATCH 2/3] setup: add discover_git_directory_reason()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-14T15:12:16Z","receivedAt":"2023-08-14T15:13:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThere are many reasons why discovering a Git directory may fail. In\nparticular, 8959555cee7 (setup_git_directory(): add an owner check for\nthe top-level directory, 2022-03-02) added ownership checks as a\nsecurity precaution.\n\nCallers attempting to set up a Git directory may want to inform the user\nabout the reason for the failure. For that, expose the enum\ndiscovery_result from within setup.c and into cache.h where\ndiscover_git_directory() is defined.\n\nI initially wanted to change the return type of discover_git_directory()\nto be this enum, but several callers rely upon the \"zero means success\".\nThe two problems with this are:\n\n1. The zero value of the enum is actually GIT_DIR_NONE, so nonpositive\n results are errors.\n\n2. There are multiple successful states, so some positive results are\n successful.\n\nInstead of updating all callers immediately, add a new method,\ndiscover_git_directory_reason(), and convert discover_git_directory() to\nbe a thin shim on top of it.\n\nBecause there are extra checks that discover_git_directory_reason() does\nafter setup_git_directory_gently_1(), there are other modes that can be\nreturned for failure states. Add these modes to the enum, but be sure to\nexplicitly add them as BUG() states in the switch of\nsetup_git_directory_gently().\n\nSigned-off-by: Derrick Stolee \n---\n setup.c | 32 +++++++++++---------------------\n setup.h | 32 ++++++++++++++++++++++++++++++--\n 2 files changed, 41 insertions(+), 23 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 18927a847b8..bbf8f684d93 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1221,19 +1221,6 @@ static const char *allowed_bare_repo_to_string(\n \treturn NULL;\n }\n \n-enum discovery_result {\n-\tGIT_DIR_NONE = 0,\n-\tGIT_DIR_EXPLICIT,\n-\tGIT_DIR_DISCOVERED,\n-\tGIT_DIR_BARE,\n-\t/* these are errors */\n-\tGIT_DIR_HIT_CEILING = -1,\n-\tGIT_DIR_HIT_MOUNT_POINT = -2,\n-\tGIT_DIR_INVALID_GITFILE = -3,\n-\tGIT_DIR_INVALID_OWNERSHIP = -4,\n-\tGIT_DIR_DISALLOWED_BARE = -5,\n-};\n-\n /*\n * We cannot decide in this function whether we are in the work tree or\n * not, since the config can only be read _after_ this function was called.\n@@ -1385,21 +1372,22 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t}\n }\n \n-int discover_git_directory(struct strbuf *commondir,\n-\t\t\t struct strbuf *gitdir)\n+enum discovery_result discover_git_directory_reason(struct strbuf *commondir,\n+\t\t\t\t\t\t struct strbuf *gitdir)\n {\n \tstruct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;\n \tsize_t gitdir_offset = gitdir->len, cwd_len;\n \tsize_t commondir_offset = commondir->len;\n \tstruct repository_format candidate = REPOSITORY_FORMAT_INIT;\n+\tenum discovery_result result;\n \n \tif (strbuf_getcwd(&dir))\n-\t\treturn -1;\n+\t\treturn GIT_DIR_CWD_FAILURE;\n \n \tcwd_len = dir.len;\n-\tif (setup_git_directory_gently_1(&dir, gitdir, NULL, 0) <= 0) {\n+\tif ((result = setup_git_directory_gently_1(&dir, gitdir, NULL, 0)) <= 0) {\n \t\tstrbuf_release(&dir);\n-\t\treturn -1;\n+\t\treturn result;\n \t}\n \n \t/*\n@@ -1429,11 +1417,11 @@ int discover_git_directory(struct strbuf *commondir,\n \t\tstrbuf_setlen(commondir, commondir_offset);\n \t\tstrbuf_setlen(gitdir, gitdir_offset);\n \t\tclear_repository_format(&candidate);\n-\t\treturn -1;\n+\t\treturn GIT_DIR_INVALID_FORMAT;\n \t}\n \n \tclear_repository_format(&candidate);\n-\treturn 0;\n+\treturn result;\n }\n \n const char *setup_git_directory_gently(int *nongit_ok)\n@@ -1516,9 +1504,11 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t*nongit_ok = 1;\n \t\tbreak;\n \tcase GIT_DIR_NONE:\n+\tcase GIT_DIR_CWD_FAILURE:\n+\tcase GIT_DIR_INVALID_FORMAT:\n \t\t/*\n \t\t * As a safeguard against setup_git_directory_gently_1 returning\n-\t\t * this value, fallthrough to BUG. Otherwise it is possible to\n+\t\t * these values, fallthrough to BUG. Otherwise it is possible to\n \t\t * set startup_info->have_repository to 1 when we did nothing to\n \t\t * find a repository.\n \t\t */\ndiff --git a/setup.h b/setup.h\nindex 58fd2605dd2..b87d0d6fb2b 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -42,6 +42,30 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);\n #define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)\n \n void setup_work_tree(void);\n+\n+/*\n+ * discover_git_directory_reason() is similar to discover_git_directory(),\n+ * except it returns an enum value instead. It is important to note that\n+ * a zero-valued return here is actually GIT_DIR_NONE, which is different\n+ * from discover_git_directory.\n+ */\n+enum discovery_result {\n+\tGIT_DIR_NONE = 0,\n+\tGIT_DIR_EXPLICIT,\n+\tGIT_DIR_DISCOVERED,\n+\tGIT_DIR_BARE,\n+\t/* these are errors */\n+\tGIT_DIR_HIT_CEILING = -1,\n+\tGIT_DIR_HIT_MOUNT_POINT = -2,\n+\tGIT_DIR_INVALID_GITFILE = -3,\n+\tGIT_DIR_INVALID_OWNERSHIP = -4,\n+\tGIT_DIR_DISALLOWED_BARE = -5,\n+\tGIT_DIR_INVALID_FORMAT = -6,\n+\tGIT_DIR_CWD_FAILURE = -7,\n+};\n+enum discovery_result discover_git_directory_reason(struct strbuf *commondir,\n+\t\t\t\t\t\t struct strbuf *gitdir);\n+\n /*\n * Find the commondir and gitdir of the repository that contains the current\n * working directory, without changing the working directory or other global\n@@ -50,8 +74,12 @@ void setup_work_tree(void);\n * both have the same result appended to the buffer. The return value is\n * either 0 upon success and non-zero if no repository was found.\n */\n-int discover_git_directory(struct strbuf *commondir,\n-\t\t\t struct strbuf *gitdir);\n+static inline int discover_git_directory(struct strbuf *commondir,\n+\t\t\t\t\t struct strbuf *gitdir)\n+{\n+\treturn discover_git_directory_reason(commondir, gitdir) <= 0;\n+}\n+\n const char *setup_git_directory_gently(int *);\n const char *setup_git_directory(void);\n char *prefix_path(const char *prefix, int len, const char *path);\n-- \ngitgitgadget\n\n"},{"id":"480653","messageId":"xmqqcyzpmx2g.fsf@gitster.g","threadId":"60111","inReplyTo":"c1c7e2f049e762b9b60614a5732e4d41db1d0da5.1692025937.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] scalar: add --[no-]src option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-14T16:02:31Z","receivedAt":"2023-08-14T16:03:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> Some users have strong aversions to Scalar's opinion that the repository\n> should be in a 'src' directory, even though it creates a clean slate for\n> placing build outputs in adjacent directories.\n>\n> The --no-src option allows users to opt-out of the default behavior.\n>\n> While adding options, make sure the usage output by 'scalar clone -h'\n> reports the same as the SYNOPSIS line in Documentation/scalar.txt.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> Documentation/scalar.txt | 8 +++++++-\n> scalar.c | 11 +++++++++--\n> t/t9211-scalar-clone.sh | 8 ++++++++\n> 3 files changed, 24 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/scalar.txt b/Documentation/scalar.txt\n> index f33436c7f65..cd65b3e230d 100644\n> --- a/Documentation/scalar.txt\n> +++ b/Documentation/scalar.txt\n> @@ -8,7 +8,8 @@ scalar - A tool for managing large Git repositories\n> SYNOPSIS\n> --------\n> [verse]\n> -scalar clone [--single-branch] [--branch ] [--full-clone] []\n> +scalar clone [--single-branch] [--branch ] [--full-clone]\n> +\t[--[no-]src] []\n> scalar list\n> scalar register []\n> scalar unregister []\n> @@ -80,6 +81,11 @@ remote-tracking branch for the branch this option was used for the initial\n> cloning. If the HEAD at the remote did not point at any branch when\n> `--single-branch` clone was made, no remote-tracking branch is created.\n> \n> +--[no-]src::\n> +\tSpecify if the repository should be created within a `src` directory\n> +\twithin ``. This is the default behavior, so use\n> +\t`--no-src` to opt-out of the creation of the `src` directory.\n\nWhile there is nothing incorrect in the above per-se, and the first\nhalf of the description is perfectly good, but I find the latter\nhalf places too much stress on the existence of the \"src\" directory.\nAs a mere mortal end-user, what is more important is not the\npresence of an extra directory, but the fact that everything I have\nis now moved one level down in the directory hierarchy to \"src/\"\ndirectory.\n\n\tThis is the default behavior; use `--no-src` to place the\n\troot of the working tree of the repository directly at\n\t``.\n\nor something along that line would have been easier to understand\nfor me. It is not the creation of `src`, but that everything is\nmoved into it, is what some users may find unusual.\n\n> +test_expect_success '`scalar clone --no-src`' '\n> +\tscalar clone --src \"file://$(pwd)/to-clone\" with-src &&\n> +\tscalar clone --no-src \"file://$(pwd)/to-clone\" without-src &&\n> +\n> +\ttest_path_is_dir with-src/src &&\n> +\ttest_path_is_missing without-src/src\n> +'\n\nAnd another thing that may be interesting, from the above point of\nview, is to compare these two:\n\n\t(cd with-src/src && ls ?*) >with &&\n\t(cd without && ls ?*) >without &&\n\ttest_cmp with without\n\nBoth output should look something like\n\n cron.txt\n first.t\n second.t\n third.t\n\nand the earlier confusion point I raised was that\n\n\t(cd with-src && ls ?*)\n\nwould not look like\n\n cron.txt\n first.t\n second.t\n src/\n third.t\n\nThanks.\n"},{"id":"480655","messageId":"xmqqpm3plh8n.fsf@gitster.g","threadId":"60111","inReplyTo":"fbba6252aeabc9a77d8529e4725feb3ea995545f.1692025937.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] setup: add discover_git_directory_reason()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-14T16:29:44Z","receivedAt":"2023-08-14T16:30:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> There are many reasons why discovering a Git directory may fail. In\n> particular, 8959555cee7 (setup_git_directory(): add an owner check for\n> the top-level directory, 2022-03-02) added ownership checks as a\n> security precaution.\n>\n> Callers attempting to set up a Git directory may want to inform the user\n> about the reason for the failure. For that, expose the enum\n> discovery_result from within setup.c and into cache.h where\n> discover_git_directory() is defined.\n>\n> I initially wanted to change the return type of discover_git_directory()\n> to be this enum, but several callers rely upon the \"zero means success\".\n> The two problems with this are:\n>\n> 1. The zero value of the enum is actually GIT_DIR_NONE, so nonpositive\n> results are errors.\n\nTrue. discover_git_directory() already knows that negative return\nvalues from setup_git_directory_gently_1() signal errors while 0 or\npositive are OK.\n\n> 2. There are multiple successful states, so some positive results are\n> successful.\n\nMakes it sound as if some positive results are not successes, but is\nthat really the case?\n\n> Instead of updating all callers immediately, add a new method,\n> discover_git_directory_reason(), and convert discover_git_directory() to\n> be a thin shim on top of it.\n\nIt makes sense to insulate callers who only want to know if the\ndiscovery was successful or not (there only are two existing callers\nanyway) from the details. And turning a thin wrapper to the new API\nthat gives richer return codes is the way to go. Nicely designed.\n\n> Because there are extra checks that discover_git_directory_reason() does\n> after setup_git_directory_gently_1(), there are other modes that can be\n> returned for failure states. Add these modes to the enum, but be sure to\n> explicitly add them as BUG() states in the switch of\n> setup_git_directory_gently().\n\nGood.\n\n> -enum discovery_result {\n> -\tGIT_DIR_NONE = 0,\n> -\tGIT_DIR_EXPLICIT,\n> -\tGIT_DIR_DISCOVERED,\n> -\tGIT_DIR_BARE,\n> -\t/* these are errors */\n> -\tGIT_DIR_HIT_CEILING = -1,\n> -\tGIT_DIR_HIT_MOUNT_POINT = -2,\n> -\tGIT_DIR_INVALID_GITFILE = -3,\n> -\tGIT_DIR_INVALID_OWNERSHIP = -4,\n> -\tGIT_DIR_DISALLOWED_BARE = -5,\n> -};\n\nSo we promote this discovery_result, that was private implementation\ndetail inside the setup code, to a public interface. Is GIT_DIR_\nprefix still appropriate, or would it make more sense to have a\ncommon substring derived from the word DISCOVERY in them?\n\n> @@ -1385,21 +1372,22 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n> \t}\n> }\n> \n> -int discover_git_directory(struct strbuf *commondir,\n> -\t\t\t struct strbuf *gitdir)\n> +enum discovery_result discover_git_directory_reason(struct strbuf *commondir,\n> +\t\t\t\t\t\t struct strbuf *gitdir)\n> {\n> \tstruct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;\n> \tsize_t gitdir_offset = gitdir->len, cwd_len;\n> \tsize_t commondir_offset = commondir->len;\n> \tstruct repository_format candidate = REPOSITORY_FORMAT_INIT;\n> +\tenum discovery_result result;\n> \n> \tif (strbuf_getcwd(&dir))\n> -\t\treturn -1;\n> +\t\treturn GIT_DIR_CWD_FAILURE;\n\nMakes sense.\n\n> \tcwd_len = dir.len;\n> -\tif (setup_git_directory_gently_1(&dir, gitdir, NULL, 0) <= 0) {\n> +\tif ((result = setup_git_directory_gently_1(&dir, gitdir, NULL, 0)) <= 0) {\n\nCan we split this into two simple statements?\n\n\tresult = setup_git_directory_gently_1(...);\n\tif (result <= 0) {\n\n> \t\tstrbuf_release(&dir);\n> -\t\treturn -1;\n> +\t\treturn result;\n> \t}\n\nOK.\n\n> @@ -1429,11 +1417,11 @@ int discover_git_directory(struct strbuf *commondir,\n> \t\tstrbuf_setlen(commondir, commondir_offset);\n> \t\tstrbuf_setlen(gitdir, gitdir_offset);\n> \t\tclear_repository_format(&candidate);\n> -\t\treturn -1;\n> +\t\treturn GIT_DIR_INVALID_FORMAT;\n\nOK, so this thing is new. Earlier we thought we found a good\nGIT_DIR but it turns out the repository is something we cannot use.\nOver time we may acquire such a \"now we found it, is it really good?\"\nsanity checks, but for now, this is the only case that turns what\ngently_1() thought was good into a bad one. OK.\n\n> \t}\n> \n> \tclear_repository_format(&candidate);\n> -\treturn 0;\n> +\treturn result;\n\nAnd it makes perfect sense that everybody who passed such a \"post\ndiscovery check\" are OK and we return the result from gently_1().\n\n> @@ -1516,9 +1504,11 @@ const char *setup_git_directory_gently(int *nongit_ok)\n> \t\t*nongit_ok = 1;\n> \t\tbreak;\n> \tcase GIT_DIR_NONE:\n> +\tcase GIT_DIR_CWD_FAILURE:\n> +\tcase GIT_DIR_INVALID_FORMAT:\n> \t\t/*\n> \t\t * As a safeguard against setup_git_directory_gently_1 returning\n> -\t\t * this value, fallthrough to BUG. Otherwise it is possible to\n> +\t\t * these values, fallthrough to BUG. Otherwise it is possible to\n> \t\t * set startup_info->have_repository to 1 when we did nothing to\n> \t\t * find a repository.\n> \t\t */\n\nOK.\n\nNot a new problem, but does anybody explicitly or implicitly return\nDIR_NONE? I didn't find any codepath that does so. Presumably it\nmay have been arranged in the hope that a code structured like so:\n\n\tenum discovery_result res = GIT_DIR_NONE;\n\n\tif (some complex condition)\n\t\tres = ...;\n\telse if (another complex condition)\n\t\tres = ...;\n\n\t... sometime later ...\n\tif (res <= 0)\n\t\twe found a bad one\n\nwould ensure that \"res\" untouched by setup_git_directory_gently_1()\nis still an error, but I am not sure if it is effective, given that\nnobody uses GIT_DIR_NONE to assign or initialize anything. And the\nsame effect can be had by leaving 'res' uninitialized---the compilers\nare our friend.\n\nNot a part of this review, but I wonder if it makes sense for us to\nget rid of DIR_NONE.\n\n> -int discover_git_directory(struct strbuf *commondir,\n> -\t\t\t struct strbuf *gitdir);\n> +static inline int discover_git_directory(struct strbuf *commondir,\n> +\t\t\t\t\t struct strbuf *gitdir)\n> +{\n> +\treturn discover_git_directory_reason(commondir, gitdir) <= 0;\n> +}\n\nThe _reason() thing is more or less like setup_git_directory_gently_1()\nin that positives are success and everything else is an error. And\nthe point of keeping this thin wrapper as discover_git_directory() is\nto insulate the existing callers that discover_git_directory() returns\n-1 for failure, while returning 0 for success.\n\nSo this wrapper smells very wrong. It now flips the polarity of the\nerror return into a positive 1, no? That does not achieve the goal\nto insulate the callers from the change in implementation.\n\nOther than that, as you wrote in the cover letter, I think it is an\nexcellent move to have an interface to expose details of what\ndiscovery has found.\n\nThanks.\n"},{"id":"480656","messageId":"xmqqcyzplgk1.fsf@gitster.g","threadId":"60111","inReplyTo":"907410f76c4a5d7ef325545f52696a9a0c00b6b3.1692025937.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] scalar reconfigure: help users remove buggy repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-14T16:44:30Z","receivedAt":"2023-08-14T16:45:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> diff --git a/scalar.c b/scalar.c\n> index 938bb73f3ce..7d87d7ea724 100644\n> --- a/scalar.c\n> +++ b/scalar.c\n> @@ -664,6 +664,7 @@ static int cmd_reconfigure(int argc, const char **argv)\n> \tgit_config(get_scalar_repos, &scalar_repos);\n> \n> \tfor (i = 0; i < scalar_repos.nr; i++) {\n> +\t\tint failed = 0;\n> \t\tconst char *dir = scalar_repos.items[i].string;\n\nOK. You need a variable that lets you tell if the repository this\nround of the loop dealt with was good, and do not want to abort the\nloop, so you cannot reuse the \"res\" outside of the loop. Makes sort\nof sense.\n\nI wonder if it makes it simpler to initialize \"failed\" to true\nand clear it when you see it succeeded, though.\n\n> @@ -674,30 +675,51 @@ static int cmd_reconfigure(int argc, const char **argv)\n> \n> \t\t\tif (errno != ENOENT) {\n> \t\t\t\twarning_errno(_(\"could not switch to '%s'\"), dir);\n> +\t\t\t\tfailed = -1;\n> +\t\t\t\tgoto loop_end;\n\nSuch a change lets you drop this assignment ...\n\n> \t\t\t}\n> \n> \t\t\tstrbuf_addstr(&buf, dir);\n> \t\t\tif (remove_deleted_enlistment(&buf))\n> +\t\t\t\tfailed = error(_(\"could not remove stale \"\n> +\t\t\t\t\t\t \"scalar.repo '%s'\"), dir);\n\n... and this one, but ...\n\n> \t\t\telse\n> -\t\t\t\twarning(_(\"removing stale scalar.repo '%s'\"),\n> +\t\t\t\twarning(_(\"removed stale scalar.repo '%s'\"),\n> \t\t\t\t\tdir);\n\n... you'd need to drop, i.e. \"failed = 0\", here while you warn. It\nis a nice touch to update the message, by the way.\n\n> \t\t\tstrbuf_release(&buf);\n> +\t\t\tgoto loop_end;\n> +\t\t}\n> +\n> +\t\tswitch (discover_git_directory_reason(&commondir, &gitdir)) {\n> +\t\tcase GIT_DIR_INVALID_OWNERSHIP:\n> +\t\t\twarning(_(\"repository at '%s' has different owner\"), dir);\n> +\t\t\tfailed = -1;\n> +\t\t\tgoto loop_end;\n> +\n> +\t\tcase GIT_DIR_DISCOVERED:\n> +\t\t\tbreak;\n\n... and you'd need to drop i.e. \"failed = 0\", here, too. but\nother assignments in the switch can go.\n\n> +\t\tdefault:\n> +\t\t\twarning(_(\"repository not found in '%s'\"), dir);\n> +\t\t\tfailed = -1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tgit_config_clear();\n> +\n> +\t\tthe_repository = &r;\n> +\t\tr.commondir = commondir.buf;\n> +\t\tr.gitdir = gitdir.buf;\n> +\n> +\t\tif (set_recommended_config(1) < 0)\n> +\t\t\tfailed = -1;\n\nAnd the polarity of the check and assignment here needs flipping.\n\n> +loop_end:\n> +\t\tif (failed) {\n> +\t\t\tres = failed;\n\nThis assignment is a bit misleading, as if the value in \"failed\"\nactually matters, when it does not. It is merely a \"did we not\nsucceed this round, 0 or non-zero?\" boolean. It would have been\neasier to see what was going on by saying\n\n\tif (failed) {\n\t\tres = -1;\n\nhere, I would think.\n\n> +\t\t\twarning(_(\"to unregister this repository from Scalar, run\\n\"\n> +\t\t\t\t \"\\tgit config --global --unset --fixed-value scalar.repo \\\"%s\\\"\"),\n> +\t\t\t\tdir);\n> \t\t}\n> \t}\n\nOther than that, this step nicely justifies why the previous step\n[PATCH 2/3] is a good thing to do.\n\nThanks.\n"},{"id":"480657","messageId":"xmqq8radlg1y.fsf@gitster.g","threadId":"60111","inReplyTo":"xmqqpm3plh8n.fsf@gitster.g","subject":"Re: [PATCH 2/3] setup: add discover_git_directory_reason()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-14T16:55:21Z","receivedAt":"2023-08-14T16:59:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> 1. The zero value of the enum is actually GIT_DIR_NONE, so nonpositive\n>> results are errors.\n>\n> True. discover_git_directory() already knows that negative return\n> values from setup_git_directory_gently_1() signal errors while 0 or\n> positive are OK.\n\nNOnononono. negative are not. 0 is not returned, so if we saw one,\nit would be an error. And positives are OK.\n\nSorry for the confusion.\n"},{"id":"480659","messageId":"abae52d5-47d5-4439-9d2b-68bb6e1c96fa@github.com","threadId":"60111","inReplyTo":"xmqqcyzpmx2g.fsf@gitster.g","subject":"Re: [PATCH 1/3] scalar: add --[no-]src option","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-08-14T19:20:45Z","receivedAt":"2023-08-14T19:21:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/14/2023 12:02 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n\n>> +--[no-]src::\n>> +\tSpecify if the repository should be created within a `src` directory\n>> +\twithin ``. This is the default behavior, so use\n>> +\t`--no-src` to opt-out of the creation of the `src` directory.\n> \n> While there is nothing incorrect in the above per-se, and the first\n> half of the description is perfectly good, but I find the latter\n> half places too much stress on the existence of the \"src\" directory.\n> As a mere mortal end-user, what is more important is not the\n> presence of an extra directory, but the fact that everything I have\n> is now moved one level down in the directory hierarchy to \"src/\"\n> directory.\n> \n> \tThis is the default behavior; use `--no-src` to place the\n> \troot of the working tree of the repository directly at\n> \t``.\n> \n> or something along that line would have been easier to understand\n> for me. It is not the creation of `src`, but that everything is\n> moved into it, is what some users may find unusual.\n\nYour confusion makes sense. Focusing on the location of the cloned\nrepository is a good way to focus the option for the reader.\n \n>> +test_expect_success '`scalar clone --no-src`' '\n>> +\tscalar clone --src \"file://$(pwd)/to-clone\" with-src &&\n>> +\tscalar clone --no-src \"file://$(pwd)/to-clone\" without-src &&\n>> +\n>> +\ttest_path_is_dir with-src/src &&\n>> +\ttest_path_is_missing without-src/src\n>> +'\n> \n> And another thing that may be interesting, from the above point of\n> view, is to compare these two:\n> \n> \t(cd with-src/src && ls ?*) >with &&\n> \t(cd without && ls ?*) >without &&\n> \ttest_cmp with without\n\nGood idea.\n\nThanks,\n-Stolee\n"},{"id":"480932","messageId":"pull.1569.v2.git.1692725056.gitgitgadget@gmail.com","threadId":"60111","inReplyTo":"pull.1569.git.1692025937.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] scalar: two downstream improvements","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-22T17:24:12Z","receivedAt":"2023-08-22T17:24:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"While updating git-for-windows/git and microsoft/git for the 2.42.0 release\nwindow, a few patches that we've been running in those forks for a while\ncame to light as something that would be beneficial to the core Git project.\nHere are some that are focused on the 'scalar' command.\n\n * Patch 1 adds a --no-src option to scalar clone to appease users who want\n to use scalar but object to the creation of the src directory.\n * Patches 2 and 3 help when scalar reconfigure -a fails. Patch 2 is a\n possibly helpful change on its own for other uses in the future.\n\n\nUpdates in V2\n=============\n\nThanks, Junio, for the helpful review!\n\n * In Patch 1, the '--[no-]src' documentation is tightened and the tests\n check the contents of the repository worktree.\n * In Patch 2, the commit message is reworded to be more clear about\n positive values of the enum.\n * In Patch 2, the GIT_DIR_NONE option of the enum is never returned, so it\n does not need to exist. A case in scalar.c referenced it, so it is\n removed as part of the patch (though that case was removed later by patch\n 3 anyway).\n * In Patch 2, the discover_git_directory() wrapper is updated to return -1\n instead of 1, as it did before this patch.\n * In Patch 3, the 'failed' variable is renamed to 'succeeded' and the cases\n that update the value are swapped. The return code is set to -1 for any\n error instead of having a custom value based on the return from error()\n or error_errno().\n\nThanks, -Stolee\n\nDerrick Stolee (3):\n scalar: add --[no-]src option\n setup: add discover_git_directory_reason()\n scalar reconfigure: help users remove buggy repos\n\n Documentation/scalar.txt | 8 ++++-\n scalar.c | 65 +++++++++++++++++++++++++++++-----------\n setup.c | 34 ++++++++-------------\n setup.h | 35 ++++++++++++++++++++--\n t/t9211-scalar-clone.sh | 12 ++++++++\n 5 files changed, 110 insertions(+), 44 deletions(-)\n\n\nbase-commit: a82fb66fed250e16d3010c75404503bea3f0ab61\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1569%2Fderrickstolee%2Fscalar-no-src-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1569/derrickstolee/scalar-no-src-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1569\n\nRange-diff vs v1:\n\n 1: c1c7e2f049e ! 1: 0b6957beccb scalar: add --[no-]src option\n @@ Documentation/scalar.txt: remote-tracking branch for the branch this option was\n `--single-branch` clone was made, no remote-tracking branch is created.\n \n +--[no-]src::\n -+\tSpecify if the repository should be created within a `src` directory\n -+\twithin ``. This is the default behavior, so use\n -+\t`--no-src` to opt-out of the creation of the `src` directory.\n ++\tBy default, `scalar clone` places the cloned repository within a\n ++\t`/src` directory. Use `--no-src` to place the cloned\n ++\trepository directly in the `` directory.\n +\n --[no-]full-clone::\n \tA sparse-checkout is initialized by default. This behavior can be\n @@ t/t9211-scalar-clone.sh: test_expect_success 'scalar clone warns when background\n +\tscalar clone --no-src \"file://$(pwd)/to-clone\" without-src &&\n +\n +\ttest_path_is_dir with-src/src &&\n -+\ttest_path_is_missing without-src/src\n ++\ttest_path_is_missing without-src/src &&\n ++\n ++\t(cd with-src/src && ls ?*) >with &&\n ++\t(cd without-src && ls ?*) >without &&\n ++\ttest_cmp with without\n +'\n +\n test_done\n 2: fbba6252aea ! 2: 787af0f9744 setup: add discover_git_directory_reason()\n @@ Commit message\n 1. The zero value of the enum is actually GIT_DIR_NONE, so nonpositive\n results are errors.\n \n - 2. There are multiple successful states, so some positive results are\n + 2. There are multiple successful states; positive results are\n successful.\n \n + It is worth noting that GIT_DIR_NONE is not returned, so we remove this\n + option from the enum. We must be careful to keep the successful reasons\n + as positive values, so they are given explicit positive values.\n + Further, a use in scalar.c was previously impossible, so it is removed.\n +\n Instead of updating all callers immediately, add a new method,\n discover_git_directory_reason(), and convert discover_git_directory() to\n be a thin shim on top of it.\n \n + One thing that is important to note is that discover_git_directory()\n + previously returned -1 on error, so let's continue that into the future.\n + There is only one caller (in scalar.c) that depends on that signedness\n + instead of a non-zero check, so clean that up, too.\n +\n Because there are extra checks that discover_git_directory_reason() does\n after setup_git_directory_gently_1(), there are other modes that can be\n returned for failure states. Add these modes to the enum, but be sure to\n @@ Commit message\n \n Signed-off-by: Derrick Stolee \n \n + ## scalar.c ##\n +@@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)\n + \t\t\t\twarning(_(\"removing stale scalar.repo '%s'\"),\n + \t\t\t\t\tdir);\n + \t\t\tstrbuf_release(&buf);\n +-\t\t} else if (discover_git_directory(&commondir, &gitdir) < 0) {\n +-\t\t\twarning_errno(_(\"git repository gone in '%s'\"), dir);\n +-\t\t\tres = -1;\n + \t\t} else {\n + \t\t\tgit_config_clear();\n + \n +\n ## setup.c ##\n @@ setup.c: static const char *allowed_bare_repo_to_string(\n \treturn NULL;\n @@ setup.c: static enum discovery_result setup_git_directory_gently_1(struct strbuf\n \n \tcwd_len = dir.len;\n -\tif (setup_git_directory_gently_1(&dir, gitdir, NULL, 0) <= 0) {\n -+\tif ((result = setup_git_directory_gently_1(&dir, gitdir, NULL, 0)) <= 0) {\n ++\tresult = setup_git_directory_gently_1(&dir, gitdir, NULL, 0);\n ++\tif (result <= 0) {\n \t\tstrbuf_release(&dir);\n -\t\treturn -1;\n +\t\treturn result;\n @@ setup.c: int discover_git_directory(struct strbuf *commondir,\n \n const char *setup_git_directory_gently(int *nongit_ok)\n @@ setup.c: const char *setup_git_directory_gently(int *nongit_ok)\n + \t\t}\n \t\t*nongit_ok = 1;\n \t\tbreak;\n - \tcase GIT_DIR_NONE:\n +-\tcase GIT_DIR_NONE:\n +\tcase GIT_DIR_CWD_FAILURE:\n +\tcase GIT_DIR_INVALID_FORMAT:\n \t\t/*\n @@ setup.h: const char *resolve_gitdir_gently(const char *suspect, int *return_erro\n + * from discover_git_directory.\n + */\n +enum discovery_result {\n -+\tGIT_DIR_NONE = 0,\n -+\tGIT_DIR_EXPLICIT,\n -+\tGIT_DIR_DISCOVERED,\n -+\tGIT_DIR_BARE,\n ++\tGIT_DIR_EXPLICIT = 1,\n ++\tGIT_DIR_DISCOVERED = 2,\n ++\tGIT_DIR_BARE = 3,\n +\t/* these are errors */\n +\tGIT_DIR_HIT_CEILING = -1,\n +\tGIT_DIR_HIT_MOUNT_POINT = -2,\n @@ setup.h: const char *resolve_gitdir_gently(const char *suspect, int *return_erro\n /*\n * Find the commondir and gitdir of the repository that contains the current\n * working directory, without changing the working directory or other global\n -@@ setup.h: void setup_work_tree(void);\n + * state. The result is appended to commondir and gitdir. If the discovered\n + * gitdir does not correspond to a worktree, then 'commondir' and 'gitdir' will\n * both have the same result appended to the buffer. The return value is\n - * either 0 upon success and non-zero if no repository was found.\n +- * either 0 upon success and non-zero if no repository was found.\n ++ * either 0 upon success and -1 if no repository was found.\n */\n -int discover_git_directory(struct strbuf *commondir,\n -\t\t\t struct strbuf *gitdir);\n +static inline int discover_git_directory(struct strbuf *commondir,\n +\t\t\t\t\t struct strbuf *gitdir)\n +{\n -+\treturn discover_git_directory_reason(commondir, gitdir) <= 0;\n ++\tif (discover_git_directory_reason(commondir, gitdir) <= 0)\n ++\t\treturn -1;\n ++\treturn 0;\n +}\n +\n const char *setup_git_directory_gently(int *);\n 3: 907410f76c4 ! 3: 7ac7311863d scalar reconfigure: help users remove buggy repos\n @@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)\n \tgit_config(get_scalar_repos, &scalar_repos);\n \n \tfor (i = 0; i < scalar_repos.nr; i++) {\n -+\t\tint failed = 0;\n ++\t\tint succeeded = 0;\n \t\tconst char *dir = scalar_repos.items[i].string;\n \n \t\tstrbuf_reset(&commondir);\n @@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)\n \t\t\t\twarning_errno(_(\"could not switch to '%s'\"), dir);\n -\t\t\t\tres = -1;\n -\t\t\t\tcontinue;\n -+\t\t\t\tfailed = -1;\n +\t\t\t\tgoto loop_end;\n \t\t\t}\n \n @@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)\n \t\t\tif (remove_deleted_enlistment(&buf))\n -\t\t\t\tres = error(_(\"could not remove stale \"\n -\t\t\t\t\t \"scalar.repo '%s'\"), dir);\n -+\t\t\t\tfailed = error(_(\"could not remove stale \"\n -+\t\t\t\t\t\t \"scalar.repo '%s'\"), dir);\n - \t\t\telse\n +-\t\t\telse\n -\t\t\t\twarning(_(\"removing stale scalar.repo '%s'\"),\n ++\t\t\t\terror(_(\"could not remove stale \"\n ++\t\t\t\t\t\"scalar.repo '%s'\"), dir);\n ++\t\t\telse {\n +\t\t\t\twarning(_(\"removed stale scalar.repo '%s'\"),\n \t\t\t\t\tdir);\n ++\t\t\t\tsucceeded = 1;\n ++\t\t\t}\n \t\t\tstrbuf_release(&buf);\n --\t\t} else if (discover_git_directory(&commondir, &gitdir) < 0) {\n --\t\t\twarning_errno(_(\"git repository gone in '%s'\"), dir);\n --\t\t\tres = -1;\n -\t\t} else {\n -\t\t\tgit_config_clear();\n --\n --\t\t\tthe_repository = &r;\n --\t\t\tr.commondir = commondir.buf;\n --\t\t\tr.gitdir = gitdir.buf;\n --\n --\t\t\tif (set_recommended_config(1) < 0)\n --\t\t\t\tres = -1;\n +\t\t\tgoto loop_end;\n +\t\t}\n +\n +\t\tswitch (discover_git_directory_reason(&commondir, &gitdir)) {\n +\t\tcase GIT_DIR_INVALID_OWNERSHIP:\n +\t\t\twarning(_(\"repository at '%s' has different owner\"), dir);\n -+\t\t\tfailed = -1;\n +\t\t\tgoto loop_end;\n +\n +\t\tcase GIT_DIR_DISCOVERED:\n ++\t\t\tsucceeded = 1;\n +\t\t\tbreak;\n +\n +\t\tdefault:\n +\t\t\twarning(_(\"repository not found in '%s'\"), dir);\n -+\t\t\tfailed = -1;\n +\t\t\tbreak;\n +\t\t}\n +\n @@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)\n +\t\tthe_repository = &r;\n +\t\tr.commondir = commondir.buf;\n +\t\tr.gitdir = gitdir.buf;\n -+\n -+\t\tif (set_recommended_config(1) < 0)\n -+\t\t\tfailed = -1;\n -+\n + \n +-\t\t\tthe_repository = &r;\n +-\t\t\tr.commondir = commondir.buf;\n +-\t\t\tr.gitdir = gitdir.buf;\n ++\t\tif (set_recommended_config(1) >= 0)\n ++\t\t\tsucceeded = 1;\n + \n +-\t\t\tif (set_recommended_config(1) < 0)\n +-\t\t\t\tres = -1;\n +loop_end:\n -+\t\tif (failed) {\n -+\t\t\tres = failed;\n ++\t\tif (!succeeded) {\n ++\t\t\tres = -1;\n +\t\t\twarning(_(\"to unregister this repository from Scalar, run\\n\"\n +\t\t\t\t \"\\tgit config --global --unset --fixed-value scalar.repo \\\"%s\\\"\"),\n +\t\t\t\tdir);\n\n-- \ngitgitgadget\n"},{"id":"480933","messageId":"0b6957beccbd1815c49c15c3525bfd6cbe077d34.1692725056.git.gitgitgadget@gmail.com","threadId":"60111","inReplyTo":"pull.1569.v2.git.1692725056.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] scalar: add --[no-]src option","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-22T17:24:13Z","receivedAt":"2023-08-22T17:24:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSome users have strong aversions to Scalar's opinion that the repository\nshould be in a 'src' directory, even though it creates a clean slate for\nplacing build outputs in adjacent directories.\n\nThe --no-src option allows users to opt-out of the default behavior.\n\nWhile adding options, make sure the usage output by 'scalar clone -h'\nreports the same as the SYNOPSIS line in Documentation/scalar.txt.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/scalar.txt | 8 +++++++-\n scalar.c | 11 +++++++++--\n t/t9211-scalar-clone.sh | 12 ++++++++++++\n 3 files changed, 28 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/scalar.txt b/Documentation/scalar.txt\nindex f33436c7f65..361f51a6473 100644\n--- a/Documentation/scalar.txt\n+++ b/Documentation/scalar.txt\n@@ -8,7 +8,8 @@ scalar - A tool for managing large Git repositories\n SYNOPSIS\n --------\n [verse]\n-scalar clone [--single-branch] [--branch ] [--full-clone] []\n+scalar clone [--single-branch] [--branch ] [--full-clone]\n+\t[--[no-]src] []\n scalar list\n scalar register []\n scalar unregister []\n@@ -80,6 +81,11 @@ remote-tracking branch for the branch this option was used for the initial\n cloning. If the HEAD at the remote did not point at any branch when\n `--single-branch` clone was made, no remote-tracking branch is created.\n \n+--[no-]src::\n+\tBy default, `scalar clone` places the cloned repository within a\n+\t`/src` directory. Use `--no-src` to place the cloned\n+\trepository directly in the `` directory.\n+\n --[no-]full-clone::\n \tA sparse-checkout is initialized by default. This behavior can be\n \tturned off via `--full-clone`.\ndiff --git a/scalar.c b/scalar.c\nindex df7358f481c..938bb73f3ce 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -409,6 +409,7 @@ static int cmd_clone(int argc, const char **argv)\n {\n \tconst char *branch = NULL;\n \tint full_clone = 0, single_branch = 0, show_progress = isatty(2);\n+\tint src = 1;\n \tstruct option clone_options[] = {\n \t\tOPT_STRING('b', \"branch\", &branch, N_(\"\"),\n \t\t\t N_(\"branch to checkout after clone\")),\n@@ -417,10 +418,13 @@ static int cmd_clone(int argc, const char **argv)\n \t\tOPT_BOOL(0, \"single-branch\", &single_branch,\n \t\t\t N_(\"only download metadata for the branch that will \"\n \t\t\t \"be checked out\")),\n+\t\tOPT_BOOL(0, \"src\", &src,\n+\t\t\t N_(\"create repository within 'src' directory\")),\n \t\tOPT_END(),\n \t};\n \tconst char * const clone_usage[] = {\n-\t\tN_(\"scalar clone [] [--] []\"),\n+\t\tN_(\"scalar clone [--single-branch] [--branch ] [--full-clone]\\n\"\n+\t\t \"\\t[--[no-]src] []\"),\n \t\tNULL\n \t};\n \tconst char *url;\n@@ -456,7 +460,10 @@ static int cmd_clone(int argc, const char **argv)\n \tif (is_directory(enlistment))\n \t\tdie(_(\"directory '%s' exists already\"), enlistment);\n \n-\tdir = xstrfmt(\"%s/src\", enlistment);\n+\tif (src)\n+\t\tdir = xstrfmt(\"%s/src\", enlistment);\n+\telse\n+\t\tdir = xstrdup(enlistment);\n \n \tstrbuf_reset(&buf);\n \tif (branch)\ndiff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh\nindex 872ad1c9c2b..7869f45ee64 100755\n--- a/t/t9211-scalar-clone.sh\n+++ b/t/t9211-scalar-clone.sh\n@@ -180,4 +180,16 @@ test_expect_success 'scalar clone warns when background maintenance fails' '\n \tgrep \"could not turn on maintenance\" err\n '\n \n+test_expect_success '`scalar clone --no-src`' '\n+\tscalar clone --src \"file://$(pwd)/to-clone\" with-src &&\n+\tscalar clone --no-src \"file://$(pwd)/to-clone\" without-src &&\n+\n+\ttest_path_is_dir with-src/src &&\n+\ttest_path_is_missing without-src/src &&\n+\n+\t(cd with-src/src && ls ?*) >with &&\n+\t(cd without-src && ls ?*) >without &&\n+\ttest_cmp with without\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"480934","messageId":"7ac7311863d2e05c3dc8e26cb821fe8a7c4b6804.1692725056.git.gitgitgadget@gmail.com","threadId":"60111","inReplyTo":"pull.1569.v2.git.1692725056.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] scalar reconfigure: help users remove buggy repos","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-22T17:24:15Z","receivedAt":"2023-08-22T17:24:26Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen running 'scalar reconfigure -a', Scalar has warning messages about\nthe repository missing (or not containing a .git directory). Failures\ncan also happen while trying to modify the repository-local config for\nthat repository.\n\nThese warnings may seem confusing to users who don't understand what\nthey mean or how to stop them.\n\nAdd a warning that instructs the user how to remove the warning in\nfuture installations.\n\nSigned-off-by: Derrick Stolee \n---\n scalar.c | 51 ++++++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 38 insertions(+), 13 deletions(-)\n\ndiff --git a/scalar.c b/scalar.c\nindex 02a38e845e1..54df0fdbb9f 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -664,6 +664,7 @@ static int cmd_reconfigure(int argc, const char **argv)\n \tgit_config(get_scalar_repos, &scalar_repos);\n \n \tfor (i = 0; i < scalar_repos.nr; i++) {\n+\t\tint succeeded = 0;\n \t\tconst char *dir = scalar_repos.items[i].string;\n \n \t\tstrbuf_reset(&commondir);\n@@ -674,27 +675,51 @@ static int cmd_reconfigure(int argc, const char **argv)\n \n \t\t\tif (errno != ENOENT) {\n \t\t\t\twarning_errno(_(\"could not switch to '%s'\"), dir);\n-\t\t\t\tres = -1;\n-\t\t\t\tcontinue;\n+\t\t\t\tgoto loop_end;\n \t\t\t}\n \n \t\t\tstrbuf_addstr(&buf, dir);\n \t\t\tif (remove_deleted_enlistment(&buf))\n-\t\t\t\tres = error(_(\"could not remove stale \"\n-\t\t\t\t\t \"scalar.repo '%s'\"), dir);\n-\t\t\telse\n-\t\t\t\twarning(_(\"removing stale scalar.repo '%s'\"),\n+\t\t\t\terror(_(\"could not remove stale \"\n+\t\t\t\t\t\"scalar.repo '%s'\"), dir);\n+\t\t\telse {\n+\t\t\t\twarning(_(\"removed stale scalar.repo '%s'\"),\n \t\t\t\t\tdir);\n+\t\t\t\tsucceeded = 1;\n+\t\t\t}\n \t\t\tstrbuf_release(&buf);\n-\t\t} else {\n-\t\t\tgit_config_clear();\n+\t\t\tgoto loop_end;\n+\t\t}\n+\n+\t\tswitch (discover_git_directory_reason(&commondir, &gitdir)) {\n+\t\tcase GIT_DIR_INVALID_OWNERSHIP:\n+\t\t\twarning(_(\"repository at '%s' has different owner\"), dir);\n+\t\t\tgoto loop_end;\n+\n+\t\tcase GIT_DIR_DISCOVERED:\n+\t\t\tsucceeded = 1;\n+\t\t\tbreak;\n+\n+\t\tdefault:\n+\t\t\twarning(_(\"repository not found in '%s'\"), dir);\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tgit_config_clear();\n+\n+\t\tthe_repository = &r;\n+\t\tr.commondir = commondir.buf;\n+\t\tr.gitdir = gitdir.buf;\n \n-\t\t\tthe_repository = &r;\n-\t\t\tr.commondir = commondir.buf;\n-\t\t\tr.gitdir = gitdir.buf;\n+\t\tif (set_recommended_config(1) >= 0)\n+\t\t\tsucceeded = 1;\n \n-\t\t\tif (set_recommended_config(1) < 0)\n-\t\t\t\tres = -1;\n+loop_end:\n+\t\tif (!succeeded) {\n+\t\t\tres = -1;\n+\t\t\twarning(_(\"to unregister this repository from Scalar, run\\n\"\n+\t\t\t\t \"\\tgit config --global --unset --fixed-value scalar.repo \\\"%s\\\"\"),\n+\t\t\t\tdir);\n \t\t}\n \t}\n \n-- \ngitgitgadget\n"},{"id":"480935","messageId":"787af0f9744e2f18c9ab680886650278a9d2f8d0.1692725056.git.gitgitgadget@gmail.com","threadId":"60111","inReplyTo":"pull.1569.v2.git.1692725056.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] setup: add discover_git_directory_reason()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-22T17:24:14Z","receivedAt":"2023-08-22T17:24:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThere are many reasons why discovering a Git directory may fail. In\nparticular, 8959555cee7 (setup_git_directory(): add an owner check for\nthe top-level directory, 2022-03-02) added ownership checks as a\nsecurity precaution.\n\nCallers attempting to set up a Git directory may want to inform the user\nabout the reason for the failure. For that, expose the enum\ndiscovery_result from within setup.c and into cache.h where\ndiscover_git_directory() is defined.\n\nI initially wanted to change the return type of discover_git_directory()\nto be this enum, but several callers rely upon the \"zero means success\".\nThe two problems with this are:\n\n1. The zero value of the enum is actually GIT_DIR_NONE, so nonpositive\n results are errors.\n\n2. There are multiple successful states; positive results are\n successful.\n\nIt is worth noting that GIT_DIR_NONE is not returned, so we remove this\noption from the enum. We must be careful to keep the successful reasons\nas positive values, so they are given explicit positive values.\nFurther, a use in scalar.c was previously impossible, so it is removed.\n\nInstead of updating all callers immediately, add a new method,\ndiscover_git_directory_reason(), and convert discover_git_directory() to\nbe a thin shim on top of it.\n\nOne thing that is important to note is that discover_git_directory()\npreviously returned -1 on error, so let's continue that into the future.\nThere is only one caller (in scalar.c) that depends on that signedness\ninstead of a non-zero check, so clean that up, too.\n\nBecause there are extra checks that discover_git_directory_reason() does\nafter setup_git_directory_gently_1(), there are other modes that can be\nreturned for failure states. Add these modes to the enum, but be sure to\nexplicitly add them as BUG() states in the switch of\nsetup_git_directory_gently().\n\nSigned-off-by: Derrick Stolee \n---\n scalar.c | 3 ---\n setup.c | 34 ++++++++++++----------------------\n setup.h | 35 ++++++++++++++++++++++++++++++++---\n 3 files changed, 44 insertions(+), 28 deletions(-)\n\ndiff --git a/scalar.c b/scalar.c\nindex 938bb73f3ce..02a38e845e1 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -686,9 +686,6 @@ static int cmd_reconfigure(int argc, const char **argv)\n \t\t\t\twarning(_(\"removing stale scalar.repo '%s'\"),\n \t\t\t\t\tdir);\n \t\t\tstrbuf_release(&buf);\n-\t\t} else if (discover_git_directory(&commondir, &gitdir) < 0) {\n-\t\t\twarning_errno(_(\"git repository gone in '%s'\"), dir);\n-\t\t\tres = -1;\n \t\t} else {\n \t\t\tgit_config_clear();\n \ndiff --git a/setup.c b/setup.c\nindex 18927a847b8..2e607632dbd 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1221,19 +1221,6 @@ static const char *allowed_bare_repo_to_string(\n \treturn NULL;\n }\n \n-enum discovery_result {\n-\tGIT_DIR_NONE = 0,\n-\tGIT_DIR_EXPLICIT,\n-\tGIT_DIR_DISCOVERED,\n-\tGIT_DIR_BARE,\n-\t/* these are errors */\n-\tGIT_DIR_HIT_CEILING = -1,\n-\tGIT_DIR_HIT_MOUNT_POINT = -2,\n-\tGIT_DIR_INVALID_GITFILE = -3,\n-\tGIT_DIR_INVALID_OWNERSHIP = -4,\n-\tGIT_DIR_DISALLOWED_BARE = -5,\n-};\n-\n /*\n * We cannot decide in this function whether we are in the work tree or\n * not, since the config can only be read _after_ this function was called.\n@@ -1385,21 +1372,23 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t}\n }\n \n-int discover_git_directory(struct strbuf *commondir,\n-\t\t\t struct strbuf *gitdir)\n+enum discovery_result discover_git_directory_reason(struct strbuf *commondir,\n+\t\t\t\t\t\t struct strbuf *gitdir)\n {\n \tstruct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;\n \tsize_t gitdir_offset = gitdir->len, cwd_len;\n \tsize_t commondir_offset = commondir->len;\n \tstruct repository_format candidate = REPOSITORY_FORMAT_INIT;\n+\tenum discovery_result result;\n \n \tif (strbuf_getcwd(&dir))\n-\t\treturn -1;\n+\t\treturn GIT_DIR_CWD_FAILURE;\n \n \tcwd_len = dir.len;\n-\tif (setup_git_directory_gently_1(&dir, gitdir, NULL, 0) <= 0) {\n+\tresult = setup_git_directory_gently_1(&dir, gitdir, NULL, 0);\n+\tif (result <= 0) {\n \t\tstrbuf_release(&dir);\n-\t\treturn -1;\n+\t\treturn result;\n \t}\n \n \t/*\n@@ -1429,11 +1418,11 @@ int discover_git_directory(struct strbuf *commondir,\n \t\tstrbuf_setlen(commondir, commondir_offset);\n \t\tstrbuf_setlen(gitdir, gitdir_offset);\n \t\tclear_repository_format(&candidate);\n-\t\treturn -1;\n+\t\treturn GIT_DIR_INVALID_FORMAT;\n \t}\n \n \tclear_repository_format(&candidate);\n-\treturn 0;\n+\treturn result;\n }\n \n const char *setup_git_directory_gently(int *nongit_ok)\n@@ -1515,10 +1504,11 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t}\n \t\t*nongit_ok = 1;\n \t\tbreak;\n-\tcase GIT_DIR_NONE:\n+\tcase GIT_DIR_CWD_FAILURE:\n+\tcase GIT_DIR_INVALID_FORMAT:\n \t\t/*\n \t\t * As a safeguard against setup_git_directory_gently_1 returning\n-\t\t * this value, fallthrough to BUG. Otherwise it is possible to\n+\t\t * these values, fallthrough to BUG. Otherwise it is possible to\n \t\t * set startup_info->have_repository to 1 when we did nothing to\n \t\t * find a repository.\n \t\t */\ndiff --git a/setup.h b/setup.h\nindex 58fd2605dd2..b48cf1c43b5 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -42,16 +42,45 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);\n #define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)\n \n void setup_work_tree(void);\n+\n+/*\n+ * discover_git_directory_reason() is similar to discover_git_directory(),\n+ * except it returns an enum value instead. It is important to note that\n+ * a zero-valued return here is actually GIT_DIR_NONE, which is different\n+ * from discover_git_directory.\n+ */\n+enum discovery_result {\n+\tGIT_DIR_EXPLICIT = 1,\n+\tGIT_DIR_DISCOVERED = 2,\n+\tGIT_DIR_BARE = 3,\n+\t/* these are errors */\n+\tGIT_DIR_HIT_CEILING = -1,\n+\tGIT_DIR_HIT_MOUNT_POINT = -2,\n+\tGIT_DIR_INVALID_GITFILE = -3,\n+\tGIT_DIR_INVALID_OWNERSHIP = -4,\n+\tGIT_DIR_DISALLOWED_BARE = -5,\n+\tGIT_DIR_INVALID_FORMAT = -6,\n+\tGIT_DIR_CWD_FAILURE = -7,\n+};\n+enum discovery_result discover_git_directory_reason(struct strbuf *commondir,\n+\t\t\t\t\t\t struct strbuf *gitdir);\n+\n /*\n * Find the commondir and gitdir of the repository that contains the current\n * working directory, without changing the working directory or other global\n * state. The result is appended to commondir and gitdir. If the discovered\n * gitdir does not correspond to a worktree, then 'commondir' and 'gitdir' will\n * both have the same result appended to the buffer. The return value is\n- * either 0 upon success and non-zero if no repository was found.\n+ * either 0 upon success and -1 if no repository was found.\n */\n-int discover_git_directory(struct strbuf *commondir,\n-\t\t\t struct strbuf *gitdir);\n+static inline int discover_git_directory(struct strbuf *commondir,\n+\t\t\t\t\t struct strbuf *gitdir)\n+{\n+\tif (discover_git_directory_reason(commondir, gitdir) <= 0)\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n const char *setup_git_directory_gently(int *);\n const char *setup_git_directory(void);\n char *prefix_path(const char *prefix, int len, const char *path);\n-- \ngitgitgadget\n\n"},{"id":"480936","messageId":"xmqqttsqlvsd.fsf@gitster.g","threadId":"60111","inReplyTo":"787af0f9744e2f18c9ab680886650278a9d2f8d0.1692725056.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] setup: add discover_git_directory_reason()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-22T19:30:26Z","receivedAt":"2023-08-22T19:30:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> There are many reasons why discovering a Git directory may fail. In\n> particular, 8959555cee7 (setup_git_directory(): add an owner check for\n> the top-level directory, 2022-03-02) added ownership checks as a\n> security precaution.\n>\n> Callers attempting to set up a Git directory may want to inform the user\n> about the reason for the failure. For that, expose the enum\n> discovery_result from within setup.c and into cache.h where\n> discover_git_directory() is defined.\n>\n> I initially wanted to change the return type of discover_git_directory()\n> to be this enum, but several callers rely upon the \"zero means success\".\n> The two problems with this are:\n>\n> 1. The zero value of the enum is actually GIT_DIR_NONE, so nonpositive\n> results are errors.\n>\n> 2. There are multiple successful states; positive results are\n> successful.\n>\n> It is worth noting that GIT_DIR_NONE is not returned, so we remove this\n> option from the enum. We must be careful to keep the successful reasons\n> as positive values, so they are given explicit positive values.\n> Further, a use in scalar.c was previously impossible, so it is removed.\n>\n> Instead of updating all callers immediately, add a new method,\n> discover_git_directory_reason(), and convert discover_git_directory() to\n> be a thin shim on top of it.\n>\n> One thing that is important to note is that discover_git_directory()\n> previously returned -1 on error, so let's continue that into the future.\n> There is only one caller (in scalar.c) that depends on that signedness\n> instead of a non-zero check, so clean that up, too.\n>\n> Because there are extra checks that discover_git_directory_reason() does\n> after setup_git_directory_gently_1(), there are other modes that can be\n> returned for failure states. Add these modes to the enum, but be sure to\n> explicitly add them as BUG() states in the switch of\n> setup_git_directory_gently().\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> scalar.c | 3 ---\n> setup.c | 34 ++++++++++++----------------------\n> setup.h | 35 ++++++++++++++++++++++++++++++++---\n> 3 files changed, 44 insertions(+), 28 deletions(-)\n>\n> diff --git a/scalar.c b/scalar.c\n> index 938bb73f3ce..02a38e845e1 100644\n> --- a/scalar.c\n> +++ b/scalar.c\n> @@ -686,9 +686,6 @@ static int cmd_reconfigure(int argc, const char **argv)\n> \t\t\t\twarning(_(\"removing stale scalar.repo '%s'\"),\n> \t\t\t\t\tdir);\n> \t\t\tstrbuf_release(&buf);\n> -\t\t} else if (discover_git_directory(&commondir, &gitdir) < 0) {\n> -\t\t\twarning_errno(_(\"git repository gone in '%s'\"), dir);\n> -\t\t\tres = -1;\n> \t\t} else {\n> \t\t\tgit_config_clear();\n> \n\nIn the original before this series, and also after applying [PATCH\n3/3], the reconfiguration is a three-step process:\n\n - what if we cannot go to that directory?\n - what if the directory is not a usable repository?\n - now we are in a usable repository, let's reconfigure.\n\nand this seems to lose the second step tentatively? It is\nresurrected in a more enhanced form in [PATCH 3/3] so it may not be\na huge deal, but it looks like it is not intended lossage.\n\n"},{"id":"480937","messageId":"1550d741-009f-4c27-90a4-f1b91701cf6c@github.com","threadId":"60111","inReplyTo":"xmqqttsqlvsd.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] setup: add discover_git_directory_reason()","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-08-22T19:39:08Z","receivedAt":"2023-08-22T19:39:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/22/2023 3:30 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> From: Derrick Stolee \n\n>> It is worth noting that GIT_DIR_NONE is not returned, so we remove this\n>> option from the enum. We must be careful to keep the successful reasons\n>> as positive values, so they are given explicit positive values.\n>> Further, a use in scalar.c was previously impossible, so it is removed.\n\n(Relevant bit from the message.)\n\n>> diff --git a/scalar.c b/scalar.c\n>> index 938bb73f3ce..02a38e845e1 100644\n>> --- a/scalar.c\n>> +++ b/scalar.c\n>> @@ -686,9 +686,6 @@ static int cmd_reconfigure(int argc, const char **argv)\n>> \t\t\t\twarning(_(\"removing stale scalar.repo '%s'\"),\n>> \t\t\t\t\tdir);\n>> \t\t\tstrbuf_release(&buf);\n>> -\t\t} else if (discover_git_directory(&commondir, &gitdir) < 0) {\n>> -\t\t\twarning_errno(_(\"git repository gone in '%s'\"), dir);\n>> -\t\t\tres = -1;\n>> \t\t} else {\n>> \t\t\tgit_config_clear();\n>> \n> \n> In the original before this series, and also after applying [PATCH\n> 3/3], the reconfiguration is a three-step process:\n> \n> - what if we cannot go to that directory?\n> - what if the directory is not a usable repository?\n> - now we are in a usable repository, let's reconfigure.\n> \n> and this seems to lose the second step tentatively? It is\n> resurrected in a more enhanced form in [PATCH 3/3] so it may not be\n> a huge deal, but it looks like it is not intended lossage.\n\nI know what happened here. At some point during my edits, this line was\nchanged to \"else if (!discover_git_directory(...))\" which became an\nunreachable case.\n\nSo, based on the intermediate patch that did not survive, it made sense,\nbut you are right that this hunk of this patch is behaving badly. Good\neye!\n\nThanks,\n-Stolee\n"},{"id":"480938","messageId":"xmqqmsyilv3e.fsf@gitster.g","threadId":"60111","inReplyTo":"7ac7311863d2e05c3dc8e26cb821fe8a7c4b6804.1692725056.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] scalar reconfigure: help users remove buggy repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-22T19:45:25Z","receivedAt":"2023-08-22T19:45:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> @@ -664,6 +664,7 @@ static int cmd_reconfigure(int argc, const char **argv)\n> \tgit_config(get_scalar_repos, &scalar_repos);\n> \n> \tfor (i = 0; i < scalar_repos.nr; i++) {\n> +\t\tint succeeded = 0;\n> \t\tconst char *dir = scalar_repos.items[i].string;\n> \n> \t\tstrbuf_reset(&commondir);\n> @@ -674,27 +675,51 @@ static int cmd_reconfigure(int argc, const char **argv)\n> \n> \t\t\tif (errno != ENOENT) {\n> \t\t\t\twarning_errno(_(\"could not switch to '%s'\"), dir);\n> -\t\t\t\tres = -1;\n> -\t\t\t\tcontinue;\n> +\t\t\t\tgoto loop_end;\n\nThis is after seeing chdir(dir) failed. If the user manually\nremoved the enlisted directory, ENOENT would be one of the most\nlikely errors. If the user dropped a file to the place after it was\nvacated, we may get ENOTDIR, which is also not so bad.\n\nIn any case, is it desirable to keep the enlistment still configured\nby jumping to loop_end in these \"other\" error conditions? If the\nreason why we cannot chdir() into it is because of some tentative\nglitch that may resolve by itself, retaining the enlistment data may\nhave value, because it can be reused without the user having to\nrecreate the enlistment when the \"tentatively unavailable\" directory\ncomes back online, I guess, but how realistic would such an error\nbe?\n\n> \t\t\t}\n> \n> \t\t\tstrbuf_addstr(&buf, dir);\n> \t\t\tif (remove_deleted_enlistment(&buf))\n> -\t\t\t\tres = error(_(\"could not remove stale \"\n> -\t\t\t\t\t \"scalar.repo '%s'\"), dir);\n> -\t\t\telse\n> -\t\t\t\twarning(_(\"removing stale scalar.repo '%s'\"),\n> +\t\t\t\terror(_(\"could not remove stale \"\n> +\t\t\t\t\t\"scalar.repo '%s'\"), dir);\n> +\t\t\telse {\n> +\t\t\t\twarning(_(\"removed stale scalar.repo '%s'\"),\n> \t\t\t\t\tdir);\n> +\t\t\t\tsucceeded = 1;\n> +\t\t\t}\n> \t\t\tstrbuf_release(&buf);\n> -\t\t} else {\n> -\t\t\tgit_config_clear();\n> +\t\t\tgoto loop_end;\n> +\t\t}\n\nSo the above is \"what if we fail to chdir()\", which looked sensible.\nThen comes the \"what if we don't have a usable repository there?\", which\nwas lost in [PATCH 2/3].\n\n> +\t\tswitch (discover_git_directory_reason(&commondir, &gitdir)) {\n> +\t\tcase GIT_DIR_INVALID_OWNERSHIP:\n> +\t\t\twarning(_(\"repository at '%s' has different owner\"), dir);\n> +\t\t\tgoto loop_end;\n> +\n> +\t\tcase GIT_DIR_DISCOVERED:\n> +\t\t\tsucceeded = 1;\n> +\t\t\tbreak;\n> +\n> +\t\tdefault:\n> +\t\t\twarning(_(\"repository not found in '%s'\"), dir);\n> +\t\t\tbreak;\n\nAmong the error cases, INVALID_OWNERSHIP is one of the possibilities\nthat merits specialized message to the end-user. I wonder if others\nalso deserve to be explained, though.\n\n - HIT_CEILING and HIT_MOUNT_POINT will happen when there is no\n usable repository between \"dir\" and the specified ceiling.\n\n - INVALID_GITFILE and INVALID_FORMAT are signs of some repository\n corruption.\n\n - DISALLOWED_BARE is unlikely to happen in the scalar context.\n\n> +\t\t}\n> +\n> +\t\tgit_config_clear();\n> +\n> +\t\tthe_repository = &r;\n> +\t\tr.commondir = commondir.buf;\n> +\t\tr.gitdir = gitdir.buf;\n> \n> -\t\t\tthe_repository = &r;\n> -\t\t\tr.commondir = commondir.buf;\n> -\t\t\tr.gitdir = gitdir.buf;\n> +\t\tif (set_recommended_config(1) >= 0)\n> +\t\t\tsucceeded = 1;\n> \n> -\t\t\tif (set_recommended_config(1) < 0)\n> -\t\t\t\tres = -1;\n> +loop_end:\n> +\t\tif (!succeeded) {\n> +\t\t\tres = -1;\n> +\t\t\twarning(_(\"to unregister this repository from Scalar, run\\n\"\n> +\t\t\t\t \"\\tgit config --global --unset --fixed-value scalar.repo \\\"%s\\\"\"),\n> +\t\t\t\tdir);\n\nAh, OK. So the strategy is to punt on accepting the responsibility\nfor removing an inaccessible directory; rather, we just report that\nwe had trouble chdir() and let the user decide. Which makes sense.\n\nThanks.\n\n> \t\t}\n> \t}\n"},{"id":"480950","messageId":"ZOXQg3V1EU/FXnwV@ugly","threadId":"60111","inReplyTo":"0b6957beccbd1815c49c15c3525bfd6cbe077d34.1692725056.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] scalar: add --[no-]src option","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-23T09:25:23Z","receivedAt":"2023-08-23T09:36:04Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"just nitpicking the commit message:\n\nOn Tue, Aug 22, 2023 at 05:24:13PM +0000, Derrick Stolee via GitGitGadget wrote:\n>From: Derrick Stolee \n>\n>Some users have strong aversions to Scalar's opinion that the repository\n>should be in a 'src' directory, even though\n\n>it\n>\ni'd use \"this\" here.\n\n>creates a clean slate for\n>placing build\n\n>outputs\n>\n\"artifacts\" maybe.\n\n>in adjacent directories.\n>\n\n>The\n>\ni'd insert \"new\" here.\n\n>--no-src option allows users to\n\n>opt-out\n>\npedantically, there should be no dash here, as it's a regular verbal \nphrase. the dash should be used when it's turned into a noun, \"to \nprovide an opt-out\". at least i think so ...\n\n>of the default behavior.\n>\n>While adding options, make sure the usage output by 'scalar clone -h'\n>reports the same as the SYNOPSIS line in Documentation/scalar.txt.\n>\n\nregards\n"},{"id":"480952","messageId":"ZOXYOWf/Vqfslmv4@ugly","threadId":"60111","inReplyTo":"787af0f9744e2f18c9ab680886650278a9d2f8d0.1692725056.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] setup: add discover_git_directory_reason()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-23T09:58:17Z","receivedAt":"2023-08-23T09:58:39Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Tue, Aug 22, 2023 at 05:24:14PM +0000, Derrick Stolee via GitGitGadget wrote:\n>From: Derrick Stolee \n>\n>There are many reasons why discovering a Git directory may fail. In\n>particular, 8959555cee7 (setup_git_directory(): add an owner check for\n>the top-level directory, 2022-03-02) added ownership checks as a\n>security precaution.\n>\n>Callers attempting to set up a Git directory may want to inform the user\n>about the reason for the failure. For that, expose the enum\n>discovery_result from within setup.c\n\n>and\n>\n\"by moving it\"\n\n>into cache.h where\n>discover_git_directory() is defined.\n>\n>I initially wanted to change the return type of discover_git_directory()\n>to be this enum, but several callers rely upon the \"zero means success\".\n>The two problems with this are:\n>\n>1. The zero value of the enum is actually GIT_DIR_NONE, so nonpositive\n> results are errors.\n>\n>2. There are multiple successful states; positive results are\n> successful.\n>\n>It is worth noting that GIT_DIR_NONE is not returned, so we remove this\n>option from the enum. We must be careful to keep the successful reasons\n>as positive values, so they are given explicit positive values.\n\n>Further, a use in scalar.c was previously impossible, so it is removed.\n>\ni have no clue wha this means. what is \"it\"?\n\n>Instead of updating all callers immediately, add a new method,\n>discover_git_directory_reason(), and convert discover_git_directory() to\n>be a thin shim on top of it.\n>\nis this really worth it, given that there are just two callers, and the \nadjustment is trivial?\nif you insist, note that the function name can be easily misread as \n\"discover_git_(directory_reason)\", which is unhelpful. i typically use \nan _impl suffix in such \"thin wrapper\" scenarios.\n\nregards\n\n"},{"id":"481030","messageId":"f751dc4d-b605-43ef-8b73-082e8acd5914@github.com","threadId":"60111","inReplyTo":"xmqqmsyilv3e.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] scalar reconfigure: help users remove buggy repos","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-08-25T17:21:35Z","receivedAt":"2023-08-25T17:22:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/22/2023 3:45 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n\n>> +\t\tswitch (discover_git_directory_reason(&commondir, &gitdir)) {\n>> +\t\tcase GIT_DIR_INVALID_OWNERSHIP:\n>> +\t\t\twarning(_(\"repository at '%s' has different owner\"), dir);\n>> +\t\t\tgoto loop_end;\n>> +\n>> +\t\tcase GIT_DIR_DISCOVERED:\n>> +\t\t\tsucceeded = 1;\n>> +\t\t\tbreak;\n>> +\n>> +\t\tdefault:\n>> +\t\t\twarning(_(\"repository not found in '%s'\"), dir);\n>> +\t\t\tbreak;\n> \n> Among the error cases, INVALID_OWNERSHIP is one of the possibilities\n> that merits specialized message to the end-user. I wonder if others\n> also deserve to be explained, though.\n\nThe specific choice of GIT_DIR_INVALID_OWNERSHIP is singled out\nbecause it's a new-ish reason and is the most confusing to users\nwhen things fail for this reason.\n \n> - HIT_CEILING and HIT_MOUNT_POINT will happen when there is no\n> usable repository between \"dir\" and the specified ceiling.\n\nThese are basically \"didn't find a Git repo\" but there are different\nreasons why Git stopped looking. I'm not sure there is something more\nvaluable to indicate here than the \"repository not found\" message\nthat already exists.\n\n> - INVALID_GITFILE and INVALID_FORMAT are signs of some repository\n> corruption.\n\nI can add a message for this kind of error, which seems helpful to\npoint out to a user.\n\n> - DISALLOWED_BARE is unlikely to happen in the scalar context.\n\nI agree.\n\nThanks,\n-Stolee\n"},{"id":"481032","messageId":"xmqqsf8780bm.fsf@gitster.g","threadId":"60111","inReplyTo":"f751dc4d-b605-43ef-8b73-082e8acd5914@github.com","subject":"Re: [PATCH v2 3/3] scalar reconfigure: help users remove buggy repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-25T18:05:17Z","receivedAt":"2023-08-25T18:06:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 8/22/2023 3:45 PM, Junio C Hamano wrote:\n>> \"Derrick Stolee via GitGitGadget\" writes:\n>\n>>> +\t\tswitch (discover_git_directory_reason(&commondir, &gitdir)) {\n>>> +\t\tcase GIT_DIR_INVALID_OWNERSHIP:\n>>> +\t\t\twarning(_(\"repository at '%s' has different owner\"), dir);\n>>> +\t\t\tgoto loop_end;\n>>> +\n>>> +\t\tcase GIT_DIR_DISCOVERED:\n>>> +\t\t\tsucceeded = 1;\n>>> +\t\t\tbreak;\n>>> +\n>>> +\t\tdefault:\n>>> +\t\t\twarning(_(\"repository not found in '%s'\"), dir);\n>>> +\t\t\tbreak;\n>> \n>> Among the error cases, INVALID_OWNERSHIP is one of the possibilities\n>> that merits specialized message to the end-user. I wonder if others\n>> also deserve to be explained, though.\n>\n> The specific choice of GIT_DIR_INVALID_OWNERSHIP is singled out\n> because it's a new-ish reason and is the most confusing to users\n> when things fail for this reason.\n> \n>> - HIT_CEILING and HIT_MOUNT_POINT will happen when there is no\n>> usable repository between \"dir\" and the specified ceiling.\n>\n> These are basically \"didn't find a Git repo\" but there are different\n> reasons why Git stopped looking. I'm not sure there is something more\n> valuable to indicate here than the \"repository not found\" message\n> that already exists.\n\nOK. I just know that \"not found\" will be greeted by \"stupid Git, if\nyou go one level up, there is a .git/ directory!\", now we have many\nusers than we used to have and people forget what they configured.\n\nBut I think it would apply much less to users who see \"repository\nnot found\" in the \"scalar reconfigure\" than ones who manually\ncreated a repository, and then forgot that they shuffled the disks\naround with cross mounting. So I agree with you that it is not\nessential to mention these reasons in this codepath (and\nsetup_git_directory() does have a reasonable message for at least\nthe mount-point case that covers the more general case).\n\n>> - INVALID_GITFILE and INVALID_FORMAT are signs of some repository\n>> corruption.\n>\n> I can add a message for this kind of error, which seems helpful to\n> point out to a user.\n\nMaybe. We can do that in a follow-up topic separately.\n\nThanks.\n"},{"id":"481045","messageId":"e9858b31db66101b04d93695390f48316bae8199.1693230746.git.gitgitgadget@gmail.com","threadId":"60111","inReplyTo":"pull.1569.v3.git.1693230746.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] scalar: add --[no-]src option","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-28T13:52:24Z","receivedAt":"2023-08-28T13:53:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSome users have strong aversions to Scalar's opinion that the repository\nshould be in a 'src' directory, even though this creates a clean slate\nfor placing build artifacts in adjacent directories.\n\nThe new --no-src option allows users to opt out of the default behavior.\n\nWhile adding options, make sure the usage output by 'scalar clone -h'\nreports the same as the SYNOPSIS line in Documentation/scalar.txt.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/scalar.txt | 8 +++++++-\n scalar.c | 11 +++++++++--\n t/t9211-scalar-clone.sh | 12 ++++++++++++\n 3 files changed, 28 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/scalar.txt b/Documentation/scalar.txt\nindex f33436c7f65..361f51a6473 100644\n--- a/Documentation/scalar.txt\n+++ b/Documentation/scalar.txt\n@@ -8,7 +8,8 @@ scalar - A tool for managing large Git repositories\n SYNOPSIS\n --------\n [verse]\n-scalar clone [--single-branch] [--branch ] [--full-clone] []\n+scalar clone [--single-branch] [--branch ] [--full-clone]\n+\t[--[no-]src] []\n scalar list\n scalar register []\n scalar unregister []\n@@ -80,6 +81,11 @@ remote-tracking branch for the branch this option was used for the initial\n cloning. If the HEAD at the remote did not point at any branch when\n `--single-branch` clone was made, no remote-tracking branch is created.\n \n+--[no-]src::\n+\tBy default, `scalar clone` places the cloned repository within a\n+\t`/src` directory. Use `--no-src` to place the cloned\n+\trepository directly in the `` directory.\n+\n --[no-]full-clone::\n \tA sparse-checkout is initialized by default. This behavior can be\n \tturned off via `--full-clone`.\ndiff --git a/scalar.c b/scalar.c\nindex df7358f481c..938bb73f3ce 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -409,6 +409,7 @@ static int cmd_clone(int argc, const char **argv)\n {\n \tconst char *branch = NULL;\n \tint full_clone = 0, single_branch = 0, show_progress = isatty(2);\n+\tint src = 1;\n \tstruct option clone_options[] = {\n \t\tOPT_STRING('b', \"branch\", &branch, N_(\"\"),\n \t\t\t N_(\"branch to checkout after clone\")),\n@@ -417,10 +418,13 @@ static int cmd_clone(int argc, const char **argv)\n \t\tOPT_BOOL(0, \"single-branch\", &single_branch,\n \t\t\t N_(\"only download metadata for the branch that will \"\n \t\t\t \"be checked out\")),\n+\t\tOPT_BOOL(0, \"src\", &src,\n+\t\t\t N_(\"create repository within 'src' directory\")),\n \t\tOPT_END(),\n \t};\n \tconst char * const clone_usage[] = {\n-\t\tN_(\"scalar clone [] [--] []\"),\n+\t\tN_(\"scalar clone [--single-branch] [--branch ] [--full-clone]\\n\"\n+\t\t \"\\t[--[no-]src] []\"),\n \t\tNULL\n \t};\n \tconst char *url;\n@@ -456,7 +460,10 @@ static int cmd_clone(int argc, const char **argv)\n \tif (is_directory(enlistment))\n \t\tdie(_(\"directory '%s' exists already\"), enlistment);\n \n-\tdir = xstrfmt(\"%s/src\", enlistment);\n+\tif (src)\n+\t\tdir = xstrfmt(\"%s/src\", enlistment);\n+\telse\n+\t\tdir = xstrdup(enlistment);\n \n \tstrbuf_reset(&buf);\n \tif (branch)\ndiff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh\nindex 872ad1c9c2b..7869f45ee64 100755\n--- a/t/t9211-scalar-clone.sh\n+++ b/t/t9211-scalar-clone.sh\n@@ -180,4 +180,16 @@ test_expect_success 'scalar clone warns when background maintenance fails' '\n \tgrep \"could not turn on maintenance\" err\n '\n \n+test_expect_success '`scalar clone --no-src`' '\n+\tscalar clone --src \"file://$(pwd)/to-clone\" with-src &&\n+\tscalar clone --no-src \"file://$(pwd)/to-clone\" without-src &&\n+\n+\ttest_path_is_dir with-src/src &&\n+\ttest_path_is_missing without-src/src &&\n+\n+\t(cd with-src/src && ls ?*) >with &&\n+\t(cd without-src && ls ?*) >without &&\n+\ttest_cmp with without\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"481046","messageId":"pull.1569.v3.git.1693230746.gitgitgadget@gmail.com","threadId":"60111","inReplyTo":"pull.1569.v2.git.1692725056.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] scalar: two downstream improvements","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-28T13:52:23Z","receivedAt":"2023-08-28T13:53:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"While updating git-for-windows/git and microsoft/git for the 2.42.0 release\nwindow, a few patches that we've been running in those forks for a while\ncame to light as something that would be beneficial to the core Git project.\nHere are some that are focused on the 'scalar' command.\n\n * Patch 1 adds a --no-src option to scalar clone to appease users who want\n to use scalar but object to the creation of the src directory.\n * Patches 2 and 3 help when scalar reconfigure -a fails. Patch 2 is a\n possibly helpful change on its own for other uses in the future.\n\n\nUpdates in V3\n=============\n\n * Several commit message edits.\n * An important case that was dropped in v2's patch 2 is reintroduced (even\n though it is modified in patch 3).\n * An error message is added for corrupt Git repositories.\n\n\nUpdates in V2\n=============\n\nThanks, Junio, for the helpful review!\n\n * In Patch 1, the '--[no-]src' documentation is tightened and the tests\n check the contents of the repository worktree.\n * In Patch 2, the commit message is reworded to be more clear about\n positive values of the enum.\n * In Patch 2, the GIT_DIR_NONE option of the enum is never returned, so it\n does not need to exist. A case in scalar.c referenced it, so it is\n removed as part of the patch (though that case was removed later by patch\n 3 anyway).\n * In Patch 2, the discover_git_directory() wrapper is updated to return -1\n instead of 1, as it did before this patch.\n * In Patch 3, the 'failed' variable is renamed to 'succeeded' and the cases\n that update the value are swapped. The return code is set to -1 for any\n error instead of having a custom value based on the return from error()\n or error_errno().\n\nThanks, -Stolee\n\nDerrick Stolee (3):\n scalar: add --[no-]src option\n setup: add discover_git_directory_reason()\n scalar reconfigure: help users remove buggy repos\n\n Documentation/scalar.txt | 8 ++++-\n scalar.c | 70 +++++++++++++++++++++++++++++-----------\n setup.c | 34 +++++++------------\n setup.h | 35 ++++++++++++++++++--\n t/t9211-scalar-clone.sh | 12 +++++++\n 5 files changed, 115 insertions(+), 44 deletions(-)\n\n\nbase-commit: a82fb66fed250e16d3010c75404503bea3f0ab61\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1569%2Fderrickstolee%2Fscalar-no-src-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1569/derrickstolee/scalar-no-src-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1569\n\nRange-diff vs v2:\n\n 1: 0b6957beccb ! 1: e9858b31db6 scalar: add --[no-]src option\n @@ Commit message\n scalar: add --[no-]src option\n \n Some users have strong aversions to Scalar's opinion that the repository\n - should be in a 'src' directory, even though it creates a clean slate for\n - placing build outputs in adjacent directories.\n + should be in a 'src' directory, even though this creates a clean slate\n + for placing build artifacts in adjacent directories.\n \n - The --no-src option allows users to opt-out of the default behavior.\n + The new --no-src option allows users to opt out of the default behavior.\n \n While adding options, make sure the usage output by 'scalar clone -h'\n reports the same as the SYNOPSIS line in Documentation/scalar.txt.\n 2: 787af0f9744 ! 2: 3c16fa6897f setup: add discover_git_directory_reason()\n @@ Commit message\n \n Callers attempting to set up a Git directory may want to inform the user\n about the reason for the failure. For that, expose the enum\n - discovery_result from within setup.c and into cache.h where\n + discovery_result from within setup.c and move it into cache.h where\n discover_git_directory() is defined.\n \n I initially wanted to change the return type of discover_git_directory()\n @@ Commit message\n It is worth noting that GIT_DIR_NONE is not returned, so we remove this\n option from the enum. We must be careful to keep the successful reasons\n as positive values, so they are given explicit positive values.\n - Further, a use in scalar.c was previously impossible, so it is removed.\n \n Instead of updating all callers immediately, add a new method,\n discover_git_directory_reason(), and convert discover_git_directory() to\n @@ Commit message\n \n Signed-off-by: Derrick Stolee \n \n - ## scalar.c ##\n -@@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)\n - \t\t\t\twarning(_(\"removing stale scalar.repo '%s'\"),\n - \t\t\t\t\tdir);\n - \t\t\tstrbuf_release(&buf);\n --\t\t} else if (discover_git_directory(&commondir, &gitdir) < 0) {\n --\t\t\twarning_errno(_(\"git repository gone in '%s'\"), dir);\n --\t\t\tres = -1;\n - \t\t} else {\n - \t\t\tgit_config_clear();\n - \n -\n ## setup.c ##\n @@ setup.c: static const char *allowed_bare_repo_to_string(\n \treturn NULL;\n 3: 7ac7311863d ! 3: ac234b15755 scalar reconfigure: help users remove buggy repos\n @@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)\n +\t\t\t\tsucceeded = 1;\n +\t\t\t}\n \t\t\tstrbuf_release(&buf);\n +-\t\t} else if (discover_git_directory(&commondir, &gitdir) < 0) {\n +-\t\t\twarning_errno(_(\"git repository gone in '%s'\"), dir);\n +-\t\t\tres = -1;\n -\t\t} else {\n -\t\t\tgit_config_clear();\n +\t\t\tgoto loop_end;\n @@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)\n +\t\t\twarning(_(\"repository at '%s' has different owner\"), dir);\n +\t\t\tgoto loop_end;\n +\n ++\t\tcase GIT_DIR_INVALID_GITFILE:\n ++\t\tcase GIT_DIR_INVALID_FORMAT:\n ++\t\t\twarning(_(\"repository at '%s' has a format issue\"), dir);\n ++\t\t\tgoto loop_end;\n ++\n +\t\tcase GIT_DIR_DISCOVERED:\n +\t\t\tsucceeded = 1;\n +\t\t\tbreak;\n @@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)\n +\t\t\twarning(_(\"repository not found in '%s'\"), dir);\n +\t\t\tbreak;\n +\t\t}\n -+\n -+\t\tgit_config_clear();\n -+\n -+\t\tthe_repository = &r;\n -+\t\tr.commondir = commondir.buf;\n -+\t\tr.gitdir = gitdir.buf;\n \n -\t\t\tthe_repository = &r;\n -\t\t\tr.commondir = commondir.buf;\n -\t\t\tr.gitdir = gitdir.buf;\n -+\t\tif (set_recommended_config(1) >= 0)\n -+\t\t\tsucceeded = 1;\n ++\t\tgit_config_clear();\n \n -\t\t\tif (set_recommended_config(1) < 0)\n -\t\t\t\tres = -1;\n ++\t\tthe_repository = &r;\n ++\t\tr.commondir = commondir.buf;\n ++\t\tr.gitdir = gitdir.buf;\n ++\n ++\t\tif (set_recommended_config(1) >= 0)\n ++\t\t\tsucceeded = 1;\n ++\n +loop_end:\n +\t\tif (!succeeded) {\n +\t\t\tres = -1;\n\n-- \ngitgitgadget\n"},{"id":"481047","messageId":"ac234b157558e6142d0d06f6baf80e1e80f063b5.1693230746.git.gitgitgadget@gmail.com","threadId":"60111","inReplyTo":"pull.1569.v3.git.1693230746.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] scalar reconfigure: help users remove buggy repos","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-28T13:52:26Z","receivedAt":"2023-08-28T13:53:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen running 'scalar reconfigure -a', Scalar has warning messages about\nthe repository missing (or not containing a .git directory). Failures\ncan also happen while trying to modify the repository-local config for\nthat repository.\n\nThese warnings may seem confusing to users who don't understand what\nthey mean or how to stop them.\n\nAdd a warning that instructs the user how to remove the warning in\nfuture installations.\n\nSigned-off-by: Derrick Stolee \n---\n scalar.c | 59 +++++++++++++++++++++++++++++++++++++++++---------------\n 1 file changed, 43 insertions(+), 16 deletions(-)\n\ndiff --git a/scalar.c b/scalar.c\nindex 938bb73f3ce..fb2940c2a00 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -664,6 +664,7 @@ static int cmd_reconfigure(int argc, const char **argv)\n \tgit_config(get_scalar_repos, &scalar_repos);\n \n \tfor (i = 0; i < scalar_repos.nr; i++) {\n+\t\tint succeeded = 0;\n \t\tconst char *dir = scalar_repos.items[i].string;\n \n \t\tstrbuf_reset(&commondir);\n@@ -674,30 +675,56 @@ static int cmd_reconfigure(int argc, const char **argv)\n \n \t\t\tif (errno != ENOENT) {\n \t\t\t\twarning_errno(_(\"could not switch to '%s'\"), dir);\n-\t\t\t\tres = -1;\n-\t\t\t\tcontinue;\n+\t\t\t\tgoto loop_end;\n \t\t\t}\n \n \t\t\tstrbuf_addstr(&buf, dir);\n \t\t\tif (remove_deleted_enlistment(&buf))\n-\t\t\t\tres = error(_(\"could not remove stale \"\n-\t\t\t\t\t \"scalar.repo '%s'\"), dir);\n-\t\t\telse\n-\t\t\t\twarning(_(\"removing stale scalar.repo '%s'\"),\n+\t\t\t\terror(_(\"could not remove stale \"\n+\t\t\t\t\t\"scalar.repo '%s'\"), dir);\n+\t\t\telse {\n+\t\t\t\twarning(_(\"removed stale scalar.repo '%s'\"),\n \t\t\t\t\tdir);\n+\t\t\t\tsucceeded = 1;\n+\t\t\t}\n \t\t\tstrbuf_release(&buf);\n-\t\t} else if (discover_git_directory(&commondir, &gitdir) < 0) {\n-\t\t\twarning_errno(_(\"git repository gone in '%s'\"), dir);\n-\t\t\tres = -1;\n-\t\t} else {\n-\t\t\tgit_config_clear();\n+\t\t\tgoto loop_end;\n+\t\t}\n+\n+\t\tswitch (discover_git_directory_reason(&commondir, &gitdir)) {\n+\t\tcase GIT_DIR_INVALID_OWNERSHIP:\n+\t\t\twarning(_(\"repository at '%s' has different owner\"), dir);\n+\t\t\tgoto loop_end;\n+\n+\t\tcase GIT_DIR_INVALID_GITFILE:\n+\t\tcase GIT_DIR_INVALID_FORMAT:\n+\t\t\twarning(_(\"repository at '%s' has a format issue\"), dir);\n+\t\t\tgoto loop_end;\n+\n+\t\tcase GIT_DIR_DISCOVERED:\n+\t\t\tsucceeded = 1;\n+\t\t\tbreak;\n+\n+\t\tdefault:\n+\t\t\twarning(_(\"repository not found in '%s'\"), dir);\n+\t\t\tbreak;\n+\t\t}\n \n-\t\t\tthe_repository = &r;\n-\t\t\tr.commondir = commondir.buf;\n-\t\t\tr.gitdir = gitdir.buf;\n+\t\tgit_config_clear();\n \n-\t\t\tif (set_recommended_config(1) < 0)\n-\t\t\t\tres = -1;\n+\t\tthe_repository = &r;\n+\t\tr.commondir = commondir.buf;\n+\t\tr.gitdir = gitdir.buf;\n+\n+\t\tif (set_recommended_config(1) >= 0)\n+\t\t\tsucceeded = 1;\n+\n+loop_end:\n+\t\tif (!succeeded) {\n+\t\t\tres = -1;\n+\t\t\twarning(_(\"to unregister this repository from Scalar, run\\n\"\n+\t\t\t\t \"\\tgit config --global --unset --fixed-value scalar.repo \\\"%s\\\"\"),\n+\t\t\t\tdir);\n \t\t}\n \t}\n \n-- \ngitgitgadget\n"},{"id":"481048","messageId":"3c16fa6897f9138fe25ad8c0d4fead333668f9fc.1693230746.git.gitgitgadget@gmail.com","threadId":"60111","inReplyTo":"pull.1569.v3.git.1693230746.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] setup: add discover_git_directory_reason()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-28T13:52:25Z","receivedAt":"2023-08-28T13:53:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThere are many reasons why discovering a Git directory may fail. In\nparticular, 8959555cee7 (setup_git_directory(): add an owner check for\nthe top-level directory, 2022-03-02) added ownership checks as a\nsecurity precaution.\n\nCallers attempting to set up a Git directory may want to inform the user\nabout the reason for the failure. For that, expose the enum\ndiscovery_result from within setup.c and move it into cache.h where\ndiscover_git_directory() is defined.\n\nI initially wanted to change the return type of discover_git_directory()\nto be this enum, but several callers rely upon the \"zero means success\".\nThe two problems with this are:\n\n1. The zero value of the enum is actually GIT_DIR_NONE, so nonpositive\n results are errors.\n\n2. There are multiple successful states; positive results are\n successful.\n\nIt is worth noting that GIT_DIR_NONE is not returned, so we remove this\noption from the enum. We must be careful to keep the successful reasons\nas positive values, so they are given explicit positive values.\n\nInstead of updating all callers immediately, add a new method,\ndiscover_git_directory_reason(), and convert discover_git_directory() to\nbe a thin shim on top of it.\n\nOne thing that is important to note is that discover_git_directory()\npreviously returned -1 on error, so let's continue that into the future.\nThere is only one caller (in scalar.c) that depends on that signedness\ninstead of a non-zero check, so clean that up, too.\n\nBecause there are extra checks that discover_git_directory_reason() does\nafter setup_git_directory_gently_1(), there are other modes that can be\nreturned for failure states. Add these modes to the enum, but be sure to\nexplicitly add them as BUG() states in the switch of\nsetup_git_directory_gently().\n\nSigned-off-by: Derrick Stolee \n---\n setup.c | 34 ++++++++++++----------------------\n setup.h | 35 ++++++++++++++++++++++++++++++++---\n 2 files changed, 44 insertions(+), 25 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 18927a847b8..2e607632dbd 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1221,19 +1221,6 @@ static const char *allowed_bare_repo_to_string(\n \treturn NULL;\n }\n \n-enum discovery_result {\n-\tGIT_DIR_NONE = 0,\n-\tGIT_DIR_EXPLICIT,\n-\tGIT_DIR_DISCOVERED,\n-\tGIT_DIR_BARE,\n-\t/* these are errors */\n-\tGIT_DIR_HIT_CEILING = -1,\n-\tGIT_DIR_HIT_MOUNT_POINT = -2,\n-\tGIT_DIR_INVALID_GITFILE = -3,\n-\tGIT_DIR_INVALID_OWNERSHIP = -4,\n-\tGIT_DIR_DISALLOWED_BARE = -5,\n-};\n-\n /*\n * We cannot decide in this function whether we are in the work tree or\n * not, since the config can only be read _after_ this function was called.\n@@ -1385,21 +1372,23 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t}\n }\n \n-int discover_git_directory(struct strbuf *commondir,\n-\t\t\t struct strbuf *gitdir)\n+enum discovery_result discover_git_directory_reason(struct strbuf *commondir,\n+\t\t\t\t\t\t struct strbuf *gitdir)\n {\n \tstruct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;\n \tsize_t gitdir_offset = gitdir->len, cwd_len;\n \tsize_t commondir_offset = commondir->len;\n \tstruct repository_format candidate = REPOSITORY_FORMAT_INIT;\n+\tenum discovery_result result;\n \n \tif (strbuf_getcwd(&dir))\n-\t\treturn -1;\n+\t\treturn GIT_DIR_CWD_FAILURE;\n \n \tcwd_len = dir.len;\n-\tif (setup_git_directory_gently_1(&dir, gitdir, NULL, 0) <= 0) {\n+\tresult = setup_git_directory_gently_1(&dir, gitdir, NULL, 0);\n+\tif (result <= 0) {\n \t\tstrbuf_release(&dir);\n-\t\treturn -1;\n+\t\treturn result;\n \t}\n \n \t/*\n@@ -1429,11 +1418,11 @@ int discover_git_directory(struct strbuf *commondir,\n \t\tstrbuf_setlen(commondir, commondir_offset);\n \t\tstrbuf_setlen(gitdir, gitdir_offset);\n \t\tclear_repository_format(&candidate);\n-\t\treturn -1;\n+\t\treturn GIT_DIR_INVALID_FORMAT;\n \t}\n \n \tclear_repository_format(&candidate);\n-\treturn 0;\n+\treturn result;\n }\n \n const char *setup_git_directory_gently(int *nongit_ok)\n@@ -1515,10 +1504,11 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t}\n \t\t*nongit_ok = 1;\n \t\tbreak;\n-\tcase GIT_DIR_NONE:\n+\tcase GIT_DIR_CWD_FAILURE:\n+\tcase GIT_DIR_INVALID_FORMAT:\n \t\t/*\n \t\t * As a safeguard against setup_git_directory_gently_1 returning\n-\t\t * this value, fallthrough to BUG. Otherwise it is possible to\n+\t\t * these values, fallthrough to BUG. Otherwise it is possible to\n \t\t * set startup_info->have_repository to 1 when we did nothing to\n \t\t * find a repository.\n \t\t */\ndiff --git a/setup.h b/setup.h\nindex 58fd2605dd2..b48cf1c43b5 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -42,16 +42,45 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);\n #define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)\n \n void setup_work_tree(void);\n+\n+/*\n+ * discover_git_directory_reason() is similar to discover_git_directory(),\n+ * except it returns an enum value instead. It is important to note that\n+ * a zero-valued return here is actually GIT_DIR_NONE, which is different\n+ * from discover_git_directory.\n+ */\n+enum discovery_result {\n+\tGIT_DIR_EXPLICIT = 1,\n+\tGIT_DIR_DISCOVERED = 2,\n+\tGIT_DIR_BARE = 3,\n+\t/* these are errors */\n+\tGIT_DIR_HIT_CEILING = -1,\n+\tGIT_DIR_HIT_MOUNT_POINT = -2,\n+\tGIT_DIR_INVALID_GITFILE = -3,\n+\tGIT_DIR_INVALID_OWNERSHIP = -4,\n+\tGIT_DIR_DISALLOWED_BARE = -5,\n+\tGIT_DIR_INVALID_FORMAT = -6,\n+\tGIT_DIR_CWD_FAILURE = -7,\n+};\n+enum discovery_result discover_git_directory_reason(struct strbuf *commondir,\n+\t\t\t\t\t\t struct strbuf *gitdir);\n+\n /*\n * Find the commondir and gitdir of the repository that contains the current\n * working directory, without changing the working directory or other global\n * state. The result is appended to commondir and gitdir. If the discovered\n * gitdir does not correspond to a worktree, then 'commondir' and 'gitdir' will\n * both have the same result appended to the buffer. The return value is\n- * either 0 upon success and non-zero if no repository was found.\n+ * either 0 upon success and -1 if no repository was found.\n */\n-int discover_git_directory(struct strbuf *commondir,\n-\t\t\t struct strbuf *gitdir);\n+static inline int discover_git_directory(struct strbuf *commondir,\n+\t\t\t\t\t struct strbuf *gitdir)\n+{\n+\tif (discover_git_directory_reason(commondir, gitdir) <= 0)\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n const char *setup_git_directory_gently(int *);\n const char *setup_git_directory(void);\n char *prefix_path(const char *prefix, int len, const char *path);\n-- \ngitgitgadget\n\n"},{"id":"481055","messageId":"xmqq5y4zi1cc.fsf@gitster.g","threadId":"60111","inReplyTo":"pull.1569.v3.git.1693230746.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/3] scalar: two downstream improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-28T16:22:11Z","receivedAt":"2023-08-28T16:23:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> While updating git-for-windows/git and microsoft/git for the 2.42.0 release\n> window, a few patches that we've been running in those forks for a while\n> came to light as something that would be beneficial to the core Git project.\n> Here are some that are focused on the 'scalar' command.\n>\n> * Patch 1 adds a --no-src option to scalar clone to appease users who want\n> to use scalar but object to the creation of the src directory.\n> * Patches 2 and 3 help when scalar reconfigure -a fails. Patch 2 is a\n> possibly helpful change on its own for other uses in the future.\n>\n>\n> Updates in V3\n> =============\n>\n> * Several commit message edits.\n> * An important case that was dropped in v2's patch 2 is reintroduced (even\n> though it is modified in patch 3).\n> * An error message is added for corrupt Git repositories.\n\nThanks. The updated series look good. Especially the updates to\nthe commit message all looked excellent.\n\nWill queue. Let's merge it down to 'next'.\n\n"}]}