[PATCH v3 00/11] Check replacement object type and minor updates

[PATCH v3 00/11] Check replacement object type and minor updates

[Christian Couder]

In this new version of the series, the only change in the 5 first
patches, is a small change in the commit message of patch 1.

Patches from 6/11 to 11/11 are all new:
	- 6/11, 7/11 and 8/11 are about bypassing the type check
	if -f is used
	- 9/11, 10/11 and 11/11 are about adding long option names

Christian Couder (11):
  replace: forbid replacing an object with one of a different type
  Documentation/replace: state that objects must be of the same type
  t6050-replace: test that objects are of the same type
  t6050-replace: add test to clean up all the replace refs
  Documentation/replace: add Creating Replacement Objects section
  replace: bypass the type check if -f option is used
  Documentation/replace: tell that -f option bypasses the type check
  t6050-replace: check that -f option bypasses the type check
  replace: allow long option names
  Documentation/replace: list long option names
  t6050-replace: use some long option names

 Documentation/git-replace.txt | 28 +++++++++++++++++++++++++---
 builtin/replace.c             | 16 +++++++++++++---
 t/t6050-replace.sh            | 31 ++++++++++++++++++++++++++++---
 3 files changed, 66 insertions(+), 9 deletions(-)

-- 
1.8.4.rc1.31.g530f5ce.dirty

[PATCH v3 01/11] replace: forbid replacing an object with one of a different type

[Christian Couder]

Users replacing an object with one of a different type were not
prevented to do so, even if it was obvious, and stated in the doc,
that bad things would result from doing that.

To avoid mistakes, it is better to just forbid that though.

If one object is replaced with one of a different type, the only way
to keep the history valid is to also replace all the other objects
that point to the replaced object. That's because:

* Annotated tags contain the type of the tagged object.

* The tree/parent lines in commits must be a tree and commits, resp.

* The object types referred to by trees are specified in the 'mode'
  field:
    100644 and 100755    blob
    160000               commit
    040000               tree
  (these are the only valid modes)

* Blobs don't point at anything.

The doc will be updated in a later patch.

