{"thread":{"id":"46466","subject":"[PATCH/RFC] contrib: rerere-train overwrites existing resolutions","startedAt":"2017-07-25T14:34:24Z","lastAt":"2017-07-28T18:40:54Z","messageCount":8,"participants":["Raman Gupta","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"325039","messageId":"1caa9bb1-9184-7335-a041-6abd2c8f616c@gmail.com","threadId":"46466","inReplyTo":null,"subject":"[PATCH/RFC] contrib: rerere-train overwrites existing resolutions","fromName":"Raman Gupta","fromEmail":"rocketraman@gmail.com","sentAt":"2017-07-25T14:34:16Z","receivedAt":"2017-07-25T14:34:24Z","isPatch":true,"sender":{"key":"rocketraman@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53049?v=4"},"body":"From 41116889f7eb2ddd590d165d9ca89646f7b15aaf Mon Sep 17 00:00:00 2001\nFrom: Raman Gupta <rocketraman@gmail.com>\nDate: Tue, 25 Jul 2017 10:28:35 -0400\nSubject: [PATCH] contrib: rerere-train overwrites existing resolutions\n\nWhen running rerere-train, the user is explicitly asking the training to\noccur based on the current merge commit. However, if the current cache\nhas a resolution for the same conflict (even if out of date or wrong),\nrerere-train does not currently update the rr-cache. Now, forget\nexisting resolutions before training so that training is always\nreflective of the trained data.\n---\n contrib/rerere-train.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh\nindex 52ad9e4..a222d38 100755\n--- a/contrib/rerere-train.sh\n+++ b/contrib/rerere-train.sh\n@@ -34,6 +34,7 @@ do\n \t\t# Cleanly merges\n \t\tcontinue\n \tfi\n+\tgit rerere forget .\n \tif test -s \"$GIT_DIR/MERGE_RR\"\n \tthen\n \t\tgit show -s --pretty=format:\"Learning from %h %s\" \"$commit\"\n-- \n2.9.4\n\n"},{"id":"325075","messageId":"xmqqwp6wtdu0.fsf@gitster.mtv.corp.google.com","threadId":"46466","inReplyTo":"1caa9bb1-9184-7335-a041-6abd2c8f616c@gmail.com","subject":"Re: [PATCH/RFC] contrib: rerere-train overwrites existing resolutions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-25T21:18:47Z","receivedAt":"2017-07-25T21:19:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Raman Gupta <rocketraman@gmail.com> writes:\n\n> From 41116889f7eb2ddd590d165d9ca89646f7b15aaf Mon Sep 17 00:00:00 2001\n> From: Raman Gupta <rocketraman@gmail.com>\n> Date: Tue, 25 Jul 2017 10:28:35 -0400\n> Subject: [PATCH] contrib: rerere-train overwrites existing resolutions\n\nThese do not belong to the body of the message.\n\nImagine that your title line appears in \"git shortlog --no-merges\"\noutput together with 600 other commits, and you see that list 3\nmonths later.  Can you tell what this change was about?\n\nTo me, it sounds like a user is complaining that it overwrites and\nloses a precious one, implying that the change is to fix it by being\nmore careful, i.e. checking if there is an existing one and avoid\noverwriting it.\n\nAs our convention is to write the log message as if you are giving\nan order to the codebase \"to behave like so\",\n\n\tcontrib/rerere-train: overwrite existing resolutions\n\nwould convey what you are doing better, I would think.\n\nAs to the change itself, I do anticipate that a user who is used to\nrerere-train will complain that the updated one overwrites existing\nresolutions, losing data, as the existing one didn't, and they are\nused to the way to correct an incorrect resolution that is recorded\nearlier, which is:\n\n * perform the botched merge again, which will materialize the\n   botched merge result in the working tree;\n\n * do \"git rerere forget\" for the path;\n\n * correct the path with the right merge result; and then finally\n\n * do \"git rerere\".\n\nIf this new behaviour is triggered only when a command line option\nis given (e.g. \"--overwrite\" which is better than \"--force\"), I\nwould imagine that it might give us an easier way to achieve the\nsame thing without learning about \"rerere forget\".  So I do not\nfundamentally oppose to having such an option, but changing the\ndefault behaviour is not going to fly.\n\n> When running rerere-train, the user is explicitly asking the training to\n> occur based on the current merge commit. However, if the current cache\n> has a resolution for the same conflict (even if out of date or wrong),\n> rerere-train does not currently update the rr-cache. Now, forget\n> existing resolutions before training so that training is always\n> reflective of the trained data.\n> ---\n\nActually, the user is asking to record what is not recorded yet,\nwhich is the originally intended use case.  You may fetch my 'pu'\nbranch into a repository you already have a copy of git.git, and\nwant to grab conflict resolutions I did between master..pu that you\nstill do not know about.  The tool is conservative and avoids\nclobbering your resolution if you already have resolved some of the\nmerges yourself.\n\nIn any case, please make it a habit to sign-off your work when\nposting here.\n\n>  contrib/rerere-train.sh | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh\n> index 52ad9e4..a222d38 100755\n> --- a/contrib/rerere-train.sh\n> +++ b/contrib/rerere-train.sh\n> @@ -34,6 +34,7 @@ do\n>  \t\t# Cleanly merges\n>  \t\tcontinue\n>  \tfi\n> +\tgit rerere forget .\n>  \tif test -s \"$GIT_DIR/MERGE_RR\"\n>  \tthen\n>  \t\tgit show -s --pretty=format:\"Learning from %h %s\" \"$commit\"\n"},{"id":"325098","messageId":"ae4d9b77-c47e-199b-d40f-ad5b49c5dd60@gmail.com","threadId":"46466","inReplyTo":"xmqqwp6wtdu0.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions","fromName":"Raman Gupta","fromEmail":"rocketraman@gmail.com","sentAt":"2017-07-25T22:48:12Z","receivedAt":"2017-07-25T22:48:18Z","isPatch":true,"sender":{"key":"rocketraman@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53049?v=4"},"body":"Provide the user an option to overwrite existing resolutions using an\n`--overwrite` flag. This might be used, for example, if the user knows\nthat they already have an entry in their rerere cache for a conflict,\nbut wish to drop it and retrain based on the merge commit(s) passed to\nthe rerere-train script.\n\nSigned-off-by: Raman Gupta <rocketraman@gmail.com>\n---\n contrib/rerere-train.sh | 36 ++++++++++++++++++++++++++++++++++--\n 1 file changed, 34 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh\nindex 52ad9e4..e25bf8a 100755\n--- a/contrib/rerere-train.sh\n+++ b/contrib/rerere-train.sh\n@@ -3,10 +3,38 @@\n # Prime rerere database from existing merge commits\n \n me=rerere-train\n-USAGE=\"$me rev-list-args\"\n \n SUBDIRECTORY_OK=Yes\n-OPTIONS_SPEC=\n+OPTS_SPEC=\"\\\n+$me [--overwrite] <rev-list-args>\n+--\n+h,help        show the help\n+o,overwrite   overwrite any existing rerere cache\n+\"\n+eval \"$(echo \"$OPTS_SPEC\" | git rev-parse --parseopt -- \"$@\" || echo exit $?)\"\n+\n+overwrite=0\n+\n+while test $# -gt 0\n+do\n+\topt=\"$1\"\n+\tcase \"$opt\" in\n+\t-o)\n+\t\toverwrite=1\n+\t\tshift\n+\t\tshift\n+\t\t;;\n+\t--)\n+\t\tshift\n+\t\tbreak\n+\t\t;;\n+\t*)\n+\t\tbreak\n+\t\texit 1\n+\t\t;;\n+\tesac\n+done\n+\n . \"$(git --exec-path)/git-sh-setup\"\n require_work_tree\n cd_to_toplevel\n@@ -34,6 +62,10 @@ do\n \t\t# Cleanly merges\n \t\tcontinue\n \tfi\n+\tif [ $overwrite == 1 ]\n+\tthen\n+\t\tgit rerere forget .\n+\tfi\n \tif test -s \"$GIT_DIR/MERGE_RR\"\n \tthen\n \t\tgit show -s --pretty=format:\"Learning from %h %s\" \"$commit\"\n-- \n2.9.4\n\n\n"},{"id":"325133","messageId":"xmqqmv7rrs45.fsf@gitster.mtv.corp.google.com","threadId":"46466","inReplyTo":"ae4d9b77-c47e-199b-d40f-ad5b49c5dd60@gmail.com","subject":"Re: [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-26T18:05:30Z","receivedAt":"2017-07-26T18:05:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Raman Gupta <rocketraman@gmail.com> writes:\n\n> Provide the user an option to overwrite existing resolutions using an\n> `--overwrite` flag. This might be used, for example, if the user knows\n> that they already have an entry in their rerere cache for a conflict,\n> but wish to drop it and retrain based on the merge commit(s) passed to\n> the rerere-train script.\n>\n> Signed-off-by: Raman Gupta <rocketraman@gmail.com>\n> ---\n>  contrib/rerere-train.sh | 36 ++++++++++++++++++++++++++++++++++--\n>  1 file changed, 34 insertions(+), 2 deletions(-)\n>\n> diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh\n> index 52ad9e4..e25bf8a 100755\n> --- a/contrib/rerere-train.sh\n> +++ b/contrib/rerere-train.sh\n> @@ -3,10 +3,38 @@\n>  # Prime rerere database from existing merge commits\n>  \n>  me=rerere-train\n> -USAGE=\"$me rev-list-args\"\n>  \n>  SUBDIRECTORY_OK=Yes\n> -OPTIONS_SPEC=\n> +OPTS_SPEC=\"\\\n> +$me [--overwrite] <rev-list-args>\n> +--\n> +h,help        show the help\n> +o,overwrite   overwrite any existing rerere cache\n> +\"\n> +eval \"$(echo \"$OPTS_SPEC\" | git rev-parse --parseopt -- \"$@\" || echo exit $?)\"\n> +\n> +overwrite=0\n\nIt is very good that you initialize overwrite explicitly here, to\nprevent it from seeping through from the caller's environment.\n\n> +while test $# -gt 0\n> +do\n> +\topt=\"$1\"\n> +\tcase \"$opt\" in\n> +\t-o)\n> +\t\toverwrite=1\n> +\t\tshift\n> +\t\tshift\n> +\t\t;;\n> +\t--)\n> +\t\tshift\n> +\t\tbreak\n> +\t\t;;\n> +\t*)\n> +\t\tbreak\n> +\t\texit 1\n> +\t\t;;\n> +\tesac\n> +done\n\nI haven't tried this patch, but would this work well with options\nmeant for the 'git rev-list --parents \"$@\"' that grabs the list of\nmerge commits to learn from?  e.g.\n\n\t$ contrib/rerere-train.sh -n 4 --merges master\n\t$ contrib/rerere-train.sh --overwrite -n 4 --merges master\n\t$ contrib/rerere-train.sh -n 4 --overwrite --merges master\n\nI do not think it is necessary to make the last one work; as long as\nthe first two work as expected, we are good even if the last one\ndies with a sensible message e.g. \"options X, Y and Z must be given\nbefore other options\" (currently \"X, Y and Z\" consists only of\n\"--overwrite\", but I think you get what I mean).\n\n>  . \"$(git --exec-path)/git-sh-setup\"\n>  require_work_tree\n>  cd_to_toplevel\n> @@ -34,6 +62,10 @@ do\n>  \t\t# Cleanly merges\n>  \t\tcontinue\n>  \tfi\n> +\tif [ $overwrite == 1 ]\n\n\tif test \"$overwrite\" = 1\n\ncf. Documentation/CodingGuidelines.\n\n> +\tthen\n> +\t\tgit rerere forget .\n> +\tfi\n>  \tif test -s \"$GIT_DIR/MERGE_RR\"\n>  \tthen\n>  \t\tgit show -s --pretty=format:\"Learning from %h %s\" \"$commit\"\n"},{"id":"325142","messageId":"32b61c4b-5e58-30d9-4345-8036bc805019@gmail.com","threadId":"46466","inReplyTo":"xmqqmv7rrs45.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions","fromName":"Raman Gupta","fromEmail":"rocketraman@gmail.com","sentAt":"2017-07-26T19:06:51Z","receivedAt":"2017-07-26T19:07:00Z","isPatch":true,"sender":{"key":"rocketraman@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53049?v=4"},"body":"On 26/07/17 02:05 PM, Junio C Hamano wrote:\n> I haven't tried this patch, but would this work well with options\n> meant for the 'git rev-list --parents \"$@\"' that grabs the list of\n> merge commits to learn from?  e.g.\n> \n> \t$ contrib/rerere-train.sh -n 4 --merges master\n> \t$ contrib/rerere-train.sh --overwrite -n 4 --merges master\n> \t$ contrib/rerere-train.sh -n 4 --overwrite --merges master\n> \n> I do not think it is necessary to make the last one work; as long as\n> the first two work as expected, we are good even if the last one\n> dies with a sensible message e.g. \"options X, Y and Z must be given\n> before other options\" (currently \"X, Y and Z\" consists only of\n> \"--overwrite\", but I think you get what I mean).\n\nYou're right -- I didn't try all the cases. I wasn't able to figure\nout how to get `rev-parse --parseopt` to deal with this situation, so\nI did it manually. I'm not super-happy with the result, but it does\nwork. Look for PATCH v3.\n\nRegards,\nRaman\n"},{"id":"325143","messageId":"3a11f2f2-9fa1-bd7b-44dc-141c7fff8127@gmail.com","threadId":"46466","inReplyTo":"xmqqmv7rrs45.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v3] contrib/rerere-train: optionally overwrite existing resolutions","fromName":"Raman Gupta","fromEmail":"rocketraman@gmail.com","sentAt":"2017-07-26T19:08:38Z","receivedAt":"2017-07-26T19:08:45Z","isPatch":true,"sender":{"key":"rocketraman@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53049?v=4"},"body":"Provide the user an option to overwrite existing resolutions using an\n`--overwrite` flag. This might be used, for example, if the user knows\nthat they already have an entry in their rerere cache for a conflict,\nbut wish to drop it and retrain based on the merge commit(s) passed to\nthe rerere-train script.\n\nSigned-off-by: Raman Gupta <rocketraman@gmail.com>\n---\n contrib/rerere-train.sh | 54 +++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 52 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh\nindex 52ad9e4..eeee45d 100755\n--- a/contrib/rerere-train.sh\n+++ b/contrib/rerere-train.sh\n@@ -3,10 +3,56 @@\n # Prime rerere database from existing merge commits\n \n me=rerere-train\n-USAGE=\"$me rev-list-args\"\n+USAGE=$(cat <<-EOF\n+usage: $me [--overwrite] <rev-list-args>\n+\n+    -h, --help            show the help\n+    -o, --overwrite       overwrite any existing rerere cache\n+EOF\n+)\n \n SUBDIRECTORY_OK=Yes\n-OPTIONS_SPEC=\n+\n+overwrite=0\n+\n+while test $# -gt 0\n+do\n+\topt=\"$1\"\n+\tcase \"$opt\" in\n+\t-h|--help)\n+\t\techo \"$USAGE\"\n+\t\texit 0\n+\t\t;;\n+\t-o|--overwrite)\n+\t\toverwrite=1\n+\t\tshift\n+\t\tbreak\n+\t\t;;\n+\t--)\n+\t\tshift\n+\t\tbreak\n+\t\t;;\n+\t*)\n+\t\tbreak\n+\t\t;;\n+\tesac\n+done\n+\n+# Overwrite or help options are not valid except as first arg\n+for opt in \"$@\"\n+do\n+\tcase \"$opt\" in\n+\t-h|--help)\n+\t\techo \"$USAGE\"\n+\t\texit 0\n+\t\t;;\n+\t-o|--overwrite)\n+\t\techo \"$USAGE\"\n+\t\texit 0\n+\t\t;;\n+\tesac\n+done\n+\n . \"$(git --exec-path)/git-sh-setup\"\n require_work_tree\n cd_to_toplevel\n@@ -34,6 +80,10 @@ do\n \t\t# Cleanly merges\n \t\tcontinue\n \tfi\n+\tif test $overwrite = 1\n+\tthen\n+\t\tgit rerere forget .\n+\tfi\n \tif test -s \"$GIT_DIR/MERGE_RR\"\n \tthen\n \t\tgit show -s --pretty=format:\"Learning from %h %s\" \"$commit\"\n-- \n2.9.4\n\n\n"},{"id":"325150","messageId":"xmqq60efq6bo.fsf@gitster.mtv.corp.google.com","threadId":"46466","inReplyTo":"32b61c4b-5e58-30d9-4345-8036bc805019@gmail.com","subject":"Re: [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-26T20:41:31Z","receivedAt":"2017-07-26T20:41:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Raman Gupta <rocketraman@gmail.com> writes:\n\n> On 26/07/17 02:05 PM, Junio C Hamano wrote:\n>> I haven't tried this patch, but would this work well with options\n>> meant for the 'git rev-list --parents \"$@\"' that grabs the list of\n>> merge commits to learn from?  e.g.\n>> \n>> \t$ contrib/rerere-train.sh -n 4 --merges master\n>> \t$ contrib/rerere-train.sh --overwrite -n 4 --merges master\n>> \t$ contrib/rerere-train.sh -n 4 --overwrite --merges master\n>> \n>> I do not think it is necessary to make the last one work; as long as\n>> the first two work as expected, we are good even if the last one\n>> dies with a sensible message e.g. \"options X, Y and Z must be given\n>> before other options\" (currently \"X, Y and Z\" consists only of\n>> \"--overwrite\", but I think you get what I mean).\n>\n> You're right -- I didn't try all the cases. I wasn't able to figure\n> out how to get `rev-parse --parseopt` to deal with this situation, so\n> I did it manually. I'm not super-happy with the result, but it does\n> work. Look for PATCH v3.\n\nYes, I think you could squash the two case arms in the later loop\ninto one i.e.\n\n\t-h|--help|-o|--overwrite)\n\t\tdie \"please don't.\" ;;\n\nbut still the repetition does look ugly.\n\nAs a contrib/ material, I do not care too deeply about it, though.\n\nWill queue.\n"},{"id":"325226","messageId":"c522f452-0b30-3408-1673-8b62ffb7f039@gmail.com","threadId":"46466","inReplyTo":"xmqq60efq6bo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions","fromName":"Raman Gupta","fromEmail":"rocketraman@gmail.com","sentAt":"2017-07-28T18:40:43Z","receivedAt":"2017-07-28T18:40:54Z","isPatch":true,"sender":{"key":"rocketraman@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53049?v=4"},"body":"On 26/07/17 04:41 PM, Junio C Hamano wrote:\n> Raman Gupta <rocketraman@gmail.com> writes:\n> \n>> On 26/07/17 02:05 PM, Junio C Hamano wrote:\n>>> I haven't tried this patch, but would this work well with options\n>>> meant for the 'git rev-list --parents \"$@\"' that grabs the list of\n>>> merge commits to learn from?  e.g.\n>>>\n>>> \t$ contrib/rerere-train.sh -n 4 --merges master\n>>> \t$ contrib/rerere-train.sh --overwrite -n 4 --merges master\n>>> \t$ contrib/rerere-train.sh -n 4 --overwrite --merges master\n>>>\n>>> I do not think it is necessary to make the last one work; as long as\n>>> the first two work as expected, we are good even if the last one\n>>> dies with a sensible message e.g. \"options X, Y and Z must be given\n>>> before other options\" (currently \"X, Y and Z\" consists only of\n>>> \"--overwrite\", but I think you get what I mean).\n>>\n>> You're right -- I didn't try all the cases. I wasn't able to figure\n>> out how to get `rev-parse --parseopt` to deal with this situation, so\n>> I did it manually. I'm not super-happy with the result, but it does\n>> work. Look for PATCH v3.\n> \n> Yes, I think you could squash the two case arms in the later loop\n> into one i.e.\n> \n> \t-h|--help|-o|--overwrite)\n> \t\tdie \"please don't.\" ;;\n\nI considered that but decided the non-collapsed version better\nsupports this list growing in the future.\n\n> but still the repetition does look ugly.\n\nAgreed.\n\n> As a contrib/ material, I do not care too deeply about it, though.\n> \n> Will queue.\n\nThanks.\n\nRegards,\nRaman\n\n"}]}