[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

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 <redacted>
Signed-off-by: Christian Couder <redacted>
---
 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

Signed-off-by: Christian Couder <redacted>
---
 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

Signed-off-by: Christian Couder <redacted>
---
 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

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 <redacted>
---
 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

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

Signed-off-by: Christian Couder <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>
---
 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

Signed-off-by: Christian Couder <redacted>
---
 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

Signed-off-by: Christian Couder <redacted>
---
 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

So that they are tested a litlle bit too.

Signed-off-by: Christian Couder <redacted>
---
 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

Signed-off-by: Christian Couder <redacted>
---
 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

Sorry for not replying earlier in the series.

From: "Christian Couder" <redacted>

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).

From: "Christian Couder" <redacted>

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).
-- 

From: "Christian Couder" <redacted>

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)

From: "Christian Couder" <redacted>
Subject: [PATCH v3 11/11] t6050-replace: use some long option names

So that they are tested a litlle bit too.

s /litlle/little/

On Sat, Aug 31, 2013 at 3:12 PM, Christian Couder
[off-list ref] wrote:

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

On Sat, Aug 31, 2013 at 3:12 PM, Christian Couder
[off-list ref] wrote:

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.

From: Eric Sunshine <redacted>

quoted

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.

From: Eric Sunshine <redacted>

On Sat, Aug 31, 2013 at 3:12 PM, Christian Couder
[off-list ref] wrote:

quoted

Signed-off-by: Christian Couder <redacted>
---
 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.

From: "Philip Oakley" <redacted>

quoted

So that they are tested a litlle bit too.

s /litlle/little/

Thanks,
Christian.

From: "Philip Oakley" <redacted>

From: "Christian Couder" <redacted>

quoted

+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.

quoted

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.

From: "Philip Oakley" <redacted>

From: "Christian Couder" <redacted>

quoted

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.

From: "Philip Oakley" <redacted>

Sorry for not replying earlier in the series.

From: "Christian Couder" <redacted>

quoted

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.

From: "Christian Couder" <redacted>

From: "Philip Oakley" <redacted>

quoted

Sorry for not replying earlier in the series.

From: "Christian Couder" <redacted>

quoted

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.

From: "Christian Couder" <redacted>

From: "Philip Oakley" <redacted>

quoted

From: "Christian Couder" <redacted>

quoted

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)

quoted

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 

From: "Philip Oakley" <redacted>

From: "Christian Couder" <redacted>

quoted

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.

From: "Christian Couder" <redacted>

From: "Philip Oakley" <redacted>

quoted

From: "Christian Couder" <redacted>

quoted

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 

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

From: "Jonathan Nieder" <redacted>

Hi,

Philip Oakley wrote:

quoted

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 

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

From: "Jonathan Nieder" <redacted>

Philip Oakley wrote:

quoted

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 

On Mon, Sep 2, 2013 at 11:50 PM, Philip Oakley [off-list ref] wrote:

From: "Christian Couder" <redacted>

quoted

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

quoted

       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.