Add a parameter --commit-to-reparent for the reparent command so #130

make1980-zz · 2018-06-08T17:28:41Z

that a list of commits starting from a specified commit to HEAD can be reparented in a cascading manner.

reparent operation should use individual commit's parent to reconstruct the parents.

mhagger

Thanks for the patch! Sorry it took so long for me to review it. I left a bunch of comments on it.

I will write up a modified version of your patch that works correctly for more complicated commit topologies. I'm curious if it works for you.

mhagger · 2018-06-24T06:21:34Z

git-imerge

@@ -3717,6 +3749,18 @@ def main(args):
            help='the tip of the branch to be merged into HEAD',
        )

+    def add_commit_to_reparent_argument(subparser):


There's no need for this to be in a function, since it is only called once. (add_*_option() functions are used when the same option applies to multiple commands.)

mhagger · 2018-06-24T06:23:47Z

git-imerge

@@ -3717,6 +3749,18 @@ def main(args):
            help='the tip of the branch to be merged into HEAD',
        )

+    def add_commit_to_reparent_argument(subparser):
+        subparser.add_argument(
+            '--commit-to-reparent', dest='commit_to_reparent', action='store', default=None,


I think it would be ok to name this option --commit.

dest is redundant because that's the default.

action is redundant because 'store' is the default action.

default=None is redundant because that is the default. However, don't you actually want default='HEAD' here? That would remove the need for cmd_reparent to check this argument and set it to HEAD if it was missing.

mhagger · 2018-06-24T06:24:08Z

git-imerge

+                'reparent operation creates a new commit object like the original commit, but with the specified parents. '
+                'reparent command first executes the reparent operation on the commit specified by the --commit-to-reparent argument to generate a new commit object, '
+                'then it uses this object to replace the first parent of its child commit on the path towards the HEAD commit.\n'
+                'the command repeats this process till the HEAD commit so that a list of new commit objects are created.'


Please limit lines to 79 characters.

mhagger · 2018-06-24T06:25:11Z

git-imerge

+        help=(
+            '[PARENT...] \n'
+            'parents is a list of SHA1.\n'
+            ),


Newline characters don't have an effect in help strings. And instead of starting the help string with [PARENT...], you can set metavar='PARENT'.

mhagger · 2018-06-24T06:26:15Z

git-imerge

@@ -3629,9 +3629,22 @@ def cmd_diagram(parser, options):


 def cmd_reparent(parser, options):
+    if len(options.parents) == 0:
+        parser.error('Parent is not specified, please specify at least one parent.')


Why do you add this new restriction? It is perfectly sensible to set a commit to have zero parents; in that case, the commit becomes an orphan commit (i.e., like the first commit made in a fresh repository).

mhagger · 2018-06-24T06:28:43Z

git-imerge

@@ -3640,8 +3653,27 @@ def cmd_reparent(parser, options):
    except ValueError as e:
        sys.exit(e.message)

-    sys.stdout.write('%s\n' % (git.reparent(commit_sha1, parent_sha1s),))


It is not OK to remove this output or complicate it. The reparent command is pretty useless if you don't do anything with its output (because it doesn't change any references itself). And likely people want to use the output directly; e.g.,

git update-ref refs/heads/mynewbranch $(git imerge reparent $PARENT)

This works with the old code because the only thing that it outputs to stdout is the final SHA-1. But it won't work with your changes.

mhagger · 2018-06-24T06:29:15Z

git-imerge

@@ -3629,9 +3629,22 @@ def cmd_diagram(parser, options):


 def cmd_reparent(parser, options):
+    if len(options.parents) == 0:
+        parser.error('Parent is not specified, please specify at least one parent.')
+    sys.stdout.write('Reparenting from HEAD to %s...\n' % options.commit_to_reparent)


It is not appropriate to emit progress messages to stdout. They should go to stderr so that the stdout can be piped to another program.

mhagger · 2018-06-24T06:31:56Z

git-imerge

+def iterative_reparent(head, commit_to_reparent, parents, git):
+    commit_list = [head];
+    current_commit = head;
+    while current_commit != commit_to_reparent:


This loop will totally fail (with an IndexError, I believe) if commit_to_reparent is not a first-parent ancestor of head. I understand that this will never be the case the way you want to use the program, but other people might want to use it other ways, and it shouldn't crash for them.

It's not very much harder to rewrite this so that it works for any ancestor of head (not just first-parent ancestors). I'll try to implement that.

mhagger · 2018-06-24T06:46:28Z

git-imerge

+            current_parents = parents
+        else:
+            current_parents = git.get_commit_parents(commit_list[i])
+            current_parents = [new_commit] + current_parents[1:len(current_parents)]


This is going to do something screwy if there is more than one ancestry path between commit_to_reparent and head. For example, suppose that there was a merge in between:

o---A---C---D---E---F---H / \ / o---B X---Y

and you run git imerge reparent --commit-to-reparent C A. This algorithm will only rewrite commits C, D, E, F, and H, but won't rewrite X or Y. The result will not be the hoped-for

o---A---C'--D'--E'--F'--H' \ / X'--Y'

but I think will look like this instead:

C'--D'--E'------F'--H' / / o---A---C---D / / \ / o---B X---Y

I understand that you are not interested in this case, but it's not that much harder to handle it correctly.

Add an option `git imerge reparent --commit=COMMIT`, which allows the parents of an arbitrary commit to be changed. Moreover, the descendants of COMMIT are also rewritten all the way to HEAD. This feature was suggested by Ke Ma <[email protected]>, and the implementation is partly derived from PR #130 submitted by Ke Ma.

Add a parameter --commit-to-reparent for the reparent command so

2f5f088

that a list of commits starting from a specified commit to HEAD can be reparented in a cascading manner.

make1980-zz mentioned this pull request Jun 8, 2018

Generate Rebase-with-history Result based on Full Result #129

Closed

Fix bug in the reparent logic - instead of using "parents" option each

df2029c

reparent operation should use individual commit's parent to reconstruct the parents.

mhagger requested changes Jun 24, 2018

View reviewed changes

mhagger mentioned this pull request Jun 24, 2018

reparent: add a --commit option #132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a parameter --commit-to-reparent for the reparent command so #130

Add a parameter --commit-to-reparent for the reparent command so #130

make1980-zz commented Jun 8, 2018

mhagger left a comment

mhagger Jun 24, 2018

mhagger Jun 24, 2018

mhagger Jun 24, 2018

mhagger Jun 24, 2018

mhagger Jun 24, 2018

mhagger Jun 24, 2018

mhagger Jun 24, 2018

mhagger Jun 24, 2018

mhagger Jun 24, 2018

Add a parameter --commit-to-reparent for the reparent command so #130

Are you sure you want to change the base?

Add a parameter --commit-to-reparent for the reparent command so #130

Conversation

make1980-zz commented Jun 8, 2018

mhagger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment