[PATCH] replace: List replacement along with the object

[PATCH] replace: List replacement along with the object

[Michael J Gruber]

The documentation could be misunderstood as if "git replace -l" lists
the replacements of the specified objects. Currently, it lists the
replaced objects.

Change the output to the form "<object> <replacement>" so that there is
an easy way to find the replacement, besides the more difficult to find
git show-ref $(git replace -l).

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Noted in passing while testing decorations.
---
 Documentation/git-replace.txt |    2 +-
 builtin/replace.c             |    2 +-
 t/t6050-replace.sh            |    8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
index 17df525..9bf3ff8 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -58,7 +58,7 @@ OPTIONS
 
 -l <pattern>::
 	List replace refs for objects that match the given pattern (or
-	all if no pattern is given).
+	all if no pattern is given) in the form "<object> <replacement>".
 	Typing "git replace" without arguments, also lists all replace
 	refs.
 
diff --git a/builtin/replace.c b/builtin/replace.c
index fe3a647..f8c5a9f 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -26,7 +26,7 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 	const char *pattern = cb_data;
 
 	if (!fnmatch(pattern, refname, 0))
-		printf("%s\n", refname);
+		printf("%s %s\n", refname, sha1_to_hex(sha1));
 
 	return 0;
 }
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 5c87f28..665b308 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -119,10 +119,10 @@ test_expect_success 'repack, clone and fetch work' '
 '
 
 test_expect_success '"git replace" listing and deleting' '
-     test "$HASH2" = "$(git replace -l)" &&
-     test "$HASH2" = "$(git replace)" &&
+     test "$HASH2 $R" = "$(git replace -l)" &&
+     test "$HASH2 $R" = "$(git replace)" &&
      aa=${HASH2%??????????????????????????????????????} &&
-     test "$HASH2" = "$(git replace -l "$aa*")" &&
+     test "$HASH2 $R" = "$(git replace -l "$aa*")" &&
      test_must_fail git replace -d $R &&
      test_must_fail git replace -d &&
      test_must_fail git replace -l -d $HASH2 &&
@@ -137,7 +137,7 @@ test_expect_success '"git replace" replacing' '
      test_must_fail git replace $HASH2 $R &&
      git replace -f $HASH2 $R &&
      test_must_fail git replace -f &&
-     test "$HASH2" = "$(git replace)"
+     test "$HASH2 $R" = "$(git replace)"
 '
 
 # This creates a side branch where the bug in H2
-- 
1.7.6.845.gc3c05

Re: [PATCH] replace: List replacement along with the object

[Christian Couder]

On Thu, Aug 25, 2011 at 4:39 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> The documentation could be misunderstood as if "git replace -l" lists
> the replacements of the specified objects. Currently, it lists the
> replaced objects.

You could just change the documentation to make it more explicit.

> Change the output to the form "<object> <replacement>" so that there is
> an easy way to find the replacement, besides the more difficult to find
> git show-ref $(git replace -l).

I shamelessly copied the "-l <pattern>" feature and the documentation
from "git tag". If you just change the output of "git replace -l" it
will make the UI inconsistent between both commands.

Maybe you could add a "-L <pattern>" feature to "git replace", "git
tag" and "git branch" that would output "<ref name> <ref content>"?

Thanks,
Christian.

Re: [PATCH] replace: List replacement along with the object

[Michael J Gruber]

Christian Couder venit, vidit, dixit 25.08.2011 18:29:
> On Thu, Aug 25, 2011 at 4:39 PM, Michael J Gruber
> <git@drmicha.warpmail.net> wrote:
>> The documentation could be misunderstood as if "git replace -l" lists
>> the replacements of the specified objects. Currently, it lists the
>> replaced objects.
> 
> You could just change the documentation to make it more explicit.

Well, sure. I just didn't find the current form that useful.

>> Change the output to the form "<object> <replacement>" so that there is
>> an easy way to find the replacement, besides the more difficult to find
>> git show-ref $(git replace -l).
> 
> I shamelessly copied the "-l <pattern>" feature and the documentation
> from "git tag". If you just change the output of "git replace -l" it
> will make the UI inconsistent between both commands.

I don't think many people will expect consistency between branch and tag
on the one hand, and replace refs on the other hand. It requires the
knowledge that a replacement is basically a lightweight tag stored in a
different namespace in refs/, which I would actually consider an
implementation detail.

