scalar: two downstream improvements

While updating git-for-windows/git and microsoft/git for the 2.42.0 release window, a few patches that we've been running in those forks for a while came to light as something that would be beneficial to the core Git project. Here are some that are focused on the 'scalar' command.

* Patch 1 adds a --no-src option to scalar clone to appease users who want to use scalar but object to the creation of the src directory.
* Patches 2 and 3 help when scalar reconfigure -a fails. Patch 2 is a possibly helpful change on its own for other uses in the future.

Thanks, -Stolee

Derrick Stolee (3):
scalar: add --[no-]src option
setup: add discover_git_directory_reason() scalar reconfigure: help users remove buggy repos

 Documentation/scalar.txt |  8 ++++-
 scalar.c                 | 67 ++++++++++++++++++++++++++++------------
 setup.c                  | 32 +++++++------------
 setup.h                  | 32 +++++++++++++++++--
 t/t9211-scalar-clone.sh  |  8 +++++
 5 files changed, 104 insertions(+), 43 deletions(-)

base-commit: a82fb66fed250e16d3010c75404503bea3f0ab61
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1569%2Fderrickstolee%2Fscalar-no-src-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1569/derrickstolee/scalar-no-src-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1569

gitgitgadget

From: Derrick Stolee <[email protected]>

Some users have strong aversions to Scalar's opinion that the repository should be in a 'src' directory, even though it creates a clean slate for placing build outputs in adjacent directories.

The --no-src option allows users to opt-out of the default behavior.

While adding options, make sure the usage output by 'scalar clone -h' reports the same as the SYNOPSIS line in Documentation/scalar.txt.

---
 Documentation/scalar.txt |  8 +++++++-
 scalar.c                 | 11 +++++++++--
 t/t9211-scalar-clone.sh  |  8 ++++++++
 3 files changed, 24 insertions(+), 3 deletions(-)

index f33436c7f65..cd65b3e230d 100644
@@ -8,7 +8,8 @@ scalar - A tool for managing large Git repositories
 SYNOPSIS
 --------
 [verse]
-scalar clone [--single-branch] [--branch <main-branch>] [--full-clone] <url> [<enlistment>]
+scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]
+	[--[no-]src] <url> [<enlistment>]
 scalar list
 scalar register [<enlistment>]
 scalar unregister [<enlistment>]

@@ -80,6 +81,11 @@ remote-tracking branch for the branch this option was used for the initial
 cloning. If the HEAD at the remote did not point at any branch when
 `--single-branch` clone was made, no remote-tracking branch is created.
 
+--[no-]src::
+	Specify if the repository should be created within a `src` directory
+	within `<enlistment>`. This is the default behavior, so use
+	`--no-src` to opt-out of the creation of the `src` directory.
+
 --[no-]full-clone::
 	A sparse-checkout is initialized by default. This behavior can be
 	turned off via `--full-clone`.

index df7358f481c..938bb73f3ce 100644
@@ -409,6 +409,7 @@ static int cmd_clone(int argc, const char **argv)
 {
 	const char *branch = NULL;
 	int full_clone = 0, single_branch = 0, show_progress = isatty(2);
+	int src = 1;
 	struct option clone_options[] = {
 		OPT_STRING('b', "branch", &branch, N_("<branch>"),
 			   N_("branch to checkout after clone")),

@@ -417,10 +418,13 @@ static int cmd_clone(int argc, const char **argv)
 		OPT_BOOL(0, "single-branch", &single_branch,
 			 N_("only download metadata for the branch that will "
 			    "be checked out")),
+		OPT_BOOL(0, "src", &src,
+			 N_("create repository within 'src' directory")),
 		OPT_END(),
 	};
 	const char * const clone_usage[] = {
-		N_("scalar clone [<options>] [--] <repo> [<dir>]"),
+		N_("scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]\n"
+		   "\t[--[no-]src] <url> [<enlistment>]"),
 		NULL
 	};
 	const char *url;

@@ -456,7 +460,10 @@ static int cmd_clone(int argc, const char **argv)
 	if (is_directory(enlistment))
 		die(_("directory '%s' exists already"), enlistment);
 
-	dir = xstrfmt("%s/src", enlistment);
+	if (src)
+		dir = xstrfmt("%s/src", enlistment);
+	else
+		dir = xstrdup(enlistment);
 
 	strbuf_reset(&buf);
 	if (branch)

index 872ad1c9c2b..7ee73aba092 100755
@@ -180,4 +180,12 @@ test_expect_success 'scalar clone warns when background maintenance fails' '
 	grep "could not turn on maintenance" err
 '
 
+test_expect_success '`scalar clone --no-src`' '
+	scalar clone --src "file://$(pwd)/to-clone" with-src &&
+	scalar clone --no-src "file://$(pwd)/to-clone" without-src &&
+
+	test_path_is_dir with-src/src &&
+	test_path_is_missing without-src/src
+'
+
 test_done

gitgitgadget

Your confusion makes sense. Focusing on the location of the cloned repository is a good way to focus the option for the reader.

Good idea.

Thanks,
-Stolee

From: Derrick Stolee <[email protected]>

There are many reasons why discovering a Git directory may fail. In particular, 8959555cee7 (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02) added ownership checks as a security precaution.

Callers attempting to set up a Git directory may want to inform the user about the reason for the failure. For that, expose the enum discovery_result from within setup.c and into cache.h where discover_git_directory() is defined.

I initially wanted to change the return type of discover_git_directory() to be this enum, but several callers rely upon the "zero means success". The two problems with this are:

1. The zero value of the enum is actually GIT_DIR_NONE, so nonpositive results are errors.

2. There are multiple successful states, so some positive results are successful.

Instead of updating all callers immediately, add a new method, discover_git_directory_reason(), and convert discover_git_directory() to be a thin shim on top of it.

Because there are extra checks that discover_git_directory_reason() does after setup_git_directory_gently_1(), there are other modes that can be returned for failure states. Add these modes to the enum, but be sure to explicitly add them as BUG() states in the switch of setup_git_directory_gently().

---
 setup.c | 32 +++++++++++---------------------
 setup.h | 32 ++++++++++++++++++++++++++++++--
 2 files changed, 41 insertions(+), 23 deletions(-)

index 18927a847b8..bbf8f684d93 100644
@@ -1221,19 +1221,6 @@ static const char *allowed_bare_repo_to_string(
 	return NULL;
 }
 
-enum discovery_result {
-	GIT_DIR_NONE = 0,
-	GIT_DIR_EXPLICIT,
-	GIT_DIR_DISCOVERED,
-	GIT_DIR_BARE,
-	/* these are errors */
-	GIT_DIR_HIT_CEILING = -1,
-	GIT_DIR_HIT_MOUNT_POINT = -2,
-	GIT_DIR_INVALID_GITFILE = -3,
-	GIT_DIR_INVALID_OWNERSHIP = -4,
-	GIT_DIR_DISALLOWED_BARE = -5,
-};
-
 /*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.

@@ -1385,21 +1372,22 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 	}
 }
 
-int discover_git_directory(struct strbuf *commondir,
-			   struct strbuf *gitdir)
+enum discovery_result discover_git_directory_reason(struct strbuf *commondir,
+						    struct strbuf *gitdir)
 {
 	struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
 	size_t gitdir_offset = gitdir->len, cwd_len;
 	size_t commondir_offset = commondir->len;
 	struct repository_format candidate = REPOSITORY_FORMAT_INIT;
+	enum discovery_result result;
 
 	if (strbuf_getcwd(&dir))
-		return -1;
+		return GIT_DIR_CWD_FAILURE;
 
 	cwd_len = dir.len;
-	if (setup_git_directory_gently_1(&dir, gitdir, NULL, 0) <= 0) {
+	if ((result = setup_git_directory_gently_1(&dir, gitdir, NULL, 0)) <= 0) {
 		strbuf_release(&dir);
-		return -1;
+		return result;
 	}
 
 	/*

@@ -1429,11 +1417,11 @@ int discover_git_directory(struct strbuf *commondir,
 		strbuf_setlen(commondir, commondir_offset);
 		strbuf_setlen(gitdir, gitdir_offset);
 		clear_repository_format(&candidate);
-		return -1;
+		return GIT_DIR_INVALID_FORMAT;
 	}
 
 	clear_repository_format(&candidate);
-	return 0;
+	return result;
 }
 
 const char *setup_git_directory_gently(int *nongit_ok)

@@ -1516,9 +1504,11 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		*nongit_ok = 1;
 		break;
 	case GIT_DIR_NONE:
+	case GIT_DIR_CWD_FAILURE:
+	case GIT_DIR_INVALID_FORMAT:
 		/*
 		 * As a safeguard against setup_git_directory_gently_1 returning
-		 * this value, fallthrough to BUG. Otherwise it is possible to
+		 * these values, fallthrough to BUG. Otherwise it is possible to
 		 * set startup_info->have_repository to 1 when we did nothing to
 		 * find a repository.
 		 */

index 58fd2605dd2..b87d0d6fb2b 100644
@@ -42,6 +42,30 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);
 #define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)
 
 void setup_work_tree(void);
