[RFC/PATCH] log: add log.firstparent option

This patch adds an option to turn on --first-parent all the
time, along with the corresponding --no-first-parent to
disable it. The "why" of this requires a bit of backstory.

Some projects (like git.git) encourage frequent rebasing to
generate a set of clean, bisectable patches for each topic.
The messy sequence of bug-ridden and bug-fixup commits is
lost in the rebase, and not part of the final history.

But other projects prefer to keep the messy history intact.
For one thing, it makes collaboration on a topic easier, as
developers can simply pull from each other during the messy
development. And two, that history may later be useful when
tracking down a bug, because it gives more insight into the
actual thought process of the developer.

But in this latter case you want _two_ views of history. You
may want to see the "simple" version in which a series of
fully-formed topics hit the branch (and you would like to
see the diff of their final form). Or you may want to see
the messy details, because you are digging into a bug
related to the topic.

One proposal we have seen in the past is to keep the messy
history as a "shadow" parent of the real commits. That is,
to introduce a new parent-like header into the commit
object, but not traverse it by default in "git log". So it
remains hidden until you ask to dig into a particular topic
(presumably with a "log --show-messy-parents" option or
similar). So by default you get the simple view, but can dig
further if you wish.

But we can observe that such shadow parents can be
implemented as real parents; the problem isn't one of the
underlying data structure, but how we present it in "git
log". In other words, a perfectly reasonable workflow is:

  - make your messy commits on a side branch

  - do a non-fast-forward merge to bring them into master.
    The commit message for this merge should be meaningful
    and describe the topic as a whole.

  - view the simple history with "git log --first-parent -m"

  - view the complex history with "git log"

But since you probably want to view the simple history most
of the time, it would be nice to be able to default to that,
and switch to the more complicated view with a command line
option. Hence this patch.

Suggested-by: Josh Bleecher Snyder <redacted>
---
This came out of a discussion I had with Josh as OSCON. I
don't think I would personally use it, because git.git is
not a messy-workflow project. But I think that GitHub pushes
people into this sort of workflow (the PR becomes the
interesting unit of change), and my understanding is that
Gerrit does, as well.

There are probably some other things we (and others) could
do to help support it:

  - currently "--first-parent -p" needs "-m" to show
    anything useful; this is being discussed elsewhere, and
    it would be nice if it Just Worked (and showed the diff
    between the merge and the first-parent)

  - the commit messages for merges are often not great. A
    few versions ago, I think, we started opening the editor
    for merges, which is good. GitHub's PR-merge includes
    the PR subject in the commit message, but not all of the
    rationale and discussion. And in both git-generated and
    GitHub-generated messages, the subject isn't amazing
    (it's "merge topic jk/some-shorthand", which is barely
    tolerable if you use good branch names; it could be
    something like the subject-line of the cover letter for
    the patch series).

    So I think this could easily be improved by GitHub (we
    have the PR subject and body, after all). It's harder
    for a mailing list project like git.git, because Git
    never actually sees the subject line. I think it would
    require teaching git-am the concept of a patch series.

    I don't know offhand what Gerrit merges look like.

  - we already have merge.ff to default to making extra
    merge commits. And if you use GitHub's UI to do the
    merge, it uses --no-ff. I don't think we would want
    these to become the default, so there's probably nothing
    else to be done there.

Signed-off-by: Jeff King <redacted>
---
 Documentation/config.txt |  4 ++++
 builtin/log.c            |  6 ++++++
 revision.c               |  2 ++
 t/t4202-log.sh           | 30 ++++++++++++++++++++++++++++++
 4 files changed, 42 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3e37b93..e9c3763 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -1802,6 +1802,10 @@ log.mailmap::
 	If true, makes linkgit:git-log[1], linkgit:git-show[1], and
 	linkgit:git-whatchanged[1] assume `--use-mailmap`.
 
+log.firstparent::
+	If true, linkgit:git-log[1] will default to `--first-parent`;
+	can be overridden by supplying `--no-first-parent`.
+
 mailinfo.scissors::
 	If true, makes linkgit:git-mailinfo[1] (and therefore
 	linkgit:git-am[1]) act by default as if the --scissors option

