[PATCH 0/4] git-prompt.sh: Full patch for submodule indicator

[PATCH 0/4] git-prompt.sh: Full patch for submodule indicator

[Benjamin Fuchs]

Hi everyone,
since I didn't get a response I decided to sent my patch again. Maybe it was because
I to sent my consecutive commits the wrong way, so a new try.
First thanks again Steffen and Gábor for your feedback.
Based on the first feedback I rework the indicator and it is now way cheaper then the
first version and has a 'dirty' indicator now.
Tests exist also now.
Looking forward to more feedback!
Greetings,
Benjamin

Benjamin Fuchs (4):
  git-prompt.sh: add submodule indicator
  git-prompt.sh: rework of submodule indicator
  git-prompt.sh: fix for submodule 'dirty' indicator
  git-prompt.sh: add tests for submodule indicator

 contrib/completion/git-prompt.sh | 36 ++++++++++++++++++++++++++++++++-
 t/t9903-bash-prompt.sh           | 43 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH 1/4] git-prompt.sh: add submodule indicator

[Benjamin Fuchs]

I expirienced that working with submodules can be confusing. This indicator
will make you notice very easy when you switch into a submodule.
The new prompt will look like this: (sub:master)
Adding a new optional env variable for the new feature.

Signed-off-by: Benjamin Fuchs <email@benjaminfuchs.de>
---
 contrib/completion/git-prompt.sh | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 97eacd7..4c82e7f 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -93,6 +93,10 @@
 # directory is set up to be ignored by git, then set
 # GIT_PS1_HIDE_IF_PWD_IGNORED to a nonempty value. Override this on the
 # repository level by setting bash.hideIfPwdIgnored to "false".
+#
+# If you would like __git_ps1 to indicate that you are in a submodule,
+# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before
+# the branch name.
 
 # check whether printf supports -v
 __git_printf_supports_v=
@@ -284,6 +288,32 @@ __git_eread ()
 	test -r "$f" && read "$@" <"$f"
 }
 
+# __git_is_submodule
+# Based on:
+# http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule
+__git_is_submodule ()
+{
+	local git_dir parent_git module_name path
+	# Find the root of this git repo, then check if its parent dir is also a repo
+	git_dir="$(git rev-parse --show-toplevel)"
+	module_name="$(basename "$git_dir")"
+	parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> /dev/null)"
+	if [[ -n $parent_git ]]; then
+		# List all the submodule paths for the parent repo
+		while read path
+		do
+			if [[ "$path" != "$module_name" ]]; then continue; fi
+			if [[ -d "$git_dir/../$path" ]];    then return 0; fi
+		done < <(cd $parent_git && git submodule --quiet foreach 'echo $path' 2> /dev/null)
+    fi
+    return 1
+}
+
+__git_ps1_submodule ()
+{
+	__git_is_submodule && printf "sub:"
+}
+
 # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
 # when called from PS1 using command substitution
 # in this mode it prints text to add to bash PS1 prompt (includes branch name)
@@ -513,8 +543,13 @@ __git_ps1 ()
 		b="\${__git_ps1_branch_name}"
 	fi
 
+	local sub=""
+	if [ -n "${GIT_PS1_SHOWSUBMODULE}" ]; then
+		sub="$(__git_ps1_submodule)"
+	fi
+
 	local f="$w$i$s$u"
-	local gitstring="$c$b${f:+$z$f}$r$p"
+	local gitstring="$c$sub$b${f:+$z$f}$r$p"
 
 	if [ $pcmode = yes ]; then
 		if [ "${__git_printf_supports_v-}" != yes ]; then
-- 
2.7.4

Re: [PATCH 1/4] git-prompt.sh: add submodule indicator

[Junio C Hamano]

Benjamin Fuchs <email@benjaminfuchs.de> writes:

> I expirienced that working with submodules can be confusing. This indicator
> will make you notice very easy when you switch into a submodule.

I am not quite sure what the above wants to say.  If you work on two
projects, A and B, then having something that quickly reminds you
which one you are in is beneficial and people often do so by having
the current directory name in the prompt.  

The log message needs to explain why working on a submodule C of a
project A is any more special, i.e. why are the existing ways the
users are using to remind them between A and B cannot be used for C.

> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 97eacd7..4c82e7f 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -93,6 +93,10 @@
>  # directory is set up to be ignored by git, then set
>  # GIT_PS1_HIDE_IF_PWD_IGNORED to a nonempty value. Override this on the
>  # repository level by setting bash.hideIfPwdIgnored to "false".
> +#
> +# If you would like __git_ps1 to indicate that you are in a submodule,
> +# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before
> +# the branch name.
>  
>  # check whether printf supports -v
>  __git_printf_supports_v=
> @@ -284,6 +288,32 @@ __git_eread ()
>  	test -r "$f" && read "$@" <"$f"
>  }
>  
> +# __git_is_submodule
> +# Based on:
> +# http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule
> +__git_is_submodule ()

It seems like this function is checking if you are "IN" submodule,
not "IS" submodule.  A misnamed function?

> +{
> +	local git_dir parent_git module_name path
> +	# Find the root of this git repo, then check if its parent dir is also a repo
> +	git_dir="$(git rev-parse --show-toplevel)"
> +	module_name="$(basename "$git_dir")"

This does not give "module_name" in the sense the word is used in
the submodule subsystem.  If this variable is useful, call that
with "path" in its name (I do not think it alone is useful at all).

> +	parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> /dev/null)"
> +	if [[ -n $parent_git ]]; then
> +		# List all the submodule paths for the parent repo
> +		while read path
> +		do
> +			if [[ "$path" != "$module_name" ]]; then continue; fi
> +			if [[ -d "$git_dir/../$path" ]];    then return 0; fi
> +		done < <(cd $parent_git && git submodule --quiet foreach 'echo $path' 2> /dev/null)

Instead of doing this "loop over submodules and just get true or
false", it may make a lot more sense to find the module name and
report that.  That would allow you to have the actual submodule
name, not a generic "sub:" which does not help the users to tell
which one of 47 submodules they have in their top-level project
they are currently in.

If your projects are layed out like so

	/home/bf/projects/A/
	/home/bf/projects/A/lib/B/ -- submodule
	/home/bf/projects/A/Doc/ -- another submodule
	/home/bf/projects/A/Doc/technical -- a subdirectory of Doc submodule

and you are in /home/bf/projects/A/Doc/technical/ subdirectory, your
$git_dir variable (which is grossly misnamed, too; call it "top"
perhaps?) would be "/home/bf/projects/A/Doc" and then your
$parent_git variable (again, that is misnamed; call it
"parent_top"?) would be "/home/bf/projects/A".  The submodule path
to the submodule you are currently in is "Doc" (you learn it as the
difference between these two).

You can ask the top-level project the name of the submodule that is
currently at "Doc" with "submodule--helper name".  Unless the project
has moved it since it first added the submodule, the module name may
also be "Doc", but if it were moved, it may be different.

And that "module name" is a more useful thing than a hardcoded
string "sub:" to use in the prompt, I would think.

Re: [PATCH 1/4] git-prompt.sh: add submodule indicator

[Benjamin Fuchs]

Hi Junio,
thanks for your reply. Some of your suggestions are covered by my rework 
in Patch [2/4].
In [2/4] I got rid of the loop by feedback of Gábor.
Sorry if my patch wasn't well formed.

On 31.01.2017 00:48, Junio C Hamano wrote:
> Benjamin Fuchs <email@benjaminfuchs.de> writes:
>
>> I expirienced that working with submodules can be confusing. This indicator
>> will make you notice very easy when you switch into a submodule.
> I am not quite sure what the above wants to say.  If you work on two
> projects, A and B, then having something that quickly reminds you
> which one you are in is beneficial and people often do so by having
> the current directory name in the prompt.
>
> The log message needs to explain why working on a submodule C of a
> project A is any more special, i.e. why are the existing ways the
> users are using to remind them between A and B cannot be used for C.
A submodule can be anywhere in your parent git repository. And while 
walking through the
parent repository it is sometime a good reminder to know when to just 
entered a submodule.
Because now all git command will work on the submodule. I guess this 
indicator is a good way
to show that.
And with patch [2/4] (and fixed it with [3/4]) I also introduced the 
"dirty" indicator for the submodule, which can
show you, that your current checked out state in the submodule is not 
the one committed
to the parent.

>
>> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
>> index 97eacd7..4c82e7f 100644
>> --- a/contrib/completion/git-prompt.sh
>> +++ b/contrib/completion/git-prompt.sh
>> @@ -93,6 +93,10 @@
>>   # directory is set up to be ignored by git, then set
>>   # GIT_PS1_HIDE_IF_PWD_IGNORED to a nonempty value. Override this on the
>>   # repository level by setting bash.hideIfPwdIgnored to "false".
>> +#
>> +# If you would like __git_ps1 to indicate that you are in a submodule,
>> +# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before
>> +# the branch name.
>>   
>>   # check whether printf supports -v
>>   __git_printf_supports_v=
>> @@ -284,6 +288,32 @@ __git_eread ()
>>   	test -r "$f" && read "$@" <"$f"
>>   }
>>   
>> +# __git_is_submodule
>> +# Based on:
>> +# http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule
>> +__git_is_submodule ()
> It seems like this function is checking if you are "IN" submodule,
> not "IS" submodule.  A misnamed function?
Reworked in Patch [2/4]
>> +{
>> +	local git_dir parent_git module_name path
>> +	# Find the root of this git repo, then check if its parent dir is also a repo
>> +	git_dir="$(git rev-parse --show-toplevel)"
>> +	module_name="$(basename "$git_dir")"
> This does not give "module_name" in the sense the word is used in
> the submodule subsystem.  If this variable is useful, call that
> with "path" in its name (I do not think it alone is useful at all).
Reworked in Patch [2/4]
>
>> +	parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> /dev/null)"
>> +	if [[ -n $parent_git ]]; then
>> +		# List all the submodule paths for the parent repo
>> +		while read path
>> +		do
>> +			if [[ "$path" != "$module_name" ]]; then continue; fi
>> +			if [[ -d "$git_dir/../$path" ]];    then return 0; fi
>> +		done < <(cd $parent_git && git submodule --quiet foreach 'echo $path' 2> /dev/null)
> Instead of doing this "loop over submodules and just get true or
> false", it may make a lot more sense to find the module name and
> report that.  That would allow you to have the actual submodule
> name, not a generic "sub:" which does not help the users to tell
> which one of 47 submodules they have in their top-level project
> they are currently in.
>
> If your projects are layed out like so
>
> 	/home/bf/projects/A/
> 	/home/bf/projects/A/lib/B/ -- submodule
> 	/home/bf/projects/A/Doc/ -- another submodule
> 	/home/bf/projects/A/Doc/technical -- a subdirectory of Doc submodule
>
> and you are in /home/bf/projects/A/Doc/technical/ subdirectory, your
> $git_dir variable (which is grossly misnamed, too; call it "top"
> perhaps?) would be "/home/bf/projects/A/Doc" and then your
> $parent_git variable (again, that is misnamed; call it
> "parent_top"?) would be "/home/bf/projects/A".  The submodule path
> to the submodule you are currently in is "Doc" (you learn it as the
> difference between these two).
>
> You can ask the top-level project the name of the submodule that is
> currently at "Doc" with "submodule--helper name".  Unless the project
> has moved it since it first added the submodule, the module name may
> also be "Doc", but if it were moved, it may be different.
>
> And that "module name" is a more useful thing than a hardcoded
> string "sub:" to use in the prompt, I would think.
Reworked in Patch [2/4].

Re: [PATCH 1/4] git-prompt.sh: add submodule indicator

[Junio C Hamano]

Benjamin Fuchs <email@benjaminfuchs.de> writes:

> In [2/4] I got rid of the loop by feedback of Gábor.
> Sorry if my patch wasn't well formed.

While it might be the way other development communities work, in the
Git development community, we do not work that way when presenting
your second and subsequent attempt to the community.

Having the initial draft from the original developers that records
the bugs and misdesigns in an earlier parts of a series and separate
patches that record how the problems were fixed to arrive at the
final shape of the codebase might be interesting to the original
developers, and they may even find such a history valuable, but in
the context of the history that will be recorded in the official
tree of the project for eternity, that just adds useless noise.

Instead of keeping the original, in which problems were pointed out,
and adding later commits to correct them incrementally, a patch is
"rerolled".  That is, you are expected to learn from the review
comments and pretend as if you did the work from scratch and you
already possessed the wisdom lent by the reviewers when you started
your work.  In the "rerolled" patches you send, you pretend as if
you worked without making mistakes you made in the earlier rounds at
all, producing (more) perfect patches from the beginning.  

In reality, you may locally be using Git tools like rebase,
cherry-pick and "add -p" while preparing these "rerolled" rounds of
patches, but the name of the game is to hide that effort from the
public and pretend to be a perfect human, recording the result of
exercising your best ability in the official history ;-).

So this is OK:

    0/3: I want to improve X, and for that I identified that I need
    A, B and C done.  A or B alone is already an improvement, but A
    and B together makes it even more useful, and implementation of
    C requires patches to do A and B.

    1/3: do A
    2/3: do B
    3/3: do C, building on A and B

This is not:

    0/3: I want to improve X, and for that I need to do C.
    1/3: I couldn't do C, and I did A instead.
    2/3: A was totally useless. I fix it to do B.
    3/3: B is not very useful, either. I fix it to do C.

Re: [PATCH 1/4] git-prompt.sh: add submodule indicator

[Stefan Beller]

