From: "brian m. carlson" <sandals@crustytoothpaste.net> To: Johannes Schindelin <Johannes.Schindelin@gmx.de> Cc: git@vger.kernel.org, "SZEDER Gábor" <szeder.dev@gmail.com> Subject: Re: [PATCH v2 2/2] rev-parse: add option for absolute or relative path formatting Date: Mon, 16 Nov 2020 02:15:00 +0000 [thread overview] Message-ID: <20201116021500.GA389879@camp.crustytoothpaste.net> (raw) In-Reply-To: <nycvar.QRO.7.76.6.2011091457300.18437@tvgsbejvaqbjf.bet> [-- Attachment #1: Type: text/plain, Size: 5418 bytes --] On 2020-11-09 at 14:46:13, Johannes Schindelin wrote: > On Fri, 9 Oct 2020, brian m. carlson wrote: > > 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. Technically, we can, but because there are cases in each one which don't make sense in the other, we end up with a situation that is hard to reason about in print_path, which is, by this point, already a little complex. So I think I'd prefer not to consolidate them. > > 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 don't think it does. I'll make sure to free it, though, since strbuf_reset doesn't do that. > > 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? It's documented to show the absolute path of the top of the repository, so it should be safe to do either one. Will switch. > > 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`? Same thing as above. Will change. > > @@ -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). I don't think that's what's happening here. I believe the intent is to insert a slash between the current working directory and the ".git" component if and only if the former lacks one. My code doesn't change that. -- brian m. carlson (he/him or they/them) Houston, Texas, US [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 263 bytes --]
next prev parent reply other threads:[~2020-11-16 2:15 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-09 19:15 [PATCH v2 0/2] rev-parse options for absolute or relative paths brian m. carlson 2020-10-09 19:15 ` [PATCH v2 1/2] abspath: add a function to resolve paths with missing components brian m. carlson 2020-10-09 21:10 ` Junio C Hamano 2020-10-10 1:10 ` brian m. carlson 2020-11-09 13:57 ` Johannes Schindelin 2020-11-09 13:55 ` Johannes Schindelin 2020-11-16 2:21 ` brian m. carlson 2020-10-09 19:15 ` [PATCH v2 2/2] rev-parse: add option for absolute or relative path formatting brian m. carlson 2020-11-09 14:46 ` Johannes Schindelin 2020-11-16 2:15 ` brian m. carlson [this message] 2020-11-04 23:01 ` [PATCH v2 0/2] rev-parse options for absolute or relative paths Emily Shaffer 2020-11-05 3:20 ` brian m. carlson 2020-11-09 13:33 ` Johannes Schindelin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201116021500.GA389879@camp.crustytoothpaste.net \ --to=sandals@crustytoothpaste.net \ --cc=Johannes.Schindelin@gmx.de \ --cc=git@vger.kernel.org \ --cc=szeder.dev@gmail.com \ --subject='Re: [PATCH v2 2/2] rev-parse: add option for absolute or relative path formatting' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).