Re: git replace: should it check for object type, and can it replace merges?

2013-08-05 Thread Philip Oakley

From: Christian Couder christian.cou...@gmail.com

Hi,
On Sat, Aug 3, 2013 at 5:13 PM, Philip Oakley philipoak...@iee.org 
wrote:
A recent comment http://stackoverflow.com/a/18027030/717355 on a 
question I
asked two years ago about 'grafts' and 'replace' indicates that users 
think
that 'git replace' can't replace a merge commit. The documentation 
doesn't

have any examples and gives the naive impression that one should only
replace a simple commit with another simple commit.


I am sorry if the documentation gives this impression.
I'd like to fix it, but I am not sure what should be changed.
Should adding an example be enough? Or do you want it to state that 
explicitely?


I did a quick markup which is shown below (an export of the commit from 
the gitk viewer)


Having looked at the code, I realised that anything can be replaced 
with

anything, which is perhaps not what was intended.


The documentation says in the BUGS section:

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


I hadn't seen that part, being 'hidden' at the end of the paragraph 
(that is, it's easy to miss;-). If my update was acceptable then that 
sentence could probably be deleted (though it may require the checks to 
actually be in the code, and not just a must imperative in my 
documentation update).




So yes it is a know bug.


A simple thought is that
the replace should be like-for-like with regard to object type, 
though that

would not include replacing a sub-module for a tree (and vice versa).

Should 'git replace' check the object types to ensure they are 
sensible?


It would probably be a good idea to do that, yeah.
But I don't know much about submodules, so I can't say if replacing a
sub-module for a tree (and vice versa) should be allowed.
Or if there should be a --force-different-objects option for these
kinds of special cases.


An extra bit of thought made me realise that while a sub-module is 
represented as a special symbolic commit, it is still just an element of 
a tree object, so would still be a tree - tree replacement, so doesn't 
break the rule.





Would it be reasonable to add examples to indicate the range of
replacements, and how to prepare alternative merge commits, or is 
that a

hostage to fortune?


Yeah, adding examples would be a good idea. I don't understand what do
you mean with range of replacements, though.


There were in two parts: 1) creating a replacement merge commit, and 2) 
creating a replacement tree,  possibly with a sub-module in it.




I am not sure preparing alternative commits or merge commits should be
an important part of the examples.

There are many cases that could be interesting to different users:

- replacing a non merge commit with a merge commit (if someone forgot
to use --no-ff when merging for example)
- replacing a merge commit with a non merge commit (if a rebase should
have been done)
- and of course replacing a non merge commit with a non merge commit,
or a merge commit with a merge commit

So I think explaining how another commit can be created from existing
commits belongs to some other parts of the git documentation.


Yes, I just haven't looked yet. I think there are some examples in the 
plumbing command man pages. They'd just need referencing.



Perhaps there could be such examples in the git hash-object and git
filter-branch documentation and we could just point to them.

Best,
Christian.
--


My quick markup, based on a local branch.
---8---
commit c12c03462f8c65a593e702896b461f1c63d67ec5
Author: Philip Oakley philipoak...@iee.org
Date:   Sat Aug 3 20:20:05 2013 +0100

   Doc: 'replace' the same object type, and mention merge commits

   Signed-off-by: Philip Oakley philipoak...@iee.org

diff --git a/Documentation/git-replace.txt 
b/Documentation/git-replace.txt

index e0b4057..2ab451c 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -20,6 +20,10 @@ 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 type of the replacement object must be the same as that of the
+object it replaces. Merge commits can be replaced by non-merge commits
+and vice versa.
+
Unless `-f` is given, the 'replace' reference must not yet exist.

Replacement references will be used by default by all Git commands
---8---

Philip 


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: git replace: should it check for object type, and can it replace merges?

2013-08-03 Thread Thomas Rast
Philip Oakley philipoak...@iee.org writes:

 A recent comment http://stackoverflow.com/a/18027030/717355 on a
 question I asked two years ago about 'grafts' and 'replace' indicates
 that users think that 'git replace' can't replace a merge commit. The
 documentation doesn't have any examples and gives the naive impression
 that one should only replace a simple commit with another simple
 commit.

 Having looked at the code, I realised that anything can be replaced
 with anything, which is perhaps not what was intended. A simple
 thought is that the replace should be like-for-like with regard to
 object type, though that would not include replacing a sub-module for
 a tree (and vice versa).

Off hand I cannot come up with any case where you can replace one object
with one of a different type, keeping the history valid.

To back that up:

* Refs can be replaced simply by changing the ref itself.

* 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 100755blob
16   commit
04   tree
  (these are the only valid modes)

* Blobs don't point at anything.

So yes:

 Should 'git replace' check the object types to ensure they are sensible?

I think that would be a very good thing to check.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html