Hi brian,
On Fri, 9 Oct 2020, brian m. carlson wrote:
> git rev-parse has several options which print various paths. Some of
> these paths are printed relative to the current working directory, and
> some are absolute.
>
> Normally, this is not a problem, but there are times when one wants
> paths entirely in one format or another. This can be done trivially if
> the paths are canonical, but canonicalizing paths is not possible on
> some shell scripting environments which lack realpath(1) and also in Go,
> which lacks functions that properly canonicalize paths on Windows.
>
> To help out the scripter, let's provide an option which turns most of
> the paths printed by git rev-parse to be either relative to the current
> working directory or absolute and canonical. Document which options are
> affected and which are not so that users are not confused.
>
> This approach is cleaner and tidier than providing duplicates of
> existing options which are either relative or absolute.
>
> Note that if the user needs both forms, it is possible to pass an
> additional option in the middle of the command line which changes the
> behavior of subsequent operations.
Good.
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index ed200c8af1..ec62b4cd16 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -583,6 +583,76 @@ static void handle_ref_opt(const char *pattern, const char *prefix)
> clear_ref_exclusion(&ref_excludes);
> }
>
> +enum format_type {
> + /* We would like a relative path. */
> + FORMAT_RELATIVE,
> + /* We would like a canonical absolute path. */
> + FORMAT_CANONICAL,
> + /* We would like the default behavior. */
> + FORMAT_DEFAULT,
> +};
> +
> +enum default_type {
> + /* Our default is a relative path. */
> + DEFAULT_RELATIVE,
> + /* Our default is a relative path if there's a shared root. */
> + DEFAULT_RELATIVE_IF_SHARED,
> + /* Our default is a canonical absolute path. */
> + DEFAULT_CANONICAL,
> + /* Our default is not to modify the item. */
> + DEFAULT_UNMODIFIED,
> +};
I wonder whether it would make sense to consolidate these two enums into a
single one.
> +static void print_path(const char *path, const char *prefix, enum format_type format, enum default_type def)
> +{
> + char *cwd = NULL;
> + /*
> + * We don't ever produce a relative path if prefix is NULL, so set the
> + * prefix to the current directory so that we can produce a relative
> + * path whenever possible. If we're using RELATIVE_IF_SHARED mode, then
> + * we want an absolute path unless the two share a common prefix, so don't
> + * set it in that case, since doing so causes a relative path to always
> + * be produced if possible.
> + */
> + if (!prefix && (format != FORMAT_DEFAULT || def != DEFAULT_RELATIVE_IF_SHARED))
> + prefix = cwd = xgetcwd();
> + if (format == FORMAT_DEFAULT && def == DEFAULT_UNMODIFIED) {
> + puts(path);
> + } else if (format == FORMAT_RELATIVE ||
> + (format == FORMAT_DEFAULT && def == DEFAULT_RELATIVE)) {
> + /*
> + * In order for relative_path to work as expected, we need to
> + * make sure that both paths are absolute paths. If we don't,
> + * we can end up with an unexpected absolute path that the user
> + * didn't want.
> + */
> + struct strbuf buf = STRBUF_INIT, realbuf = STRBUF_INIT, prefixbuf = STRBUF_INIT;
> + if (!is_absolute_path(path)) {
> + if (!strbuf_realpath_missing(&realbuf, path))
> + die(_("Unable to resolve path '%s'"), path);
> + path = realbuf.buf;
> + }
> + if (!is_absolute_path(prefix)) {
> + if (!strbuf_realpath_missing(&prefixbuf, prefix))
> + die(_("Unable to resolve path '%s'"), path);
> + prefix = prefixbuf.buf;
> + }
> + puts(relative_path(path, prefix, &buf));
> + strbuf_release(&buf);
> + } else if (format == FORMAT_DEFAULT && def == DEFAULT_RELATIVE_IF_SHARED) {
> + struct strbuf buf = STRBUF_INIT;
> + puts(relative_path(path, prefix, &buf));
> + strbuf_release(&buf);
> + } else {
> + struct strbuf buf = STRBUF_INIT;
> + if (!strbuf_realpath_missing(&buf, path))
> + die(_("Unable to resolve path '%s'"), path);
> + puts(buf.buf);
> + strbuf_release(&buf);
> + }
> + free(cwd);
> +}
> +
> int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> {
> int i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;
> @@ -595,6 +665,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> struct object_context unused;
> struct strbuf buf = STRBUF_INIT;
> const int hexsz = the_hash_algo->hexsz;
> + enum format_type format = FORMAT_DEFAULT;
>
> if (argc > 1 && !strcmp("--parseopt", argv[1]))
> return cmd_parseopt(argc - 1, argv + 1, prefix);
> @@ -650,8 +721,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> if (!argv[i + 1])
> die("--git-path requires an argument");
> strbuf_reset(&buf);
> - puts(relative_path(git_path("%s", argv[i + 1]),
> - prefix, &buf));
> + print_path(git_path("%s", argv[i + 1]), prefix, format, DEFAULT_RELATIVE_IF_SHARED);
One thing that the original code did was to reuse the same `strbuf`. Not
sure whether this matters in practice.
> i++;
> continue;
> }
> @@ -683,6 +753,16 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> show_file(arg, 0);
> continue;
> }
> + if (opt_with_value(arg, "--path-format", &arg)) {
> + if (!strcmp(arg, "absolute")) {
> + format = FORMAT_CANONICAL;
> + } else if (!strcmp(arg, "relative")) {
> + format = FORMAT_RELATIVE;
> + } else {
> + die("unknown argument to --path-format: %s", arg);
> + }
> + continue;
> + }
> if (!strcmp(arg, "--default")) {
> def = argv[++i];
> if (!def)
> @@ -803,7 +883,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> if (!strcmp(arg, "--show-toplevel")) {
> const char *work_tree = get_git_work_tree();
> if (work_tree)
> - puts(work_tree);
> + print_path(work_tree, prefix, format, DEFAULT_CANONICAL);
The way `print_path()`'s code is structured, it is not immediately obvious
to me whether the patch changes behavior here. I _suspect_ that we're now
calling `strbuf_realpath_missing()` and react to its return value, which
is different from before.
Wouldn't make `DEFAULT_UNMODIFIED` make more sense here?
> else
> die("this operation must be run in a work tree");
> continue;
> @@ -811,7 +891,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> if (!strcmp(arg, "--show-superproject-working-tree")) {
> struct strbuf superproject = STRBUF_INIT;
> if (get_superproject_working_tree(&superproject))
> - puts(superproject.buf);
> + print_path(superproject.buf, prefix, format, DEFAULT_CANONICAL);
Shouldn't this be `DEFAULT_UNMODIFIED`?
> strbuf_release(&superproject);
> continue;
> }
> @@ -846,16 +926,18 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
> char *cwd;
> int len;
> + enum format_type wanted = format;
> if (arg[2] == 'g') { /* --git-dir */
> if (gitdir) {
> - puts(gitdir);
> + print_path(gitdir, prefix, format, DEFAULT_UNMODIFIED);
> continue;
> }
> if (!prefix) {
> - puts(".git");
> + print_path(".git", prefix, format, DEFAULT_UNMODIFIED);
> continue;
> }
> } else { /* --absolute-git-dir */
> + wanted = FORMAT_CANONICAL;
> if (!gitdir && !prefix)
> gitdir = ".git";
> if (gitdir) {
> @@ -868,14 +950,14 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> }
> cwd = xgetcwd();
> len = strlen(cwd);
> - printf("%s%s.git\n", cwd, len && cwd[len-1] != '/' ? "/" : "");
> + strbuf_reset(&buf);
> + strbuf_addf(&buf, "%s%s.git", cwd, len && cwd[len-1] != '/' ? "/" : "");
So `DEFAULT_CANONICAL` ensures a trailing `/`? I do not see that in
`print_path()`'s implementation, and also, I would love to see a different
name for that ("canonical", from my Java past, suggests something like
"real path" to me).
Thanks,
Dscho
> free(cwd);
> + print_path(buf.buf, prefix, wanted, DEFAULT_CANONICAL);
> continue;
> }
> if (!strcmp(arg, "--git-common-dir")) {
> - strbuf_reset(&buf);
> - puts(relative_path(get_git_common_dir(),
> - prefix, &buf));
> + print_path(get_git_common_dir(), prefix, format, DEFAULT_RELATIVE_IF_SHARED);
> continue;
> }
> if (!strcmp(arg, "--is-inside-git-dir")) {
> @@ -905,8 +987,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> if (the_index.split_index) {
> const struct object_id *oid = &the_index.split_index->base_oid;
> const char *path = git_path("sharedindex.%s", oid_to_hex(oid));
> - strbuf_reset(&buf);
> - puts(relative_path(path, prefix, &buf));
> + print_path(path, prefix, format, DEFAULT_RELATIVE);
> }
> continue;
> }
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index 408b97d5af..51d7d40ec1 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -3,6 +3,16 @@
> test_description='test git rev-parse'
> . ./test-lib.sh
>
> +test_one () {
> + dir="$1" &&
> + expect="$2" &&
> + shift &&
> + shift &&
> + echo "$expect" >expect &&
> + git -C "$dir" rev-parse "$@" >actual &&
> + test_cmp expect actual
> +}
> +
> # usage: [options] label is-bare is-inside-git is-inside-work prefix git-dir absolute-git-dir
> test_rev_parse () {
> d=
> @@ -60,7 +70,13 @@ ROOT=$(pwd)
>
> test_expect_success 'setup' '
> mkdir -p sub/dir work &&
> - cp -R .git repo.git
> + cp -R .git repo.git &&
> + git checkout -B main &&
> + test_commit abc &&
> + git checkout -b side &&
> + test_commit def &&
> + git checkout main &&
> + git worktree add worktree side
> '
>
> test_rev_parse toplevel false false true '' .git "$ROOT/.git"
> @@ -88,6 +104,45 @@ test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = tru
>
> test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
>
> +test_expect_success 'rev-parse --path-format=absolute' '
> + test_one "." "$ROOT/.git" --path-format=absolute --git-dir &&
> + test_one "." "$ROOT/.git" --path-format=absolute --git-common-dir &&
> + test_one "sub/dir" "$ROOT/.git" --path-format=absolute --git-dir &&
> + test_one "sub/dir" "$ROOT/.git" --path-format=absolute --git-common-dir &&
> + test_one "worktree" "$ROOT/.git/worktrees/worktree" --path-format=absolute --git-dir &&
> + test_one "worktree" "$ROOT/.git" --path-format=absolute --git-common-dir &&
> + test_one "." "$ROOT" --path-format=absolute --show-toplevel &&
> + test_one "." "$ROOT/.git/objects" --path-format=absolute --git-path objects &&
> + test_one "." "$ROOT/.git/objects/foo/bar/baz" --path-format=absolute --git-path objects/foo/bar/baz
> +'
> +
> +test_expect_success 'rev-parse --path-format=relative' '
> + test_one "." ".git" --path-format=relative --git-dir &&
> + test_one "." ".git" --path-format=relative --git-common-dir &&
> + test_one "sub/dir" "../../.git" --path-format=relative --git-dir &&
> + test_one "sub/dir" "../../.git" --path-format=relative --git-common-dir &&
> + test_one "worktree" "../.git/worktrees/worktree" --path-format=relative --git-dir &&
> + test_one "worktree" "../.git" --path-format=relative --git-common-dir &&
> + test_one "." "./" --path-format=relative --show-toplevel &&
> + test_one "." ".git/objects" --path-format=relative --git-path objects &&
> + test_one "." ".git/objects/foo/bar/baz" --path-format=relative --git-path objects/foo/bar/baz
> +'
> +
> +test_expect_success '--path-format=relative does not affect --absolute-git-dir' '
> + git rev-parse --path-format=relative --absolute-git-dir >actual &&
> + echo "$ROOT/.git" >expect &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success '--path-format can change in the middle of the command line' '
> + git rev-parse --path-format=absolute --git-dir --path-format=relative --git-path objects/foo/bar >actual &&
> + cat >expect <<-EOF &&
> + $ROOT/.git
> + .git/objects/foo/bar
> + EOF
> + test_cmp expect actual
> +'
> +
> test_expect_success 'git-common-dir from worktree root' '
> echo .git >expect &&
> git rev-parse --git-common-dir >actual &&
>
