Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2f5f088
Add a parameter --commit-to-reparent for the reparent command so
Jun 8, 2018
df2029c
Fix bug in the reparent logic - instead of using "parents" option each
Jun 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 57 additions & 4 deletions git-imerge
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sys.stdout.write('Reparenting from HEAD to %s...\n' % options.commit_to_reparent)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if options.commit_to_reparent is None:
commit_to_reparent = 'HEAD'
else:
commit_to_reparent = options.commit_to_reparent
git = GitRepository()
try:
commit_sha1 = git.get_commit_sha1('HEAD')
commit_to_reparent_sha1 = git.get_commit_sha1(commit_to_reparent)
except ValueError:
sys.exit('%s is not a valid commit', commit_to_reparent)

try:
head_sha1 = git.get_commit_sha1('HEAD')
except ValueError:
sys.exit('HEAD is not a valid commit')

Expand All @@ -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),))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

iterative_reparent(head_sha1, commit_to_reparent_sha1, parent_sha1s, git)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

current_parents = git.get_commit_parents(current_commit)
first_parent_commit = current_parents[0]
commit_list.append(first_parent_commit)
current_commit = first_parent_commit

commit_list.reverse()
for i in range(0, len(commit_list)):
if i == 0:
current_parents = parents
else:
current_parents = git.get_commit_parents(commit_list[i])
current_parents = [new_commit] + current_parents[1:len(current_parents)]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


new_commit = git.reparent(commit_list[i], current_parents)
sys.stdout.write('Commit to reparent: %s, new commit: %s, new parents: %s\n' % (commit_list[i], new_commit, current_parents))

def main(args):
NAME_INIT_HELP = 'name to use for this incremental merge'
Expand Down Expand Up @@ -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):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

subparser.add_argument(
'--commit-to-reparent', dest='commit_to_reparent', action='store', default=None,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

help=(
'target commit to reparent. reparent will happen from this commit all the way back to HEAD. '
'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.'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please limit lines to 79 characters.

),
)

parser = argparse.ArgumentParser(
description=__doc__,
formatter_class=argparse.RawDescriptionHelpFormatter,
Expand Down Expand Up @@ -3927,12 +3971,21 @@ def main(args):

subparser = subparsers.add_parser(
'reparent',
help='change the parents of the HEAD commit',
help=(
'change the parents of the specified commit and propagate the change to HEAD commit'
),
)
subparser.add_argument(
'parents', nargs='*', help='[PARENT...]',
'parents',
nargs='*',
help=(
'[PARENT...] \n'
'parents is a list of SHA1.\n'
),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

add_commit_to_reparent_argument(subparser)

options = parser.parse_args(args)

# Set an environment variable GIT_IMERGE=1 while we are running.
Expand Down