Re: [PATCH v2 2/2] rev-parse: add option for absolute or relative path formatting
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- Nov 16, 2020, 02:15 UTC
- Message-ID
- <20201116021500.GA389879@camp.crustytoothpaste.net>
- In-Reply-To
- <nycvar.QRO.7.76.6.2011091457300.18437@tvgsbejvaqbjf.bet>
On 2020-11-09 at 14:46:13, Johannes Schindelin wrote:
Show 31 quoted lines
> 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.
Show 21 quoted lines
> > 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.
Show 33 quoted lines
> > 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.
Show 11 quoted lines
> > 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.
Show 12 quoted lines
> > @@ -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