Avoiding uninteresting merges in Cairo

Avoiding uninteresting merges in Cairo

[Shawn Pearce]

I was just talking with jwatt today in #git and he pointed me at
the following email from

 http://lists.freedesktop.org/archives/cairo/2006-April/006648.html

after he had a merge gone wrong in his working directory and was
asking for help.

Cario has seriously been using `reset --hard HEAD^` as part of its
workflow since April?  Why haven't you pushed for a rebase merge
strategy to be tried before a trivial index merge as an option in
git-merge (e.g. pull.twohead=rebase recursive)?

-- 

Re: Avoiding uninteresting merges in Cairo

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1975 bytes --]

On Thu, 14 Dec 2006 21:06:29 -0500, Shawn Pearce wrote:
>  http://lists.freedesktop.org/archives/cairo/2006-April/006648.html
>
> after he had a merge gone wrong in his working directory and was
> asking for help.
>
> Cario has seriously been using `reset --hard HEAD^` as part of its
> workflow since April?

I wrote that in April, but I've never pointed anyone at it since
then. I've learned a lot since then, (and git has changed too).
One thing I've been meaning to do is to write up a more complete
introduction on using git to manage cairo's tree to put into cairo's
wiki or so, (it would have helped a newcomer like jwatt here).

> Why haven't you pushed for a rebase merge
> strategy to be tried before a trivial index merge as an option in
> git-merge (e.g. pull.twohead=rebase recursive)?

I've mentioned it as an idea once or twice on this list. I've never
"pushed" for any change along these lines simply because I haven't
ever done any implementation for this.

Also, I don't actually need this. I don't use the "reset --hard"
workflow suggested in the mail above. I always obtain remote changes
with "git fetch" and then examine things locally and decide to either
merge (or fast forward) with "git pull", (though maybe I'll start
using "git merge" now), or else to use "git rebase" to avoid the noisy
merge commits.

That workflow of fetching changes and then exploring them locally
before merging is why I rarely take advantage of the combined
fetch+merge aspect of "git pull". It's exceedingly rare for me to ever
call "git pull" with any URL but ".", (though I use "git fetch" with
various URLs on a regular basis).

-Carl

PS. It's funny how much easier it is to type "cario" than "cairo". I
used to do that all the time, (though in this sentence here, when I
actually _tried_ to type "cario" it came out as "cairo" the first
time). I think the "cario" typo is a Qwerty artefact---the left hand
gets all its keypresses in before the right hand gets going.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Avoiding uninteresting merges in Cairo

[Shawn Pearce]

Carl Worth <cworth@cworth.org> wrote:
> One thing I've been meaning to do is to write up a more complete
> introduction on using git to manage cairo's tree to put into cairo's
> wiki or so, (it would have helped a newcomer like jwatt here).