On Mon, Jan 30, 2017 at 7:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Benjamin Fuchs <email@benjaminfuchs.de> writes:
>
>> In [2/4] I got rid of the loop by feedback of Gábor.
>> Sorry if my patch wasn't well formed.
>
> While it might be the way other development communities work, in the
> Git development community, we do not work that way when presenting
> your second and subsequent attempt to the community.
>
> Having the initial draft from the original developers that records
> the bugs and misdesigns in an earlier parts of a series and separate
> patches that record how the problems were fixed to arrive at the
> final shape of the codebase might be interesting to the original
> developers, and they may even find such a history valuable, but in
> the context of the history that will be recorded in the official
> tree of the project for eternity, that just adds useless noise.
>
> Instead of keeping the original, in which problems were pointed out,
> and adding later commits to correct them incrementally, a patch is
> "rerolled".  That is, you are expected to learn from the review
> comments and pretend as if you did the work from scratch and you
> already possessed the wisdom lent by the reviewers when you started
> your work.  In the "rerolled" patches you send, you pretend as if
> you worked without making mistakes you made in the earlier rounds at
> all, producing (more) perfect patches from the beginning.
>
> In reality, you may locally be using Git tools like rebase,
> cherry-pick and "add -p" while preparing these "rerolled" rounds of
> patches, but the name of the game is to hide that effort from the
> public and pretend to be a perfect human, recording the result of
> exercising your best ability in the official history ;-).
>
> So this is OK:
>
>     0/3: I want to improve X, and for that I identified that I need
>     A, B and C done.  A or B alone is already an improvement, but A
>     and B together makes it even more useful, and implementation of
>     C requires patches to do A and B.
>
>     1/3: do A
>     2/3: do B
>     3/3: do C, building on A and B
>
> This is not:
>
>     0/3: I want to improve X, and for that I need to do C.
>     1/3: I couldn't do C, and I did A instead.
>     2/3: A was totally useless. I fix it to do B.
>     3/3: B is not very useful, either. I fix it to do C.
>

I agree with Junio here,
"git rebase --interactive" and then editing/squashing commits
is your friend.

(unrelated side note:)
At GitMerge facebook presented their improvements on mercurial
and one of the things was "hg absorb". It would take the dirty hunks/lines
of the working tree and amend them into the "stack of commits", i.e. into
your local unpublished history. So instead of making fixup commits
and doing the whole interactive rebase thing, it would do it automatically
for you. I think that is a neat time saver.

Thanks,
Stefan

Re: [PATCH 1/4] git-prompt.sh: add submodule indicator

[Jacob Keller]

On Sun, Feb 5, 2017 at 8:23 PM, Stefan Beller <sbeller@google.com> wrote:
>
> (unrelated side note:)
> At GitMerge facebook presented their improvements on mercurial
> and one of the things was "hg absorb". It would take the dirty hunks/lines
> of the working tree and amend them into the "stack of commits", i.e. into
> your local unpublished history. So instead of making fixup commits
> and doing the whole interactive rebase thing, it would do it automatically
> for you. I think that is a neat time saver.
>
> Thanks,
> Stefan

How exactly was it different from doing "git  commit --fixup xyz" and
"git rebase -i --autosquash"? Like, what was the advantage to the user
facing workflow? Just curious to see if we could learn something from
it.

Regards,
Jake

Re: [PATCH 1/4] git-prompt.sh: add submodule indicator

[Stefan Beller]

On Sun, Feb 5, 2017 at 9:55 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Sun, Feb 5, 2017 at 8:23 PM, Stefan Beller <sbeller@google.com> wrote:
>>
>> (unrelated side note:)
>> At GitMerge facebook presented their improvements on mercurial
>> and one of the things was "hg absorb". It would take the dirty hunks/lines
>> of the working tree and amend them into the "stack of commits", i.e. into
>> your local unpublished history. So instead of making fixup commits
>> and doing the whole interactive rebase thing, it would do it automatically
>> for you. I think that is a neat time saver.
>>
>> Thanks,
>> Stefan
>
> How exactly was it different from doing "git  commit --fixup xyz" and
> "git rebase -i --autosquash"? Like, what was the advantage to the user
> facing workflow? Just curious to see if we could learn something from
> it.

My impression is that all local changes were split up and the "xyz"
was determined based off a heuristic, e.g. blame(?) and then the
rebase is run automatically after that, i.e. having just one command for
a complete workflow here, moving up a whole level in abstraction.

[PATCH 2/4] git-prompt.sh: rework of submodule indicator

[Benjamin Fuchs]

Rework of the first patch. The prompt now will look like this:
(+name:master). I tried to considere all suggestions.
Tests still missing.

Signed-off-by: Benjamin Fuchs <email@benjaminfuchs.de>
---
 contrib/completion/git-prompt.sh | 49 ++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 4c82e7f..c44b9a2 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -95,8 +95,10 @@
 # repository level by setting bash.hideIfPwdIgnored to "false".
 #
 # If you would like __git_ps1 to indicate that you are in a submodule,
-# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before
-# the branch name.
+# set GIT_PS1_SHOWSUBMODULE to a nonempty value. In this case the name
+# of the submodule will be prepended to the branch name (e.g. module:master).
+# The name will be prepended by "+" if the currently checked out submodule
+# commit does not match the SHA-1 found in the index of the containing repository.
 
 # check whether printf supports -v
 __git_printf_supports_v=
@@ -288,30 +290,27 @@ __git_eread ()
 	test -r "$f" && read "$@" <"$f"
 }
 
-# __git_is_submodule
-# Based on:
-# http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule
-__git_is_submodule ()
-{
-	local git_dir parent_git module_name path
-	# Find the root of this git repo, then check if its parent dir is also a repo
-	git_dir="$(git rev-parse --show-toplevel)"
-	module_name="$(basename "$git_dir")"
-	parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> /dev/null)"
-	if [[ -n $parent_git ]]; then
-		# List all the submodule paths for the parent repo
-		while read path
-		do
-			if [[ "$path" != "$module_name" ]]; then continue; fi
-			if [[ -d "$git_dir/../$path" ]];    then return 0; fi
-		done < <(cd $parent_git && git submodule --quiet foreach 'echo $path' 2> /dev/null)
-    fi
-    return 1
-}
-
+# __git_ps1_submodule
+#
+# $1 - git_dir path
+#
+# Returns the name of the submodule if we are currently inside one. The name
+# will be prepended by "+" if the currently checked out submodule commit does
+# not match the SHA-1 found in the index of the containing repository.
+# NOTE: git_dir looks like in a ...
+# - submodule: "GIT_PARENT/.git/modules/PATH_TO_SUBMODULE"
+# - parent: "GIT_PARENT/.git" (exception "." in .git)
 __git_ps1_submodule ()
 {
-	__git_is_submodule && printf "sub:"
+	local git_dir="$1"
+	local submodule_name="$(basename "$git_dir")"
+	if [ "$submodule_name" != ".git" ] && [ "$submodule_name" != "." ]; then
+		local parent_top="${git_dir%.git*}"
+		local submodule_top="${git_dir#*modules}"
+		local status=""
+		git diff -C "$parent_top" --no-ext-diff --ignore-submodules=dirty --quiet -- "$submodule_top" 2>/dev/null || status="+"
+		printf "$status$submodule_name:"
+	fi
 }
 
 # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
@@ -545,7 +544,7 @@ __git_ps1 ()
 
 	local sub=""
 	if [ -n "${GIT_PS1_SHOWSUBMODULE}" ]; then
-		sub="$(__git_ps1_submodule)"
+		sub="$(__git_ps1_submodule $g)"
 	fi
 
 	local f="$w$i$s$u"
-- 
2.7.4

Re: [PATCH 2/4] git-prompt.sh: rework of submodule indicator

[SZEDER Gábor]

> Rework of the first patch. The prompt now will look like this:
> (+name:master). I tried to considere all suggestions.
> Tests still missing.
> 
> Signed-off-by: Benjamin Fuchs <email@benjaminfuchs.de>
> ---
>  contrib/completion/git-prompt.sh | 49 ++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 4c82e7f..c44b9a2 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -95,8 +95,10 @@
>  # repository level by setting bash.hideIfPwdIgnored to "false".
>  #
>  # If you would like __git_ps1 to indicate that you are in a submodule,
> -# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before
> -# the branch name.
> +# set GIT_PS1_SHOWSUBMODULE to a nonempty value. In this case the name
> +# of the submodule will be prepended to the branch name (e.g. module:master).
> +# The name will be prepended by "+" if the currently checked out submodule
> +# commit does not match the SHA-1 found in the index of the containing repository.
>  
>  # check whether printf supports -v
>  __git_printf_supports_v=
> @@ -288,30 +290,27 @@ __git_eread ()
>  	test -r "$f" && read "$@" <"$f"
>  }
>  
> -# __git_is_submodule
> -# Based on:
> -# http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule
> -__git_is_submodule ()
> -{
> -	local git_dir parent_git module_name path
> -	# Find the root of this git repo, then check if its parent dir is also a repo
> -	git_dir="$(git rev-parse --show-toplevel)"
> -	module_name="$(basename "$git_dir")"
> -	parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> /dev/null)"
> -	if [[ -n $parent_git ]]; then
> -		# List all the submodule paths for the parent repo
> -		while read path
> -		do
> -			if [[ "$path" != "$module_name" ]]; then continue; fi
> -			if [[ -d "$git_dir/../$path" ]];    then return 0; fi
> -		done < <(cd $parent_git && git submodule --quiet foreach 'echo $path' 2> /dev/null)
> -    fi
> -    return 1
> -}
> -
> +# __git_ps1_submodule
> +#
> +# $1 - git_dir path
> +#
> +# Returns the name of the submodule if we are currently inside one. The name
> +# will be prepended by "+" if the currently checked out submodule commit does
> +# not match the SHA-1 found in the index of the containing repository.
> +# NOTE: git_dir looks like in a ...
> +# - submodule: "GIT_PARENT/.git/modules/PATH_TO_SUBMODULE"
> +# - parent: "GIT_PARENT/.git" (exception "." in .git)
>  __git_ps1_submodule ()
>  {
> -	__git_is_submodule && printf "sub:"
> +	local git_dir="$1"
> +	local submodule_name="$(basename "$git_dir")"
> +	if [ "$submodule_name" != ".git" ] && [ "$submodule_name" != "." ]; then
> +		local parent_top="${git_dir%.git*}"
> +		local submodule_top="${git_dir#*modules}"
> +		local status=""
> +		git diff -C "$parent_top" --no-ext-diff --ignore-submodules=dirty --quiet -- "$submodule_top" 2>/dev/null || status="+"

This 'git diff' has to read the index of the parent repository, right?
That can be potentially very expensive, if the parent repository, and
thus its index, is big.

You might want to provide finer granularity controls and separate the
"$PWD is in a submodule" indicator from the "submodule commit doesn't
match" indicator.  Even if someone is in general interested in the
former, he might have some huge repositories where he would prefer to
disable the latter, because executing 'git diff' would make the prompt
lag.

I'm not sure we need brand new control knobs, though.  Perhaps we can
get away by checking the env and config variables used for the dirty
index and worktree status indicator, after all they, too, are about
whether to run 'git diff' for the prompt in a repository or not.

> +		printf "$status$submodule_name:"
> +	fi
>  }
>  
>  # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
> @@ -545,7 +544,7 @@ __git_ps1 ()
>  
>  	local sub=""
>  	if [ -n "${GIT_PS1_SHOWSUBMODULE}" ]; then
> -		sub="$(__git_ps1_submodule)"
> +		sub="$(__git_ps1_submodule $g)"

In Bash, and in shells in general, the visibility of variables works
differently than in "regular" programming languages:

  - Any variable existing in a caller, even the ones declared as
    'local' in the caller, are visible in all callees.
    
    This means you don't have to pass $g as parameter to
    __git_ps1_submodule(), because you can access it inside that
    function directly.  This has the benefit that there is one less
    place where you can forget to quote a path variable :)

  - If the callee modifies any variable that isn't declared as local
    in the callee, then the caller will see the modified value of that
    variable, unless the callee was invoked in a subshell.

    Here your sole purpose is to set $sub to something like "+sub:",
    which is determined in __git_ps1_submodule().  You don't need the
    command substitution for that, you can set $sub directly in
    __git_ps1_submodule(), sparing the overhead of fork()ing a
    subshell.  That's important for the sake of git users on Windows.

>  	fi
>  
>  	local f="$w$i$s$u"
-- 
2.7.4

[PATCH 3/4] git-prompt.sh: fix for submodule 'dirty' indicator

[Benjamin Fuchs]

Fixing wrong git diff line.

Signed-off-by: Benjamin Fuchs <email@benjaminfuchs.de>
---
 contrib/completion/git-prompt.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index c44b9a2..43b28e9 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -306,9 +306,9 @@ __git_ps1_submodule ()
 	local submodule_name="$(basename "$git_dir")"
 	if [ "$submodule_name" != ".git" ] && [ "$submodule_name" != "." ]; then
 		local parent_top="${git_dir%.git*}"
-		local submodule_top="${git_dir#*modules}"
+		local submodule_top="${git_dir#*modules/}"
 		local status=""
-		git diff -C "$parent_top" --no-ext-diff --ignore-submodules=dirty --quiet -- "$submodule_top" 2>/dev/null || status="+"
+		git -C "$parent_top" diff --no-ext-diff --ignore-submodules=dirty --quiet -- "$submodule_top" 2>/dev/null || status="+"
 		printf "$status$submodule_name:"
 	fi
 }
@@ -544,7 +544,7 @@ __git_ps1 ()
 
 	local sub=""
 	if [ -n "${GIT_PS1_SHOWSUBMODULE}" ]; then
