threads / patch / 28216

patchreplace: List replacement along with the object

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

## tl;dr

8 messages between Aug 25, 2011 and Aug 27, 2011. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Michael J Gruber· Aug 25, 2011, 14:39 UTC · lore

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(-)
Show changes to 3 files +6 −6

Documentation/git-replace.txt, builtin/replace.c, t/t6050-replace.sh

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
Christian Couder· Aug 25, 2011, 16:29 UTC · re: Michael J Gruber · lore

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

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.

Michael J Gruber· Aug 26, 2011, 07:38 UTC · re: Christian Couder · lore

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

Christian Couder venit, vidit, dixit 25.08.2011 18:29:
Show 7 quoted lines
> 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.
Show 7 quoted lines
>> 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:
Show 8 quoted lines
> 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
Michael J Gruber· Aug 26, 2011, 07:53 UTC · re: Michael J Gruber · lore

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

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(-)
Show changes to Documentation/git-replace.txt +8 −0
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
Junio C Hamano· Aug 26, 2011, 16:30 UTC · re: Michael J Gruber · lore

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

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 21 quoted lines
> 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?

Michael J Gruber· Aug 27, 2011, 14:07 UTC · re: Junio C Hamano · lore

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

Junio C Hamano venit, vidit, dixit 26.08.2011 18:30:
Show 34 quoted lines
> 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.

Show 7 quoted lines
> 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
Christian Couder· Aug 26, 2011, 08:13 UTC · re: Michael J Gruber · lore

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

On Fri, Aug 26, 2011 at 9:38 AM, Michael J Gruber <git@drmicha.warpmail.net> wrote:

Show 24 quoted lines
> 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.

Show 5 quoted lines
>> 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.

Junio C Hamano· Aug 25, 2011, 19:07 UTC · re: Michael J Gruber · lore

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

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?

← back to recent threads