login
Search
As discussed earlier today, this brings back Junio's earlier patch series
that introduced (and then used) a -X option for configuring merge
strategies. My favourite use of this is -Xsubtree=<dir>, which lets you
provide the actual subdir prefix when using the subtree merge strategy.
Avery Pennarun (8):
git-merge-file --ours, --theirs
builtin-merge.c: call exclude_cmds() correctly.
git-merge-recursive-{ours,theirs}
Teach git-merge to pass -X<option> to the backend strategy module
Teach git-pull to pass -X<option> to git-merge
Make "subtree" part more orthogonal to the rest of merge-recursive.
Extend merge-subtree tests to test -Xsubtree=dir.
Document that merge strategies can now take their own options
.gitignore | 2 +
Documentation/git-merge-file.txt | 12 +++++-
Documentation/merge-options.txt | 4 ++
Documentation/merge-strategies.txt | 29 ++++++++++++++-
builtin-checkout.c | 2 +-
builtin-merge-file.c | 5 ++-
builtin-merge-recursive.c | 24 ++++++++++---
builtin-merge.c | 44 +++++++++++++++++++++--
cache.h | 1 +
contrib/examples/git-merge.sh | 3 +-
git-compat-util.h | 1 +
git-pull.sh | 17 ++++++++-
git.c | 2 +
ll-merge.c | 20 +++++-----
ll-merge.h | 2 +-
match-trees.c | 69 +++++++++++++++++++++++++++++++++++-
merge-recursive.c | 35 +++++++++++++++---
merge-recursive.h | 7 +++-
strbuf.c | 9 +++++
t/t6029-merge-subtree.sh | 47 ++++++++++++++++++++++++-
t/t6034-merge-ours-theirs.sh | 64 +++++++++++++++++++++++++++++++++
xdiff/xdiff.h | 7 +++-
xdiff/xmerge.c | 11 +++++-
23 files changed, 377 insertions(+), 40 ...We need to call exclude_cmds() after the loop, not during the loop, because
excluding a command from the array can change the indexes of objects in the
array. The result is that, depending on file ordering, some commands
weren't excluded as they should have been.
Signed-off-by: Avery Pennarun <apenwarr@gmail.com>
---
builtin-merge.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/builtin-merge.c b/builtin-merge.c
index 57eedd4..855cf65 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -107,8 +107,8 @@ static struct strategy *get_strategy(const char *name)
found = 1;
if (!found)
add_cmdname(&not_strategies, ent->name, ent->len);
- exclude_cmds(&main_cmds, &not_strategies);
}
+ exclude_cmds(&main_cmds, &not_strategies);
}
if (!is_in_cmdlist(&main_cmds, name) && !is_in_cmdlist(&other_cmds, name)) {
fprintf(stderr, "Could not find merge strategy '%s'.\n", name);
--
1.6.6.rc0.62.gaccf
--
(Patch originally by Junio Hamano <gitster@pobox.com>.)
Signed-off-by: Avery Pennarun <apenwarr@gmail.com>
---
git-pull.sh | 17 +++++++++++++++--
t/t6034-merge-ours-theirs.sh | 8 ++++++++
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/git-pull.sh b/git-pull.sh
index bfeb4a0..6d961b6 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -18,6 +18,7 @@ test -z "$(git ls-files -u)" ||
strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
log_arg= verbosity=
+merge_args=
curr_branch=$(git symbolic-ref -q HEAD)
curr_branch_short=$(echo "$curr_branch" | sed "s|refs/heads/||")
rebase=$(git config --bool branch.$curr_branch_short.rebase)
@@ -62,6 +63,18 @@ do
esac
strategy_args="${strategy_args}-s $strategy "
;;
+ -X*)
+ case "$#,$1" in
+ 1,-X)
+ usage ;;
+ *,-X)
+ xx="-X $2"
+ shift ;;
+ *,*)
+ xx="$1" ;;
+ esac
+ merge_args="$merge_args$xx "
+ ;;
-r|--r|--re|--reb|--reba|--rebas|--rebase)
rebase=true
;;
@@ -216,7 +229,7 @@ fi
merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit
test true = "$rebase" &&
- exec git-rebase $diffstat $strategy_args --onto $merge_head \
+ exec git-rebase $diffstat $strategy_args $merge_args --onto $merge_head \
${oldremoteref:-$merge_head}
-exec git-merge $diffstat $no_commit $squash $no_ff $ff_only $log_arg $strategy_args \
+exec git-merge $diffstat $no_commit $squash $no_ff $ff_only $log_arg $strategy_args $merge_args \
"$merge_name" HEAD $merge_head $verbosity
diff --git a/t/t6034-merge-ours-theirs.sh b/t/t6034-merge-ours-theirs.sh
index 08c9f79..8ab3d61 100755
--- a/t/t6034-merge-ours-theirs.sh
+++ b/t/t6034-merge-ours-theirs.sh
@@ -53,4 +53,12 @@ test_expect_success 'recursive favouring ours' '
! grep 1 file
'
+test_expect_success 'pull with -X' '
+ git reset --hard master && git pull -s recursive -Xours . side &&
+ git reset --hard master && git pull -s recursive -X ours . side &&
+ git reset --hard master && ...This tests the configurable -Xsubtree feature of merge-recursive. Signed-off-by: Avery Pennarun <apenwarr@gmail.com> --- t/t6029-merge-subtree.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 46 insertions(+), 1 deletions(-) diff --git a/t/t6029-merge-subtree.sh b/t/t6029-merge-subtree.sh index 5bbfa44..3900d9f 100755 --- a/t/t6029-merge-subtree.sh +++ b/t/t6029-merge-subtree.sh @@ -52,6 +52,7 @@ test_expect_success 'initial merge' ' git merge -s ours --no-commit gui/master && git read-tree --prefix=git-gui/ -u gui/master && git commit -m "Merge git-gui as our subdirectory" && + git checkout -b work && git ls-files -s >actual && ( echo "100644 $o1 0 git-gui/git-gui.sh" @@ -65,9 +66,10 @@ test_expect_success 'merge update' ' echo git-gui2 > git-gui.sh && o3=$(git hash-object git-gui.sh) && git add git-gui.sh && + git checkout -b master2 && git commit -m "update git-gui" && cd ../git && - git pull -s subtree gui master && + git pull -s subtree gui master2 && git ls-files -s >actual && ( echo "100644 $o3 0 git-gui/git-gui.sh" @@ -76,4 +78,47 @@ test_expect_success 'merge update' ' test_cmp expected actual ' +test_expect_success 'initial ambiguous subtree' ' + cd ../git && + git reset --hard master && + git checkout -b master2 && + git merge -s ours --no-commit gui/master && + git read-tree --prefix=git-gui2/ -u gui/master && + git commit -m "Merge git-gui2 as our subdirectory" && + git checkout -b work2 && + git ls-files -s >actual && + ( + echo "100644 $o1 0 git-gui/git-gui.sh" + echo "100644 $o1 0 git-gui2/git-gui.sh" + echo "100644 $o2 0 git.c" + ) >expected && + test_cmp expected actual +' + +test_expect_success 'merge using explicit' ' + cd ../git && + git reset --hard master2 && + git pull -Xsubtree=git-gui gui master2 && + git ls-files -s >actual && + ( + echo "100644 $o3 0 git-gui/git-gui.sh" + echo "100644 $o1 0 git-gui2/git-gui.sh" + echo "100644 $o2 0 git.c" + ) >expected ...
Also document the recently added -Xtheirs, -Xours and -Xsubtree[=path]
options to the merge-recursive strategy.
(Patch originally by Junio Hamano <gitster@pobox.com>.)
Signed-off-by: Avery Pennarun <apenwarr@gmail.com>
---
Documentation/merge-options.txt | 4 ++++
Documentation/merge-strategies.txt | 29 ++++++++++++++++++++++++++++-
2 files changed, 32 insertions(+), 1 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index fec3394..95244d2 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -74,3 +74,7 @@ option can be used to override --squash.
-v::
--verbose::
Be verbose.
+
+-X<option>::
+ Pass merge strategy specific option through to the merge
+ strategy.
diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 42910a3..360dd6f 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -1,6 +1,11 @@
MERGE STRATEGIES
----------------
+The merge mechanism ('git-merge' and 'git-pull' commands) allows the
+backend 'merge strategies' to be chosen with `-s` option. Some strategies
+can also take their own options, which can be passed by giving `-X<option>`
+arguments to 'git-merge' and/or 'git-pull'.
+
resolve::
This can only resolve two heads (i.e. the current branch
and another branch you pulled from) using a 3-way merge
@@ -20,6 +25,27 @@ recursive::
Additionally this can detect and handle merges involving
renames. This is the default merge strategy when
pulling or merging one branch.
++
+The 'recursive' strategy can take the following options:
+
+ours;;
+ This option forces conflicting hunks to be auto-resolved cleanly by
+ favoring 'our' version. Changes from the other tree that do not
+ conflict with our side are reflected to the merge result.
++
+This should not be confused with the 'ours' merge strategy, which does not
+even look at what the other tree contains at all. That ...This makes "subtree" more orthogonal to the rest of recursive merge, so
that you can use subtree and ours/theirs features at the same time. For
example, you can now say:
git merge -s subtree -Xtheirs other
to merge with "other" branch while shifting it up or down to match the
shape of the tree of the current branch, and resolving conflicts favoring
the changes "other" branch made over changes made in the current branch.
It also allows the prefix used to shift the trees to be specified using
the "-Xsubtree=$prefix" option. Giving an empty prefix tells the command
to figure out how much to shift trees automatically as we have always
done. "merge -s subtree" is the same as "merge -s recursive -Xsubtree="
(or "merge -s recursive -Xsubtree").
(Patch originally by Junio Hamano <gitster@pobox.com>.)
Signed-off-by: Avery Pennarun <apenwarr@gmail.com>
---
builtin-merge-recursive.c | 6 ++-
builtin-merge.c | 6 ++-
cache.h | 1 +
match-trees.c | 69 ++++++++++++++++++++++++++++++++++++++++++++-
merge-recursive.c | 16 +++++++---
merge-recursive.h | 3 +-
6 files changed, 90 insertions(+), 11 deletions(-)
diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
index 53f8f05..d9404e1 100644
--- a/builtin-merge-recursive.c
+++ b/builtin-merge-recursive.c
@@ -28,7 +28,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
if (argv[0]) {
int namelen = strlen(argv[0]);
if (!suffixcmp(argv[0], "-subtree"))
- o.recursive_variant = MERGE_RECURSIVE_SUBTREE;
+ o.subtree_shift = "";
}
if (argc < 4)
@@ -45,7 +45,9 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
else if (!strcmp(arg+2, "theirs"))
o.recursive_variant = MERGE_RECURSIVE_THEIRS;
else if (!strcmp(arg+2, "subtree"))
- o.recursive_variant = MERGE_RECURSIVE_SUBTREE;
+ o.subtree_shift = "";
+ else if (!prefixcmp(arg+2, ...The block comment with NEEDSWORK should be removed from here. I may have forgotten to in the original one, but that is not an excuse to replicate a bad job ;-) --
You should take the full authorship of this patch without even mentioning This part needs the usual "sq-then-eval" trick; -X subtree="My Programs" on the command line will be split by the shell if you didn't do so. --
This uses the low-level mechanism for "ours" and "theirs" autoresolution introduced by the previous commit to introduce two additional merge strategies, merge-recursive-ours and merge-recursive-theirs. (Patch originally by Junio Hamano <gitster@pobox.com>.) Signed-off-by: Avery Pennarun <apenwarr@gmail.com> --- .gitignore | 2 + Makefile | 3 ++ builtin-checkout.c | 2 +- builtin-merge-recursive.c | 9 ++++-- builtin-merge.c | 4 ++- contrib/examples/git-merge.sh | 3 +- git-compat-util.h | 1 + git.c | 2 + ll-merge.c | 20 +++++++------- ll-merge.h | 2 +- merge-recursive.c | 21 ++++++++++++++- merge-recursive.h | 6 +++- strbuf.c | 9 ++++++ t/t6034-merge-ours-theirs.sh | 56 +++++++++++++++++++++++++++++++++++++++++ 14 files changed, 120 insertions(+), 20 deletions(-) create mode 100755 t/t6034-merge-ours-theirs.sh diff --git a/.gitignore b/.gitignore index ac02a58..87467d6 100644 --- a/.gitignore +++ b/.gitignore @@ -79,6 +79,8 @@ /git-merge-one-file /git-merge-ours /git-merge-recursive +/git-merge-recursive-ours +/git-merge-recursive-theirs /git-merge-resolve /git-merge-subtree /git-mergetool diff --git a/Makefile b/Makefile index 5a0b3d4..f92b375 100644 --- a/Makefile +++ b/Makefile @@ -401,6 +401,8 @@ BUILT_INS += git-format-patch$X BUILT_INS += git-fsck-objects$X BUILT_INS += git-get-tar-commit-id$X BUILT_INS += git-init$X +BUILT_INS += git-merge-recursive-ours$X +BUILT_INS += git-merge-recursive-theirs$X BUILT_INS += git-merge-subtree$X BUILT_INS += git-peek-remote$X BUILT_INS += git-repo-config$X @@ -1909,6 +1911,7 @@ check-docs:: do \ case "$$v" in \ git-merge-octopus | git-merge-ours | git-merge-recursive | \ + git-merge-recursive-ours | git-merge-recursive-theirs | \ git-merge-resolve | ...
Distinguishing slight variation of modes of operation between the vanilla merge-recursive and merge-recursive-ours by the command name may have been an easy way to experiment, but we should bite the bullet and allow backend specific options to be given by the end user. (Patch originally by Junio Hamano <gitster@pobox.com>.) Signed-off-by: Avery Pennarun <apenwarr@gmail.com> --- Makefile | 3 --- builtin-merge-recursive.c | 21 +++++++++++++++------ builtin-merge.c | 40 ++++++++++++++++++++++++++++++++++++---- t/t6034-merge-ours-theirs.sh | 4 ++-- 4 files changed, 53 insertions(+), 15 deletions(-) diff --git a/Makefile b/Makefile index f92b375..5a0b3d4 100644 --- a/Makefile +++ b/Makefile @@ -401,8 +401,6 @@ BUILT_INS += git-format-patch$X BUILT_INS += git-fsck-objects$X BUILT_INS += git-get-tar-commit-id$X BUILT_INS += git-init$X -BUILT_INS += git-merge-recursive-ours$X -BUILT_INS += git-merge-recursive-theirs$X BUILT_INS += git-merge-subtree$X BUILT_INS += git-peek-remote$X BUILT_INS += git-repo-config$X @@ -1911,7 +1909,6 @@ check-docs:: do \ case "$$v" in \ git-merge-octopus | git-merge-ours | git-merge-recursive | \ - git-merge-recursive-ours | git-merge-recursive-theirs | \ git-merge-resolve | git-merge-subtree | \ git-fsck-objects | git-init-db | \ git-?*--?* ) continue ;; \ diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c index f5082da..53f8f05 100644 --- a/builtin-merge-recursive.c +++ b/builtin-merge-recursive.c @@ -29,18 +29,27 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) int namelen = strlen(argv[0]); if (!suffixcmp(argv[0], "-subtree")) o.recursive_variant = MERGE_RECURSIVE_SUBTREE; - else if (!suffixcmp(argv[0], "-ours")) - o.recursive_variant = MERGE_RECURSIVE_OURS; - else if (!suffixcmp(argv[0], "-theirs")) - o.recursive_variant = MERGE_RECURSIVE_THEIRS; } if (argc < 4) usagef("%s <base>... -- ...
You added .gitignore entries for the two programs previously, and are removing them in this patch, but forgot to remove them from .gitignore in this patch. As I already suggested you to, if you squash this to the previous one, it Should "merge --no-extended" silently be ignored, or be diagnosed as an I actually didn't name X for "extended" but more for "external" (to the merge program proper). "--strategy-option" perhaps? --
Two comments. - The original series was done over a few weeks in 'pu' and this intermediate step was done before a better alternative of not using these two extra merge strategies were discovered ("...may have been an easy way to experiment, but we should bite the bullet", in the next patch). As the second round to seriously polish the series for inclusion, it would make much more sense to squash this with the next patch to erase this failed approach that has already been shown as clearly inferiour. - I think we should avoid adding the extra argument to ll_merge_fn() by combining virtual_ancestor and favor into one "flags" parameter. If you do so, we do not have to change the callsites again next time we need to add new optional features that needs only a few bits. I vaguely recall that I did the counterpart of this patch that way exactly for the above reason, but it is more than a year ago, so maybe I didn't do it that way. --
You did do that, in fact, but I had to redo a bunch of the flag stuff anyway since a few other flags had been added in the meantime. I actually tried it both ways (with and without an extra parameter), but I observed that: - There are more lines of code (and more confusion) if you use an all-in-one flags vs. what I did. - Several functions have the same signature with all-in-one flags vs. their current boolean parameter, so the code would compile (and then subtly not work) if I forgot to modify a particular function. - When we go to add a third flag parameter, it wouldn't be any harder to join them together at that time, and because it would *again* modify the function signatures (from two flag params back down to one), the compiler would *again* be able to catch any functions we forgot to adjust. If you think this logic doesn't work, I can redo it with all-in-one flags as you request. Avery --
Think of the "flag" parameter as a mini "struct option". When you add a feature to a function at or near the leaf level of call chains that are potentially deep, you add one element to the option structure, and take advantage of the fact that existing callers put a sane default value in the new field, i.e. 0, by doing a "memset(&opt, 0, sizeof(opt))" already, so that the callsites that do not even have to know about the new feature will keep working the same old way without breakage. You saw this exact pattern in the [1/8] patch in your series to cram new "favor this side" information into an existing parameter. As you mentioned, sometimes changing function signature is preferred to catch semantic differences at compilation time. The report given by the compiler of extra or missing parameter at the call site is a wonderful way to find out that you forgot to convert them to the new semantics of the function. This also helps when there is an in-flight patch that adds a new callsite to the function whose semantics you are changing. The semantic conflict is caught when compiling the result of a merge with a branch with such a patch. It is a trick worth knowing about. The approach however cuts both ways. When you are adding an optional feature that is used only in a very few call sites, the semantic merge conflict resulting from such a function signature change is rarely worth it. As long as you choose the default "no-op" value carefully enough so that existing callers will naturally use it without modification, the old code will work the way they did before the new optional feature was added. In other words, "let's implement this as purely an opt-in feature" is often preferrable over "let's force semantic conflict and compilation failure, just in case existing callsites may also want to trigger this new feature". That is why [1/8] patch in your series uses 0 to mean "don't do the funny 'favor' trick, but just leave the conflicts there". I've queued the series with ...
There's just one bit to add to this: when converting a non-bitfield int into a bitfield, really odd things can happen. That was my main rationale for avoiding the change to bitfield without changing the signature. That said, the amount of code isn't really that big so Since I see you didn't change a couple of things you mentioned in earlier comments (the NEEDSWORK comment and the sq-then-eval trick) do you still want me to respin this series? Thanks, Avery --
The commit still is NEEDSWORK and shouldn't be in 'next' in its current shape. I don't think the topic is 1.6.6 material yet, and we will be in pre-release feature freeze any minute now, so there is no urgency. As I did the sq-then-eval in many places in our Porcelain scripts (and many of them are converted to C and lost the need for the trick), I may get tempted to fix it up when I am bored ;-). But no promises. Thanks. --
Oh, I think you meant the "NEEDSWORK -- we limit to depth 2 when we guess" and that has been with us ever since we added subtree merge, and it is no reason to block the topic. I had the sq-then-eval stuff in mind when I wrote above. Sorry for the confusion. --
I'll interpret that as "no, I should not respin the series because Junio plans to deal with it" :) Do let me know if there's anything I should do to help this advance from pu->next sooner (if they delay is not simply because of the code freeze). Have fun, Avery --
As an independent bugfix, I would prefer this to be made against 'maint' and not as a part of this series. How did you notice it? Can you make a test case out of your experience of noticing this bug in the first place, by the way (I am suspecting that you saw some breakage and chased it in the debugger)? --
The story behind this one is a bit silly, but since you asked: I forgot to add recursive-ours and recursive-theirs to the list of known merge strategies, but was surprised to find that my test for recursive-theirs passed, while recursive-ours didn't. Investigating further, I found that the printed list of "found" strategies included recursive-theirs but not recursive-ours. Turns out that this is because when recursive-ours was being (correctly) removed, that slot in the array was being filled by recursive-theirs, and then immediately i++, which meant that recursive-theirs was never checked for exclusion as it should have been. Of course, after fixing this bug *both* tests were broken, but the correct thing to do was add both strategies to the list, which hides the effect of this bugfix. Since the bug is actually that *too many* strategies are listed instead of too few, it's pretty minor and I doubt it needs to go into maint. Also, I don't know of a way to test it that would be reliable. And I doubt this particular bug will recur anyway. (If it were too *few* strategies listed, I'm guessing it would be caught by any number of other tests.) Suggestions welcome. Thanks, Avery --
Often people want their conflicting merges autoresolved by favouring upstream changes (or their own --- it's the same thing), and hinted to run "git diff --name-only | xargs git checkout MERGE_HEAD --". This is essentially to accept automerge results for the paths that are fully resolved automatically while taking their version of the file in full for paths that have conflicts. This is problematic on two counts. One problem is that this is not exactly what these people want. They usually want to salvage as much automerge result as possible. In particular, they want to keep autoresolved parts in conflicting paths, as well as the paths that are fully autoresolved. This patch teaches two new modes of operation to the lowest-lever merge machinery, xdl_merge(). Instead of leaving the conflicted lines from both sides enclosed in <<<, ===, and >>> markers, you can tell the conflicts to be resolved favouring your side or their side of changes. A larger problem is that this tends to encourage a bad workflow by allowing them to record such a mixed up half-merge result as a full commit without auditing. This commit does not tackle this latter issue. In git, we usually give long enough rope to users with strange wishes as long as the risky features is not on by default. (Patch originally by Junio Hamano <gitster@pobox.com>.) Signed-off-by: Avery Pennarun <apenwarr@gmail.com> --- Documentation/git-merge-file.txt | 12 ++++++++++-- builtin-merge-file.c | 5 ++++- xdiff/xdiff.h | 7 ++++++- xdiff/xmerge.c | 11 +++++++++-- 4 files changed, 29 insertions(+), 6 deletions(-) diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt index 3035373..b9d2276 100644 --- a/Documentation/git-merge-file.txt +++ b/Documentation/git-merge-file.txt @@ -10,7 +10,8 @@ SYNOPSIS -------- [verse] 'git merge-file' [-L <current-name> [-L <base-name> [-L <other-name>]]] - [-p|--stdout] [-q|--quiet] ...
Except for parse-optification, this one is more or less a verbatim copy of
my patch, and I think I probably deserve an in-body "From: " line for this
[PATCH 1/8], [PATCH 6/8] and [PATCH 8/8] to take the full authorship of
This is a bad change. It forces the high-level layer of the resulting
code to be aware that the favor bits are shifted by 4 and it is different
from what the low-level layer expects. If I were porting it to
parse-options, I would have kept OURS = 1 and THEIRS = 2 as the original
patch, and instead did something like:
ret = xdl_merge(mmfs + 1, mmfs + 0, names[0], mmfs + 2, names[2],
- &xpp, merge_level | merge_style, &result);
+ &xpp, XDL_MERGE_FLAGS(merge_level, merge_style, merge_favor), &result);
with an updated definition like this:
#define XDL_MERGE_FLAGS(level, style, favor) ((level)|(style)|((favor)<<4)
--
Could you give an guideline to decide when to take authorship and when to give it to others? The above seems somewhat arbitrary to me. -- Nanako Shiraishi http://ivory.ap.teacup.com/nanako3/ --
It might seem that way to you, as you do not write much C, but I am reasonably sure Avery understands after having worked on the series. Imagine that Avery were an area expert (the subsystem maintainer) on "git merge" and downwards, and somebody who did not know that "merge" has already been rewritten in C, nor some parts of the system have been rewritten to use parse-options, submitted a patch series for review and Avery is helping to polish it up [*1*]. As the subsystem maintainer, Avery looks at the patches, updates parts of the code that is based on obsolete infrastructure, adds lacking tests and documentation as necessary, and forwards the tested result upwards for inclusion. How would the messages from Avery to me look? Patches that were majorly reworked should be attributed to Avery, and obviously new patches that are added for missing tests should be, too. For example, changes made to git-merge.sh by the original submitter must be discarded and redone from scratch to builtin-merge.c, and if you look at the changes, it would be quite obvious that the original patch wouldn't have served as anything more than giving the specification. But the ones with minor updates should retain the original authorship. It unfortunately is not black-and-white, though. In any case, where does Avery's credit go? Is there a point in helping to polish others' patches? It is recoded on the Signed-off-by line. When somebody passes a patch from somebody else, an S-o-b is added for DCO purposes, but it also leaves the "patch trail"---these people looked at the patch, spent effort to make sure it is suitable for inclusion by reviewing, polishing, and enhancing. A subsystem maintainer, or anybody who helps to polish others contribution, may not necessarily have his name as the "author" of the patch, and if the patch forwarding is done via e-mail, his name won't be on the "committer" line either. But the contribution is still noted and appreciated, and the hint to pay attention to is ...
I understand. Thank you for a detailed explanation. -- Nanako Shiraishi http://ivory.ap.teacup.com/nanako3/ --
I'm quite open to doing this however you want; I definitely consider it your patch series. My main measurable contribution is just the unit tests that I wrote. However, when thinking about this, I wasn't worried so much about the correct placement of credit as of discredit. The merge code has changed sufficiently since you wrote this patch series that every one of them required quite a lot of conflict resolution. Most of the conflicts were pretty obvious how to resolve, but it was tedious and error prone, and there's a reasonably high probability that I screwed up something while doing so. I imagined what people would expect to see when they do 'git blame' to explain the source of a problem. If they see your name, you might be blamed for my errors; if they see my name with a "based on a patch by Junio" in the changelog, then I would be (probably correctly) blamed for the errors, while you can retain credit for the success. Mostly, however, I didn't want to be sending out patches in your name that weren't actually done by you. If you'd like me to do so, Ouch, yes, that wasn't very clear thinking on my part. I meant for XDL_MERGE_FAVOR(flags) to return either XDL_MERGE_FAVOR_OURS or XDL_MERGE_FAVOR_THEIRS, but clearly it doesn't. Will fix. Avery --