-		sub="$(__git_ps1_submodule $g)"
+		sub="$(__git_ps1_submodule "$g")"
 	fi
 
 	local f="$w$i$s$u"
-- 
2.7.4

[PATCH 4/4] git-prompt.sh: add tests for submodule indicator

[Benjamin Fuchs]

Signed-off-by: Benjamin Fuchs <email@benjaminfuchs.de>
---
 t/t9903-bash-prompt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 97c9b32..4dce366 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -37,6 +37,11 @@ test_expect_success 'setup for prompt tests' '
 	git commit -m "yet another b2" file &&
 	mkdir ignored_dir &&
 	echo "ignored_dir/" >>.gitignore &&
+	git checkout -b submodule &&
+	git submodule add ./. sub &&
+	git -C sub checkout master &&
+	git add sub &&
+	git commit -m submodule &&
 	git checkout master
 '
 
@@ -755,4 +760,42 @@ test_expect_success 'prompt - hide if pwd ignored - inside gitdir (stderr)' '
 	test_cmp expected "$actual"
 '
 
+test_expect_success 'prompt - submodule indicator' '
+	printf " (sub:master)" >expected &&
+	git checkout submodule &&
+	test_when_finished "git checkout master" &&
+	(
+		cd sub &&
+		GIT_PS1_SHOWSUBMODULE=1 &&
+		__git_ps1 >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - submodule indicator - verify false' '
+	printf " (master)" >expected &&
+	git checkout submodule &&
+	test_when_finished "git checkout master" &&
+	(
+		cd sub &&
+		GIT_PS1_SHOWSUBMODULE= &&
+		__git_ps1 >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - submodule indicator - dirty status indicator' '
+	printf " (+sub:b1)" >expected &&
+	git checkout submodule &&
+	git -C sub checkout b1 &&
+	test_when_finished "git checkout master" &&
+	(
+		cd sub &&
+		GIT_PS1_SHOWSUBMODULE=1 &&
+		__git_ps1 >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+
 test_done
-- 
2.7.4

Re: [PATCH 4/4] git-prompt.sh: add tests for submodule indicator

[SZEDER Gábor]

On Mon, Jan 30, 2017 at 9:44 PM, Benjamin Fuchs <email@benjaminfuchs.de> wrote:
> Signed-off-by: Benjamin Fuchs <email@benjaminfuchs.de>
> ---
>  t/t9903-bash-prompt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> index 97c9b32..4dce366 100755
> --- a/t/t9903-bash-prompt.sh
> +++ b/t/t9903-bash-prompt.sh
> @@ -37,6 +37,11 @@ test_expect_success 'setup for prompt tests' '
>         git commit -m "yet another b2" file &&
>         mkdir ignored_dir &&
>         echo "ignored_dir/" >>.gitignore &&
> +       git checkout -b submodule &&
> +       git submodule add ./. sub &&

./. ?

> +       git -C sub checkout master &&
> +       git add sub &&
> +       git commit -m submodule &&
>         git checkout master
>  '
>
> @@ -755,4 +760,42 @@ test_expect_success 'prompt - hide if pwd ignored - inside gitdir (stderr)' '
>         test_cmp expected "$actual"
>  '
>
> +test_expect_success 'prompt - submodule indicator' '
> +       printf " (sub:master)" >expected &&
> +       git checkout submodule &&
> +       test_when_finished "git checkout master" &&
> +       (
> +               cd sub &&
> +               GIT_PS1_SHOWSUBMODULE=1 &&
> +               __git_ps1 >"$actual"
> +       ) &&
> +       test_cmp expected "$actual"
> +'
> +
> +test_expect_success 'prompt - submodule indicator - verify false' '

I was puzzled by the "verify false" here.  You mean "disabled", right?

> +       printf " (master)" >expected &&
> +       git checkout submodule &&
> +       test_when_finished "git checkout master" &&
> +       (
> +               cd sub &&
> +               GIT_PS1_SHOWSUBMODULE= &&
> +               __git_ps1 >"$actual"
> +       ) &&
> +       test_cmp expected "$actual"
> +'
> +
> +test_expect_success 'prompt - submodule indicator - dirty status indicator' '
> +       printf " (+sub:b1)" >expected &&
> +       git checkout submodule &&
> +       git -C sub checkout b1 &&
> +       test_when_finished "git checkout master" &&
> +       (
> +               cd sub &&
> +               GIT_PS1_SHOWSUBMODULE=1 &&
> +               __git_ps1 >"$actual"
> +       ) &&
> +       test_cmp expected "$actual"
> +'
> +
> +
>  test_done
> --
> 2.7.4
>

Re: [PATCH 4/4] git-prompt.sh: add tests for submodule indicator

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Mon, Jan 30, 2017 at 9:44 PM, Benjamin Fuchs <email@benjaminfuchs.de> wrote:
>> Signed-off-by: Benjamin Fuchs <email@benjaminfuchs.de>
>> ---
>>  t/t9903-bash-prompt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 43 insertions(+)
>>
>> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
>> index 97c9b32..4dce366 100755
>> --- a/t/t9903-bash-prompt.sh
>> +++ b/t/t9903-bash-prompt.sh
>> @@ -37,6 +37,11 @@ test_expect_success 'setup for prompt tests' '
>>         git commit -m "yet another b2" file &&
>>         mkdir ignored_dir &&
>>         echo "ignored_dir/" >>.gitignore &&
>> +       git checkout -b submodule &&
>> +       git submodule add ./. sub &&
>
> ./. ?

Good eyes.  This is a pattern we are trying to wean ourselves off
of.  E.g. cf.

    https://public-inbox.org/git/20170105192904.1107-2-sbeller@google.com/#t

Hopefully this reminds us to resurrect and finish the test fixes in
that thread?

Re: [PATCH 4/4] git-prompt.sh: add tests for submodule indicator

[Stefan Beller]

On Tue, Jan 31, 2017 at 2:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> On Mon, Jan 30, 2017 at 9:44 PM, Benjamin Fuchs <email@benjaminfuchs.de> wrote:
>>> Signed-off-by: Benjamin Fuchs <email@benjaminfuchs.de>
>>> ---
>>>  t/t9903-bash-prompt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 43 insertions(+)
>>>
>>> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
>>> index 97c9b32..4dce366 100755
>>> --- a/t/t9903-bash-prompt.sh
>>> +++ b/t/t9903-bash-prompt.sh
>>> @@ -37,6 +37,11 @@ test_expect_success 'setup for prompt tests' '
>>>         git commit -m "yet another b2" file &&
>>>         mkdir ignored_dir &&
>>>         echo "ignored_dir/" >>.gitignore &&
>>> +       git checkout -b submodule &&
>>> +       git submodule add ./. sub &&
>>
>> ./. ?
>
> Good eyes.  This is a pattern we are trying to wean ourselves off
> of.  E.g. cf.
>
>     https://public-inbox.org/git/20170105192904.1107-2-sbeller@google.com/#t
>
> Hopefully this reminds us to resurrect and finish the test fixes in
> that thread?

I plan to eventually, yes. but that is a refactoring, that has lower prio
than getting checkout working recursing into submodules.

[RFC PATCH] rev-parse: add --show-superproject-working-tree

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Marking this as RFC as documentation and tests are missing.

 builtin/rev-parse.c |  7 +++++
 submodule.c         | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 submodule.h         |  8 +++++
 3 files changed, 102 insertions(+)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index e08677e559..2549643267 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -12,6 +12,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "split-index.h"
+#include "submodule.h"
 
 #define DO_REVS		1
 #define DO_NOREV	2
@@ -779,6 +780,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 					puts(work_tree);
 				continue;
 			}
+			if (!strcmp(arg, "--show-superproject-working-tree")) {
+				const char *superproject = get_superproject_working_tree();
+				if (superproject)
+					puts(superproject);
+				continue;
+			}
 			if (!strcmp(arg, "--show-prefix")) {
 				if (prefix)
 					puts(prefix);
diff --git a/submodule.c b/submodule.c
index 3b98766a6b..a63aef2c6b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1514,3 +1514,90 @@ void absorb_git_dir_into_superproject(const char *prefix,
 		strbuf_release(&sb);
 	}
 }
+
+static int superproject_exists(void)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf sb = STRBUF_INIT;
+	struct strbuf buf = STRBUF_INIT;
+	const char *one_up = real_path_if_valid("../");
+	const char *dirname;
+	int code, has_superproject = 0;
+
+	if (!one_up)
+		/* At the root of the file system. */
+		return 0;
+
+	dirname = relative_path(xgetcwd(), one_up, &sb);
+	prepare_submodule_repo_env(&cp.env_array);
+	argv_array_pop(&cp.env_array);
+	argv_array_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
+			"ls-tree", "HEAD", "--", dirname, NULL);
+
+	cp.no_stdin = 1;
+	cp.no_stderr = 1;
+	cp.out = -1;
+	cp.git_cmd = 1;
+
+	if (start_command(&cp))
+		die(_("could not start ls-tree in .."));
+
+	strbuf_read(&buf, cp.out, 7);
+	close(cp.out);
+	if (starts_with(buf.buf, "160000"))
+		/* there is a superproject having this as a submodule */
+		has_superproject = 1;
+
+	code = finish_command(&cp);
+
+	if (code == 128)
+		/* not a git repository */
+		goto out;
+	if (code == 0 && !has_superproject)
+		/* there is an unrelated git repository */
+		goto out;
+
+	if (code)
+		die(_("ls-tree returned unexpected return code"));
+
+	return 1;
+
+out:
+	strbuf_release(&sb);
+
+	return 0;
+}
+
+const char *get_superproject_working_tree()
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf sb = STRBUF_INIT;
+
+	if (!superproject_exists())
+		return NULL;
+
+	prepare_submodule_repo_env(&cp.env_array);
+	argv_array_pop(&cp.env_array);
+
+	argv_array_pushl(&cp.args, "-C", "..",
+			"rev-parse", "--show-toplevel", NULL);
+
+	cp.no_stdin = 1;
+	cp.no_stderr = 1;
+	cp.out = -1;
+	cp.git_cmd = 1;
+
+	if (start_command(&cp))
+		die(_("could not start rev-parse in .."));
+
+	strbuf_reset(&sb);
+	strbuf_read(&sb, cp.out, PATH_MAX);
+
+	/* remove trailing new line */
+	strbuf_rtrim(&sb);
+
+	if (finish_command(&cp))
+		die(_("rev-parse died unexpectedly"));
+
+	return strbuf_detach(&sb, NULL);
+}
diff --git a/submodule.h b/submodule.h
index 05ab674f06..f207bb8d5f 100644
--- a/submodule.h
+++ b/submodule.h
@@ -93,4 +93,12 @@ extern void prepare_submodule_repo_env(struct argv_array *out);
 extern void absorb_git_dir_into_superproject(const char *prefix,
 					     const char *path,
 					     unsigned flags);
+
+/*
+ * Return the absolute path of the working tree of the superproject, which this
+ * project is a submodule of. If this repository is not a submodule of
+ * another repository, return NULL.
+ */
+extern const char *get_superproject_working_tree();
+
 #endif
-- 
2.12.0.189.g3bc53220cb.dirty

Re: [RFC PATCH] rev-parse: add --show-superproject-working-tree

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> diff --git a/submodule.c b/submodule.c
> index 3b98766a6b..a63aef2c6b 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1514,3 +1514,90 @@ void absorb_git_dir_into_superproject(const char *prefix,
>  		strbuf_release(&sb);
>  	}
>  }

Please have a comment here what this function expects (e.g. it is
called after we ran repository discovery and $cwd is at the root of
the working tree of the current project, we _know_ we have a working
tree, etc.).

> +
> +static int superproject_exists(void)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf sb = STRBUF_INIT;
> +	struct strbuf buf = STRBUF_INIT;
> +	const char *one_up = real_path_if_valid("../");
> +	const char *dirname;
> +	int code, has_superproject = 0;
> +
> +	if (!one_up)
> +		/* At the root of the file system. */
> +		return 0;

Hmph.  I would have expected that "/../" would be the same as "/".

> +	dirname = relative_path(xgetcwd(), one_up, &sb);

So, the idea is we start at the root level of the current project's
working tree, and we go up one level, then we know the last component
of the path our submodule is bound at in the superproject.

> +	prepare_submodule_repo_env(&cp.env_array);
> +	argv_array_pop(&cp.env_array);
> +	argv_array_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
> +			"ls-tree", "HEAD", "--", dirname, NULL);

This would ask our superproject what is at the "dirname" in its
HEAD.  Two possible issues:

 - Shouldn't that be looking at its index instead?  It would be more
   correct for unborn branch case, and new or moved submodule case.

 - If our submodule is bound at path sub/dir in the superproject,
   the relative-path thing above would get "dir" and this ls-tree
   ends up asking what is at "dir", but the question you really want
   to ask is what is at "sub/dir", isn't it?

> +const char *get_superproject_working_tree()

const char *get_superproject_working_tree(void)

Re: [RFC PATCH] rev-parse: add --show-superproject-working-tree

[Junio C Hamano]

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

>> +	dirname = relative_path(xgetcwd(), one_up, &sb);
>
> So, the idea is we start at the root level of the current project's
> working tree, and we go up one level, then we know the last component
> of the path our submodule is bound at in the superproject.
>
>> +	prepare_submodule_repo_env(&cp.env_array);
>> +	argv_array_pop(&cp.env_array);
>> +	argv_array_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
>> +			"ls-tree", "HEAD", "--", dirname, NULL);
>
> This would ask our superproject what is at the "dirname" in its
> HEAD.  Two possible issues:
>
>  - Shouldn't that be looking at its index instead?  It would be more
>    correct for unborn branch case, and new or moved submodule case.
>
>  - If our submodule is bound at path sub/dir in the superproject,
>    the relative-path thing above would get "dir" and this ls-tree
>    ends up asking what is at "dir", but the question you really want
>    to ask is what is at "sub/dir", isn't it?