Acked-by: Philip Oakley <philipoakley@iee.org>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/replace.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/builtin/replace.c b/builtin/replace.c
index 59d3115..9a94769 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -85,6 +85,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 			  int force)
 {
 	unsigned char object[20], prev[20], repl[20];
+	enum object_type obj_type, repl_type;
 	char ref[PATH_MAX];
 	struct ref_lock *lock;
 
@@ -100,6 +101,15 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 	if (check_refname_format(ref, 0))
 		die("'%s' is not a valid ref name.", ref);
 
+	obj_type = sha1_object_info(object, NULL);
+	repl_type = sha1_object_info(repl, NULL);
+	if (obj_type != repl_type)
+		die("Objects must be of the same type.\n"
+		    "'%s' points to a replaced object of type '%s'\n"
+		    "while '%s' points to a replacement object of type '%s'.",
+		    object_ref, typename(obj_type),
+		    replace_ref, typename(repl_type));
+
 	if (read_ref(ref, prev))
 		hashclr(prev);
 	else if (!force)
-- 
1.8.4.rc1.31.g530f5ce.dirty

Re: [PATCH v3 01/11] replace: forbid replacing an object with one of a different type

[Philip Oakley]

Sorry for not replying earlier in the series.

From: "Christian Couder" <chriscool@tuxfamily.org>
> Users replacing an object with one of a different type were not
> prevented to do so, even if it was obvious, and stated in the doc,
> that bad things would result from doing that.
>
> To avoid mistakes, it is better to just forbid that though.
>
> If one object is replaced with one of a different type, the only way
> to keep the history valid is to also replace all the other objects
> that point to the replaced object.

Isn't this a recursion problem? Taken in that order one unravels the 
whole DAG.

However if considered in the reverse direction, one can replace an 
existing object within the DAG with a carefully crafted alternative of 
the same type, but which then wrongly references other dangling objects 
which are then replaced by objects which have the right type (this last 
replacement requires -f force).

> That's because:
>
> * Annotated tags contain the type of the tagged object.
>
> * The tree/parent lines in commits must be a tree and commits, resp.
>
> * The object types referred to by trees are specified in the 'mode'
>  field:
>    100644 and 100755    blob
>    160000               commit
>    040000               tree
>  (these are the only valid modes)
>
> * Blobs don't point at anything.
>
> The doc will be updated in a later patch.
>
> Acked-by: Philip Oakley <philipoakley@iee.org>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> builtin/replace.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 59d3115..9a94769 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -85,6 +85,7 @@ static int replace_object(const char *object_ref, 
> const char *replace_ref,
>    int force)
> {
>  unsigned char object[20], prev[20], repl[20];
> + enum object_type obj_type, repl_type;
>  char ref[PATH_MAX];
>  struct ref_lock *lock;
>
> @@ -100,6 +101,15 @@ static int replace_object(const char *object_ref, 
> const char *replace_ref,
>  if (check_refname_format(ref, 0))
>  die("'%s' is not a valid ref name.", ref);
>
> + obj_type = sha1_object_info(object, NULL);
> + repl_type = sha1_object_info(repl, NULL);
> + if (obj_type != repl_type)
> + die("Objects must be of the same type.\n"
> +     "'%s' points to a replaced object of type '%s'\n"
> +     "while '%s' points to a replacement object of type '%s'.",
> +     object_ref, typename(obj_type),
> +     replace_ref, typename(repl_type));
> +
>  if (read_ref(ref, prev))
>  hashclr(prev);
>  else if (!force)
> -- 
> 1.8.4.rc1.31.g530f5ce.dirty
>
>
> 

Re: [PATCH v3 01/11] replace: forbid replacing an object with one of a different type

[Christian Couder]

From: "Philip Oakley" <philipoakley@iee.org>
>
> Sorry for not replying earlier in the series.
> 
> From: "Christian Couder" <chriscool@tuxfamily.org>
>> Users replacing an object with one of a different type were not
>> prevented to do so, even if it was obvious, and stated in the doc,
>> that bad things would result from doing that.
>>
>> To avoid mistakes, it is better to just forbid that though.
>>
>> If one object is replaced with one of a different type, the only way
>> to keep the history valid is to also replace all the other objects
>> that point to the replaced object.
> 
> Isn't this a recursion problem? Taken in that order one unravels the
> whole DAG.
> 
> However if considered in the reverse direction, one can replace an
> existing object within the DAG with a carefully crafted alternative of
> the same type, but which then wrongly references other dangling
> objects which are then replaced by objects which have the right type
> (this last replacement requires -f force).

I am not sure I understand what you are saying.

Anyway in a previous version of this patch I tried to be more explicit
about this, but Junio basically said that he found no value in
discussing this more explicitely...

Thanks,
Christian.

Re: [PATCH v3 01/11] replace: forbid replacing an object with one of a different type

[Philip Oakley]

From: "Christian Couder" <chriscool@tuxfamily.org>
> From: "Philip Oakley" <philipoakley@iee.org>
>>
>> Sorry for not replying earlier in the series.
>>
>> From: "Christian Couder" <chriscool@tuxfamily.org>
>>> Users replacing an object with one of a different type were not
>>> prevented to do so, even if it was obvious, and stated in the doc,
>>> that bad things would result from doing that.
>>>
>>> To avoid mistakes, it is better to just forbid that though.
>>>
>>> If one object is replaced with one of a different type, the only way
>>> to keep the history valid is to also replace all the other objects
>>> that point to the replaced object.
>>
>> Isn't this a recursion problem? Taken in that order one unravels the
>> whole DAG.
>>
>> However if considered in the reverse direction, one can replace an
>> existing object within the DAG with a carefully crafted alternative 
>> of
>> the same type, but which then wrongly references other dangling
>> objects which are then replaced by objects which have the right type
>> (this last replacement requires -f force).
>
> I am not sure I understand what you are saying.
>
> Anyway in a previous version of this patch I tried to be more explicit
> about this, but Junio basically said that he found no value in
> discussing this more explicitely...

I would agree that it's not worth discussing it more explicitly.

My comment was more about the direction of the line of reasoning which I 
felt was a bit Catch 22 when starting from an existing complete DAG (no 
garbage) and attempting to replace an object with another of a different 
type and still have a valid DAG.  The construction of the replacement 
items needs to be in the right order if one of the replacements is of 
the 'wrong' type (such a construction requires the creation or uses, and 
ultimately replacement of, extraneous objects that aren't (yet) in the 
DAG).

But as already been said that's a problem for the user of the --force 
option ;-)

Philip

>
> Thanks,
> Christian.
> 

[PATCH v3 02/11] Documentation/replace: state that objects must be of the same type

[Christian Couder]

A previous patch ensures that both the replaced and the replacement
objects passed to git replace must be of the same type.

While at it state that there is no other restriction on both objects.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-replace.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
index e0b4057..aa66d27 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -20,6 +20,9 @@ The name of the 'replace' reference is the SHA-1 of the object that is
 replaced. The content of the 'replace' reference is the SHA-1 of the
 replacement object.
 
+The replaced object and the replacement object must be of the same type.
+There is no other restriction on them.
+
 Unless `-f` is given, the 'replace' reference must not yet exist.
 
 Replacement references will be used by default by all Git commands
@@ -69,9 +72,7 @@ go back to a replaced commit will move the branch to the replacement
 commit instead of the replaced commit.
 
 There may be other problems when using 'git rev-list' related to
-pending objects. And of course things may break if an object of one
-type is replaced by an object of another type (for example a blob
-replaced by a commit).
+pending objects.
 
 SEE ALSO
 --------
-- 
1.8.4.rc1.31.g530f5ce.dirty

[PATCH v3 03/11] t6050-replace: test that objects are of the same type

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index decdc33..5c352c4 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -263,4 +263,17 @@ test_expect_success 'not just commits' '
 	test_cmp file.replaced file
 '
 
+test_expect_success 'replaced and replacement objects must be of the same type' '
+	test_must_fail git replace mytag $HASH1 2>err &&
+	grep "mytag. points to a replaced object of type .tag" err &&
+	grep "$HASH1. points to a replacement object of type .commit" err &&
+	test_must_fail git replace HEAD^{tree} HEAD~1 2>err &&
+	grep "HEAD^{tree}. points to a replaced object of type .tree" err &&
+	grep "HEAD~1. points to a replacement object of type .commit" err &&
+	BLOB=$(git rev-parse :file) &&
+	test_must_fail git replace HEAD^ $BLOB 2>err &&
+	grep "HEAD^. points to a replaced object of type .commit" err &&
+	grep "$BLOB. points to a replacement object of type .blob" err
+'
+
 test_done
-- 
1.8.4.rc1.31.g530f5ce.dirty

[PATCH v3 04/11] t6050-replace: add test to clean up all the replace refs

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 5c352c4..05be228 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -276,4 +276,10 @@ test_expect_success 'replaced and replacement objects must be of the same type'
 	grep "$BLOB. points to a replacement object of type .blob" err
 '
 
+test_expect_success 'replace ref cleanup' '
+	test -n "$(git replace)" &&
+	git replace -d $(git replace) &&
+	test -z "$(git replace)"
+'
+
 test_done
-- 
1.8.4.rc1.31.g530f5ce.dirty

[PATCH v3 05/11] Documentation/replace: add Creating Replacement Objects section

[Christian Couder]

There were no hints in the documentation about how to create
replacement objects.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-replace.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
index aa66d27..736b48c 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -64,6 +64,19 @@ OPTIONS
 	Typing "git replace" without arguments, also lists all replace
 	refs.
 
+CREATING REPLACEMENT OBJECTS
+----------------------------
+
+linkgit:git-filter-branch[1], linkgit:git-hash-object[1] and
+linkgit:git-rebase[1], among other git commands, can be used to create
+replacement objects from existing objects.
+
+If you want to replace many blobs, trees or commits that are part of a
+string of commits, you may just want to create a replacement string of
+commits and then only replace the commit at the tip of the target
+string of commits with the commit at the tip of the replacement string
+of commits.
+
 BUGS
 ----
 Comparing blobs or trees that have been replaced with those that
@@ -76,6 +89,9 @@ pending objects.
 
 SEE ALSO
 --------
+linkgit:git-hash-object[1]
+linkgit:git-filter-branch[1]
+linkgit:git-rebase[1]
 linkgit:git-tag[1]
 linkgit:git-branch[1]
 linkgit:git[1]
-- 
1.8.4.rc1.31.g530f5ce.dirty

Re: [PATCH v3 05/11] Documentation/replace: add Creating Replacement Objects section

[Philip Oakley]

From: "Christian Couder" <chriscool@tuxfamily.org>
> There were no hints in the documentation about how to create
> replacement objects.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> Documentation/git-replace.txt | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/Documentation/git-replace.txt 
> b/Documentation/git-replace.txt
> index aa66d27..736b48c 100644
> --- a/Documentation/git-replace.txt
> +++ b/Documentation/git-replace.txt
> @@ -64,6 +64,19 @@ OPTIONS
>  Typing "git replace" without arguments, also lists all replace
>  refs.
>
> +CREATING REPLACEMENT OBJECTS
> +----------------------------
> +
> +linkgit:git-filter-branch[1], linkgit:git-hash-object[1] and
> +linkgit:git-rebase[1],

Let's not forget the obvious 'git commit' or 'git merge' on a temporary 
branch for creating a replacement commit.

In particular we need to have covered the alternate to a graft of "A B 
C" (i.e. A is now a merge of B & C) if we are to deprecate grafts with 
any conviction. (https://git.wiki.kernel.org/index.php/GraftPoint)

> among other git commands, can be used to create
> +replacement objects from existing objects.
> +
> +If you want to replace many blobs, trees or commits that are part of 
> a
> +string of commits, you may just want to create a replacement string 
> of
> +commits and then only replace the commit at the tip of the target
> +string of commits with the commit at the tip of the replacement 
> string
> +of commits.
> +
> BUGS
> ----
> Comparing blobs or trees that have been replaced with those that
> @@ -76,6 +89,9 @@ pending objects.
>
> SEE ALSO
> --------
> +linkgit:git-hash-object[1]
> +linkgit:git-filter-branch[1]
> +linkgit:git-rebase[1]
> linkgit:git-tag[1]
> linkgit:git-branch[1]
> linkgit:git[1]
> -- 
> 1.8.4.rc1.31.g530f5ce.dirty
>
>
> 

Re: [PATCH v3 05/11] Documentation/replace: add Creating Replacement Objects section

[Christian Couder]

From: "Philip Oakley" <philipoakley@iee.org>
>
> From: "Christian Couder" <chriscool@tuxfamily.org>
>
>> +CREATING REPLACEMENT OBJECTS
>> +----------------------------
>> +
>> +linkgit:git-filter-branch[1], linkgit:git-hash-object[1] and
>> +linkgit:git-rebase[1],
> 
> Let's not forget the obvious 'git commit' or 'git merge' on a
> temporary branch for creating a replacement commit.

As it is obvious, and as it is somehow addressed in the below part of
this section, I don't think it is worth talking about git commit or
git merge or git cherry-pick or any other command.
 
> In particular we need to have covered the alternate to a graft of "A B
> C" (i.e. A is now a merge of B & C) if we are to deprecate grafts with
> any conviction. (https://git.wiki.kernel.org/index.php/GraftPoint)

Adding such an example in a new EXAMPLE section would address this
better. If people agree I will do it in a following patch.

>> among other git commands, can be used to create
>> +replacement objects from existing objects.
>> +
>> +If you want to replace many blobs, trees or commits that are part of
>> a
>> +string of commits, you may just want to create a replacement string
>> of
>> +commits and then only replace the commit at the tip of the target
>> +string of commits with the commit at the tip of the replacement
>> string
>> +of commits.

Thanks,
Christian.

[PATCH v3 06/11] replace: bypass the type check if -f option is used

[Christian Couder]

If -f option, which means '--force', is used, we can allow an object
to be replaced with one of a different type, as the user should know
what (s)he is doing.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/replace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 9a94769..95736d9 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -103,7 +103,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 
 	obj_type = sha1_object_info(object, NULL);
 	repl_type = sha1_object_info(repl, NULL);
-	if (obj_type != repl_type)
+	if (!force && obj_type != repl_type)
 		die("Objects must be of the same type.\n"
 		    "'%s' points to a replaced object of type '%s'\n"
 		    "while '%s' points to a replacement object of type '%s'.",
-- 
1.8.4.rc1.31.g530f5ce.dirty

[PATCH v3 07/11] Documentation/replace: tell that -f option bypasses the type check

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-replace.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
index 736b48c..a2bd2ee 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -21,10 +21,12 @@ replaced. The content of the 'replace' reference is the SHA-1 of the
 replacement object.
 
 The replaced object and the replacement object must be of the same type.
-There is no other restriction on them.
+This restriction can be bypassed using `-f`.
 
 Unless `-f` is given, the 'replace' reference must not yet exist.
 
+There is no other restriction on the replaced and replacement objects.
+
 Replacement references will be used by default by all Git commands
 except those doing reachability traversal (prune, pack transfer and
 fsck).
-- 
1.8.4.rc1.31.g530f5ce.dirty

Re: [PATCH v3 07/11] Documentation/replace: tell that -f option bypasses the type check

[Philip Oakley]

From: "Christian Couder" <chriscool@tuxfamily.org>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> Documentation/git-replace.txt | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-replace.txt 
> b/Documentation/git-replace.txt
> index 736b48c..a2bd2ee 100644
> --- a/Documentation/git-replace.txt
> +++ b/Documentation/git-replace.txt
> @@ -21,10 +21,12 @@ replaced. The content of the 'replace' reference 
> is the SHA-1 of the
> replacement object.
>
> The replaced object and the replacement object must be of the same 
> type.
> -There is no other restriction on them.
> +This restriction can be bypassed using `-f`.
>
> Unless `-f` is given, the 'replace' reference must not yet exist.
>
> +There is no other restriction on the replaced and replacement 
> objects.

Is this trying to allude to the fact that merge commits may be exchanged 
with non-merge commits? I strongly believe that this ability to exchange 
merge and non-merge commits should be stated _explicitly_ to counteract 
the false beliefs that are listed out on the internet.

It's probably better stated in a separate patch for that explicit 
purpose to avoid mixed messages within this commit.

> +
> Replacement references will be used by default by all Git commands
> except those doing reachability traversal (prune, pack transfer and
> fsck).
> -- 

Re: [PATCH v3 07/11] Documentation/replace: tell that -f option bypasses the type check

[Christian Couder]

From: "Philip Oakley" <philipoakley@iee.org>
>
> From: "Christian Couder" <chriscool@tuxfamily.org>
>>
>> The replaced object and the replacement object must be of the same
>> type.
>> -There is no other restriction on them.
>> +This restriction can be bypassed using `-f`.
>>
>> Unless `-f` is given, the 'replace' reference must not yet exist.
>>
>> +There is no other restriction on the replaced and replacement
>> objects.
> 
> Is this trying to allude to the fact that merge commits may be
> exchanged with non-merge commits? I strongly believe that this ability
> to exchange merge and non-merge commits should be stated _explicitly_
> to counteract the false beliefs that are listed out on the internet.

Maybe we can show that in an example. But I think the patch is quite
clear as it is and should be enough.

If we really want to correct some false beliefs, the best would be to
state the truth where those false beliefs are stated.

> It's probably better stated in a separate patch for that explicit
> purpose to avoid mixed messages within this commit.

If people agree, I will add a another patch with an example in an
EXAMPLE section.

Thanks,
Christian.

Re: [PATCH v3 07/11] Documentation/replace: tell that -f option bypasses the type check

[Philip Oakley]

From: "Christian Couder" <chriscool@tuxfamily.org>
> From: "Philip Oakley" <philipoakley@iee.org>
>>
>> From: "Christian Couder" <chriscool@tuxfamily.org>
>>>
>>> The replaced object and the replacement object must be of the same
>>> type.
>>> -There is no other restriction on them.
>>> +This restriction can be bypassed using `-f`.
>>>
>>> Unless `-f` is given, the 'replace' reference must not yet exist.
>>>
>>> +There is no other restriction on the replaced and replacement
>>> objects.
>>
>> Is this trying to allude to the fact that merge commits may be
>> exchanged with non-merge commits? I strongly believe that this
>> ability
>> to exchange merge and non-merge commits should be stated _explicitly_
>> to counteract the false beliefs that are listed out on the internet.
>
> Maybe we can show that in an example. But I think the patch is quite
> clear as it is and should be enough.
>
> If we really want to correct some false beliefs, the best would be to
> state the truth where those false beliefs are stated.
>
I've added a sub-comment to the original SO post that started this
thread (my post $gmane/231598 - SO/a/18027030/717355), but given the
guy's blog has comments going back to 2009 I suspect its a bit of a
http://xkcd.com/386/ task, hence my desire that it's explicitly
mentioned in the 'replace' documentation. In addition, if the guy 
doesn't correct his post I'll mark it down in a couple of days to make 
it plain to other readers it's in error.

The creation of a (merge?) commit that's equivalent to a graft line
isn't something that jumps out (to me) as an easy couple lines of bash
script.

A 'graft2replace' script (or 'git-graft' command) which takes an
existing graft file (or command line list) and creates the replacement 
objects and then does the replace, all while still in a dirty tree would 
be the holy grail for properly deprecating grafts (which are sooo easy 
to create)

>> It's probably better stated in a separate patch for that explicit
>> purpose to avoid mixed messages within this commit.
>
> If people agree, I will add a another patch with an example in an
> EXAMPLE section.
>
> Thanks,
> Christian.
>
>
Thanks for your work on this.
Philip 

Re: [PATCH v3 07/11] Documentation/replace: tell that -f option bypasses the type check

[Christian Couder]

From: "Philip Oakley" <philipoakley@iee.org>
>
> From: "Christian Couder" <chriscool@tuxfamily.org>
>
>> Maybe we can show that in an example. But I think the patch is quite
>> clear as it is and should be enough.
>>
>> If we really want to correct some false beliefs, the best would be to
>> state the truth where those false beliefs are stated.
>>
> I've added a sub-comment to the original SO post that started this
> thread (my post $gmane/231598 - SO/a/18027030/717355), but given the
> guy's blog has comments going back to 2009 I suspect its a bit of a
> http://xkcd.com/386/ task, hence my desire that it's explicitly
> mentioned in the 'replace' documentation. In addition, if the guy
> doesn't correct his post I'll mark it down in a couple of days to make
> it plain to other readers it's in error.
> 
> The creation of a (merge?) commit that's equivalent to a graft line
> isn't something that jumps out (to me) as an easy couple lines of bash
> script.
>
> A 'graft2replace' script (or 'git-graft' command) which takes an
> existing graft file (or command line list) and creates the replacement
> objects and then does the replace, all while still in a dirty tree
> would be the holy grail for properly deprecating grafts (which are
> sooo easy to create)

You mean something like the following:

$ cat  ./graft2replace.sh 
#!/bin/bash

while read orig parents
do
        printf "%s" "git cat-file commit $orig"
        printf "%s" " | perl -n -e 'print unless /^parent /'"
        insn=''
        for commit in $parents; do insn="$insn print \"parent $commit\\n\";"; done
        printf "%s" " | perl -n -e 'print; if (/^tree /) { $insn }'"
        printf "%s\n" " > new_commit.txt"
        printf "%s\n" 'REPL=$(git hash-object -t commit -w new_commit.txt)'
        printf "%s\n" "git replace $orig \$REPL"
done

This generates shell instructions from a graft file. Then you only need to execute the generated shell instructions.
For example:

$ cat graft_file.txt 
5bf34fff3186254d7254583675d10ddf98df989b 79fe155489351e8af829a3106e7150397c57d863 dcfbab6bea3df3166503f3084cec2679f10f9e80
fb5657082148297b61fbca7e64d51c1e7870309a

$ cat graft_file.txt | ./graft2replace.sh 
git cat-file commit 5bf34fff3186254d7254583675d10ddf98df989b | perl -n -e 'print unless /^parent /' | perl -n -e 'print; if (/^tree /) {  print "parent 79fe155489351e8af829a3106e7150397c57d863\n"; print "parent dcfbab6bea3df3166503f3084cec2679f10f9e80\n"; }' > new_commit.txt
REPL=$(git hash-object -t commit -w new_commit.txt)
git replace 5bf34fff3186254d7254583675d10ddf98df989b $REPL
git cat-file commit fb5657082148297b61fbca7e64d51c1e7870309a | perl -n -e 'print unless /^parent /' | perl -n -e 'print; if (/^tree /) {  }' > new_commit.txt
REPL=$(git hash-object -t commit -w new_commit.txt)
git replace fb5657082148297b61fbca7e64d51c1e7870309a $REPL

Note that I haven't really tested it.

Best,
Christian.

Re: [PATCH v3 07/11] Documentation/replace: tell that -f option bypasses the type check

[Philip Oakley]

From: "Christian Couder" <chriscool@tuxfamily.org>
> From: "Philip Oakley" <philipoakley@iee.org>
>>
>> From: "Christian Couder" <chriscool@tuxfamily.org>
>>
>>> Maybe we can show that in an example. But I think the patch is quite
>>> clear as it is and should be enough.
>>>
>>> If we really want to correct some false beliefs, the best would be 
>>> to
>>> state the truth where those false beliefs are stated.
>>>
>> I've added a sub-comment to the original SO post that started this
>> thread (my post $gmane/231598 - SO/a/18027030/717355), but given the
>> guy's blog has comments going back to 2009 I suspect its a bit of a
>> http://xkcd.com/386/ task, hence my desire that it's explicitly
>> mentioned in the 'replace' documentation. In addition, if the guy
>> doesn't correct his post I'll mark it down in a couple of days to 
>> make
>> it plain to other readers it's in error.
>>
>> The creation of a (merge?) commit that's equivalent to a graft line
>> isn't something that jumps out (to me) as an easy couple lines of 
>> bash
>> script.
>>
>> A 'graft2replace' script (or 'git-graft' command) which takes an
>> existing graft file (or command line list) and creates the 
>> replacement
>> objects and then does the replace, all while still in a dirty tree
>> would be the holy grail for properly deprecating grafts (which are
>> sooo easy to create)
>
> You mean something like the following:
>
> $ cat  ./graft2replace.sh
> #!/bin/bash
>
> while read orig parents
> do
>        printf "%s" "git cat-file commit $orig"
>        printf "%s" " | perl -n -e 'print unless /^parent /'"
>        insn=''
>        for commit in $parents; do insn="$insn print \"parent 
> $commit\\n\";"; done
>        printf "%s" " | perl -n -e 'print; if (/^tree /) { $insn }'"
>        printf "%s\n" " > new_commit.txt"
>        printf "%s\n" 'REPL=$(git hash-object -t commit -w 
> new_commit.txt)'

Does `hash-object` do the inverese of `cat-file commit`?

I didn't find the hash-object(1) man page very informative on that 
matter and a (very) quick look at its code made me think it was just 
hashing the raw contents which wouldn't be what what was wanted.

>        printf "%s\n" "git replace $orig \$REPL"
> done
>
> This generates shell instructions from a graft file. Then you only 
> need to execute the generated shell instructions.
> For example:
>
> $ cat graft_file.txt
> 5bf34fff3186254d7254583675d10ddf98df989b 
> 79fe155489351e8af829a3106e7150397c57d863 
> dcfbab6bea3df3166503f3084cec2679f10f9e80
> fb5657082148297b61fbca7e64d51c1e7870309a
>
> $ cat graft_file.txt | ./graft2replace.sh
> git cat-file commit 5bf34fff3186254d7254583675d10ddf98df989b | 
> perl -n -e 'print unless /^parent /' | perl -n -e 'print; if (/^tree 
> /) {  print "parent 79fe155489351e8af829a3106e7150397c57d863\n"; print 
> "parent dcfbab6bea3df3166503f3084cec2679f10f9e80\n"; }' > 
> new_commit.txt
> REPL=$(git hash-object -t commit -w new_commit.txt)
> git replace 5bf34fff3186254d7254583675d10ddf98df989b $REPL
> git cat-file commit fb5657082148297b61fbca7e64d51c1e7870309a | 
> perl -n -e 'print unless /^parent /' | perl -n -e 'print; if (/^tree 
> /) {  }' > new_commit.txt
> REPL=$(git hash-object -t commit -w new_commit.txt)
> git replace fb5657082148297b61fbca7e64d51c1e7870309a $REPL
>
> Note that I haven't really tested it.
>
> Best,
> Christian.
> --

I think we could call it 'git-graft', being the help function/script 
that converts raw grafts to proper object replacements ;-)

Philip 

Re: [PATCH v3 07/11] Documentation/replace: tell that -f option bypasses the type check

[Jonathan Nieder]

Hi,

Philip Oakley wrote:

> Does `hash-object` do the inverese of `cat-file commit`?
>
> I didn't find the hash-object(1) man page very informative on that matter

Hm.  The manpage says:

	Computes the object ID value for an object with specified type
	with the contents of the named file [...], and optionally writes
	the resulting object into the object database.

And then:

	-w
		Actually write the object into the object database.

Any ideas for making this clearer?

Thanks,
Jonathan

Re: [PATCH v3 07/11] Documentation/replace: tell that -f option bypasses the type check

[Philip Oakley]

From: "Jonathan Nieder" <jrnieder@gmail.com>
> Hi,
>
> Philip Oakley wrote:
>
>> Does `hash-object` do the inverese of `cat-file commit`?
>>
>> I didn't find the hash-object(1) man page very informative on that 
>> matter
>
> Hm.  The manpage says:
>
> Computes the object ID value for an object with specified type
> with the contents of the named file [...], and optionally writes
> the resulting object into the object database.
>
> And then:
>
> -w
> Actually write the object into the object database.
>
> Any ideas for making this clearer?

The problem is the file format, in the sense that the earlier `git 
cat-file commit $orig` has a human readable output which is a 
description of the commit header, rather than the specific binary 
content. I couldn't tell if the command would cope with such a human 
readable file.

In Christian's quick untested script he'd matched the two commands as 
opposites, which doesn't appear to be the case, and I'm somewhat 
ignorant of what hash-object is doing - I'd expect that it simply hashed 
the already constructed data, but my ignorance starts showing at this 
point ;-)

