frustrated forensics: hard to find diff that undid a fix

frustrated forensics: hard to find diff that undid a fix

[Adam Monsen]

I made a fix a month ago on the master branch in a shared repo. A week
later, a colleague did a merge that undid the fix. I didn't figure out
the problem until just now because I'd been assuming the fix was still
on master. I mean, if it wasn't, I should see a reverse patch using "git
log -p master", right? Wrong. Turns out the fix was undone as part of
merge conflict resolution (I think).

Is there some way to include merge conflict resolutions in "git log -p"
or "git show"? Apparently some important information can be hidden in
the conflict resolution. Or, more likely, I just don't understand how
this bit of git works.

I also tried bisect and pickaxe. Bisect wrongly identified the first bad
commit, and pickaxe just didn't see the change at all.

    ~ * ~

Here's some details in case anyone wants to (a) point out where I messed
up or (b) help me avoid this kind of blunder in the future.

1. The repo is git://mifos.git.sourceforge.net/gitroot/mifos/head
(mirror: https://github.com/mifos/head ). Branch master.

2. My commit 2a1ed0436 introduced a fix that includes the text
"native2ascii". Shows up in "git log -p -1 2a1ed0436" and "git show
2a1ed0436". Life is good.

3. It appears that the merge commit 0f8132386 tossed my "native2ascii"
fix. The only way I could figure out to actually visualize this is "git
diff 58320586..0f813238".

This took a while to figure out. Am I missing something obvious?

Re: frustrated forensics: hard to find diff that undid a fix

[Jonathan del Strother]

On 5 March 2011 06:20, Adam Monsen <haircut@gmail.com> wrote:
> I made a fix a month ago on the master branch in a shared repo. A week
> later, a colleague did a merge that undid the fix. I didn't figure out
> the problem until just now because I'd been assuming the fix was still
> on master. I mean, if it wasn't, I should see a reverse patch using "git
> log -p master", right? Wrong. Turns out the fix was undone as part of
> merge conflict resolution (I think).
>
> Is there some way to include merge conflict resolutions in "git log -p"
> or "git show"? Apparently some important information can be hidden in
> the conflict resolution. Or, more likely, I just don't understand how
> this bit of git works.
>
> I also tried bisect and pickaxe. Bisect wrongly identified the first bad
> commit, and pickaxe just didn't see the change at all.
>
>    ~ * ~
>
> Here's some details in case anyone wants to (a) point out where I messed
> up or (b) help me avoid this kind of blunder in the future.
>
> 1. The repo is git://mifos.git.sourceforge.net/gitroot/mifos/head
> (mirror: https://github.com/mifos/head ). Branch master.
>
> 2. My commit 2a1ed0436 introduced a fix that includes the text
> "native2ascii". Shows up in "git log -p -1 2a1ed0436" and "git show
> 2a1ed0436". Life is good.
>
> 3. It appears that the merge commit 0f8132386 tossed my "native2ascii"
> fix. The only way I could figure out to actually visualize this is "git
> diff 58320586..0f813238".
>
> This took a while to figure out. Am I missing something obvious?


Seems like a bunch of people (myself included) have been tripped up by
this behaviour in the past few months.   Did something change to start
this, or has it always been there?

-Jonathan

Re: frustrated forensics: hard to find diff that undid a fix

[Jakub Narebski]

Adam Monsen <haircut@gmail.com> writes:

> I made a fix a month ago on the master branch in a shared repo. A week
> later, a colleague did a merge that undid the fix. I didn't figure out
> the problem until just now because I'd been assuming the fix was still
> on master. I mean, if it wasn't, I should see a reverse patch using "git
> log -p master", right? Wrong. Turns out the fix was undone as part of
> merge conflict resolution (I think).
> 
> Is there some way to include merge conflict resolutions in "git log -p"
> or "git show"? Apparently some important information can be hidden in
> the conflict resolution. Or, more likely, I just don't understand how
> this bit of git works.

By default "git log -p" and "git show" considers merges uninteresting.
Try "git log -p -c" or "git log -p -m".
 
> I also tried bisect and pickaxe. Bisect wrongly identified the first bad
> commit, and pickaxe just didn't see the change at all.

I guess that pickaxe also needs -c or -m.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: frustrated forensics: hard to find diff that undid a fix

[Jonathan Nieder]

Hi,

Jakub Narebski wrote:

> I guess that pickaxe also needs -c or -m.

I am not so sure.

Pickaxe is used to ask, "what commit introduced this string?".  Using
"git log --raw -c", I can see that the current state of
contrib/fast-import/git-p4 came about in commit 6d74e5c9d (Merge
branch 'mh/p4', 2011-03-04):

| $ git log --oneline --raw -c
| 07873dc Merge branch 'maint'
| 
| 6d74e5c Merge branch 'mh/p4'
| 
| ::100755 100755 100755 a4f440d... 8b00fd8... 2df3bb2... MM
| contrib/fast-import/git-p4
[...]

Now, working backwards, I ask git:

| $ git log --oneline -S "$(cat contrib/fast-import/git-p4)" maint..master
| $

No hits.  Maybe it's from one of those mergey diffs?

| $ git log --oneline -m -S "$(cat contrib/fast-import/git-p4)" maint..master
| 07873dc (from 964498e) Merge branch 'maint'
| 6d74e5c (from 08fd871) Merge branch 'mh/p4'
| 6d74e5c (from c9dbab0) Merge branch 'mh/p4'
| $

Too many hits (it includes every merge in which one side contains
the string and the other does not).  How about -c, which seemed to
produce such nice output with --raw?

| $ git log --oneline -c -S "$(cat contrib/fast-import/git-p4)" maint..master
| 07873dc Merge branch 'maint'
| 6d74e5c Merge branch 'mh/p4'
| 08fd871 Merge branch 'mg/maint-difftool-vim-readonly'
| 5cb3c9b Merge branch 'jn/maint-commit-missing-template'
| 1538f21 Merge branch 'jk/diffstat-binary'
| 24161eb Merge branch 'lt/rename-no-extra-copy-detection'
| [...]

Oh.  diff_tree_combined_merge simply doesn't know about pickaxe,
so -c and --cc with -S print _all_ merges.

So the only sensible way to use pickaxe with merges is

| $ git log --oneline -m --first-parent \
|	-S "$(cat contrib/fast-import/git-p4)" maint..master
| 6d74e5c Merge branch 'mh/p4'

for now.  I'd be happy to help anyone hoping to improve this.
(Hopefully all that is needed is something like the
diff_queue_is_empty() check from v0.99~504 --- Diffcore updates,
2005-05-22.)

Re: frustrated forensics: hard to find diff that undid a fix

[Jeff King]

On Sat, Mar 05, 2011 at 06:51:00AM -0600, Jonathan Nieder wrote:

> | $ git log --oneline -m -S "$(cat contrib/fast-import/git-p4)" maint..master
> | 07873dc (from 964498e) Merge branch 'maint'
> | 6d74e5c (from 08fd871) Merge branch 'mh/p4'
> | 6d74e5c (from c9dbab0) Merge branch 'mh/p4'
> | $
> 
> Too many hits (it includes every merge in which one side contains
> the string and the other does not).  How about -c, which seemed to
> produce such nice output with --raw?

Yeah, I think the problem is that the diffcore chain doesn't know enough
about the merge. It just sees the filepairs between the commit and each
parent separately. I looked into this recently as part of this thread:

  http://article.gmane.org/gmane.comp.version-control.git/165743

I described some reasonable semantics there, but implementing them
looked non-trivial. I'd be very happy to be proven wrong. Patches
welcome. :)

-Peff

Re: frustrated forensics: hard to find diff that undid a fix

[Adam Monsen]

Jakub Narebski wrote:
> By default "git log -p" and "git show" considers merges uninteresting.
> Try "git log -p -c" or "git log -p -m".

Holy cow, that's a lifesaver! Thank you, Jakub!!

I totally missed the whole "Diff Formatting" section in the git-log(1)
manpage.

"git log -p -c" does exactly what I want. I'll make an alias or
something for these options.

I'm confused by this section of git-log(1):

  COMBINED DIFF FORMAT
    "git-diff-tree", "git-diff-files" and "git-diff" can take -c
    or --cc option to produce combined diff. For showing a merge
    commit with "git log -p", this is the default format; you can
    force showing full diff with the -m option.

Sounds like this is saying that -c is the default with "git log -p".
I'll submit a patch to fix that.

[PATCH 0/2] improve combined diff documentation

[Adam Monsen]

The "combined diff format" documentation in diff-generate-patch.txt
incorrectly implied that "log -p" shows conflict resolutions as
combined diffs.

With these small documentation fixes I'm hoping to save someone the
time it took me to figure out how a merge conflict was resolved.

Related thread:

http://thread.gmane.org/gmane.comp.version-control.git/168481

The patches apply cleanly to maint.

Hope this helps,
-Adam ("meonkeys" on IRC)

Adam Monsen (2):
  documentation fix: git log -p does not imply -c.
  English grammar fixes for combined diff doc.

 Documentation/diff-generate-patch.txt |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

-- 
1.7.2.3

[PATCH 1/2] documentation fix: git log -p does not imply -c.

[Adam Monsen]

Relates to the thread with subject "frustrated forensics: hard to find
diff that undid a fix" on the git mailing list.

	http://thread.gmane.org/gmane.comp.version-control.git/168481

I don't wish for anyone to repeat my bungled forensics episode.
Hopefully this will help others git along happily.

Signed-off-by: Adam Monsen <haircut@gmail.com>
---
 Documentation/diff-generate-patch.txt |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/Documentation/diff-generate-patch.txt b/Documentation/diff-generate-patch.txt
index 3ac2bea..6cd5270 100644
--- a/Documentation/diff-generate-patch.txt
+++ b/Documentation/diff-generate-patch.txt
@@ -75,10 +75,9 @@ combined diff format
 --------------------
 
 "git-diff-tree", "git-diff-files" and "git-diff" can take '-c' or
-'--cc' option to produce 'combined diff'.  For showing a merge commit
-with "git log -p", this is the default format; you can force showing
-full diff with the '-m' option.
-A 'combined diff' format looks like this:
+'--cc' option to produce 'combined diff'.  You can force showing
+full diff with the '-m' option.  A 'combined diff' format looks like
+this:
 
 ------------
 diff --combined describe.c
-- 
1.7.2.3

Re: [PATCH 1/2] documentation fix: git log -p does not imply -c.

[Junio C Hamano]

Adam Monsen <haircut@gmail.com> writes:

> Relates to the thread with subject "frustrated forensics: hard to find
> diff that undid a fix" on the git mailing list.
>
> 	http://thread.gmane.org/gmane.comp.version-control.git/168481
>
> I don't wish for anyone to repeat my bungled forensics episode.

But this patch is wrong, isn't it?

The --cc format _is_ the default, not -c format.

Re: [PATCH 1/2] documentation fix: git log -p does not imply -c.

[Jeff King]

On Sun, Mar 06, 2011 at 04:36:28PM -0800, Junio C Hamano wrote:

> Adam Monsen <haircut@gmail.com> writes:
> 
> > Relates to the thread with subject "frustrated forensics: hard to find
> > diff that undid a fix" on the git mailing list.
> >
> > 	http://thread.gmane.org/gmane.comp.version-control.git/168481
> >
> > I don't wish for anyone to repeat my bungled forensics episode.
> 
> But this patch is wrong, isn't it?
> 
> The --cc format _is_ the default, not -c format.

Hmm. "git show" seems to show --cc, but "git log -p" does not show
anything. Try:

  $ git show 1538f21
  $ git log -p -1 1538f21

So the current doc seems to be wrong. Should we be fixing the code and
not the doc?

-Peff

Re: [PATCH 1/2] documentation fix: git log -p does not imply -c.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Mar 06, 2011 at 04:36:28PM -0800, Junio C Hamano wrote:
>
>> The --cc format _is_ the default, not -c format.
>
> Hmm. "git show" seems to show --cc, but "git log -p" does not show
> anything.

The intention has always been to default to --cc since 0fe7c1d (built-in
diff: assorted updates., 2006-04-29) for "diff" if I am not misremembering
things, but you are right---"log" is a tad different.

The code does not want to use --cc by default for "log", and I don't think
that should change.  See 1aec791 (git log: don't do merge diffs by
default, 2006-04-19).

Re: [PATCH 1/2] documentation fix: git log -p does not imply -c.

[Jeff King]

On Mon, Mar 07, 2011 at 10:37:21AM -0800, Junio C Hamano wrote:

> > Hmm. "git show" seems to show --cc, but "git log -p" does not show
> > anything.
> 
> The intention has always been to default to --cc since 0fe7c1d (built-in
> diff: assorted updates., 2006-04-29) for "diff" if I am not misremembering
> things, but you are right---"log" is a tad different.
> 
> The code does not want to use --cc by default for "log", and I don't think
> that should change.  See 1aec791 (git log: don't do merge diffs by
> default, 2006-04-19).

Thanks for the history. I think the doc problem was an inaccuracy that
snuck in during 272bd3c (Include diff options in the git-log manpage,
2007-11-01). Nearly identical text (without the inaccuracy) is in the
"Diff Format For Merges" section in diff-format.txt.

Furthermore, the copied text talks about diff-index and diff-tree, but
gets included inline in git-log(1) (although the part in diff-format.txt
does not get included in git-log's manpage)[1]. So probably it's
reasonable to clean it up to something like:

diff --git a/Documentation/diff-generate-patch.txt b/Documentation/diff-generate-patch.txt
index 3ac2bea..3d02da9 100644
--- a/Documentation/diff-generate-patch.txt
+++ b/Documentation/diff-generate-patch.txt
@@ -74,10 +74,12 @@ separate lines indicate the old and the new mode.
 combined diff format
 --------------------
 
-"git-diff-tree", "git-diff-files" and "git-diff" can take '-c' or
-'--cc' option to produce 'combined diff'.  For showing a merge commit
-with "git log -p", this is the default format; you can force showing
-full diff with the '-m' option.
+Any diff-generating command can take the `-c` or `--cc` option to
+produced a 'combined diff' when showing a merge. This is the default
+format when showing merge conflicts with linkgit:git-diff[1] or a merge
+commit with linkgit:git-show[1]. Note also that you can vie the full
+diff with the `-m` option.
+
 A 'combined diff' format looks like this:
 
 ------------

-- >8 --

Is there any way to get "git diff" to show combined-form besides an
index with conflicts? I couldn't convince it to show me a merge commit
beside its parents, since it doesn't have an equivalent to diff-tree's
--stdin option.

-Peff

[1] Reading over this, the whole section could use some editing. I think
this is another example that needs to be broken out into its own
user-visible manpage. That is, we have too much "if you use the -p
option to command X, or command Y by default, or command Z without
--raw, then you see this format". That's pretty dense. Instead command X
should have:

  -p::
  --stat::
  --summary::
  [etc]
    Generate diffs in this format. See "git help diff-formats" for
    details. The default format is "-p".

and then diff-format.txt should _just_ be a description of the diff
formats, without worrying about commands at all. And probably the text
above should be factored out as part of diff-options.txt. But that's all
part of a much bigger documentation architecture change that I am hoping
to get to eventually. For now, I think it's worth just tweaking this
text to stop being inaccurate.

[PATCH v3] Documentation fix: git log -p does not imply -c.

[Adam Monsen]

Relates to the thread with subject "frustrated forensics: hard to find
diff that undid a fix" on the git mailing list.

    http://thread.gmane.org/gmane.comp.version-control.git/168481

I don't wish for anyone to repeat my bungled forensics episode.
Hopefully this will help others git along happily.

Signed-off-by: Adam Monsen <haircut@gmail.com>
---
This is really Peff's patch, I
* fixed a typo (vie -> view)
* am sending it as an acutal patch in case that's easier to apply than
  a diff in an email

I wasn't sure what "Author:" should be, go ahead and change it to the
most appropriate person.

Hope this helps,
-Adam

 Documentation/diff-generate-patch.txt |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/diff-generate-patch.txt b/Documentation/diff-generate-patch.txt
index 3ac2bea..5d478c1 100644
--- a/Documentation/diff-generate-patch.txt
+++ b/Documentation/diff-generate-patch.txt
@@ -74,10 +74,12 @@ separate lines indicate the old and the new mode.
 combined diff format
 --------------------
 
-"git-diff-tree", "git-diff-files" and "git-diff" can take '-c' or
-'--cc' option to produce 'combined diff'.  For showing a merge commit
-with "git log -p", this is the default format; you can force showing
-full diff with the '-m' option.
+Any diff-generating command can take the `-c` or `--cc` option to
+produced a 'combined diff' when showing a merge. This is the default
+format when showing merge conflicts with linkgit:git-diff[1] or a merge
+commit with linkgit:git-show[1]. Note also that you can view the full
+diff with the `-m` option.
+
 A 'combined diff' format looks like this:
 
 ------------
-- 
1.7.2.3

Re: [PATCH v3] Documentation fix: git log -p does not imply -c.

[Junio C Hamano]

Adam Monsen <haircut@gmail.com> writes:

> This is really Peff's patch, I
> * fixed a typo (vie -> view)
> * am sending it as an acutal patch in case that's easier to apply than
>   a diff in an email

Thanks.  Such a patch to summarize the discussion so far is greatly
appreciated.

>  Documentation/diff-generate-patch.txt |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/diff-generate-patch.txt b/Documentation/diff-generate-patch.txt
> index 3ac2bea..5d478c1 100644
> --- a/Documentation/diff-generate-patch.txt
> +++ b/Documentation/diff-generate-patch.txt
> @@ -74,10 +74,12 @@ separate lines indicate the old and the new mode.
>  combined diff format
>  --------------------
>  
> -"git-diff-tree", "git-diff-files" and "git-diff" can take '-c' or
> -'--cc' option to produce 'combined diff'.  For showing a merge commit
> -with "git log -p", this is the default format; you can force showing
> -full diff with the '-m' option.
> +Any diff-generating command can take the `-c` or `--cc` option to
> +produced a 'combined diff' when showing a merge. This is the default

s/produced/produce/, I think.

> +format when showing merge conflicts with linkgit:git-diff[1] or a merge
> +commit with linkgit:git-show[1]. Note also that you can view the full
> +diff with the `-m` option.

This "Note" is a bit unclear what command it applies to, isn't it?  I know
it applies to all the commands mentioned in the previous sentence in the
paragraph, but we are not writing the documentation for me, so perhaps 

	Note also that you can give the `-m' option to any of these
	commands to force generation of diffs with individual parents of a
	merge.

Also -c and --cc are technically _not_ about "showing merge conflicts".
It is about "showing a merge commit".  I don't know if we want to teach
the distinction in this part of the document, though.

If you resolve a conflicted merge taking the results from only one side
for a given hunk, --cc won't show anything.  If on the other hand, you
futz with a clean merge so that your result does not match with any
parent, --cc will show it.

Cf.

 http://thread.gmane.org/gmane.comp.version-control.git/89415

[PATCH v4] Documentation fix: git log -p does not imply -c.

[Adam Monsen]

Relates to the thread with subject "frustrated forensics: hard to find
diff that undid a fix" on the git mailing list.

    http://thread.gmane.org/gmane.comp.version-control.git/168481

I don't wish for anyone to repeat my bungled forensics episode.
Hopefully this will help others git along happily.

See also:

    http://thread.gmane.org/gmane.comp.version-control.git/89415

Signed-off-by: Adam Monsen <haircut@gmail.com>
---

Junio wrote:
> s/produced/produce/, I think.

Ah, indeed. Fixed, thanks.

I also sidestepped the difference between merge commits and conflicts by
changing the wording slightly.

Your changes to the "Note" seemed helpful, I put them in. Thanks!

 Documentation/diff-generate-patch.txt |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/diff-generate-patch.txt b/Documentation/diff-generate-patch.txt
index 3ac2bea..c57460c 100644
--- a/Documentation/diff-generate-patch.txt
+++ b/Documentation/diff-generate-patch.txt
@@ -74,10 +74,13 @@ separate lines indicate the old and the new mode.
 combined diff format
 --------------------
 
-"git-diff-tree", "git-diff-files" and "git-diff" can take '-c' or
-'--cc' option to produce 'combined diff'.  For showing a merge commit
-with "git log -p", this is the default format; you can force showing
-full diff with the '-m' option.
+Any diff-generating command can take the `-c` or `--cc` option to
+produce a 'combined diff' when showing a merge. This is the default
+format when showing merges with linkgit:git-diff[1] or
+linkgit:git-show[1]. Note also that you can give the `-m' option to any
+of these commands to force generation of diffs with individual parents
+of a merge.
+
 A 'combined diff' format looks like this:
 
 ------------
-- 
1.7.2.3

Re: [PATCH v4] Documentation fix: git log -p does not imply -c.

[Junio C Hamano]

Adam Monsen <haircut@gmail.com> writes:

> Relates to the thread with subject "frustrated forensics: hard to find
> diff that undid a fix" on the git mailing list.
>
>     http://thread.gmane.org/gmane.comp.version-control.git/168481
>
> I don't wish for anyone to repeat my bungled forensics episode.
> Hopefully this will help others git along happily.
>
> See also:
>
>     http://thread.gmane.org/gmane.comp.version-control.git/89415
>
> Signed-off-by: Adam Monsen <haircut@gmail.com>

Please don't do this.

Re-read what you wrote above while pretending that you do not have any
knowledge of the "frustrated forensics" you did.  Does it convey _any_
useful information?  Log messages should be sufficiently understandable
offline without having the web access.

Instead, summarize why the change is necessary.  IOW, don't be lazy now
while writing the log, to save time for people who later need to read log.

Something like

    Subject: diff format documentation: clarify --cc and -c

    The description was unclear if -c or --cc was the default (--cc is for
    some commands), and incorrectly implied that the default applies to
    all the diff generating commands.

    Most importantly, "log" does not default to "--cc" (it defaults to
    "--no-merges") and "log -p" obeys the user's wish to see non-combined
    format.  Only "diff" (during merge and three-blob comparison) and
    "show" use --cc as the default.

should be sufficient.

Re: [PATCH v4] Documentation fix: git log -p does not imply -c.

[Adam Monsen]

Junio C Hamano wrote:
> Log messages should be sufficiently understandable offline without 
> having the web access.

Ok! Makes sense.

I read some stuff before writing it (like
Documentation/SubmittingPatches), but what I should have done is just
thumb through the log. Many commit messages are as you say they should be.

> Something like ...<snipped>... should be sufficient.

Thanks, I'll use that. It includes history and code details I didn't know.

This is good advice about how to fit in to the git community... would
you like a "commit message guide"? I did something like this for another
community (Mifos), and they found it helpful. Here's a rough draft:

-----------8<-----------

Commit message guide
====================

The suggested *format* of a commit message is covered in DISCUSSION in
git-commit(1). This guide covers philosophy of commit messages.

- Read previous commit messages. Emulate the best ones.
- Reveal your intentions.
- Answer questions you anticipate others will ask.
- Imagine you are reading this same commit message 10 years from now.
  What would be most helpful for you to quickly recall why these
  changes were made?
- Imagine someone else is reading this same commit message 10 years
  from now. What would be most helpful for them to quickly understand
  what this commit changes and why it was done?
- Commit messages should be sufficiently understandable without access
  to any online content.
- Be verbose!
- This is your chance to use time- and context-sensitive information
  relevant to code changed.
- Refer to related content.
  - other commits
  - mailing list discussions (but not in lieu of a proper description)

----------->8-----------

If you want a guide like this, some questions:
* do you want asciidoc, something else, or don't care?
* name it Documentation/CommitMessageGuide ? or something else?

Re: [PATCH v4] Documentation fix: git log -p does not imply -c.

[Junio C Hamano]

Adam Monsen <haircut@gmail.com> writes:

> I read some stuff before writing it (like
> Documentation/SubmittingPatches), but what I should have done is just
> thumb through the log. Many commit messages are as you say they should be.

See below...

> The suggested *format* of a commit message is covered in DISCUSSION in
> git-commit(1). This guide covers philosophy of commit messages.
>
> - Read previous commit messages. Emulate the best ones.

I thought you were trying to teach new people how to gauge the "goodness"
with this list.  How would they know which ones are "best"? ;-)

> - Answer questions you anticipate others will ask.

Not quite sure.  I'd rather see people ask questions to themselves while
working on the change, so that the end product does not have even a room
for questioning.

> - Reveal your intentions.
> - Imagine you are reading this same commit message 10 years from now.
>   What would be most helpful for you to quickly recall why these
>   changes were made?
> - Imagine someone else is reading this same commit message 10 years
>   from now. What would be most helpful for them to quickly understand
>   what this commit changes and why it was done?

These three points are important and are the same thing.  I often
encourage people to write their logs while keeping these points in mind:

 (1) Remember that only the first paragraph is shown in MUA (as Subject:)
     and output from tools like "git shortlog", "git log --oneline", "gitk"
     and "gitweb".  Say what the change is about concisely and clearly
     there.

     A good "Subject:" greatly helps me writing Release Notes; if shortlog
     output does not remind me what the change is about, I would need to
     go back to "git show", which is very time consuming.

 (2) Explain the problem you are trying to solve first.  Don't stop at
     saying "'git xyzzy' ran with the --frotz option does not work".

     Clearly define why you think the current behaviour is broken and what
     you think is expected.  I.e. say "'git xyzzy' ran with the --frotz
     option does not show nitfol from its output; it should" instead of
     just "does not work".

     You may discuss earlier design decisions that you do not agree with
     (e.g. "The commit that introduced the option explains why nitfol does
     not matter under --frotz mode, but it is useful to have in this use
     case...") here.

     The second paragraph is primarily to make sure that future people
     reading the log know what effect the change _wanted_ to make, in case
     the code gets broken later and started doing different things, or the
     change is buggy from day one and didn't work as advertised in the
     commit log message.

     A good problem description also helps the reviewers to spot X-Y
     problem at the design level.  A patch that addresses a non-existent
     problem is not worth reading nor commenting on---the time is better
     spent on giving an alternative solution to the real problem the patch
     author wanted to address.

 (3) Then describe your solution.  You may want to give observations on
     the current code (e.g. "This is because the code in the frotz()
     function that calls xyzzy_output() forgets to set the nitfol flag")
     if this is about an implementation bug whose solution is subtle.

     Optionally discuss alternative approaches you considered, if any, and
     state the reason you ended up solving the problem differently.

     This section is to give hints to later people about other approaches
     already attempted and failed (or not tried due to lack of time, but
     may be worth pursuing).

 (4) If it is a performance fix/enhancement, benchmarking result would
     come here.

I try to give this list to new contributors early in their initiation
process (ideally before their patches hit the codebase).  That is probably
why many of the existing commits you saw in "git log" more or less
conformed to the recommendation.

> - Be verbose!

Please don't.  We want sufficiently detailed description, but we don't
want verbosity.

> If you want a guide like this, some questions:
> * do you want asciidoc, something else, or don't care?
> * name it Documentation/CommitMessageGuide ? or something else?

I think people who wrote the existing "Checklist" section on "Commits:"
meant to outline what constitutes a good commit log message (look for "the
body should").  It already says "motivation" and "contrast with previous
behaviour".  Cf.  47afed5 (SubmittingPatches: itemize and reflect upon
well written changes, 2009-04-28).

I unfortunately don't seem to be able to parse what the last part of what
47afed5 brought in is trying to say.  Perhaps a bad cut-and-paste job?

How about this as a not-too-verbose compromise?

-- >8 --
Subject: SubmittingPatches: clarify the expected commit log description

Earlier, 47afed5 (SubmittingPatches: itemize and reflect upon well written
changes, 2009-04-28) added a discussion on the contents of the commit log
message, but the last part of the new paragraph didn't make much sense.
Reword it slightly to make it more readable.

Update the "quicklist" to clarify what we mean by "motivation" and
"contrast".  Also mildly discourage external references.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/SubmittingPatches |   26 ++++++++++++++++++--------
 1 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 72741eb..c3b0816 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -10,10 +10,18 @@ Checklist (and a short version for the impatient):
 	  description (50 characters is the soft limit, see DISCUSSION
 	  in git-commit(1)), and should skip the full stop
 	- the body should provide a meaningful commit message, which:
-		- uses the imperative, present tense: "change",
-		  not "changed" or "changes".
-		- includes motivation for the change, and contrasts
-		  its implementation with previous behaviour
+	  . explains the problem the change tries to solve, iow, what
+	    is wrong with the current code without the change.
+	  . justifies the way the change solves the problem, iow, why
+	    the result with the change is better.
+	  . alternate solutions considered but discarded, if any.
+	- describe changes in imperative mood, e.g. "make xyzzy do frotz"
+	  instead of "[This patch] makes xyzzy do frotz" or "[I] changed
+	  xyzzy to do frotz", as if you are giving orders to the codebase
+	  to change its behaviour.
+	- try to make sure your explanation can be understood without
+	  external resources. Instead of giving a URL to a mailing list
+	  archive, summarize the relevant points of the discussion.
 	- add a "Signed-off-by: Your Name <you@example.com>" line to the
 	  commit message (or just use the option "-s" when committing)
 	  to confirm that you agree to the Developer's Certificate of Origin
@@ -90,7 +98,10 @@ your commit head.  Instead, always make a commit with complete
 commit message and generate a series of patches from your
 repository.  It is a good discipline.
 
-Describe the technical detail of the change(s).
+Give an explanation for the change(s) that is detailed enough so
+that people can judge if it is good thing to do, without reading
+the actual patch text to determine how well the code does what
+the explanation promises to do.
 
 If your description starts to get too long, that's a sign that you
 probably need to split up your commit to finer grained pieces.
@@ -99,9 +110,8 @@ help reviewers check the patch, and future maintainers understand
 the code, are the most beautiful patches.  Descriptions that summarise
 the point in the subject well, and describe the motivation for the
 change, the approach taken by the change, and if relevant how this
-differs substantially from the prior version, can be found on Usenet
-archives back into the late 80's.  Consider it like good Netiquette,
-but for code.
+differs substantially from the prior version, are all good things
+to have.
 
 Oh, another thing.  I am picky about whitespaces.  Make sure your
 changes do not trigger errors with the sample pre-commit hook shipped

Re: [PATCH v4] Documentation fix: git log -p does not imply -c.

[Adam Monsen]

Junio C Hamano wrote:
>> - Read previous commit messages. Emulate the best ones.
> 
> I thought you were trying to teach new people how to gauge the "goodness"
> with this list.  How would they know which ones are "best"? ;-)

Heh, glad you asked. I was being intentionally vague. The list just
provides some ideas. "Best" is an ideal. It's subjective. I don't want
people to just do what I say, I want to inspire them to do something better.

Maybe:

  Read previous commit messages. Strive to make yours better.

Again, we're talking about commit messages, not raising children or
anything. But commit messages help others, so why not try to make them the
"best" they can be?

> I try to give this list to new contributors early in their initiation
> process (ideally before their patches hit the codebase).  That is probably
> why many of the existing commits you saw in "git log" more or less
> conformed to the recommendation.

Cool. That's well-written and helpful.

>> - Be verbose!
> 
> Please don't.  We want sufficiently detailed description, but we don't
> want verbosity.

:) fair enough. I wrote this because, in the Mifos community, I struggle
getting people to write enough of *anything* in commit messages. Good to
know that's not a problem in git land.

> How about this as a not-too-verbose compromise?

I like it. Some specific comments below.

> -		- includes motivation for the change, and contrasts
> -		  its implementation with previous behaviour
> +	  . explains the problem the change tries to solve, iow, what
> +	    is wrong with the current code without the change.

Good, but spell out "in other words". I had to look up that acronym.

> +	  . justifies the way the change solves the problem, iow, why
> +	    the result with the change is better.

Ditto.

> +	  . alternate solutions considered but discarded, if any.

Prepend with "mentions" or something.

> +	- try to make sure your explanation can be understood without
> +	  external resources. Instead of giving a URL to a mailing list
> +	  archive, summarize the relevant points of the discussion.

+1, but I prefer both the summary *and* the link.

So maybe:

  Instead of providing only a URL to a mailing list archive, summarize
  the relevant points of the discussion.

> -Describe the technical detail of the change(s).
> +Give an explanation for the change(s) that is detailed enough so
> +that people can judge if it is good thing to do, without reading
> +the actual patch text to determine how well the code does what
> +the explanation promises to do.

+1!

I noticed a few grammar errors in the doc, too. I'll reply with
another patch.

Sorry if this continued work on the SubmittingPatches
documentation is getting annoying. It's been useful for me to learn
how things are done around here. And it's important that the
document be well written, so people actually use it. I just thought
we might as well improve it a little more since we've already started.

[PATCH] SubmittingPatches: clean up commit message tips

[Adam Monsen]

Removed uncommon acronyms in the "Checklist" section. I had to look up
"iow" online, but this documentation should stand alone (without online
access) as well as commit messages.

Leave wiggle room for including URLs in commit messages. Having both
summaries of mailing list discussions as well as URLs in commit messages
gives the reader the choice of how deep into history they wish to dig.

Modify the section about trivial changes slightly... it makes more sense
that it is discouraging diffs pasted in emails as opposed to patches
generated with "git am".

Remove recommendations on commit messages from the "Make separate
commits for logically separate changes" section, they are covered
well and explicitly in the "Checklist" section on top. This section need
not repeat same, and the deleted text doesn't really apply to the
section it was under. Also remove irrelevant text about good commit
messages. The only relevant part of that paragraph was the first
sentence about breaking apart big commits into separate patches.

Consistently use the phrase "commit message" to mean the chunk of text
that describes a commit.

Also in "Checklist", make the third bullet under "...a meaningful
commit message, which:" flow better grammatically by adding "mentions".

Relevant mailing list discussion:

http://thread.gmane.org/gmane.comp.version-control.git/168481/focus=168717

Signed-off-by: Adam Monsen <haircut@gmail.com>
---

I realize that this commit probably violates the very virtues it recommends by
having a long commit message and fixing several things in one shot. Let me know
if you'd like me to break it apart. Also, I may of course have made further
errors. It does adhere to the subject of cleaning up commit message tips.

Hope this helps,
-Adam

 Documentation/SubmittingPatches |   38 ++++++++++++++++----------------------
 1 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index c3b0816..3f8bff5 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -10,18 +10,19 @@ Checklist (and a short version for the impatient):
 	  description (50 characters is the soft limit, see DISCUSSION
 	  in git-commit(1)), and should skip the full stop
 	- the body should provide a meaningful commit message, which:
-	  . explains the problem the change tries to solve, iow, what
-	    is wrong with the current code without the change.
-	  . justifies the way the change solves the problem, iow, why
-	    the result with the change is better.
-	  . alternate solutions considered but discarded, if any.
+	  . explains the problem the change tries to solve, in other words,
+	    what is wrong with the current code without the change.
+	  . justifies the way the change solves the problem, in other words,
+	    why the result with the change is better.
+	  . mentions alternate solutions considered but discarded, if any.
 	- describe changes in imperative mood, e.g. "make xyzzy do frotz"
 	  instead of "[This patch] makes xyzzy do frotz" or "[I] changed
 	  xyzzy to do frotz", as if you are giving orders to the codebase
 	  to change its behaviour.
 	- try to make sure your explanation can be understood without
-	  external resources. Instead of giving a URL to a mailing list
-	  archive, summarize the relevant points of the discussion.
+	  external resources. Instead of providing only a URL to a
+	  mailing list archive, summarize the relevant points of the
+	  discussion.
 	- add a "Signed-off-by: Your Name <you@example.com>" line to the
 	  commit message (or just use the option "-s" when committing)
 	  to confirm that you agree to the Developer's Certificate of Origin
@@ -52,14 +53,14 @@ Checklist (and a short version for the impatient):
 
 Long version:
 
-I started reading over the SubmittingPatches document for Linux
+I started reading over the SubmittingPatches document for the Linux
 kernel, primarily because I wanted to have a document similar to
-it for the core GIT to make sure people understand what they are
-doing when they write "Signed-off-by" line.
+it for core GIT to make sure people understand what they are
+doing when they include a "Signed-off-by" line.
 
 But the patch submission requirements are a lot more relaxed
-here on the technical/contents front, because the core GIT is
-thousand times smaller ;-).  So here is only the relevant bits.
+here on the technical/contents front, because the core GIT is a
+thousand times smaller ;-).  So here are just the relevant bits.
 
 (0) Decide what to base your work on.
 
