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

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

[Jeff King]

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 <josharian@gmail.com>
---
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 <peff@peff.net>
---
 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

Config variables and scripting // was Re: [RFC/PATCH] log: add log.firstparent option

[David Aguilar]

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

Re: Config variables and scripting // was Re: [RFC/PATCH] log: add log.firstparent option

[Jeff King]

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:
> > 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

Re: Config variables and scripting // was Re: [RFC/PATCH] log: add log.firstparent option

[Jeff King]

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

Re: Config variables and scripting // was Re: [RFC/PATCH] log: add log.firstparent option

[Jacob Keller]

On Wed, Jul 22, 2015 at 10:48 PM, Jeff King <peff@peff.net> wrote:
> 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
> --
> 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

Re: Config variables and scripting // was Re: [RFC/PATCH] log: add log.firstparent option

[Jeff King]

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

Re: Config variables and scripting // was Re: [RFC/PATCH] log: add log.firstparent option

[Jacob Keller]

On Wed, Jul 22, 2015 at 11:53 PM, Jeff King <peff@peff.net> 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

Re: Config variables and scripting // was Re: [RFC/PATCH] log: add log.firstparent option

[Michael J Gruber]

Jacob Keller venit, vidit, dixit 23.07.2015 08:55:
> On Wed, Jul 22, 2015 at 11:53 PM, Jeff King <peff@peff.net> 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
> 

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

Re: Config variables and scripting // was Re: [RFC/PATCH] log: add log.firstparent option

[Jeff King]

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

Re: Config variables and scripting // was Re: [RFC/PATCH] log: add log.firstparent option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

Very well said.

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

[Stefan Beller]

On Wed, Jul 22, 2015 at 6:23 PM, Jeff King <peff@peff.net> 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 <josharian@gmail.com>
> ---
> 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 <peff@peff.net>

The signoff is better placed above :)

> ---
>  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
> --
> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

While I can see the reasoning behind the above [*1*], I am not sure
if the output with "--first-parent" would always be a good match for
the "simple" version.  Wouldn't the people who keep these messy
histories also those who merge project trunk into a random topic and
push the result back as the updated project trunk?  Admittedly, that
merely is saying that "--first-parent" is not a solution to such a
project, and does not say much about the usefulness of the new
configuration, so I'd queue it as-is.

[Footnote]

*1* I do not necessarily agree, though.  The history being messy is
    a sign that "the actual thought process of the developer" was
    not clearly expressed in the work, and it is not likely that you
    have more insight by looking at it---you will see more mess, for
    certain, though.

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

[Jacob Keller]

On Thu, Jul 23, 2015 at 3:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> 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.
>
> While I can see the reasoning behind the above [*1*], I am not sure
> if the output with "--first-parent" would always be a good match for
> the "simple" version.  Wouldn't the people who keep these messy
> histories also those who merge project trunk into a random topic and
> push the result back as the updated project trunk?  Admittedly, that
> merely is saying that "--first-parent" is not a solution to such a
> project, and does not say much about the usefulness of the new
> configuration, so I'd queue it as-is.
>
> [Footnote]
>
> *1* I do not necessarily agree, though.  The history being messy is
>     a sign that "the actual thought process of the developer" was
>     not clearly expressed in the work, and it is not likely that you
>     have more insight by looking at it---you will see more mess, for
>     certain, though.
> --
> 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

Based on the work I've done with people who don't mind "messy"
history, I find that they tend to break the first-parent rule fairly
often, and I don't personally find much value in the "mess" of their
history. However, I think the ability for git-am to see a patch series
cover-letter and include a merge message would be valuable, as there
is often information lost in the cover letter when you apply a series.
I'm not sure how much work this would take in practice though.

I think some projects definitely benefit from the first-parent setup,
and it could be valuable, but I do tend to agree with Junio here that
the mess is always helpful. If may be helpful if people's commit
messages on that mess are good, but generally those that don't take
the time to rebase local work and re-express the commit messages are
not going to leave insightful messages the first time. However, have
the ability to view history this way is still possibly valuable.

Regards,
Jake

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

[Jeff King]

On Thu, Jul 23, 2015 at 03:46:33PM -0700, Junio C Hamano wrote:

> While I can see the reasoning behind the above [*1*], I am not sure
> if the output with "--first-parent" would always be a good match for
> the "simple" version.  Wouldn't the people who keep these messy
> histories also those who merge project trunk into a random topic and
> push the result back as the updated project trunk?  Admittedly, that
> merely is saying that "--first-parent" is not a solution to such a
> project, and does not say much about the usefulness of the new
> configuration, so I'd queue it as-is.
> 
> [Footnote]
> 
> *1* I do not necessarily agree, though.  The history being messy is
>     a sign that "the actual thought process of the developer" was
>     not clearly expressed in the work, and it is not likely that you
>     have more insight by looking at it---you will see more mess, for
>     certain, though.

I don't think people who keep "messy" commit histories are necessarily
prone to pushing to the trunk. And ditto for the footnote, I do not
think "messy" is necessarily a bad thing. It is really about whether you
choose to rebase out follow-on fixups (e.g., those that come from
review), or whether you choose to leave them in.

And there is a very good reason to leave them in: two people
collaborating on a topic have a much easier time if the other person is
not constantly rebasing.

So a GitHub-oriented workflow can be something like (and this is what we
use to develop GitHub internally):

  1. I do the usual process of creating a branch, writing some commits,
     pushing them up, and opening a pull request. This is equivalent to
     git.git, except the final step is "send to the list". I may use
     "rebase -i" during this process to make a nice sequence of commits.

  2. During review, I may get comments. In a git.git workflow, I am
     expected to re-roll the topic myself to address the comments. If I
     am lucky, the reviewer expressed their comments as a diff, and I
     can squash them in as appropriate. But in the GitHub workflow, the
     reviewer may simply check out the branch themselves locally, commit
     the fixes, and push them up. They become part of the PR.

  3. I may go back and forth with the reviewer several times, and there
     may be intermingled commits from both of us. At the end, one of us
     decides to merge the PR, and GitHub's merge button creates a non-ff
     commit on master, merging in the topic.

So we're "messy" in the sense that we chose to record the back-and-forth
during the development of the topic. But at no point was anybody in
danger of back-merging and putting the result directly onto master. Even
if they did back-merge from master onto the topic[1], the non-ff merge
to master means that any first-parent traversal starting from master
will never stray onto a PR branch at all.

Of course not everybody does this. But I think it's a perfectly sane
workflow. The PR merges become the most interesting unit of change to
review or find in the history, with the individual commits closer to
"what really happened".

-Peff

[1] We do actually back-merge master to topics all the time, because we
    have to make sure that we whatever we test and deploy contains all
    of what is already in master, even if the topic was originally
    forked from an older point. IOW, the QA process for merging is
    basically:

      - merge back from master so we know the tree at the tip of our
	topic is what _would_ be on master if we merged

      - test that tip (with automated tests, temporarily deploying to
	the live site, etc)

      - it's OK to merge to master if it has not moved; the merge would
        be a fast-forward. Instead, we create a merge commit, but the
	tree content will be the same (so we know that we are merging
	and deploying what has just passed QA).

    Incidentally, doing "git log --first-parent" while on the topic
    branch from this workflow does the reasonable thing. Because the
    merges on the topic branch are all backwards merges from master,
    following the first parent keeps you on the topic branch (rather
    than showing you all the upstream commits you just pulled in).

    Where I'd be most worried about a default first-parent hurting you
    is when you "git log $some_random_commit", at which point you may or
    may not want first-parent behavior. I suppose we could have a config
    option that kicks in only when using the default "HEAD". That seems
    more likely to do what you want all the time, but it also sounds
    rather confusingly magical.

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

[Jeff King]

On Thu, Jul 23, 2015 at 03:46:33PM -0700, Junio C Hamano wrote:

> Admittedly, that
> merely is saying that "--first-parent" is not a solution to such a
> project, and does not say much about the usefulness of the new
> configuration, so I'd queue it as-is.

Yeah, I agree that this patch does not make a judgement on particular
workflows, and can stand on its own. I buried a related deep in a
footnote in my other email, so let me bring it up again here.

I am not entirely convinced this won't bite somebody who gets a sha1
from some other source, and then wants to run:

  git log $some_other_sha1

who might be quite confused to start a first-parent traversal from
somewhere other than the tip of "master" or the tip of a topic branch.

-Peff

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

[Jeff King]

On Thu, Jul 23, 2015 at 11:07:58PM -0700, Jacob Keller wrote:

> I think some projects definitely benefit from the first-parent setup,
> and it could be valuable, but I do tend to agree with Junio here that
> the mess is always helpful. If may be helpful if people's commit
> messages on that mess are good, but generally those that don't take
> the time to rebase local work and re-express the commit messages are
> not going to leave insightful messages the first time. However, have
> the ability to view history this way is still possibly valuable.

I think a really simple example is something like:

  1. somebody implements as feature. It needs to handle cases a, b, and
     c, but it only handles case a. Therefore it is buggy.

  2. During review, somebody notices case b, and a new commit is made to
     fix it. Nobody notices case c.

  3. The topic is merged.

  4. Much later, somebody notices the system is buggy and hunts in the
     history.

In a "clean" history, the patches from steps 1 and 2 are squashed. While
reading the history, you see only "implement feature X", and no mention
of the bug and its fix. But even if the person writes a terrible commit
message for step (2), even seeing it pulled out into its own diff shows
the exact nature of the already-seen bug, and may make it more obvious
to realize that case (c) is a problem.

I realize that's kind of vague. Another way to think about it is: in a
squashing workflow like git.git, any time you have to turn to the
mailing list to read the original sequence of re-rolls, you would have
been better off if that information were in git. That's a minority case,
but I certainly have turned to it (in some cases, the "fix" from our
step 2 above actually introduces the new bug, and it's nice to see the
reasoning that went into it :) ).

Not that I am advocating for git.git to move to such a workflow. I think
on balance the "clean" history is nicer to work with. I am only arguing
that keeping the messy history is not without value; there are some
cases where it is nice to have (and we keep it in the list archive,
which is a minor pain to access compared to git).

-Peff

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

[Jeff King]

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.

> >     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.

> >     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).

> >   - 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 <peff@peff.net>
> 
> 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

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

[Jacob Keller]

On Fri, Jul 24, 2015 at 12:34 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Jul 23, 2015 at 11:07:58PM -0700, Jacob Keller wrote:
>
>> I think some projects definitely benefit from the first-parent setup,
>> and it could be valuable, but I do tend to agree with Junio here that
>> the mess is always helpful. If may be helpful if people's commit
>> messages on that mess are good, but generally those that don't take
>> the time to rebase local work and re-express the commit messages are
>> not going to leave insightful messages the first time. However, have
>> the ability to view history this way is still possibly valuable.
>
> I think a really simple example is something like:
>
>   1. somebody implements as feature. It needs to handle cases a, b, and
>      c, but it only handles case a. Therefore it is buggy.
>
>   2. During review, somebody notices case b, and a new commit is made to
>      fix it. Nobody notices case c.
>
>   3. The topic is merged.
>
>   4. Much later, somebody notices the system is buggy and hunts in the
>      history.
>
> In a "clean" history, the patches from steps 1 and 2 are squashed. While
> reading the history, you see only "implement feature X", and no mention
> of the bug and its fix. But even if the person writes a terrible commit
> message for step (2), even seeing it pulled out into its own diff shows
> the exact nature of the already-seen bug, and may make it more obvious
> to realize that case (c) is a problem.
>
> I realize that's kind of vague. Another way to think about it is: in a
> squashing workflow like git.git, any time you have to turn to the
> mailing list to read the original sequence of re-rolls, you would have
> been better off if that information were in git. That's a minority case,
> but I certainly have turned to it (in some cases, the "fix" from our
> step 2 above actually introduces the new bug, and it's nice to see the
> reasoning that went into it :) ).
>

Actually this and the GitHub workflow make sense to me. I am concerned
like you are about what to do in the case for passing commits.. maybe
just make the option turn it on only if no arguments are passed (ie:
"git log") and clearly document that it does not enable the behavior
for arbitrary commits? That would make it useful without being too
confusing nor too magical. Though, it does mean if you do pass a
commit and want it on you'd have to specify.. I certainly would find
it more confusing to have it enabled if I pass an arbitrary commit..
Maybe not though, I am not 100% sure on that.

> Not that I am advocating for git.git to move to such a workflow. I think
> on balance the "clean" history is nicer to work with. I am only arguing
> that keeping the messy history is not without value; there are some
> cases where it is nice to have (and we keep it in the list archive,
> which is a minor pain to access compared to git).
>
> -Peff

For projects which make this easier (pull request style) I think this
is making more sense. I'm not sure exactly whether it would be best to
only enable via "git log" with no parameters, but it does seem weird
to turn this on and then accidentally get the behavior on a random
commit log sequence.... Maybe it's not a huge deal either way though..