regards

Philip 

Re: [PATCH v3 07/11] Documentation/replace: tell that -f option bypasses the type check

[Jonathan Nieder]

Philip Oakley wrote:

> The problem is the file format, in the sense that the earlier `git cat-file
> commit $orig` has a human readable output which is a description of the
> commit header, rather than the specific binary content.

Ah.  That's the actual "raw" commit object format, though.

The manpage for git-cat-file(1) says:

	SYNOPSIS
		git cat-file (-t | -s | -e | -p | <type> | --textconv ) <object>
		git cat-file (--batch | --batch-check) < <list-of-objects>

	DESCRIPTION
		In its first form, the command provides the content or the
		type of an object in the repository. [...]

	OUTPUT
		...
		If <type> is specified, the raw (though uncompressed)
		contents of the <object> will be returned.

I agree that this isn't as clear as it should be.  I see a few problems:

 (1) The synopsis treats "git cat-file -t/-s/-e/-p <object>",
     "git cat-file --textconv <tree>:<path>", and
     "git cat-file <type> <object>" as the same form of the command.
     It would be easier to explain these as three different forms.

 (2) There is no EXAMPLES section and no examples.

 (3) There is no pointer to the git object formats.  A pointer to a
     new gitobject(5) manpage would presumably make everything clearer.

https://www.kernel.org/pub/software/scm/git/docs/user-manual.html#examining-the-data
might be a good source of text to start from for solving (1), since
it explains the command a little better.

Thanks,
Jonathan

Re: [PATCH v3 07/11] Documentation/replace: tell that -f option bypasses the type check

[Philip Oakley]

From: "Jonathan Nieder" <jrnieder@gmail.com>
> Philip Oakley wrote:
>
>> The problem is the file format, in the sense that the earlier `git 
>> cat-file
>> commit $orig` has a human readable output which is a description of 
>> the
>> commit header, rather than the specific binary content.
>
> Ah.  That's the actual "raw" commit object format, though.
>

Aha.. Sudden realisation that the cat-file _is_ the 'raw' format and 
that the sha1's etc are shown in ascii hex, rather than being in a 
compact binary format (same for 'unix' dates etc.)

So the 'human readable' output is exactly the 'type_name' field followed 
by a single space (SP) followed by the sha1 in ascii hex (i.e. 
tree/parent), or appropriate data in the well defined format (for 
author/committer) SP 'email' SP date. etc.

It was the Human readable == Machine readable that I'd missed.

> The manpage for git-cat-file(1) says:
>
> SYNOPSIS
> git cat-file (-t | -s | -e | -p | <type> | --textconv ) <object>
> git cat-file (--batch | --batch-check) < <list-of-objects>
>
> DESCRIPTION
> In its first form, the command provides the content or the
> type of an object in the repository. [...]
>
> OUTPUT
> ...
> If <type> is specified, the raw (though uncompressed)
> contents of the <object> will be returned.
>
> I agree that this isn't as clear as it should be.  I see a few 
> problems:
>
> (1) The synopsis treats "git cat-file -t/-s/-e/-p <object>",
>     "git cat-file --textconv <tree>:<path>", and
>     "git cat-file <type> <object>" as the same form of the command.
>     It would be easier to explain these as three different forms.
>
> (2) There is no EXAMPLES section and no examples.
>
> (3) There is no pointer to the git object formats.  A pointer to a
>     new gitobject(5) manpage would presumably make everything clearer.
>
> https://www.kernel.org/pub/software/scm/git/docs/user-manual.html#examining-the-data
> might be a good source of text to start from for solving (1), since
> it explains the command a little better.

