[PATCH] builtin/push.c: Add `--notes` option

[PATCH] builtin/push.c: Add `--notes` option

[Rodolphe Belouin]

Make the user able to call `git push --notes` instead of
`git push refs/notes/*`

Signed-off-by: Rodolphe Belouin <rodolphe.belouin@gmail.com>
---
 Documentation/git-push.txt | 9 +++++++--
 builtin/push.c             | 9 +++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 9eec740..2502dbd 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -9,7 +9,7 @@ git-push - Update remote refs along with associated objects
 SYNOPSIS
 --------
 [verse]
-'git push' [--all | --mirror | --tags] [--follow-tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
+'git push' [--all | --mirror | --tags | --notes] [--follow-tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
 	   [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose] [-u | --set-upstream]
 	   [--force-with-lease[=<refname>[:<expect>]]]
 	   [--no-verify] [<repository> [<refspec>...]]
@@ -30,7 +30,7 @@ current branch is consulted to determine where to push.  If the
 configuration is missing, it defaults to 'origin'.
 
 When the command line does not specify what to push with `<refspec>...`
-arguments or `--all`, `--mirror`, `--tags` options, the command finds
+arguments or `--all`, `--mirror`, `--tags`, `--notes` options, the command finds
 the default `<refspec>` by consulting `remote.*.push` configuration,
 and if it is not found, honors `push.default` configuration to decide
 what to push (See gitlink:git-config[1] for the meaning of `push.default`).
@@ -124,6 +124,11 @@ already exists on the remote side.
 	from the remote but are pointing at commit-ish that are
 	reachable from the refs being pushed.
 
+--notes::
+	All refs under `refs/notes` are pushed, in
+	addition to refspecs explicitly listed on the command
+	line.
+
 --receive-pack=<git-receive-pack>::
 --exec=<git-receive-pack>::
 	Path to the 'git-receive-pack' program on the remote
diff --git a/builtin/push.c b/builtin/push.c
index 7b1b66c..c407ccc 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -445,6 +445,7 @@ static int option_parse_recurse_submodules(const struct option *opt,
 int cmd_push(int argc, const char **argv, const char *prefix)
 {
 	int flags = 0;
+	int notes = 0;
 	int tags = 0;
 	int rc;
 	const char *repo = NULL;	/* default repository */
@@ -455,6 +456,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT( 0 , "mirror", &flags, N_("mirror all refs"),
 			    (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE)),
 		OPT_BOOL( 0, "delete", &deleterefs, N_("delete refs")),
+		OPT_BOOL( 0 , "notes", &notes, N_("push notes (can't be used with --all or --mirror)")),
 		OPT_BOOL( 0 , "tags", &tags, N_("push tags (can't be used with --all or --mirror)")),
 		OPT_BIT('n' , "dry-run", &flags, N_("dry run"), TRANSPORT_PUSH_DRY_RUN),
 		OPT_BIT( 0,  "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
@@ -484,14 +486,17 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 
-	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
-		die(_("--delete is incompatible with --all, --mirror and --tags"));
+	if (deleterefs && (tags || notes || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
+		die(_("--delete is incompatible with --all, --mirror, --tags and --notes"));
 	if (deleterefs && argc < 2)
 		die(_("--delete doesn't make sense without any refs"));
 
 	if (tags)
 		add_refspec("refs/tags/*");
 
+	if (notes)
+		add_refspec("refs/notes/*");
+
 	if (argc > 0) {
 		repo = argv[0];
 		set_refspecs(argv + 1, argc - 1);
-- 
1.8.4.412.g79e46c9.dirty

Re: [PATCH] builtin/push.c: Add `--notes` option

[Johan Herland]

On Fri, Sep 20, 2013 at 2:20 PM, Rodolphe Belouin
<rodolphe.belouin@gmail.com> wrote:
> Make the user able to call `git push --notes` instead of
> `git push refs/notes/*`

I'm sorry for not replying to this earlier. I'm unsure how much of the
earlier discussions around pushing and pulling notes you have
followed, but the short story is that (as demontrated by the script in
[1]) pushing and pulling notes is not as simple as it might initially
seem.

Notes behave more like branches than tags (in that we expect them to
change over time, and we (often) want to collaborate on advancing
them). In order to support distributed collaboration on branches, we
have remote-tracking branches (remotes/$remote/$branch) and associated
behavior in git, which keeps track of the state of other repos, and
supports the integration of remote and local changes. That
infrastructure does not yet exist for notes.

As a result, collaborating on notes quickly becomes painful when you
have to fetch "magic" refspecs and manually orchestrate the
integration of remote and local notes changes (as shown in [1]).

The proposed solution for this is to reorganize the refs/remotes/
hierarchy to allow for remote-tracking notes in addition to
remote-tracking branches (and also other kinds of remote-tracking
refs). However this reorganization is itself a big project and will
not be completed in the short term.

So, how does this apply to your patch?

There is (AFAICS) nothing wrong with the patch itself (except maybe
some added tests would be nice...), but we must consider its
implications and how it guides the use of notes.

First, as you state in the commit message, this allows use of a simple
option instead of a supplying a refspec. And as long as you are the
only person pushing notes into that repo (and you only fast-forward),
this should work well, and lower the barrier for exchanging notes with
a remote repo.

However, as soon as you start collaborating with others, you will need
to integrate their changes, and that's where current Git stops
providing any help: You are left to do the manual integration shown in
[1]. This integration requires you to know about and juggle refspecs
quite intimately, and it could even be argued that the simplicity of
"git push --notes" tricks an unsuspecting user into a future manual
integration regime that is probably outside most git users' comfort
zone...

That said, if there is consensus that "push --notes" is valuable in
the short term (before the larger remote refs reorg is complete), and
that it won't set a user interface precedent that will obviously be
broken by said reorg, then I am not opposed to this patch.


Hope this helps,

...Johan


[1]:
#!/bin/sh

set -e

rm -rf notes_test_area
mkdir notes_test_area
cd notes_test_area

# Prepare server with initial note
git init server
cd server
echo foo>foo
git add foo
git commit -m foo
git notes add -m "Initial note on foo"
echo bar >>foo
git commit -a -m bar
cd ..

# Clone two clients and transfer notes
git clone server clientA
cd clientA
git fetch origin refs/notes/commits:refs/notes/commits
cd ..

git clone server clientB
cd clientB
git fetch origin refs/notes/commits:refs/notes/commits
cd ..

# Add notes in both clients
cd clientA
git notes add -m "clientA's note on bar"
cd ../clientB
git notes add -m "clientB's note on bar"
cd ..

# Push notes from clientA
cd clientA
git push origin 'refs/notes/*' # This works
cd ..

# Push notes from clientB
cd clientB
git push origin 'refs/notes/*' || echo "This command fails!"
# Must merge changes from origin's refs/notes/commits into our own
# refs/notes/commits, but there is no built-in machinery to do so.
# Do it manually instead:
git fetch origin refs/notes/commits:refs/notes/commits_from_origin
git notes merge commits_from_origin || echo "Oops, conflict!"
# Conflict in .git/NOTES_MERGE_WORKTREE/$SHA1. Resolve manually
commit=$(git rev-parse HEAD)
echo "clientA's and clientB's notes on bar" >
".git/NOTES_MERGE_WORKTREE/$commit"
# Commit conflict resolution to finalize notes merge
git notes merge --commit
# Remove temporary placeholder for origin's notes
git update-ref -d refs/notes/commits_from_origin
# Now we can finally try to push again
git push origin 'refs/notes/*' # This works
cd ..

# Behold the end result
cd server
git log --graph -p -c refs/notes/commits
cd ..

echo "done"