threads / patch / 21970

patchLet format-patch and rebase ignore trivial merges.

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

## tl;dr

11 messages between Dec 16, 2009 and Dec 18, 2009. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Bernhard R. Link· Dec 16, 2009, 16:45 UTC · lore

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(-)
Show changes to 6 files +25 −2

Documentation/rev-list-options.txt, builtin-log.c, git-rebase--interactive.sh, git-rebase.sh, revision.c, revision.h

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,
Johannes Sixt· Dec 16, 2009, 16:53 UTC · re: Bernhard R. Link · lore

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

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
Bernhard R. Link· Dec 17, 2009, 09:35 UTC · re: Johannes Sixt · lore

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

* Johannes Sixt <j.sixt@viscovery.net> [091216 17:53]:
Show 8 quoted lines
> 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.
Johannes Sixt· Dec 17, 2009, 11:40 UTC · re: Bernhard R. Link · lore

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

Bernhard R. Link schrieb:
Show 10 quoted lines
> * 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).

Show 5 quoted lines
> 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
Bernhard R. Link· Dec 17, 2009, 21:48 UTC · re: Johannes Sixt · lore

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

* Johannes Sixt <j.sixt@viscovery.net> [091217 12:40]:
Show 5 quoted lines
> > 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
Junio C Hamano· Dec 17, 2009, 22:44 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 10 quoted lines
> 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.

Bernhard R. Link· Dec 18, 2009, 13:06 UTC · re: Junio C Hamano · lore

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

* Junio C Hamano <gitster@pobox.com> [091217 23:44]:
[order of replies changed for the sake of answers]
Show 7 quoted lines
> 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).

Show 7 quoted lines
> 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
Johannes Sixt· Dec 18, 2009, 13:22 UTC · re: Bernhard R. Link · lore

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

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
Bernhard R. Link· Dec 18, 2009, 14:47 UTC · re: Johannes Sixt · lore

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

* Johannes Sixt <j.sixt@viscovery.net> [091218 14:22]:
Show 8 quoted lines
> > 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.
Bernhard R. Link· Dec 18, 2009, 15:11 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 5 files +10 −3

builtin-log.c, git-rebase--interactive.sh, git-rebase.sh, revision.c, revision.h

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,
Junio C Hamano· Dec 18, 2009, 18:23 UTC · re: Bernhard R. Link · lore

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

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

Show 7 quoted lines
> 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)".

← back to recent threads