# Re: [PATCH] rerere: record (or avoid misrecording) resolved, skipped or aborted rebase/am 7 messages from 2006-12-08 to 2006-12-09. Participants: Junio C Hamano, Eric Wong, Jakub Narebski. Thread: https://gitlist.dev/t/43149 ## Junio C Hamano, 2006-12-08 20:43 Subject: Re: [PATCH 1/5] rerere: avoid misrecording on a skipped or aborted rebase/am Message-ID: <7v7ix2i0e5.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v7ix2i0e5.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <11655749724034-git-send-email-normalperson@yhbt.net> ``` Eric Wong writes: > +if (@ARGV && $ARGV[0] eq 'clear') { > + for my $path (keys %merge_rr) { > + my $name = $merge_rr{$path}; > + if (-d "$rr_dir/$name") { > + rmtree(["$rr_dir/$name"]); > + } > + } > + unlink $merge_rr; > + exit 0; > +} Come to think of it, I am not sure about this one. Don't you need to make sure that there is no existing resolution before removing it? In other words, shouldn't the removal be like this? if (-d "$rr_dir/$name" && ! -f "$rr_dir/$name/postimage") { rmtree(["$rr_dir/$name"]); } ``` ## Eric Wong, 2006-12-08 21:28 Subject: Re: [PATCH 1/5] rerere: avoid misrecording on a skipped or aborted rebase/am Message-ID: <20061208212830.GB13944@localdomain> URL: https://gitlist.dev/e/20061208212830.GB13944%40localdomain In-Reply-To: <7v7ix2i0e5.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano wrote: > Eric Wong writes: > > > +if (@ARGV && $ARGV[0] eq 'clear') { > > + for my $path (keys %merge_rr) { > > + my $name = $merge_rr{$path}; > > + if (-d "$rr_dir/$name") { > > + rmtree(["$rr_dir/$name"]); > > + } > > + } > > + unlink $merge_rr; > > + exit 0; > > +} > > Come to think of it, I am not sure about this one. Don't you > need to make sure that there is no existing resolution before > removing it? In other words, shouldn't the removal be like this? > > if (-d "$rr_dir/$name" && ! -f "$rr_dir/$name/postimage") { > rmtree(["$rr_dir/$name"]); > } Yes. However, it seems unlikely, that MERGE_RR would point to something that has a postimage. I suppose that a machine could crash or a user could've hit ^C at the correct millisecond that would cause rerere to not update MERGE_RR correctly. Updated patches on the way (leaving out the status updates). -- ``` ## Eric Wong, 2006-12-08 21:29 Subject: [PATCH] rerere: add clear, diff, and status commands Message-ID: <11656133963055-git-send-email-normalperson@yhbt.net> URL: https://gitlist.dev/e/11656133963055-git-send-email-normalperson%40yhbt.net In-Reply-To: <20061208212830.GB13944@localdomain> ``` git-am and git-rebase will be updated to use 'clear', and diff/status can be used to aid the user in tracking progress in the resolution process. Signed-off-by: Eric Wong --- Documentation/git-rerere.txt | 27 +++++++++++++++++++++++++-- git-rerere.perl | 25 +++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/Documentation/git-rerere.txt b/Documentation/git-rerere.txt index 8b6b651..22494b2 100644 --- a/Documentation/git-rerere.txt +++ b/Documentation/git-rerere.txt @@ -7,8 +7,7 @@ git-rerere - Reuse recorded resolve SYNOPSIS -------- -'git-rerere' - +'git-rerere' [clear|diff|status] DESCRIPTION ----------- @@ -167,6 +166,30 @@ would conflict the same way the test merge you resolved earlier. `git-rerere` is run by `git rebase` to help you resolve this conflict. +COMMANDS +-------- + +Normally, git-rerere is run without arguments or user-intervention. +However, it has several commands that allow it to interact with +its working state. + +'clear':: + +This resets the metadata used by rerere if a merge resolution is to be +is aborted. Calling gitlink:git-am[1] --skip or gitlink:git-rebase[1] +[--skip|--abort] will automatcally invoke this command. + +'diff':: + +This displays diffs for the current state of the resolution. It is +useful for tracking what has changed while the user is resolving +conflicts. Additional arguments are passed directly to the system +diff(1) command installed in PATH. + +'status':: + +Like diff, but this only prints the filenames that will be tracked +for resolutions. Author ------ diff --git a/git-rerere.perl b/git-rerere.perl index d3664ff..2703d01 100755 --- a/git-rerere.perl +++ b/git-rerere.perl @@ -172,6 +172,31 @@ sub merge { -d "$rr_dir" || exit(0); read_rr(); + +if (my $arg = shift @ARGV) { + if ($arg eq 'clear') { + for my $path (keys %merge_rr) { + my $name = $merge_rr{$path}; + if (-d "$rr_dir/$name" && ! -f "$rr_dir/$name/postimage") { + rmtree(["$rr_dir/$name"]); + } + } + unlink $merge_rr; + } elsif ($arg eq 'status') { + for my $path (keys %merge_rr) { + print $path, "\n"; + } + } elsif ($arg eq 'diff') { + for my $path (keys %merge_rr) { + my $name = $merge_rr{$path}; + system(qw/diff/, @ARGV, + '-L', "a/$path", '-L', "b/$path", + "$rr_dir/$name/preimage", $path); + } + } + exit 0; +} + my %conflict = map { $_ => 1 } find_conflict(); # MERGE_RR records paths with conflicts immediately after merge -- 1.4.4.2.g860f4 ``` ## Eric Wong, 2006-12-08 21:29 Subject: [PATCH] rerere: record (or avoid misrecording) resolved, skipped or aborted rebase/am Message-ID: <1165613397460-git-send-email-normalperson@yhbt.net> URL: https://gitlist.dev/e/1165613397460-git-send-email-normalperson%40yhbt.net In-Reply-To: <11656133963055-git-send-email-normalperson@yhbt.net> ``` Data in rr-cache isn't valid after a patch application is skipped or and aborted, so our next commit could be misrecorded as a resolution of that skipped/failed commit, which is wrong. git-am --skip, git-rebase --skip/--abort will automatically invoke git-rerere clear to avoid this. Also, since git-am --resolved indicates a resolution was succesful, remember to run git-rerere to record the resolution (and not surprise the user when the next commit is made). Signed-off-by: Eric Wong --- git-am.sh | 8 ++++++++ git-rebase.sh | 8 ++++++++ 2 files changed, 16 insertions(+), 0 deletions(-) diff --git a/git-am.sh b/git-am.sh index afe322b..5df6787 100755 --- a/git-am.sh +++ b/git-am.sh @@ -246,6 +246,10 @@ last=`cat "$dotest/last"` this=`cat "$dotest/next"` if test "$skip" = t then + if test -d "$GIT_DIR/rr-cache" + then + git-rerere clear + fi this=`expr "$this" + 1` resume= fi @@ -408,6 +412,10 @@ do stop_here_user_resolve $this fi apply_status=0 + if test -d "$GIT_DIR/rr-cache" + then + git rerere + fi ;; esac diff --git a/git-rebase.sh b/git-rebase.sh index 25530df..2b4f347 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -139,6 +139,10 @@ do --skip) if test -d "$dotest" then + if test -d "$GIT_DIR/rr-cache" + then + git-rerere clear + fi prev_head="`cat $dotest/prev_head`" end="`cat $dotest/end`" msgnum="`cat $dotest/msgnum`" @@ -157,6 +161,10 @@ do exit ;; --abort) + if test -d "$GIT_DIR/rr-cache" + then + git-rerere clear + fi if test -d "$dotest" then rm -r "$dotest" -- 1.4.4.2.g860f4 ``` ## Jakub Narebski, 2006-12-08 21:44 Subject: Re: [PATCH] rerere: record (or avoid misrecording) resolved, skipped or aborted rebase/am Message-ID: URL: https://gitlist.dev/e/elcm6u%24l8t%241%40sea.gmane.org In-Reply-To: <1165613397460-git-send-email-normalperson@yhbt.net> ``` Eric Wong wrote: Just one comment: > +       if test -d "$GIT_DIR/rr-cache" > +       then > +               git-rerere clear > +       fi >         this=`expr "$this" + 1` >         resume= >  fi > @@ -408,6 +412,10 @@ do >                         stop_here_user_resolve $this >                 fi >                 apply_status=0 > +               if test -d "$GIT_DIR/rr-cache" > +               then > +                       git rerere > +               fi Why do you use once "git-rerere", and another time "git rerere"? Shouldn't scripts always use git-command form? -- Jakub Narebski Warsaw, Poland ShadeHawk on #git ``` ## Eric Wong, 2006-12-08 21:50 Subject: Re: [PATCH] rerere: record (or avoid misrecording) resolved, skipped or aborted rebase/am Message-ID: <20061208215021.GA28018@localdomain> URL: https://gitlist.dev/e/20061208215021.GA28018%40localdomain In-Reply-To: ``` Jakub Narebski wrote: > Eric Wong wrote: > > Just one comment: > > > +       if test -d "$GIT_DIR/rr-cache" > > +       then > > +               git-rerere clear > > +       fi > >         this=`expr "$this" + 1` > >         resume= > >  fi > > @@ -408,6 +412,10 @@ do > >                         stop_here_user_resolve $this > >                 fi > >                 apply_status=0 > > +               if test -d "$GIT_DIR/rr-cache" > > +               then > > +                       git rerere > > +               fi > > Why do you use once "git-rerere", and another time "git rerere"? > Shouldn't scripts always use git-command form? Yes. Or at least I remember git-command form being preferred. I don't think I've been entirely consistent in the things I've written, and my shell history is just as inconsistent :x -- ``` ## Junio C Hamano, 2006-12-09 20:08 Subject: Re: [PATCH] rerere: record (or avoid misrecording) resolved, skipped or aborted rebase/am Message-ID: <7vbqmcbzn2.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vbqmcbzn2.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <1165613397460-git-send-email-normalperson@yhbt.net> ``` I looked at your previous 5-patch series but will replace with this updated 2-patch series. The rerere command started its life as a mere helper for Porcelains, but with 'status' and 'diff', you are making it somewhat Porcelain-ish. I still have not decided myself, but I think we probably would want to treat this as a Porcelain and give a bit of usability to it, so what I've committed has two changes from your version: (1) 'diff' without @ARGV defaults to '-u'; (2) running rerere with unknown sub command die()s instead of silently exiting with 0. The difference between your previous 5-patch series and this round raises an interesting issue. You used to have a safety valve in git-commit to prevent users from running it while rebase/am is in progress, which I imagine would be a common mistake. There are three commands in git suite that a user would use to eventually make new commits, out of something other than what was edited and prepared in the working tree, that can stop in the middle. (1) git-am is used to make new commits from e-mailed patches, and stops in the middle when a patch does not apply. "git-am --resolved/--skip" are the ways to continue. (2) git-rebase is used to rewrite the history of a branch, and stops in the middle when a patch does not apply or merge conflicts. "git-rebase --continue/--skip/--abort" are possible exits [*1*]. (3) git-pull/git-merge are used to make merge commits with another branch, and stops in the middle when it conflicts. Possible exits are "git commit" or "git reset". Now, I am not going to propose to fix this inconsistency and forbid use of "git commit" to resolve "git-merge" (instead, the user would say "git-merge --resolved/--abort"). As the workflow goes, these are very different operations and the user is often well aware which workflow is in progress; having different commands for different exit route is fine. But in order to implement the safety valve like you did cleanly, we should first have an enumeration of states (the above is a subset --- there is the "base state" in which you are working towards a commit based on HEAD, which is obviously concluded with a "git commit", and "bisect in progress") and make it easy for commands to communicate what state the working tree is in. That would also help 'git-explain'. [Footnote] *1* Resuming rebase was much worse before Sean updated it with commit 031321c6 to hide that it uses git-am as its backend, which is purely its implementation detail. ```