A quick run of the example "git cat-file commit HEAD", seen in the 
context of your email helped me appreciate the situation.

>
> Thanks,
> Jonathan
>

Philip 

Re: [PATCH v3 07/11] Documentation/replace: tell that -f option bypasses the type check

[Christian Couder]

On Mon, Sep 2, 2013 at 11:50 PM, Philip Oakley <philipoakley@iee.org> wrote:
>
> From: "Christian Couder" <chriscool@tuxfamily.org>
>
>> You mean something like the following:
>>
>> $ cat  ./graft2replace.sh
>> #!/bin/bash
>>
>> while read orig parents
>> do
>>        printf "%s" "git cat-file commit $orig"
>>        printf "%s" " | perl -n -e 'print unless /^parent /'"
>>        insn=''
>>        for commit in $parents; do insn="$insn print \"parent
>> $commit\\n\";"; done
>>        printf "%s" " | perl -n -e 'print; if (/^tree /) { $insn }'"
>>        printf "%s\n" " > new_commit.txt"
>>        printf "%s\n" 'REPL=$(git hash-object -t commit -w new_commit.txt)'
>
>
> Does `hash-object` do the inverese of `cat-file commit`?
>
> I didn't find the hash-object(1) man page very informative on that matter
> and a (very) quick look at its code made me think it was just hashing the
> raw contents which wouldn't be what what was wanted.

