[PATCH] Let format-patch and rebase ignore trivial merges.

[PATCH] Let format-patch and rebase ignore trivial merges.

[Bernhard R. Link]

As git rebase and git format-patch linearize commits,
having the same change in different branches causes in the
best case duplicate patches in the produced series and in the
worst case conflicts. If there are trivial merges involved
(i.e. merges that do not change the tree), then this patch
will cause git to only look at one branch, thereby avoiding
duplicates and reducing the chance of conflicts.

There are two new options --prune-tree and --no-prune-tree
added.

--prune-tree makes rev-list without paths equivalent to
"git rev-list $options -- ." (or .. or ../.. and so on,
if you are in some subdirectory).
This is the new default for format-patch and rebase

--no-prune-tree deactivates --prune-tree.

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
 Documentation/rev-list-options.txt |   11 +++++++++++
 builtin-log.c                      |    1 +
 git-rebase--interactive.sh         |    1 +
 git-rebase.sh                      |    2 +-
 revision.c                         |   11 ++++++++++-
 revision.h                         |    1 +
 6 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 1f57aed..6c5e90c 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -328,6 +328,17 @@ The following options select the commits to be shown:
 
 	Commits modifying the given <paths> are selected.
 
+--prune-tree::
+
+	No paths is equivalent to the whole tree as path.
+	That means merges with the same tree follow only one parent.
+	(Default for format-patch and rebase).
+
+--no-prune-tree::
+
+	No paths means not doing history simplification based on paths.
+	(Default for everything but format-patch and rebase).
+
 --simplify-by-decoration::
 
 	Commits that are referred by some branch or tag are selected.
diff --git a/builtin-log.c b/builtin-log.c
index 1766349..efc2f40 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -960,6 +960,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.diff = 1;
 	rev.combine_merges = 0;
 	rev.ignore_merges = 1;
+	rev.prune_tree = 1;
 	DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
 
 	rev.subject_prefix = fmt_patch_subject_prefix;
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0bd3bf7..ea23d9b 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -703,6 +703,7 @@ first and then run 'git rebase --continue' again."
 		fi
 		git rev-list $MERGES_OPTION --pretty=oneline --abbrev-commit \
 			--abbrev=7 --reverse --left-right --topo-order \
+			--prune-tree \
 			$REVISIONS | \
 			sed -n "s/^>//p" | while read shortsha1 rest
 		do
diff --git a/git-rebase.sh b/git-rebase.sh
index b121f45..2186619 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -539,7 +539,7 @@ echo "$head_name" > "$dotest/head-name"
 echo "$GIT_QUIET" > "$dotest/quiet"
 
 msgnum=0
-for cmt in `git rev-list --reverse --no-merges "$revisions"`
+for cmt in `git rev-list --reverse --no-merges --prune-tree "$revisions"`
 do
 	msgnum=$(($msgnum + 1))
 	echo "$cmt" > "$dotest/cmt.$msgnum"
diff --git a/revision.c b/revision.c
index a8a3c3a..3350af6 100644
--- a/revision.c
+++ b/revision.c
@@ -1112,6 +1112,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->dense = 1;
 	} else if (!strcmp(arg, "--sparse")) {
 		revs->dense = 0;
+	} else if (!strcmp(arg, "--prune-tree")) {
+		revs->prune_tree = 1;
+	} else if (!strcmp(arg, "--no-prune-tree")) {
+		revs->prune_tree = 0;
 	} else if (!strcmp(arg, "--show-all")) {
 		revs->show_all = 1;
 	} else if (!strcmp(arg, "--remove-empty")) {
@@ -1408,8 +1412,13 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 		}
 	}
 
-	if (prune_data)
+	if (prune_data) {
 		revs->prune_data = get_pathspec(revs->prefix, prune_data);
+	} else if (revs->prune_tree) {
+		/* limit whole tree (limits trivial merges to one side) */
+		static const char *whole_tree[2] = { "", NULL };
+		revs->prune_data = whole_tree;
+	}
 
 	if (revs->def == NULL)
 		revs->def = def;