@@ -93,7 +94,7 @@ commit is the tip of the topic branch.
 (1) Make separate commits for logically separate changes.
 
 Unless your patch is really trivial, you should not be sending
-out a patch that was generated between your working tree and
+out a diff that was generated between your working tree and
 your commit head.  Instead, always make a commit with complete
 commit message and generate a series of patches from your
 repository.  It is a good discipline.
@@ -103,15 +104,8 @@ that people can judge if it is good thing to do, without reading
 the actual patch text to determine how well the code does what
 the explanation promises to do.
 
-If your description starts to get too long, that's a sign that you
-probably need to split up your commit to finer grained pieces.
-That being said, patches which plainly describe the things that
-help reviewers check the patch, and future maintainers understand
-the code, are the most beautiful patches.  Descriptions that summarise
-the point in the subject well, and describe the motivation for the
-change, the approach taken by the change, and if relevant how this
-differs substantially from the prior version, are all good things
-to have.
+If your commit message starts to get too long, that's a sign that you
+probably need to split your commit into finer grained pieces.
 
 Oh, another thing.  I am picky about whitespaces.  Make sure your
 changes do not trigger errors with the sample pre-commit hook shipped
-- 
1.7.2.3

Re: [PATCH] SubmittingPatches: clean up commit message tips