I agree with Jonathan's suggest to add an EXAMPLE section in
hash-object(1) manpage and maybe a new gitobject(5) manpage too.

You can also find a few examples of how git hash-object can be used in
t/t6050-replace.sh:

For example:

     R=$(git cat-file commit $HASH2 | sed -e "s/A U/O/" | git
hash-object -t commit --stdin -w) &&
     git cat-file commit $R | grep "author O Thor" &&
     git update-ref refs/replace/$HASH2 $R

>>        printf "%s\n" "git replace $orig \$REPL"
>> done
>>
>> This generates shell instructions from a graft file. Then you only need to
>> execute the generated shell instructions.
>> For example:
>>
>> $ cat graft_file.txt
>> 5bf34fff3186254d7254583675d10ddf98df989b
>> 79fe155489351e8af829a3106e7150397c57d863
>> dcfbab6bea3df3166503f3084cec2679f10f9e80
>> fb5657082148297b61fbca7e64d51c1e7870309a
>>
>> $ cat graft_file.txt | ./graft2replace.sh
>> git cat-file commit 5bf34fff3186254d7254583675d10ddf98df989b | perl -n -e
>> 'print unless /^parent /' | perl -n -e 'print; if (/^tree /) {  print
>> "parent 79fe155489351e8af829a3106e7150397c57d863\n"; print "parent
>> dcfbab6bea3df3166503f3084cec2679f10f9e80\n"; }' > new_commit.txt
>> REPL=$(git hash-object -t commit -w new_commit.txt)
>> git replace 5bf34fff3186254d7254583675d10ddf98df989b $REPL
>> git cat-file commit fb5657082148297b61fbca7e64d51c1e7870309a | perl -n -e
>> 'print unless /^parent /' | perl -n -e 'print; if (/^tree /) {  }' >
>> new_commit.txt
>> REPL=$(git hash-object -t commit -w new_commit.txt)
>> git replace fb5657082148297b61fbca7e64d51c1e7870309a $REPL
>>
>> Note that I haven't really tested it.

Also note that it is obviously broken if you have commits with a
commit message that has lines starting with 'parent ' or 'tree '.

> I think we could call it 'git-graft', being the help function/script that
> converts raw grafts to proper object replacements ;-)

I will have a look at improving it, testing it and sending a patch to
put it in contrib/.

Thanks,
Christian.

[PATCH v3 08/11] t6050-replace: check that -f option bypasses the type check

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 05be228..0b07a0b 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -276,6 +276,12 @@ test_expect_success 'replaced and replacement objects must be of the same type'
 	grep "$BLOB. points to a replacement object of type .blob" err
 '
 
+test_expect_success '-f option bypasses the type check' '
+	git replace -f mytag $HASH1 2>err &&
+	git replace -f HEAD^{tree} HEAD~1 2>err &&
+	git replace -f HEAD^ $BLOB 2>err
+'
+
 test_expect_success 'replace ref cleanup' '
 	test -n "$(git replace)" &&
 	git replace -d $(git replace) &&
-- 
1.8.4.rc1.31.g530f5ce.dirty

Re: [PATCH v3 08/11] t6050-replace: check that -f option bypasses the type check

[Eric Sunshine]

