repo: add paths.toplevel to repo info - #2264

"Jayesh Daga via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jayesh Daga <jayeshdaga99@gmail.com>
>
> repo info currently does not expose the repository's
> working tree root, even though this information is
> available via `repo_get_work_tree()` and
> `git rev-parse --show-toplevel`.
>
> Add a new field `paths.toplevel` to expose this value.
>
> While doing so, document the correspondence between
> `git rev-parse` options and `repo info` fields to make
> it easier to identify missing or future additions.
>
> For bare repositories, this value is empty, consistent
> with other non-applicable fields.
>

Don't we still have to decide how we want to support relative vs
absolute paths? [1]

Also seeing that you're a GSoC candidate and this is part of the project
that you've (and other potential contributors) applied for, our
recommendation is to not start working on a project before the selection
process.

[snip]

[1]: https://lore.kernel.org/git/CA+rGoLfbzXqP1Tw+94jMmWcSGPoefMv5E_fvwriad-O5CUeKHQ@mail.gmail.com/T/#m1e2b69b6b097c42b06088349947e43831cf5988f

"Jayesh Daga via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jayesh Daga <jayeshdaga99@gmail.com>
>
> repo info currently does not expose the repository's
> working tree root, even though this information is
> available via `repo_get_work_tree()` and
> `git rev-parse --show-toplevel`.
>
> Add a new field `paths.toplevel` to expose this value.
>
> While doing so, document the correspondence between
> `git rev-parse` options and `repo info` fields to make
> it easier to identify missing or future additions.
>
> For bare repositories, this value is empty, consistent
> with other non-applicable fields.
>
> Signed-off-by: Jayesh Daga jayeshdaga99@gmail.com
> ---

So "document the correspondence" added to the v2 iteration is still
up there.  I expect to see such a change in the patch body.

>     repo: add paths.toplevel to repo info
>     
>     repo info currently does not expose the repository's working tree root,
>     even though this information is available via repo_get_work_tree().
>     
>     This makes it harder for scripts to retrieve the repository root through
>     a structured interface, often requiring the use of git rev-parse
>     --show-toplevel.
>     
>     Add a new field paths.toplevel to git repo info that returns the working
>     tree root. For bare repositories, this value is empty, consistent with
>     other non-applicable fields.
>     
>     This provides a consistent and script-friendly way to query repository
>     paths without invoking additional commands.
>     
>     Signed-off-by: Jayesh Daga jayeshdaga99@gmail.com

This space below the three-dash is not where reiterate what you
already wrote in the commit log message.  Rather, this is a place to
give additional explanation that should not go in the commit log
message, like "the previous iteration botched X and Y which have
been corrected in this version", etc.

I see this mistake often in patches from GGG users---please double
check how to use that tool (I am not a GGG user, but I vaguely
recall that GGG duplicates what you wrote in the pull request
message after the three-dashes, so perhaps you are expected to
rewrite the pull request message every time to summarize the
differences since the last iteration before you submit a new
iteration, or something?).

> Range-diff vs v2:
>
>  1:  05e34bfe2c ! 1:  8a58b6ce03 repo: add paths.toplevel to repo info
>      @@ Commit message
>           For bare repositories, this value is empty, consistent
>           with other non-applicable fields.
>       
>      -    Signed-off-by: Jayesh Daga [jayeshdaga99@gmail.com](mailto:jayeshdaga99@gmail.com)
>      +    Signed-off-by: Jayesh Daga jayeshdaga99@gmail.com

OK.

>      -@@ builtin/repo.c: static const struct repo_info_field repo_info_field[] = {
>      +@@ builtin/repo.c: static int get_references_format(struct repository *repo, struct strbuf *buf)
>      + 	return 0;
>      + }
>      + 
>      ++/*
>      ++ * rev-parse --<opt>        repo info <field>
>      ++ *
>      ++ * is-bare-repository      layout.bare
>      ++ * is-shallow-repository   layout.shallow
>      ++ * show-object-format      object.format
>      ++ * show-ref-format         references.format
>      ++ * show-toplevel           paths.toplevel
>      ++ *
>      ++ * show-cdup               <missing>
>      ++ * show-prefix             <missing>
>      ++ *
>      ++ * Some <missing> entries may be candidates for future
>      ++ * implementation, while others may not be needed.

It is a surprisingly small set.  Did you do your own research, or
did you only copied what I gave as examples?

> +static int get_paths_toplevel(struct repository *repo, struct strbuf *buf)
> +{
> +	const char *wt = repo_get_work_tree(repo);
> +
> +	if (!wt)
> +		return -1; /* match existing error style */