diff --git a/revision.h b/revision.h
index d368003..d007aaa 100644
--- a/revision.h
+++ b/revision.h
@@ -38,6 +38,7 @@ struct rev_info {
 	/* Traversal flags */
 	unsigned int	dense:1,
 			prune:1,
+			prune_tree:1,
 			no_merges:1,
 			merges_only:1,
 			no_walk:1,

Re: [PATCH] Let format-patch and rebase ignore trivial merges.

[Johannes Sixt]

Please do not set Mail-Followup-To (and use reply-to-all to keep the Cc list).

Bernhard R. Link schrieb:
> --prune-tree makes rev-list without paths equivalent to
> "git rev-list $options -- ." (or .. or ../.. and so on,
> if you are in some subdirectory).
> This is the new default for format-patch and rebase

Why do you need a new option when you can just add "-- ." to the rev-list
invocation?

-- Hannes

Re: [PATCH] Let format-patch and rebase ignore trivial merges.

[Bernhard R. Link]

* Johannes Sixt <j.sixt@viscovery.net> [091216 17:53]:
> Bernhard R. Link schrieb:
> > --prune-tree makes rev-list without paths equivalent to
> > "git rev-list $options -- ." (or .. or ../.. and so on,
> > if you are in some subdirectory).
> > This is the new default for format-patch and rebase
>
> Why do you need a new option when you can just add "-- ." to the rev-list
> invocation?

I want the default for format-patch changed.

For this I think it is easiest to add a new rev_info flag, as otherwise
format-patch would need to duplicate parsing the rev_list options
and either duplicate applying revs->prune_data or changing the argv for
setup_revisions with some special casing of bare repository and non-bare
repository cases.

And if there is that rev_info flag I think it is most logical to make
it accessible from the outside.

That also allows to revert to the old format-patch behaviour,
in case someone uses format-patch not to get some appliable patches but
some differently formated log or for something else I cannot imagine.

And when there is that option, I think it is more robust to use that
in merge -m and merge -i, as "-- ." only does the right thing by chance
because both only work with a non-bare repository and have
cd_to_toplevel.

Hochachtungsvoll,
	Bernhard R. Link
-- 
Please do not CC me if git@vger.kernel.org also gets a copy.

Re: [PATCH] Let format-patch and rebase ignore trivial merges.

[Johannes Sixt]

Bernhard R. Link schrieb:
> * Johannes Sixt <j.sixt@viscovery.net> [091216 17:53]:
>> Bernhard R. Link schrieb:
>>> --prune-tree makes rev-list without paths equivalent to
>>> "git rev-list $options -- ." (or .. or ../.. and so on,
>>> if you are in some subdirectory).
>>> This is the new default for format-patch and rebase
>> Why do you need a new option when you can just add "-- ." to the rev-list
>> invocation?
> 
> I want the default for format-patch changed.

I do not see why format-patch would have to be changed. The case that you
outline (a merge -s ours happened and you want to follow only one parent)
is rare enough and even more rarly will somebody want to apply
format-patch to such a history.

But I guess that you are actually not interested in format-patch per se,
but rather in rebase (which uses format-patch).

> For this I think it is easiest to add a new rev_info flag, as otherwise
> format-patch would need to duplicate parsing the rev_list options
> and either duplicate applying revs->prune_data or changing the argv for
> setup_revisions with some special casing of bare repository and non-bare
> repository cases.

I haven't looked at the code, but wouldn't it be matter of "if we do not
have any pathspec, add '.'" *after* all options are parsed?

> And when there is that option, I think it is more robust to use that
> in merge -m and merge -i, as "-- ." only does the right thing by chance
> because both only work with a non-bare repository and have
> cd_to_toplevel.

git rev-list -- . works in a bare repository, too. If you hard-code "-- ."
in the rev-list invocations in git-rebase[--interactive], then it cannot
be said that this works "by chance" due to cd_to_toplevel.

-- Hannes

Re: [PATCH] Let format-patch and rebase ignore trivial merges.

[Bernhard R. Link]

* Johannes Sixt <j.sixt@viscovery.net> [091217 12:40]:
> > I want the default for format-patch changed.
>
> I do not see why format-patch would have to be changed. The case that you
> outline (a merge -s ours happened and you want to follow only one parent)
> is rare enough

While it is rare, the result format-patch currently produces is quite a
desaster without any need.

> and even more rarly will somebody want to apply format-patch to such a history.
> But I guess that you are actually not interested in format-patch per se,
> but rather in rebase (which uses format-patch).

I'm looking for a nice way to store the history of a patches in a Debian package.
Currently the best way is to use quilt and store the patches in git.
Topgit is quite overkill, git directly preserving history means no way to
export sane patches. And git rebase -i means losing history of previous
states and pullability.

An way to combine those is doing many trivial merges, but that kills
rebase and format-patch. (While the patch exporting for creating the
debian source packages could change to the right directory and give the
proper arguments, needing to remember the extra argument and teaching
anyone else involved how to call it to get what to sent to upstream
is annoying).

> I haven't looked at the code, but wouldn't it be matter of "if we do not
> have any pathspec, add '.'" *after* all options are parsed?

That's what I would say my patch is doing.

> git rev-list -- . works in a bare repository, too. If you hard-code "-- ."
> in the rev-list invocations in git-rebase[--interactive], then it cannot
> be said that this works "by chance" due to cd_to_toplevel.

It works in a bare repository. But it does not work when called from a
subdirectory of the working dir.

The easiest way I see to express generally

git rev-list --prune-tree $args

is

topdir=$(git rev-parse --show-cdup)
if test -z "$topdir" ; then
        topdir=.
fi
set -- $args
while test $# -gt 0 ; do
        if test "x$1" = "x--" ; then
                break
        fi
        shift
done
if test $# -gt 1 ; then
        git rev-list $args
elif test $# -eq 1 ; then
        git rev-list $args $topdir
else
        git rev-list $args -- $topdir
fi

Hochachtungsvoll,
	Bernhard R. Link
-- 
"Never contain programs so few bugs, as when no debugging tools are available!"
	Niklaus Wirth

Re: [PATCH] Let format-patch and rebase ignore trivial merges.

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Please do not set Mail-Followup-To (and use reply-to-all to keep the Cc list).
>
> Bernhard R. Link schrieb:
>> --prune-tree makes rev-list without paths equivalent to
>> "git rev-list $options -- ." (or .. or ../.. and so on,
>> if you are in some subdirectory).
>> This is the new default for format-patch and rebase
>
> Why do you need a new option when you can just add "-- ." to the rev-list
> invocation?

I agree that --[no-]prune-tree options are unnecessary.  The patch to
builtin-log.c, the second hunk to revision.c, and revision.h would be
sufficient and all others should be dropped.  Instead, the shell script
Porcelains can simply add "-- ." at the end of their rev-list invocations.

That way, we don't have to add anything to the documentation either.

But I wonder if it is an indication of something screwy in the workflow,
if a branch that merges others with "-s ours" is where the patches for
upstream submission is taken from with format-patch, or what is rebased
and internally gets its patches extracted with format-patch.

A branch that merges with "-s ours" is typically done so that others can
pull and build against (and "-s ours" is used to cauterize the history of
a bad side branch), and good bits merged into it would also have come from
a different clean branch that is merged into that branch.  It might make
more sense to format-patch that clean branch when preparing for upstream
submission, than the "aggregated mesh of commits" branch with "-s ours"
fix-ups.

On the other hand, a branch that will be rebased to keep up with others is
by definition private, and I don't see a reason to mark with "-s ours" to
cauterize history of an unrelated side branch that tried to do something
similar to what the branch is trying to achieve in that setting.  You can
instead ignore such a side branch and not merge with it.  So I don't know
how a sane history you are going to rebase ends up containing a "-s ours"
merge to begin with.

Re: [PATCH] Let format-patch and rebase ignore trivial merges.

[Bernhard R. Link]

* Junio C Hamano <gitster@pobox.com> [091217 23:44]:
[order of replies changed for the sake of answers]
> On the other hand, a branch that will be rebased to keep up with others is
> by definition private, and I don't see a reason to mark with "-s ours" to
> cauterize history of an unrelated side branch that tried to do something
> similar to what the branch is trying to achieve in that setting.  You can
> instead ignore such a side branch and not merge with it.  So I don't know
> how a sane history you are going to rebase ends up containing a "-s ours"
> merge to begin with.

Think of a team working to prepare a complicated change that is to be
presented as multiple easily reviewable patches.

If you do something like that alone on a single computer, you will
usually have a branch, collect some commits and merge fixes for previous
commits together with rebase -i. If it takes a longer time you also
rebase to upstream from time to time, fixing all the conflicts and so
on. (You can also just collect and hope to still separate them into
different patches at the end, but that usually gets messy in my
experience).
Those rebases will make you lose some history, which you can work around
by having some extra branches with older states. If you work on
different computers, pulling and pushing the current state of the branch
around needs special care as the non-fast-forward needed all the time
might also easily overwrite and newer with an older state (and keeping
track of the older branches is a big mess unless you have one central
repository).
If there are multiple people working on this, things will not get
easier. In this case having the new clean branch containing a trivial merge
with second parent the old history will both allow easy push and pull
and keep the history so one can look at older states.
(see http://marc.info/?l=git&m=125959221911443&w=2)

A special case for this are modifications in Debian packages. The
patches have to be rebased to every new upstream, while at the same
time should always be in a state so they can be sent upstream and
upstream can pick some of them. (And ideally the debian source package
does include the patches as nice topic separated patch files, so other
distributions/users can easily pick those independent of what vcs they
use).

> A branch that merges with "-s ours" is typically done so that others can
> pull and build against (and "-s ours" is used to cauterize the history of
> a bad side branch), and good bits merged into it would also have come from
> a different clean branch that is merged into that branch.  It might make
> more sense to format-patch that clean branch when preparing for upstream
> submission, than the "aggregated mesh of commits" branch with "-s ours"
> fix-ups.

format-patch has to choose a parent. Choosing the first one make the
most sense for me (as the first is the only real 'special' one).
In the workflows I envision the first parent would also be the one with
the clean history.

Hochachtungsvoll,
	Bernhard R. Link
-- 
"Never contain programs so few bugs, as when no debugging tools are available!"
	Niklaus Wirth

Re: [PATCH] Let format-patch and rebase ignore trivial merges.

[Johannes Sixt]

Please do not cull Cc list.

Bernhard R. Link schrieb:
> format-patch has to choose a parent. Choosing the first one make the
> most sense for me (as the first is the only real 'special' one).
> In the workflows I envision the first parent would also be the one with
> the clean history.

Then use

   git format-patch --first-parent upstream..

-- Hannes

Re: [PATCH] Let format-patch and rebase ignore trivial merges.

[Bernhard R. Link]

* Johannes Sixt <j.sixt@viscovery.net> [091218 14:22]:
> > format-patch has to choose a parent. Choosing the first one make the
> > most sense for me (as the first is the only real 'special' one).
> > In the workflows I envision the first parent would also be the one with
> > the clean history.
>
> Then use
>
>    git format-patch --first-parent upstream..

As already described in the thread my mail contained a link to, this
will miss patches if there were also real merges (which there will).

But the point that there is only a --first-parent and no --last-parent
shows that the first parent is special, so format-patch should choose
the first one.

Hochachtungsvoll,
	Bernhard R. Link
-- 
Please do not CC me if also sending to git@vger.kernel.org.

[PATCH v2] Let format-patch and rebase ignore trivial merges.

[Bernhard R. Link]

As git rebase and git format-patch linearize commits,
having the same change in different branches causes in the
best case duplicate patches in the produced series and in the
worst case conflicts. If there are trivial merges involved
(i.e. merges that do not change the tree), then this patch
will cause git to only look at one branch, thereby avoiding
duplicates and reducing the chance of conflicts.

Signed-off-by: Bernhard R. Link <brlink@debian.org>
---
 builtin-log.c              |    1 +
 git-rebase--interactive.sh |    2 +-
 git-rebase.sh              |    2 +-
 revision.c                 |    7 ++++++-
 revision.h                 |    1 +
 5 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 1766349..efc2f40 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -960,6 +960,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.diff = 1;
 	rev.combine_merges = 0;
 	rev.ignore_merges = 1;
+	rev.prune_tree = 1;
 	DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
 
 	rev.subject_prefix = fmt_patch_subject_prefix;
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0bd3bf7..e5c134b 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -703,7 +703,7 @@ first and then run 'git rebase --continue' again."
 		fi
 		git rev-list $MERGES_OPTION --pretty=oneline --abbrev-commit \
 			--abbrev=7 --reverse --left-right --topo-order \
-			$REVISIONS | \
+			$REVISIONS -- . | \
 			sed -n "s/^>//p" | while read shortsha1 rest
 		do
 			if test t != "$PRESERVE_MERGES"
diff --git a/git-rebase.sh b/git-rebase.sh
index b121f45..dab6949 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -539,7 +539,7 @@ echo "$head_name" > "$dotest/head-name"
 echo "$GIT_QUIET" > "$dotest/quiet"
 
 msgnum=0
-for cmt in `git rev-list --reverse --no-merges "$revisions"`
+for cmt in `git rev-list --reverse --no-merges "$revisions" -- .`
 do
 	msgnum=$(($msgnum + 1))
 	echo "$cmt" > "$dotest/cmt.$msgnum"
diff --git a/revision.c b/revision.c
index a8a3c3a..b27b682 100644
--- a/revision.c
+++ b/revision.c
@@ -1408,8 +1408,13 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 		}
 	}
 