IOW, the basic ontline of the idea may be OK, but I think you would
want to do something along this line:

    - chdir to .. from the root of your submodule working tree;
    - in that .. directory, ask what prefix it is (you'd get
      "sub/dir", or "not a git" if you are not a submodule);
    - in that .. directory, ask ls-files what sub/dir is;
    - if it is 160000, you're happy.

Re: [RFC PATCH] rev-parse: add --show-superproject-working-tree

[Junio C Hamano]

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

>>  - If our submodule is bound at path sub/dir in the superproject,
>>    the relative-path thing above would get "dir" and this ls-tree
>>    ends up asking what is at "dir", but the question you really want
>>    to ask is what is at "sub/dir", isn't it?
>
> IOW, the basic ontline of the idea may be OK, but I think you would
> want to do something along this line:
>
>     - chdir to .. from the root of your submodule working tree;
>     - in that .. directory, ask what prefix it is (you'd get
>       "sub/dir", or "not a git" if you are not a submodule);
>     - in that .. directory, ask ls-files what sub/dir is;
>     - if it is 160000, you're happy.

Nah, that wouldn't be necessary and would not work.  --prefix would
be "sub" in that case, and you'd need to concatenate the "dir" that
is the basename of the path to the submodule to get "sub/dir".  

Besides, output from "ls-files" by default is relative to cwd, so if
you did ls-files or ls-tree HEAD in "sub/", you'll find where you
came from as "dir", not "sub/dir", so the original code happens to
work even from a subdirectory of a superproject, but the reason why
it works is a bit subtle.  Perhaps it deserves in-code comment to
explain it.

Re: [RFC PATCH] rev-parse: add --show-superproject-working-tree

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +const char *get_superproject_working_tree()

const char *get_superproject_working_tree(void)

The same for the *.h file declaration.

> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	if (!superproject_exists())
> +		return NULL;
> + ...
> +	return strbuf_detach(&sb, NULL);

Having reviewed it, I somehow think you do not want to have a
separate superproject_exists() that grabs some part of the
information this caller needs and then discards it.

The helper already does these things:

    - xgetcwd(), which may give you "/local/repo/super/sub/dir"

    - relative_path() with the result and "..", which may give you
      "dir"

    - ls-tree HEAD "dir" to see what is in "sub/dir" of the
      repository that governs ".."; if "sub/dir" is a gitlink,
      you know you started in a working tree of a repository
      different from the one that governs "..".

And the caller is trying to figure out where the root of the
superproject is, i.e. "/local/repo/super", and the helper has half
of the answer to that already.  If you ask the "ls-tree HEAD" (as I
said, I think it should be "ls-files") to give you not "dir" but
"sub/dir", you can subtract it from the result of xgetcwd() you did
at the beginning of the helper, and that gives what this caller of
the helper wants.

So perhaps your superproject_exists() helper can be eliminated and
instead coded in get_superproject_working_tree() in place to do:

	- xgetcwd() to get "/local/repo/super/sub/dir".

	- relative_path() to get "dir".

	- ask "ls-{tree,files} --full-name HEAD dir" to get "160000"
          and "sub/dir".

	- subtract "sub/dir" from the tail of the "/local/repo/super/sub/dir"
	  you got from xgetcwd() earlier.

	- return the result.

with a failure/unmet expectations (like not finding 160000) from any
step returning an error, or something like that.

Re: [RFC PATCH] rev-parse: add --show-superproject-working-tree

[Stefan Beller]

On Tue, Mar 7, 2017 at 10:44 AM, Junio C Hamano <gitster@pobox.com> wrote:

> So perhaps your superproject_exists() helper can be eliminated

That is what I had originally, but I assumed a strict helper function
for "existence of the superproject" would be interesting in the future,
e.g. for get_superproject_git_dir, or on its own. There was an attempt
to have the shell prompt indicate if you are in a submodule,
which would not need to know the worktree or git dir of the
superproject, but only its existence.

> instead coded in get_superproject_working_tree() in place to do:
>
>         - xgetcwd() to get "/local/repo/super/sub/dir".

Did you mean .../super/dir/sub ?

If not and we run this command from a directory inside the
submodule, the usual prefix mechanics should go to the
root of the submodule, such that the ".." will be just enough
to break out of that submodule repo.

The interesting part is in the superproject, to see if we are
in a directory (and where the root of the superproject is).

>         - relative_path() to get "dir".

ok.

>         - ask "ls-{tree,files} --full-name HEAD dir" to get "160000"
>           and "sub/dir".

"ls-files --stage --full-name" to get
160000 ... dir/sub

>
>         - subtract "sub/dir" from the tail of the "/local/repo/super/sub/dir"
>           you got from xgetcwd() earlier.

makes sense.

>
>         - return the result.
>
> with a failure/unmet expectations (like not finding 160000) from any
> step returning an error, or something like that.

That seems better as we only need to spawn one process.

(This could also mean I am bad at reading our own man pages)

Thanks,
Stefan

Re: [RFC PATCH] rev-parse: add --show-superproject-working-tree

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Mar 7, 2017 at 10:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> So perhaps your superproject_exists() helper can be eliminated
>
> That is what I had originally, but I assumed a strict helper function
> for "existence of the superproject" would be interesting in the future,
> e.g. for get_superproject_git_dir, or on its own. There was an attempt
> to have the shell prompt indicate if you are in a submodule,
> which would not need to know the worktree or git dir of the
> superproject, but only its existence.
>
>> instead coded in get_superproject_working_tree() in place to do:
>>
>>         - xgetcwd() to get "/local/repo/super/sub/dir".
>
> Did you mean .../super/dir/sub ?

I meant "/local/repo/super/sub/dir".  I am using this case to
illustrate: the root of the superproject is at /local/repo/super,
its submodule we are interested in is at sub/dir, and the function
is working inside the submodule--after the repository discovery
moves the cwd, xgetcwd() would give the root of the working tree of
the submodule, which is at "/local/repo/super/sub/dir".

And that would give us "dir" by taking that as relative to its "../"

>>         - relative_path() to get "dir".
>
> ok.

indeed.

>>         - ask "ls-{tree,files} --full-name HEAD dir" to get "160000"
>>           and "sub/dir".
>
> "ls-files --stage --full-name" to get
> 160000 ... dir/sub

Yeah, also when usihng ls-files you obviously do not give HEAD but
you do give "dir" as the pathspec.  And you get "sub/dir" as the
result.

[PATCHv2] rev-parse: add --show-superproject-working-tree

[Stefan Beller]

In some situations it is useful to know if the given repository
is a submodule of another repository.

Add the flag --show-superproject-working-tree to git-rev-parse
to make it easy to find out if there is a superproject.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

* not RFC anymore, but for real this time; including a test and docs :)

* Following Junios advice: there is only one function
  (superproject_exists was dropped) using ls-files.
  (the test actually tests for a staged submodule) 
  
Thanks,
Stefan


 Documentation/git-rev-parse.txt |  5 +++
 builtin/rev-parse.c             |  7 ++++
 submodule.c                     | 83 +++++++++++++++++++++++++++++++++++++++++
 submodule.h                     |  8 ++++
 t/t1500-rev-parse.sh            | 14 +++++++
 5 files changed, 117 insertions(+)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 91c02b8c85..b841bad7c7 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -261,6 +261,11 @@ print a message to stderr and exit with nonzero status.
 --show-toplevel::
 	Show the absolute path of the top-level directory.
 
+--show-superproject-working-tree
+	Show the absolute path of the top-level directory of
+	the superproject. A superproject is a repository that records
+	this repository as a submodule.
+
 --shared-index-path::
 	Show the path to the shared index file in split index mode, or
 	empty if not in split-index mode.
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index e08677e559..2549643267 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -12,6 +12,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "split-index.h"
+#include "submodule.h"
 
 #define DO_REVS		1
 #define DO_NOREV	2
@@ -779,6 +780,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 					puts(work_tree);
 				continue;
 			}
+			if (!strcmp(arg, "--show-superproject-working-tree")) {
+				const char *superproject = get_superproject_working_tree();
+				if (superproject)
+					puts(superproject);
+				continue;
+			}
 			if (!strcmp(arg, "--show-prefix")) {
 				if (prefix)
 					puts(prefix);
diff --git a/submodule.c b/submodule.c
index 3b98766a6b..06473d3646 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1514,3 +1514,86 @@ void absorb_git_dir_into_superproject(const char *prefix,
 		strbuf_release(&sb);
 	}
 }
+
+const char *get_superproject_working_tree(void)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf sb = STRBUF_INIT;
+	const char *one_up = real_path_if_valid("../");
+	const char *cwd = xgetcwd();
+	const char *ret = NULL;
+	const char *subpath;
+	int code;
+	ssize_t len;
+
+	if (!is_inside_work_tree())
+		/*
+		 * FIXME:
+		 * We might have a superproject, but it is harder
+		 * to determine.
+		 */
+		return NULL;
+
+	if (!one_up)
+		return NULL;
+
+	subpath = relative_path(cwd, one_up, &sb);
+
+	prepare_submodule_repo_env(&cp.env_array);
+	argv_array_pop(&cp.env_array);
+
+	argv_array_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
+			"ls-files", "--stage", "--full-name", "--", subpath, NULL);
+	strbuf_reset(&sb);
+
+	cp.no_stdin = 1;
+	cp.no_stderr = 1;
+	cp.out = -1;
+	cp.git_cmd = 1;
+
+	if (start_command(&cp))
+		die(_("could not start ls-files in .."));
+
+	len = strbuf_read(&sb, cp.out, PATH_MAX);
+	close(cp.out);
+
+	if (starts_with(sb.buf, "160000")) {
+		int super_sub_len;
+		int cwd_len = strlen(cwd);
+		char *super_sub, *super_wt;
+
+		/*
+		 * There is a superproject having this repo as a submodule.
+		 * The format is <mode> SP <hash> SP <stage> TAB <full name> LF,
+		 * First remove LF, then skip up to \t.
+		 */
+		strbuf_rtrim(&sb);
+		super_sub = strchr(sb.buf, '\t') + 1;
+
+		super_sub_len = sb.buf + sb.len - super_sub;
+		if (super_sub_len > cwd_len ||
+		    strcmp(&cwd[cwd_len - super_sub_len], super_sub))
+			die (_("BUG: returned path string doesn't match cwd?"));
+
+		super_wt = xstrdup(cwd);
+		super_wt[cwd_len - super_sub_len] = '\0';
+
+		ret = real_path(super_wt);
+
+		free(super_wt);
+	}
+	strbuf_release(&sb);
+
+	code = finish_command(&cp);
+
+	if (code == 128)
+		/* '../' is not a git repository */
+		return NULL;
+	if (code == 0 && len == 0)
+		/* There is an unrelated git repository at '../' */
+		return NULL;
+	if (code)
+		die(_("ls-tree returned unexpected return code %d"), code);
+
+	return ret;
+}
diff --git a/submodule.h b/submodule.h
index 05ab674f06..c8a0c9cb29 100644
--- a/submodule.h
+++ b/submodule.h
@@ -93,4 +93,12 @@ extern void prepare_submodule_repo_env(struct argv_array *out);
 extern void absorb_git_dir_into_superproject(const char *prefix,
 					     const char *path,
 					     unsigned flags);
+
+/*
+ * Return the absolute path of the working tree of the superproject, which this
+ * project is a submodule of. If this repository is not a submodule of
+ * another repository, return NULL.
+ */
+extern const char *get_superproject_working_tree(void);
+
 #endif
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 9ed8b8ccba..03d3c7f6d6 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -116,4 +116,18 @@ test_expect_success 'git-path inside sub-dir' '
 	test_cmp expect actual
 '
 
+test_expect_success 'showing the superproject correctly' '
+	git rev-parse --show-superproject-working-tree >out &&
+	test_must_be_empty out &&
+
+	test_create_repo super &&
+	test_commit -C super test_commit &&
+	test_create_repo sub &&
+	test_commit -C sub test_commit &&
+	git -C super submodule add ../sub dir/sub &&
+	echo $(pwd)/super >expect  &&
+	git -C super/dir/sub rev-parse --show-superproject-working-tree >out &&
+	test_cmp expect out
+'
+
 test_done
-- 
2.12.0.190.g6a12a61b77.dirty

Re: [PATCHv2] rev-parse: add --show-superproject-working-tree

[Junio C Hamano]

Looks more or less right but invoke "ls-files -z" and reading the \0
delimited output would be easier; otherwise you would have to worry
about c-unquoting the pathname when the submodule is bound at a path
with funny character (like a double-quote) in it.

Also, returning the exact string of the path from the API function is
absolutely the right thing. I however have to wonder if rev-parse need
to do the c-quoting unless it is told to show pathnames in its output
without quoting (perhaps with "-z")? Or are paths from "rev-parse"
(like "--git-dir", "--show-toplevel", etc.) already excempt from the
usual quoting rules---if so, doing puts() and nothing else is fine to
be consistent with the existing practice (in the longer term, I am
sure we would need to revisit so that scripts can handle paths with
funny characters sensibly, but that would be a different topic if
existing ones like "--git-dir" are already unsafe).

Sorry for top-posting (I am not on a terminal right now).