On Sat, Aug 31, 2013 at 3:12 PM, Christian Couder
<chriscool@tuxfamily.org> wrote:
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  t/t6050-replace.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> index 05be228..0b07a0b 100755
> --- a/t/t6050-replace.sh
> +++ b/t/t6050-replace.sh
> @@ -276,6 +276,12 @@ test_expect_success 'replaced and replacement objects must be of the same type'
>         grep "$BLOB. points to a replacement object of type .blob" err
>  '
>
> +test_expect_success '-f option bypasses the type check' '
> +       git replace -f mytag $HASH1 2>err &&
> +       git replace -f HEAD^{tree} HEAD~1 2>err &&
> +       git replace -f HEAD^ $BLOB 2>err
> +'

Is there a non-obvious reason you are redirecting stderr to a file in
this test? Unlike the test added earlier, this one never consults the
error output. By dropping this apparently unnecessary redirection,
diagnosis of a regression potentially becomes simpler since any error
output from git-replace will become visible when the test is run
verbosely.

> +
>  test_expect_success 'replace ref cleanup' '
>         test -n "$(git replace)" &&
>         git replace -d $(git replace) &&
> --
> 1.8.4.rc1.31.g530f5ce.dirty

Re: [PATCH v3 08/11] t6050-replace: check that -f option bypasses the type check

[Christian Couder]

From: Eric Sunshine <sunshine@sunshineco.com>
>
> On Sat, Aug 31, 2013 at 3:12 PM, Christian Couder
> <chriscool@tuxfamily.org> wrote:
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>>  t/t6050-replace.sh | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
>> index 05be228..0b07a0b 100755
>> --- a/t/t6050-replace.sh
>> +++ b/t/t6050-replace.sh
>> @@ -276,6 +276,12 @@ test_expect_success 'replaced and replacement objects must be of the same type'
>>         grep "$BLOB. points to a replacement object of type .blob" err
>>  '
>>
>> +test_expect_success '-f option bypasses the type check' '
>> +       git replace -f mytag $HASH1 2>err &&
>> +       git replace -f HEAD^{tree} HEAD~1 2>err &&
>> +       git replace -f HEAD^ $BLOB 2>err
>> +'
> 
> Is there a non-obvious reason you are redirecting stderr to a file in
> this test? Unlike the test added earlier, this one never consults the
> error output. By dropping this apparently unnecessary redirection,
> diagnosis of a regression potentially becomes simpler since any error
> output from git-replace will become visible when the test is run
> verbosely.

You are right! I will drop these redirections.

Thanks,
Christian.

[PATCH v3 09/11] replace: allow long option names

[Christian Couder]

It is now standard practice in Git to have both short and long option
names. So let's give a long option name to the git replace options too.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/replace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 95736d9..d4d1b75 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -128,9 +128,9 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 {
 	int list = 0, delete = 0, force = 0;
 	struct option options[] = {
-		OPT_BOOLEAN('l', NULL, &list, N_("list replace refs")),
-		OPT_BOOLEAN('d', NULL, &delete, N_("delete replace refs")),
-		OPT_BOOLEAN('f', NULL, &force, N_("replace the ref if it exists")),
+		OPT_BOOLEAN('l', "list", &list, N_("list replace refs")),
+		OPT_BOOLEAN('d', "delete", &delete, N_("delete replace refs")),
+		OPT_BOOLEAN('f', "force", &force, N_("replace the ref if it exists")),
 		OPT_END()
 	};
 
-- 
1.8.4.rc1.31.g530f5ce.dirty

[PATCH v3 10/11] Documentation/replace: list long option names

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-replace.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
index a2bd2ee..414000e 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -54,13 +54,16 @@ achieve the same effect as the `--no-replace-objects` option.
 OPTIONS
 -------
 -f::
+--force::
 	If an existing replace ref for the same object exists, it will
 	be overwritten (instead of failing).
 
 -d::
+--delete::
 	Delete existing replace refs for the given objects.
 
 -l <pattern>::
+--list <pattern>::
 	List replace refs for objects that match the given pattern (or
 	all if no pattern is given).
 	Typing "git replace" without arguments, also lists all replace
-- 
1.8.4.rc1.31.g530f5ce.dirty

[PATCH v3 11/11] t6050-replace: use some long option names

[Christian Couder]

So that they are tested a litlle bit too.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 0b07a0b..5dc26e8 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -122,9 +122,9 @@ test_expect_success '"git replace" listing and deleting' '
      test "$HASH2" = "$(git replace -l)" &&
      test "$HASH2" = "$(git replace)" &&
      aa=${HASH2%??????????????????????????????????????} &&
-     test "$HASH2" = "$(git replace -l "$aa*")" &&
+     test "$HASH2" = "$(git replace --list "$aa*")" &&
      test_must_fail git replace -d $R &&
-     test_must_fail git replace -d &&
+     test_must_fail git replace --delete &&
      test_must_fail git replace -l -d $HASH2 &&
      git replace -d $HASH2 &&
      git show $HASH2 | grep "A U Thor" &&
@@ -147,7 +147,7 @@ test_expect_success '"git replace" resolves sha1' '
      git show $HASH2 | grep "O Thor" &&
      test_must_fail git replace $HASH2 $R &&
      git replace -f $HASH2 $R &&
-     test_must_fail git replace -f &&
+     test_must_fail git replace --force &&
      test "$HASH2" = "$(git replace)"
 '
 
@@ -278,7 +278,7 @@ test_expect_success 'replaced and replacement objects must be of the same type'
 
 test_expect_success '-f option bypasses the type check' '
 	git replace -f mytag $HASH1 2>err &&
-	git replace -f HEAD^{tree} HEAD~1 2>err &&
+	git replace --force HEAD^{tree} HEAD~1 2>err &&
 	git replace -f HEAD^ $BLOB 2>err
 '
 
-- 
1.8.4.rc1.31.g530f5ce.dirty

Re: [PATCH v3 11/11] t6050-replace: use some long option names

[Philip Oakley]

From: "Christian Couder" <chriscool@tuxfamily.org>
Subject: [PATCH v3 11/11] t6050-replace: use some long option names


> So that they are tested a litlle bit too.
s /litlle/little/

