threads / patch / 34818

v3, 11 partsreplace: forbid replacing an object with one of a different type

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

## tl;dr

33 messages between Aug 31, 2013 and Sep 3, 2013. Diffs are folded; open one to read it.

replies: 32people: 4as markdown or json

Christian Couder· Aug 31, 2013, 19:12 UTC · lore

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

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
Christian Couder· Aug 31, 2013, 19:12 UTC · re: Christian Couder · lore

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(+)
Show changes to builtin/replace.c +10 −0
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
Philip Oakley· Aug 31, 2013, 22:11 UTC · re: Christian Couder · lore

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

Sorry for not replying earlier in the series.
From: "Christian Couder" <chriscool@tuxfamily.org>
Show 9 quoted lines
> 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).

Show 58 quoted lines
> 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
>
>
>
Christian Couder· Sep 1, 2013, 11:53 UTC · re: Philip Oakley · lore

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

From: "Philip Oakley" <philipoakley@iee.org>
Show 22 quoted lines
>
> 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.

Philip Oakley· Sep 1, 2013, 19:26 UTC · re: Christian Couder · lore

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

From: "Christian Couder" <chriscool@tuxfamily.org>
Show 30 quoted lines
> 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.
>
Christian Couder· Aug 31, 2013, 19:12 UTC · re: Christian Couder · lore

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

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(-)
Show changes to Documentation/git-replace.txt +4 −3
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
Christian Couder· Aug 31, 2013, 19:12 UTC · re: Christian Couder · lore

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)
Show changes to t/t6050-replace.sh +13 −0
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
Christian Couder· Aug 31, 2013, 19:12 UTC · re: Christian Couder · lore

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 6 ++++++
 1 file changed, 6 insertions(+)
Show changes to t/t6050-replace.sh +6 −0
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
Christian Couder· Aug 31, 2013, 19:12 UTC · re: Christian Couder · lore

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

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(+)
Show changes to Documentation/git-replace.txt +16 −0
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
Philip Oakley· Aug 31, 2013, 22:19 UTC · re: Christian Couder · lore

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

From: "Christian Couder" <chriscool@tuxfamily.org>
Show 22 quoted lines
> 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)

Show 30 quoted lines
> 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
>
>
>
Christian Couder· Sep 1, 2013, 10:27 UTC · re: Philip Oakley · lore

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

From: "Philip Oakley" <philipoakley@iee.org>
Show 11 quoted lines
>
> 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.

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

Christian Couder· Aug 31, 2013, 19:12 UTC · re: Christian Couder · lore

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

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(-)
Show changes to builtin/replace.c +1 −1
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
Christian Couder· Aug 31, 2013, 19:12 UTC · re: Christian Couder · lore

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-replace.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to Documentation/git-replace.txt +3 −1
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
Philip Oakley· Aug 31, 2013, 22:16 UTC · re: Christian Couder · lore

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

From: "Christian Couder" <chriscool@tuxfamily.org>
Show 23 quoted lines
> 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.

Show 5 quoted lines
> +
> Replacement references will be used by default by all Git commands
> except those doing reachability traversal (prune, pack transfer and
> fsck).
> --
Christian Couder· Sep 1, 2013, 11:49 UTC · re: Philip Oakley · lore

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

From: "Philip Oakley" <philipoakley@iee.org>
Show 17 quoted lines
>
> 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.

Philip Oakley· Sep 1, 2013, 20:11 UTC · re: Christian Couder · lore

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

From: "Christian Couder" <chriscool@tuxfamily.org>
Show 26 quoted lines
> 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)

Show 10 quoted lines
>> 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

Christian Couder· Sep 2, 2013, 06:11 UTC · re: Philip Oakley · lore

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

From: "Philip Oakley" <philipoakley@iee.org>
Show 26 quoted lines
>
> 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.

Philip Oakley· Sep 2, 2013, 21:50 UTC · re: Christian Couder · lore

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

From: "Christian Couder" <chriscool@tuxfamily.org>
Show 48 quoted lines
> 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.

Show 32 quoted lines
>        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
Jonathan Nieder· Sep 2, 2013, 21:55 UTC · re: Philip Oakley · lore

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

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

Philip Oakley· Sep 2, 2013, 22:13 UTC · re: Jonathan Nieder · lore

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

From: "Jonathan Nieder" <jrnieder@gmail.com>
Show 21 quoted lines
> 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
Jonathan Nieder· Sep 2, 2013, 22:26 UTC · re: Philip Oakley · lore

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

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

Philip Oakley· Sep 2, 2013, 22:45 UTC · re: Jonathan Nieder · lore

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

From: "Jonathan Nieder" <jrnieder@gmail.com>
Show 10 quoted lines
> 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.
Show 31 quoted lines
> 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
Christian Couder· Sep 3, 2013, 09:29 UTC · re: Philip Oakley · lore

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

On Mon, Sep 2, 2013 at 11:50 PM, Philip Oakley <philipoakley@iee.org> wrote:
Show 25 quoted lines
>
> 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
Show 27 quoted lines
>>        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.

Christian Couder· Aug 31, 2013, 19:12 UTC · re: Christian Couder · lore

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 6 ++++++
 1 file changed, 6 insertions(+)
Show changes to t/t6050-replace.sh +6 −0
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
Eric Sunshine· Sep 1, 2013, 07:50 UTC · re: Christian Couder · lore

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

On Sat, Aug 31, 2013 at 3:12 PM, Christian Couder <chriscool@tuxfamily.org> wrote:

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

Show 6 quoted lines
> +
>  test_expect_success 'replace ref cleanup' '
>         test -n "$(git replace)" &&
>         git replace -d $(git replace) &&
> --
> 1.8.4.rc1.31.g530f5ce.dirty
Christian Couder· Sep 1, 2013, 10:02 UTC · re: Eric Sunshine · lore

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

From: Eric Sunshine <sunshine@sunshineco.com>
Show 28 quoted lines
>
> 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.

Christian Couder· Aug 31, 2013, 19:12 UTC · re: Christian Couder · lore

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

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(-)
Show changes to builtin/replace.c +3 −3
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
Christian Couder· Aug 31, 2013, 19:12 UTC · re: Christian Couder · lore

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-replace.txt | 3 +++
 1 file changed, 3 insertions(+)
Show changes to Documentation/git-replace.txt +3 −0
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
Christian Couder· Aug 31, 2013, 19:12 UTC · re: Christian Couder · lore

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

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(-)
Show changes to t/t6050-replace.sh +4 −4
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
Philip Oakley· Aug 31, 2013, 22:19 UTC · re: Christian Couder · lore

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

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/
Show 47 quoted lines
>
> 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
>
>
Eric Sunshine· Sep 1, 2013, 08:07 UTC · re: Christian Couder · lore

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

On Sat, Aug 31, 2013 at 3:12 PM, Christian Couder <chriscool@tuxfamily.org> wrote:

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

Show 22 quoted lines
>       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
Christian Couder· Sep 1, 2013, 10:01 UTC · re: Eric Sunshine · lore

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

From: Eric Sunshine <sunshine@sunshineco.com>
Show 21 quoted lines
>
>> 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.

← back to recent threads