> Maybe you could add a "-L <pattern>" feature to "git replace", "git
> tag" and "git branch" that would output "<ref name> <ref content>"?

I'd use "-v" then if this is about consistency, because that *always*
means "verbose", and migrate the misnamed "git tag -v"...

Junio C Hamano venit, vidit, dixit 25.08.2011 21:07:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> The documentation could be misunderstood as if "git replace -l" lists
>> the replacements of the specified objects. Currently, it lists the
>> replaced objects.
> Seeing that you had to change existing tests, I do not think this is an
> improvement. The existing scripts can read the list of objects and find
> replacement themselves (if they want to find that out, that is), no?

If "replace -l" is considered fair game for scripts then the output
should probably not change, though I left the meaning of "$1" for each
line of the output as is on purpose.

But, how would scripts find the replacement? rev-parse does not do it,
rev-list does not do it, and using show-ref requires the user to know
about the actual implementation as refs under refs/replace.

Seems that the doc change is the only option.

Michael

[PATCHv2] git-replace.txt: Clarify list mode

[Michael J Gruber]

Clarify that in list mode, "git replace" outputs the shortened ref
names, not their values.

Also, point to the difficult to find git show-ref $(git replace -l).

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-replace.txt |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
index 17df525..cd00837 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -61,6 +61,13 @@ OPTIONS
 	all if no pattern is given).
 	Typing "git replace" without arguments, also lists all replace
 	refs.
++
+Note that this lists the names of the replace refs, not their values
+(not their replacements). You can get the latter like this, e.g.:
+
+------------------------------------------------
+$ git show-ref $(git replace -l)
+------------------------------------------------
 
 BUGS
 ----
@@ -76,6 +83,7 @@ replaced by a commit).
 
 SEE ALSO
 --------
+linkgit:git-show-ref[1]
 linkgit:git-tag[1]
 linkgit:git-branch[1]
 linkgit:git[1]
-- 
1.7.6.845.gc3c05

Re: [PATCHv2] git-replace.txt: Clarify list mode

[Junio C Hamano]

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

> Clarify that in list mode, "git replace" outputs the shortened ref
> names, not their values.
>
> Also, point to the difficult to find git show-ref $(git replace -l).
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  Documentation/git-replace.txt |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
> index 17df525..cd00837 100644
> --- a/Documentation/git-replace.txt
> +++ b/Documentation/git-replace.txt
> @@ -61,6 +61,13 @@ OPTIONS
>  	all if no pattern is given).
>  	Typing "git replace" without arguments, also lists all replace
>  	refs.
> ++
> +Note that this lists the names of the replace refs, not their values
> +(not their replacements). You can get the latter like this, e.g.:

Hmm, the update is _not wrong_ per-se, but...

I highly doubt we would want to try to cover confusions that may come from
any and all different mis-/pre-conceptions people may have by making the
description _longer_.

Which part of the wording in the existing description made you think that
the command might list both names and their contents?  We should identify
that misleading description (if there is) and fix that, instead of tacking
clarifying clauses at the end.

Given these statements:

	"git replace" lists all replace refs.
        "ls" lists paths in the directory.

I would say a natural reading of them is that they list "replace refs" and
"paths", not "replace refs and their contents" and "paths and their contents".

By the way, one thing I forgot to say was that I do not think the variant
of the output you wanted to have is necessarily a bad thing (it is bad to
change the existing output to that variant, breaking other
people). Perhaps it can become "-v(erbose)" output?

Re: [PATCHv2] git-replace.txt: Clarify list mode

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 26.08.2011 18:30:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Clarify that in list mode, "git replace" outputs the shortened ref
>> names, not their values.
>>
>> Also, point to the difficult to find git show-ref $(git replace -l).
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>>  Documentation/git-replace.txt |    8 ++++++++
>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
>> index 17df525..cd00837 100644
>> --- a/Documentation/git-replace.txt
>> +++ b/Documentation/git-replace.txt
>> @@ -61,6 +61,13 @@ OPTIONS
>>  	all if no pattern is given).
>>  	Typing "git replace" without arguments, also lists all replace
>>  	refs.
>> ++
>> +Note that this lists the names of the replace refs, not their values
>> +(not their replacements). You can get the latter like this, e.g.:
> 
> Hmm, the update is _not wrong_ per-se, but...
> 
> I highly doubt we would want to try to cover confusions that may come from
> any and all different mis-/pre-conceptions people may have by making the
> description _longer_.
> 
> Which part of the wording in the existing description made you think that
> the command might list both names and their contents?  We should identify
> that misleading description (if there is) and fix that, instead of tacking
> clarifying clauses at the end.