diff --git a/builtin/log.c b/builtin/log.c
index 8781049..3e9b034 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -31,6 +31,7 @@ static const char *default_date_mode = NULL;
 
 static int default_abbrev_commit;
 static int default_show_root = 1;
+static int default_first_parent;
 static int decoration_style;
 static int decoration_given;
 static int use_mailmap_config;

@@ -109,6 +110,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)
 	rev->abbrev_commit = default_abbrev_commit;
 	rev->show_root_diff = default_show_root;
 	rev->subject_prefix = fmt_patch_subject_prefix;
+	rev->first_parent_only = default_first_parent;
 	DIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);
 
 	if (default_date_mode)

@@ -396,6 +398,10 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		use_mailmap_config = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "log.firstparent")) {
+		default_first_parent = git_config_bool(var, value);
+		return 0;
+	}
 
 	if (grep_config(var, value, cb) < 0)
 		return -1;

diff --git a/revision.c b/revision.c
index ab97ffd..a03a84b 100644
--- a/revision.c
+++ b/revision.c

@@ -1760,6 +1760,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		return argcount;
 	} else if (!strcmp(arg, "--first-parent")) {
 		revs->first_parent_only = 1;
+	} else if (!strcmp(arg, "--no-first-parent")) {
+		revs->first_parent_only = 0;
 	} else if (!strcmp(arg, "--ancestry-path")) {
 		revs->ancestry_path = 1;
 		revs->simplify_history = 0;

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 1b2e981..de1c35d 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh

@@ -871,4 +871,34 @@ test_expect_success 'log --graph --no-walk is forbidden' '
 	test_must_fail git log --graph --no-walk
 '
 
+test_expect_success 'setup simple merge for first-parent tests' '
+	git tag fp-base &&
+	test_commit master &&
+	git checkout -b fp-side &&
+	test_commit side &&
+	git checkout master &&
+	git merge --no-ff fp-side
+'
+
+test_expect_success 'log.firstparent config turns on first-parent' '
+	test_config log.firstparent true &&
+	cat >expect <<-\EOF &&
+	Merge branch '\''fp-side'\''
+	master
+	EOF
+	git log --format=%s fp-base.. >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --no-first-parent override log.firstparent' '
+	test_config log.firstparent true &&
+	cat >expect <<-\EOF &&
+	Merge branch '\''fp-side'\''
+	side
+	master
+	EOF
+	git log --no-first-parent --format=%s fp-base.. >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.5.0.rc2.540.ge5d4f14

On Wed, Jul 22, 2015 at 06:23:44PM -0700, Jeff King wrote:

This patch adds an option to turn on --first-parent all the
time, along with the corresponding --no-first-parent to
disable it.

[Putting on my scripter hat]

I sometimes think, "it would be really helpful if we had a way
to tell Git that it should ignore config variables".

This is especially helpful for script writers.   It's pretty
easy to break existing scripts by introducing new config knobs.

For example, "user.name" and "user.email" can be whitelisted by
the calling script and and everything else would just use the
stock defaults.

That way, script writers don't have to do version checks to
figuring out when and when not to include flags like
--no-first-parent, etc.

Would something like,

	GIT_CONFIG_WHITELIST="user.email user.name" \
	git ...

be a sensible interface to such a feature?
-- 
David

On Wed, Jul 22, 2015 at 09:40:10PM -0700, David Aguilar wrote:

On Wed, Jul 22, 2015 at 06:23:44PM -0700, Jeff King wrote:

quoted

This patch adds an option to turn on --first-parent all the
time, along with the corresponding --no-first-parent to
disable it.

[Putting on my scripter hat]

I sometimes think, "it would be really helpful if we had a way
to tell Git that it should ignore config variables".

This is especially helpful for script writers.   It's pretty
easy to break existing scripts by introducing new config knobs.

I think the purpose of --no-first-parent here is slightly orthogonal. It
is meant to help the user during the odd time that they need to
countermand their config.

Script writers should not care here, because they should not be parsing
the output of the porcelain "log" command in the first place. It already
has many gotchas (e.g., log.date, log.abbrevCommit).

I am sympathetic, though. There are some things that git-log can do that
rev-list cannot, so people end up using it in scripts. I think you can
avoid it with a "rev-list | diff-tree" pipeline, though I'm not 100%
sure if that covers all cases. But I would much rather see a solution
along the lines of making the plumbing cover more cases, rather than
trying to make the porcelain behave in a script.

That way, script writers don't have to do version checks to
figuring out when and when not to include flags like
--no-first-parent, etc.

One trick you can do is:

  git -c log.firstparent=false log ...

Older versions of git will ignore the unknown config option, and newer
ones will override anything the user has in their config file.

Would something like,

	GIT_CONFIG_WHITELIST="user.email user.name" \
	git ...

be a sensible interface to such a feature?

I dunno.  That's at least easy to implement. But the existing suggested
interface is really "run the plumbing", and then it automatically has a
sensible set of config options for each command, so that scripts don't
have to make their own whitelist (e.g., diff-tree still loads userdiff
config, but not anything that would change the output drastically, like
diff.mnemonicprefix).

-Peff

On Wed, Jul 22, 2015 at 10:14:45PM -0700, Jeff King wrote:

Script writers should not care here, because they should not be parsing
the output of the porcelain "log" command in the first place. It already
has many gotchas (e.g., log.date, log.abbrevCommit).

I am sympathetic, though. There are some things that git-log can do that
rev-list cannot, so people end up using it in scripts. I think you can
avoid it with a "rev-list | diff-tree" pipeline, though I'm not 100%
sure if that covers all cases. But I would much rather see a solution
along the lines of making the plumbing cover more cases, rather than
trying to make the porcelain behave in a script.

Ah, I see in a nearby thread that you just recently fixed a problem with
git-subtree and log.date, so I see now why you are so interested. :)