+
+/*
+ * discover_git_directory_reason() is similar to discover_git_directory(),
+ * except it returns an enum value instead. It is important to note that
+ * a zero-valued return here is actually GIT_DIR_NONE, which is different
+ * from discover_git_directory.
+ */
+enum discovery_result {
+	GIT_DIR_NONE = 0,
+	GIT_DIR_EXPLICIT,
+	GIT_DIR_DISCOVERED,
+	GIT_DIR_BARE,
+	/* these are errors */
+	GIT_DIR_HIT_CEILING = -1,
+	GIT_DIR_HIT_MOUNT_POINT = -2,
+	GIT_DIR_INVALID_GITFILE = -3,
+	GIT_DIR_INVALID_OWNERSHIP = -4,
+	GIT_DIR_DISALLOWED_BARE = -5,
+	GIT_DIR_INVALID_FORMAT = -6,
+	GIT_DIR_CWD_FAILURE = -7,
+};
+enum discovery_result discover_git_directory_reason(struct strbuf *commondir,
+						    struct strbuf *gitdir);
+
 /*
  * Find the commondir and gitdir of the repository that contains the current
  * working directory, without changing the working directory or other global

@@ -50,8 +74,12 @@ void setup_work_tree(void);
  * both have the same result appended to the buffer.  The return value is
  * either 0 upon success and non-zero if no repository was found.
  */
-int discover_git_directory(struct strbuf *commondir,
-			   struct strbuf *gitdir);
+static inline int discover_git_directory(struct strbuf *commondir,
+					 struct strbuf *gitdir)
+{
+	return discover_git_directory_reason(commondir, gitdir) <= 0;
+}
+
 const char *setup_git_directory_gently(int *);
 const char *setup_git_directory(void);
 char *prefix_path(const char *prefix, int len, const char *path);

gitgitgadget

True. discover_git_directory() already knows that negative return values from setup_git_directory_gently_1() signal errors while 0 or positive are OK.

Makes it sound as if some positive results are not successes, but is that really the case?

It makes sense to insulate callers who only want to know if the discovery was successful or not (there only are two existing callers anyway) from the details. And turning a thin wrapper to the new API that gives richer return codes is the way to go. Nicely designed.