Regards,
Jake

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

[Jacob Keller]

On Fri, Jul 24, 2015 at 12:40 AM, Jeff King <peff@peff.net> 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

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

[Jeff King]

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

> On Fri, Jul 24, 2015 at 12:40 AM, Jeff King <peff@peff.net> 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. :)
> 
> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think a really simple example is something like:
>
>   1. somebody implements as feature. It needs to handle cases a, b, and
>      c, but it only handles case a. Therefore it is buggy.
>
>   2. During review, somebody notices case b, and a new commit is made to
>      fix it. Nobody notices case c.
>
>   3. The topic is merged.
>
>   4. Much later, somebody notices the system is buggy and hunts in the
>      history.
>
> In a "clean" history, the patches from steps 1 and 2 are squashed. While
> reading the history, you see only "implement feature X", and no mention
> of the bug and its fix. But even if the person writes a terrible commit
> message for step (2), even seeing it pulled out into its own diff shows
> the exact nature of the already-seen bug, and may make it more obvious
> to realize that case (c) is a problem.
>
> I realize that's kind of vague. Another way to think about it is: in a
> squashing workflow like git.git, any time you have to turn to the
> mailing list to read the original sequence of re-rolls, you would have
> been better off if that information were in git. That's a minority case,
> but I certainly have turned to it (in some cases, the "fix" from our
> step 2 above actually introduces the new bug, and it's nice to see the
> reasoning that went into it :) ).
>
> Not that I am advocating for git.git to move to such a workflow. 

I actually do not think the above is quite true.  In our kind of
"clean history, we do not squash 1 & 2.  See Paul's "rewrite am in
C" series, for example, that starts from a "buggy" (in the sense
that it does almost nothing in the beginning and then gradually
builds on).

Instead, even somebody did not have foresight to realize 'b' when
she adds code to handle 'a', we would make sure the solution for 'a'
is sufficiently clearly described in commit #1.

In other words, without #1 and #2 squashed together, the history you
presented in your example _could_ be already "clean".  It just boils
down to the quality of individual commit.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Jul 23, 2015 at 03:46:33PM -0700, Junio C Hamano wrote:
>
>> Admittedly, that
>> merely is saying that "--first-parent" is not a solution to such a
>> project, and does not say much about the usefulness of the new
>> configuration, so I'd queue it as-is.
>
> Yeah, I agree that this patch does not make a judgement on particular
> workflows, and can stand on its own. I buried a related deep in a
> footnote in my other email, so let me bring it up again here.
>
> I am not entirely convinced this won't bite somebody who gets a sha1
> from some other source, and then wants to run:
>
>   git log $some_other_sha1
>
> who might be quite confused to start a first-parent traversal from
> somewhere other than the tip of "master" or the tip of a topic branch.

Yeah, you actually convinced me reasonably well that it would
happen.  I'd never use it myself.  If people want to shoot
themselves in the foot, be my guest ;-)

Perhaps we should drop this, and give a shorter synonym to the
option?
.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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. :)

I think in the cycle we merged Couder's trailer stuff we updated the
helper functions to locate where the S-o-b should go in an existing
message and consolidated (or, at least "talked about consolidating")
them into a single helper.  I do not think we wrote any special case
for "a line with three-dashes and nothing else on it" when we did
so, but that function would be the logical place to do so.

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

[Jeff King]

On Fri, Jul 24, 2015 at 08:04:21AM -0700, Junio C Hamano wrote:

> > I think a really simple example is something like:
> >
> >   1. somebody implements as feature. It needs to handle cases a, b, and
> >      c, but it only handles case a. Therefore it is buggy.
> >
> >   2. During review, somebody notices case b, and a new commit is made to
> >      fix it. Nobody notices case c.
> >
> >   3. The topic is merged.
> >
> >   4. Much later, somebody notices the system is buggy and hunts in the
> >      history.
> [...]
> 
> I actually do not think the above is quite true.  In our kind of
> "clean history, we do not squash 1 & 2.  See Paul's "rewrite am in
> C" series, for example, that starts from a "buggy" (in the sense
> that it does almost nothing in the beginning and then gradually
> builds on).
> 
> Instead, even somebody did not have foresight to realize 'b' when
> she adds code to handle 'a', we would make sure the solution for 'a'
> is sufficiently clearly described in commit #1.

