Re: [PATCH v3] repo: add paths.toplevel to repo info

[Junio C Hamano <gitster@pobox.com>]

"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