[Junio C Hamano]

Adam Monsen <haircut@gmail.com> writes:

> Removed uncommon acronyms in the "Checklist" section. I had to look up
> "iow" online, but this documentation should stand alone (without online
> access) as well as commit messages.

We have only a handful of instances of that phrase in the codebase (run
"git grep" for them), so I am not strongly opposed to spelling it out, but
use of IOW is quite common on this list---I suspect that it is largely
because Linus uses the phrase quite often.  Cf.

    $ git log | grep -e IOW -e '^Author: ' | grep -B1 -e IOW

> Leave wiggle room for including URLs in commit messages.

I don't think the updated text is too bad, but I don't very much like the
above "wiggle room".

The guideline is written to suggest what you absolutely should include; it
is obviously Ok to add other things as necessary.  If common sense tells
the reader external references will help recollection, the guideline does
not forbid to include them. IOW, there are enough wiggle rooms already.

> Modify the section about trivial changes slightly... it makes more sense
> that it is discouraging diffs pasted in emails as opposed to patches
> generated with "git am".

s/am/format-patch/;

> Remove recommendations on commit messages from the "Make separate
> commits for logically separate changes" section,...
> sentence about breaking apart big commits into separate patches.

While I do not particularly hate this part, I think people who did the
"Checklist vs Long Version" meant to make each of them stand on its own.
Lazy people (or people who think they are experienced enough) read the
former, while the others who pride themselves being thorough will skip the
"for-lazy-people" digest version and read only "the real thing".

