* [PATCH/RFC] contrib: rerere-train overwrites existing resolutions @ 2017-07-25 14:34 Raman Gupta 2017-07-25 21:18 ` Junio C Hamano 0 siblings, 1 reply; 8+ messages in thread From: Raman Gupta @ 2017-07-25 14:34 UTC (permalink / raw) To: git From 41116889f7eb2ddd590d165d9ca89646f7b15aaf Mon Sep 17 00:00:00 2001 From: Raman Gupta <rocketraman@gmail.com> Date: Tue, 25 Jul 2017 10:28:35 -0400 Subject: [PATCH] contrib: rerere-train overwrites existing resolutions When running rerere-train, the user is explicitly asking the training to occur based on the current merge commit. However, if the current cache has a resolution for the same conflict (even if out of date or wrong), rerere-train does not currently update the rr-cache. Now, forget existing resolutions before training so that training is always reflective of the trained data. --- contrib/rerere-train.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh index 52ad9e4..a222d38 100755 --- a/contrib/rerere-train.sh +++ b/contrib/rerere-train.sh @@ -34,6 +34,7 @@ do # Cleanly merges continue fi + git rerere forget . if test -s "$GIT_DIR/MERGE_RR" then git show -s --pretty=format:"Learning from %h %s" "$commit" -- 2.9.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH/RFC] contrib: rerere-train overwrites existing resolutions 2017-07-25 14:34 [PATCH/RFC] contrib: rerere-train overwrites existing resolutions Raman Gupta @ 2017-07-25 21:18 ` Junio C Hamano 2017-07-25 22:48 ` [PATCH v2] contrib/rerere-train: optionally overwrite " Raman Gupta 0 siblings, 1 reply; 8+ messages in thread From: Junio C Hamano @ 2017-07-25 21:18 UTC (permalink / raw) To: Raman Gupta; +Cc: git Raman Gupta <rocketraman@gmail.com> writes: > From 41116889f7eb2ddd590d165d9ca89646f7b15aaf Mon Sep 17 00:00:00 2001 > From: Raman Gupta <rocketraman@gmail.com> > Date: Tue, 25 Jul 2017 10:28:35 -0400 > Subject: [PATCH] contrib: rerere-train overwrites existing resolutions These do not belong to the body of the message. Imagine that your title line appears in "git shortlog --no-merges" output together with 600 other commits, and you see that list 3 months later. Can you tell what this change was about? To me, it sounds like a user is complaining that it overwrites and loses a precious one, implying that the change is to fix it by being more careful, i.e. checking if there is an existing one and avoid overwriting it. As our convention is to write the log message as if you are giving an order to the codebase "to behave like so", contrib/rerere-train: overwrite existing resolutions would convey what you are doing better, I would think. As to the change itself, I do anticipate that a user who is used to rerere-train will complain that the updated one overwrites existing resolutions, losing data, as the existing one didn't, and they are used to the way to correct an incorrect resolution that is recorded earlier, which is: * perform the botched merge again, which will materialize the botched merge result in the working tree; * do "git rerere forget" for the path; * correct the path with the right merge result; and then finally * do "git rerere". If this new behaviour is triggered only when a command line option is given (e.g. "--overwrite" which is better than "--force"), I would imagine that it might give us an easier way to achieve the same thing without learning about "rerere forget". So I do not fundamentally oppose to having such an option, but changing the default behaviour is not going to fly. > When running rerere-train, the user is explicitly asking the training to > occur based on the current merge commit. However, if the current cache > has a resolution for the same conflict (even if out of date or wrong), > rerere-train does not currently update the rr-cache. Now, forget > existing resolutions before training so that training is always > reflective of the trained data. > --- Actually, the user is asking to record what is not recorded yet, which is the originally intended use case. You may fetch my 'pu' branch into a repository you already have a copy of git.git, and want to grab conflict resolutions I did between master..pu that you still do not know about. The tool is conservative and avoids clobbering your resolution if you already have resolved some of the merges yourself. In any case, please make it a habit to sign-off your work when posting here. > contrib/rerere-train.sh | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh > index 52ad9e4..a222d38 100755 > --- a/contrib/rerere-train.sh > +++ b/contrib/rerere-train.sh > @@ -34,6 +34,7 @@ do > # Cleanly merges > continue > fi > + git rerere forget . > if test -s "$GIT_DIR/MERGE_RR" > then > git show -s --pretty=format:"Learning from %h %s" "$commit" ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions 2017-07-25 21:18 ` Junio C Hamano @ 2017-07-25 22:48 ` Raman Gupta 2017-07-26 18:05 ` Junio C Hamano 0 siblings, 1 reply; 8+ messages in thread From: Raman Gupta @ 2017-07-25 22:48 UTC (permalink / raw) To: git Provide the user an option to overwrite existing resolutions using an `--overwrite` flag. This might be used, for example, if the user knows that they already have an entry in their rerere cache for a conflict, but wish to drop it and retrain based on the merge commit(s) passed to the rerere-train script. Signed-off-by: Raman Gupta <rocketraman@gmail.com> --- contrib/rerere-train.sh | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh index 52ad9e4..e25bf8a 100755 --- a/contrib/rerere-train.sh +++ b/contrib/rerere-train.sh @@ -3,10 +3,38 @@ # Prime rerere database from existing merge commits me=rerere-train -USAGE="$me rev-list-args" SUBDIRECTORY_OK=Yes -OPTIONS_SPEC= +OPTS_SPEC="\ +$me [--overwrite] <rev-list-args> +-- +h,help show the help +o,overwrite overwrite any existing rerere cache +" +eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)" + +overwrite=0 + +while test $# -gt 0 +do + opt="$1" + case "$opt" in + -o) + overwrite=1 + shift + shift + ;; + --) + shift + break + ;; + *) + break + exit 1 + ;; + esac +done + . "$(git --exec-path)/git-sh-setup" require_work_tree cd_to_toplevel @@ -34,6 +62,10 @@ do # Cleanly merges continue fi + if [ $overwrite == 1 ] + then + git rerere forget . + fi if test -s "$GIT_DIR/MERGE_RR" then git show -s --pretty=format:"Learning from %h %s" "$commit" -- 2.9.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions 2017-07-25 22:48 ` [PATCH v2] contrib/rerere-train: optionally overwrite " Raman Gupta @ 2017-07-26 18:05 ` Junio C Hamano 2017-07-26 19:06 ` Raman Gupta 2017-07-26 19:08 ` [PATCH v3] " Raman Gupta 0 siblings, 2 replies; 8+ messages in thread From: Junio C Hamano @ 2017-07-26 18:05 UTC (permalink / raw) To: Raman Gupta; +Cc: git Raman Gupta <rocketraman@gmail.com> writes: > Provide the user an option to overwrite existing resolutions using an > `--overwrite` flag. This might be used, for example, if the user knows > that they already have an entry in their rerere cache for a conflict, > but wish to drop it and retrain based on the merge commit(s) passed to > the rerere-train script. > > Signed-off-by: Raman Gupta <rocketraman@gmail.com> > --- > contrib/rerere-train.sh | 36 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 34 insertions(+), 2 deletions(-) > > diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh > index 52ad9e4..e25bf8a 100755 > --- a/contrib/rerere-train.sh > +++ b/contrib/rerere-train.sh > @@ -3,10 +3,38 @@ > # Prime rerere database from existing merge commits > > me=rerere-train > -USAGE="$me rev-list-args" > > SUBDIRECTORY_OK=Yes > -OPTIONS_SPEC= > +OPTS_SPEC="\ > +$me [--overwrite] <rev-list-args> > +-- > +h,help show the help > +o,overwrite overwrite any existing rerere cache > +" > +eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)" > + > +overwrite=0 It is very good that you initialize overwrite explicitly here, to prevent it from seeping through from the caller's environment. > +while test $# -gt 0 > +do > + opt="$1" > + case "$opt" in > + -o) > + overwrite=1 > + shift > + shift > + ;; > + --) > + shift > + break > + ;; > + *) > + break > + exit 1 > + ;; > + esac > +done I haven't tried this patch, but would this work well with options meant for the 'git rev-list --parents "$@"' that grabs the list of merge commits to learn from? e.g. $ contrib/rerere-train.sh -n 4 --merges master $ contrib/rerere-train.sh --overwrite -n 4 --merges master $ contrib/rerere-train.sh -n 4 --overwrite --merges master I do not think it is necessary to make the last one work; as long as the first two work as expected, we are good even if the last one dies with a sensible message e.g. "options X, Y and Z must be given before other options" (currently "X, Y and Z" consists only of "--overwrite", but I think you get what I mean). > . "$(git --exec-path)/git-sh-setup" > require_work_tree > cd_to_toplevel > @@ -34,6 +62,10 @@ do > # Cleanly merges > continue > fi > + if [ $overwrite == 1 ] if test "$overwrite" = 1 cf. Documentation/CodingGuidelines. > + then > + git rerere forget . > + fi > if test -s "$GIT_DIR/MERGE_RR" > then > git show -s --pretty=format:"Learning from %h %s" "$commit" ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions 2017-07-26 18:05 ` Junio C Hamano @ 2017-07-26 19:06 ` Raman Gupta 2017-07-26 20:41 ` Junio C Hamano 2017-07-26 19:08 ` [PATCH v3] " Raman Gupta 1 sibling, 1 reply; 8+ messages in thread From: Raman Gupta @ 2017-07-26 19:06 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On 26/07/17 02:05 PM, Junio C Hamano wrote: > I haven't tried this patch, but would this work well with options > meant for the 'git rev-list --parents "$@"' that grabs the list of > merge commits to learn from? e.g. > > $ contrib/rerere-train.sh -n 4 --merges master > $ contrib/rerere-train.sh --overwrite -n 4 --merges master > $ contrib/rerere-train.sh -n 4 --overwrite --merges master > > I do not think it is necessary to make the last one work; as long as > the first two work as expected, we are good even if the last one > dies with a sensible message e.g. "options X, Y and Z must be given > before other options" (currently "X, Y and Z" consists only of > "--overwrite", but I think you get what I mean). You're right -- I didn't try all the cases. I wasn't able to figure out how to get `rev-parse --parseopt` to deal with this situation, so I did it manually. I'm not super-happy with the result, but it does work. Look for PATCH v3. Regards, Raman ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions 2017-07-26 19:06 ` Raman Gupta @ 2017-07-26 20:41 ` Junio C Hamano 2017-07-28 18:40 ` Raman Gupta 0 siblings, 1 reply; 8+ messages in thread From: Junio C Hamano @ 2017-07-26 20:41 UTC (permalink / raw) To: Raman Gupta; +Cc: git Raman Gupta <rocketraman@gmail.com> writes: > On 26/07/17 02:05 PM, Junio C Hamano wrote: >> I haven't tried this patch, but would this work well with options >> meant for the 'git rev-list --parents "$@"' that grabs the list of >> merge commits to learn from? e.g. >> >> $ contrib/rerere-train.sh -n 4 --merges master >> $ contrib/rerere-train.sh --overwrite -n 4 --merges master >> $ contrib/rerere-train.sh -n 4 --overwrite --merges master >> >> I do not think it is necessary to make the last one work; as long as >> the first two work as expected, we are good even if the last one >> dies with a sensible message e.g. "options X, Y and Z must be given >> before other options" (currently "X, Y and Z" consists only of >> "--overwrite", but I think you get what I mean). > > You're right -- I didn't try all the cases. I wasn't able to figure > out how to get `rev-parse --parseopt` to deal with this situation, so > I did it manually. I'm not super-happy with the result, but it does > work. Look for PATCH v3. Yes, I think you could squash the two case arms in the later loop into one i.e. -h|--help|-o|--overwrite) die "please don't." ;; but still the repetition does look ugly. As a contrib/ material, I do not care too deeply about it, though. Will queue. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] contrib/rerere-train: optionally overwrite existing resolutions 2017-07-26 20:41 ` Junio C Hamano @ 2017-07-28 18:40 ` Raman Gupta 0 siblings, 0 replies; 8+ messages in thread From: Raman Gupta @ 2017-07-28 18:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On 26/07/17 04:41 PM, Junio C Hamano wrote: > Raman Gupta <rocketraman@gmail.com> writes: > >> On 26/07/17 02:05 PM, Junio C Hamano wrote: >>> I haven't tried this patch, but would this work well with options >>> meant for the 'git rev-list --parents "$@"' that grabs the list of >>> merge commits to learn from? e.g. >>> >>> $ contrib/rerere-train.sh -n 4 --merges master >>> $ contrib/rerere-train.sh --overwrite -n 4 --merges master >>> $ contrib/rerere-train.sh -n 4 --overwrite --merges master >>> >>> I do not think it is necessary to make the last one work; as long as >>> the first two work as expected, we are good even if the last one >>> dies with a sensible message e.g. "options X, Y and Z must be given >>> before other options" (currently "X, Y and Z" consists only of >>> "--overwrite", but I think you get what I mean). >> >> You're right -- I didn't try all the cases. I wasn't able to figure >> out how to get `rev-parse --parseopt` to deal with this situation, so >> I did it manually. I'm not super-happy with the result, but it does >> work. Look for PATCH v3. > > Yes, I think you could squash the two case arms in the later loop > into one i.e. > > -h|--help|-o|--overwrite) > die "please don't." ;; I considered that but decided the non-collapsed version better supports this list growing in the future. > but still the repetition does look ugly. Agreed. > As a contrib/ material, I do not care too deeply about it, though. > > Will queue. Thanks. Regards, Raman ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3] contrib/rerere-train: optionally overwrite existing resolutions 2017-07-26 18:05 ` Junio C Hamano 2017-07-26 19:06 ` Raman Gupta @ 2017-07-26 19:08 ` Raman Gupta 1 sibling, 0 replies; 8+ messages in thread From: Raman Gupta @ 2017-07-26 19:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Provide the user an option to overwrite existing resolutions using an `--overwrite` flag. This might be used, for example, if the user knows that they already have an entry in their rerere cache for a conflict, but wish to drop it and retrain based on the merge commit(s) passed to the rerere-train script. Signed-off-by: Raman Gupta <rocketraman@gmail.com> --- contrib/rerere-train.sh | 54 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh index 52ad9e4..eeee45d 100755 --- a/contrib/rerere-train.sh +++ b/contrib/rerere-train.sh @@ -3,10 +3,56 @@ # Prime rerere database from existing merge commits me=rerere-train -USAGE="$me rev-list-args" +USAGE=$(cat <<-EOF +usage: $me [--overwrite] <rev-list-args> + + -h, --help show the help + -o, --overwrite overwrite any existing rerere cache +EOF +) SUBDIRECTORY_OK=Yes -OPTIONS_SPEC= + +overwrite=0 + +while test $# -gt 0 +do + opt="$1" + case "$opt" in + -h|--help) + echo "$USAGE" + exit 0 + ;; + -o|--overwrite) + overwrite=1 + shift + break + ;; + --) + shift + break + ;; + *) + break + ;; + esac +done + +# Overwrite or help options are not valid except as first arg +for opt in "$@" +do + case "$opt" in + -h|--help) + echo "$USAGE" + exit 0 + ;; + -o|--overwrite) + echo "$USAGE" + exit 0 + ;; + esac +done + . "$(git --exec-path)/git-sh-setup" require_work_tree cd_to_toplevel @@ -34,6 +80,10 @@ do # Cleanly merges continue fi + if test $overwrite = 1 + then + git rerere forget . + fi if test -s "$GIT_DIR/MERGE_RR" then git show -s --pretty=format:"Learning from %h %s" "$commit" -- 2.9.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-07-28 18:40 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-25 14:34 [PATCH/RFC] contrib: rerere-train overwrites existing resolutions Raman Gupta 2017-07-25 21:18 ` Junio C Hamano 2017-07-25 22:48 ` [PATCH v2] contrib/rerere-train: optionally overwrite " Raman Gupta 2017-07-26 18:05 ` Junio C Hamano 2017-07-26 19:06 ` Raman Gupta 2017-07-26 20:41 ` Junio C Hamano 2017-07-28 18:40 ` Raman Gupta 2017-07-26 19:08 ` [PATCH v3] " Raman Gupta
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).