login
Search
Hi, This is the 2nd iteration of the 'git notes merge' patch series. Changes between the 1st WIP/RFC iteration and this version: - Rebased on top of e93487d which obsoletes patch #7 in the previous series. AFAICS, it now merges cleanly against "master", and almost cleanly (modulo one trivial conflict) against "next". If desired, I can rebase the series onto "next" to avoid the trivial conflict. - (Jonathan Nieder) Future-proof by always checking add_note() return value. - (Stephen Boyd) Simplify argc logic. - (Stephen Boyd) Use test_commit. - (Stephen Boyd) Use "automatically resolves" instead of "auto-resolves". - Renamed -X/--resolve to -s/--strategy. This is more in line with 'git merge' (although the available strategies are different), and allows us to use -X for strategy-specific options in the future. - Documentation: Moved documentation of notes merge strategies into its own section. - Implemented manual conflict resolution of notes. Conflicting note entries are stored in .git/NOTES_MERGE_WORKTREE/*, while the non-conflicting note entries are committed, and referenced by .git/NOTES_MERGE_PARTIAL (aka. the partial merge result). When the conflicts have been edited/fixed, the user calls 'git notes merge --commit' to recombine the partial merge result and the resolved conflicts to produce the final notes merge result. Alternatively, the user can call 'git notes merge --reset' to abort the notes merge. - Add another auto-resolving notes merge strategy: "cat_sort_uniq" - Test merging of notes trees at different fanout levels To be done in later series: - Handle merging of notes trees with non-notes - Allow notes merge strategies to be specified (per (group of?) notes ref) in the config Some open questions: - Should we refuse to finalize a notes merge when conflict markers present in .git/NOTES_MERGE_WORKTREE? Since add/commit in regular merges does NOT do this, I have not implemented it for notes ...
This continuation of the 'git notes merge' implementation teaches notes-merge
to properly do real merges between notes trees: Two diffs are performed, one
from $base to $remote, and another from $base to $local. The paths in each
diff are normalized to SHA1 object names. The two diffs are then consolidated
into a single list of change pairs to be evaluated. Each change pair consist
of:
- The annotated object's SHA1
- The $base SHA1 (i.e. the common ancestor notes for this object)
- The $local SHA1 (i.e. the current notes for this object)
- The $remote SHA1 (i.e. the to-be-merged notes for this object)
From the pair ($base -> $local, $base -> $remote), we can determine the merge
result using regular 3-way rules. If conflicts are encountered in this
process, we fail loudly and exit (conflict handling to be added in a future
patch), If we can complete the merge without conflicts, the resulting
notes tree is committed, and the current notes ref updated.
The patch includes added testcases verifying that we can successfully do real
conflict-less merges.
This patch has been improved by the following contributions:
- Jonathan Nieder: Future-proof by always checking add_note() return value
- Stephen Boyd: Use test_commit
Cc: Jonathan Nieder <jrnieder@gmail.com>
Cc: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
builtin/notes.c | 15 ++-
notes-merge.c | 325 +++++++++++++++++++++++++++++++++++++++++++++++-
notes-merge.h | 15 ++-
t/t3308-notes-merge.sh | 188 ++++++++++++++++++++++++++++
4 files changed, 532 insertions(+), 11 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 717548d..82d2ffe 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -765,6 +765,7 @@ static int merge(int argc, const char **argv, const char *prefix)
{
struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
unsigned char result_sha1[20];
+ struct notes_tree *t;
struct notes_merge_options ...Don't use C99 comments. --
Thanks. Fix will be in the next iteration. ...Johan -- Johan Herland, <johan@herland.net> www.herland.net --
When the notes merge conflicts in .git/NOTES_MERGE_WORKTREE have been resolved, we need to record a new notes commit on the appropriate notes ref with the resolved notes. This patch implements 'git notes merge --commit' which the user should run after resolving conflicts in the notes merge worktree. This command finalizes the notes merge by recombining the partial notes tree from part 1 with the now-resolved conflicts in the notes merge worktree in a merge commit, and updating the appropriate ref to this merge commit. In order to correctly finalize the merge, we need to keep track of three things: - The partial merge result from part 1, containing the auto-merged notes. This is now stored into a ref called .git/NOTES_MERGE_PARTIAL. - The unmerged notes. These are already stored in .git/NOTES_MERGE_WORKTREE, thanks to part 1. - The notes ref to be updated by the finalized merge result. This is now stored in a symref called .git/NOTES_MERGE_REF. In addition to "git notes merge --commit", which uses the above details to create the finalized notes merge commit, this patch also implements "git notes merge --reset", which aborts the ongoing notes merge by simply removing the files/directory described above. FTR, "git notes merge --commit" reuses "git notes merge --reset" to remove the information described above (.git/NOTES_MERGE_*) after the notes merge have been successfully finalized. The patch also contains documentation and testcases for the two new options. Signed-off-by: Johan Herland <johan@herland.net> --- Documentation/git-notes.txt | 23 +++++ builtin/notes.c | 107 +++++++++++++++++++- notes-merge.c | 69 +++++++++++++ notes-merge.h | 23 +++++ t/t3310-notes-merge-manual-resolve.sh | 176 +++++++++++++++++++++++++++++++++ 5 files changed, 395 insertions(+), 3 deletions(-) diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt index ...
This "it s ..." sentence doesn't make sense. --
Indeed. Will be fixed in the next iteration. Here's the updated text: --commit:: Finalize an in-progress 'git notes merge'. Use this option when you have resolved the conflicts that 'git notes merge' stored in .git/NOTES_MERGE_WORKTREE. This amends the partial merge commit created by 'git notes merge' (stored in .git/NOTES_MERGE_PARTIAL) by adding the notes in .git/NOTES_MERGE_WORKTREE. The notes ref stored in the .git/NOTES_MERGE_REF symref is updated to the resulting commit. ...Johan -- Johan Herland, <johan@herland.net> www.herland.net --
This new strategy is similar to "concatenate", but in addition to concatenating the two note candidates, this strategy sorts the resulting lines, and removes duplicate lines from the result. This is equivalent to applying the "cat | sort | uniq" shell pipeline to the two note candidates. This strategy is useful if the notes follow a line-based format where one wants to avoid duplicate lines in the merge result. Note that if either of the note candidates contain duplicate lines _prior_ to the merge, these will also be removed by this merge strategy. The patch also contains tests and documentation for the new strategy. Signed-off-by: Johan Herland <johan@herland.net> --- Documentation/git-notes.txt | 12 +++- builtin/notes.c | 8 ++- notes-merge.c | 6 ++ notes-merge.h | 3 +- notes.c | 76 ++++++++++++++++++ notes.h | 1 + t/t3309-notes-merge-auto-resolve.sh | 145 +++++++++++++++++++++++++++++++++++ 7 files changed, 247 insertions(+), 4 deletions(-) diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt index ede7755..874b77a 100644 --- a/Documentation/git-notes.txt +++ b/Documentation/git-notes.txt @@ -155,7 +155,7 @@ OPTIONS --strategy=<strategy>:: When merging notes, resolve notes conflicts using the given strategy. The following strategies are recognized: "manual" - (default), "ours", "theirs" and "union". + (default), "ours", "theirs", "union" and "cat_sort_uniq". See the "NOTES MERGE STRATEGIES" section below for more information on each notes merge strategy. @@ -231,6 +231,16 @@ ref). "union" automatically resolves notes conflicts by concatenating the local and remote versions. +"cat_sort_uniq" is similar to "union", but in addition to concatenating +the local and remote versions, this strategy also sorts the resulting +lines, and removes duplicate lines from the result. This is ...
This patch has been improved by the following contributions: - Stephen Boyd: Use "automatically resolves" instead of "auto-resolves" Cc: Stephen Boyd <bebarino@gmail.com> Signed-off-by: Johan Herland <johan@herland.net> --- Documentation/git-notes.txt | 44 ++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 43 insertions(+), 1 deletions(-) diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt index 2981d8c..b9a061c 100644 --- a/Documentation/git-notes.txt +++ b/Documentation/git-notes.txt @@ -14,6 +14,7 @@ SYNOPSIS 'git notes' append [-F <file> | -m <msg> | (-c | -C) <object>] [<object>] 'git notes' edit [<object>] 'git notes' show [<object>] +'git notes' merge [-v | -q] [-s <strategy> ] <notes_ref> 'git notes' remove [<object>] 'git notes' prune [-n | -v] @@ -83,6 +84,16 @@ edit:: show:: Show the notes for a given object (defaults to HEAD). +merge:: + Merge the given notes ref into the current notes ref. + This will try to merge the changes made by the given + notes ref (called "remote") since the merge-base (if + any) into the current notes ref (called "local"). ++ +If conflicts arise (and a strategy for automatically resolving +conflicting notes (see the -s/--strategy option) is not given, +the merge fails (TODO). + remove:: Remove the notes for a given object (defaults to HEAD). This is equivalent to specifying an empty note message to @@ -133,9 +144,23 @@ OPTIONS Do not remove anything; just report the object names whose notes would be removed. +-s <strategy>:: +--strategy=<strategy>:: + When merging notes, resolve notes conflicts using the given + strategy. The following strategies are recognized: "manual" + (default), "ours", "theirs" and "union". + See the "NOTES MERGE STRATEGIES" section below for more + information on each notes merge strategy. + +-q:: +--quiet:: + When merging notes, operate quietly. + -v:: --verbose:: - Report all object names whose notes are removed. + When merging ...
Unbalanced '(' here. Can you just drop the first one?
--
Done. Will be in next iteration. Thanks, ...Johan -- Johan Herland, <johan@herland.net> www.herland.net --
When manually resolving a notes merge, if the merging ref has moved since
the merge started, we should fail to complete the merge, and alert the user
to what's going on.
This situation may arise if you start a 'git notes merge' which results in
conflicts, and you then update the current notes ref (using for example
'git notes add/copy/amend/edit/remove/prune', 'git update-ref', etc.),
before you get around to resolving the notes conflicts and calling
'git notes merge --commit'.
We detect this situation by comparing the first parent of the partial merge
commit (which was created when the merge started) to the current value of the
merging notes ref (pointed to by the .git/NOTES_MERGE_REF symref).
If we don't fail in this situation, the notes merge commit would overwrite
the updated notes ref, thus losing the changes that happened in the meantime.
The patch includes a testcase verifying that we fail correctly in this
situation.
Signed-off-by: Johan Herland <johan@herland.net>
---
builtin/notes.c | 11 ++++-
t/t3310-notes-merge-manual-resolve.sh | 76 +++++++++++++++++++++++++++++++++
2 files changed, 85 insertions(+), 2 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 961dcbf..27cab4b 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -786,7 +786,7 @@ static int merge_reset(struct notes_merge_options *o)
static int merge_commit(struct notes_merge_options *o)
{
struct strbuf msg = STRBUF_INIT;
- unsigned char sha1[20];
+ unsigned char sha1[20], parent_sha1[20];
struct notes_tree *t;
struct commit *partial;
struct pretty_print_context pretty_ctx;
@@ -803,6 +803,11 @@ static int merge_commit(struct notes_merge_options *o)
else if (parse_commit(partial))
die("Could not parse commit from NOTES_MERGE_PARTIAL.");
+ if (partial->parents)
+ hashcpy(parent_sha1, partial->parents->item->object.sha1);
+ else
+ hashclr(parent_sha1);
+
t = xcalloc(1, sizeof(struct notes_tree));
init_notes(t, ...This initial implementation of 'git notes merge' only handles the trivial
merge cases (i.e. where the merge is either a no-op, or a fast-forward).
The patch includes testcases for these trivial merge cases.
Future patches will extend the functionality of 'git notes merge'.
This patch has been improved by the following contributions:
- Stephen Boyd: Simplify argc logic
- Stephen Boyd: Use test_commit
Cc: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
Makefile | 2 +
builtin/notes.c | 53 ++++++++++++++++++
notes-merge.c | 103 +++++++++++++++++++++++++++++++++++
notes-merge.h | 30 ++++++++++
t/t3308-notes-merge.sh | 139 ++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 327 insertions(+), 0 deletions(-)
create mode 100644 notes-merge.c
create mode 100644 notes-merge.h
create mode 100755 t/t3308-notes-merge.sh
diff --git a/Makefile b/Makefile
index f33648d..14c0ff1 100644
--- a/Makefile
+++ b/Makefile
@@ -503,6 +503,7 @@ LIB_H += mailmap.h
LIB_H += merge-recursive.h
LIB_H += notes.h
LIB_H += notes-cache.h
+LIB_H += notes-merge.h
LIB_H += object.h
LIB_H += pack.h
LIB_H += pack-refs.h
@@ -593,6 +594,7 @@ LIB_OBJS += merge-recursive.o
LIB_OBJS += name-hash.o
LIB_OBJS += notes.o
LIB_OBJS += notes-cache.o
+LIB_OBJS += notes-merge.o
LIB_OBJS += object.o
LIB_OBJS += pack-check.o
LIB_OBJS += pack-refs.o
diff --git a/builtin/notes.c b/builtin/notes.c
index 9c91c59..ec1d042 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -17,6 +17,7 @@
#include "run-command.h"
#include "parse-options.h"
#include "string-list.h"
+#include "notes-merge.h"
static const char * const git_notes_usage[] = {
"git notes [--ref <notes_ref>] [list [<object>]]",
@@ -25,6 +26,7 @@ static const char * const git_notes_usage[] = {
"git notes [--ref <notes_ref>] append [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]",
"git notes [--ref <notes_ref>] ...Mm, sounds like a nice thing to have anyway. Now to digress: some generalities about test scripts. This will probably be very tedious. I'm writing it as groundwork before reviewing your test (since I don't feel on solid ground about this topic at all). More focused thoughts on other aspects of the patch will follow in a separate message. First: See http://thread.gmane.org/gmane.comp.version-control.git/155596/focus=155673 If you don't like what you see, no need to read the rest of this message. :) Motivation: Not being the best of shell script readers (or writers, for that matter), I need all the help from stylistic cues I can get. Old test scripts have a consistent style generally, but newer ones are starting to diverge from one another. A rough test format: #!/bin/sh # # Copyright (c) 2010 ... # test_description='... . ./test-lib.sh . ... constant data function definitions setup } test } test } repeat as necessary test } test_done Test description: test_description = 'One-line description Some words or diagrams describing the invariants (e.g., history) maintained between tests' Constant data: Simple commands to produce test vectors, expected output, and other variables and files that it might be convenient to have unchanged available throughout the test script. Because not guarded by a test assertion, this section would not include any git commands, nor any commands that might fail or write to the terminal. So: this section might use "cat <<\", "mkdir -p", "cp", "cp -R", "mv", "printf", "foo=", "unset", and "export", but not much else. Function definitions: Tests can define functions inside a test_expect_success block, too, but the generally useful functions would go up front with the constant data. Setup: test_expect_success '(setup | set up) ...' ' Commands to set up for the next batch of tests. Can rely on previous setup and can do just ...
In practice, I am afraid it would take a lot more discipline than any of the current test script does to actually preserve the invariants. But if we can arrange that, it would be ideal. For one thing, it would finally make the subtest skip feature of GIT_SKIP_TESTS usable. An obvious way to do so would be Tests: test_expect_success|failure '... some claim...' ' + Commands to establish a known precondition without + depending on the state left by previous steps. Commands to test that claim. - Could do all sorts of things, as long as they do not - disturb the invariants established by the - constant_data and setup sections. ' but if done naively, the time it takes to re-establish a known precondition for each and every test would add up to substantial overhead. --
This alone should already be a nice UI improvement: in the
fast-forward case, with this patch applied, in place of
git fetch . refs/notes/x:refs/notes/commits
one could write
git notes merge x
This reminds me: it would be nice if non-builtin scripts could use
git notes --get-notes-ref $refarg
to learn the configured notes ref. In other words, shouldn't the
default_notes_ref() exposed in patch 2 also be exposed to scripted
callers? If someone else doesn't get around to it, I can mock
something up in the next few days.
Should the verbosities be of enum type?
enum notes_merge_verbosity {
DEFAULT_VERBOSITY = 2,
MAX_VERBOSITY = 5,
etc
I would find it easier to read
if (o->verbosity >= DEFAULT_VERBOSITY)
fprintf(stderr, ...)
Would trace_printf be a good fit for messages like this one? If not,
any idea about how it could be made to fit some day?
(It is especially nice to be able to direct the trace somewhere other
Can an empty ref be distinguished from a missing ref? It would be
nice to error out when breakage is detected.
With a recursive merge of the ancestors, of course. :)
The difficult part is what to do when a merge of ancestors results in
conflicts.
Maybe
o.verbosity += verbosity;
or
o.verbosity = DEFAULT_NOTES_MERGE_VERBOSITY + verbosity;
Thanks for a pleasant read.
--
I agree, but I'd like to name it 'git notes get-ref' instead, to stay There is a patch implementing 'git notes get-ref' in the next iteration Yep, that's the idea. notes-merge.h is really only an extension of Maybe. I've added these constants and used them where it made sense The current version is modeled on the show() and output() functions in merge-recursive.c. I think that works better in this situation. Agreed. The OUTPUT(o, 5, ...) instances should use trace_printf() I'm not sure when you think we should (or shouldn't) error out. FWIW, I've modeled the current code on what 'git merge' does. See [1] for Exactly. I feel notes merge conflicts are confusing enough to the average user, and I'd rather not expose them to resolving conflicts in _recursive_ notes merges... I guess this becomes a discussion of whether we should model notes merges on the 'resolve' merge strategy or the 'recursive' merge strategy. Without having studied each strategy in-depth, I don't know how much "better" 'recursive' is than 'resolve', especially not from the POV of notes merges. Are there features of 'recursive' that are useful to notes merges? (All I know of the effectual differences between 'resolve' and 'recursive' is the hand waving on the 'git merge' man page: "...has been reported to result in fewer merge conflicts without causing mis-merges by tests done on actual merge commits taken from Linux 2.6 kernel development history.") In conclusion, if someone can show that's it's worth implementing recursive notes merging, then I'll gladly do it, but until then I'll Almost. If you look at the notes_merge() docs in notes-merge.h by the end of this series, you'll see the following return values: 0: Merge trivially succeeded in an existing commit (e.g. fast-forward). 1: Merge successfully completes a merge commit (i.e. no conflicts). Thanks for reading! :) ...Johan [1]: Let's analyze the different cases here: 1. local_ref ("our" side of the merge) ...
Hmm --- isn't the point of output() that it indents to the appropriate
level to portray a recursive merge?
Similarly, show() prevents those confusing messages from the internal
merge between ancestors from being printed when the user is not
interested.
But if you think they are important abstractions to maintain, I won't
get_sha1() can return -1 in the following cases:
- starts with :/, regex does not match.
- starts with :, entry not present in index
- in form <rev>:<path>, <path> does not exist in <rev>
- in form <rev>^, <rev> does not exist or that parent does not
exist
- tag being used as commit points to a tree instead
- et c.
Especially if the caller tries
git notes merge 'afjkdlsa^{gobbledeegook'
I would not like the merge to succeed.
So as I see it, there are four cases:
- get_sha1() succeeds and returns a commit ==> merge that rev
- get_sha1() succeeds and returns a non-commit ==> fail
- get_sha1() fails, but resolve_ref() indicates a ref valid
for writing ==> merge empty tree
- get_sha1() fails, invalid refname ==> fail
I think 'resolve' should be good enough for now. We can always add
'recursive' later.
The cases where 'recursive' might help are those in which both sides
made a lot of changes to the same notes. It helps in two ways:
signaling conflicts that might have been otherwise missed and merging
cleanly in cases that might otherwise produce spurious conflicts.
In theory, it makes the result less "arbitrary"; in practice, it seems
to help avoid some conflicts in ugly cases like merging one week's pu
with the next week's pu.
What kind of caller would care about the distinction between 1 and -1
here (just curious)?
Jonathan
--
I see. I still find the OUTPUT() macro more readable, but I've amended
the patch to eliminate the extraneous show() function. Hence, you'll
find this in the next iteration:
#define OUTPUT(o, v, ...) \
do { \
if ((o)->verbosity >= (v)) { \
printf(__VA_ARGS__); \
puts(""); \
} \
Agreed, but I believe that command currently returns:
fatal: Could not parse commit 'refs/notes/afjkdlsa^{gobbledeegook'.
(or using afjkdlsa^{gobbledeegook as "our" side of the merge):
My tests indicate that case 4 does fail (correctly). However I also
agree that this is not at all clear from the current code, so I've
rewritten this code to something that is hopefully more straightforward
(this is the next iteration):
/* Dereference o->local_ref into local_sha1 */
if (!resolve_ref(o->local_ref, local_sha1, 0, NULL))
die("Failed to resolve local notes ref '%s'", o->local_ref);
else if (!check_ref_format(o->local_ref) && is_null_sha1(local_sha1))
local = NULL; /* local_sha1 == null_sha1 indicates unborn ref */
else if (!(local = lookup_commit_reference(local_sha1)))
die("Could not parse local commit %s (%s)",
sha1_to_hex(local_sha1), o->local_ref);
trace_printf("\tlocal commit: %.7s\n", sha1_to_hex(local_sha1));
/* Dereference o->remote_ref into remote_sha1 */
if (get_sha1(o->remote_ref, remote_sha1)) {
/*
* Failed to get remote_sha1. If o->remote_ref looks like an
* unborn ref, perform the merge using an empty notes tree.
*/
if (!check_ref_format(o->remote_ref)) {
hashclr(remote_sha1);
remote = NULL;
}
else
die("Failed to resolve remote notes ref '%s'",
o->remote_ref);
}
else if (!(remote = lookup_commit_reference(remote_sha1)))
die("Could not parse remote commit %s (%s)",
sha1_to_hex(remote_sha1), o->remote_ref);
trace_printf("\tremote commit: %.7s\n", sha1_to_hex(remote_sha1));
Any caller who cares about the merge at all, I'd imagine
If 1 (or 0) the merge is complete, and there's nothing ...expand_notes_ref() is a new function that performs the DWIM transformation
of "foo" -> "refs/notes/foo" where notes refs are expected.
This is done in preparation for future patches which will also need this
DWIM functionality.
Signed-off-by: Johan Herland <johan@herland.net>
---
builtin/notes.c | 17 +++++++++++------
1 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 35f6eb6..9c91c59 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -83,6 +83,16 @@ struct msg_arg {
struct strbuf buf;
};
+static void expand_notes_ref(struct strbuf *sb)
+{
+ if (!prefixcmp(sb->buf, "refs/notes/"))
+ return; /* we're happy */
+ else if (!prefixcmp(sb->buf, "notes/"))
+ strbuf_insert(sb, 0, "refs/", 5);
+ else
+ strbuf_insert(sb, 0, "refs/notes/", 11);
+}
+
static int list_each_note(const unsigned char *object_sha1,
const unsigned char *note_sha1, char *note_path,
void *cb_data)
@@ -839,13 +849,8 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
if (override_notes_ref) {
struct strbuf sb = STRBUF_INIT;
- if (!prefixcmp(override_notes_ref, "refs/notes/"))
- /* we're happy */;
- else if (!prefixcmp(override_notes_ref, "notes/"))
- strbuf_addstr(&sb, "refs/");
- else
- strbuf_addstr(&sb, "refs/notes/");
strbuf_addstr(&sb, override_notes_ref);
+ expand_notes_ref(&sb);
setenv("GIT_NOTES_REF", sb.buf, 1);
strbuf_release(&sb);
}
--
1.7.3.98.g5ad7d9
--
Aside: I'm not sure this is the most convenient rule to use after all.
Example:
$ git log --notes-ref=charon/notes/full
fatal: unrecognized argument: --notes-ref=charon/notes/full
$ git log --show-notes=charon/notes/full
warning: notes ref refs/notes/charon/notes/full is invalid
...
$ git log --show-notes=remotes/charon/notes/full
warning: notes ref refs/notes/remotes/charon/notes/full is invalid
...
$ git log --show-notes=refs/remotes/charon/notes/full -1
commit 16461e8e5fc5b2dbe9176b9a8313c395e1e07304
Merge: c3e5a06 79bd09f
Author: Junio C Hamano <gitster@pobox.com>
Date: Thu Sep 30 16:02:27 2010 -0700
Merge branch 'il/remote-fd-ext' into pu
* il/remote-fd-ext:
fixup! git-remote-fd
Notes (remotes/charon/notes/full):
Pu-Overview:
What's cooking in git.git (Sep 2010, #07; Wed, 29)
$
And now that I think of it, the revision args parser uses its own code
for this...
Regardless, this patch is a step in the right direction imho.
--
FTR, that rule is not introduced by this patch, the patch merely moves it into Indeed, if you keep notes refs outside refs/notes, the current behaviour is not very user-friendly. So far, we've required all notes refs to be within refs/notes. (See for example init_notes_check() in builtin/notes.c, which refuses to work with notes refs outside 'refs/notes/', hence was probably the reason for adding the above code in the first place.) I guess this moves us towards the discussion of how to handle remote notes refs and how to synchronize them on fetch/push. In short, if we want to keep notes refs outside of refs/notes (e.g. in refs/remotes/foo/notes), we need to I assuming you're talking about the revision args parser's DWIMing of "foo" into the first existing ref in the following list: 1. $GIT_DIR/foo 2. refs/foo 3. refs/tags/foo 4. refs/heads/foo 5. refs/remotes/foo 6. refs/remotes/foo/HEAD If we want to reuse this DWIMery, we obviously want a different list of "auto- completions". Maybe something like: 1. refs/notes/foo 2. refs/remote-notes/foo (depends on how we organize remote notes refs) Thanks, I expect future patches to change this code in order to deal with remote notes refs. For the purposes of the current patch series, I will assume that all notes refs live under refs/notes. ...Johan -- Johan Herland, <johan@herland.net> www.herland.net --
Create new function create_notes_commit() which is slightly more general than
commit_notes() (accepts multiple commit parents and does not auto-update the
notes ref). This function will be used by the notes-merge functionality in
future patches.
Also rewrite builtin/notes.c:commit_notes() to reuse this new function.
Signed-off-by: Johan Herland <johan@herland.net>
---
builtin.h | 2 +-
builtin/notes.c | 28 +++++-----------------------
notes-merge.c | 27 +++++++++++++++++++++++++++
notes-merge.h | 14 ++++++++++++++
4 files changed, 47 insertions(+), 24 deletions(-)
diff --git a/builtin.h b/builtin.h
index ed6ee26..908d850 100644
--- a/builtin.h
+++ b/builtin.h
@@ -17,7 +17,7 @@ extern void prune_packed_objects(int);
extern int fmt_merge_msg(int merge_summary, struct strbuf *in,
struct strbuf *out);
extern int fmt_merge_msg_shortlog(struct strbuf *in, struct strbuf *out);
-extern int commit_notes(struct notes_tree *t, const char *msg);
+extern void commit_notes(struct notes_tree *t, const char *msg);
struct notes_rewrite_cfg {
struct notes_tree **trees;
diff --git a/builtin/notes.c b/builtin/notes.c
index ec1d042..717548d 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -288,18 +288,17 @@ static int parse_reedit_arg(const struct option *opt, const char *arg, int unset
return parse_reuse_arg(opt, arg, unset);
}
-int commit_notes(struct notes_tree *t, const char *msg)
+void commit_notes(struct notes_tree *t, const char *msg)
{
- struct commit_list *parent;
- unsigned char tree_sha1[20], prev_commit[20], new_commit[20];
struct strbuf buf = STRBUF_INIT;
+ unsigned char commit_sha1[20];
if (!t)
t = &default_notes_tree;
if (!t->initialized || !t->ref || !*t->ref)
die("Cannot commit uninitialized/unreferenced notes tree");
if (!t->dirty)
- return 0; /* don't have to commit an unchanged tree */
+ return; /* don't have to commit an unchanged tree */
/* Prepare commit message and reflog message */
...This brings notes merge in line with regular merge's behaviour.
Signed-off-by: Johan Herland <johan@herland.net>
---
builtin/notes.c | 2 +-
notes-merge.c | 11 ++++++++++-
notes-merge.h | 2 +-
t/t3310-notes-merge-manual-resolve.sh | 12 ++++++++++++
4 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 8be7d9e..961dcbf 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -902,7 +902,7 @@ static int merge(int argc, const char **argv, const char *prefix)
strbuf_addf(&msg, "notes: Merged notes from %s into %s",
remote_ref.buf, default_notes_ref());
- o.commit_msg = msg.buf + 7; // skip "notes: " prefix
+ strbuf_add(&(o.commit_msg), msg.buf + 7, msg.len - 7); // skip "notes: "
result = notes_merge(&o, t, result_sha1);
diff --git a/notes-merge.c b/notes-merge.c
index cf1f8da..8b34a32 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -7,6 +7,7 @@
#include "dir.h"
#include "notes.h"
#include "notes-merge.h"
+#include "strbuf.h"
struct notes_merge_pair {
unsigned char obj[20], base[20], local[20], remote[20];
@@ -15,6 +16,7 @@ struct notes_merge_pair {
void init_notes_merge_options(struct notes_merge_options *o)
{
memset(o, 0, sizeof(struct notes_merge_options));
+ strbuf_init(&(o->commit_msg), 0);
o->verbosity = 2;
}
@@ -383,6 +385,12 @@ static int merge_one_change_manual(struct notes_merge_options *o,
sha1_to_hex(p->obj), sha1_to_hex(p->base),
sha1_to_hex(p->local), sha1_to_hex(p->remote));
+ /* add "Conflicts:" section to commit message first time through */
+ if (!o->has_worktree)
+ strbuf_addstr(&(o->commit_msg), "\n\nConflicts:\n");
+
+ strbuf_addf(&(o->commit_msg), "\t%s\n", sha1_to_hex(p->obj));
+
OUTPUT(o, 2, "Auto-merging notes for %s", sha1_to_hex(p->obj));
check_notes_merge_worktree(o);
if (is_null_sha1(p->local)) {
@@ -622,12 +630,13 @@ int ...Clearly specify how combine_notes functions are expected to handle null_sha1
in input, and how they may set the result to null_sha1 in order to cause
the removal of a note.
Also document that passing note_sha1 == null_sha1 to add_note() is usually
a no-op, except in cases where combining it with an existing note yields a
new/changed result.
The only functional changes in this patch concern the handling of null_sha1
in notes_tree_insert(). Otherwise the patch consists solely of reordering
functions in notes.c to avoid use-before-declaration, and adding
documentation to notes.h.
Signed-off-by: Johan Herland <johan@herland.net>
---
notes.c | 158 +++++++++++++++++++++++++++++++++-----------------------------
notes.h | 16 ++++++-
2 files changed, 99 insertions(+), 75 deletions(-)
diff --git a/notes.c b/notes.c
index 79fd850..9e92021 100644
--- a/notes.c
+++ b/notes.c
@@ -150,6 +150,79 @@ static struct leaf_node *note_tree_find(struct notes_tree *t,
}
/*
+ * How to consolidate an int_node:
+ * If there are > 1 non-NULL entries, give up and return non-zero.
+ * Otherwise replace the int_node at the given index in the given parent node
+ * with the only entry (or a NULL entry if no entries) from the given tree,
+ * and return 0.
+ */
+static int note_tree_consolidate(struct int_node *tree,
+ struct int_node *parent, unsigned char index)
+{
+ unsigned int i;
+ void *p = NULL;
+
+ assert(tree && parent);
+ assert(CLR_PTR_TYPE(parent->a[index]) == tree);
+
+ for (i = 0; i < 16; i++) {
+ if (GET_PTR_TYPE(tree->a[i]) != PTR_TYPE_NULL) {
+ if (p) /* more than one entry */
+ return -2;
+ p = tree->a[i];
+ }
+ }
+
+ /* replace tree with p in parent[index] */
+ parent->a[index] = p;
+ free(tree);
+ return 0;
+}
+
+/*
+ * To remove a leaf_node:
+ * Search to the tree location appropriate for the given leaf_node's key:
+ * - If location does not hold a matching entry, abort and do nothing.
+ * - Replace the matching leaf_node with a NULL entry ...No note present, but the node for one is. This skips insertion of The note-present case, where the combine_notes() function can The more usual no-note-present case. Again, this skips insertion of empty notes. Do I understand correctly that the point of the main point of this patch is to allow combine_notes() functions to request that a note be deleted? If so, it would be nice if the commit message said so. Regardless, for what it's worth, Acked-by: Jonathan Nieder <jrnieder@gmail.com> --
Indeed, that is the main point. I believe the paragraph following the commit subject did indeed explain this, but I will try to clarify this Thanks, ...Johan -- Johan Herland, <johan@herland.net> www.herland.net --
Notes trees may exist at different fanout levels internally. This
implementation detail should not be visible to the user, and it should
certainly not affect the merging of notes tree.
This patch adds testcases verifying the correctness of 'git notes merge'
when merging notes trees at different fanout levels.
Signed-off-by: Johan Herland <johan@herland.net>
---
t/t3311-notes-merge-fanout.sh | 436 +++++++++++++++++++++++++++++++++++++++++
1 files changed, 436 insertions(+), 0 deletions(-)
create mode 100755 t/t3311-notes-merge-fanout.sh
diff --git a/t/t3311-notes-merge-fanout.sh b/t/t3311-notes-merge-fanout.sh
new file mode 100755
index 0000000..d1c7b69
--- /dev/null
+++ b/t/t3311-notes-merge-fanout.sh
@@ -0,0 +1,436 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Johan Herland
+#
+
+test_description='Test notes merging at various fanout levels'
+
+. ./test-lib.sh
+
+verify_notes () {
+ notes_ref="$1"
+ commit="$2"
+ if test -f "expect_notes_$notes_ref"
+ then
+ git -c core.notesRef="refs/notes/$notes_ref" notes |
+ sort >"output_notes_$notes_ref" &&
+ test_cmp "expect_notes_$notes_ref" "output_notes_$notes_ref" ||
+ return 1
+ fi &&
+ git -c core.notesRef="refs/notes/$notes_ref" log --format="%H %s%n%N" \
+ "$commit" >"output_log_$notes_ref" &&
+ test_cmp "expect_log_$notes_ref" "output_log_$notes_ref"
+}
+
+verify_fanout () {
+ notes_ref="$1"
+ # Expect entire notes tree to have a fanout == 1
+ git rev-parse --quiet --verify "refs/notes/$notes_ref" >/dev/null &&
+ git ls-tree -r --name-only "refs/notes/$notes_ref" |
+ while read path
+ do
+ case "$path" in
+ ??/??????????????????????????????????????)
+ : true
+ ;;
+ *)
+ echo "Invalid path \"$path\"" &&
+ return 1
+ ;;
+ esac
+ done
+}
+
+verify_no_fanout () {
+ notes_ref="$1"
+ # Expect entire notes tree to have a fanout == 0
+ git rev-parse --quiet --verify "refs/notes/$notes_ref" >/dev/null &&
+ git ls-tree -r --name-only "refs/notes/$notes_ref" |
+ while read ...The new -s/--strategy command-line option to 'git notes merge' allow the user
to choose how notes merge conflicts should be resolved. There are four valid
strategies to choose from:
1. "manual" (the default): This will let the user manually resolve conflicts.
This option currently fails with an error message. It will be implemented
properly in future patches.
2. "ours": This automatically chooses the local version of a conflict, and
discards the remote version.
3. "theirs": This automatically chooses the remote version of a conflict, and
discards the local version.
4. "union": This automatically resolves the conflict by appending the remote
version to the local version.
The strategies are implemented using the combine_notes_* functions from the
notes.h API.
The patch also includes testcases verifying the correct implementation of
these strategies.
This patch has been improved by the following contributions:
- Jonathan Nieder: Future-proof by always checking add_note() return value
- Stephen Boyd: Use test_commit
Cc: Jonathan Nieder <jrnieder@gmail.com>
Cc: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
builtin/notes.c | 21 ++-
notes-merge.c | 31 ++-
notes-merge.h | 6 +
t/t3309-notes-merge-auto-resolve.sh | 502 +++++++++++++++++++++++++++++++++++
4 files changed, 558 insertions(+), 2 deletions(-)
create mode 100755 t/t3309-notes-merge-auto-resolve.sh
diff --git a/builtin/notes.c b/builtin/notes.c
index 82d2ffe..79290db 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -26,7 +26,7 @@ static const char * const git_notes_usage[] = {
"git notes [--ref <notes_ref>] append [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]",
"git notes [--ref <notes_ref>] edit [<object>]",
"git notes [--ref <notes_ref>] show [<object>]",
- "git notes [--ref <notes_ref>] merge [-v | -q] <notes_ref>",
+ "git notes [--ref <notes_ref>] ...This will say: fatal: confused: combine_notes_overwrite failed Do we actually need the "confused" part? Heh, maybe we need a confused() function? --
No, the message should say "Unknown -s/--strategy: %s". Will be fixed in the Well, combine_notes_overwrite() can only return 0, so if we get a non-zero return here I will certainly be confused, since this should be impossible. Maybe better to drop the check altogether and simply leave add_note(...)? Have fun! :) ...Johan -- Johan Herland, <johan@herland.net> www.herland.net --
Conflicts (that are to be resolved manually) are written into a special- purpose working tree, located at .git/NOTES_MERGE_WORKTREE. Within this directory, conflicting notes entries are stored (with conflict markers produced by ll_merge()) using the SHA1 of the annotated object. The .git/NOTES_MERGE_WORKTREE directory will only contain the _conflicting_ note entries. The non-conflicting note entries (aka. the partial merge result) are stored in 'local_tree', and the SHA1 of the resulting commit is written to 'result_sha1'. The return value from notes_merge() is -1. The user is told to edit the files within the .git/NOTES_MERGE_WORKTREE directory in order to resolve the conflicts. The patch also contains documentation and testcases for the correct setup of .git/NOTES_MERGE_WORKTREE. The next part will recombine the partial notes merge result with the resolved conflicts in .git/NOTES_MERGE_WORKTREE to produce the complete merge result. Signed-off-by: Johan Herland <johan@herland.net> --- Documentation/git-notes.txt | 10 +- builtin/notes.c | 8 +- notes-merge.c | 166 ++++++++++++++++++- notes-merge.h | 11 +- t/t3310-notes-merge-manual-resolve.sh | 292 +++++++++++++++++++++++++++++++++ 5 files changed, 474 insertions(+), 13 deletions(-) create mode 100755 t/t3310-notes-merge-manual-resolve.sh diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt index b9a061c..12fda54 100644 --- a/Documentation/git-notes.txt +++ b/Documentation/git-notes.txt @@ -92,7 +92,9 @@ merge:: + If conflicts arise (and a strategy for automatically resolving conflicting notes (see the -s/--strategy option) is not given, -the merge fails (TODO). +the "manual" resolver is used. This resolver checks out the +conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`), +and instructs the user to manually resolve the conflicts there. remove:: Remove the notes for a given ...
The combine_notes_fn functions uses a non-zero return value to indicate failure. However, this return value was converted to a call to die() in note_tree_insert(). Instead, propagate this return value out to add_note(), and return it from there to enable the caller to handle errors appropriately. This patch has been improved by the following contributions: - Jonathan Nieder: Future-proof by always checking add_note() return value Cc: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Johan Herland <johan@herland.net> --- builtin/notes.c | 11 +++++---- notes-cache.c | 3 +- notes.c | 59 ++++++++++++++++++++++++++++-------------------------- notes.h | 11 +++++++-- 4 files changed, 46 insertions(+), 38 deletions(-) diff --git a/builtin/notes.c b/builtin/notes.c index fbc347c..35f6eb6 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -573,8 +573,8 @@ static int add(int argc, const char **argv, const char *prefix) if (is_null_sha1(new_note)) remove_note(t, object); - else - add_note(t, object, new_note, combine_notes_overwrite); + else if (add_note(t, object, new_note, combine_notes_overwrite)) + die("confused: combine_notes_overwrite failed"); snprintf(logmsg, sizeof(logmsg), "Notes %s by 'git notes %s'", is_null_sha1(new_note) ? "removed" : "added", "add"); @@ -653,7 +653,8 @@ static int copy(int argc, const char **argv, const char *prefix) goto out; } - add_note(t, object, from_note, combine_notes_overwrite); + if (add_note(t, object, from_note, combine_notes_overwrite)) + die("confused: combine_notes_overwrite failed"); commit_notes(t, "Notes added by 'git notes copy'"); out: free_notes(t); @@ -712,8 +713,8 @@ static int append_edit(int argc, const char **argv, const char *prefix) if (is_null_sha1(new_note)) remove_note(t, object); - else - add_note(t, object, new_note, combine_notes_overwrite); + else if (add_note(t, object, new_note, combine_notes_overwrite)) + die("confused: ...
It's always odd deciding what to do in the code paths that "can't happen". Ideally one would want static analyzers to check that, while at the same time subjecting the user to some nice graceful fallback behavior when it happens anyway. fatal: confused: combine_notes_overwrite failed In this case I'm not sure what the message in die() should be, but it seems sane to die with some message. Maybe this should be mentioned in the commit message, though? --
Ok, I've added a paragraph to the commit message mentioning that most of add_note()'s callers now implement the die() previously located in notes_tree_insert(). Will be in the next iteration. Thanks, ...Johan -- Johan Herland, <johan@herland.net> www.herland.net --
Signed-off-by: Johan Herland <johan@herland.net> --- notes.h | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/notes.h b/notes.h index 65fc3a6..c0288b0 100644 --- a/notes.h +++ b/notes.h @@ -104,6 +104,10 @@ const unsigned char *get_note(struct notes_tree *t, * Copy a note from one object to another in the given notes_tree. * * Fails if the to_obj already has a note unless 'force' is true. + * + * IMPORTANT: The changes made by copy_note() to the given notes_tree structure + * are not persistent until a subsequent call to write_notes_tree() returns + * zero. */ int copy_note(struct notes_tree *t, const unsigned char *from_obj, const unsigned char *to_obj, @@ -149,6 +153,7 @@ int copy_note(struct notes_tree *t, * notes tree) from within the callback: * - add_note() * - remove_note() + * - copy_note() * - free_notes() */ typedef int each_note_fn(const unsigned char *object_sha1, -- 1.7.3.98.g5ad7d9 --
This reminds me: I sometimes wish there were a
Documentation/technical/api-notes.txt giving a high-level overview of
the API, something like this to start (warning: formatting probably
broken):
-- 8< --
notes API
=========
So you want to write or access persistent, per-object text? The notes
API might help.
Calling sequence
----------------
The caller:
* Allocates and clears a `struct notes_tree`, then fills it based on a
new or existing notes ref with `init_notes()`.
* Adds, removes, and retrieves notes as desired:
. To add notes: use `write_sha1_file()` to create a blob object
containing the information to be stored, and add it to the
in-core notes tree with `add_note()`.
. Retrieve notes as blob objects with `get_note()`, or as
text with `format_note()`.
. Change which objects a note is attached to with `copy_note()`
and `remove_note()`.
* Can iterate over all notes with `for_each_note()`.
* Can remove notes attached to missing objects with `prune_notes()`.
* (Optionally) makes changes persist with `write_notes_tree()`.
* Frees resources associated to the notes tree with `free_notes()`.
A program like 'git log' that needs to find the notes corresponding
to certain objects in multiple notes trees might instead use the
display notes API. In this case, the caller:
* (Optionally) prepares a `struct display_notes_opt` with settings
about which notes trees to use.
* Initializes the display notes machinery with `init_display_notes()`.
* Retrieves notes for each object of interest with
`format_display_notes()`.
-- >8 --
--
I thought studying the notes.h file and its comments was a sufficient introduction on how to work with notes in code. I'm certainly biased, though (as I obviously already know how to work with notes in code)... If you believe that api-notes.txt is a valuable addition to This looks like a good start. ...Johan -- Johan Herland, <johan@herland.net> www.herland.net --
Yeah, I took most of the explanation from notes.h. But it can be nice to have the high-level "here are the functions you have to call" Thanks. --
Signed-off-by: Johan Herland <johan@herland.net>
---
notes.c | 2 +-
notes.h | 14 ++++++++++++++
2 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/notes.c b/notes.c
index 1978244..79fd850 100644
--- a/notes.c
+++ b/notes.c
@@ -898,7 +898,7 @@ static int notes_display_config(const char *k, const char *v, void *cb)
return 0;
}
-static const char *default_notes_ref(void)
+const char *default_notes_ref(void)
{
const char *notes_ref = NULL;
if (!notes_ref)
diff --git a/notes.h b/notes.h
index c0288b0..20db42f 100644
--- a/notes.h
+++ b/notes.h
@@ -44,6 +44,20 @@ extern struct notes_tree {
} default_notes_tree;
/*
+ * Return the default notes ref.
+ *
+ * The default notes ref is the notes ref that is used when notes_ref == NULL
+ * is passed to init_notes().
+ *
+ * This the first of the following to be defined:
+ * 1. The '--ref' option to 'git notes', if given
+ * 2. The $GIT_NOTES_REF environment variable, if set
+ * 3. The value of the core.notesRef config variable, if set
+ * 4. GIT_NOTES_DEFAULT_REF (i.e. "refs/notes/commits")
+ */
+const char *default_notes_ref(void);
+
+/*
* Flags controlling behaviour of notes tree initialization
*
* Default behaviour is to initialize the notes tree from the tree object
--
1.7.3.98.g5ad7d9
--
The rest of the file uses tabs for indenting. Fix the one function
that doesn't.
Signed-off-by: Johan Herland <johan@herland.net>
---
t/t3303-notes-subtrees.sh | 18 +++++++++---------
1 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/t/t3303-notes-subtrees.sh b/t/t3303-notes-subtrees.sh
index 75ec187..d571708 100755
--- a/t/t3303-notes-subtrees.sh
+++ b/t/t3303-notes-subtrees.sh
@@ -168,15 +168,15 @@ INPUT_END
}
verify_concatenated_notes () {
- git log | grep "^ " > output &&
- i=$number_of_commits &&
- while [ $i -gt 0 ]; do
- echo " commit #$i" &&
- echo " first note for commit #$i" &&
- echo " second note for commit #$i" &&
- i=$(($i-1));
- done > expect &&
- test_cmp expect output
+ git log | grep "^ " > output &&
+ i=$number_of_commits &&
+ while [ $i -gt 0 ]; do
+ echo " commit #$i" &&
+ echo " first note for commit #$i" &&
+ echo " second note for commit #$i" &&
+ i=$(($i-1));
+ done > expect &&
+ test_cmp expect output
}
test_expect_success 'test notes in no fanout concatenated with 2/38-fanout' 'test_concatenated_notes "s|^..|&/|" ""'
--
1.7.3.98.g5ad7d9
--
When using combine_notes_concatenate() to concatenate notes, it currently
ensures exactly one newline character between the given notes. However,
when using builtin/notes.c:create_note() to concatenate notes (e.g. by
'git notes append'), it adds a newline character to the trailing newline
of the preceding notes object, thus resulting in _two_ newlines (aka. a
blank line) separating contents of the two notes.
This patch brings combine_notes_concatenate() into consistency with
builtin/notes.c:create_note(), by ensuring exactly _two_ newline characters
between concatenated notes.
The patch also changes a few notes-related selftests accordingly.
Signed-off-by: Johan Herland <johan@herland.net>
---
notes.c | 7 ++++---
t/t3301-notes.sh | 4 ++++
t/t3303-notes-subtrees.sh | 1 +
t/t3404-rebase-interactive.sh | 1 +
t/t9301-fast-import-notes.sh | 5 +++++
5 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/notes.c b/notes.c
index 4f3d094..28afe2c 100644
--- a/notes.c
+++ b/notes.c
@@ -814,16 +814,17 @@ int combine_notes_concatenate(unsigned char *cur_sha1,
return 0;
}
- /* we will separate the notes by a newline anyway */
+ /* we will separate the notes by two newlines anyway */
if (cur_msg[cur_len - 1] == '\n')
cur_len--;
/* concatenate cur_msg and new_msg into buf */
- buf_len = cur_len + 1 + new_len;
+ buf_len = cur_len + 2 + new_len;
buf = (char *) xmalloc(buf_len);
memcpy(buf, cur_msg, cur_len);
buf[cur_len] = '\n';
- memcpy(buf + cur_len + 1, new_msg, new_len);
+ buf[cur_len + 1] = '\n';
+ memcpy(buf + cur_len + 2, new_msg, new_len);
free(cur_msg);
free(new_msg);
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 1d82f79..4bf4e52 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -955,6 +955,7 @@ Date: Thu Apr 7 15:27:13 2005 -0700
Notes (other):
a fresh note
+$whitespace
another fresh note
EOF
@@ -976,8 +977,11 @@ Date: Thu ...Heya, I'm torn, but I think that since we actually can here, we should call it 'git notes merge --abort'. I know that there's no 'git merge --abort', but IIRC that's for technical reasons only. -- Cheers, Sverre Rabbelier --
Heya, Definitely not both, that would be confusing, and would limit us if we Hmmm, I don't know if that does what the user wants, (I haven't used 'git reset --merge' before), but if it does, that sounds like a good solution. -- Cheers, Sverre Rabbelier --
Yeah, but for consistency's sake I don't want to name it 'git notes From git-merge(1): "If you tried a merge which resulted in complex conflicts and want to start over, you can recover with git reset --merge." AFAICS, there's no better candidate synonym for 'git merge --abort'. ...Johan -- Johan Herland, <johan@herland.net> www.herland.net --
Maybe there _should_ be a 'git merge --abort' (as a synonym to 'git reset --merge')? ...Johan -- Johan Herland, <johan@herland.net> www.herland.net --