>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> t/t6050-replace.sh | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> index 0b07a0b..5dc26e8 100755
> --- a/t/t6050-replace.sh
> +++ b/t/t6050-replace.sh
> @@ -122,9 +122,9 @@ test_expect_success '"git replace" listing and 
> deleting' '
>      test "$HASH2" = "$(git replace -l)" &&
>      test "$HASH2" = "$(git replace)" &&
>      aa=${HASH2%??????????????????????????????????????} &&
> -     test "$HASH2" = "$(git replace -l "$aa*")" &&
> +     test "$HASH2" = "$(git replace --list "$aa*")" &&
>      test_must_fail git replace -d $R &&
> -     test_must_fail git replace -d &&
> +     test_must_fail git replace --delete &&
>      test_must_fail git replace -l -d $HASH2 &&
>      git replace -d $HASH2 &&
>      git show $HASH2 | grep "A U Thor" &&
> @@ -147,7 +147,7 @@ test_expect_success '"git replace" resolves sha1' 
> '
>      git show $HASH2 | grep "O Thor" &&
>      test_must_fail git replace $HASH2 $R &&
>      git replace -f $HASH2 $R &&
> -     test_must_fail git replace -f &&
> +     test_must_fail git replace --force &&
>      test "$HASH2" = "$(git replace)"
> '
>
> @@ -278,7 +278,7 @@ test_expect_success 'replaced and replacement 
> objects must be of the same type'
>
> test_expect_success '-f option bypasses the type check' '
>  git replace -f mytag $HASH1 2>err &&
> - git replace -f HEAD^{tree} HEAD~1 2>err &&
> + git replace --force HEAD^{tree} HEAD~1 2>err &&
>  git replace -f HEAD^ $BLOB 2>err
> '
>
> -- 
> 1.8.4.rc1.31.g530f5ce.dirty
>
> 

Re: [PATCH v3 11/11] t6050-replace: use some long option names

[Christian Couder]

From: "Philip Oakley" <philipoakley@iee.org>
>
>> So that they are tested a litlle bit too.
> s /litlle/little/

Thanks,
Christian.

Re: [PATCH v3 11/11] t6050-replace: use some long option names

[Eric Sunshine]

On Sat, Aug 31, 2013 at 3:12 PM, Christian Couder
<chriscool@tuxfamily.org> wrote:
> So that they are tested a litlle bit too.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  t/t6050-replace.sh | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> index 0b07a0b..5dc26e8 100755
> --- a/t/t6050-replace.sh
> +++ b/t/t6050-replace.sh
> @@ -122,9 +122,9 @@ test_expect_success '"git replace" listing and deleting' '
>       test "$HASH2" = "$(git replace -l)" &&
>       test "$HASH2" = "$(git replace)" &&
>       aa=${HASH2%??????????????????????????????????????} &&
> -     test "$HASH2" = "$(git replace -l "$aa*")" &&
> +     test "$HASH2" = "$(git replace --list "$aa*")" &&
>       test_must_fail git replace -d $R &&
> -     test_must_fail git replace -d &&
> +     test_must_fail git replace --delete &&
>       test_must_fail git replace -l -d $HASH2 &&

Is this sort of change a good idea? A person reading the code, but not
familiar with this particular patch, might not understand the
seemingly random choice of short versus long option usage. Such a
person might be led to wonder if there is some subtle difference
between the short and long forms, and then unnecessarily spend time
digging into the code and documentation in an attempt to figure it
out. Alternately, someone reading the code might be led to assume that
the person who added the tests was being sloppy.

Hopefully, t0040-parse-options should already be proof enough that
long options are correctly handled, but if you really want to test
them here too, it seems like a separate test would be more appropriate
than randomly changing short form options to long in various tests.

>       git replace -d $HASH2 &&
>       git show $HASH2 | grep "A U Thor" &&
> @@ -147,7 +147,7 @@ test_expect_success '"git replace" resolves sha1' '
>       git show $HASH2 | grep "O Thor" &&
>       test_must_fail git replace $HASH2 $R &&
>       git replace -f $HASH2 $R &&
> -     test_must_fail git replace -f &&
> +     test_must_fail git replace --force &&
>       test "$HASH2" = "$(git replace)"
>  '
>
> @@ -278,7 +278,7 @@ test_expect_success 'replaced and replacement objects must be of the same type'
>
>  test_expect_success '-f option bypasses the type check' '
>         git replace -f mytag $HASH1 2>err &&
> -       git replace -f HEAD^{tree} HEAD~1 2>err &&
> +       git replace --force HEAD^{tree} HEAD~1 2>err &&
>         git replace -f HEAD^ $BLOB 2>err
>  '
>
> --
> 1.8.4.rc1.31.g530f5ce.dirty

Re: [PATCH v3 11/11] t6050-replace: use some long option names

[Christian Couder]

From: Eric Sunshine <sunshine@sunshineco.com>
>
>> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
>> index 0b07a0b..5dc26e8 100755
>> --- a/t/t6050-replace.sh
>> +++ b/t/t6050-replace.sh
>> @@ -122,9 +122,9 @@ test_expect_success '"git replace" listing and deleting' '
>>       test "$HASH2" = "$(git replace -l)" &&
>>       test "$HASH2" = "$(git replace)" &&
>>       aa=${HASH2%??????????????????????????????????????} &&
>> -     test "$HASH2" = "$(git replace -l "$aa*")" &&
>> +     test "$HASH2" = "$(git replace --list "$aa*")" &&
>>       test_must_fail git replace -d $R &&
>> -     test_must_fail git replace -d &&
>> +     test_must_fail git replace --delete &&
>>       test_must_fail git replace -l -d $HASH2 &&
> 
> Is this sort of change a good idea? A person reading the code, but not
> familiar with this particular patch, might not understand the
> seemingly random choice of short versus long option usage. Such a
> person might be led to wonder if there is some subtle difference
> between the short and long forms,

I don't think that we should care about people who might wonder if
there are subtle differences between short and long forms, because of
such tests.

The doc says that there is no difference, and there is also no
difference in other git commands, and git replace is an advanced
command, and very few regular users probably read tests, ...

> and then unnecessarily spend time
> digging into the code and documentation in an attempt to figure it
> out. Alternately, someone reading the code might be led to assume that
> the person who added the tests was being sloppy.

Why not just assume that the person who added the tests is so great as
to even take care of testing the long options names?

> Hopefully, t0040-parse-options should already be proof enough that
> long options are correctly handled, 

Yeah, indeed!

> but if you really want to test
> them here too, it seems like a separate test would be more appropriate
> than randomly changing short form options to long in various tests.

Introducing some randomness in tests also has some value, you
know. And at the same time it doesn't even slow down the test suite
while adding separate tests would slow it down, and make readers spend
more time reading the whole test suite, and would be a bigger burden
for git developers to maintain for no good reason.

Best,
Christian.