And I was also reminded by that usage of why rev-list is annoying in
scripts: even with "--format", it insists on writing the "commit ..."
header. I wonder if we could fix that...

-Peff

On Wed, Jul 22, 2015 at 10:48 PM, Jeff King [off-list ref] wrote:

On Wed, Jul 22, 2015 at 10:14:45PM -0700, Jeff King wrote:

quoted

Script writers should not care here, because they should not be parsing
the output of the porcelain "log" command in the first place. It already
has many gotchas (e.g., log.date, log.abbrevCommit).

I am sympathetic, though. There are some things that git-log can do that
rev-list cannot, so people end up using it in scripts. I think you can
avoid it with a "rev-list | diff-tree" pipeline, though I'm not 100%
sure if that covers all cases. But I would much rather see a solution
along the lines of making the plumbing cover more cases, rather than
trying to make the porcelain behave in a script.

Ah, I see in a nearby thread that you just recently fixed a problem with
git-subtree and log.date, so I see now why you are so interested. :)

And I was also reminded by that usage of why rev-list is annoying in
scripts: even with "--format", it insists on writing the "commit ..."
header. I wonder if we could fix that...

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Agreed. Fix the plumbing instead and document how/why to use it
instead of the porcelain. We might do better to help clearly document
which commands are porcelain and which are plumbing maybe by
referencing which plumbings to use in place of various porcelain
commands. I know, for example, that git status already does this.

Regards,
Jake

On Wed, Jul 22, 2015 at 11:32:49PM -0700, Jacob Keller wrote:

Agreed. Fix the plumbing instead and document how/why to use it
instead of the porcelain. We might do better to help clearly document
which commands are porcelain and which are plumbing maybe by
referencing which plumbings to use in place of various porcelain
commands. I know, for example, that git status already does this.

"man git" already has such a list (which is generated from the
annotations in command-list.txt). But I agree that it would probably be
helpful to point people directly from "git log" to "git rev-list" and
vice versa.

-Peff

On Wed, Jul 22, 2015 at 11:53 PM, Jeff King [off-list ref] wrote:

"man git" already has such a list (which is generated from the
annotations in command-list.txt). But I agree that it would probably be
helpful to point people directly from "git log" to "git rev-list" and
vice versa.

-Peff

That's good. I just know that I've had many a co-worker complain
because the man page felt too technical because they accidentally
found their way into a plumbing section. If I heard a specific case of
confusion again in the future I'll try to work up a patch for it.

Regards,
Jake

Jacob Keller venit, vidit, dixit 23.07.2015 08:55:

On Wed, Jul 22, 2015 at 11:53 PM, Jeff King [off-list ref] wrote:

quoted

"man git" already has such a list (which is generated from the
annotations in command-list.txt). But I agree that it would probably be
helpful to point people directly from "git log" to "git rev-list" and
vice versa.

-Peff

That's good. I just know that I've had many a co-worker complain
because the man page felt too technical because they accidentally
found their way into a plumbing section. If I heard a specific case of
confusion again in the future I'll try to work up a patch for it.

Regards,
Jake

That reminds me of my attempt to add those "categories" to the man pages
of each command (rather than just to that of "git") so that users know
where they landed. It died off, though: I preferred just specifying the
category (maybe with a long form), others including the whole
explanation of the category (which I thought would be too much text; we
have the glossary for that).

Would something like that help? Maybe "category" plus optionally pointer
to a related command in the "other" category.

Michael

On Thu, Jul 23, 2015 at 11:53:37AM +0200, Michael J Gruber wrote:

That reminds me of my attempt to add those "categories" to the man pages
of each command (rather than just to that of "git") so that users know
where they landed. It died off, though: I preferred just specifying the
category (maybe with a long form), others including the whole
explanation of the category (which I thought would be too much text; we
have the glossary for that).

Would something like that help? Maybe "category" plus optionally pointer
to a related command in the "other" category.

Maybe a "SCRIPTING" section at the end of the page?

-Peff

On Wed, Jul 22, 2015 at 6:23 PM, Jeff King [off-list ref] wrote:

This patch adds an option to turn on --first-parent all the
time, along with the corresponding --no-first-parent to
disable it. The "why" of this requires a bit of backstory.

Some projects (like git.git) encourage frequent rebasing to
generate a set of clean, bisectable patches for each topic.
The messy sequence of bug-ridden and bug-fixup commits is
lost in the rebase, and not part of the final history.

But other projects prefer to keep the messy history intact.
For one thing, it makes collaboration on a topic easier, as
developers can simply pull from each other during the messy
development. And two, that history may later be useful when
tracking down a bug, because it gives more insight into the
actual thought process of the developer.

But in this latter case you want _two_ views of history. You
may want to see the "simple" version in which a series of
fully-formed topics hit the branch (and you would like to
see the diff of their final form). Or you may want to see
the messy details, because you are digging into a bug
related to the topic.

One proposal we have seen in the past is to keep the messy
history as a "shadow" parent of the real commits. That is,
to introduce a new parent-like header into the commit
object, but not traverse it by default in "git log". So it
remains hidden until you ask to dig into a particular topic
(presumably with a "log --show-messy-parents" option or
similar). So by default you get the simple view, but can dig
further if you wish.

But we can observe that such shadow parents can be
implemented as real parents; the problem isn't one of the
underlying data structure, but how we present it in "git
log". In other words, a perfectly reasonable workflow is:

  - make your messy commits on a side branch

  - do a non-fast-forward merge to bring them into master.
    The commit message for this merge should be meaningful
    and describe the topic as a whole.

  - view the simple history with "git log --first-parent -m"

  - view the complex history with "git log"

But since you probably want to view the simple history most
of the time, it would be nice to be able to default to that,
and switch to the more complicated view with a command line
option. Hence this patch.

Suggested-by: Josh Bleecher Snyder <redacted>
---
This came out of a discussion I had with Josh as OSCON. I
don't think I would personally use it, because git.git is
not a messy-workflow project. But I think that GitHub pushes
people into this sort of workflow (the PR becomes the
interesting unit of change), and my understanding is that
Gerrit does, as well.

Github pull request messages
are similar to cover letters, so you could send a series with a
good cover letter, but crappy unfinished patches inside the series.
After applying all patches it could be all nice, i.e. compiles, tests, adds the
new functionality. It might be just a commit in between which may not even
compile. That's my understanding of the messy-workflow.

Gerrit cannot provide such a workflow easily as it's rather commit
centric and not branch centered. So you need to approve each
commit on its own and until 2 weeks ago you even needed to submit
each commit. (By now Gerrit has learned to submit a full branch, that
is you submit a commit and all its ancestors will integrate as well if they
were approved.)
Previously when the ancestors were not approved the commit would be
"submitted, merge pending", so it would wait for each commit to be approved
and submitted.
And because of this commit-centric workflow, the crappy commit in the series
is put into spot light.

Apart from that, the first-parent option gains some traction currently, Compare
https://git.eclipse.org/r/#/c/52381/ for example ;)

There are probably some other things we (and others) could
do to help support it:

  - currently "--first-parent -p" needs "-m" to show
    anything useful; this is being discussed elsewhere, and
    it would be nice if it Just Worked (and showed the diff
    between the merge and the first-parent)

  - the commit messages for merges are often not great. A
    few versions ago, I think, we started opening the editor
    for merges, which is good. GitHub's PR-merge includes
    the PR subject in the commit message, but not all of the
    rationale and discussion. And in both git-generated and
    GitHub-generated messages, the subject isn't amazing
    (it's "merge topic jk/some-shorthand", which is barely
    tolerable if you use good branch names; it could be
    something like the subject-line of the cover letter for
    the patch series).

    So I think this could easily be improved by GitHub (we
    have the PR subject and body, after all). It's harder
    for a mailing list project like git.git, because Git
    never actually sees the subject line. I think it would
    require teaching git-am the concept of a patch series.

This would be cool I would imagine.

    I don't know offhand what Gerrit merges look like.

Let's say it's complicated. Depending on configuration a few things
may happen. There are different integration strategies
* merge always
* merge if necessary (fastforward else)
* fastforward only
* rebase if necessary (to make it a linear history)
* cherrypick (which may add footers like Reviewed-by: to have that
information in the git history)

I think the "merge always" strategy would comfort from this patch.

  - we already have merge.ff to default to making extra
    merge commits. And if you use GitHub's UI to do the
    merge, it uses --no-ff. I don't think we would want
    these to become the default, so there's probably nothing
    else to be done there.

Signed-off-by: Jeff King <redacted>

The signoff is better placed above :)

On Thu, Jul 23, 2015 at 03:14:50PM -0700, Stefan Beller wrote:

Github pull request messages
are similar to cover letters, so you could send a series with a
good cover letter, but crappy unfinished patches inside the series.
After applying all patches it could be all nice, i.e. compiles, tests, adds the
new functionality. It might be just a commit in between which may not even
compile. That's my understanding of the messy-workflow.

Yeah, that's certainly one messy workflow. There are many levels of
messy. But I think fundamentally it comes down to: do you show the steps
that went into the result, or do you squash them out into a clean
history?

Gerrit cannot provide such a workflow easily as it's rather commit
centric and not branch centered. So you need to approve each
commit on its own and until 2 weeks ago you even needed to submit
each commit. (By now Gerrit has learned to submit a full branch, that
is you submit a commit and all its ancestors will integrate as well if they
were approved.)
Previously when the ancestors were not approved the commit would be
"submitted, merge pending", so it would wait for each commit to be approved
and submitted.
And because of this commit-centric workflow, the crappy commit in the series
is put into spot light.

Yeah, I agree that does not sound quite the same as the GitHub flow.

quoted

    So I think this could easily be improved by GitHub (we
    have the PR subject and body, after all). It's harder
    for a mailing list project like git.git, because Git
    never actually sees the subject line. I think it would
    require teaching git-am the concept of a patch series.

This would be cool I would imagine.