On Tue, Mar 7, 2017 at 4:56 PM, Stefan Beller <sbeller@google.com> wrote:
> In some situations it is useful to know if the given repository
> is a submodule of another repository.
>
> Add the flag --show-superproject-working-tree to git-rev-parse
> to make it easy to find out if there is a superproject.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> * not RFC anymore, but for real this time; including a test and docs :)
>
> * Following Junios advice: there is only one function
>   (superproject_exists was dropped) using ls-files.
>   (the test actually tests for a staged submodule)
>
> Thanks,
> Stefan
>
>
>  Documentation/git-rev-parse.txt |  5 +++
>  builtin/rev-parse.c             |  7 ++++
>  submodule.c                     | 83 +++++++++++++++++++++++++++++++++++++++++
>  submodule.h                     |  8 ++++
>  t/t1500-rev-parse.sh            | 14 +++++++
>  5 files changed, 117 insertions(+)
>
> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> index 91c02b8c85..b841bad7c7 100644
> --- a/Documentation/git-rev-parse.txt
> +++ b/Documentation/git-rev-parse.txt
> @@ -261,6 +261,11 @@ print a message to stderr and exit with nonzero status.
>  --show-toplevel::
>         Show the absolute path of the top-level directory.
>
> +--show-superproject-working-tree
> +       Show the absolute path of the top-level directory of
> +       the superproject. A superproject is a repository that records
> +       this repository as a submodule.
> +
>  --shared-index-path::
>         Show the path to the shared index file in split index mode, or
>         empty if not in split-index mode.
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index e08677e559..2549643267 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -12,6 +12,7 @@
>  #include "diff.h"
>  #include "revision.h"
>  #include "split-index.h"
> +#include "submodule.h"
>
>  #define DO_REVS                1
>  #define DO_NOREV       2
> @@ -779,6 +780,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>                                         puts(work_tree);
>                                 continue;
>                         }
> +                       if (!strcmp(arg, "--show-superproject-working-tree")) {
> +                               const char *superproject = get_superproject_working_tree();
> +                               if (superproject)
> +                                       puts(superproject);
> +                               continue;
> +                       }
>                         if (!strcmp(arg, "--show-prefix")) {
>                                 if (prefix)
>                                         puts(prefix);
> diff --git a/submodule.c b/submodule.c
> index 3b98766a6b..06473d3646 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1514,3 +1514,86 @@ void absorb_git_dir_into_superproject(const char *prefix,
>                 strbuf_release(&sb);
>         }
>  }
> +
> +const char *get_superproject_working_tree(void)
> +{
> +       struct child_process cp = CHILD_PROCESS_INIT;
> +       struct strbuf sb = STRBUF_INIT;
> +       const char *one_up = real_path_if_valid("../");
> +       const char *cwd = xgetcwd();
> +       const char *ret = NULL;
> +       const char *subpath;
> +       int code;
> +       ssize_t len;
> +
> +       if (!is_inside_work_tree())
> +               /*
> +                * FIXME:
> +                * We might have a superproject, but it is harder
> +                * to determine.
> +                */
> +               return NULL;
> +
> +       if (!one_up)
> +               return NULL;
> +
> +       subpath = relative_path(cwd, one_up, &sb);
> +
> +       prepare_submodule_repo_env(&cp.env_array);
> +       argv_array_pop(&cp.env_array);
> +
> +       argv_array_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
> +                       "ls-files", "--stage", "--full-name", "--", subpath, NULL);
> +       strbuf_reset(&sb);
> +
> +       cp.no_stdin = 1;
> +       cp.no_stderr = 1;
> +       cp.out = -1;
> +       cp.git_cmd = 1;
> +
> +       if (start_command(&cp))
> +               die(_("could not start ls-files in .."));
> +
> +       len = strbuf_read(&sb, cp.out, PATH_MAX);
> +       close(cp.out);
> +
> +       if (starts_with(sb.buf, "160000")) {
> +               int super_sub_len;
> +               int cwd_len = strlen(cwd);
> +               char *super_sub, *super_wt;
> +
> +               /*
> +                * There is a superproject having this repo as a submodule.
> +                * The format is <mode> SP <hash> SP <stage> TAB <full name> LF,
> +                * First remove LF, then skip up to \t.
> +                */
> +               strbuf_rtrim(&sb);
> +               super_sub = strchr(sb.buf, '\t') + 1;
> +
> +               super_sub_len = sb.buf + sb.len - super_sub;
> +               if (super_sub_len > cwd_len ||
> +                   strcmp(&cwd[cwd_len - super_sub_len], super_sub))
> +                       die (_("BUG: returned path string doesn't match cwd?"));
> +
> +               super_wt = xstrdup(cwd);
> +               super_wt[cwd_len - super_sub_len] = '\0';
> +
> +               ret = real_path(super_wt);
> +
> +               free(super_wt);
> +       }
> +       strbuf_release(&sb);
> +
> +       code = finish_command(&cp);
> +
> +       if (code == 128)
> +               /* '../' is not a git repository */
> +               return NULL;
> +       if (code == 0 && len == 0)
> +               /* There is an unrelated git repository at '../' */
> +               return NULL;
> +       if (code)
> +               die(_("ls-tree returned unexpected return code %d"), code);
> +
> +       return ret;
> +}
> diff --git a/submodule.h b/submodule.h
> index 05ab674f06..c8a0c9cb29 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -93,4 +93,12 @@ extern void prepare_submodule_repo_env(struct argv_array *out);
>  extern void absorb_git_dir_into_superproject(const char *prefix,
>                                              const char *path,
>                                              unsigned flags);
> +
> +/*
> + * Return the absolute path of the working tree of the superproject, which this
> + * project is a submodule of. If this repository is not a submodule of
> + * another repository, return NULL.
> + */
> +extern const char *get_superproject_working_tree(void);
> +
>  #endif
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index 9ed8b8ccba..03d3c7f6d6 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -116,4 +116,18 @@ test_expect_success 'git-path inside sub-dir' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'showing the superproject correctly' '
> +       git rev-parse --show-superproject-working-tree >out &&
> +       test_must_be_empty out &&
> +
> +       test_create_repo super &&
> +       test_commit -C super test_commit &&
> +       test_create_repo sub &&
> +       test_commit -C sub test_commit &&
> +       git -C super submodule add ../sub dir/sub &&
> +       echo $(pwd)/super >expect  &&
> +       git -C super/dir/sub rev-parse --show-superproject-working-tree >out &&
> +       test_cmp expect out
> +'
> +
>  test_done
> --
> 2.12.0.190.g6a12a61b77.dirty
>

Re: [PATCHv2] rev-parse: add --show-superproject-working-tree

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +			if (!strcmp(arg, "--show-superproject-working-tree")) {
> +				const char *superproject = get_superproject_working_tree();
> +				if (superproject)
> +					puts(superproject);
> +				continue;
> +			}

Returning the exact string of the path from the API function is
absolutely the right thing. I however have to wonder if rev-parse
need to do the c-quoting unless it is told to show pathnames in its
output without quoting (perhaps with "-z").  Paths from "rev-parse"
(like "--git-dir", "--show-toplevel", etc.) already are excempt from
the usual quoting rules, so doing puts() and nothing else is fine to
be consistent with the existing practice, but in the longer term, I
am sure we would need to revisit so that scripts can handle paths
with funny characters sensibly, but that would be a different topic
if existing ones like "--git-dir" are already unsafe.

>  			if (!strcmp(arg, "--show-prefix")) {
>  				if (prefix)
>  					puts(prefix);
> diff --git a/submodule.c b/submodule.c
> index 3b98766a6b..06473d3646 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1514,3 +1514,86 @@ void absorb_git_dir_into_superproject(const char *prefix,
>  		strbuf_release(&sb);
>  	}
>  }
> +
> +const char *get_superproject_working_tree(void)
> +{
> +...
> +	argv_array_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
> +			"ls-files", "--stage", "--full-name", "--", subpath, NULL);
> +	strbuf_reset(&sb);
> +...
> +	if (starts_with(sb.buf, "160000")) {
> +		int super_sub_len;
> +		int cwd_len = strlen(cwd);
> +		char *super_sub, *super_wt;
> +
> +		/*
> +		 * There is a superproject having this repo as a submodule.
> +		 * The format is <mode> SP <hash> SP <stage> TAB <full name> LF,
> +		 * First remove LF, then skip up to \t.
> +		 */

Looks more or less right but invoke "ls-files -z" and reading the \0
delimited output would be easier; otherwise you would have to worry
about c-unquoting the pathname when the submodule is bound at a path
with funny character (like a double-quote) in it.

[PATCHv3] rev-parse: add --show-superproject-working-tree

[Stefan Beller]

In some situations it is useful to know if the given repository
is a submodule of another repository.

Add the flag --show-superproject-working-tree to git-rev-parse
to make it easy to find out if there is a superproject.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
> Looks more or less right but invoke "ls-files -z" and reading the \0
> delimited output would be easier;

done

> otherwise you would have to worry
> about c-unquoting the pathname when the submodule is bound at a path
> with funny character (like a double-quote) in it.
 
paths from "rev-parse" (like "--git-dir", "--show-toplevel", etc.) already
are excempt from quoting rules, apparently.

  $ git rev-parse --show-toplevel
/usr/local/google/home/sbeller/OSS/git/t/trash directory.t1500-rev-parse


interdiff to v2:
  #diff --git a/submodule.c b/submodule.c
  #index 06473d3646..bb405653fd 100644
  #--- a/submodule.c
  #+++ b/submodule.c
  #@@ -1543,7 +1543,8 @@ const char *get_superproject_working_tree(void)
  # 	argv_array_pop(&cp.env_array);
  # 
  # 	argv_array_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
  #-			"ls-files", "--stage", "--full-name", "--", subpath, NULL);
  #+			"ls-files", "-z", "--stage", "--full-name", "--",
  #+			subpath, NULL);
  # 	strbuf_reset(&sb);
  # 
  # 	cp.no_stdin = 1;
  #@@ -1564,13 +1565,12 @@ const char *get_superproject_working_tree(void)
  # 
  # 		/*
  # 		 * There is a superproject having this repo as a submodule.
  #-		 * The format is <mode> SP <hash> SP <stage> TAB <full name> LF,
  #-		 * First remove LF, then skip up to \t.
  #+		 * The format is <mode> SP <hash> SP <stage> TAB <full name> \0,
  #+		 * We're only interested in the name after the tab.
  # 		 */
  #-		strbuf_rtrim(&sb);
  # 		super_sub = strchr(sb.buf, '\t') + 1;
  #+		super_sub_len = sb.buf + sb.len - super_sub - 1;
  # 
  #-		super_sub_len = sb.buf + sb.len - super_sub;
  # 		if (super_sub_len > cwd_len ||
  # 		    strcmp(&cwd[cwd_len - super_sub_len], super_sub))
  # 			die (_("BUG: returned path string doesn't match cwd?"));
  #@@ -1579,7 +1579,6 @@ const char *get_superproject_working_tree(void)
  # 		super_wt[cwd_len - super_sub_len] = '\0';
  # 
  # 		ret = real_path(super_wt);
  #-
  # 		free(super_wt);
  # 	}
  # 	strbuf_release(&sb);
  
  Thanks,
  Stefan
  

 Documentation/git-rev-parse.txt |  5 +++
 builtin/rev-parse.c             |  7 ++++
 submodule.c                     | 82 +++++++++++++++++++++++++++++++++++++++++
 submodule.h                     |  8 ++++
 t/t1500-rev-parse.sh            | 14 +++++++
 5 files changed, 116 insertions(+)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 91c02b8c85..b841bad7c7 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -261,6 +261,11 @@ print a message to stderr and exit with nonzero status.
 --show-toplevel::
 	Show the absolute path of the top-level directory.
 
+--show-superproject-working-tree
+	Show the absolute path of the top-level directory of
+	the superproject. A superproject is a repository that records
+	this repository as a submodule.
+
 --shared-index-path::
 	Show the path to the shared index file in split index mode, or
 	empty if not in split-index mode.
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index e08677e559..2549643267 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -12,6 +12,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "split-index.h"
+#include "submodule.h"
 
 #define DO_REVS		1
 #define DO_NOREV	2
@@ -779,6 +780,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 					puts(work_tree);
 				continue;
 			}
+			if (!strcmp(arg, "--show-superproject-working-tree")) {
+				const char *superproject = get_superproject_working_tree();
+				if (superproject)
+					puts(superproject);
+				continue;
+			}
 			if (!strcmp(arg, "--show-prefix")) {
 				if (prefix)
 					puts(prefix);
diff --git a/submodule.c b/submodule.c
index 3b98766a6b..bb405653fd 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1514,3 +1514,85 @@ void absorb_git_dir_into_superproject(const char *prefix,
 		strbuf_release(&sb);
 	}
 }