Sometimes. There's definitely human wisdom going into the decision to
squash or make a new commit, which is a good thing. Here's a
counter-example to the am series. When I wrote the object-freshening
code, I accidentally inverted the test for checking whether packs were
fresh. This was noticed in review, and I corrected it by inverting the
is_fresh function. But in doing so, I introduced a new bug, where the
test for loose objects was inverted.

The original fix was squashed in the re-roll, but much later we noticed
and diagnosed that new bug. It was very valuable to me to read the
mailing list archive to see what happened, because the fact that there
_was_ a bug fix was lost in the clean history.

-Peff

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

[Jeff King]

On Fri, Jul 24, 2015 at 08:31:55AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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. :)
> 
> I think in the cycle we merged Couder's trailer stuff we updated the
> helper functions to locate where the S-o-b should go in an existing
> message and consolidated (or, at least "talked about consolidating")
> them into a single helper.  I do not think we wrote any special case
> for "a line with three-dashes and nothing else on it" when we did
> so, but that function would be the logical place to do so.

Yeah, it nicely has the concept of "ignore this footer". But we would
want it only to kick in when doing emails (where the "---" is
syntactically significant), I would think. So something like the patch
below (no commit message because I'm in an airport right now; I'll add
tests and repost in the next day or two).

diff --git a/log-tree.c b/log-tree.c
index 7b1b57a..8a9c35b 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -688,8 +688,16 @@ void show_log(struct rev_info *opt)
 		ctx.from_ident = &opt->from_ident;
 	pretty_print_commit(&ctx, commit, &msgbuf);
 
-	if (opt->add_signoff)
-		append_signoff(&msgbuf, 0, APPEND_SIGNOFF_DEDUP);
+	if (opt->add_signoff) {
+		int ignore = 0;
+		if (ctx.fmt == CMIT_FMT_EMAIL) {
+			const char *dashes = strstr(msgbuf.buf, "---\n");
+			if (dashes &&
+			    (dashes == msgbuf.buf || dashes[-1] == '\n'))
+				ignore = msgbuf.len - (dashes - msgbuf.buf);
+		}
+		append_signoff(&msgbuf, ignore, APPEND_SIGNOFF_DEDUP);
+	}
 
 	if ((ctx.fmt != CMIT_FMT_USERFORMAT) &&
 	    ctx.notes_message && *ctx.notes_message) {

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

[Jeff King]

On Fri, Jul 24, 2015 at 06:36:34PM -0700, Jeff King wrote:

> > I think in the cycle we merged Couder's trailer stuff we updated the
> > helper functions to locate where the S-o-b should go in an existing
> > message and consolidated (or, at least "talked about consolidating")
> > them into a single helper.  I do not think we wrote any special case
> > for "a line with three-dashes and nothing else on it" when we did
> > so, but that function would be the logical place to do so.
> 
> Yeah, it nicely has the concept of "ignore this footer". But we would
> want it only to kick in when doing emails (where the "---" is
> syntactically significant), I would think. So something like the patch
> below (no commit message because I'm in an airport right now; I'll add
> tests and repost in the next day or two).

This works for "format-patch -s". But I guess that leaves open the
question of "commit --signoff". It should not matter when making a
commit new (after all, you have not yet had a chance to put the "---"
in). But something like "git commit --amend --signoff" might want to
handle it. Of course we have no idea if any "---" we find there is meant
to be an email notes-separator by the user, or if they happened to use
"---" for something else[1] (which is a bad idea if you have an emailed
patches workflow, but many people do not). So it's a bit riskier.

-Peff

[1] While reading the old "git commit --notes" thread recently, Johan
    Herland gave a plausible confusing example:

      What
      ----

      A commit message using markdown-like formatting conventions.

      Why
      ---

      To show that "---" can be part of a commit message. :)

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

[Jeff King]

On Fri, Jul 24, 2015 at 08:07:49AM -0700, Junio C Hamano wrote:

> > I am not entirely convinced this won't bite somebody who gets a sha1
> > from some other source, and then wants to run:
> >
> >   git log $some_other_sha1
> >
> > who might be quite confused to start a first-parent traversal from
> > somewhere other than the tip of "master" or the tip of a topic branch.
> 
> Yeah, you actually convinced me reasonably well that it would
> happen.  I'd never use it myself.  If people want to shoot
> themselves in the foot, be my guest ;-)
> 
> Perhaps we should drop this, and give a shorter synonym to the
> option?