I talked with some GitHub folks about this. One of the challenges is
that the PR body is in Markdown, but we'd probably want "real" text in
the merge commit. One option would be to simply convert
Markdown->HTML->Text, which should provide a fairly clean version. It
will take some playing with, but I'm going to see what I can do.

quoted

    I don't know offhand what Gerrit merges look like.

Let's say it's complicated. Depending on configuration a few things
may happen. There are different integration strategies
* merge always
* merge if necessary (fastforward else)
* fastforward only
* rebase if necessary (to make it a linear history)
* cherrypick (which may add footers like Reviewed-by: to have that
information in the git history)

I think the "merge always" strategy would comfort from this patch.

Yeah, I think for anything else it's not a good idea (especially if you
fast-forward onto master).

quoted

  - we already have merge.ff to default to making extra
    merge commits. And if you use GitHub's UI to do the
    merge, it uses --no-ff. I don't think we would want
    these to become the default, so there's probably nothing
    else to be done there.

Signed-off-by: Jeff King <redacted>

The signoff is better placed above :)

Whoops. Usually I "format-patch -s" and then add any notes while
sending. But the wifi at OSCON was so abysmal that instead I wrote the
notes directly into the commit message to send the whole thing later.
And of course format-patch is not smart enough to know that I meant
everything after the "---" as notes. :)

-Peff

On Fri, Jul 24, 2015 at 12:40 AM, Jeff King [off-list ref] wrote:

Whoops. Usually I "format-patch -s" and then add any notes while
sending. But the wifi at OSCON was so abysmal that instead I wrote the
notes directly into the commit message to send the whole thing later.
And of course format-patch is not smart enough to know that I meant
everything after the "---" as notes. :)

-Peff

Kind of a side track but...

I think it's up to the caller of git-am to use "--scissors" to cut the
log? But maybe we could add an option to git-format patch which
formats and cuts via scissors as it generates the message? Not sure
the best way to interpret this, but I know I've had trouble where I
wrote some notes into an email and lost it because I killed the email
for some other edit. Keeping them inside my local commits before
sending out email would be handy.. hmmmm

Regards,
Jake

On Fri, Jul 24, 2015 at 12:46:57AM -0700, Jacob Keller wrote:

On Fri, Jul 24, 2015 at 12:40 AM, Jeff King [off-list ref] wrote:

quoted

Whoops. Usually I "format-patch -s" and then add any notes while
sending. But the wifi at OSCON was so abysmal that instead I wrote the
notes directly into the commit message to send the whole thing later.
And of course format-patch is not smart enough to know that I meant
everything after the "---" as notes. :)

Kind of a side track but...

I think it's up to the caller of git-am to use "--scissors" to cut the
log? But maybe we could add an option to git-format patch which
formats and cuts via scissors as it generates the message? Not sure
the best way to interpret this, but I know I've had trouble where I
wrote some notes into an email and lost it because I killed the email
for some other edit. Keeping them inside my local commits before
sending out email would be handy.. hmmmm

The "---" is orthogonal to "--scissors". With "--scissors", the full
format of the body is:

   some notes or cover letter

   -- >8 --
   the actual commit message

   ---
   more notes

   diff --git ...etc...

So here I was trying to use the "---" to add notes at the end (not
because --scissors is not used consistently, but because I wanted the
reader to see them after reading the commit message). So you could keep
notes in the commit message by writing:

  my notes here

  -- >8 --
  the real commit message

and then "format-patch -s" just works, because it is munging the end.
But if you want to be able to add commit notes at the end, you need
format-patch to realize that any trailers should go before the "---"
(i.e., to realize that the "---" is syntactically significant, and not
just part of your message).

Another option would be to teach git-commit to split the "---" from the
commit message itself, and put the bits after it into git-notes (and
then format-patch already knows how to handle that). I had a patch
series to do that long ago, but I found that I never used it (I usually
_do_ type my notes in the mailer as I'm sending), so I never seriously
pushed for inclusion. I might be able to dig it out of the archive if
you're interested.

-Peff