So overall, I am not enthused by this version.  Input from others may
be appreciated.

[PATCH v5] diff format documentation: clarify --cc and -c

[Adam Monsen]

The description was unclear if -c or --cc was the default (--cc is for
some commands), and incorrectly implied that the default applies to
all the diff generating commands.

Most importantly, "log" does not default to "--cc" (it defaults to
"--no-merges") and "log -p" obeys the user's wish to see non-combined
format.  Only "diff" (during merge and three-blob comparison) and
"show" use --cc as the default.

Signed-off-by: Adam Monsen <haircut@gmail.com>
---

Here's another try at "my" first git patch, a one-paragraph
documentation change. Now featuring a much-improved commit message by
Junio.

 Documentation/diff-generate-patch.txt |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/diff-generate-patch.txt b/Documentation/diff-generate-patch.txt
index 3ac2bea..c57460c 100644
--- a/Documentation/diff-generate-patch.txt
+++ b/Documentation/diff-generate-patch.txt
@@ -74,10 +74,13 @@ separate lines indicate the old and the new mode.
 combined diff format
 --------------------
 
-"git-diff-tree", "git-diff-files" and "git-diff" can take '-c' or
-'--cc' option to produce 'combined diff'.  For showing a merge commit
-with "git log -p", this is the default format; you can force showing
-full diff with the '-m' option.
+Any diff-generating command can take the `-c` or `--cc` option to
+produce a 'combined diff' when showing a merge. This is the default
+format when showing merges with linkgit:git-diff[1] or
+linkgit:git-show[1]. Note also that you can give the `-m' option to any
+of these commands to force generation of diffs with individual parents
+of a merge.
+
 A 'combined diff' format looks like this:
 
 ------------
-- 
1.7.2.3

[PATCH v6] diff format documentation: clarify --cc and -c

[Adam Monsen]

The description was unclear if -c or --cc was the default (--cc is for
some commands), and incorrectly implied that the default applies to
all diff generating commands.

Most importantly, "log" does not default to "--cc" (it defaults to
"--no-merges") and "log -p" obeys the user's wish to see non-combined
format.  Only "diff" (during merge and three-blob comparison) and
"show" use --cc as the default.

The genesis of this patch was me getting frustrated trying to find
changes hidden in conflict resolutions of a merge commit. Jeff King
proposed a documentation fix. I made it into a patch, and worked on it
with Junio. See the thread "frustrated forensics: hard to find diff
that undid a fix" on the git mailing list:

  http://thread.gmane.org/gmane.comp.version-control.git/168481

For more historical information about viewing merge conflict
resolutions, see this post by Linus from 2008:

  http://article.gmane.org/gmane.comp.version-control.git/89415

Signed-off-by: Adam Monsen <haircut@gmail.com>
---

Please ignore v5. I forgot to include links to mailing list archives
in that version.

 Documentation/diff-generate-patch.txt |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/diff-generate-patch.txt b/Documentation/diff-generate-patch.txt
index 3ac2bea..c57460c 100644
--- a/Documentation/diff-generate-patch.txt
+++ b/Documentation/diff-generate-patch.txt
@@ -74,10 +74,13 @@ separate lines indicate the old and the new mode.
 combined diff format
 --------------------
 
-"git-diff-tree", "git-diff-files" and "git-diff" can take '-c' or
-'--cc' option to produce 'combined diff'.  For showing a merge commit
-with "git log -p", this is the default format; you can force showing
-full diff with the '-m' option.
+Any diff-generating command can take the `-c` or `--cc` option to
+produce a 'combined diff' when showing a merge. This is the default
+format when showing merges with linkgit:git-diff[1] or
+linkgit:git-show[1]. Note also that you can give the `-m' option to any
+of these commands to force generation of diffs with individual parents
+of a merge.
+
 A 'combined diff' format looks like this:
 
 ------------