I'm still on the fence to have the config kick in only for HEAD.
Something like (on top of my other patch, and the tests would still need
adjusted):

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e9c3763..f2b6a21 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1802,9 +1802,11 @@ 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`.
+log.defaultImpliesFirstParent::
+	If true, linkgit:git-log[1] will default to `--first-parent`
+	when showing the default ref (i.e., if you run only `git
+	log` to show `HEAD`, but not `git log $sha1`). Can be overridden
+	by supplying `--no-first-parent`.
 
 mailinfo.scissors::
 	If true, makes linkgit:git-mailinfo[1] (and therefore
diff --git a/builtin/log.c b/builtin/log.c
index 3e9b034..2bdb3fc 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -31,7 +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 default_implies_first_parent;
 static int decoration_style;
 static int decoration_given;
 static int use_mailmap_config;
@@ -110,7 +110,6 @@ 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)
@@ -398,8 +397,8 @@ 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);
+	if (!strcmp(var, "log.defaultimpliesfirstparent")) {
+		default_implies_first_parent = git_config_bool(var, value);
 		return 0;
 	}
 
@@ -504,6 +503,8 @@ static int show_tree_object(const unsigned char *sha1,
 
 static void show_rev_tweak_rev(struct rev_info *rev, struct setup_revision_opt *opt)
 {
+	if (default_implies_first_parent && !rev->pending.nr)
+		rev->first_parent_only = 1;
 	if (rev->ignore_merges) {
 		/* There was no "-m" on the command line */
 		rev->ignore_merges = 0;


It feels somewhat magical, but at least the config option name makes it
painfully clear exactly when it would kick in. I dunno. I am happy
enough for myself to just run "--first-parent" when that is what I want
to see. Giving it a shorter name would not help much, I think. It is not
the number of characters, but the fact that most people do not _know_
that --first-parent exists in the first place, or that it would be
useful in this case. I hoped with a config option it might become
something projects could recommend to their users[1] if the project has a
matching workflow. But maybe we could also rely on those same projects
to educate their users.

-Peff

[1] And if not an official recommendation from a project, this is the
    sort of "tips and tricks" information that may spread informally. But in
    theory so could knowledge of --first-parent.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This works for "format-patch -s". But I guess that leaves open the
> question of "commit --signoff". It should not matter when making a
> commit new (after all, you have not yet had a chance to put the "---"
> in). But something like "git commit --amend --signoff" might want to
> handle it. Of course we have no idea if any "---" we find there is meant
> to be an email notes-separator by the user, or if they happened to use
> "---" for something else[1] (which is a bad idea if you have an emailed
> patches workflow, but many people do not). So it's a bit riskier.
>
> -Peff
>
> [1] While reading the old "git commit --notes" thread recently, Johan
>     Herland gave a plausible confusing example:
>
>     ...
>       Why
>       ---
>
>       To show that "---" can be part of a commit message. :)

That is all true, but such a commit already is problematic when used
as an input to "am", regardless of where the sign-off goes.  With or
without our change, the intended explanation of "why" part will be
missing from the resulting commit at the receiving end.  The only
difference your change makes is that the resulting truncated log
message at least will have a sign-off, albeit at a place that the
user did not intend to put it.

We could invent a new and more prominent delimiter, teach
"format-patch" to add that between the log and patch if and only if
the log has a three-dashes line in it (with an option to override
that "if and only if" default), and teach "mailsplit" to pay
attention to it.  People who are relying on the fact that a
three-dashes line in the local log message will be stripped off at
the receiving end have to pass that "The commit has three-dash in it
as a cut-mark on purpose; don't add that prominent delimiter" option
when formatting their patches out for submission.

But I somehow think it is not worth the effort.  It is fairly well
established that three-dash lines are cut marks and Johan's example
log message above deliberately violates only to spite itself.  My
knee-jerk advice is that people can just rephrase s/Why/Reason/ and
be done with it.

I dunno.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 24, 2015 at 08:07:49AM -0700, Junio C Hamano wrote:
>
>> Yeah, you actually convinced me reasonably well that it would
>> happen.  I'd never use it myself.  If people want to shoot
>> themselves in the foot, be my guest ;-)
>> 
>> Perhaps we should drop this, and give a shorter synonym to the
>> option?
>
> I'm still on the fence to have the config kick in only for HEAD.

Hmm, I cannot tell offhand if the confusion factor is worth it (I
didn't say "I don't think it is worth it").  I'd imagine that one
common thing to want is to get an overview of what has happened
upstream since the topic one is currently working on forked from it,
i.e. "log --first-parent ..master", for an individual contributor,
and nother is to see what has happened since the last stable point,
i.e. "log --first-parent origin.." or "log --first-parent v1.0..",
for an integrator.  Neither is covered by the "fp when implied HEAD".

When I am playing an individual contributor, I often want to see my
progress with "log -9" or something, only because "log origin.." is
longer to type and I know my topic is not that long as nine commits.
I guess implied first-parent would not hurt that much in that case,
simply because I do not expect too many merges on a topic, but it
feels wrong to default to first-parent traversal there.

So...

> It feels somewhat magical, but at least the config option name makes it
> painfully clear exactly when it would kick in. I dunno. I am happy
> enough for myself to just run "--first-parent" when that is what I want
> to see. Giving it a shorter name would not help much, I think.

I admit I may be minority, but two common things I do everyday are
"log --first-parent v2.5.0-rc0.." and "log --first-parent master..pu";
I could certainly use a short-hand there.

I already have alias for it, so this is not to help me personally,
but "log -FO" to trigger first-parent one-line would make the alias
unnecessary.

> It is not
> the number of characters, but the fact that most people do not _know_
> that --first-parent exists in the first place, or that it would be
> useful in this case.

That is a separate "education" problem.  My suggestion was more
about "I know there is a way already, but it is cumbersome to type".

> I hoped with a config option it might become
> something projects could recommend to their users[1] if the project has a
> matching workflow. But maybe we could also rely on those same projects
> to educate their users.

They could educate their users to use "log -F" just like they could
tell them to say "config log.firstparent true", I would think.  The
risk of the latter is that those who blindly follow the config path
without understanding what is going on will not even realize that
the problem is that they told Git to only follow the first-parent
path, when they do want to see commits on the side branch, let alone
discovering how to countermand it from the command line one shot.

An instruction to use an extra option, on the other hand, makes it
clear that there is a non-default thing going on, which is more
discoverable: "perhaps I can run it without -F?"

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

[Jacob Keller]

On Sat, Jul 25, 2015 at 10:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Fri, Jul 24, 2015 at 08:07:49AM -0700, Junio C Hamano wrote:
>>
>>> Yeah, you actually convinced me reasonably well that it would
>>> happen.  I'd never use it myself.  If people want to shoot
>>> themselves in the foot, be my guest ;-)
>>>
>>> Perhaps we should drop this, and give a shorter synonym to the
>>> option?
>>
>> I'm still on the fence to have the config kick in only for HEAD.
>
> Hmm, I cannot tell offhand if the confusion factor is worth it (I
> didn't say "I don't think it is worth it").  I'd imagine that one
> common thing to want is to get an overview of what has happened
> upstream since the topic one is currently working on forked from it,
> i.e. "log --first-parent ..master", for an individual contributor,
> and nother is to see what has happened since the last stable point,
> i.e. "log --first-parent origin.." or "log --first-parent v1.0..",
> for an integrator.  Neither is covered by the "fp when implied HEAD".
>
> When I am playing an individual contributor, I often want to see my
> progress with "log -9" or something, only because "log origin.." is
> longer to type and I know my topic is not that long as nine commits.
> I guess implied first-parent would not hurt that much in that case,
> simply because I do not expect too many merges on a topic, but it
> feels wrong to default to first-parent traversal there.
>
> So...
>
>> It feels somewhat magical, but at least the config option name makes it
>> painfully clear exactly when it would kick in. I dunno. I am happy
>> enough for myself to just run "--first-parent" when that is what I want
>> to see. Giving it a shorter name would not help much, I think.
>
> I admit I may be minority, but two common things I do everyday are
> "log --first-parent v2.5.0-rc0.." and "log --first-parent master..pu";
> I could certainly use a short-hand there.
>
> I already have alias for it, so this is not to help me personally,
> but "log -FO" to trigger first-parent one-line would make the alias
> unnecessary.
>
>> It is not
>> the number of characters, but the fact that most people do not _know_
>> that --first-parent exists in the first place, or that it would be
>> useful in this case.
>
> That is a separate "education" problem.  My suggestion was more
> about "I know there is a way already, but it is cumbersome to type".
>
>> I hoped with a config option it might become
>> something projects could recommend to their users[1] if the project has a
>> matching workflow. But maybe we could also rely on those same projects
>> to educate their users.
>
> They could educate their users to use "log -F" just like they could
> tell them to say "config log.firstparent true", I would think.  The
> risk of the latter is that those who blindly follow the config path
> without understanding what is going on will not even realize that
> the problem is that they told Git to only follow the first-parent
> path, when they do want to see commits on the side branch, let alone
> discovering how to countermand it from the command line one shot.
>
> An instruction to use an extra option, on the other hand, makes it
> clear that there is a non-default thing going on, which is more
> discoverable: "perhaps I can run it without -F?"

I like the idea of a shorthand more than the configuration simply
because it doesn't make sense (to me) to always be enabled, and it is
much easier to do so without realizing that you've done it.

Regards,
Jake

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

[Jeff King]

On Sat, Jul 25, 2015 at 10:18:45AM -0700, Junio C Hamano wrote:

> > [1] While reading the old "git commit --notes" thread recently, Johan
> >     Herland gave a plausible confusing example:
> >
> >     ...
> >       Why
> >       ---
> >
> >       To show that "---" can be part of a commit message. :)
> 
> That is all true, but such a commit already is problematic when used
> as an input to "am", regardless of where the sign-off goes.

Right, and that is why I think there is no problem at all with treating
"---" specially as part of format-patch. What I was trying to say here
is that doing it for "git commit" is less obviously OK. Many people do
not use "am" at all, and are otherwise fine with a message like the one
above (tools like rebase used to eat their message, but I think that was
fixed long ago).

> We could invent a new and more prominent delimiter, teach
> "format-patch" to add that between the log and patch if and only if
> the log has a three-dashes line in it (with an option to override
> that "if and only if" default), and teach "mailsplit" to pay
> attention to it.  People who are relying on the fact that a
> three-dashes line in the local log message will be stripped off at
> the receiving end have to pass that "The commit has three-dash in it
> as a cut-mark on purpose; don't add that prominent delimiter" option
> when formatting their patches out for submission.
> 
> But I somehow think it is not worth the effort.  It is fairly well
> established that three-dash lines are cut marks and Johan's example
> log message above deliberately violates only to spite itself.  My
> knee-jerk advice is that people can just rephrase s/Why/Reason/ and
> be done with it.

Yeah, I agree it is not worth the effort. Three-dash is a totally fine
micro-format for email messages, and I do not see anybody complaining
about it. I just think that people who do not use "am" should not have
to care about it.

-Peff

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

[Jeff King]

On Sat, Jul 25, 2015 at 10:41:21AM -0700, Junio C Hamano wrote:

> > I'm still on the fence to have the config kick in only for HEAD.
> 
> Hmm, I cannot tell offhand if the confusion factor is worth it (I
> didn't say "I don't think it is worth it").
> [...]

I've snipped most of your response because it all seemed pretty
reasonable to me.

At this point I think I am of the opinion that the decision to use
--first-parent is sufficiently nuanced that it the config option is not
really a "drop-in" solution for people, even if their projects follow
the matching workflow. Like you, I am not really _against_ it, but since
nobody in this thread is saying "yes, I would turn that on", that may be
a sign. The patch is out there on the list, and I'd encourage people who
think it might be useful to apply the patch and report back in a few
weeks or months if they find it useful.

We _could_ merge the patch to make that experimentation easier for
users. The downside is we will be stuck supporting the log.firstParent
option forever, but I don't think it is actively _wrong_ to have. Just
possibly useless.

And poor Josh, who so nicely came to the Git table at OSCON and talked
to me about his project's workflow, has now had to put up with a slew of
emails and no applied patch. :)

But maybe this discussion is of some use; it has not been fruitless, as
I think the best answer so far is "encourage awareness and appropriate
use of --first-parent".

> I admit I may be minority, but two common things I do everyday are
> "log --first-parent v2.5.0-rc0.." and "log --first-parent master..pu";
> I could certainly use a short-hand there.
> 
> I already have alias for it, so this is not to help me personally,
> but "log -FO" to trigger first-parent one-line would make the alias
> unnecessary.

I do not have an alias, but I spell it "--fir<TAB>". :)

-Peff