[PATCH 1/1] Add --first-parent support to interactive rebase.

[PATCH 1/1] Add --first-parent support to interactive rebase.

[Björn Steinbrink]

By default, rebase will take all commits from the branch that is to be
rebased which are missing in upstream. The new --first-parent option
allows to just follow the first parent and thus completely ignore
merges.

Additionally, when used together with --preserve-merges (which is the
more useful use-case) it will no longer rebase the commits from the
merged-in branches, but instead redo the merge with the original
parents.

That means that:
     ---H------I topicB
    /    \      \
    | D---E---F--G topicA
    |/
    A---B---C master

does no longer become:
                -H'--------I'
               /  \         \
              D'---E'---F'---G' topicA
             /
    A---B---C master
     \
      H---I topicB

but instead:
    A---B---C master
     \       \
      \       D'---E'---F'---G' topicA
       \          /         /
        ---------H---------I topicB

Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index e4326d3..0b5f4b6 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git-rebase' [-i | --interactive] [-v | --verbose] [-m | --merge]
 	[-C<n>] [ --whitespace=<option>] [-p | --preserve-merges]
-	[--onto <newbase>] <upstream> [<branch>]
+	[--first-parent] [--onto <newbase>] <upstream> [<branch>]
 'git-rebase' --continue | --skip | --abort
 
 DESCRIPTION
@@ -166,6 +166,52 @@ This is useful if F and G were flawed in some way, or should not be
 part of topicA.  Note that the argument to --onto and the <upstream>
 parameter can be any valid commit-ish.
 
+If you have a branch that contains merges which you want to preserve, you
+can use the --preserve-merges option. By default, this will duplicate all
+merged commits on top of <upstream> (or <newbase>) and try to redo the
+merges using the new commits. Given this situation:
+
+------------
+     ---H------I topicB
+    /    \      \
+    | D---E---F--G topicA
+    |/
+    A---B---C master
+------------
+
+then the command
+
+    git rebase -i --preserve-merges master topicA
+
+would result in
+
+------------
+                -H'--------I'
+               /  \         \
+              D'---E'---F'---G' topicA
+             /
+    A---B---C master
+     \
+      H---I topicB
+------------
+
+If you pass --first-parent in addition to --preserve-merges, the commits
+from the merged-in branches won't be duplicated.
+
+So the command
+
+    git rebase -i --preserve-merges --first-parent master topicA
+
+would instead result in
+
+------------
+    A---B---C master
+     \       \
+      \       D'---E'---F'---G' topicA
+       \          /         /
+        ---------H---------I topicB
+------------
+
 In case of conflict, git-rebase will stop at the first problematic commit
 and leave conflict markers in the tree.  You can use git diff to locate
 the markers (<<<<<<) and make edits to resolve the conflict.  For each
@@ -246,6 +292,13 @@ OPTIONS
 	Instead of ignoring merges, try to recreate them.  This option
 	only works in interactive mode.
 
+\--first-parent::
+	Only follow the first parent commits in merge commits when looking
+	for the commits that are to be rebased. This is most useful with -p
+	as it will cause rebase to recreate the merges against the original
+	branches instead of rebasing those branches as well. This option
+	only works in interactive mode.
+
 include::merge-strategies.txt[]
 
 NOTES
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e63e1c9..38b070e 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -22,6 +22,7 @@ DONE="$DOTEST"/done
 MSG="$DOTEST"/message
 SQUASH_MSG="$DOTEST"/message-squash
 REWRITTEN="$DOTEST"/rewritten
+FIRST_PARENT=
 PRESERVE_MERGES=
 STRATEGY=
 VERBOSE=
@@ -143,14 +144,20 @@ pick_one_preserving_merges () {
 			preserve=f
 			new_p=$(cat "$REWRITTEN"/$p)
 			test $p != $new_p && fast_forward=f
-			case "$new_parents" in
-			*$new_p*)
-				;; # do nothing; that parent is already there
-			*)
-				new_parents="$new_parents $new_p"
-				;;
-			esac
+		elif test t = "$FIRST_PARENT"
+		then
+			new_p=$p
+		else
+			continue
 		fi