+
+const char *get_superproject_working_tree(void)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf sb = STRBUF_INIT;
+	const char *one_up = real_path_if_valid("../");
+	const char *cwd = xgetcwd();
+	const char *ret = NULL;
+	const char *subpath;
+	int code;
+	ssize_t len;
+
+	if (!is_inside_work_tree())
+		/*
+		 * FIXME:
+		 * We might have a superproject, but it is harder
+		 * to determine.
+		 */
+		return NULL;
+
+	if (!one_up)
+		return NULL;
+
+	subpath = relative_path(cwd, one_up, &sb);
+
+	prepare_submodule_repo_env(&cp.env_array);
+	argv_array_pop(&cp.env_array);
+
+	argv_array_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
+			"ls-files", "-z", "--stage", "--full-name", "--",
+			subpath, NULL);
+	strbuf_reset(&sb);
+
+	cp.no_stdin = 1;
+	cp.no_stderr = 1;
+	cp.out = -1;
+	cp.git_cmd = 1;
+
+	if (start_command(&cp))
+		die(_("could not start ls-files in .."));
+
+	len = strbuf_read(&sb, cp.out, PATH_MAX);
+	close(cp.out);
+
+	if (starts_with(sb.buf, "160000")) {
+		int super_sub_len;
+		int cwd_len = strlen(cwd);
+		char *super_sub, *super_wt;
+
+		/*
+		 * There is a superproject having this repo as a submodule.
+		 * The format is <mode> SP <hash> SP <stage> TAB <full name> \0,
+		 * We're only interested in the name after the tab.
+		 */
+		super_sub = strchr(sb.buf, '\t') + 1;
+		super_sub_len = sb.buf + sb.len - super_sub - 1;
+
+		if (super_sub_len > cwd_len ||
+		    strcmp(&cwd[cwd_len - super_sub_len], super_sub))
+			die (_("BUG: returned path string doesn't match cwd?"));
+
+		super_wt = xstrdup(cwd);
+		super_wt[cwd_len - super_sub_len] = '\0';
+
+		ret = real_path(super_wt);
+		free(super_wt);
+	}
+	strbuf_release(&sb);
+
+	code = finish_command(&cp);
+
+	if (code == 128)
+		/* '../' is not a git repository */
+		return NULL;
+	if (code == 0 && len == 0)
+		/* There is an unrelated git repository at '../' */
+		return NULL;
+	if (code)
+		die(_("ls-tree returned unexpected return code %d"), code);
+
+	return ret;
+}
diff --git a/submodule.h b/submodule.h
index 05ab674f06..c8a0c9cb29 100644
--- a/submodule.h
+++ b/submodule.h
@@ -93,4 +93,12 @@ extern void prepare_submodule_repo_env(struct argv_array *out);
 extern void absorb_git_dir_into_superproject(const char *prefix,
 					     const char *path,
 					     unsigned flags);
+
+/*
+ * Return the absolute path of the working tree of the superproject, which this
+ * project is a submodule of. If this repository is not a submodule of
+ * another repository, return NULL.
+ */
+extern const char *get_superproject_working_tree(void);
+
 #endif
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 9ed8b8ccba..03d3c7f6d6 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -116,4 +116,18 @@ test_expect_success 'git-path inside sub-dir' '
 	test_cmp expect actual
 '
 
+test_expect_success 'showing the superproject correctly' '
+	git rev-parse --show-superproject-working-tree >out &&
+	test_must_be_empty out &&
+
+	test_create_repo super &&
+	test_commit -C super test_commit &&
+	test_create_repo sub &&
+	test_commit -C sub test_commit &&
+	git -C super submodule add ../sub dir/sub &&
+	echo $(pwd)/super >expect  &&
+	git -C super/dir/sub rev-parse --show-superproject-working-tree >out &&
+	test_cmp expect out
+'
+
 test_done
-- 
2.12.0.190.g6e60aba09d.dirty

Re: [PATCHv3] rev-parse: add --show-superproject-working-tree

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +--show-superproject-working-tree
> +	Show the absolute path of the top-level directory of
> +	the superproject. A superproject is a repository that records
> +	this repository as a submodule.

The top-level directory of the superproject's working tree?

Describe error conditions, like "you get an error if there is no
such project"?

> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index e08677e559..2549643267 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -12,6 +12,7 @@
>  #include "diff.h"
>  #include "revision.h"
>  #include "split-index.h"
> +#include "submodule.h"
>  
>  #define DO_REVS		1
>  #define DO_NOREV	2
> @@ -779,6 +780,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  					puts(work_tree);
>  				continue;
>  			}
> +			if (!strcmp(arg, "--show-superproject-working-tree")) {
> +				const char *superproject = get_superproject_working_tree();
> +				if (superproject)
> +					puts(superproject);
> +				continue;

When is superproject NULL?  Shouldn't we die with "there is no
superproject that uses us as a submodule"?

I can accept "it is not an error to ask for the root of the
superproject's working tree when we do not have any---that is one
way to ask if we have a superproject or not".  But if that is the
case, the code can stay the same, but the documentation should say
so, something like...

	Show the absolute path of the root of the superproject's
	working tree (if exists) that uses the current repository as
	its submodule.  Outputs nothing if the current repository is
	not used as a submodule by any project.

[PATCHv4] rev-parse: add --show-superproject-working-tree

[Stefan Beller]

In some situations it is useful to know if the given repository
is a submodule of another repository.

Add the flag --show-superproject-working-tree to git-rev-parse
to make it easy to find out if there is a superproject. When no
superproject exists, the output will be empty.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

> I can accept "it is not an error to ask for the root of the
> superproject's working tree when we do not have any---that is one
> way to ask if we have a superproject or not".  But if that is the
> case, the code can stay the same, but the documentation should say
> so, something like...

 *this patch*
 
* Documentation shamelessly stolen from Junio.

Thanks,
Stefan

 Documentation/git-rev-parse.txt |  6 +++
 builtin/rev-parse.c             |  7 ++++
 submodule.c                     | 82 +++++++++++++++++++++++++++++++++++++++++
 submodule.h                     |  8 ++++
 t/t1500-rev-parse.sh            | 14 +++++++
 5 files changed, 117 insertions(+)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 91c02b8c85..c40c470448 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -261,6 +261,12 @@ print a message to stderr and exit with nonzero status.
 --show-toplevel::
 	Show the absolute path of the top-level directory.
 
+--show-superproject-working-tree
+	Show the absolute path of the root of the superproject's
+	working tree (if exists) that uses the current repository as
+	its submodule.  Outputs nothing if the current repository is
+	not used as a submodule by any project.
+
 --shared-index-path::
 	Show the path to the shared index file in split index mode, or
 	empty if not in split-index mode.
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index e08677e559..2549643267 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -12,6 +12,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "split-index.h"
+#include "submodule.h"
 
 #define DO_REVS		1
 #define DO_NOREV	2
@@ -779,6 +780,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 					puts(work_tree);
 				continue;
 			}
+			if (!strcmp(arg, "--show-superproject-working-tree")) {
+				const char *superproject = get_superproject_working_tree();
+				if (superproject)
+					puts(superproject);
+				continue;
+			}
 			if (!strcmp(arg, "--show-prefix")) {
 				if (prefix)
 					puts(prefix);
diff --git a/submodule.c b/submodule.c
index 3b98766a6b..bb405653fd 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1514,3 +1514,85 @@ void absorb_git_dir_into_superproject(const char *prefix,
 		strbuf_release(&sb);
 	}
 }
+
+const char *get_superproject_working_tree(void)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf sb = STRBUF_INIT;
+	const char *one_up = real_path_if_valid("../");
+	const char *cwd = xgetcwd();
+	const char *ret = NULL;
+	const char *subpath;
+	int code;
+	ssize_t len;
+
+	if (!is_inside_work_tree())
+		/*
+		 * FIXME:
+		 * We might have a superproject, but it is harder
+		 * to determine.
+		 */
+		return NULL;
+
+	if (!one_up)
+		return NULL;
+
+	subpath = relative_path(cwd, one_up, &sb);
+
+	prepare_submodule_repo_env(&cp.env_array);
+	argv_array_pop(&cp.env_array);
+
+	argv_array_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
+			"ls-files", "-z", "--stage", "--full-name", "--",
+			subpath, NULL);
+	strbuf_reset(&sb);
+
+	cp.no_stdin = 1;
+	cp.no_stderr = 1;
+	cp.out = -1;
+	cp.git_cmd = 1;
+
+	if (start_command(&cp))
+		die(_("could not start ls-files in .."));
+
+	len = strbuf_read(&sb, cp.out, PATH_MAX);
+	close(cp.out);
+
+	if (starts_with(sb.buf, "160000")) {
+		int super_sub_len;
+		int cwd_len = strlen(cwd);
+		char *super_sub, *super_wt;
+
+		/*
+		 * There is a superproject having this repo as a submodule.
+		 * The format is <mode> SP <hash> SP <stage> TAB <full name> \0,
+		 * We're only interested in the name after the tab.
+		 */
+		super_sub = strchr(sb.buf, '\t') + 1;
+		super_sub_len = sb.buf + sb.len - super_sub - 1;
+
+		if (super_sub_len > cwd_len ||
+		    strcmp(&cwd[cwd_len - super_sub_len], super_sub))
+			die (_("BUG: returned path string doesn't match cwd?"));
+
+		super_wt = xstrdup(cwd);
+		super_wt[cwd_len - super_sub_len] = '\0';
+
+		ret = real_path(super_wt);
+		free(super_wt);
+	}
+	strbuf_release(&sb);
+
+	code = finish_command(&cp);
+
+	if (code == 128)
+		/* '../' is not a git repository */
+		return NULL;
+	if (code == 0 && len == 0)
+		/* There is an unrelated git repository at '../' */
+		return NULL;
+	if (code)
+		die(_("ls-tree returned unexpected return code %d"), code);
+
+	return ret;
+}
diff --git a/submodule.h b/submodule.h
index 05ab674f06..c8a0c9cb29 100644
--- a/submodule.h
+++ b/submodule.h
@@ -93,4 +93,12 @@ extern void prepare_submodule_repo_env(struct argv_array *out);
 extern void absorb_git_dir_into_superproject(const char *prefix,
 					     const char *path,
 					     unsigned flags);
+
+/*
+ * Return the absolute path of the working tree of the superproject, which this
+ * project is a submodule of. If this repository is not a submodule of
+ * another repository, return NULL.
+ */
+extern const char *get_superproject_working_tree(void);
+
 #endif
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 9ed8b8ccba..03d3c7f6d6 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -116,4 +116,18 @@ test_expect_success 'git-path inside sub-dir' '
 	test_cmp expect actual
 '
 
+test_expect_success 'showing the superproject correctly' '
+	git rev-parse --show-superproject-working-tree >out &&
+	test_must_be_empty out &&
+
+	test_create_repo super &&
+	test_commit -C super test_commit &&
+	test_create_repo sub &&
+	test_commit -C sub test_commit &&
+	git -C super submodule add ../sub dir/sub &&
+	echo $(pwd)/super >expect  &&
+	git -C super/dir/sub rev-parse --show-superproject-working-tree >out &&
+	test_cmp expect out
+'
+
 test_done
-- 
2.12.0.190.g6e60aba09d.dirty

Re: [PATCHv4] rev-parse: add --show-superproject-working-tree

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>  *this patch*
>  
> * Documentation shamelessly stolen from Junio.

You stole it, and then ...

> +test_expect_success 'showing the superproject correctly' '
> +	git rev-parse --show-superproject-working-tree >out &&
> +	test_must_be_empty out &&

... made sure that the newly documented behaviour is tested as a
feature.  Good.

> +	test_create_repo super &&
> +	test_commit -C super test_commit &&
> +	test_create_repo sub &&
> +	test_commit -C sub test_commit &&
> +	git -C super submodule add ../sub dir/sub &&
> +	echo $(pwd)/super >expect  &&
> +	git -C super/dir/sub rev-parse --show-superproject-working-tree >out &&
> +	test_cmp expect out
> +'
> +

Re: [PATCHv4] rev-parse: add --show-superproject-working-tree

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> Add the flag --show-superproject-working-tree to git-rev-parse
> to make it easy to find out if there is a superproject. When no
> superproject exists, the output will be empty.

I'm a little late for this, but think it's better to comment late than
never.

After this patch, git rev-parse has two options relating to the working
tree:

	git rev-parse --is-inside-work-tree
	git rev-parse --show-superproject-working-tree

Is this inconsistency necessary?  Now that there is a "git worktree"
command that allows making a second working tree for the same
repository, I would have thought we had standardized on work tree as
the name in UI.

Thoughts?
Jonathan

[PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Stefan Beller]

If I recall correctly, "worktree" is the feature/command, and
"working tree" is an instance in the file system, even when you only
have one working tree.

Using that mental model, the documentation for 'git worktree list'
clearly talks about working trees, so fix it.

Reflow the lines as well.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

 Duy, this is my instant-make-a-apatch-to-fix-the-world reaction
 in reply to Jonathan.
 
 Thanks,
 Stefan

 Documentation/git-worktree.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 553cf8413f..e080007ed0 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -58,10 +58,10 @@ as if `-b $(basename <path>)` was specified.
 
 list::
 
-List details of each worktree.  The main worktree is listed first, followed by
-each of the linked worktrees.  The output details include if the worktree is
-bare, the revision currently checked out, and the branch currently checked out
-(or 'detached HEAD' if none).
+List details of each working tree.  The main working tree is listed first,
+followed by each of the linked working trees.  The output details include if
+the working tree is bare, the revision currently checked out, and the branch
+currently checked out (or 'detached HEAD' if none).
 
 lock::
 
-- 
2.12.0.306.g4a9b9b32d4.dirty

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> If I recall correctly, "worktree" is the feature/command, and
> "working tree" is an instance in the file system, even when you only
> have one working tree.

I'm not sure I agree with this distinction.  "worktree" is just a
short name for "working tree".

Unfortunately gitglossary(7) doesn't make this clear at all --- it
uses the term worktree a few times (and appears to mean "working tree"
when it does --- e.g.

	Pathspecs are used on the command line of [...] and many other
	commands to limit the scope of operations to some subset of
	the tree or worktree.

) but never defines it.