Full disclosure: I misunderstood it when I read it the first time. I
never expected it to list pairs of refs, but the questions is: Does it
list the original sha1 or the replacement sha1?

"replace ref" can be very easily misunderstood as "the ref which
replaces", i.e. the replacement sha1. I know a ref is not a rev, but
"replace ref" can easily be misread as "replace rev", "replaced ref" etc.

Secondly, "git replace -l sha1" is completely useless, and I did not
expect that either. Granted, it outputs sha1 or not, depending on
whether it is replaced or not, so "completely" is a bit harsh, but still.

So, while I still have things to learn about git, I've also had my share
of exposure to refs and revs, and if I misunderstand a man page, it
indicates that there may be others whom I could help with what I've
learned from my own misunderstandings.

Also, as regards to clarifying: In out man pages, we often show
practical, non-obvious ways in which a user can combine commands, and I
think git show-ref $(git replace -l) is one of them.

> Given these statements:
> 
> 	"git replace" lists all replace refs.
>         "ls" lists paths in the directory.
> 
> I would say a natural reading of them is that they list "replace refs" and
> "paths", not "replace refs and their contents" and "paths and their contents".

This is natural, and the confusion above is a non-issue, *if* the reader
is very aware of the implementation of replacements as lightweight
tag-like refs with the sha1 as refname.

> By the way, one thing I forgot to say was that I do not think the variant
> of the output you wanted to have is necessarily a bad thing (it is bad to
> change the existing output to that variant, breaking other
> people). Perhaps it can become "-v(erbose)" output?

I'd say yes, Christian seems to prefer having "-v" even closer to
"branch -v". I'd say a user has the right to expect "-v,--verbose"
giving more information, but the type and extent of information should
depend on the actual command :)

Michael

Re: [PATCH] replace: List replacement along with the object

[Christian Couder]

On Fri, Aug 26, 2011 at 9:38 AM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Christian Couder venit, vidit, dixit 25.08.2011 18:29:
>> On Thu, Aug 25, 2011 at 4:39 PM, Michael J Gruber
>> <git@drmicha.warpmail.net> wrote:
>>> The documentation could be misunderstood as if "git replace -l" lists
>>> the replacements of the specified objects. Currently, it lists the
>>> replaced objects.
>>
>> You could just change the documentation to make it more explicit.
>
> Well, sure. I just didn't find the current form that useful.
>
>>> Change the output to the form "<object> <replacement>" so that there is
>>> an easy way to find the replacement, besides the more difficult to find
>>> git show-ref $(git replace -l).
>>
>> I shamelessly copied the "-l <pattern>" feature and the documentation
>> from "git tag". If you just change the output of "git replace -l" it
>> will make the UI inconsistent between both commands.
>
> I don't think many people will expect consistency between branch and tag
> on the one hand, and replace refs on the other hand. It requires the
> knowledge that a replacement is basically a lightweight tag stored in a
> different namespace in refs/, which I would actually consider an
> implementation detail.

It is an implementation detail, but anyway UI consistency is important
and I would suggest the same behavior even if it was implemented in
another way.
By the way it would be nice to make "git remote" more similar to "git
branch", "git tag" and "git replace" while you are at it.

>> Maybe you could add a "-L <pattern>" feature to "git replace", "git
>> tag" and "git branch" that would output "<ref name> <ref content>"?
>
> I'd use "-v" then if this is about consistency, because that *always*
> means "verbose", and migrate the misnamed "git tag -v"...

Yeah, but "git branch -v" is decribed like this:

    Show sha1 and commit subject line for each head, along with
relationship to upstream branch (if any). If given twice, print the
name of the upstream branch, as well.

So if you implement it in "git replace" and "git tag", you should at
least show the commit subject line too.

Thanks,
Christian.

Re: [PATCH] replace: List replacement along with the object

[Junio C Hamano]

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

> The documentation could be misunderstood as if "git replace -l" lists
> the replacements of the specified objects. Currently, it lists the
> replaced objects.

Seeing that you had to change existing tests, I do not think this is an
improvement. The existing scripts can read the list of objects and find
replacement themselves (if they want to find that out, that is), no?