+
+		case "$new_parents" in
+		*$new_p*)
+			;; # do nothing; that parent is already there
+		*)
+			new_parents="$new_parents $new_p"
+			;;
+		esac
 	done
 	case $fast_forward in
 	t)
@@ -412,6 +419,9 @@ do
 	-p|--preserve-merges)
 		PRESERVE_MERGES=t
 		;;
+	--first-parent)
+		FIRST_PARENT=t
+		;;
 	-i|--interactive)
 		# yeah, we know
 		;;
@@ -481,6 +491,10 @@ do
 		else
 			MERGES_OPTION=--no-merges
 		fi
+		if test t = "$FIRST_PARENT"
+		then
+			MERGES_OPTION="--first-parent $MERGES_OPTION"
+		fi
 
 		SHORTUPSTREAM=$(git rev-parse --short $UPSTREAM)
 		SHORTHEAD=$(git rev-parse --short $HEAD)
-- 
1.5.3.4.456.g072a

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1066 bytes --]

Hi,

On Wed, 31 Oct 2007, Björn Steinbrink wrote:

> @@ -246,6 +292,13 @@ OPTIONS
>  	Instead of ignoring merges, try to recreate them.  This option
>  	only works in interactive mode.
>  
> +\--first-parent::
> +	Only follow the first parent commits in merge commits when looking
> +	for the commits that are to be rebased. This is most useful with -p
> +	as it will cause rebase to recreate the merges against the original
> +	branches instead of rebasing those branches as well. This option
> +	only works in interactive mode.
> +

Hmm.  I had to read this several times to understand it.  Maybe something 
like this instead?

\--first-parent::
	When you want to preserve merges, this option allows you to rebase 
	only the commits which were not merged in, i.e. which are in the
	first parent ancestry of the current HEAD.
+
This option only makes sense together with --preserve-merges.

Also, could you please add a test case to make sure that your patch works 
as advertised (and that this functionality will not be broken in future 
commits)?

Thanks,
Dscho

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Björn Steinbrink]

On 2007.10.31 03:34:47 +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Wed, 31 Oct 2007, Björn Steinbrink wrote:
> 
> > @@ -246,6 +292,13 @@ OPTIONS
> >  	Instead of ignoring merges, try to recreate them.  This option
> >  	only works in interactive mode.
> >  
> > +\--first-parent::
> > +	Only follow the first parent commits in merge commits when looking
> > +	for the commits that are to be rebased. This is most useful with -p
> > +	as it will cause rebase to recreate the merges against the original
> > +	branches instead of rebasing those branches as well. This option
> > +	only works in interactive mode.
> > +
> 
> Hmm.  I had to read this several times to understand it.  Maybe something 
> like this instead?
> 
> \--first-parent::
> 	When you want to preserve merges, this option allows you to rebase 
> 	only the commits which were not merged in, i.e. which are in the
> 	first parent ancestry of the current HEAD.
> +
> This option only makes sense together with --preserve-merges.

Hm, I think that it might make might sense without -p. Say that your
topic branch is following two other branches like this:

---o---o---o--------o topicB
            \        \
--o---A---o---o---o---o---B topicA
         /       /
o---o---o---o---o master

topicB branched off from master earlier than topicA and you currently
require stuff from master..topicB _and_ topicB..master, so AFAICT, you
need sth. like the above.

Let's say that topicB simplifies some internal API and you desperately
wanted to use that, while master introduced some new stuff that you also
use. Now your stuff is finished, but it becomes obvious that topicB is
still too broken to go into master any time soon. Then you could do:

git rebase -i --first-parent master topicA

to get:

--o---o---o topicB (branched from master somewhere to the left)

             o---o---o---A---B topicA
            /