I saw three things happen to poor jwatt that really threw him for
a loop, and I think they are all more likely related to Git than
to the Cairo project specifically:

 * His user.name/user.email is probably not what he wanted;

 * His Cygwin/Windows system made some *.c/*.c files 0755 without
   him realizing it;

 * gitweb made it appear as though a whole lot of Carl's recent
   work was somehow undone in the merge.

-- 

Re: Avoiding uninteresting merges in Cairo

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1944 bytes --]

On Thu, 14 Dec 2006 22:25:30 -0500, Shawn Pearce wrote:
> I saw three things happen to poor jwatt that really threw him for
> a loop, and I think they are all more likely related to Git than
> to the Cairo project specifically:

Yes. Also a fourth one was a filesystem case-insensitivity issue. We
recently imported an external library into cairo that had some
filenames with capital letters. We then renamed them to the
all-lowercase filenames we like. Poor jwatt was unlucky enough to have
cloned with a capitalized filename, and then was trying to pull the
latest with the lowercase filename and he got:

	fatal: Untracked working tree file 'test/pdiff/lpyramid.h'
	would be overwritten by merge

This was in some sense worse than the other problems since it stopped
him cold and gave him no idea what was wrong nor how to fix it.

>  * His user.name/user.email is probably not what he wanted;
>
>  * His Cygwin/Windows system made some *.c/*.c files 0755 without
>    him realizing it;

Those two issues did silently put garbage in the commits. I don't know
how to best fix the name/email thing. Maybe on the first commit with
no user.name and user.email configuration git could create them,
announce them to the user, and instruct them on how to change them:

	No name or email configuration exists. Using:

		U-JONATHAN-X60S\jonathan <jonathan@Jonathan-X60s.(none)>

	You can change these with the following commands:

		git repo-config user.name Your Name
		git repo-config user.email user@example.com

And maybe mention --global as well.

As for the filemode setting, shouldn't that be configured by default
the right way for Windows git?

>  * gitweb made it appear as though a whole lot of Carl's recent
>    work was somehow undone in the merge.

That looks like a simple gitweb bug. None of the other tools, (gitk,
git log -p), consider a trivial merge commit like this as having
anything interesting in it worth displaying.

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Avoiding uninteresting merges in Cairo

[Shawn Pearce]

Carl Worth <cworth@cworth.org> wrote:
> On Thu, 14 Dec 2006 22:25:30 -0500, Shawn Pearce wrote:
> > I saw three things happen to poor jwatt that really threw him for
> > a loop, and I think they are all more likely related to Git than
> > to the Cairo project specifically:
> 
> Yes. Also a fourth one was a filesystem case-insensitivity issue. We
> recently imported an external library into cairo that had some
> filenames with capital letters. We then renamed them to the
> all-lowercase filenames we like. Poor jwatt was unlucky enough to have
> cloned with a capitalized filename, and then was trying to pull the
> latest with the lowercase filename and he got:
> 
> 	fatal: Untracked working tree file 'test/pdiff/lpyramid.h'
> 	would be overwritten by merge
> 
> This was in some sense worse than the other problems since it stopped
> him cold and gave him no idea what was wrong nor how to fix it.

This is a huge problem on filesystems where case does not matter
(FAT, Mac OS X's HFS+, NTFS).  We really should put something into
git to detect this when it happens and help the user get out of
the box they have worked themselves into.
 
> >  * His user.name/user.email is probably not what he wanted;
> >
> >  * His Cygwin/Windows system made some *.c/*.c files 0755 without
> >    him realizing it;
> 
> Those two issues did silently put garbage in the commits. I don't know
> how to best fix the name/email thing. Maybe on the first commit with
> no user.name and user.email configuration git could create them,
> announce them to the user, and instruct them on how to change them:
> 
> 	No name or email configuration exists. Using:
> 
> 		U-JONATHAN-X60S\jonathan <jonathan@Jonathan-X60s.(none)>
> 
> 	You can change these with the following commands:
> 
> 		git repo-config user.name Your Name
> 		git repo-config user.email user@example.com
> 
> And maybe mention --global as well.

I've been thinking that is the right thing to do, somewhere
in git-commit, but I can't decide if its best to save that
per-repository or per user, or be interactive and get confirmation
from the user, or just display it and hope the user noticed.
 
> As for the filemode setting, shouldn't that be configured by default
> the right way for Windows git?

No.  The problem is Git tries to be smart and see if filemode works
by chmodding the config file and then looking to see if that worked
the way we expected it to.  It usually does on Cygwin when using
NTFS.  But then if you edit a file non-executable file with a native
Windows application Cygwin may suddenly think its executable again,
and now our core.filemode=true guess isn't actually right anymore.

We probably should just hardcode core.filemode to false on Cygwin.
If the user actually wants core.filemode to be true on Cygwin then
he/she will probably also know enough about this issue with native
Windows applications that the additional hurdle of turning on the
option by hand won't be a big one for them.

> >  * gitweb made it appear as though a whole lot of Carl's recent
> >    work was somehow undone in the merge.
> 
> That looks like a simple gitweb bug. None of the other tools, (gitk,
> git log -p), consider a trivial merge commit like this as having
> anything interesting in it worth displaying.

Right.  gitweb is doing `diff HEAD^1 HEAD` which is quite wrong on
a merge commit.  Especially here where the trunk was pulled into
the topic branch, rather than the topic branch being pulled into
the trunk.

-- 

Re: Avoiding uninteresting merges in Cairo

[Jakub Narebski]

Carl Worth wrote:

> On Thu, 14 Dec 2006 22:25:30 -0500, Shawn Pearce wrote:

>>  * gitweb made it appear as though a whole lot of Carl's recent
>>    work was somehow undone in the merge.
> 
> That looks like a simple gitweb bug. None of the other tools, (gitk,
> git log -p), consider a trivial merge commit like this as having
> anything interesting in it worth displaying.

It's not a bug, it is rather lack of feature (or misfeature).

Gitweb deals with multiparent commits (merge commits) that it uses
_first_ parent. No special casing merges (like git-show doesn't
show whatchanged/patch for merges), no support for -c/--cc diff
(combined diff format).
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: Avoiding uninteresting merges in Cairo

[Shawn Pearce]

Jakub Narebski <jnareb@gmail.com> wrote:
> Carl Worth wrote:
> 
> > On Thu, 14 Dec 2006 22:25:30 -0500, Shawn Pearce wrote:
> 
> >>  * gitweb made it appear as though a whole lot of Carl's recent
> >>    work was somehow undone in the merge.
> > 
> > That looks like a simple gitweb bug. None of the other tools, (gitk,
> > git log -p), consider a trivial merge commit like this as having
> > anything interesting in it worth displaying.
> 
> It's not a bug, it is rather lack of feature (or misfeature).

Its a bug.

I'm not a gitweb user (meaning I almost never look at something
in gitweb).  But I'm clearly also not a Git newbie.  ;-)

I could not fathom why that merge commit was being displayed that
way in gitweb.  I had to clone the cairo project just so I could
actually look at the commit with log/show/whatchanged/diff-tree,
because I couldn't believe what I was seeing from gitweb.
 
-- 

Re: Avoiding uninteresting merges in Cairo

[Jakub Narebski]

Shawn Pearce wrote:
> Jakub Narebski <jnareb@gmail.com> wrote:
>> Carl Worth wrote:
>> 
>>> On Thu, 14 Dec 2006 22:25:30 -0500, Shawn Pearce wrote:
>> 
>>>>  * gitweb made it appear as though a whole lot of Carl's recent
>>>>    work was somehow undone in the merge.
>>> 
>>> That looks like a simple gitweb bug. None of the other tools, (gitk,
>>> git log -p), consider a trivial merge commit like this as having
>>> anything interesting in it worth displaying.
>> 
>> It's not a bug, it is rather lack of feature (or misfeature).
> 
> Its a bug.
> 
> I'm not a gitweb user (meaning I almost never look at something
> in gitweb).  But I'm clearly also not a Git newbie.  ;-)
> 
> I could not fathom why that merge commit was being displayed that
> way in gitweb.  I had to clone the cairo project just so I could
> actually look at the commit with log/show/whatchanged/diff-tree,
> because I couldn't believe what I was seeing from gitweb.

Do you mean "commit" view or "commitdiff" view in gitweb for merges
is confusing?

If it is "commit" view, it is fairly easy to remove difftree/whatchanged
output below commit message for merges. However while git-show nor 
git-diff-tree doesn't show diff for merge messages, the diftree output
in "commit" view might be taken as 'damages'; git diff --summary always
takes summary of diff against first parent.

If it is "commitdiff" view... well, I plan on adding combined diff 
output to commitdiff, but I need raw (whatchanged) output with the
same files which would be shown in git-diff --cc for merges (compact
combined diff output). Otherwise I'd have to use combined (-c) output
in gitweb, rather than more terse --cc output.

-- 
Jakub Narebski

Re: Avoiding uninteresting merges in Cairo

[Shawn Pearce]

Jakub Narebski <jnareb@gmail.com> wrote:
> Do you mean "commit" view or "commitdiff" view in gitweb for merges
> is confusing?

The commit view was what was confusing me.
 
> If it is "commit" view, it is fairly easy to remove difftree/whatchanged
> output below commit message for merges. However while git-show nor 
> git-diff-tree doesn't show diff for merge messages, the diftree output
> in "commit" view might be taken as 'damages'; git diff --summary always
> takes summary of diff against first parent.

I can see that, but it depends on what the current branch was when
you do the merge.  In the cairo case I was looking at yesterday the
"damages" from the merge were actually what was already considered to
be in the mainline as part of the project due to committer pulling
the remote master into his own master before pushing.

Given that the "push; whoops; pull; push" pattern is so common to
Git I think it is sort of misleading to show a merge like this;
lots of activity may have been pulled in from the central branch
during that pull.

iirc git whatchanged for a merge shows only files which cannot be
found in any of the parents; these the files which the merge commit
itself contributed directly.  If that set is empty then the commit
just isn't shown by git whatchanged.

-- 

[PATCH] gitweb: Do not show difftree for merges in "commit" view

[Jakub Narebski]

Do not show difftree against first parent for merges (commits with
more than one parent) in "commit" view, because it usually is
misleading.  git-show and git-whatchanged doesn't show diff for merges
either.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 040ee71..ebf35a1 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3572,14 +3572,19 @@ sub git_commit {
 	my %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
 
 	my $parent = $co{'parent'};
+	my $parents = $co{'parents'};
 	if (!defined $parent) {
 		$parent = "--root";
 	}
-	open my $fd, "-|", git_cmd(), "diff-tree", '-r', "--no-commit-id",
-		@diff_opts, $parent, $hash, "--"
-		or die_error(undef, "Open git-diff-tree failed");
-	my @difftree = map { chomp; $_ } <$fd>;
-	close $fd or die_error(undef, "Reading git-diff-tree failed");
+	my @difftree;
+	if (@$parents <= 1) {
+		# difftree output is not printed for merges
+		open my $fd, "-|", git_cmd(), "diff-tree", '-r', "--no-commit-id",
+			@diff_opts, $parent, $hash, "--"
+				or die_error(undef, "Open git-diff-tree failed");
+		@difftree = map { chomp; $_ } <$fd>;
+		close $fd or die_error(undef, "Reading git-diff-tree failed");
+	}
 
 	# non-textual hash id's can be cached
 	my $expires;
@@ -3641,7 +3646,7 @@ sub git_commit {
 	}
 	print "</td>" .
 	      "</tr>\n";
-	my $parents = $co{'parents'};
+
 	foreach my $par (@$parents) {
 		print "<tr>" .
 		      "<td>parent</td>" .
@@ -3663,7 +3668,10 @@ sub git_commit {
 	git_print_log($co{'comment'});
 	print "</div>\n";
 
-	git_difftree_body(\@difftree, $hash, $parent);
+	if (@$parents <= 1) {
+		# do not output difftree/whatchanged for merges
+		git_difftree_body(\@difftree, $hash, $parent);
+	}
 
 	git_footer_html();
 }
-- 
1.4.4.1

Re: Avoiding uninteresting merges in Cairo

[Junio C Hamano]

Carl Worth <cworth@cworth.org> writes:

> On Thu, 14 Dec 2006 21:06:29 -0500, Shawn Pearce wrote:
>> ...
>> Cairo has seriously been using `reset --hard HEAD^` as part of its
>> workflow since April?
> ...
> Also, I don't actually need this. I don't use the "reset --hard"
> workflow suggested in the mail above. I always obtain remote changes
> with "git fetch" and then examine things locally and decide to either
> merge (or fast forward) with "git pull", (though maybe I'll start
> using "git merge" now), or else to use "git rebase" to avoid the noisy
> merge commits.

I wish I have more time to keep track of "recommended workflows"
that projects using git give to its developers.

I think what you recommend makes quite a lot of sense, for
very centralized setting, and would leave much more usable
history than the simple-minded way cvs-migration suggests.