[...]
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -58,10 +58,10 @@ as if `-b $(basename <path>)` was specified.
>  
>  list::
>  
> -List details of each worktree.  The main worktree is listed first, followed by
> -each of the linked worktrees.  The output details include if the worktree is
> -bare, the revision currently checked out, and the branch currently checked out
> -(or 'detached HEAD' if none).
> +List details of each working tree.  The main working tree is listed first,
> +followed by each of the linked working trees.  The output details include if
> +the working tree is bare, the revision currently checked out, and the branch
> +currently checked out (or 'detached HEAD' if none).

The patch itself looks good.

Thanks,
Jonathan

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Stefan Beller]

On Fri, Mar 17, 2017 at 3:55 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Stefan Beller wrote:
>
>> If I recall correctly, "worktree" is the feature/command, and
>> "working tree" is an instance in the file system, even when you only
>> have one working tree.
>
> I'm not sure I agree with this distinction.  "worktree" is just a
> short name for "working tree".
>
> Unfortunately gitglossary(7) doesn't make this clear at all --- it
> uses the term worktree a few times (and appears to mean "working tree"
> when it does --- e.g.
>
>         Pathspecs are used on the command line of [...] and many other
>         commands to limit the scope of operations to some subset of
>         the tree or worktree.
>
> ) but never defines it.
>

So maybe it's time to look for volunteers for a cleanup patch. ;)
I tried finding the discussion on worktree vs "working tree"
and did not succeed. :/

Maybe Duy has a better memory there.

Thanks,
Stefan

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> Unfortunately gitglossary(7) doesn't make this clear at all --- it
>> uses the term worktree a few times (and appears to mean "working tree"
>> when it does --- e.g.
>>
>>         Pathspecs are used on the command line of [...] and many other
>>         commands to limit the scope of operations to some subset of
>>         the tree or worktree.
>>
>> ) but never defines it.
>
> So maybe it's time to look for volunteers for a cleanup patch. ;)
> I tried finding the discussion on worktree vs "working tree"
> and did not succeed. :/

A rough rule of thumb.  Anything that was written before 2014 or
back when nobody knew df0b6cfb ("worktree: new place for "git prune
--worktrees"", 2015-06-29) was coming that says "worktree" is using
the term to refer to what we call "working tree" these days.

What Jonathan quoted above came from 3bd2bcfa ("glossary: define
pathspec", 2010-12-15) and certainly predates the one that replaced
contrib/workdir, so we should update it to read "working tree".

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I'm not sure I agree with this distinction.  "worktree" is just a
> short name for "working tree".

That is probably primarily my fault.  Loooooooooong before the "git
worktree" that is meant to replace the "workdir" thing in contrib/
was invented, some people said "working tree" while some other
people called the same thing "worktree", simply because it is
shorter.  Unfortunately I was among the latter, and wrote quite a
lot of documents and in-code comments, so it got stuck.  But later
we somehow ended up deciding on using "working tree" to refer to the
hierarchy of files that are checked out from a $GIT_DIR.

So if you see a message in the list archive, or an in-tree document
or in-code comment that was written way before that "let's call the
thing non-bare repositories have 'working tree'" decision was made
and have not been updated since, you are likely to find "worktree"
used to refer to "working tree".

And then relatively recently, Duy's feature started calling itself
"worktree".  The mention of "worktree" in git-worktree.txt, by
definition, is a lot newer than the "let's call the thing a non-bare
repository has 'working tree'" and was written after the word
"worktree" gained the new meaning, and it refers to that "different
things that can be referred to by setting your $GIT_DIR at them".

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> -List details of each worktree.  The main worktree is listed first, followed by
> -each of the linked worktrees.  The output details include if the worktree is
> -bare, the revision currently checked out, and the branch currently checked out
> -(or 'detached HEAD' if none).
> +List details of each working tree.  The main working tree is listed first,
> +followed by each of the linked working trees.  The output details include if
> +the working tree is bare, the revision currently checked out, and the branch
> +currently checked out (or 'detached HEAD' if none).

I do not think this is correct.

Think of a "worktree" something that roughly corresponds to
different location you can refer to with $GIT_DIR.

A $GIT_DIR may be a true repository.  Or it may be borrowing many of
the things from its primary repository.  Even though "git worktree"
Porcelain may not currently be equipped to create a "bare" $GIT_DIR,
there is no fundamental reason that a secondary "worktree" must have
a working tree, i.e. a "worktree" could be a "bare" one (it would be
the first use of per-worktree config; a secondary worktree that is a
bare can be made by borrowing from the primary worktree that has a
working tree).

Now, what does "git worktree list" enumearate?  It does not list
"working trees"; its output are list of things, each of which you
could point at with $GIT_DIR and work with.

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Stefan Beller]

On Fri, Mar 17, 2017 at 6:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> -List details of each worktree.  The main worktree is listed first, followed by
>> -each of the linked worktrees.  The output details include if the worktree is
>> -bare, the revision currently checked out, and the branch currently checked out
>> -(or 'detached HEAD' if none).
>> +List details of each working tree.  The main working tree is listed first,
>> +followed by each of the linked working trees.  The output details include if
>> +the working tree is bare, the revision currently checked out, and the branch
>> +currently checked out (or 'detached HEAD' if none).
>
> I do not think this is correct.
>
> Think of a "worktree" something that roughly corresponds to
> different location you can refer to with $GIT_DIR.
>
> A $GIT_DIR may be a true repository.  Or it may be borrowing many of
> the things from its primary repository.  Even though "git worktree"
> Porcelain may not currently be equipped to create a "bare" $GIT_DIR,
> there is no fundamental reason that a secondary "worktree" must have
> a working tree, i.e. a "worktree" could be a "bare" one (it would be
> the first use of per-worktree config; a secondary worktree that is a
> bare can be made by borrowing from the primary worktree that has a
> working tree).

While it may be true that you can have bare worktrees; I would question
why anyone wants to do this, as the only thing it provides is an
additional HEAD (plus its reflog).

> Now, what does "git worktree list" enumearate?  It does not list
> "working trees"; its output are list of things, each of which you
> could point at with $GIT_DIR and work with.

$ git worktree list
/.../git                   0b47ebba82 [status-short]
/.../git_origin_master     5588dbffbd (detached HEAD)
/.../submodule_remote_dot  3d9025bd69 (detached HEAD)

This looks to me as if it is listing actual working trees. But looking
at the actual code, I confirm we list worktrees here, even bare
worktrees are taken care of.

That said, "worktree prune" (as well as (un)lock) are operating
on "worktrees" as you defined above:

$ git worktree add test
$ git worktree list
# see the "test" worktree listed by its working tree
rm -rf test # remove the working tree attached to the test worktree
$ git worktree list
# see the "test" worktree listed by its working tree, still there
$ git worktree prune
$ git worktree list
# the "test" worktree is gone.

So maybe we need a patch that is s/working tree/worktree/
in the prune section?

Thanks,
Stefan

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> While it may be true that you can have bare worktrees; I would question
> why anyone wants to do this, as the only thing it provides is an
> additional HEAD (plus its reflog).

A more plausible situation is you start with a bare one as the
primary and used to make local clones to do your work in the world
before "git worktree".  It would be a natural extension to your
workflow to instead create worktrees of of that bare one as the
primary worktree with secondaries with working trees.

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Jonathan Nieder]

Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:

>> While it may be true that you can have bare worktrees; I would question
>> why anyone wants to do this, as the only thing it provides is an
>> additional HEAD (plus its reflog).
>
> A more plausible situation is you start with a bare one as the
> primary and used to make local clones to do your work in the world
> before "git worktree".  It would be a natural extension to your
> workflow to instead create worktrees of of that bare one as the
> primary worktree with secondaries with working trees.

For what it's worth, this conversation makes me think it was a mistake
to call this construct a worktree.

It's fine for the command to have one name and the documentation to
use a longer, clearer name to explain it.  What should that longer,
clearer name be?

Thanks,
Jonathan

[PATCH 0/2] use "working trees" instead of "worktree" in our API

[Stefan Beller]

> For what it's worth, this conversation makes me think it was a mistake
> to call this construct a worktree.

So the way forward is to purge the use of "worktree" meaning actual working trees?

Thanks,
Stefan

Stefan Beller (2):
  git.c: introduce --working-tree superseding --work-tree
  revparse: introduce --is-inside-working-tree

 Documentation/git-rev-parse.txt |  4 ++--
 Documentation/git.txt           | 12 ++++++------
 builtin/rev-parse.c             |  3 ++-
 git.c                           |  5 +++--
 4 files changed, 13 insertions(+), 11 deletions(-)

-- 
2.12.0.306.g4a9b9b32d4.dirty

[PATCH 1/2] git.c: introduce --working-tree superseding --work-tree

[Stefan Beller]

Discussion on bf0231c661 (rev-parse: add --show-superproject-working-tree,
2017-03-08) pointed out we are inconsistent with the naming of "working
tree" and "worktree" even in user facing commands and documentation[1].

Introduce the new --working-tree option, which is the same as
--work-tree. As --work-tree is considered slightly incorrect[2], stop
mentioning it in the documentation.  But we need to keep its functionality
as it is plumbing.

An alternative was considered off list to rename the newly added option
'--show-superproject-working-tree' by dropping the part mentioning the
working tree to side step this discussion. However we need to make sure
that option still refers to the working tree, as a new option
'show-superproject-git-dir' might be considered useful in the future,
and we do not want to take the canonical '--show-superproject' now.

[1] https://public-inbox.org/git/20170317222842.GP26789@aiede.mtv.corp.google.com/
[2] https://public-inbox.org/git/xmqqo9wy4hxa.fsf@gitster.mtv.corp.google.com/

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git.txt | 12 ++++++------
 git.c                 |  5 +++--
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index df0941d456..763f3b5563 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git' [--version] [--help] [-C <path>] [-c <name>=<value>]
     [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
     [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
-    [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
+    [--git-dir=<path>] [--working-tree=<path>] [--namespace=<name>]
     [--super-prefix=<path>]
     <command> [<args>]
 
@@ -551,12 +551,12 @@ help ...`.
 	<path>`.
 +
 This option affects options that expect path name like `--git-dir` and
-`--work-tree` in that their interpretations of the path names would be
+`--working-tree` in that their interpretations of the path names would be
 made relative to the working directory caused by the `-C` option. For
 example the following invocations are equivalent:
 
-    git --git-dir=a.git --work-tree=b -C c status
-    git --git-dir=c/a.git --work-tree=c/b status
+    git --git-dir=a.git --working-tree=b -C c status
+    git --git-dir=c/a.git --working-tree=c/b status
 
 -c <name>=<value>::
 	Pass a configuration parameter to the command. The value
@@ -602,7 +602,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string.
 	setting the `GIT_DIR` environment variable. It can be an absolute
 	path or relative path to current working directory.
 
---work-tree=<path>::
+--working-tree=<path>::
 	Set the path to the working tree. It can be an absolute path
 	or a path relative to the current working directory.
 	This can also be controlled by setting the GIT_WORK_TREE
@@ -892,7 +892,7 @@ Git so take care if using a foreign front-end.
 
 `GIT_WORK_TREE`::
 	Set the path to the root of the working tree.
-	This can also be controlled by the `--work-tree` command-line
+	This can also be controlled by the `--working-tree` command-line
 	option and the core.worktree configuration variable.
 
 `GIT_NAMESPACE`::
diff --git a/git.c b/git.c
index 33f52acbcc..a76ff97232 100644
--- a/git.c
+++ b/git.c
@@ -149,9 +149,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			setenv(GIT_NAMESPACE_ENVIRONMENT, cmd, 1);
 			if (envchanged)
 				*envchanged = 1;
-		} else if (!strcmp(cmd, "--work-tree")) {
+		} else if (!strcmp(cmd, "--work-tree") ||
+			   !strcmp(cmd, "--working-tree")) {
 			if (*argc < 2) {
-				fprintf(stderr, "No directory given for --work-tree.\n" );
+				fprintf(stderr, "No directory given for %s.\n", cmd);
 				usage(git_usage_string);
 			}
 			setenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1);
-- 
2.12.0.306.g4a9b9b32d4.dirty

Re: [PATCH 1/2] git.c: introduce --working-tree superseding --work-tree

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  Documentation/git.txt | 12 ++++++------
>  git.c                 |  5 +++--
>  2 files changed, 9 insertions(+), 8 deletions(-)

I think this is a step in the right direction.  Thanks for writing it.

Nits:

- tests are still using --work-tree --- this patch didn't add any tests
  for --working-tree.  If --working-tree is what we prefer, it may make
  sense to update tests to use --working-tree and add a test or two to
  make sure the existing --work-tree synonym still works.

- this patch updated the argv[i] == "--work-tree" case but forgot to
  update the argv[i].has_prefix("--work-tree=") case

- since this is a feature used for scripting, I don't think we can
  pretend the name change never happened.  We think we need to
  document both option names and say what version introduced the new
  one so script authors can make an informed decision about which to
  use.  Later we can make the --work-tree synonym more obscure, but in
  the short term I suspect it is what most script authors will still
  want to use.

> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
[...]
> @@ -892,7 +892,7 @@ Git so take care if using a foreign front-end.
>  
>  `GIT_WORK_TREE`::
>  	Set the path to the root of the working tree.
> -	This can also be controlled by the `--work-tree` command-line
> +	This can also be controlled by the `--working-tree` command-line
>  	option and the core.worktree configuration variable.

I suspect we don't want to rename GIT_WORK_TREE --- it's not
user-facing in the same way as --work-tree is, scripts make direct use
of it (and they unset it when appropriate!), and dealing with the
permutations of what to do if some subset of environment variables is
set seems very complicated.

For comparison, core.worktree is user-facing.  Is it also in scope for
this change?

Thanks and hope that helps,
Jonathan

[PATCH 2/2] revparse: introduce --is-inside-working-tree

[Stefan Beller]

This behaves the same as 'is-inside-worktree' and supersedes it.
See prior patch for discussion of "working tree" vs. "worktree"

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-rev-parse.txt | 4 ++--
 builtin/rev-parse.c             | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index c40c470448..55ee3bde55 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -228,8 +228,8 @@ print a message to stderr and exit with nonzero status.
 	When the current working directory is below the repository
 	directory print "true", otherwise "false".
 
---is-inside-work-tree::
-	When the current working directory is inside the work tree of the
+--is-inside-working-tree::
+	When the current working directory is inside the working tree of the
 	repository print "true", otherwise "false".
 
 --is-bare-repository::
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 2549643267..04da518058 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -851,7 +851,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 						: "false");
 				continue;
 			}