---o---o---o master

Depending on how much topicA really depends on topicB, you might need to
fix a bunch of stuff, but it might be worth it.

How about:
\--first-parent::
	When this option is given and --preserve-merges is not, then
	merge commits are completely ignored and only commits from the
	first parent ancestry are rebased. This allows to pretend that
	merges never happened.

	If --preserve-merges is also given, the merge commits are
	preserved, but only their first parent is rebased as opposed to
	the default behaviour which would rebase all parents.

> Also, could you please add a test case to make sure that your patch works 
> as advertised (and that this functionality will not be broken in future 
> commits)?

Ok, might take some time, as I currently have no clue how the test stuff
for git works :-/ Well, I'm sure #git will be helpful :-)

Thanks,
Björn

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Johannes Schindelin]

Hi,

On Wed, 31 Oct 2007, Bj?rn Steinbrink wrote:

> On 2007.10.31 03:34:47 +0000, Johannes Schindelin wrote:
> 
> > On Wed, 31 Oct 2007, Bj?rn Steinbrink wrote:
> > 
> > > @@ -246,6 +292,13 @@ OPTIONS
> > >  	Instead of ignoring merges, try to recreate them.  This option
> > >  	only works in interactive mode.
> > >  
> > > +\--first-parent::
> > > +	Only follow the first parent commits in merge commits when looking
> > > +	for the commits that are to be rebased. This is most useful with -p
> > > +	as it will cause rebase to recreate the merges against the original
> > > +	branches instead of rebasing those branches as well. This option
> > > +	only works in interactive mode.
> > > +
> > 
> > Hmm.  I had to read this several times to understand it.  Maybe 
> > something like this instead?
> > 
> > \--first-parent::
> > 	When you want to preserve merges, this option allows you to rebase 
> > 	only the commits which were not merged in, i.e. which are in the
> > 	first parent ancestry of the current HEAD.
> > +
> > This option only makes sense together with --preserve-merges.
> 
> Hm, I think that it might make might sense without -p. Say that your 
> topic branch is following two other branches like this:
> 
> ---o---o---o--------o topicB
>             \        \
> --o---A---o---o---o---o---B topicA
>          /       /
> o---o---o---o---o master
> 
> topicB branched off from master earlier than topicA and you currently
> require stuff from master..topicB _and_ topicB..master, so AFAICT, you
> need sth. like the above.
> 
> Let's say that topicB simplifies some internal API and you desperately
> wanted to use that, while master introduced some new stuff that you also
> use. Now your stuff is finished, but it becomes obvious that topicB is
> still too broken to go into master any time soon. Then you could do:
> 
> git rebase -i --first-parent master topicA
> 
> to get:
> 
> --o---o---o topicB (branched from master somewhere to the left)
> 
>              o---o---o---A---B topicA
>             /
> ---o---o---o master
> 
> Depending on how much topicA really depends on topicB, you might need to
> fix a bunch of stuff, but it might be worth it.

Okay, I see now.

> How about:
> \--first-parent::
> 	When this option is given and --preserve-merges is not, then
> 	merge commits are completely ignored and only commits from the
> 	first parent ancestry are rebased. This allows to pretend that
> 	merges never happened.
> 
> 	If --preserve-merges is also given, the merge commits are
> 	preserved, but only their first parent is rebased as opposed to
> 	the default behaviour which would rebase all parents.

Okay.

> > Also, could you please add a test case to make sure that your patch 
> > works as advertised (and that this functionality will not be broken in 
> > future commits)?
> 
> Ok, might take some time, as I currently have no clue how the test stuff 
> for git works :-/ Well, I'm sure #git will be helpful :-)

Just have a look at t/t3404-rebase-interactive.sh.  The easiest way to 
proceed would be to read it from the end.  You'll see that every test case 
starts with "test_expect_success", followed by a message and a piece of 
shell code.