Good.

	result = setup_git_directory_gently_1(...);
	if (result <= 0) {

	enum discovery_result res = GIT_DIR_NONE;

	if (some complex condition)
		res = ...;
	else if (another complex condition)
		res = ...;

	... sometime later ...
	if (res <= 0)
		we found a bad one

NOnononono. negative are not. 0 is not returned, so if we saw one, it would be an error. And positives are OK.

Sorry for the confusion.

From: Derrick Stolee <[email protected]>

When running 'scalar reconfigure -a', Scalar has warning messages about the repository missing (or not containing a .git directory). Failures can also happen while trying to modify the repository-local config for that repository.

These warnings may seem confusing to users who don't understand what they mean or how to stop them.

Add a warning that instructs the user how to remove the warning in future installations.

---
 scalar.c | 56 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 17 deletions(-)

index 938bb73f3ce..7d87d7ea724 100644
@@ -664,6 +664,7 @@ static int cmd_reconfigure(int argc, const char **argv)
 	git_config(get_scalar_repos, &scalar_repos);
 
 	for (i = 0; i < scalar_repos.nr; i++) {
+		int failed = 0;
 		const char *dir = scalar_repos.items[i].string;
 
 		strbuf_reset(&commondir);

@@ -674,30 +675,51 @@ static int cmd_reconfigure(int argc, const char **argv)
 
 			if (errno != ENOENT) {
 				warning_errno(_("could not switch to '%s'"), dir);
-				res = -1;
-				continue;
+				failed = -1;
+				goto loop_end;
 			}
 
 			strbuf_addstr(&buf, dir);
 			if (remove_deleted_enlistment(&buf))
-				res = error(_("could not remove stale "
-					      "scalar.repo '%s'"), dir);
+				failed = error(_("could not remove stale "
+						 "scalar.repo '%s'"), dir);
 			else
-				warning(_("removing stale scalar.repo '%s'"),
+				warning(_("removed stale scalar.repo '%s'"),
 					dir);
 			strbuf_release(&buf);
-		} else if (discover_git_directory(&commondir, &gitdir) < 0) {
-			warning_errno(_("git repository gone in '%s'"), dir);
-			res = -1;
-		} else {
-			git_config_clear();
-
-			the_repository = &r;
-			r.commondir = commondir.buf;
-			r.gitdir = gitdir.buf;
-
-			if (set_recommended_config(1) < 0)
-				res = -1;
+			goto loop_end;
+		}
+
+		switch (discover_git_directory_reason(&commondir, &gitdir)) {
+		case GIT_DIR_INVALID_OWNERSHIP:
+			warning(_("repository at '%s' has different owner"), dir);
+			failed = -1;
+			goto loop_end;
+
+		case GIT_DIR_DISCOVERED:
+			break;
+
+		default:
+			warning(_("repository not found in '%s'"), dir);
+			failed = -1;
+			break;
+		}
+
+		git_config_clear();
+
+		the_repository = &r;
+		r.commondir = commondir.buf;
+		r.gitdir = gitdir.buf;
+
+		if (set_recommended_config(1) < 0)
+			failed = -1;
+
+loop_end:
+		if (failed) {
+			res = failed;
+			warning(_("to unregister this repository from Scalar, run\n"
+				  "\tgit config --global --unset --fixed-value scalar.repo \"%s\""),
+				dir);
 		}
 	}
 

gitgitgadget

While updating git-for-windows/git and microsoft/git for the 2.42.0 release window, a few patches that we've been running in those forks for a while came to light as something that would be beneficial to the core Git project. Here are some that are focused on the 'scalar' command.

* Patch 1 adds a --no-src option to scalar clone to appease users who want to use scalar but object to the creation of the src directory.
* Patches 2 and 3 help when scalar reconfigure -a fails. Patch 2 is a possibly helpful change on its own for other uses in the future.

Updates in V2
=============

Thanks, Junio, for the helpful review!

* In Patch 1, the '--[no-]src' documentation is tightened and the tests check the contents of the repository worktree.
* In Patch 2, the commit message is reworded to be more clear about positive values of the enum.
* In Patch 2, the GIT_DIR_NONE option of the enum is never returned, so it does not need to exist. A case in scalar.c referenced it, so it is removed as part of the patch (though that case was removed later by patch 3 anyway).
* In Patch 2, the discover_git_directory() wrapper is updated to return -1 instead of 1, as it did before this patch.
* In Patch 3, the 'failed' variable is renamed to 'succeeded' and the cases that update the value are swapped. The return code is set to -1 for any error instead of having a custom value based on the return from error() or error_errno().

Thanks, -Stolee

Derrick Stolee (3):
scalar: add --[no-]src option
setup: add discover_git_directory_reason() scalar reconfigure: help users remove buggy repos

 Documentation/scalar.txt |  8 ++++-
 scalar.c                 | 65 +++++++++++++++++++++++++++++-----------
 setup.c                  | 34 ++++++++-------------
 setup.h                  | 35 ++++++++++++++++++++--
 t/t9211-scalar-clone.sh  | 12 ++++++++
 5 files changed, 110 insertions(+), 44 deletions(-)

base-commit: a82fb66fed250e16d3010c75404503bea3f0ab61
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1569%2Fderrickstolee%2Fscalar-no-src-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1569/derrickstolee/scalar-no-src-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1569

Range-diff vs v1:

1: c1c7e2f049e ! 1: 0b6957beccb scalar: add --[no-]src option
@@ Documentation/scalar.txt: remote-tracking branch for the branch this option was
--single-branch clone was made, no remote-tracking branch is created.

+--[no-]src::
-+ Specify if the repository should be created within a src directory
-+ within <enlistment>. This is the default behavior, so use
-+ --no-src to opt-out of the creation of the src directory.
++ By default, scalar clone places the cloned repository within a
++ <entlistment>/src directory. Use --no-src to place the cloned
++ repository directly in the <enlistment> directory.
+
--[no-]full-clone::
A sparse-checkout is initialized by default. This behavior can be
@@ t/t9211-scalar-clone.sh: test_expect_success 'scalar clone warns when background
+ scalar clone --no-src "file://$(pwd)/to-clone" without-src &&
+
+ test_path_is_dir with-src/src &&
-+ test_path_is_missing without-src/src
++ test_path_is_missing without-src/src &&
++
++ (cd with-src/src && ls ?*) >with &&
++ (cd without-src && ls ?*) >without &&
++ test_cmp with without
+'
+
test_done
2: fbba6252aea ! 2: 787af0f9744 setup: add discover_git_directory_reason()
@@ Commit message
1. The zero value of the enum is actually GIT_DIR_NONE, so nonpositive results are errors.

- 2. There are multiple successful states, so some positive results are
+ 2. There are multiple successful states; positive results are
successful.

+ It is worth noting that GIT_DIR_NONE is not returned, so we remove this
+ option from the enum. We must be careful to keep the successful reasons
+ as positive values, so they are given explicit positive values.
+ Further, a use in scalar.c was previously impossible, so it is removed.
+
Instead of updating all callers immediately, add a new method, discover_git_directory_reason(), and convert discover_git_directory() to be a thin shim on top of it.

+ One thing that is important to note is that discover_git_directory()
+ previously returned -1 on error, so let's continue that into the future.
+ There is only one caller (in scalar.c) that depends on that signedness
+ instead of a non-zero check, so clean that up, too.
+
Because there are extra checks that discover_git_directory_reason() does after setup_git_directory_gently_1(), there are other modes that can be returned for failure states. Add these modes to the enum, but be sure to
@@ Commit message

+ ## scalar.c ##
+@@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)
+ warning(_("removing stale scalar.repo '%s'"),
+ dir);
+ strbuf_release(&buf);
+- } else if (discover_git_directory(&commondir, &gitdir) < 0) {
+- warning_errno(_("git repository gone in '%s'"), dir);
+- res = -1;
+ } else {
+ git_config_clear();
+
+
## setup.c ##
@@ setup.c: static const char *allowed_bare_repo_to_string(
return NULL;
@@ setup.c: static enum discovery_result setup_git_directory_gently_1(struct strbuf

cwd_len = dir.len;
- if (setup_git_directory_gently_1(&dir, gitdir, NULL, 0) <= 0) {
-+ if ((result = setup_git_directory_gently_1(&dir, gitdir, NULL, 0)) <= 0) {
++ result = setup_git_directory_gently_1(&dir, gitdir, NULL, 0);
++ if (result <= 0) {
strbuf_release(&dir);
- return -1;
+ return result;
@@ setup.c: int discover_git_directory(struct strbuf *commondir,

const char *setup_git_directory_gently(int *nongit_ok)
@@ setup.c: const char *setup_git_directory_gently(int *nongit_ok)
+ }
*nongit_ok = 1;
break;
- case GIT_DIR_NONE:
+- case GIT_DIR_NONE:
+ case GIT_DIR_CWD_FAILURE:
+ case GIT_DIR_INVALID_FORMAT:
/*
@@ setup.h: const char *resolve_gitdir_gently(const char *suspect, int *return_erro
+ * from discover_git_directory.
+ */
+enum discovery_result {
-+ GIT_DIR_NONE = 0,
-+ GIT_DIR_EXPLICIT,
-+ GIT_DIR_DISCOVERED,
-+ GIT_DIR_BARE,
++ GIT_DIR_EXPLICIT = 1,
++ GIT_DIR_DISCOVERED = 2,
++ GIT_DIR_BARE = 3,
+ /* these are errors */
+ GIT_DIR_HIT_CEILING = -1,
+ GIT_DIR_HIT_MOUNT_POINT = -2,
@@ setup.h: const char *resolve_gitdir_gently(const char *suspect, int *return_erro
/*
* Find the commondir and gitdir of the repository that contains the current
* working directory, without changing the working directory or other global
-@@ setup.h: void setup_work_tree(void);
+ * state. The result is appended to commondir and gitdir. If the discovered
+ * gitdir does not correspond to a worktree, then 'commondir' and 'gitdir' will
* both have the same result appended to the buffer. The return value is
- * either 0 upon success and non-zero if no repository was found. +- * either 0 upon success and non-zero if no repository was found. ++ * either 0 upon success and -1 if no repository was found.
*/
-int discover_git_directory(struct strbuf *commondir,
- struct strbuf *gitdir);
+static inline int discover_git_directory(struct strbuf *commondir,
+ struct strbuf *gitdir)
+{
-+ return discover_git_directory_reason(commondir, gitdir) <= 0;
++ if (discover_git_directory_reason(commondir, gitdir) <= 0)
++ return -1;
++ return 0;
+}
+
const char *setup_git_directory_gently(int *);
3: 907410f76c4 ! 3: 7ac7311863d scalar reconfigure: help users remove buggy repos

     @@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)
       	git_config(get_scalar_repos, &scalar_repos);
       
       	for (i = 0; i < scalar_repos.nr; i++) {
     -+		int failed = 0;
     ++		int succeeded = 0;
       		const char *dir = scalar_repos.items[i].string;
       
       		strbuf_reset(&commondir);
     @@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)
       				warning_errno(_("could not switch to '%s'"), dir);
      -				res = -1;
      -				continue;
     -+				failed = -1;
      +				goto loop_end;
       			}
       
     @@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)
       			if (remove_deleted_enlistment(&buf))
      -				res = error(_("could not remove stale "
      -					      "scalar.repo '%s'"), dir);
     -+				failed = error(_("could not remove stale "
     -+						 "scalar.repo '%s'"), dir);
     - 			else
     +-			else
      -				warning(_("removing stale scalar.repo '%s'"),
     ++				error(_("could not remove stale "
     ++					"scalar.repo '%s'"), dir);
     ++			else {
      +				warning(_("removed stale scalar.repo '%s'"),
       					dir);
     ++				succeeded = 1;
     ++			}
       			strbuf_release(&buf);
     --		} else if (discover_git_directory(&commondir, &gitdir) < 0) {
     --			warning_errno(_("git repository gone in '%s'"), dir);
     --			res = -1;
      -		} else {
      -			git_config_clear();
     --
     --			the_repository = &r;
     --			r.commondir = commondir.buf;
     --			r.gitdir = gitdir.buf;
     --
     --			if (set_recommended_config(1) < 0)
     --				res = -1;
      +			goto loop_end;
      +		}
      +
      +		switch (discover_git_directory_reason(&commondir, &gitdir)) {
      +		case GIT_DIR_INVALID_OWNERSHIP:
      +			warning(_("repository at '%s' has different owner"), dir);
     -+			failed = -1;
      +			goto loop_end;
      +
      +		case GIT_DIR_DISCOVERED:
     ++			succeeded = 1;
      +			break;
      +
      +		default:
      +			warning(_("repository not found in '%s'"), dir);
     -+			failed = -1;
      +			break;
      +		}
      +
     @@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)
      +		the_repository = &r;
      +		r.commondir = commondir.buf;
      +		r.gitdir = gitdir.buf;
     -+
     -+		if (set_recommended_config(1) < 0)
     -+			failed = -1;
     -+
     + 
     +-			the_repository = &r;
     +-			r.commondir = commondir.buf;
     +-			r.gitdir = gitdir.buf;
     ++		if (set_recommended_config(1) >= 0)
     ++			succeeded = 1;
     + 
     +-			if (set_recommended_config(1) < 0)
     +-				res = -1;
      +loop_end:
     -+		if (failed) {
     -+			res = failed;
     ++		if (!succeeded) {
     ++			res = -1;
      +			warning(_("to unregister this repository from Scalar, run\n"
      +				  "\tgit config --global --unset --fixed-value scalar.repo \"%s\""),
      +				dir);

gitgitgadget

From: Derrick Stolee <[email protected]>

Some users have strong aversions to Scalar's opinion that the repository should be in a 'src' directory, even though it creates a clean slate for placing build outputs in adjacent directories.

The --no-src option allows users to opt-out of the default behavior.

While adding options, make sure the usage output by 'scalar clone -h' reports the same as the SYNOPSIS line in Documentation/scalar.txt.

---
 Documentation/scalar.txt |  8 +++++++-
 scalar.c                 | 11 +++++++++--
 t/t9211-scalar-clone.sh  | 12 ++++++++++++
 3 files changed, 28 insertions(+), 3 deletions(-)

index f33436c7f65..361f51a6473 100644
@@ -8,7 +8,8 @@ scalar - A tool for managing large Git repositories
 SYNOPSIS
 --------
 [verse]
-scalar clone [--single-branch] [--branch <main-branch>] [--full-clone] <url> [<enlistment>]
+scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]
+	[--[no-]src] <url> [<enlistment>]
 scalar list
 scalar register [<enlistment>]
 scalar unregister [<enlistment>]

@@ -80,6 +81,11 @@ remote-tracking branch for the branch this option was used for the initial
 cloning. If the HEAD at the remote did not point at any branch when
 `--single-branch` clone was made, no remote-tracking branch is created.
 
+--[no-]src::
+	By default, `scalar clone` places the cloned repository within a
+	`<entlistment>/src` directory. Use `--no-src` to place the cloned
+	repository directly in the `<enlistment>` directory.
+
 --[no-]full-clone::
 	A sparse-checkout is initialized by default. This behavior can be
 	turned off via `--full-clone`.

index df7358f481c..938bb73f3ce 100644
@@ -409,6 +409,7 @@ static int cmd_clone(int argc, const char **argv)
 {
 	const char *branch = NULL;
 	int full_clone = 0, single_branch = 0, show_progress = isatty(2);
+	int src = 1;
 	struct option clone_options[] = {
 		OPT_STRING('b', "branch", &branch, N_("<branch>"),
 			   N_("branch to checkout after clone")),

@@ -417,10 +418,13 @@ static int cmd_clone(int argc, const char **argv)
 		OPT_BOOL(0, "single-branch", &single_branch,
 			 N_("only download metadata for the branch that will "
 			    "be checked out")),
+		OPT_BOOL(0, "src", &src,
+			 N_("create repository within 'src' directory")),
 		OPT_END(),
 	};
 	const char * const clone_usage[] = {
-		N_("scalar clone [<options>] [--] <repo> [<dir>]"),
+		N_("scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]\n"
+		   "\t[--[no-]src] <url> [<enlistment>]"),
 		NULL
 	};
 	const char *url;

@@ -456,7 +460,10 @@ static int cmd_clone(int argc, const char **argv)
 	if (is_directory(enlistment))
 		die(_("directory '%s' exists already"), enlistment);
 
-	dir = xstrfmt("%s/src", enlistment);
+	if (src)
+		dir = xstrfmt("%s/src", enlistment);
+	else
+		dir = xstrdup(enlistment);
 
 	strbuf_reset(&buf);
 	if (branch)

index 872ad1c9c2b..7869f45ee64 100755
@@ -180,4 +180,16 @@ test_expect_success 'scalar clone warns when background maintenance fails' '
 	grep "could not turn on maintenance" err
 '
 
+test_expect_success '`scalar clone --no-src`' '
+	scalar clone --src "file://$(pwd)/to-clone" with-src &&
+	scalar clone --no-src "file://$(pwd)/to-clone" without-src &&
+
+	test_path_is_dir with-src/src &&
+	test_path_is_missing without-src/src &&
+
+	(cd with-src/src && ls ?*) >with &&
+	(cd without-src && ls ?*) >without &&
+	test_cmp with without
+'
+
 test_done

gitgitgadget

just nitpicking the commit message:

i'd use "this" here.

"artifacts" maybe.

i'd insert "new" here.

pedantically, there should be no dash here, as it's a regular verbal phrase. the dash should be used when it's turned into a noun, "to provide an opt-out". at least i think so ...

regards

From: Derrick Stolee <[email protected]>

There are many reasons why discovering a Git directory may fail. In particular, 8959555cee7 (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02) added ownership checks as a security precaution.

Callers attempting to set up a Git directory may want to inform the user about the reason for the failure. For that, expose the enum discovery_result from within setup.c and into cache.h where discover_git_directory() is defined.

I initially wanted to change the return type of discover_git_directory() to be this enum, but several callers rely upon the "zero means success". The two problems with this are:

1. The zero value of the enum is actually GIT_DIR_NONE, so nonpositive results are errors.

2. There are multiple successful states; positive results are successful.

It is worth noting that GIT_DIR_NONE is not returned, so we remove this option from the enum. We must be careful to keep the successful reasons as positive values, so they are given explicit positive values. Further, a use in scalar.c was previously impossible, so it is removed.

Instead of updating all callers immediately, add a new method, discover_git_directory_reason(), and convert discover_git_directory() to be a thin shim on top of it.

One thing that is important to note is that discover_git_directory() previously returned -1 on error, so let's continue that into the future. There is only one caller (in scalar.c) that depends on that signedness instead of a non-zero check, so clean that up, too.

Because there are extra checks that discover_git_directory_reason() does after setup_git_directory_gently_1(), there are other modes that can be returned for failure states. Add these modes to the enum, but be sure to explicitly add them as BUG() states in the switch of setup_git_directory_gently().

---
 scalar.c |  3 ---
 setup.c  | 34 ++++++++++++----------------------
 setup.h  | 35 ++++++++++++++++++++++++++++++++---
 3 files changed, 44 insertions(+), 28 deletions(-)

index 938bb73f3ce..02a38e845e1 100644
@@ -686,9 +686,6 @@ static int cmd_reconfigure(int argc, const char **argv)
 				warning(_("removing stale scalar.repo '%s'"),
 					dir);
 			strbuf_release(&buf);
-		} else if (discover_git_directory(&commondir, &gitdir) < 0) {
-			warning_errno(_("git repository gone in '%s'"), dir);
-			res = -1;
 		} else {
 			git_config_clear();
 

index 18927a847b8..2e607632dbd 100644
@@ -1221,19 +1221,6 @@ static const char *allowed_bare_repo_to_string(
 	return NULL;
 }
 
-enum discovery_result {
-	GIT_DIR_NONE = 0,
-	GIT_DIR_EXPLICIT,
-	GIT_DIR_DISCOVERED,
-	GIT_DIR_BARE,
-	/* these are errors */
-	GIT_DIR_HIT_CEILING = -1,
-	GIT_DIR_HIT_MOUNT_POINT = -2,
-	GIT_DIR_INVALID_GITFILE = -3,
-	GIT_DIR_INVALID_OWNERSHIP = -4,
-	GIT_DIR_DISALLOWED_BARE = -5,
-};
-
 /*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.

@@ -1385,21 +1372,23 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 	}
 }
 
-int discover_git_directory(struct strbuf *commondir,
-			   struct strbuf *gitdir)
+enum discovery_result discover_git_directory_reason(struct strbuf *commondir,
+						    struct strbuf *gitdir)
 {
 	struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
 	size_t gitdir_offset = gitdir->len, cwd_len;
 	size_t commondir_offset = commondir->len;
 	struct repository_format candidate = REPOSITORY_FORMAT_INIT;
+	enum discovery_result result;
 
 	if (strbuf_getcwd(&dir))
-		return -1;
+		return GIT_DIR_CWD_FAILURE;
 
 	cwd_len = dir.len;
-	if (setup_git_directory_gently_1(&dir, gitdir, NULL, 0) <= 0) {
+	result = setup_git_directory_gently_1(&dir, gitdir, NULL, 0);
+	if (result <= 0) {
 		strbuf_release(&dir);
-		return -1;
+		return result;
 	}
 
 	/*

@@ -1429,11 +1418,11 @@ int discover_git_directory(struct strbuf *commondir,
 		strbuf_setlen(commondir, commondir_offset);
 		strbuf_setlen(gitdir, gitdir_offset);
 		clear_repository_format(&candidate);
-		return -1;
+		return GIT_DIR_INVALID_FORMAT;
 	}
 
 	clear_repository_format(&candidate);
-	return 0;
+	return result;
 }
 
 const char *setup_git_directory_gently(int *nongit_ok)

@@ -1515,10 +1504,11 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		}
 		*nongit_ok = 1;
 		break;
-	case GIT_DIR_NONE:
+	case GIT_DIR_CWD_FAILURE:
+	case GIT_DIR_INVALID_FORMAT:
 		/*
 		 * As a safeguard against setup_git_directory_gently_1 returning
-		 * this value, fallthrough to BUG. Otherwise it is possible to
+		 * these values, fallthrough to BUG. Otherwise it is possible to
 		 * set startup_info->have_repository to 1 when we did nothing to
 		 * find a repository.
 		 */

index 58fd2605dd2..b48cf1c43b5 100644
@@ -42,16 +42,45 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);
 #define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)
 
 void setup_work_tree(void);
+
+/*
+ * discover_git_directory_reason() is similar to discover_git_directory(),
+ * except it returns an enum value instead. It is important to note that
+ * a zero-valued return here is actually GIT_DIR_NONE, which is different
+ * from discover_git_directory.
+ */
+enum discovery_result {
+	GIT_DIR_EXPLICIT = 1,
+	GIT_DIR_DISCOVERED = 2,
+	GIT_DIR_BARE = 3,
+	/* these are errors */
+	GIT_DIR_HIT_CEILING = -1,
+	GIT_DIR_HIT_MOUNT_POINT = -2,
+	GIT_DIR_INVALID_GITFILE = -3,
+	GIT_DIR_INVALID_OWNERSHIP = -4,
+	GIT_DIR_DISALLOWED_BARE = -5,
+	GIT_DIR_INVALID_FORMAT = -6,
+	GIT_DIR_CWD_FAILURE = -7,
+};
+enum discovery_result discover_git_directory_reason(struct strbuf *commondir,
+						    struct strbuf *gitdir);
+
 /*
  * Find the commondir and gitdir of the repository that contains the current
  * working directory, without changing the working directory or other global
  * state. The result is appended to commondir and gitdir.  If the discovered
  * gitdir does not correspond to a worktree, then 'commondir' and 'gitdir' will
  * both have the same result appended to the buffer.  The return value is
- * either 0 upon success and non-zero if no repository was found.
+ * either 0 upon success and -1 if no repository was found.
  */
-int discover_git_directory(struct strbuf *commondir,
-			   struct strbuf *gitdir);
+static inline int discover_git_directory(struct strbuf *commondir,
+					 struct strbuf *gitdir)
+{
+	if (discover_git_directory_reason(commondir, gitdir) <= 0)
+		return -1;
+	return 0;
+}
+
 const char *setup_git_directory_gently(int *);
 const char *setup_git_directory(void);
 char *prefix_path(const char *prefix, int len, const char *path);

gitgitgadget

(Relevant bit from the message.)

I know what happened here. At some point during my edits, this line was changed to "else if (!discover_git_directory(...))" which became an unreachable case.

So, based on the intermediate patch that did not survive, it made sense, but you are right that this hunk of this patch is behaving badly. Good eye!

Thanks,
-Stolee

"by moving it"

i have no clue wha this means. what is "it"?

is this really worth it, given that there are just two callers, and the adjustment is trivial?
if you insist, note that the function name can be easily misread as "discover_git_(directory_reason)", which is unhelpful. i typically use an _impl suffix in such "thin wrapper" scenarios.

regards

From: Derrick Stolee <[email protected]>

When running 'scalar reconfigure -a', Scalar has warning messages about the repository missing (or not containing a .git directory). Failures can also happen while trying to modify the repository-local config for that repository.

These warnings may seem confusing to users who don't understand what they mean or how to stop them.

Add a warning that instructs the user how to remove the warning in future installations.

---
 scalar.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 13 deletions(-)

index 02a38e845e1..54df0fdbb9f 100644
@@ -664,6 +664,7 @@ static int cmd_reconfigure(int argc, const char **argv)
 	git_config(get_scalar_repos, &scalar_repos);
 
 	for (i = 0; i < scalar_repos.nr; i++) {
+		int succeeded = 0;
 		const char *dir = scalar_repos.items[i].string;
 
 		strbuf_reset(&commondir);

@@ -674,27 +675,51 @@ static int cmd_reconfigure(int argc, const char **argv)
 
 			if (errno != ENOENT) {
 				warning_errno(_("could not switch to '%s'"), dir);
-				res = -1;
-				continue;
+				goto loop_end;
 			}
 
 			strbuf_addstr(&buf, dir);
 			if (remove_deleted_enlistment(&buf))
-				res = error(_("could not remove stale "
-					      "scalar.repo '%s'"), dir);
-			else
-				warning(_("removing stale scalar.repo '%s'"),
+				error(_("could not remove stale "
+					"scalar.repo '%s'"), dir);
+			else {
+				warning(_("removed stale scalar.repo '%s'"),
 					dir);
+				succeeded = 1;
+			}
 			strbuf_release(&buf);
-		} else {
-			git_config_clear();
+			goto loop_end;
+		}
+
+		switch (discover_git_directory_reason(&commondir, &gitdir)) {
+		case GIT_DIR_INVALID_OWNERSHIP:
+			warning(_("repository at '%s' has different owner"), dir);
+			goto loop_end;
+
+		case GIT_DIR_DISCOVERED:
+			succeeded = 1;
+			break;
+
+		default:
+			warning(_("repository not found in '%s'"), dir);
+			break;
+		}
+
+		git_config_clear();
+
+		the_repository = &r;
+		r.commondir = commondir.buf;
+		r.gitdir = gitdir.buf;
 
-			the_repository = &r;
-			r.commondir = commondir.buf;
-			r.gitdir = gitdir.buf;
+		if (set_recommended_config(1) >= 0)
+			succeeded = 1;
 
-			if (set_recommended_config(1) < 0)
-				res = -1;
+loop_end:
+		if (!succeeded) {
+			res = -1;
+			warning(_("to unregister this repository from Scalar, run\n"
+				  "\tgit config --global --unset --fixed-value scalar.repo \"%s\""),
+				dir);
 		}
 	}
 

gitgitgadget

The specific choice of GIT_DIR_INVALID_OWNERSHIP is singled out because it's a new-ish reason and is the most confusing to users when things fail for this reason.

These are basically "didn't find a Git repo" but there are different reasons why Git stopped looking. I'm not sure there is something more valuable to indicate here than the "repository not found" message that already exists.

I can add a message for this kind of error, which seems helpful to point out to a user.

I agree.

Thanks,
-Stolee

OK. I just know that "not found" will be greeted by "stupid Git, if you go one level up, there is a .git/ directory!", now we have many users than we used to have and people forget what they configured.

But I think it would apply much less to users who see "repository not found" in the "scalar reconfigure" than ones who manually created a repository, and then forgot that they shuffled the disks around with cross mounting. So I agree with you that it is not essential to mention these reasons in this codepath (and setup_git_directory() does have a reasonable message for at least the mount-point case that covers the more general case).

Maybe. We can do that in a follow-up topic separately.

Thanks.

While updating git-for-windows/git and microsoft/git for the 2.42.0 release window, a few patches that we've been running in those forks for a while came to light as something that would be beneficial to the core Git project. Here are some that are focused on the 'scalar' command.

* Patch 1 adds a --no-src option to scalar clone to appease users who want to use scalar but object to the creation of the src directory.
* Patches 2 and 3 help when scalar reconfigure -a fails. Patch 2 is a possibly helpful change on its own for other uses in the future.

Updates in V3
=============

* Several commit message edits.
* An important case that was dropped in v2's patch 2 is reintroduced (even though it is modified in patch 3).
* An error message is added for corrupt Git repositories.

Updates in V2
=============

Thanks, Junio, for the helpful review!

* In Patch 1, the '--[no-]src' documentation is tightened and the tests check the contents of the repository worktree.
* In Patch 2, the commit message is reworded to be more clear about positive values of the enum.
* In Patch 2, the GIT_DIR_NONE option of the enum is never returned, so it does not need to exist. A case in scalar.c referenced it, so it is removed as part of the patch (though that case was removed later by patch 3 anyway).
* In Patch 2, the discover_git_directory() wrapper is updated to return -1 instead of 1, as it did before this patch.
* In Patch 3, the 'failed' variable is renamed to 'succeeded' and the cases that update the value are swapped. The return code is set to -1 for any error instead of having a custom value based on the return from error() or error_errno().

Thanks, -Stolee

Derrick Stolee (3):
scalar: add --[no-]src option
setup: add discover_git_directory_reason() scalar reconfigure: help users remove buggy repos

 Documentation/scalar.txt |  8 ++++-
 scalar.c                 | 70 +++++++++++++++++++++++++++++-----------
 setup.c                  | 34 +++++++------------
 setup.h                  | 35 ++++++++++++++++++--
 t/t9211-scalar-clone.sh  | 12 +++++++
 5 files changed, 115 insertions(+), 44 deletions(-)

base-commit: a82fb66fed250e16d3010c75404503bea3f0ab61
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1569%2Fderrickstolee%2Fscalar-no-src-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1569/derrickstolee/scalar-no-src-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1569

Range-diff vs v2:

1: 0b6957beccb ! 1: e9858b31db6 scalar: add --[no-]src option
@@ Commit message
scalar: add --[no-]src option

Some users have strong aversions to Scalar's opinion that the repository
- should be in a 'src' directory, even though it creates a clean slate for
- placing build outputs in adjacent directories.
+ should be in a 'src' directory, even though this creates a clean slate
+ for placing build artifacts in adjacent directories.

- The --no-src option allows users to opt-out of the default behavior.
+ The new --no-src option allows users to opt out of the default behavior.

While adding options, make sure the usage output by 'scalar clone -h' reports the same as the SYNOPSIS line in Documentation/scalar.txt.
2: 787af0f9744 ! 2: 3c16fa6897f setup: add discover_git_directory_reason()
@@ Commit message

Callers attempting to set up a Git directory may want to inform the user about the reason for the failure. For that, expose the enum
- discovery_result from within setup.c and into cache.h where
+ discovery_result from within setup.c and move it into cache.h where
discover_git_directory() is defined.

I initially wanted to change the return type of discover_git_directory()
@@ Commit message
It is worth noting that GIT_DIR_NONE is not returned, so we remove this option from the enum. We must be careful to keep the successful reasons as positive values, so they are given explicit positive values.
- Further, a use in scalar.c was previously impossible, so it is removed.

Instead of updating all callers immediately, add a new method, discover_git_directory_reason(), and convert discover_git_directory() to
@@ Commit message

- ## scalar.c ##
-@@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)
- warning(_("removing stale scalar.repo '%s'"),
- dir);
- strbuf_release(&buf);
-- } else if (discover_git_directory(&commondir, &gitdir) < 0) {
-- warning_errno(_("git repository gone in '%s'"), dir);
-- res = -1;
- } else {
- git_config_clear();
-
-
## setup.c ##
@@ setup.c: static const char *allowed_bare_repo_to_string(
return NULL;
3: 7ac7311863d ! 3: ac234b15755 scalar reconfigure: help users remove buggy repos

     @@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)
      +				succeeded = 1;
      +			}
       			strbuf_release(&buf);
     +-		} else if (discover_git_directory(&commondir, &gitdir) < 0) {
     +-			warning_errno(_("git repository gone in '%s'"), dir);
     +-			res = -1;
      -		} else {
      -			git_config_clear();
      +			goto loop_end;
     @@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)
      +			warning(_("repository at '%s' has different owner"), dir);
      +			goto loop_end;
      +
     ++		case GIT_DIR_INVALID_GITFILE:
     ++		case GIT_DIR_INVALID_FORMAT:
     ++			warning(_("repository at '%s' has a format issue"), dir);
     ++			goto loop_end;
     ++
      +		case GIT_DIR_DISCOVERED:
      +			succeeded = 1;
      +			break;
     @@ scalar.c: static int cmd_reconfigure(int argc, const char **argv)
      +			warning(_("repository not found in '%s'"), dir);
      +			break;
      +		}
     -+
     -+		git_config_clear();
     -+
     -+		the_repository = &r;
     -+		r.commondir = commondir.buf;
     -+		r.gitdir = gitdir.buf;
       
      -			the_repository = &r;
      -			r.commondir = commondir.buf;
      -			r.gitdir = gitdir.buf;
     -+		if (set_recommended_config(1) >= 0)
     -+			succeeded = 1;
     ++		git_config_clear();
       
      -			if (set_recommended_config(1) < 0)
      -				res = -1;
     ++		the_repository = &r;
     ++		r.commondir = commondir.buf;
     ++		r.gitdir = gitdir.buf;
     ++
     ++		if (set_recommended_config(1) >= 0)
     ++			succeeded = 1;
     ++
      +loop_end:
      +		if (!succeeded) {
      +			res = -1;

gitgitgadget

From: Derrick Stolee <[email protected]>

Some users have strong aversions to Scalar's opinion that the repository should be in a 'src' directory, even though this creates a clean slate for placing build artifacts in adjacent directories.

The new --no-src option allows users to opt out of the default behavior.

While adding options, make sure the usage output by 'scalar clone -h' reports the same as the SYNOPSIS line in Documentation/scalar.txt.

---
 Documentation/scalar.txt |  8 +++++++-
 scalar.c                 | 11 +++++++++--
 t/t9211-scalar-clone.sh  | 12 ++++++++++++
 3 files changed, 28 insertions(+), 3 deletions(-)

index f33436c7f65..361f51a6473 100644
@@ -8,7 +8,8 @@ scalar - A tool for managing large Git repositories
 SYNOPSIS
 --------
 [verse]
-scalar clone [--single-branch] [--branch <main-branch>] [--full-clone] <url> [<enlistment>]
+scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]
+	[--[no-]src] <url> [<enlistment>]
 scalar list
 scalar register [<enlistment>]
 scalar unregister [<enlistment>]

@@ -80,6 +81,11 @@ remote-tracking branch for the branch this option was used for the initial
 cloning. If the HEAD at the remote did not point at any branch when
 `--single-branch` clone was made, no remote-tracking branch is created.
 
+--[no-]src::
+	By default, `scalar clone` places the cloned repository within a
+	`<entlistment>/src` directory. Use `--no-src` to place the cloned
+	repository directly in the `<enlistment>` directory.
+
 --[no-]full-clone::
 	A sparse-checkout is initialized by default. This behavior can be
 	turned off via `--full-clone`.

index df7358f481c..938bb73f3ce 100644
@@ -409,6 +409,7 @@ static int cmd_clone(int argc, const char **argv)
 {
 	const char *branch = NULL;
 	int full_clone = 0, single_branch = 0, show_progress = isatty(2);
+	int src = 1;
 	struct option clone_options[] = {
 		OPT_STRING('b', "branch", &branch, N_("<branch>"),
 			   N_("branch to checkout after clone")),

@@ -417,10 +418,13 @@ static int cmd_clone(int argc, const char **argv)
 		OPT_BOOL(0, "single-branch", &single_branch,
 			 N_("only download metadata for the branch that will "
 			    "be checked out")),
+		OPT_BOOL(0, "src", &src,
+			 N_("create repository within 'src' directory")),
 		OPT_END(),
 	};
 	const char * const clone_usage[] = {
-		N_("scalar clone [<options>] [--] <repo> [<dir>]"),
+		N_("scalar clone [--single-branch] [--branch <main-branch>] [--full-clone]\n"
+		   "\t[--[no-]src] <url> [<enlistment>]"),
 		NULL
 	};
 	const char *url;

@@ -456,7 +460,10 @@ static int cmd_clone(int argc, const char **argv)
 	if (is_directory(enlistment))
 		die(_("directory '%s' exists already"), enlistment);
 
-	dir = xstrfmt("%s/src", enlistment);
+	if (src)
+		dir = xstrfmt("%s/src", enlistment);
+	else
+		dir = xstrdup(enlistment);
 
 	strbuf_reset(&buf);
 	if (branch)

index 872ad1c9c2b..7869f45ee64 100755
@@ -180,4 +180,16 @@ test_expect_success 'scalar clone warns when background maintenance fails' '
 	grep "could not turn on maintenance" err
 '
 
+test_expect_success '`scalar clone --no-src`' '
+	scalar clone --src "file://$(pwd)/to-clone" with-src &&
+	scalar clone --no-src "file://$(pwd)/to-clone" without-src &&
+
+	test_path_is_dir with-src/src &&
+	test_path_is_missing without-src/src &&
+
+	(cd with-src/src && ls ?*) >with &&
+	(cd without-src && ls ?*) >without &&
+	test_cmp with without
+'
+
 test_done

gitgitgadget

From: Derrick Stolee <[email protected]>

There are many reasons why discovering a Git directory may fail. In particular, 8959555cee7 (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02) added ownership checks as a security precaution.

Callers attempting to set up a Git directory may want to inform the user about the reason for the failure. For that, expose the enum discovery_result from within setup.c and move it into cache.h where discover_git_directory() is defined.

I initially wanted to change the return type of discover_git_directory() to be this enum, but several callers rely upon the "zero means success". The two problems with this are:

1. The zero value of the enum is actually GIT_DIR_NONE, so nonpositive results are errors.

2. There are multiple successful states; positive results are successful.

It is worth noting that GIT_DIR_NONE is not returned, so we remove this option from the enum. We must be careful to keep the successful reasons as positive values, so they are given explicit positive values.

Instead of updating all callers immediately, add a new method, discover_git_directory_reason(), and convert discover_git_directory() to be a thin shim on top of it.

One thing that is important to note is that discover_git_directory() previously returned -1 on error, so let's continue that into the future. There is only one caller (in scalar.c) that depends on that signedness instead of a non-zero check, so clean that up, too.

Because there are extra checks that discover_git_directory_reason() does after setup_git_directory_gently_1(), there are other modes that can be returned for failure states. Add these modes to the enum, but be sure to explicitly add them as BUG() states in the switch of setup_git_directory_gently().

---
 setup.c | 34 ++++++++++++----------------------
 setup.h | 35 ++++++++++++++++++++++++++++++++---
 2 files changed, 44 insertions(+), 25 deletions(-)

index 18927a847b8..2e607632dbd 100644
@@ -1221,19 +1221,6 @@ static const char *allowed_bare_repo_to_string(
 	return NULL;
 }
 
-enum discovery_result {
-	GIT_DIR_NONE = 0,
-	GIT_DIR_EXPLICIT,
-	GIT_DIR_DISCOVERED,
-	GIT_DIR_BARE,
-	/* these are errors */
-	GIT_DIR_HIT_CEILING = -1,
-	GIT_DIR_HIT_MOUNT_POINT = -2,
-	GIT_DIR_INVALID_GITFILE = -3,
-	GIT_DIR_INVALID_OWNERSHIP = -4,
-	GIT_DIR_DISALLOWED_BARE = -5,
-};
-
 /*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.

@@ -1385,21 +1372,23 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 	}
 }
 
-int discover_git_directory(struct strbuf *commondir,
-			   struct strbuf *gitdir)
+enum discovery_result discover_git_directory_reason(struct strbuf *commondir,
+						    struct strbuf *gitdir)
 {
 	struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
 	size_t gitdir_offset = gitdir->len, cwd_len;
 	size_t commondir_offset = commondir->len;
 	struct repository_format candidate = REPOSITORY_FORMAT_INIT;
+	enum discovery_result result;
 
 	if (strbuf_getcwd(&dir))
-		return -1;
+		return GIT_DIR_CWD_FAILURE;
 
 	cwd_len = dir.len;
-	if (setup_git_directory_gently_1(&dir, gitdir, NULL, 0) <= 0) {
+	result = setup_git_directory_gently_1(&dir, gitdir, NULL, 0);
+	if (result <= 0) {
 		strbuf_release(&dir);
-		return -1;
+		return result;
 	}
 
 	/*

@@ -1429,11 +1418,11 @@ int discover_git_directory(struct strbuf *commondir,
 		strbuf_setlen(commondir, commondir_offset);
 		strbuf_setlen(gitdir, gitdir_offset);
 		clear_repository_format(&candidate);
-		return -1;
+		return GIT_DIR_INVALID_FORMAT;
 	}
 
 	clear_repository_format(&candidate);
-	return 0;
+	return result;
 }
 
 const char *setup_git_directory_gently(int *nongit_ok)

@@ -1515,10 +1504,11 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		}
 		*nongit_ok = 1;
 		break;
-	case GIT_DIR_NONE:
+	case GIT_DIR_CWD_FAILURE:
+	case GIT_DIR_INVALID_FORMAT:
 		/*
 		 * As a safeguard against setup_git_directory_gently_1 returning
-		 * this value, fallthrough to BUG. Otherwise it is possible to
+		 * these values, fallthrough to BUG. Otherwise it is possible to
 		 * set startup_info->have_repository to 1 when we did nothing to
 		 * find a repository.
 		 */

index 58fd2605dd2..b48cf1c43b5 100644
@@ -42,16 +42,45 @@ const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);
 #define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)
 
 void setup_work_tree(void);
+
+/*
+ * discover_git_directory_reason() is similar to discover_git_directory(),
+ * except it returns an enum value instead. It is important to note that
+ * a zero-valued return here is actually GIT_DIR_NONE, which is different
+ * from discover_git_directory.
+ */
+enum discovery_result {
+	GIT_DIR_EXPLICIT = 1,
+	GIT_DIR_DISCOVERED = 2,
+	GIT_DIR_BARE = 3,
+	/* these are errors */
+	GIT_DIR_HIT_CEILING = -1,
+	GIT_DIR_HIT_MOUNT_POINT = -2,
+	GIT_DIR_INVALID_GITFILE = -3,
+	GIT_DIR_INVALID_OWNERSHIP = -4,
+	GIT_DIR_DISALLOWED_BARE = -5,
+	GIT_DIR_INVALID_FORMAT = -6,
+	GIT_DIR_CWD_FAILURE = -7,
+};
+enum discovery_result discover_git_directory_reason(struct strbuf *commondir,
+						    struct strbuf *gitdir);
+
 /*
  * Find the commondir and gitdir of the repository that contains the current
  * working directory, without changing the working directory or other global
  * state. The result is appended to commondir and gitdir.  If the discovered
  * gitdir does not correspond to a worktree, then 'commondir' and 'gitdir' will
  * both have the same result appended to the buffer.  The return value is
- * either 0 upon success and non-zero if no repository was found.
+ * either 0 upon success and -1 if no repository was found.
  */
-int discover_git_directory(struct strbuf *commondir,
-			   struct strbuf *gitdir);
+static inline int discover_git_directory(struct strbuf *commondir,
+					 struct strbuf *gitdir)
+{
+	if (discover_git_directory_reason(commondir, gitdir) <= 0)
+		return -1;
+	return 0;
+}
+
 const char *setup_git_directory_gently(int *);
 const char *setup_git_directory(void);
 char *prefix_path(const char *prefix, int len, const char *path);

gitgitgadget

From: Derrick Stolee <[email protected]>

When running 'scalar reconfigure -a', Scalar has warning messages about the repository missing (or not containing a .git directory). Failures can also happen while trying to modify the repository-local config for that repository.

These warnings may seem confusing to users who don't understand what they mean or how to stop them.

Add a warning that instructs the user how to remove the warning in future installations.

---
 scalar.c | 59 +++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 16 deletions(-)

index 938bb73f3ce..fb2940c2a00 100644
@@ -664,6 +664,7 @@ static int cmd_reconfigure(int argc, const char **argv)
 	git_config(get_scalar_repos, &scalar_repos);
 
 	for (i = 0; i < scalar_repos.nr; i++) {
+		int succeeded = 0;
 		const char *dir = scalar_repos.items[i].string;
 
 		strbuf_reset(&commondir);

@@ -674,30 +675,56 @@ static int cmd_reconfigure(int argc, const char **argv)
 
 			if (errno != ENOENT) {
 				warning_errno(_("could not switch to '%s'"), dir);
-				res = -1;
-				continue;
+				goto loop_end;
 			}
 
 			strbuf_addstr(&buf, dir);
 			if (remove_deleted_enlistment(&buf))
-				res = error(_("could not remove stale "
-					      "scalar.repo '%s'"), dir);
-			else
-				warning(_("removing stale scalar.repo '%s'"),
+				error(_("could not remove stale "
+					"scalar.repo '%s'"), dir);
+			else {
+				warning(_("removed stale scalar.repo '%s'"),
 					dir);
+				succeeded = 1;
+			}
 			strbuf_release(&buf);
-		} else if (discover_git_directory(&commondir, &gitdir) < 0) {
-			warning_errno(_("git repository gone in '%s'"), dir);
-			res = -1;
-		} else {
-			git_config_clear();
+			goto loop_end;
+		}
+
+		switch (discover_git_directory_reason(&commondir, &gitdir)) {
+		case GIT_DIR_INVALID_OWNERSHIP:
+			warning(_("repository at '%s' has different owner"), dir);
+			goto loop_end;
+
+		case GIT_DIR_INVALID_GITFILE:
+		case GIT_DIR_INVALID_FORMAT:
+			warning(_("repository at '%s' has a format issue"), dir);
+			goto loop_end;
+
+		case GIT_DIR_DISCOVERED:
+			succeeded = 1;
+			break;
+
+		default:
+			warning(_("repository not found in '%s'"), dir);
+			break;
+		}
 
-			the_repository = &r;
-			r.commondir = commondir.buf;
-			r.gitdir = gitdir.buf;
+		git_config_clear();
 
-			if (set_recommended_config(1) < 0)
-				res = -1;
+		the_repository = &r;
+		r.commondir = commondir.buf;
+		r.gitdir = gitdir.buf;
+
+		if (set_recommended_config(1) >= 0)
+			succeeded = 1;
+
+loop_end:
+		if (!succeeded) {
+			res = -1;
+			warning(_("to unregister this repository from Scalar, run\n"
+				  "\tgit config --global --unset --fixed-value scalar.repo \"%s\""),
+				dir);
 		}
 	}
 

gitgitgadget

Thanks. The updated series look good. Especially the updates to the commit message all looked excellent.

Will queue. Let's merge it down to 'next'.