{"thread":{"id":"34974","subject":"[PATCH] builtin/push.c: Add `--notes` option","startedAt":"2013-09-20T12:20:08Z","lastAt":"2013-09-29T11:59:43Z","messageCount":2,"participants":["Rodolphe Belouin","Johan Herland"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"227934","messageId":"1379679608-72475-1-git-send-email-rodolphe.belouin@gmail.com","threadId":"34974","inReplyTo":null,"subject":"[PATCH] builtin/push.c: Add `--notes` option","fromName":"Rodolphe Belouin","fromEmail":"rodolphe.belouin@gmail.com","sentAt":"2013-09-20T12:20:08Z","receivedAt":"2013-09-20T12:20:08Z","isPatch":true,"sender":{"key":"rodolphe.belouin@gmail.com","avatar":null},"body":"Make the user able to call `git push --notes` instead of\n`git push refs/notes/*`\n\nSigned-off-by: Rodolphe Belouin <rodolphe.belouin@gmail.com>\n---\n Documentation/git-push.txt | 9 +++++++--\n builtin/push.c             | 9 +++++++--\n 2 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 9eec740..2502dbd 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -9,7 +9,7 @@ git-push - Update remote refs along with associated objects\n SYNOPSIS\n --------\n [verse]\n-'git push' [--all | --mirror | --tags] [--follow-tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]\n+'git push' [--all | --mirror | --tags | --notes] [--follow-tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]\n \t   [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose] [-u | --set-upstream]\n \t   [--force-with-lease[=<refname>[:<expect>]]]\n \t   [--no-verify] [<repository> [<refspec>...]]\n@@ -30,7 +30,7 @@ current branch is consulted to determine where to push.  If the\n configuration is missing, it defaults to 'origin'.\n \n When the command line does not specify what to push with `<refspec>...`\n-arguments or `--all`, `--mirror`, `--tags` options, the command finds\n+arguments or `--all`, `--mirror`, `--tags`, `--notes` options, the command finds\n the default `<refspec>` by consulting `remote.*.push` configuration,\n and if it is not found, honors `push.default` configuration to decide\n what to push (See gitlink:git-config[1] for the meaning of `push.default`).\n@@ -124,6 +124,11 @@ already exists on the remote side.\n \tfrom the remote but are pointing at commit-ish that are\n \treachable from the refs being pushed.\n \n+--notes::\n+\tAll refs under `refs/notes` are pushed, in\n+\taddition to refspecs explicitly listed on the command\n+\tline.\n+\n --receive-pack=<git-receive-pack>::\n --exec=<git-receive-pack>::\n \tPath to the 'git-receive-pack' program on the remote\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 7b1b66c..c407ccc 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -445,6 +445,7 @@ static int option_parse_recurse_submodules(const struct option *opt,\n int cmd_push(int argc, const char **argv, const char *prefix)\n {\n \tint flags = 0;\n+\tint notes = 0;\n \tint tags = 0;\n \tint rc;\n \tconst char *repo = NULL;\t/* default repository */\n@@ -455,6 +456,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT( 0 , \"mirror\", &flags, N_(\"mirror all refs\"),\n \t\t\t    (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE)),\n \t\tOPT_BOOL( 0, \"delete\", &deleterefs, N_(\"delete refs\")),\n+\t\tOPT_BOOL( 0 , \"notes\", &notes, N_(\"push notes (can't be used with --all or --mirror)\")),\n \t\tOPT_BOOL( 0 , \"tags\", &tags, N_(\"push tags (can't be used with --all or --mirror)\")),\n \t\tOPT_BIT('n' , \"dry-run\", &flags, N_(\"dry run\"), TRANSPORT_PUSH_DRY_RUN),\n \t\tOPT_BIT( 0,  \"porcelain\", &flags, N_(\"machine-readable output\"), TRANSPORT_PUSH_PORCELAIN),\n@@ -484,14 +486,17 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n \n-\tif (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))\n-\t\tdie(_(\"--delete is incompatible with --all, --mirror and --tags\"));\n+\tif (deleterefs && (tags || notes || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))\n+\t\tdie(_(\"--delete is incompatible with --all, --mirror, --tags and --notes\"));\n \tif (deleterefs && argc < 2)\n \t\tdie(_(\"--delete doesn't make sense without any refs\"));\n \n \tif (tags)\n \t\tadd_refspec(\"refs/tags/*\");\n \n+\tif (notes)\n+\t\tadd_refspec(\"refs/notes/*\");\n+\n \tif (argc > 0) {\n \t\trepo = argv[0];\n \t\tset_refspecs(argv + 1, argc - 1);\n-- \n1.8.4.412.g79e46c9.dirty\n"},{"id":"228452","messageId":"CALKQrgcdk8==Qf3z9yuxu=9BBmFJ-8Dj+WZALXoc9j+GAKqEgg@mail.gmail.com","threadId":"34974","inReplyTo":"1379679608-72475-1-git-send-email-rodolphe.belouin@gmail.com","subject":"Re: [PATCH] builtin/push.c: Add `--notes` option","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-09-29T11:59:43Z","receivedAt":"2013-09-29T11:59:43Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Fri, Sep 20, 2013 at 2:20 PM, Rodolphe Belouin\n<rodolphe.belouin@gmail.com> wrote:\n> Make the user able to call `git push --notes` instead of\n> `git push refs/notes/*`\n\nI'm sorry for not replying to this earlier. I'm unsure how much of the\nearlier discussions around pushing and pulling notes you have\nfollowed, but the short story is that (as demontrated by the script in\n[1]) pushing and pulling notes is not as simple as it might initially\nseem.\n\nNotes behave more like branches than tags (in that we expect them to\nchange over time, and we (often) want to collaborate on advancing\nthem). In order to support distributed collaboration on branches, we\nhave remote-tracking branches (remotes/$remote/$branch) and associated\nbehavior in git, which keeps track of the state of other repos, and\nsupports the integration of remote and local changes. That\ninfrastructure does not yet exist for notes.\n\nAs a result, collaborating on notes quickly becomes painful when you\nhave to fetch \"magic\" refspecs and manually orchestrate the\nintegration of remote and local notes changes (as shown in [1]).\n\nThe proposed solution for this is to reorganize the refs/remotes/\nhierarchy to allow for remote-tracking notes in addition to\nremote-tracking branches (and also other kinds of remote-tracking\nrefs). However this reorganization is itself a big project and will\nnot be completed in the short term.\n\nSo, how does this apply to your patch?\n\nThere is (AFAICS) nothing wrong with the patch itself (except maybe\nsome added tests would be nice...), but we must consider its\nimplications and how it guides the use of notes.\n\nFirst, as you state in the commit message, this allows use of a simple\noption instead of a supplying a refspec. And as long as you are the\nonly person pushing notes into that repo (and you only fast-forward),\nthis should work well, and lower the barrier for exchanging notes with\na remote repo.\n\nHowever, as soon as you start collaborating with others, you will need\nto integrate their changes, and that's where current Git stops\nproviding any help: You are left to do the manual integration shown in\n[1]. This integration requires you to know about and juggle refspecs\nquite intimately, and it could even be argued that the simplicity of\n\"git push --notes\" tricks an unsuspecting user into a future manual\nintegration regime that is probably outside most git users' comfort\nzone...\n\nThat said, if there is consensus that \"push --notes\" is valuable in\nthe short term (before the larger remote refs reorg is complete), and\nthat it won't set a user interface precedent that will obviously be\nbroken by said reorg, then I am not opposed to this patch.\n\n\nHope this helps,\n\n...Johan\n\n\n[1]:\n#!/bin/sh\n\nset -e\n\nrm -rf notes_test_area\nmkdir notes_test_area\ncd notes_test_area\n\n# Prepare server with initial note\ngit init server\ncd server\necho foo>foo\ngit add foo\ngit commit -m foo\ngit notes add -m \"Initial note on foo\"\necho bar >>foo\ngit commit -a -m bar\ncd ..\n\n# Clone two clients and transfer notes\ngit clone server clientA\ncd clientA\ngit fetch origin refs/notes/commits:refs/notes/commits\ncd ..\n\ngit clone server clientB\ncd clientB\ngit fetch origin refs/notes/commits:refs/notes/commits\ncd ..\n\n# Add notes in both clients\ncd clientA\ngit notes add -m \"clientA's note on bar\"\ncd ../clientB\ngit notes add -m \"clientB's note on bar\"\ncd ..\n\n# Push notes from clientA\ncd clientA\ngit push origin 'refs/notes/*' # This works\ncd ..\n\n# Push notes from clientB\ncd clientB\ngit push origin 'refs/notes/*' || echo \"This command fails!\"\n# Must merge changes from origin's refs/notes/commits into our own\n# refs/notes/commits, but there is no built-in machinery to do so.\n# Do it manually instead:\ngit fetch origin refs/notes/commits:refs/notes/commits_from_origin\ngit notes merge commits_from_origin || echo \"Oops, conflict!\"\n# Conflict in .git/NOTES_MERGE_WORKTREE/$SHA1. Resolve manually\ncommit=$(git rev-parse HEAD)\necho \"clientA's and clientB's notes on bar\" >\n\".git/NOTES_MERGE_WORKTREE/$commit\"\n# Commit conflict resolution to finalize notes merge\ngit notes merge --commit\n# Remove temporary placeholder for origin's notes\ngit update-ref -d refs/notes/commits_from_origin\n# Now we can finally try to push again\ngit push origin 'refs/notes/*' # This works\ncd ..\n\n# Behold the end result\ncd server\ngit log --graph -p -c refs/notes/commits\ncd ..\n\necho \"done\"\n"}]}