I usually enhance some existing test script instead of inventing a new 
one.

In your case, I would run t3404 by

	cd t
	sh t3404*

The working directory of these tests is in the subdirectory trash/ of t/.  
After one run of t3404, I would go there and look at what is there with 
gitk.

In your case, you want to have at least a few merges.  Build them up in 
the test case, using echo, git add, git commit and git checkout.  Then run 
an appropriate git rebase -i --first-commit [-p], and test that the 
outcome makes sense.

You need not test _everything_.  Just the differences with regards to 
normal rebase.  For example, that a side branch is _not_ rebased, but 
"git rev-parse HEAD~2^2" is the same as before the rebase.

And remember to connect all commands with && so that a failure in one 
command leads to the failure of the whole test case.

Hth,
Dscho

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Wincent Colaiuta]

El 31/10/2007, a las 5:17, Björn Steinbrink escribió:

> How about:
> \--first-parent::
> 	When this option is given and --preserve-merges is not, then
> 	merge commits are completely ignored and only commits from the
> 	first parent ancestry are rebased. This allows to pretend that
> 	merges never happened.
>
> 	If --preserve-merges is also given, the merge commits are
> 	preserved, but only their first parent is rebased as opposed to
> 	the default behaviour which would rebase all parents.


Minor nitpick:

-	first parent ancestry are rebased. This allows to pretend that
+	first parent ancestry are rebased. This allows you to pretend that

Cheers,
Wincent

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Junio C Hamano]

Your MUA seems to mark the UTF-8 message you are sending out as
8859-1, which means your name in the message gets corrupt.

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

> By default, rebase will take all commits from the branch that is to be
> rebased which are missing in upstream. The new --first-parent option
> allows to just follow the first parent and thus completely ignore
> merges.
>
> Additionally, when used together with --preserve-merges (which is the
> more useful use-case) it will no longer rebase the commits from the
> merged-in branches, but instead redo the merge with the original
> parents.
>
> That means that:
>      ---H------I topicB

Please add a blank line after "means that:" for readability.

>     /    \      \
> ...
> does no longer become:
>                 -H'--------I'

Likewise; also, "no longer becomes:".

>                /  \         \
>               D'---E'---F'---G' topicA
>              /
>     A---B---C master
>      \
>       H---I topicB
>
> but instead:
>     A---B---C master

Likewise.

> ...
>         ---------H---------I topicB

And crucially, you forgot to say "... when you do X".

I am assuming that you meant:

    This (picture) becomes this (picture) instead of this (picture)
    when you run "git rebase -p -m master topicA".

but without it, the nice ASCII drawings loses their value.

It is somewhat disturbing that this treats the first parent too
special.

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Björn Steinbrink]

On 2007.10.30 22:05:27 -0700, Junio C Hamano wrote:
> Your MUA seems to mark the UTF-8 message you are sending out as
> 8859-1, which means your name in the message gets corrupt.