-			if (!strcmp(arg, "--is-inside-work-tree")) {
+			if (!strcmp(arg, "--is-inside-work-tree") ||
+			    !strcmp(arg, "--is-inside-working-tree")) {
 				printf("%s\n", is_inside_work_tree() ? "true"
 						: "false");
 				continue;
-- 
2.12.0.306.g4a9b9b32d4.dirty

Re: [PATCH 2/2] revparse: introduce --is-inside-working-tree

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> This behaves the same as 'is-inside-worktree' and supersedes it.
> See prior patch for discussion of "working tree" vs. "worktree"
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  Documentation/git-rev-parse.txt | 4 ++--
>  builtin/rev-parse.c             | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)

This is less invasive than the previous patch and can probably stand
alone.  Some of the same nits apply:

* tests?

* documentation would need to warn people that this option is new, for
  now. In fact it's even tempting to make --is-inside-working-tree
  the hidden/discouraged one for a while, until script authors can
  count on git having had it available for a while, and only then
  encourage its use.

Thanks and hope that helps,
Jonathan

Re: [PATCH 0/2] use "working trees" instead of "worktree" in our API

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> For what it's worth, this conversation makes me think it was a mistake
>> to call this construct a worktree.
>
> So the way forward is to purge the use of "worktree" meaning actual working trees?

GIT_WORK_TREE environment would have be a victim of this clean-up,
so is setup_work_tree(), together with numerous in-code comment
about "work tree".

While I would say that we would certainly pick one and stick to it
if we were inventing Git from scratch today and just started caring
the distinction between core.bare and not, I am not sure how far we
would want to go, and what's the expected payoff of doing this
clean-up would be, given that we are starting from today's world.

So, I dunno.  If the response and list concensus to Jonathan's
earlier comment came up with a better name for the newer "worktree"
concept, we may not have to even worry about this and instead can
just declare "these are used interchangeably".

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Duy Nguyen]

On Tue, Mar 21, 2017 at 1:50 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Junio C Hamano wrote:
>> Stefan Beller <sbeller@google.com> writes:
>
>>> While it may be true that you can have bare worktrees; I would question
>>> why anyone wants to do this, as the only thing it provides is an
>>> additional HEAD (plus its reflog).
>>
>> A more plausible situation is you start with a bare one as the
>> primary and used to make local clones to do your work in the world
>> before "git worktree".  It would be a natural extension to your
>> workflow to instead create worktrees of of that bare one as the
>> primary worktree with secondaries with working trees.
>
> For what it's worth, this conversation makes me think it was a mistake
> to call this construct a worktree.

For the record, I am totally confused with Junio's last line, with two
"with"s, "worktree" and "working trees" in the same phrase :D

> It's fine for the command to have one name and the documentation to
> use a longer, clearer name to explain it.  What should that longer,
> clearer name be?

No comments from me. I'll let you know that if Eric (or Junio?) didn't
stop me, we would have had $GIT_DIR/repos now instead of
$GIT_DIR/worktrees, just some extra confusion toppings.
-- 
Duy

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Tue, Mar 21, 2017 at 1:50 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Junio C Hamano wrote:
>>> Stefan Beller <sbeller@google.com> writes:
>>
>>>> While it may be true that you can have bare worktrees; I would question
>>>> why anyone wants to do this, as the only thing it provides is an
>>>> additional HEAD (plus its reflog).
>>>
>>> A more plausible situation is you start with a bare one as the
>>> primary and used to make local clones to do your work in the world
>>> before "git worktree".  It would be a natural extension to your
>>> workflow to instead create worktrees of of that bare one as the
>>> primary worktree with secondaries with working trees.
>>
>> For what it's worth, this conversation makes me think it was a mistake
>> to call this construct a worktree.
>
> For the record, I am totally confused with Junio's last line, with two
> "with"s, "worktree" and "working trees" in the same phrase :D

In case this wasn't just a tangential note, what I meant was:

 - In the old world, you may have had a single bare repository and
   then made clones, each of which has a working tree (i.e. non-bare
   clones), and worked inside these clones.

 - In the "git worktree" world, you can start from that same single
   bare repository, but instead of cloning it, use "git worktree" to
   create "worktree"s, each of which has a working tree, and work
   inside these "worktree"s.

and the latter would be a natural extension to the workflow the
former wanted to use.

>> It's fine for the command to have one name and the documentation to
>> use a longer, clearer name to explain it.  What should that longer,
>> clearer name be?
>
> No comments from me. I'll let you know that if Eric (or Junio?) didn't
> stop me, we would have had $GIT_DIR/repos now instead of
> $GIT_DIR/worktrees, just some extra confusion toppings.

I forgot about that part of the history, but you are saying you
wanted to call these "repos", not "worktrees"?  I can see why
somebody (or me?) would stop that by fearing "repo" is a bit too
confusing with a "repository", in the same way that we are now
realizing that "worktree" is too similar to an old synonym we used
to call "working tree".

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 21.03.2017 16:48:
> Duy Nguyen <pclouds@gmail.com> writes:
> 
>> On Tue, Mar 21, 2017 at 1:50 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> Junio C Hamano wrote:
>>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>>> While it may be true that you can have bare worktrees; I would question
>>>>> why anyone wants to do this, as the only thing it provides is an
>>>>> additional HEAD (plus its reflog).
>>>>
>>>> A more plausible situation is you start with a bare one as the
>>>> primary and used to make local clones to do your work in the world
>>>> before "git worktree".  It would be a natural extension to your
>>>> workflow to instead create worktrees of of that bare one as the
>>>> primary worktree with secondaries with working trees.
>>>
>>> For what it's worth, this conversation makes me think it was a mistake
>>> to call this construct a worktree.
>>
>> For the record, I am totally confused with Junio's last line, with two
>> "with"s, "worktree" and "working trees" in the same phrase :D
> 
> In case this wasn't just a tangential note, what I meant was:
> 
>  - In the old world, you may have had a single bare repository and
>    then made clones, each of which has a working tree (i.e. non-bare
>    clones), and worked inside these clones.
> 
>  - In the "git worktree" world, you can start from that same single
>    bare repository, but instead of cloning it, use "git worktree" to
>    create "worktree"s, each of which has a working tree, and work
>    inside these "worktree"s.
> 
> and the latter would be a natural extension to the workflow the
> former wanted to use.
> 
>>> It's fine for the command to have one name and the documentation to
>>> use a longer, clearer name to explain it.  What should that longer,
>>> clearer name be?
>>
>> No comments from me. I'll let you know that if Eric (or Junio?) didn't
>> stop me, we would have had $GIT_DIR/repos now instead of
>> $GIT_DIR/worktrees, just some extra confusion toppings.
> 
> I forgot about that part of the history, but you are saying you
> wanted to call these "repos", not "worktrees"?  I can see why
> somebody (or me?) would stop that by fearing "repo" is a bit too
> confusing with a "repository", in the same way that we are now
> realizing that "worktree" is too similar to an old synonym we used
> to call "working tree".
> 

I would say the new thing is really a "checkout", but that opens another
can of worms. On the other hand, "git checkout" already does:
- switching of branches
- creation of branches
- detaching of head
- partial updates of the working tree
So why shouldn't it manage worktrees, as well?

While that may sound a bit sarcastic it indicates that we may want to
rethink some things at some point rather than adding up to the
conflation. The discussion in this thread seems to show that "worktree"
is just as a good a name for the new feature, while "workbase" or
"workroot" (or "workdir") or so could have been for the old one.

Are we at a point where we can still rename the new feature at least? If
yes, and keeping everything else is mandatory, than "workspace" or
"working space" may be a serious contender for naming the new thing.

Michael

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Are we at a point where we can still rename the new feature at least? If
> yes, and keeping everything else is mandatory, than "workspace" or
> "working space" may be a serious contender for naming the new thing.

I do not have a good answer to the first question, but workspace
does sound like a good name for what this feature is trying to
achieve.

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Duy Nguyen]

On Fri, Mar 24, 2017 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> Are we at a point where we can still rename the new feature at least? If
>> yes, and keeping everything else is mandatory, than "workspace" or
>> "working space" may be a serious contender for naming the new thing.
>
> I do not have a good answer to the first question, but workspace
> does sound like a good name for what this feature is trying to
> achieve.
>

Now is not too late to rename the command from worktree to workspace
(and keep "worktree" as an alias that will be eventually deleted).
Should we do it? I would keep file names, function names... unchanged
though, not worth the amount of new conflicts.
-- 
Duy

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Michael J Gruber]

Duy Nguyen venit, vidit, dixit 25.03.2017 13:07:
> On Fri, Mar 24, 2017 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>>
>>> Are we at a point where we can still rename the new feature at least? If
>>> yes, and keeping everything else is mandatory, than "workspace" or
>>> "working space" may be a serious contender for naming the new thing.
>>
>> I do not have a good answer to the first question, but workspace
>> does sound like a good name for what this feature is trying to
>> achieve.
>>
> 
> Now is not too late to rename the command from worktree to workspace
> (and keep "worktree" as an alias that will be eventually deleted).
> Should we do it? I would keep file names, function names... unchanged
> though, not worth the amount of new conflicts.

I guess I would go for a full change. Our technical documentation often
merely consists of the source code, so we should reduce potential
confusion there, too.

Michael

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Jacob Keller]

On Fri, Apr 7, 2017 at 6:59 AM, Michael J Gruber <git@grubix.eu> wrote:
> Duy Nguyen venit, vidit, dixit 25.03.2017 13:07:
>> On Fri, Mar 24, 2017 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>>>
>>>> Are we at a point where we can still rename the new feature at least? If
>>>> yes, and keeping everything else is mandatory, than "workspace" or
>>>> "working space" may be a serious contender for naming the new thing.
>>>
>>> I do not have a good answer to the first question, but workspace
>>> does sound like a good name for what this feature is trying to
>>> achieve.
>>>
>>
>> Now is not too late to rename the command from worktree to workspace
>> (and keep "worktree" as an alias that will be eventually deleted).
>> Should we do it? I would keep file names, function names... unchanged
>> though, not worth the amount of new conflicts.
>
> I guess I would go for a full change. Our technical documentation often
> merely consists of the source code, so we should reduce potential
> confusion there, too.
>
> Michael

Yes, changing the command name but not all the code would cause more
confusion in the long run.

However, personally I never confused "worktree" and "working trees"
myself. I fee like they are related concepts anyways, in that a fresh
clone has one worktree which its its working tree. Renaming it all to
workspace doesn't bother me either.

Thanks,
Jake

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

[Duy Nguyen]

On Tue, Mar 21, 2017 at 10:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Tue, Mar 21, 2017 at 1:50 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> Junio C Hamano wrote:
>>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>>> While it may be true that you can have bare worktrees; I would question
>>>>> why anyone wants to do this, as the only thing it provides is an
>>>>> additional HEAD (plus its reflog).
>>>>
>>>> A more plausible situation is you start with a bare one as the
>>>> primary and used to make local clones to do your work in the world
>>>> before "git worktree".  It would be a natural extension to your
>>>> workflow to instead create worktrees of of that bare one as the
>>>> primary worktree with secondaries with working trees.
>>>
>>> For what it's worth, this conversation makes me think it was a mistake
>>> to call this construct a worktree.
>>
>> For the record, I am totally confused with Junio's last line, with two
>> "with"s, "worktree" and "working trees" in the same phrase :D
>
> In case this wasn't just a tangential note, what I meant was:
>
>  - In the old world, you may have had a single bare repository and
>    then made clones, each of which has a working tree (i.e. non-bare
>    clones), and worked inside these clones.
>
>  - In the "git worktree" world, you can start from that same single
>    bare repository, but instead of cloning it, use "git worktree" to
>    create "worktree"s, each of which has a working tree, and work
>    inside these "worktree"s.
>
> and the latter would be a natural extension to the workflow the
> former wanted to use.

Yes I really want that, and even the ability to convert a normal one
repo (with one working tree) to the latter, moving the repository to
somewhere safe.

>>> It's fine for the command to have one name and the documentation to
>>> use a longer, clearer name to explain it.  What should that longer,
>>> clearer name be?
>>
>> No comments from me. I'll let you know that if Eric (or Junio?) didn't
>> stop me, we would have had $GIT_DIR/repos now instead of
>> $GIT_DIR/worktrees, just some extra confusion toppings.
>
> I forgot about that part of the history, but you are saying you
> wanted to call these "repos", not "worktrees"?

From $GIT_DIR perspective (which points to
$GIT_COMMON_DIR/worktrees/blah) then they do look like a repository
with lots of part borrowed from $GIT_COMMON_DIR. I was simply saying
I'm bad at naming things. "worktrees" is a better name than "repos".

> I can see why
> somebody (or me?) would stop that by fearing "repo" is a bit too
> confusing with a "repository", in the same way that we are now
> realizing that "worktree" is too similar to an old synonym we used
> to call "working tree".
-- 
Duy