-- 
1.7.2.3

Re: [PATCH v3] Documentation fix: git log -p does not imply -c.

[Jeff King]

On Mon, Mar 07, 2011 at 04:21:54PM -0800, Junio C Hamano wrote:

> > +produced a 'combined diff' when showing a merge. This is the default
> 
> s/produced/produce/, I think.

Oops.

> > +format when showing merge conflicts with linkgit:git-diff[1] or a merge
> > +commit with linkgit:git-show[1]. Note also that you can view the full
> > +diff with the `-m` option.
> 
> This "Note" is a bit unclear what command it applies to, isn't it?  I know
> it applies to all the commands mentioned in the previous sentence in the
> paragraph, but we are not writing the documentation for me, so perhaps 
> 
> 	Note also that you can give the `-m' option to any of these
> 	commands to force generation of diffs with individual parents of a
> 	merge.

Yeah, reading it again, I agree it is unclear. Yours is better.

> Also -c and --cc are technically _not_ about "showing merge conflicts".
> It is about "showing a merge commit".  I don't know if we want to teach
> the distinction in this part of the document, though.

Yeah, I almost said that, but I couldn't figure out a way to convince
"git diff" to show me a merge commit, and clearly it is one of the
interesting commands to mention as "defaults to --cc". Again, I think
this section would be better as "This is what combined diff looks like"
and leave the discussion of "this is how and when you trigger combined
diff" to the individual command manpages. But that is a much bigger
cleanup.

> If you resolve a conflicted merge taking the results from only one side
> for a given hunk, --cc won't show anything.  If on the other hand, you
> futz with a clean merge so that your result does not match with any
> parent, --cc will show it.

Right, that's the same as "diff-tree --cc" would show if you committed
it, no? Which makes sense to me.

-Peff

[PATCH 2/2] English grammar fixes for combined diff doc.

[Adam Monsen]

This is incredibly minor, I only separated it from the first patch so it
was easier to see how this is separate from the other change.

Signed-off-by: Adam Monsen <haircut@gmail.com>
---
 Documentation/diff-generate-patch.txt |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/Documentation/diff-generate-patch.txt b/Documentation/diff-generate-patch.txt
index 6cd5270..607fa54 100644
--- a/Documentation/diff-generate-patch.txt
+++ b/Documentation/diff-generate-patch.txt
@@ -74,10 +74,9 @@ separate lines indicate the old and the new mode.
 combined diff format
 --------------------
 
-"git-diff-tree", "git-diff-files" and "git-diff" can take '-c' or
-'--cc' option to produce 'combined diff'.  You can force showing
-full diff with the '-m' option.  A 'combined diff' format looks like
-this:
+"git-diff-tree", "git-diff-files" and "git-diff" can take a '-c' or
+'--cc' option to produce 'combined diff' output.  You can force showing
+full diffs with the '-m' option.  A 'combined diff' looks like this:
 
 ------------
 diff --combined describe.c
-- 
1.7.2.3

Re: frustrated forensics: hard to find diff that undid a fix

[Martin von Zweigbergk]

On Fri, 4 Mar 2011, Adam Monsen wrote:

> I made a fix a month ago on the master branch in a shared repo. A week
> later, a colleague did a merge that undid the fix. I didn't figure out
> the problem until just now because I'd been assuming the fix was still
> on master. I mean, if it wasn't, I should see a reverse patch using "git
> log -p master", right? Wrong. Turns out the fix was undone as part of
> merge conflict resolution (I think).
> 
> Is there some way to include merge conflict resolutions in "git log -p"
> or "git show"? Apparently some important information can be hidden in
> the conflict resolution. Or, more likely, I just don't understand how
> this bit of git works.
> 
> I also tried bisect and pickaxe. Bisect wrongly identified the first bad
> commit, and pickaxe just didn't see the change at all.

I was also bitten by this at work not too long ago. I started
wondering if it would make sense to introduce a new option to git log
and friends that would show the differences compared to a recreated
merge commit. In the simple case where there are no merge conflicts,
this would show only changes that someone explicitly dropped, like in
your case. If there were conflicts, I imagine it would show the same
output as -c or --cc. Does this make any sense?

One of the reasons that people sometimes drop the 'theirs' side while
merging is that they see the files show up when running 'git status'
and they think "Hmm... I didn't modify this file, let's reset
it". Would it be completely nonsensical to suggest that 'git status'
could learn to, during a merge, compare to a recreated merge commit
instead of comparing to HEAD?

Let me know what you think. I haven't really thought this through, so
I wouldn't be surprised if I'm just talking nonsense.


/Martin