Hm, that would be git-send-email then, anything I need to configure?
(Actually I don't see it marking the message as anything)

> Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> 
> > By default, rebase will take all commits from the branch that is to be
> > rebased which are missing in upstream. The new --first-parent option
> > allows to just follow the first parent and thus completely ignore
> > merges.
> >
> > Additionally, when used together with --preserve-merges (which is the
> > more useful use-case) it will no longer rebase the commits from the
> > merged-in branches, but instead redo the merge with the original
> > parents.
> >
> > That means that:

Given this situation:

> >      ---H------I topicB
> >     /    \      \
> > ...
> > does no longer become:

Which results in:

> >                 -H'--------I'
> >                /  \         \
> >               D'---E'---F'---G' topicA
> >              /
> >     A---B---C master
> >      \
> >       H---I topicB

When you do "git-rebase -p -i master topicA"

You can now also get:

> >     A---B---C master
> > ...
> >         ---------H---------I topicB

When you do "git-rebase -p -i --first-parent master topicA"


That's better, right?

> And crucially, you forgot to say "... when you do X".
> 
> I am assuming that you meant:
> 
>     This (picture) becomes this (picture) instead of this (picture)
>     when you run "git rebase -p -m master topicA".
> 
> but without it, the nice ASCII drawings loses their value.

:-/

> It is somewhat disturbing that this treats the first parent too
> special.

The original use-case for the "-p -i --first-parent" case was a question
on #git, where someone had sth. like this:

   o---o---o---o---o remote/branch
        \           \
     o---o---o---o---o topicA
    /
   o---o---o master trunk

Now that guy was using git-svn to dcommit into svn from master. To
dcommit the changes from topicA he had to have that based on master, and
he wanted to preserve the merges from remote/branch to have them
squashed when dcommitted to svn. So what he wanted was:


     ...---o---o---o---o---o remote/branch
                \           \
             o---o---o---o---o topicA
            /
   o---o---o master trunk

The default behaviour of rebase would totally flat out the history and
instead of two sqaush merges (which he wanted), svn would've seen a huge
amount of commits comning from remote/branch. And the plain -p behaviour
would have duplicated all those branches from remote/branch for no good
reason, so I came up with that --first-parent thing.

Better ideas are welcome, I just don't know git well enough to come up
with anything better...

Thanks,
Björn

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Dmitry Potapov]

On Wed, Oct 31, 2007 at 06:53:03AM +0100, Björn Steinbrink wrote:
> On 2007.10.30 22:05:27 -0700, Junio C Hamano wrote:
> > Your MUA seems to mark the UTF-8 message you are sending out as
> > 8859-1, which means your name in the message gets corrupt.
> 
> Hm, that would be git-send-email then, anything I need to configure?
> (Actually I don't see it marking the message as anything)

I believe that the issue is with Junio's mail client. Indeed, the
context encoding for the mail *body* was specified as 8859-1, but
that should have none effect on fields in the mail header, because
any field is the header should be either printable ASCII or encoded
to contain only ASCII characters as specified in RFC 1522:

encoded-word = "=?" charset "?" encoding "?" encoded-text "?="

Here is the From field from the mail:

From:	=?utf-8?q?Bj=C3=B6rn=20Steinbrink?= <B.Steinbrink@gmx.de>

So, as far as I can tell, it is encoded properly using utf-8.

Dmitry

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Karl Hasselström]

On 2007-10-31 16:43:58 +0300, Dmitry Potapov wrote:

> I believe that the issue is with Junio's mail client. Indeed, the
> context encoding for the mail *body* was specified as 8859-1, but
> that should have none effect on fields in the mail header, because
> any field is the header should be either printable ASCII or encoded
> to contain only ASCII characters as specified in RFC 1522:

Yes. But it's the body that's been mangled -- specifically, the
Sign-off line.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Dmitry Potapov]

On Wed, Oct 31, 2007 at 03:00:28PM +0100, Karl Hasselström wrote:
> On 2007-10-31 16:43:58 +0300, Dmitry Potapov wrote:
> 
> > I believe that the issue is with Junio's mail client. Indeed, the
> > context encoding for the mail *body* was specified as 8859-1, but
> > that should have none effect on fields in the mail header, because
> > any field is the header should be either printable ASCII or encoded
> > to contain only ASCII characters as specified in RFC 1522:
> 
> Yes. But it's the body that's been mangled -- specifically, the
> Sign-off line.

Hmm... I looked at the mail again and I cannot see where 8859-1 is
specified. It seems that context encoding is not specified at all.
Of course, it is incorrect to use non ASCII characters in a mail
without specifying encoding. Apparently, because I use utf-8 in the
terminal, the Sign-off line displays correctly for me, so I did not
notice the problem. Sorry for the noise...

Dmitry

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Jeff King]

On Wed, Oct 31, 2007 at 05:36:41PM +0300, Dmitry Potapov wrote:

> Hmm... I looked at the mail again and I cannot see where 8859-1 is
> specified. It seems that context encoding is not specified at all.
> Of course, it is incorrect to use non ASCII characters in a mail
> without specifying encoding. Apparently, because I use utf-8 in the
> terminal, the Sign-off line displays correctly for me, so I did not
> notice the problem. Sorry for the noise...

It is our old friend vger adding the iso-8859-1 header, I think, since
no encoding was specified.

I think the problem is that git-format-patch only decides whether to
append a MIME header based on the commit message contents; it does not
take the Signed-Off-By into account. This may also be the cause of the
recent complaints from Matti Aarnio.

-Peff

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Björn Steinbrink]

On 2007.10.31 14:05:57 -0400, Jeff King wrote:
> On Wed, Oct 31, 2007 at 05:36:41PM +0300, Dmitry Potapov wrote:
> 
> > Hmm... I looked at the mail again and I cannot see where 8859-1 is
> > specified. It seems that context encoding is not specified at all.
> > Of course, it is incorrect to use non ASCII characters in a mail
> > without specifying encoding. Apparently, because I use utf-8 in the
> > terminal, the Sign-off line displays correctly for me, so I did not
> > notice the problem. Sorry for the noise...
> 
> It is our old friend vger adding the iso-8859-1 header, I think, since
> no encoding was specified.
> 
> I think the problem is that git-format-patch only decides whether to
> append a MIME header based on the commit message contents; it does not
> take the Signed-Off-By into account. This may also be the cause of the
> recent complaints from Matti Aarnio.

Yep, that's it. If the Signed-Off-By was added by commit -s, it works,
while format-patch -s causes the header to be missing, although the
Signed-Off-By is utf-8 encoded. Will try to remember that.

Thanks,
Björn

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It is our old friend vger adding the iso-8859-1 header, I think, since
> no encoding was specified.
>
> I think the problem is that git-format-patch only decides whether to
> append a MIME header based on the commit message contents; it does not
> take the Signed-Off-By into account. This may also be the cause of the
> recent complaints from Matti Aarnio.

I agree.  "Signed-off-by" as part of the existing commit message
should be fine, but with "format-patch -s" the code needs to be
careful.

The following is on top of 'master'.  I haven't tested it.  If it
works, great.  If it doesn't, at least it should illustrate what
needs to be touched.

Ideally a fix to 'maint' is needed --- the pretty-print
infrastructure on the 'master' side has strbuf changes and the
patch may have conflicts at the textual level, but it should be
straightforward to adjust to it by anybody so inclined (hint,
hint).

---

 builtin-branch.c      |    2 +-
 builtin-log.c         |    2 +-
 builtin-rev-list.c    |    3 ++-
 builtin-show-branch.c |    2 +-
 commit.c              |    5 ++---
 commit.h              |    4 +++-
 log-tree.c            |   15 ++++++++++++++-
 7 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 3da8b55..3e020cc 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -276,7 +276,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 		commit = lookup_commit(item->sha1);
 		if (commit && !parse_commit(commit)) {
 			pretty_print_commit(CMIT_FMT_ONELINE, commit,
-					    &subject, 0, NULL, NULL, 0);
+					    &subject, 0, NULL, NULL, 0, 0);
 			sub = subject.buf;
 		}
 		printf("%c %s%-*s%s %s %s\n", c, branch_get_color(color),
diff --git a/builtin-log.c b/builtin-log.c
index e8b982d..8b2bf63 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -787,7 +787,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 			struct strbuf buf;
 			strbuf_init(&buf, 0);
 			pretty_print_commit(CMIT_FMT_ONELINE, commit,
-			                    &buf, 0, NULL, NULL, 0);
+			                    &buf, 0, NULL, NULL, 0, 0);
 			printf("%c %s %s\n", sign,
 			       sha1_to_hex(commit->object.sha1), buf.buf);
 			strbuf_release(&buf);
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 4439332..6970467 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -86,7 +86,8 @@ static void show_commit(struct commit *commit)
 		struct strbuf buf;
 		strbuf_init(&buf, 0);
 		pretty_print_commit(revs.commit_format, commit,
-					&buf, revs.abbrev, NULL, NULL, revs.date_mode);
+				    &buf, revs.abbrev, NULL, NULL,
+				    revs.date_mode, 0);
 		if (buf.len)
 			printf("%s%c", buf.buf, hdr_termination);
 		strbuf_release(&buf);
diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 07a0c23..6dc835d 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -266,7 +266,7 @@ static void show_one_commit(struct commit *commit, int no_name)
 	strbuf_init(&pretty, 0);
 	if (commit->object.parsed) {
 		pretty_print_commit(CMIT_FMT_ONELINE, commit,
-					&pretty, 0, NULL, NULL, 0);
+				    &pretty, 0, NULL, NULL, 0, 0);
 		pretty_str = pretty.buf;
 	}
 	if (!prefixcmp(pretty_str, "[PATCH] "))
diff --git a/commit.c b/commit.c
index ac24266..8262f6a 100644
--- a/commit.c
+++ b/commit.c
@@ -479,7 +479,7 @@ static int get_one_line(const char *msg)
 }
 
 /* High bit set, or ISO-2022-INT */
-static int non_ascii(int ch)
+int non_ascii(int ch)
 {
 	ch = (ch & 0xff);
 	return ((ch & 0x80) || (ch == 0x1b));
@@ -1046,12 +1046,11 @@ static void pp_remainder(enum cmit_fmt fmt,
 void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 				  struct strbuf *sb, int abbrev,
 				  const char *subject, const char *after_subject,
-				  enum date_mode dmode)
+				  enum date_mode dmode, int plain_non_ascii)
 {
 	unsigned long beginning_of_body;
 	int indent = 4;
 	const char *msg = commit->buffer;
-	int plain_non_ascii = 0;
 	char *reencoded;
 	const char *encoding;
 
diff --git a/commit.h b/commit.h
index b779de8..678c62b 100644
--- a/commit.h
+++ b/commit.h
@@ -61,13 +61,15 @@ enum cmit_fmt {
 	CMIT_FMT_UNSPECIFIED,
 };
 
+extern int non_ascii(int);
 extern enum cmit_fmt get_commit_format(const char *arg);
 extern void format_commit_message(const struct commit *commit,
                                   const void *format, struct strbuf *sb);
 extern void pretty_print_commit(enum cmit_fmt fmt, const struct commit*,
                                 struct strbuf *,
                                 int abbrev, const char *subject,
-                                const char *after_subject, enum date_mode);
+                                const char *after_subject, enum date_mode,
+				int non_ascii_present);
 
 /** Removes the first commit from a list sorted by date, and adds all
  * of its parents.
diff --git a/log-tree.c b/log-tree.c
index 3763ce9..a34beb0 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -125,6 +125,18 @@ static unsigned int digits_in_number(unsigned int number)
 	return result;
 }
 
+static int has_non_ascii(const char *s)
+{
+	int ch;
+	if (!s)
+		return 0;
+	while ((ch = *s++) != '\0') {
+		if (non_ascii(ch))
+			return 1;
+	}
+	return 0;
+}
+
 void show_log(struct rev_info *opt, const char *sep)
 {
 	struct strbuf msgbuf;
@@ -273,7 +285,8 @@ void show_log(struct rev_info *opt, const char *sep)
 	 */
 	strbuf_init(&msgbuf, 0);
 	pretty_print_commit(opt->commit_format, commit, &msgbuf,
-				  abbrev, subject, extra_headers, opt->date_mode);
+			    abbrev, subject, extra_headers, opt->date_mode,
+			    has_non_ascii(opt->add_signoff));
 
 	if (opt->add_signoff)
 		append_signoff(&msgbuf, opt->add_signoff);

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Jeff King]