-	if (prune_data)
+	if (prune_data) {
 		revs->prune_data = get_pathspec(revs->prefix, prune_data);
+	} else if (revs->prune_tree) {
+		/* limit whole tree (limits trivial merges to one side) */
+		static const char *whole_tree[2] = { "", NULL };
+		revs->prune_data = whole_tree;
+	}
 
 	if (revs->def == NULL)
 		revs->def = def;
diff --git a/revision.h b/revision.h
index d368003..d007aaa 100644
--- a/revision.h
+++ b/revision.h
@@ -38,6 +38,7 @@ struct rev_info {
 	/* Traversal flags */
 	unsigned int	dense:1,
 			prune:1,
+			prune_tree:1,
 			no_merges:1,
 			merges_only:1,
 			no_walk:1,

Re: [PATCH v2] Let format-patch and rebase ignore trivial merges.

[Junio C Hamano]

"Bernhard R. Link" <brlink@debian.org> writes:

[offtopic: weren't you already asked not to try redirecting away direct
responses to you by using M-F-T and wasting time of people who do want to
respond directly to you?  Please don't.]

> As git rebase and git format-patch linearize commits,
> having the same change in different branches causes in the
> best case duplicate patches in the produced series and in the
> worst case conflicts. If there are trivial merges involved
> (i.e. merges that do not change the tree), then this patch
> will cause git to only look at one branch, thereby avoiding
> duplicates and reducing the chance of conflicts.

The patch text itself from the cursory review looks Ok (I haven't thought
things through yet, let alone applying it, though).

One issue in the proposed commit log message above is that "trivial merge"
is an established technical term that means something very different from
"resulting tree of the merge matches exactly one of the parents' tree",
and it needs to be reworded.  In this case it is easy [*1*].  Drop
everything after "If there are ...", and add something like this as a
separate paragraph:

    Avoid outputting duplicate patches by ignoring all other parents when
    the merge result matches exactly one of the parents.

The code comment also needs to be adjusted.

Thanks.

[Footnote]

*1* It is a good habit to acquire to question yourself if you can omit "X"
altogether and just say "Y" after writing "X (i.e. Y)".