That is not a very helpful comment.  It sounds more like "I do not
understand why I am supposed to return -1 here upon error, but I am
just blindly following what everybody else does" excuse, than "I
return -1 upon an error because the caller is prepared to act on it
and do X, which is what we want to see when repo_get_work_tree(repo)
returns NULL, which signals condition Y that should be reported as
such" explanation.  If the reason why returning -1 is so obvious
that we do not even have to say it (which I suspect is the case),
then future readers would appreciate if you didn't add such a
comment.  If the reason why this needs to return -1 (and not -2 or
-999 or whatever) is so subtle and needs to be explained, then the
comment does not give even a single bit of information to help them
understand why.

> +	strbuf_addstr(buf, wt);
> +	return 0;
> +}

> diff --git a/t/t1900-repo-info.sh b/t/t1900-repo-info.sh
> index 39bb77dda0..470e06e8c2 100755
> --- a/t/t1900-repo-info.sh
> +++ b/t/t1900-repo-info.sh
> @@ -155,4 +155,20 @@ test_expect_success 'git repo info -h shows only repo info usage' '
>  	test_grep ! "git repo structure" actual
>  '
>  
> +test_expect_success 'repo info paths.toplevel' '
> +    git repo info paths.toplevel >actual &&
> +    echo "paths.toplevel=$(git rev-parse --show-toplevel)" >expected &&

These seem to be SP*4 indented; one level of indent is supposed to
be a single tab, not 4-column.

You may have copied it from other tests, but the above is
done in an unusual order and I found it harder to read.

We prepare an expected output in a file called "expect" first, and
then store the output of the command we are testing in a file called
"actual", and then compare "test_cmp expect actual", in this order.

If you deliberately misspell the command to pass "--shaw-toplabel",
does it break this "echo" invocation, or will we only notice the
breakage/mistake when we check the output in the "expect" file
against 

> +    test_cmp expected actual
> +'
> +
> +test_expect_success 'repo info paths.toplevel (bare repo)' '
> +    git init --bare bare.git &&
> +    (
> +	cd bare.git &&
> +	git repo info paths.toplevel >actual &&
> +	echo "paths.toplevel=" >expected &&
> +	test_cmp expected actual
> +    )
> +'
> +
>  test_done
>
> base-commit: 256554692df0685b45e60778b08802b720880c50

Karthik Nayak <karthik.188@gmail.com> writes:

> Don't we still have to decide how we want to support relative vs
> absolute paths? [1]

I suspect more than just a few requests to this command yield
path-valued response, so do we want an independent boolean "if we
are showing a path, show it in relative (yes) or absolute (no)"
option, or something?

> Also seeing that you're a GSoC candidate and this is part of the project
> that you've (and other potential contributors) applied for, our
> recommendation is to not start working on a project before the selection
> process.

Perhaps I should refrain from commenting on these patches to
discourage the authors if that is the case.  I am not raising an
objection, but do you have a pointer to the rationale behind the
recommendation?

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Don't we still have to decide how we want to support relative vs
>> absolute paths? [1]
>
> I suspect more than just a few requests to this command yield
> path-valued response, so do we want an independent boolean "if we
> are showing a path, show it in relative (yes) or absolute (no)"
> option, or something?
>

I think there was some ideas around this:

  - Have a default mode and a way to swap it. Perhaps a boolean config
    on the command level
  - Output both the relative and absolute paths. So we'd have
    paths.toplevel.absolute
    paths.toplevel.relative
    or something like that.

Either ways, I think we should finalize the decision first, that way we
don't have to have this discussion with each path-valued option.

>> Also seeing that you're a GSoC candidate and this is part of the project
>> that you've (and other potential contributors) applied for, our
>> recommendation is to not start working on a project before the selection
>> process.
>
> Perhaps I should refrain from commenting on these patches to
> discourage the authors if that is the case.  I am not raising an
> objection, but do you have a pointer to the rationale behind the
> recommendation?

Officially the GSoC rules [1] only say:

  Any work done on the Project prior to acceptance of the Project Proposal
  will not be considered for Evaluations.

So there is no rule stopping a contributor from making such changes, and
ultimately, open source projects benefit from contributions regardless
of the contributor's application status.

However it gets confusing for other contributors who have proposals for
the same topic, since they expect a certain status quo and a moving goal
post makes it a lot harder.

[1]: https://summerofcode.withgoogle.com/rules