Re: [PATCH RFC 4/4] rebase -i: add --refs option to rewrite heads within branch

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 22, 2009, 23:37 UTC
Message-ID: <7vzl5awpf1.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20091222222316.GY30538@dr-wily.mit.edu>

Greg Price <price@ksplice.com> writes:

Show 25 quoted lines

> The new option --refs causes the TODO file to contain a "ref" command
> for each head pointing to a selected commit, other than the one we are
> already rebasing.  The effect of this is that when a branch contains
> intermediate branches, like so:
>
>       part1 part2 topic
>         |     |     |
>         v     v     v
>   A--*--*--*--*--*--*
>    \
>     B <--master
>
> a single command like "git rebase -i --refs master topic" suffices to
> rewrite all the heads that are part of the topic, like so:
>
>         part1 part2 topic
>   A       |     |     |
>    \      v     v     v
>     B--*--*--*--*--*--*
>     ^
>     |
>     master
>
> Signed-off-by: Greg Price <price@ksplice.com>
> ---

Two comments and a half.

 - As decoration is a fairly expensive operation (which is the reason why
   loading_ref_decorations() is called lazily by format_decoration() in
   the first place, especially in repositories with tons of refs), you
   shouldn't give --format=%D to rev-list when the new feature is not
   asked for.

 - This seems to rewrite only branch heads; don't you want to allow users
   to rewrite lightweight tags and possibly annotated ones as well, by
   perhaps giving "--rewrite-refs=refs/heads/" or "--rewrite-refs=refs/"
   to limit what parts of the ref namespace to consider rewriting?

 - Otherwise the option should not be called "refs" but be named using the
   word "branch" to clarify that it affects _only_ branches.

Obviously the series also needs tests.

I also have to wonder if this feature should also handle a case like this:

                  side
                  |
                  V
                  *
                 /
        part1   *    topic
          |    /      |
          v   /       v
    A--*--*--*--*--*--*
     \
      B <--master

===>

                     side
                     |
                     V
                     *
                    /
           part1   *    topic
     A       |    /      |
      \      v   /       v
       B--*--*--*--*--*--*
       ^
       |
       master

especially if it were to be specific to branch management.

On the other hand, if the "partN" markers in your example workflow are primarily meant to be used to mark places on a branch (as opposed to arbitrary branch tips that independent development starting from them can further continue), it would make a lot more sense to use lightweight or annotated tags for them, and instead of "--refs" that rewrites only other branch tips, it might make a lot more sense to have "--rewrite-tags" that rewrites tags that point at the commits that are rewritten, without touching any branch tip.