On Wed, Oct 31, 2007 at 02:53:47PM -0700, Junio C Hamano wrote:

> The following is on top of 'master'.  I haven't tested it.  If it
> works, great.  If it doesn't, at least it should illustrate what
> needs to be touched.

You beat me to it, as I was busy flaming Linus. :)

The patch I started is very similar to this, but I had one concern that
I was tracking down: is the author name encoding necessarily the same as
the commit text encoding?

-Peff

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... I had one concern that
> I was tracking down: is the author name encoding necessarily the same as
> the commit text encoding?

The user is screwing himself already if that is the case and
uses -s to format-patch, isn't he?

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Jeff King]

On Wed, Oct 31, 2007 at 03:31:20PM -0700, Junio C Hamano wrote:

> > ... I had one concern that
> > I was tracking down: is the author name encoding necessarily the same as
> > the commit text encoding?
> 
> The user is screwing himself already if that is the case and
> uses -s to format-patch, isn't he?

Hrm, they probably _should_ be the same in the output. It's not clear to
me what encoding we assume the name comes in (utf-8, I guess). Looks
like we don't touch it at all when putting it in the signoff. I think we
should just be able to reencode when appending the signoff; patch is
below.

I'm sure there are other weird interactions lurking. For example, do we
correctly detect an existing signoff if we are storing in a non-utf8
encoding? I must admit to being a little ignorant to some of the
encoding magic of git, having a us-ascii name myself.

---

diff --git a/log-tree.c b/log-tree.c
index 3763ce9..906942d 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -3,6 +3,7 @@
 #include "commit.h"
 #include "log-tree.h"
 #include "reflog-walk.h"
+#include "utf8.h"
 
 struct decoration name_decoration = { "object names" };
 
@@ -111,7 +112,14 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
 		strbuf_addch(sb, '\n');
 
 	strbuf_addstr(sb, signed_off_by);
-	strbuf_add(sb, signoff, signoff_len);
+	if (git_log_output_encoding) {
+		char *encoded_name = reencode_string(signoff,
+				git_log_output_encoding, "utf-8");
+		strbuf_addstr(sb, encoded_name);
+		free(encoded_name);
+	}
+	else
+		strbuf_add(sb, signoff, signoff_len);
 	strbuf_addch(sb, '\n');
 }
 

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Oct 31, 2007 at 03:31:20PM -0700, Junio C Hamano wrote:
>
>> > ... I had one concern that
>> > I was tracking down: is the author name encoding necessarily the same as
>> > the commit text encoding?
>> 
>> The user is screwing himself already if that is the case and
>> uses -s to format-patch, isn't he?
>
> Hrm, they probably _should_ be the same in the output. It's not clear to
> me what encoding we assume the name comes in (utf-8, I guess). Looks
> like we don't touch it at all when putting it in the signoff. I think we
> should just be able to reencode when appending the signoff; patch is
> below.

I think assuming utf-8 and reencoding is actively wrong.
Existing setups of people with names that cannot be expressed in
ASCII would already have the commit encoding specified in the
configuration and user.name stored in that encoding, so passing
things through as we have always done is the right thing to do.

Re: [PATCH 1/1] Add --first-parent support to interactive rebase.

[Jeff King]

On Wed, Oct 31, 2007 at 09:10:30PM -0700, Junio C Hamano wrote:

> I think assuming utf-8 and reencoding is actively wrong.
> Existing setups of people with names that cannot be expressed in
> ASCII would already have the commit encoding specified in the
> configuration and user.name stored in that encoding, so passing
> things through as we have always done is the right thing to do.

That will break any time somebody uses -s with a --encoding= that is
different from their usual encoding. My patch assumes the source is
utf-8, but should perhaps assume some other default encoding from the
config.

But if this is not a problem for people, I'm not going to push it. I
don't actually use any of these features; it was just something I
noticed while looking at the actual bug.

-Peff