"rerere" minor clean-up
v2 cover "rerere" minor clean-up
Here is an collection of various minor clean-ups in the implementation of one of my most favourite feature, rerere.
This still hasn't reached the step to make the right refactoring to allow me to fix a bug I wanted to fix, which prompted me to look at this code, but it should give me a good starting point.
Junio C Hamano (13):
rerere: fix an off-by-one non-bug
rerere: plug conflict ID leaks
rerere: lift PATH_MAX limitation
rerere: write out each record of MERGE_RR in one go
rerere: report autoupdated paths only after actually updating them
rerere: drop want_sp parameter from is_cmarker()
rerere: stop looping unnecessarily
rerere: explain the rerere I/O abstraction
rerere: explain MERGE_RR management helpers
rerere: explain the primary codepath
rerere: explain "rerere forget" codepath
rerere: explain the remainder
rerere: refactor "replay" part of do_plain_rerere()
rerere.c | 389 +++++++++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 293 insertions(+), 96 deletions(-)
Signature · 1 line
2.5.0-rc0-209-g5e1f148
PATCH v2 1/13 rerere: fix an off-by-one non-bug
When ac49f5ca (rerere "remaining", 2011-02-16) split out a new helper function check_one_conflict() out of find_conflict() function, so that the latter will use the returned value from the new helper to update the loop control variable that is an index into active_cache[], the new variable incremented the index by one too many when it found a path with only stage #1 entry at the very end of active_cache[].
This "strange" return value does not have any effect on the loop control of two callers of this function, as they all notice that active_nr+2 is larger than active_nr just like active_nr+1 is, but nevertheless it puzzles the readers when they are trying to figure out what the function is trying to do.
In fact, there is no need to do an early return. The code that follows after skipping the stage #1 entry is fully prepared to handle a case where the entry is at the very end of active_cache[].
Help future readers from unnecessary confusion by dropping an early return. We skip the stage #1 entry, and if there are stage #2 and stage #3 entries for the same path, we diagnose the path as THREE_STAGED (otherwise we say PUNTED), and then we skip all entries for the same path.
--- rerere.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
rerere.c +2 -4
@@ -369,10 +369,8 @@ static int check_one_conflict(int i, int *type) } *type =PUNTED ; -if (ce_stage (e) ==1 ) { -if (active_nr <= ++i) -return i +1 ; - } +if (ce_stage (e) ==1 ) + i++;/* Only handle regular files with both stages #2 and #3 */ if (i +1 < active_nr) {
Signature · 1 line
2.5.0-rc0-209-g5e1f148
PATCH v2 2/13 rerere: plug conflict ID leaks
In reply to Junio C HamanoThe merge_rr string list stores the conflict ID (a hexadecimal string that is used to index into $GIT_DIR/rr-cache) in the .util field of its elements, and when do_plain_rerere() resolves a conflict, the field is cleared. Also, when rerere_forget() recomputes the conflict ID to updates the preimage file, the conflict ID for the path is updated.
We forgot to free the existing conflict ID when we did these two operations.
--- rerere.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
rerere.c +5 -2
@@ -559,6 +559,7 @@ static int do_plain_rerere(struct string_list *rr, int fd)fprintf (stderr,"Recorded resolution for '%s'.\n" , path);copy_file (rerere_path (name,"postimage" ), path,0666 ); mark_resolved: +free (rr->items[i].util); rr->items[i].util =NULL ; }
@@ -627,6 +628,7 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)char *hex;unsigned char sha1[20 ];int ret; +struct string_list_item *item; ret =handle_cache (path, sha1,NULL );if (ret <1 )
@@ -641,8 +643,9 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)handle_cache (path, sha1,rerere_path (hex,"preimage" ));fprintf (stderr,"Updated preimage for '%s'\n" , path); - -string_list_insert (rr, path)->util = hex; + item =string_list_insert (rr, path); +free (item->util); + item->util = hex;fprintf (stderr,"Forgot resolution for %s\n" , path);return 0 ; }
Signature · 1 line
2.5.0-rc0-209-g5e1f148
PATCH v2 3/13 rerere: lift PATH_MAX limitation
In reply to Junio C HamanoThe MERGE_RR file records a collection of NUL-terminated entries, each of which consists of
- a hash that identifies the conflict
- a HT
- the pathname
We used to read this piece-by-piece, and worse yet, read the pathname part a byte at a time into a fixed buffer of size PATH_MAX.
Instead, read a whole entry using strbuf_getwholeline() and parse out the fields. This way, we issue fewer read(2) calls and more importantly we do not have to limit the pathname to PATH_MAX.
--- rerere.c | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-)
rerere.c +15 -20
@@ -35,32 +35,27 @@ static int has_rerere_resolution(const char *hex)static void read_rr (struct string_list *rr) { -unsigned char sha1[20 ]; -char buf[PATH_MAX ]; +struct strbuf buf =STRBUF_INIT ;FILE *in =fopen (merge_rr_path,"r" ); +if (!in)return ; -while (fread (buf,40 ,1 , in) ==1 ) { -int i; -char *name; -if (get_sha1_hex (buf, sha1)) +while (!strbuf_getwholeline (&buf, in,'\0' )) { +char *path; +unsigned char sha1[20 ]; + +/* There has to be the hash, tab, path and then NUL */ +if (buf.len <42 ||get_sha1_hex (buf.buf, sha1))die ("corrupt MERGE_RR" ); - buf[40 ] ='\0' ; - name =xstrdup (buf); -if (fgetc (in) !='\t' ) + +if (buf.buf[40 ] !='\t' )die ("corrupt MERGE_RR" ); -for (i =0 ; i <sizeof (buf); i++) { -int c =fgetc (in); -if (c <0 ) -die ("corrupt MERGE_RR" ); - buf[i] = c; -if (c ==0 ) -break ; - } -if (i ==sizeof (buf)) -die ("filename too long" ); -string_list_insert (rr, buf)->util = name; + buf.buf[40 ] ='\0' ; + path = buf.buf +41 ; + +string_list_insert (rr, path)->util =xstrdup (buf.buf); } +strbuf_release (&buf);fclose (in); }
Signature · 1 line
2.5.0-rc0-209-g5e1f148
PATCH v2 4/13 rerere: write out each record of MERGE_RR in one go
In reply to Junio C HamanoInstead of writing the hash for a conflict, a HT, and the path with three separate write_in_full() calls, format them into a single record into a strbuf and write it out in one go.
As a more recent "rerere remaining" codepath abuses the .util field of the merge_rr data to store a sentinel token, make sure that codepath does not call into this function (of course, "remaining" is a read-only operation and currently does not call it).
--- rerere.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
rerere.c +9 -7
@@ -65,16 +65,18 @@ static int write_rr(struct string_list *rr, int out_fd) {int i;for (i =0 ; i < rr->nr; i++) { -const char *path; -int length; +struct strbuf buf =STRBUF_INIT ; + +assert (rr->items[i].util !=RERERE_RESOLVED );if (!rr->items[i].util)continue ; - path = rr->items[i].string; - length =strlen (path) +1 ; -if (write_in_full (out_fd, rr->items[i].util,40 ) !=40 || -write_str_in_full (out_fd,"\t" ) !=1 || -write_in_full (out_fd, path, length) != length) +strbuf_addf (&buf,"%s\t%s%c" , + (char *)rr->items[i].util, + rr->items[i].string,0 ); +if (write_in_full (out_fd, buf.buf, buf.len) != buf.len)die ("unable to write rerere record" ); + +strbuf_release (&buf); }if (commit_lock_file (&write_lock) !=0 )die ("unable to write rerere record" );
Signature · 1 line
2.5.0-rc0-209-g5e1f148
PATCH v2 5/13 rerere: report autoupdated paths only after actually updating them
In reply to Junio C Hamano--- rerere.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
rerere.c +12 -10
@@ -482,6 +482,8 @@ static void update_paths(struct string_list *update)struct string_list_item *item = &update->items[i];if (add_file_to_cache (item->string,0 ))exit (128 ); +fprintf (stderr,"Staged '%s' using previous resolution.\n" , + item->string); }if (active_cache_changed) {
@@ -536,16 +538,16 @@ static int do_plain_rerere(struct string_list *rr, int fd)const char *name = (const char *)rr->items[i].util;if (has_rerere_resolution (name)) { -if (!merge (name, path)) { -const char *msg; -if (rerere_autoupdate) { -string_list_insert (&update, path); - msg ="Staged '%s' using previous resolution.\n" ; - }else - msg ="Resolved '%s' using previous resolution.\n" ; -fprintf (stderr, msg, path); - goto mark_resolved; - } +if (merge (name, path)) +continue ; + +if (rerere_autoupdate) +string_list_insert (&update, path); +else +fprintf (stderr, +"Resolved '%s' using previous resolution.\n" , + path); + goto mark_resolved; }/* Let's see if we have resolved it. */
Signature · 1 line
2.5.0-rc0-209-g5e1f148
PATCH v2 6/13 rerere: drop want_sp parameter from is_cmarker()
In reply to Junio C HamanoAs the nature of the conflict marker line determies if there should a SP and label after it, the caller shouldn't have to pass the parameter redundantly.
--- rerere.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
rerere.c +22 -5
@@ -148,8 +148,25 @@ static int rerere_file_getline(struct strbuf *sb, struct rerere_io *io_)return strbuf_getwholeline (sb, io->input,'\n' ); } -static int is_cmarker (char *buf,int marker_char,int marker_size,int want_sp) +/* +* Require the exact number of conflict marker letters, no more, no +* less, followed by SP or any whitespace +* (including LF). +*/ +static int is_cmarker (char *buf,int marker_char,int marker_size) { +int want_sp; + +/* +* The beginning of our version and the end of their version +* always are labeled like "<<<<< ours" or ">>>>> theirs", +* hence we set want_sp for them. Note that the version from +* the common ancestor in diff3-style output is not always +* labelled (e.g. "||||| common" is often seen but "|||||" +* alone is also valid), so we do not set want_sp. +*/ + want_sp = (marker_char =='<' ) || (marker_char =='>' ); +while (marker_size--)if (*buf++ != marker_char)return 0 ;
@@ -172,19 +189,19 @@ static int handle_path(unsigned char *sha1, struct rerere_io *io, int marker_sizgit_SHA1_Init (&ctx);while (!io->getline (&buf, io)) { -if (is_cmarker (buf.buf,'<' , marker_size,1 )) { +if (is_cmarker (buf.buf,'<' , marker_size)) {if (hunk !=RR_CONTEXT ) goto bad; hunk =RR_SIDE_1 ; - }else if (is_cmarker (buf.buf,'|' , marker_size,0 )) { + }else if (is_cmarker (buf.buf,'|' , marker_size)) {if (hunk !=RR_SIDE_1 ) goto bad; hunk =RR_ORIGINAL ; - }else if (is_cmarker (buf.buf,'=' , marker_size,0 )) { + }else if (is_cmarker (buf.buf,'=' , marker_size)) {if (hunk !=RR_SIDE_1 && hunk !=RR_ORIGINAL ) goto bad; hunk =RR_SIDE_2 ; - }else if (is_cmarker (buf.buf,'>' , marker_size,1 )) { + }else if (is_cmarker (buf.buf,'>' , marker_size)) {if (hunk !=RR_SIDE_2 ) goto bad;if (strbuf_cmp (&one, &two) >0 )
Signature · 1 line
2.5.0-rc0-209-g5e1f148
PATCH v2 7/13 rerere: stop looping unnecessarily
In reply to Junio C Hamanohandle_cache() loops 3 times starting from an index entry that is unmerged, while ignoring an entry for a path that is different from what we are looking for.
As the index is sorted, once we see a different path, we know we saw all stages for the path we are interested in. Just loop while we see the same path and then break, instead of continuing for 3 times.
--- rerere.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
rerere.c +7 -10
@@ -329,24 +329,21 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpureturn -1 ; pos = -pos -1 ; -for (i =0 ; i <3 ; i++) { +while (pos < active_nr) {enum object_type type;unsigned long size; -int j; -if (active_nr <= pos) -break ; ce = active_cache[pos++];if (ce_namelen (ce) != len ||memcmp (ce->name, path, len)) -continue ; - j =ce_stage (ce) -1 ; - mmfile[j].ptr =read_sha1_file (ce->sha1, &type, &size); - mmfile[j].size = size; +break ; + i =ce_stage (ce) -1 ; + mmfile[i].ptr =read_sha1_file (ce->sha1, &type, &size); + mmfile[i].size = size; } -for (i =0 ; i <3 ; i++) { +for (i =0 ; i <3 ; i++)if (!mmfile[i].ptr && !mmfile[i].size) mmfile[i].ptr =xstrdup ("" ); - } +/* * NEEDSWORK: handle conflicts from merges with * merge.renormalize set, too
Signature · 1 line
2.5.0-rc0-209-g5e1f148
PATCH v2 8/13 rerere: explain the rerere I/O abstraction
In reply to Junio C HamanoExplain the internals of rerere as in-code comments.
This one covers our thin I/O abstraction to read from either a file or a memory while optionally writing out to a file.
--- rerere.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)
rerere.c +31 -7
@@ -83,6 +83,21 @@ static int write_rr(struct string_list *rr, int out_fd)return 0 ; } +/* +* "rerere" interacts with conflicted file contents using this I/O +* abstraction. It reads a conflicted contents from one place via +* "getline()" method, and optionally can write it out after +* normalizing the conflicted hunks to the "output". Subclasses of +* rerere_io embed this structure at the beginning of their own +* rerere_io object. +*/ +struct rerere_io { +int (*getline)(struct strbuf *,struct rerere_io *); +FILE *output; +int wrerror; +/* some more stuff */ +}; +static void ferr_write (const void *p,size_t count,FILE *fp,int *err) {if (!count || *err)
@@ -96,19 +111,15 @@ static inline void ferr_puts(const char *s, FILE *fp, int *err)ferr_write (s,strlen (s), fp, err); } -struct rerere_io { -int (*getline)(struct strbuf *,struct rerere_io *); -FILE *output; -int wrerror; -/* some more stuff */ -}; -static void rerere_io_putstr (const char *str,struct rerere_io *io) {if (io->output)ferr_puts (str, io->output, &io->wrerror); } +/* +* Write a conflict marker to io->output (if defined). +*/ static void rerere_io_putconflict (int ch,int size,struct rerere_io *io) {char buf[64 ];
@@ -137,11 +148,17 @@ static void rerere_io_putmem(const char *mem, size_t sz, struct rerere_io *io)ferr_write (mem, sz, io->output, &io->wrerror); } +/* +* Subclass of rerere_io that reads from an on-disk file +*/ struct rerere_io_file {struct rerere_io io;FILE *input; }; +/* +* ... and its getline() method implementation +*/ static int rerere_file_getline (struct strbuf *sb,struct rerere_io *io_) {struct rerere_io_file *io = (struct rerere_io_file *)io_;
@@ -286,11 +303,18 @@ static int handle_file(const char *path, unsigned char *sha1, const char *outputreturn hunk_no; } +/* +* Subclass of rerere_io that reads from an in-core buffer that is a +* strbuf +*/ struct rerere_io_mem {struct rerere_io io;struct strbuf input; }; +/* +* ... and its getline() method implementation +*/ static int rerere_mem_getline (struct strbuf *sb,struct rerere_io *io_) {struct rerere_io_mem *io = (struct rerere_io_mem *)io_;
Signature · 1 line
2.5.0-rc0-209-g5e1f148
PATCH v2 9/13 rerere: explain MERGE_RR management helpers
In reply to Junio C HamanoExplain the internals of rerere as in-code comments, while sprinkling "NEEDSWORK" comment to highlight iffy bits and questionable assumptions.
This one covers the "$GIT_DIR/MERGE_RR" file and in-core merge_rr that are used to keep track of the status of "rerere" session in progress.
--- rerere.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
rerere.c +41 -0
@@ -33,6 +33,13 @@ static int has_rerere_resolution(const char *hex)return !stat (rerere_path (hex,"postimage" ), &st); } +/* +* $GIT_DIR/MERGE_RR file is a collection of records, each of which is +* "conflict ID", a HT and pathname, terminated with a NUL, and is +* used to keep track of the set of paths that "rerere" may need to +* work on (i.e. what is left by the previous invocation of "git +* rerere" during the current conflict resolution session). +*/ static void read_rr (struct string_list *rr) {struct strbuf buf =STRBUF_INIT ;
@@ -394,6 +401,14 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpureturn hunk_no; } +/* +* Look at a cache entry at "i" and see if it is not conflicting, +* conflicting and we are willing to handle, or conflicting and +* we are unable to handle, and return the determination in *type. +* Return the cache index to be looked at next, by skipping the +* stages we have already looked at in this invocation of this +* function. +*/ static int check_one_conflict (int i,int *type) {const struct cache_entry *e = active_cache[i];
@@ -425,6 +440,17 @@ static int check_one_conflict(int i, int *type)return i; } +/* +* Scan the index and find paths that have conflicts that rerere can +* handle, i.e. the ones that has both stages #2 and #3. +* +* NEEDSWORK: we do not record or replay a previous "resolve by +* deletion" for a delete-modify conflict, as that is inherently risky +* without knowing what modification is being discarded. The only +* safe case, i.e. both side doing the deletion and modification that +* are identical to the previous round, might want to be handled, +* though. +*/ static int find_conflict (struct string_list *conflict) {int i;
@@ -441,6 +467,21 @@ static int find_conflict(struct string_list *conflict)return 0 ; } +/* +* The merge_rr list is meant to hold outstanding conflicted paths +* that rerere could handle. Abuse the list by adding other types of +* entries to allow the caller to show "rerere remaining". +* +* - Conflicted paths that rerere does not handle are added +* - Conflicted paths that have been resolved are marked as such +* by storing RERERE_RESOLVED to .util field (where conflict ID +* is expected to be stored). +* +* Do *not* write MERGE_RR file out after calling this function. +* +* NEEDSWORK: we may want to fix the caller that implements "rerere +* remaining" to do this without abusing merge_rr. +*/ int rerere_remaining (struct string_list *merge_rr) {int i;
Signature · 1 line
2.5.0-rc0-209-g5e1f148
PATCH v2 10/13 rerere: explain the primary codepath
In reply to Junio C HamanoExplain the internals of rerere as in-code comments, while sprinkling "NEEDSWORK" comment to highlight iffy bits and questionable assumptions.
This one covers the codepath reached from rerere(), the primary interface to the subsystem.
--- rerere.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 82 insertions(+), 13 deletions(-)
rerere.c +82 -13
@@ -199,6 +199,21 @@ static int is_cmarker(char *buf, int marker_char, int marker_size)return isspace (*buf); } +/* +* Read contents a file with conflicts, normalize the conflicts +* by (1) discarding the common ancestor version in diff3-style, +* (2) reordering our side and their side so that whichever sorts +* alphabetically earlier comes before the other one, while +* computing the "conflict ID", which is just an SHA-1 hash of +* one side of the conflict, NUL, the other side of the conflict, +* and NUL concatenated together. +* +* Return the number of conflict hunks found. +* +* NEEDSWORK: the logic and theory of operation behind this conflict +* normalization may deserve to be documented somewhere, perhaps in +* Documentation/technical/rerere.txt. +*/ static int handle_path (unsigned char *sha1,struct rerere_io *io,int marker_size) {git_SHA_CTX ctx;
@@ -269,6 +284,10 @@ static int handle_path(unsigned char *sha1, struct rerere_io *io, int marker_sizreturn hunk_no; } +/* +* Scan the path for conflicts, do the "handle_path()" thing above, and +* return the number of conflict hunks found. +*/ static int handle_file (const char *path,unsigned char *sha1,const char *output) {int hunk_no =0 ;
@@ -506,29 +525,54 @@ int rerere_remaining(struct string_list *merge_rr)return 0 ; } +/* +* Find the conflict identified by "name"; the change between its +* "preimage" (i.e. a previous contents with conflict markers) and its +* "postimage" (i.e. the corresponding contents with conflicts +* resolved) may apply cleanly to the contents stored in "path", i.e. +* the conflict this time around. +* +* Returns 0 for successful replay of recorded resolution, or non-zero +* for failure. +*/ static int merge (const char *name,const char *path) {int ret;mmfile_t cur = {NULL ,0 }, base = {NULL ,0 }, other = {NULL ,0 };mmbuffer_t result = {NULL ,0 }; +/* +* Normalize the conflicts in path and write it out to +* "thisimage" temporary file. +*/ if (handle_file (path,NULL ,rerere_path (name,"thisimage" )) <0 )return 1 ;if (read_mmfile (&cur,rerere_path (name,"thisimage" )) || -read_mmfile (&base,rerere_path (name,"preimage" )) || -read_mmfile (&other,rerere_path (name,"postimage" ))) { +read_mmfile (&base,rerere_path (name,"preimage" )) || +read_mmfile (&other,rerere_path (name,"postimage" ))) { ret =1 ; goto out; } + +/* +* A three-way merge. Note that this honors user-customizable +* low-level merge driver settings. +*/ ret =ll_merge (&result, path, &base,NULL , &cur,"" , &other,"" ,NULL );if (!ret) {FILE *f; +/* +* A successful replay of recorded resolution. +* Mark that "postimage" was used to help gc. +*/ if (utime (rerere_path (name,"postimage" ),NULL ) <0 )warning ("failed utime() on %s: %s" ,rerere_path (name,"postimage" ),strerror (errno)); + +/* Update "path" with the resolution */ f =fopen (path,"w" );if (!f) return error("Could not open %s: %s", path,
@@ -581,41 +625,61 @@ static int do_plain_rerere(struct string_list *rr, int fd)find_conflict (&conflict);/* -* MERGE_RR records paths with conflicts immediately after merge -* failed. Some of the conflicted paths might have been hand resolved -* in the working tree since then, but the initial run would catch all -* and register their preimages. +* MERGE_RR records paths with conflicts immediately after +* merge failed. Some of the conflicted paths might have been +* hand resolved in the working tree since then, but the +* initial run would catch all and register their preimages. */ -for (i =0 ; i < conflict.nr; i++) {const char *path = conflict.items[i].string;if (!string_list_has_string (rr, path)) {unsigned char sha1[20 ];char *hex;int ret; + +/* +* Ask handle_file() to scan and assign a +* conflict ID. No need to write anything out +* yet. +*/ ret =handle_file (path, sha1,NULL );if (ret <1 )continue ; hex =xstrdup (sha1_to_hex (sha1));string_list_insert (rr, path)->util = hex; + +/* +* If the directory does not exist, create +* it. mkdir_in_gitdir() will fail with +* EEXIST if there already is one. +* +* NEEDSWORK: make sure "gc" does not remove +* preimage without removing the directory. +*/ if (mkdir_in_gitdir (git_path ("rr-cache/%s" , hex)))continue ; + +/* +* We are the first to encounter this +* conflict. Ask handle_file() to write the +* normalized contents to the "preimage" file. +*/ handle_file (path,NULL ,rerere_path (hex,"preimage" ));fprintf (stderr,"Recorded preimage for '%s'\n" , path); } }/* -* Now some of the paths that had conflicts earlier might have been -* hand resolved. Others may be similar to a conflict already that -* was resolved before. +* Some of the paths that had conflicts earlier might have +* been resolved by the user. Others may be similar to a +* conflict already that was resolved before. */ -for (i =0 ; i < rr->nr; i++) {int ret;const char *path = rr->items[i].string;const char *name = (const char *)rr->items[i].util; +/* Is there a recorded resolution we could attempt to apply? */ if (has_rerere_resolution (name)) {if (merge (name, path))continue ;
@@ -629,13 +693,13 @@ static int do_plain_rerere(struct string_list *rr, int fd) goto mark_resolved; } -/* Let's see if we have resolved it. */ +/* Let's see if the user has resolved it. */ ret =handle_file (path,NULL ,NULL );if (ret)continue ; -fprintf (stderr,"Recorded resolution for '%s'.\n" , path);copy_file (rerere_path (name,"postimage" ), path,0666 ); +fprintf (stderr,"Recorded resolution for '%s'.\n" , path); mark_resolved:free (rr->items[i].util); rr->items[i].util =NULL ;
@@ -689,6 +753,11 @@ int setup_rerere(struct string_list *merge_rr, int flags)return fd; } +/* +* The main entry point that is called internally from codepaths that +* perform mergy operations, possibly leaving conflicted index entries +* and working tree files. +*/ int rerere (int flags) {struct string_list merge_rr =STRING_LIST_INIT_DUP ;
Signature · 1 line
2.5.0-rc0-209-g5e1f148
PATCH v2 11/13 rerere: explain "rerere forget" codepath
In reply to Junio C HamanoExplain the internals of rerere as in-code comments, while sprinkling "NEEDSWORK" comment to highlight iffy bits and questionable assumptions.
This covers the codepath that implements "rerere forget".
--- rerere.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
rerere.c +24 -0
@@ -413,6 +413,10 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpustrbuf_init (&io.input,0 );strbuf_attach (&io.input, result.ptr, result.size, result.size); +/* +* Grab the conflict ID and optionally write the original +* contents with conflict markers out. +*/ hunk_no =handle_path (sha1, (struct rerere_io *)&io, marker_size);strbuf_release (&io.input);if (io.io.output)
@@ -777,9 +781,15 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)int ret;struct string_list_item *item; +/* +* Recreate the original conflict from the stages in the +* index and compute the conflict ID +*/ ret =handle_cache (path, sha1,NULL );if (ret <1 )return error ("Could not parse conflict hunks in '%s'" , path); + +/* Nuke the recorded resolution for the conflict */ hex =xstrdup (sha1_to_hex (sha1)); filename =rerere_path (hex,"postimage" );if (unlink (filename))
@@ -787,9 +797,18 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr) ?error ("no remembered resolution for %s" , path) : error("cannot unlink %s: %s", filename, strerror(errno))); +/* +* Update the preimage so that the user can resolve the +* conflict in the working tree, run us again to record +* the postimage. +*/ handle_cache (path, sha1,rerere_path (hex,"preimage" ));fprintf (stderr,"Updated preimage for '%s'\n" , path); +/* +* And remember that we can record resolution for this +* conflict when the user is done. +*/ item =string_list_insert (rr, path);free (item->util); item->util = hex;
@@ -808,6 +827,11 @@ int rerere_forget(struct pathspec *pathspec) fd =setup_rerere (&merge_rr,RERERE_NOAUTOUPDATE ); +/* +* The paths may have been resolved (incorrectly); +* recover the original conflicted state and then +* find the conflicted paths. +*/ unmerge_cache (pathspec);find_conflict (&conflict);for (i =0 ; i < conflict.nr; i++) {
Signature · 1 line
2.5.0-rc0-209-g5e1f148
PATCH v2 12/13 rerere: explain the remainder
In reply to Junio C HamanoExplain the internals of rerere as in-code comments, while sprinkling "NEEDSWORK" comment to highlight iffy bits and questionable assumptions.
This covers the codepath that implements "rerere gc" and "rerere clear".
--- rerere.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
rerere.c +20 -0
@@ -844,6 +844,9 @@ int rerere_forget(struct pathspec *pathspec)return write_rr (&merge_rr, fd); } +/* +* Garbage collection support +*/ static time_t rerere_created_at (const char *name) {struct stat st;
@@ -856,11 +859,19 @@ static time_t rerere_last_used_at(const char *name)return stat (rerere_path (name,"postimage" ), &st) ? (time_t )0 : st.st_mtime; } +/* +* Remove the recorded resolution for a given conflict ID +*/ static void unlink_rr_item (const char *name) {unlink (rerere_path (name,"thisimage" ));unlink (rerere_path (name,"preimage" ));unlink (rerere_path (name,"postimage" )); +/* +* NEEDSWORK: what if this rmdir() fails? Wouldn't we then +* assume that we already have preimage recorded in +* do_plain_rerere()? +*/ rmdir (git_path ("rr-cache/%s" , name)); }
@@ -880,6 +891,7 @@ void rerere_gc(struct string_list *rr) dir =opendir (git_path ("rr-cache" ));if (!dir)die_errno ("unable to open rr-cache directory" ); +/* Collect stale conflict IDs ... */ while ((e =readdir (dir))) {if (is_dot_or_dotdot (e->d_name))continue ;
@@ -897,11 +909,19 @@ void rerere_gc(struct string_list *rr)string_list_append (&to_remove, e->d_name); }closedir (dir); +/* ... and then remove them one-by-one */ for (i =0 ; i < to_remove.nr; i++)unlink_rr_item (to_remove.items[i].string);string_list_clear (&to_remove,0 ); } +/* +* During a conflict resolution, after "rerere" recorded the +* preimages, abandon them if the user did not resolve them or +* record their resolutions. And drop $GIT_DIR/MERGE_RR. +* +* NEEDSWORK: shouldn't we be calling this from "reset --hard"? +*/ void rerere_clear (struct string_list *merge_rr) {int i;
Signature · 1 line
2.5.0-rc0-209-g5e1f148
PATCH v2 13/13 rerere: refactor "replay" part of do_plain_rerere()
In reply to Junio C HamanoExtract the body of a loop that attempts to replay recorded resolution for each conflicted path into a helper function, not because I want to call it from multiple places later, but because the logic has become too deeply nested and hard to read.
--- rerere.c | 75 ++++++++++++++++++++++++++++++++++------------------------------ 1 file changed, 40 insertions(+), 35 deletions(-)
rerere.c +40 -35
@@ -620,6 +620,44 @@ static void update_paths(struct string_list *update)rollback_lock_file (&index_lock); } +/* +* The path indicated by rr_item may still have conflict for which we +* have a recorded resolution, in which case replay it and optionally +* update it. Or it may have been resolved by the user and we may +* only have the preimage for that conflict, in which case the result +* needs to be recorded as a resolution in a postimage file. +*/ +static void do_rerere_one_path (struct string_list_item *rr_item, +struct string_list *update) +{ +const char *path = rr_item->string; +const char *name = (const char *)rr_item->util; + +/* Is there a recorded resolution we could attempt to apply? */ +if (has_rerere_resolution (name)) { +if (merge (name, path)) +return ;/* failed to replay */ + +if (rerere_autoupdate) +string_list_insert (update, path); +else +fprintf (stderr, +"Resolved '%s' using previous resolution.\n" , + path); + goto mark_resolved; + } + +/* Let's see if the user has resolved it. */ +if (handle_file (path,NULL ,NULL )) +return ;/* not yet resolved */ + +copy_file (rerere_path (name,"postimage" ), path,0666 ); +fprintf (stderr,"Recorded resolution for '%s'.\n" , path); +mark_resolved: +free (rr_item->util); + rr_item->util =NULL ; +} +static int do_plain_rerere (struct string_list *rr,int fd) {struct string_list conflict =STRING_LIST_INIT_DUP ;
@@ -673,41 +711,8 @@ static int do_plain_rerere(struct string_list *rr, int fd) } } -/* -* Some of the paths that had conflicts earlier might have -* been resolved by the user. Others may be similar to a -* conflict already that was resolved before. -*/ -for (i =0 ; i < rr->nr; i++) { -int ret; -const char *path = rr->items[i].string; -const char *name = (const char *)rr->items[i].util; - -/* Is there a recorded resolution we could attempt to apply? */ -if (has_rerere_resolution (name)) { -if (merge (name, path)) -continue ; - -if (rerere_autoupdate) -string_list_insert (&update, path); -else -fprintf (stderr, -"Resolved '%s' using previous resolution.\n" , - path); - goto mark_resolved; - } - -/* Let's see if the user has resolved it. */ - ret =handle_file (path,NULL ,NULL ); -if (ret) -continue ; - -copy_file (rerere_path (name,"postimage" ), path,0666 ); -fprintf (stderr,"Recorded resolution for '%s'.\n" , path); - mark_resolved: -free (rr->items[i].util); - rr->items[i].util =NULL ; - } +for (i =0 ; i < rr->nr; i++) +do_rerere_one_path (&rr->items[i], &update);if (update.nr)update_paths (&update);
Signature · 1 line
2.5.0-rc0-209-g5e1f148
v3 cover "rerere" preparatory clean-up
In reply to Junio C HamanoThis is a resend of v2:
http://thread.gmane.org/gmane.comp.version-control.git/273117
plus 5 new changes. I have a few more real-fix changes that build on top, but they are not sufficiently polished to be published yet compared to these early clean-up bits.
Junio C Hamano (18):
rerere: fix an off-by-one non-bug
rerere: plug conflict ID leaks
rerere: lift PATH_MAX limitation
rerere: write out each record of MERGE_RR in one go
rerere: report autoupdated paths only after actually updating them
rerere: drop want_sp parameter from is_cmarker()
rerere: stop looping unnecessarily
rerere: explain the rerere I/O abstraction
rerere: explain MERGE_RR management helpers
rerere: explain the primary codepath
rerere: explain "rerere forget" codepath
rerere: explain the remainder
rerere: refactor "replay" part of do_plain_rerere()
rerere: further de-dent do_plain_rerere()
rerere: further clarify do_rerere_one_path()
rerere: call conflict-ids IDs
rerere: use "struct rerere_id" instead of "char *" for conflict ID
rerere: un-nest merge() further
builtin/rerere.c | 4 +- rerere.c | 544 ++++++++++++++++++++++++++++++++++++++++--------------- rerere.h | 12 +- 3 files changed, 407 insertions(+), 153 deletions(-)
Signature · 1 line
2.5.0-rc2-340-g0cccc16
PATCH v3 1/18 rerere: fix an off-by-one non-bug
When ac49f5ca (rerere "remaining", 2011-02-16) split out a new helper function check_one_conflict() out of find_conflict() function, so that the latter will use the returned value from the new helper to update the loop control variable that is an index into active_cache[], the new variable incremented the index by one too many when it found a path with only stage #1 entry at the very end of active_cache[].
This "strange" return value does not have any effect on the loop control of two callers of this function, as they all notice that active_nr+2 is larger than active_nr just like active_nr+1 is, but nevertheless it puzzles the readers when they are trying to figure out what the function is trying to do.
In fact, there is no need to do an early return. The code that follows after skipping the stage #1 entry is fully prepared to handle a case where the entry is at the very end of active_cache[].
Help future readers from unnecessary confusion by dropping an early return. We skip the stage #1 entry, and if there are stage #2 and stage #3 entries for the same path, we diagnose the path as THREE_STAGED (otherwise we say PUNTED), and then we skip all entries for the same path.
--- rerere.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
rerere.c +2 -4
@@ -369,10 +369,8 @@ static int check_one_conflict(int i, int *type) } *type =PUNTED ; -if (ce_stage (e) ==1 ) { -if (active_nr <= ++i) -return i +1 ; - } +if (ce_stage (e) ==1 ) + i++;/* Only handle regular files with both stages #2 and #3 */ if (i +1 < active_nr) {
Signature · 1 line
2.5.0-rc2-340-g0cccc16
Re: [PATCH v3 01/18] rerere: fix an off-by-one non-bug
Quoted 34 lines · Junio C Hamano
When
ac49f5ca(rerere "remaining", 2011-02-16) split out a new helper functioncheck_one_conflict()out offind_conflict()function, so that the latter will use the returned value from the new helper to update the loop control variable that is an index into active_cache[], the new variable incremented the index by one too many when it found a path with only stage #1 entry at the very end of active_cache[].This "strange" return value does not have any effect on the loop control of two callers of this function, as they all notice that active_nr+2 is larger than
active_nrjust like active_nr+1 is, but nevertheless it puzzles the readers when they are trying to figure out what the function is trying to do.In fact, there is no need to do an early return. The code that follows after skipping the stage #1 entry is fully prepared to handle a case where the entry is at the very end of active_cache[].
Help future readers from unnecessary confusion by dropping an early return. We skip the stage #1 entry, and if there are stage #2 and stage #3 entries for the same path, we diagnose the path as
THREE_STAGED(otherwise we say PUNTED), and then we skip all entries for the same path.Signed-off-by Junio C Hamano <[email protected]>--- rerere.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)rerere.c +0
-0
rerere.c static int check_one_conflict(int i, int *type) · line 373
Quoted 9 lines
rerere.c +2
-4@@ -369,10 +369,8 @@ static int check_one_conflict(int i, int *type) } *type =PUNTED ; -if (ce_stage (e) ==1 ) { -if (active_nr <= ++i) -return i +1 ; - } +if (ce_stage (e) ==1 ) + i++;
As a conflicted index can have multiple stage #1 entries when dealing with a criss-cross merge, this should probably be
while (ce_stage(e) == 1)
i++;instead.
Quoted 2 lines
rerere.c +0
-0/* Only handle regular files with both stages #2 and #3 */ if (i +1 < active_nr) {
PATCH v3 2/18 rerere: plug conflict ID leaks
In reply to Junio C HamanoThe merge_rr string list stores the conflict ID (a hexadecimal string that is used to index into $GIT_DIR/rr-cache) in the .util field of its elements, and when do_plain_rerere() resolves a conflict, the field is cleared. Also, when rerere_forget() recomputes the conflict ID to updates the preimage file, the conflict ID for the path is updated.
We forgot to free the existing conflict ID when we did these two operations.
--- rerere.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
rerere.c +5 -2
@@ -559,6 +559,7 @@ static int do_plain_rerere(struct string_list *rr, int fd)fprintf (stderr,"Recorded resolution for '%s'.\n" , path);copy_file (rerere_path (name,"postimage" ), path,0666 ); mark_resolved: +free (rr->items[i].util); rr->items[i].util =NULL ; }
@@ -627,6 +628,7 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)char *hex;unsigned char sha1[20 ];int ret; +struct string_list_item *item; ret =handle_cache (path, sha1,NULL );if (ret <1 )
@@ -641,8 +643,9 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)handle_cache (path, sha1,rerere_path (hex,"preimage" ));fprintf (stderr,"Updated preimage for '%s'\n" , path); - -string_list_insert (rr, path)->util = hex; + item =string_list_insert (rr, path); +free (item->util); + item->util = hex;fprintf (stderr,"Forgot resolution for %s\n" , path);return 0 ; }
Signature · 1 line
2.5.0-rc2-340-g0cccc16
PATCH v3 3/18 rerere: lift PATH_MAX limitation
In reply to Junio C HamanoThe MERGE_RR file records a collection of NUL-terminated entries, each of which consists of
- a hash that identifies the conflict
- a HT
- the pathname
We used to read this piece-by-piece, and worse yet, read the pathname part a byte at a time into a fixed buffer of size PATH_MAX.
Instead, read a whole entry using strbuf_getwholeline() and parse out the fields. This way, we issue fewer read(2) calls and more importantly we do not have to limit the pathname to PATH_MAX.
--- rerere.c | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-)
rerere.c +15 -20
@@ -35,32 +35,27 @@ static int has_rerere_resolution(const char *hex)static void read_rr (struct string_list *rr) { -unsigned char sha1[20 ]; -char buf[PATH_MAX ]; +struct strbuf buf =STRBUF_INIT ;FILE *in =fopen (merge_rr_path,"r" ); +if (!in)return ; -while (fread (buf,40 ,1 , in) ==1 ) { -int i; -char *name; -if (get_sha1_hex (buf, sha1)) +while (!strbuf_getwholeline (&buf, in,'\0' )) { +char *path; +unsigned char sha1[20 ]; + +/* There has to be the hash, tab, path and then NUL */ +if (buf.len <42 ||get_sha1_hex (buf.buf, sha1))die ("corrupt MERGE_RR" ); - buf[40 ] ='\0' ; - name =xstrdup (buf); -if (fgetc (in) !='\t' ) + +if (buf.buf[40 ] !='\t' )die ("corrupt MERGE_RR" ); -for (i =0 ; i <sizeof (buf); i++) { -int c =fgetc (in); -if (c <0 ) -die ("corrupt MERGE_RR" ); - buf[i] = c; -if (c ==0 ) -break ; - } -if (i ==sizeof (buf)) -die ("filename too long" ); -string_list_insert (rr, buf)->util = name; + buf.buf[40 ] ='\0' ; + path = buf.buf +41 ; + +string_list_insert (rr, path)->util =xstrdup (buf.buf); } +strbuf_release (&buf);fclose (in); }
Signature · 1 line
2.5.0-rc2-340-g0cccc16
PATCH v3 4/18 rerere: write out each record of MERGE_RR in one go
In reply to Junio C HamanoInstead of writing the hash for a conflict, a HT, and the path with three separate write_in_full() calls, format them into a single record into a strbuf and write it out in one go.
As a more recent "rerere remaining" codepath abuses the .util field of the merge_rr data to store a sentinel token, make sure that codepath does not call into this function (of course, "remaining" is a read-only operation and currently does not call it).
--- rerere.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
rerere.c +9 -7
@@ -65,16 +65,18 @@ static int write_rr(struct string_list *rr, int out_fd) {int i;for (i =0 ; i < rr->nr; i++) { -const char *path; -int length; +struct strbuf buf =STRBUF_INIT ; + +assert (rr->items[i].util !=RERERE_RESOLVED );if (!rr->items[i].util)continue ; - path = rr->items[i].string; - length =strlen (path) +1 ; -if (write_in_full (out_fd, rr->items[i].util,40 ) !=40 || -write_str_in_full (out_fd,"\t" ) !=1 || -write_in_full (out_fd, path, length) != length) +strbuf_addf (&buf,"%s\t%s%c" , + (char *)rr->items[i].util, + rr->items[i].string,0 ); +if (write_in_full (out_fd, buf.buf, buf.len) != buf.len)die ("unable to write rerere record" ); + +strbuf_release (&buf); }if (commit_lock_file (&write_lock) !=0 )die ("unable to write rerere record" );
Signature · 1 line
2.5.0-rc2-340-g0cccc16
PATCH v3 5/18 rerere: report autoupdated paths only after actually updating them
In reply to Junio C Hamano--- rerere.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
rerere.c +12 -10
@@ -482,6 +482,8 @@ static void update_paths(struct string_list *update)struct string_list_item *item = &update->items[i];if (add_file_to_cache (item->string,0 ))exit (128 ); +fprintf (stderr,"Staged '%s' using previous resolution.\n" , + item->string); }if (active_cache_changed) {
@@ -536,16 +538,16 @@ static int do_plain_rerere(struct string_list *rr, int fd)const char *name = (const char *)rr->items[i].util;if (has_rerere_resolution (name)) { -if (!merge (name, path)) { -const char *msg; -if (rerere_autoupdate) { -string_list_insert (&update, path); - msg ="Staged '%s' using previous resolution.\n" ; - }else - msg ="Resolved '%s' using previous resolution.\n" ; -fprintf (stderr, msg, path); - goto mark_resolved; - } +if (merge (name, path)) +continue ; + +if (rerere_autoupdate) +string_list_insert (&update, path); +else +fprintf (stderr, +"Resolved '%s' using previous resolution.\n" , + path); + goto mark_resolved; }/* Let's see if we have resolved it. */
Signature · 1 line
2.5.0-rc2-340-g0cccc16
PATCH v3 6/18 rerere: drop want_sp parameter from is_cmarker()
In reply to Junio C HamanoAs the nature of the conflict marker line determies if there should a SP and label after it, the caller shouldn't have to pass the parameter redundantly.
--- rerere.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
rerere.c +22 -5
@@ -148,8 +148,25 @@ static int rerere_file_getline(struct strbuf *sb, struct rerere_io *io_)return strbuf_getwholeline (sb, io->input,'\n' ); } -static int is_cmarker (char *buf,int marker_char,int marker_size,int want_sp) +/* +* Require the exact number of conflict marker letters, no more, no +* less, followed by SP or any whitespace +* (including LF). +*/ +static int is_cmarker (char *buf,int marker_char,int marker_size) { +int want_sp; + +/* +* The beginning of our version and the end of their version +* always are labeled like "<<<<< ours" or ">>>>> theirs", +* hence we set want_sp for them. Note that the version from +* the common ancestor in diff3-style output is not always +* labelled (e.g. "||||| common" is often seen but "|||||" +* alone is also valid), so we do not set want_sp. +*/ + want_sp = (marker_char =='<' ) || (marker_char =='>' ); +while (marker_size--)if (*buf++ != marker_char)return 0 ;
@@ -172,19 +189,19 @@ static int handle_path(unsigned char *sha1, struct rerere_io *io, int marker_sizgit_SHA1_Init (&ctx);while (!io->getline (&buf, io)) { -if (is_cmarker (buf.buf,'<' , marker_size,1 )) { +if (is_cmarker (buf.buf,'<' , marker_size)) {if (hunk !=RR_CONTEXT ) goto bad; hunk =RR_SIDE_1 ; - }else if (is_cmarker (buf.buf,'|' , marker_size,0 )) { + }else if (is_cmarker (buf.buf,'|' , marker_size)) {if (hunk !=RR_SIDE_1 ) goto bad; hunk =RR_ORIGINAL ; - }else if (is_cmarker (buf.buf,'=' , marker_size,0 )) { + }else if (is_cmarker (buf.buf,'=' , marker_size)) {if (hunk !=RR_SIDE_1 && hunk !=RR_ORIGINAL ) goto bad; hunk =RR_SIDE_2 ; - }else if (is_cmarker (buf.buf,'>' , marker_size,1 )) { + }else if (is_cmarker (buf.buf,'>' , marker_size)) {if (hunk !=RR_SIDE_2 ) goto bad;if (strbuf_cmp (&one, &two) >0 )
Signature · 1 line
2.5.0-rc2-340-g0cccc16
Re: [PATCH v3 06/18] rerere: drop want_sp parameter from is_cmarker()
From: "Junio C Hamano" <[email protected]>
Quoted 1 line
As the nature of the conflict marker line determies if there should
"should be a"?
i.e. s/a /be a / below
Quoted 74 lines
a SP and label after it, the caller shouldn't have to pass the parameter redundantly.
Signed-off-by Junio C Hamano <[email protected]>---
rerere.c| 27 ++++++++++++++++++++++-----1 file changed, 22 insertions(+), 5 deletions(-)rerere.c +18
-1@@ -148,8 +148,25 @@ static int rerere_file_getline(struct strbuf *sb, struct rerere_io *io_)return strbuf_getwholeline (sb, io->input,'\n' ); } -static int is_cmarker (char *buf,int marker_char,int marker_size, int want_sp) +/* +* Require the exact number of conflict marker letters, no more, no +* less, followed by SP or any whitespace +* (including LF). +*/ +static int is_cmarker (char *buf,int marker_char,int marker_size) { +int want_sp; + +/* +* The beginning of our version and the end of their version +* always are labeled like "<<<<< ours" or ">>>>> theirs", +* hence we set want_sp for them. Note that the version from +* the common ancestor in diff3-style output is not always +* labelled (e.g. "||||| common" is often seen but "|||||" +* alone is also valid), so we do not set want_sp. +*/ + want_sp = (marker_char =='<' ) || (marker_char =='>' ); +while (marker_size--)if (*buf++ !=
marker_char)
return 0;rerere.c +0
-0@@ -172,19 +189,19 @@ static int handle_path(unsigned char *sha1,struct
rerere_io*io, intmarker_siz
git_SHA1_Init(&ctx);while (!io->getline(&buf, io)) {
- if (is_cmarker(buf.buf, '<', marker_size, 1)) {
+ if (is_cmarker(buf.buf, '<', marker_size)) { if (hunk !=RR_CONTEXT)
goto bad;
hunk =RR_SIDE_1;
- } else if (is_cmarker(buf.buf, '|', marker_size, 0)) {
+ } else if (is_cmarker(buf.buf, '|', marker_size)) { if (hunk !=RR_SIDE_1)
goto bad;
hunk =RR_ORIGINAL;
- } else if (is_cmarker(buf.buf, '=', marker_size, 0)) {
+ } else if (is_cmarker(buf.buf, '=', marker_size)) { if (hunk !=RR_SIDE_1&& hunk !=RR_ORIGINAL) goto bad;
hunk =RR_SIDE_2;
- } else if (is_cmarker(buf.buf, '>', marker_size, 1)) {
+ } else if (is_cmarker(buf.buf, '>', marker_size)) { if (hunk !=RR_SIDE_2)
goto bad;
if (strbuf_cmp(&one, &two)> 0)Signature · 6 lines
2.5.0-rc2-340-g0cccc16 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 06/18] rerere: drop want_sp parameter from is_cmarker()
Quoted 5 lines · Philip Oakley
From: "Junio C Hamano" <[email protected]>
Quoted 1 line
As the nature of the conflict marker line determies if there should
"should be a"?
i.e. s/a /be a / below
Also: s/determies/determines/
PATCH v3 7/18 rerere: stop looping unnecessarily
In reply to Junio C Hamanohandle_cache() loops 3 times starting from an index entry that is unmerged, while ignoring an entry for a path that is different from what we are looking for.
As the index is sorted, once we see a different path, we know we saw all stages for the path we are interested in. Just loop while we see the same path and then break, instead of continuing for 3 times.
--- rerere.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
rerere.c +7 -10
@@ -329,24 +329,21 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpureturn -1 ; pos = -pos -1 ; -for (i =0 ; i <3 ; i++) { +while (pos < active_nr) {enum object_type type;unsigned long size; -int j; -if (active_nr <= pos) -break ; ce = active_cache[pos++];if (ce_namelen (ce) != len ||memcmp (ce->name, path, len)) -continue ; - j =ce_stage (ce) -1 ; - mmfile[j].ptr =read_sha1_file (ce->sha1, &type, &size); - mmfile[j].size = size; +break ; + i =ce_stage (ce) -1 ; + mmfile[i].ptr =read_sha1_file (ce->sha1, &type, &size); + mmfile[i].size = size; } -for (i =0 ; i <3 ; i++) { +for (i =0 ; i <3 ; i++)if (!mmfile[i].ptr && !mmfile[i].size) mmfile[i].ptr =xstrdup ("" ); - } +/* * NEEDSWORK: handle conflicts from merges with * merge.renormalize set, too
Signature · 1 line
2.5.0-rc2-340-g0cccc16
Re: [PATCH v3 07/18] rerere: stop looping unnecessarily
Quoted 29 lines · Junio C Hamano
handle_cache()loops 3 times starting from an index entry that is unmerged, while ignoring an entry for a path that is different from what we are looking for.As the index is sorted, once we see a different path, we know we saw all stages for the path we are interested in. Just loop while we see the same path and then break, instead of continuing for 3 times.
Signed-off-by Junio C Hamano <[email protected]>--- rerere.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)rerere.c +1
-4@@ -329,24 +329,21 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpureturn -1 ; pos = -pos -1 ; -for (i =0 ; i <3 ; i++) { +while (pos < active_nr) {enum object_type type;unsigned long size; -int j; -if (active_nr <= pos) -break ;
rerere.c static int handle_cache(const char *path, unsigned char *sha1, const char *outpu · line 341
Quoted 10 lines
rerere.c +4
-4ce = active_cache[pos++];if (ce_namelen (ce) != len ||memcmp (ce->name, path, len)) -continue ; - j =ce_stage (ce) -1 ; - mmfile[j].ptr =read_sha1_file (ce->sha1, &type, &size); - mmfile[j].size = size; +break ; + i =ce_stage (ce) -1 ; + mmfile[i].ptr =read_sha1_file (ce->sha1, &type, &size); + mmfile[i].size = size;
If the conflicted index has multiple stage #1 entries, this will leak what was previously read. As the array is initialized to NULLs, perhaps
i = ce_stage(ce) - 1;
if (!mmfile[i].ptr) {
mmfile[i].ptr = read_sha1_file(ce->sha1, &type, &size);
mmfile[i].size = size;
}should be sufficient. This does change the semantics, in that we used to use the last stage #1 entry as the common ancestor when doing the plain-vanilla three-way merge, but with the leak fix, we will use the first stage #1 entry. But it does not change it in any meaningful way---we are using only one of multiple stage #1 entries arbitrarily picked either way.
Quoted 10 lines
rerere.c +2
-2} -for (i =0 ; i <3 ; i++) { +for (i =0 ; i <3 ; i++)if (!mmfile[i].ptr && !mmfile[i].size) mmfile[i].ptr =xstrdup ("" ); - } +/* * NEEDSWORK: handle conflicts from merges with * merge.renormalize set, too
PATCH v3 8/18 rerere: explain the rerere I/O abstraction
In reply to Junio C HamanoExplain the internals of rerere as in-code comments.
This one covers our thin I/O abstraction to read from either a file or a memory while optionally writing out to a file.
--- rerere.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)
rerere.c +31 -7
@@ -83,6 +83,21 @@ static int write_rr(struct string_list *rr, int out_fd)return 0 ; } +/* +* "rerere" interacts with conflicted file contents using this I/O +* abstraction. It reads a conflicted contents from one place via +* "getline()" method, and optionally can write it out after +* normalizing the conflicted hunks to the "output". Subclasses of +* rerere_io embed this structure at the beginning of their own +* rerere_io object. +*/ +struct rerere_io { +int (*getline)(struct strbuf *,struct rerere_io *); +FILE *output; +int wrerror; +/* some more stuff */ +}; +static void ferr_write (const void *p,size_t count,FILE *fp,int *err) {if (!count || *err)
@@ -96,19 +111,15 @@ static inline void ferr_puts(const char *s, FILE *fp, int *err)ferr_write (s,strlen (s), fp, err); } -struct rerere_io { -int (*getline)(struct strbuf *,struct rerere_io *); -FILE *output; -int wrerror; -/* some more stuff */ -}; -static void rerere_io_putstr (const char *str,struct rerere_io *io) {if (io->output)ferr_puts (str, io->output, &io->wrerror); } +/* +* Write a conflict marker to io->output (if defined). +*/ static void rerere_io_putconflict (int ch,int size,struct rerere_io *io) {char buf[64 ];
@@ -137,11 +148,17 @@ static void rerere_io_putmem(const char *mem, size_t sz, struct rerere_io *io)ferr_write (mem, sz, io->output, &io->wrerror); } +/* +* Subclass of rerere_io that reads from an on-disk file +*/ struct rerere_io_file {struct rerere_io io;FILE *input; }; +/* +* ... and its getline() method implementation +*/ static int rerere_file_getline (struct strbuf *sb,struct rerere_io *io_) {struct rerere_io_file *io = (struct rerere_io_file *)io_;
@@ -286,11 +303,18 @@ static int handle_file(const char *path, unsigned char *sha1, const char *outputreturn hunk_no; } +/* +* Subclass of rerere_io that reads from an in-core buffer that is a +* strbuf +*/ struct rerere_io_mem {struct rerere_io io;struct strbuf input; }; +/* +* ... and its getline() method implementation +*/ static int rerere_mem_getline (struct strbuf *sb,struct rerere_io *io_) {struct rerere_io_mem *io = (struct rerere_io_mem *)io_;
Signature · 1 line
2.5.0-rc2-340-g0cccc16
Re: [PATCH v3 08/18] rerere: explain the rerere I/O abstraction
rerere.c static inline void ferr_puts(const char *s, FILE *fp, int *err) · line 125
Quoted 6 lines · Junio C Hamano
rerere.c +3
-0+/* +* Write a conflict marker to io->output (if defined). +*/ static void rerere_io_putconflict (int ch,int size,struct rerere_io *io) {char buf[64 ];
This is unrelated to the topic of this step, but this function seems to be very poorly put together with duct tape.
Quoted 10 lines
static void
rerere_io_putconflict(int ch, int size, struct rerere_io *io){ char buf[64]; while (size) { if (size < sizeof(buf) - 2) { memset(buf, ch, size); buf[size] = '\n'; buf[size + 1] = '\0'; size = 0;
The if() guarding this block has an off-by-one error in the benign direction. When size is 62, the marker with terminating LF and NUL should still fit within the buf[], so it should use "<=", not "<", to compare.
Quoted 7 lines
} else {
int sz = sizeof(buf) - 1;
if (size <= sz)
sz -= (sz - size) + 1;
memset(buf, ch, sz);
buf[sz] = '\0';
size -= sz;This is an even more awkward construct. sz is what we have to work with the substring that we cannot emit with a single call (because buf[] is too small), so naturally I would expect it to be more like
int to_emit = size;
if (sz <= size)
to_emit = sz;
memset(buf, ch, to_emit);
buf[to_emit] = '\0';
size -= to_emit;which makes the "if (size <= sz)" in the code look very suspicious.
But rewriting it to the "more natural" version would make it buggy. At a right boundary condition, i.e. size == 63, we may find that the conflict marker is too long with LF and NUL to fit in buf[] and come here, and then fill the full conflict marker with NUL just fine in buf[], decrementing size to 0, emit that 63-byte long marker. The next iteration will exit the loop without emitting the LF.
The unnatural is what is saving us from such a bug.
if (size <= sz) sz -= (sz - size) + 1;
It ensures that subtraction of sz (i.e. "to_emit") from size before the next iteration will never brings size down to zero by reducing sz by one too much, forcing another iteration, which will then have size smaller than "sizeof(buf) - 2" and have us emit the LF.
Not buggy, but ugly.
Quoted 4 lines
} rerere_io_putstr(buf, io); } }
PATCH v3 9/18 rerere: explain MERGE_RR management helpers
In reply to Junio C HamanoExplain the internals of rerere as in-code comments, while sprinkling "NEEDSWORK" comment to highlight iffy bits and questionable assumptions.
This one covers the "$GIT_DIR/MERGE_RR" file and in-core merge_rr that are used to keep track of the status of "rerere" session in progress.
--- rerere.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
rerere.c +41 -0
@@ -33,6 +33,13 @@ static int has_rerere_resolution(const char *hex)return !stat (rerere_path (hex,"postimage" ), &st); } +/* +* $GIT_DIR/MERGE_RR file is a collection of records, each of which is +* "conflict ID", a HT and pathname, terminated with a NUL, and is +* used to keep track of the set of paths that "rerere" may need to +* work on (i.e. what is left by the previous invocation of "git +* rerere" during the current conflict resolution session). +*/ static void read_rr (struct string_list *rr) {struct strbuf buf =STRBUF_INIT ;
@@ -394,6 +401,14 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpureturn hunk_no; } +/* +* Look at a cache entry at "i" and see if it is not conflicting, +* conflicting and we are willing to handle, or conflicting and +* we are unable to handle, and return the determination in *type. +* Return the cache index to be looked at next, by skipping the +* stages we have already looked at in this invocation of this +* function. +*/ static int check_one_conflict (int i,int *type) {const struct cache_entry *e = active_cache[i];
@@ -425,6 +440,17 @@ static int check_one_conflict(int i, int *type)return i; } +/* +* Scan the index and find paths that have conflicts that rerere can +* handle, i.e. the ones that has both stages #2 and #3. +* +* NEEDSWORK: we do not record or replay a previous "resolve by +* deletion" for a delete-modify conflict, as that is inherently risky +* without knowing what modification is being discarded. The only +* safe case, i.e. both side doing the deletion and modification that +* are identical to the previous round, might want to be handled, +* though. +*/ static int find_conflict (struct string_list *conflict) {int i;
@@ -441,6 +467,21 @@ static int find_conflict(struct string_list *conflict)return 0 ; } +/* +* The merge_rr list is meant to hold outstanding conflicted paths +* that rerere could handle. Abuse the list by adding other types of +* entries to allow the caller to show "rerere remaining". +* +* - Conflicted paths that rerere does not handle are added +* - Conflicted paths that have been resolved are marked as such +* by storing RERERE_RESOLVED to .util field (where conflict ID +* is expected to be stored). +* +* Do *not* write MERGE_RR file out after calling this function. +* +* NEEDSWORK: we may want to fix the caller that implements "rerere +* remaining" to do this without abusing merge_rr. +*/ int rerere_remaining (struct string_list *merge_rr) {int i;
Signature · 1 line
2.5.0-rc2-340-g0cccc16
PATCH v3 10/18 rerere: explain the primary codepath
In reply to Junio C HamanoExplain the internals of rerere as in-code comments, while sprinkling "NEEDSWORK" comment to highlight iffy bits and questionable assumptions.
This one covers the codepath reached from rerere(), the primary interface to the subsystem.
--- rerere.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 82 insertions(+), 13 deletions(-)
rerere.c +82 -13
@@ -199,6 +199,21 @@ static int is_cmarker(char *buf, int marker_char, int marker_size)return isspace (*buf); } +/* +* Read contents a file with conflicts, normalize the conflicts +* by (1) discarding the common ancestor version in diff3-style, +* (2) reordering our side and their side so that whichever sorts +* alphabetically earlier comes before the other one, while +* computing the "conflict ID", which is just an SHA-1 hash of +* one side of the conflict, NUL, the other side of the conflict, +* and NUL concatenated together. +* +* Return the number of conflict hunks found. +* +* NEEDSWORK: the logic and theory of operation behind this conflict +* normalization may deserve to be documented somewhere, perhaps in +* Documentation/technical/rerere.txt. +*/ static int handle_path (unsigned char *sha1,struct rerere_io *io,int marker_size) {git_SHA_CTX ctx;
@@ -269,6 +284,10 @@ static int handle_path(unsigned char *sha1, struct rerere_io *io, int marker_sizreturn hunk_no; } +/* +* Scan the path for conflicts, do the "handle_path()" thing above, and +* return the number of conflict hunks found. +*/ static int handle_file (const char *path,unsigned char *sha1,const char *output) {int hunk_no =0 ;
@@ -506,29 +525,54 @@ int rerere_remaining(struct string_list *merge_rr)return 0 ; } +/* +* Find the conflict identified by "name"; the change between its +* "preimage" (i.e. a previous contents with conflict markers) and its +* "postimage" (i.e. the corresponding contents with conflicts +* resolved) may apply cleanly to the contents stored in "path", i.e. +* the conflict this time around. +* +* Returns 0 for successful replay of recorded resolution, or non-zero +* for failure. +*/ static int merge (const char *name,const char *path) {int ret;mmfile_t cur = {NULL ,0 }, base = {NULL ,0 }, other = {NULL ,0 };mmbuffer_t result = {NULL ,0 }; +/* +* Normalize the conflicts in path and write it out to +* "thisimage" temporary file. +*/ if (handle_file (path,NULL ,rerere_path (name,"thisimage" )) <0 )return 1 ;if (read_mmfile (&cur,rerere_path (name,"thisimage" )) || -read_mmfile (&base,rerere_path (name,"preimage" )) || -read_mmfile (&other,rerere_path (name,"postimage" ))) { +read_mmfile (&base,rerere_path (name,"preimage" )) || +read_mmfile (&other,rerere_path (name,"postimage" ))) { ret =1 ; goto out; } + +/* +* A three-way merge. Note that this honors user-customizable +* low-level merge driver settings. +*/ ret =ll_merge (&result, path, &base,NULL , &cur,"" , &other,"" ,NULL );if (!ret) {FILE *f; +/* +* A successful replay of recorded resolution. +* Mark that "postimage" was used to help gc. +*/ if (utime (rerere_path (name,"postimage" ),NULL ) <0 )warning ("failed utime() on %s: %s" ,rerere_path (name,"postimage" ),strerror (errno)); + +/* Update "path" with the resolution */ f =fopen (path,"w" );if (!f) return error("Could not open %s: %s", path,
@@ -581,41 +625,61 @@ static int do_plain_rerere(struct string_list *rr, int fd)find_conflict (&conflict);/* -* MERGE_RR records paths with conflicts immediately after merge -* failed. Some of the conflicted paths might have been hand resolved -* in the working tree since then, but the initial run would catch all -* and register their preimages. +* MERGE_RR records paths with conflicts immediately after +* merge failed. Some of the conflicted paths might have been +* hand resolved in the working tree since then, but the +* initial run would catch all and register their preimages. */ -for (i =0 ; i < conflict.nr; i++) {const char *path = conflict.items[i].string;if (!string_list_has_string (rr, path)) {unsigned char sha1[20 ];char *hex;int ret; + +/* +* Ask handle_file() to scan and assign a +* conflict ID. No need to write anything out +* yet. +*/ ret =handle_file (path, sha1,NULL );if (ret <1 )continue ; hex =xstrdup (sha1_to_hex (sha1));string_list_insert (rr, path)->util = hex; + +/* +* If the directory does not exist, create +* it. mkdir_in_gitdir() will fail with +* EEXIST if there already is one. +* +* NEEDSWORK: make sure "gc" does not remove +* preimage without removing the directory. +*/ if (mkdir_in_gitdir (git_path ("rr-cache/%s" , hex)))continue ; + +/* +* We are the first to encounter this +* conflict. Ask handle_file() to write the +* normalized contents to the "preimage" file. +*/ handle_file (path,NULL ,rerere_path (hex,"preimage" ));fprintf (stderr,"Recorded preimage for '%s'\n" , path); } }/* -* Now some of the paths that had conflicts earlier might have been -* hand resolved. Others may be similar to a conflict already that -* was resolved before. +* Some of the paths that had conflicts earlier might have +* been resolved by the user. Others may be similar to a +* conflict already that was resolved before. */ -for (i =0 ; i < rr->nr; i++) {int ret;const char *path = rr->items[i].string;const char *name = (const char *)rr->items[i].util; +/* Is there a recorded resolution we could attempt to apply? */ if (has_rerere_resolution (name)) {if (merge (name, path))continue ;
@@ -629,13 +693,13 @@ static int do_plain_rerere(struct string_list *rr, int fd) goto mark_resolved; } -/* Let's see if we have resolved it. */ +/* Let's see if the user has resolved it. */ ret =handle_file (path,NULL ,NULL );if (ret)continue ; -fprintf (stderr,"Recorded resolution for '%s'.\n" , path);copy_file (rerere_path (name,"postimage" ), path,0666 ); +fprintf (stderr,"Recorded resolution for '%s'.\n" , path); mark_resolved:free (rr->items[i].util); rr->items[i].util =NULL ;
@@ -689,6 +753,11 @@ int setup_rerere(struct string_list *merge_rr, int flags)return fd; } +/* +* The main entry point that is called internally from codepaths that +* perform mergy operations, possibly leaving conflicted index entries +* and working tree files. +*/ int rerere (int flags) {struct string_list merge_rr =STRING_LIST_INIT_DUP ;
Signature · 1 line
2.5.0-rc2-340-g0cccc16
PATCH v3 11/18 rerere: explain "rerere forget" codepath
In reply to Junio C HamanoExplain the internals of rerere as in-code comments, while sprinkling "NEEDSWORK" comment to highlight iffy bits and questionable assumptions.
This covers the codepath that implements "rerere forget".
--- rerere.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
rerere.c +24 -0
@@ -413,6 +413,10 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpustrbuf_init (&io.input,0 );strbuf_attach (&io.input, result.ptr, result.size, result.size); +/* +* Grab the conflict ID and optionally write the original +* contents with conflict markers out. +*/ hunk_no =handle_path (sha1, (struct rerere_io *)&io, marker_size);strbuf_release (&io.input);if (io.io.output)
@@ -777,9 +781,15 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr)int ret;struct string_list_item *item; +/* +* Recreate the original conflict from the stages in the +* index and compute the conflict ID +*/ ret =handle_cache (path, sha1,NULL );if (ret <1 )return error ("Could not parse conflict hunks in '%s'" , path); + +/* Nuke the recorded resolution for the conflict */ hex =xstrdup (sha1_to_hex (sha1)); filename =rerere_path (hex,"postimage" );if (unlink (filename))
@@ -787,9 +797,18 @@ static int rerere_forget_one_path(const char *path, struct string_list *rr) ?error ("no remembered resolution for %s" , path) : error("cannot unlink %s: %s", filename, strerror(errno))); +/* +* Update the preimage so that the user can resolve the +* conflict in the working tree, run us again to record +* the postimage. +*/ handle_cache (path, sha1,rerere_path (hex,"preimage" ));fprintf (stderr,"Updated preimage for '%s'\n" , path); +/* +* And remember that we can record resolution for this +* conflict when the user is done. +*/ item =string_list_insert (rr, path);free (item->util); item->util = hex;
@@ -808,6 +827,11 @@ int rerere_forget(struct pathspec *pathspec) fd =setup_rerere (&merge_rr,RERERE_NOAUTOUPDATE ); +/* +* The paths may have been resolved (incorrectly); +* recover the original conflicted state and then +* find the conflicted paths. +*/ unmerge_cache (pathspec);find_conflict (&conflict);for (i =0 ; i < conflict.nr; i++) {
Signature · 1 line
2.5.0-rc2-340-g0cccc16
PATCH v3 12/18 rerere: explain the remainder
In reply to Junio C HamanoExplain the internals of rerere as in-code comments, while sprinkling "NEEDSWORK" comment to highlight iffy bits and questionable assumptions.
This covers the codepath that implements "rerere gc" and "rerere clear".
--- rerere.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
rerere.c +20 -0
@@ -844,6 +844,9 @@ int rerere_forget(struct pathspec *pathspec)return write_rr (&merge_rr, fd); } +/* +* Garbage collection support +*/ static time_t rerere_created_at (const char *name) {struct stat st;
@@ -856,11 +859,19 @@ static time_t rerere_last_used_at(const char *name)return stat (rerere_path (name,"postimage" ), &st) ? (time_t )0 : st.st_mtime; } +/* +* Remove the recorded resolution for a given conflict ID +*/ static void unlink_rr_item (const char *name) {unlink (rerere_path (name,"thisimage" ));unlink (rerere_path (name,"preimage" ));unlink (rerere_path (name,"postimage" )); +/* +* NEEDSWORK: what if this rmdir() fails? Wouldn't we then +* assume that we already have preimage recorded in +* do_plain_rerere()? +*/ rmdir (git_path ("rr-cache/%s" , name)); }
@@ -880,6 +891,7 @@ void rerere_gc(struct string_list *rr) dir =opendir (git_path ("rr-cache" ));if (!dir)die_errno ("unable to open rr-cache directory" ); +/* Collect stale conflict IDs ... */ while ((e =readdir (dir))) {if (is_dot_or_dotdot (e->d_name))continue ;
@@ -897,11 +909,19 @@ void rerere_gc(struct string_list *rr)string_list_append (&to_remove, e->d_name); }closedir (dir); +/* ... and then remove them one-by-one */ for (i =0 ; i < to_remove.nr; i++)unlink_rr_item (to_remove.items[i].string);string_list_clear (&to_remove,0 ); } +/* +* During a conflict resolution, after "rerere" recorded the +* preimages, abandon them if the user did not resolve them or +* record their resolutions. And drop $GIT_DIR/MERGE_RR. +* +* NEEDSWORK: shouldn't we be calling this from "reset --hard"? +*/ void rerere_clear (struct string_list *merge_rr) {int i;
Signature · 1 line
2.5.0-rc2-340-g0cccc16
PATCH v3 13/18 rerere: refactor "replay" part of do_plain_rerere()
In reply to Junio C HamanoExtract the body of a loop that attempts to replay recorded resolution for each conflicted path into a helper function, not because I want to call it from multiple places later, but because the logic has become too deeply nested and hard to read.
--- rerere.c | 75 ++++++++++++++++++++++++++++++++++------------------------------ 1 file changed, 40 insertions(+), 35 deletions(-)
rerere.c +40 -35
@@ -620,6 +620,44 @@ static void update_paths(struct string_list *update)rollback_lock_file (&index_lock); } +/* +* The path indicated by rr_item may still have conflict for which we +* have a recorded resolution, in which case replay it and optionally +* update it. Or it may have been resolved by the user and we may +* only have the preimage for that conflict, in which case the result +* needs to be recorded as a resolution in a postimage file. +*/ +static void do_rerere_one_path (struct string_list_item *rr_item, +struct string_list *update) +{ +const char *path = rr_item->string; +const char *name = (const char *)rr_item->util; + +/* Is there a recorded resolution we could attempt to apply? */ +if (has_rerere_resolution (name)) { +if (merge (name, path)) +return ;/* failed to replay */ + +if (rerere_autoupdate) +string_list_insert (update, path); +else +fprintf (stderr, +"Resolved '%s' using previous resolution.\n" , + path); + goto mark_resolved; + } + +/* Let's see if the user has resolved it. */ +if (handle_file (path,NULL ,NULL )) +return ;/* not yet resolved */ + +copy_file (rerere_path (name,"postimage" ), path,0666 ); +fprintf (stderr,"Recorded resolution for '%s'.\n" , path); +mark_resolved: +free (rr_item->util); + rr_item->util =NULL ; +} +static int do_plain_rerere (struct string_list *rr,int fd) {struct string_list conflict =STRING_LIST_INIT_DUP ;
@@ -673,41 +711,8 @@ static int do_plain_rerere(struct string_list *rr, int fd) } } -/* -* Some of the paths that had conflicts earlier might have -* been resolved by the user. Others may be similar to a -* conflict already that was resolved before. -*/ -for (i =0 ; i < rr->nr; i++) { -int ret; -const char *path = rr->items[i].string; -const char *name = (const char *)rr->items[i].util; - -/* Is there a recorded resolution we could attempt to apply? */ -if (has_rerere_resolution (name)) { -if (merge (name, path)) -continue ; - -if (rerere_autoupdate) -string_list_insert (&update, path); -else -fprintf (stderr, -"Resolved '%s' using previous resolution.\n" , - path); - goto mark_resolved; - } - -/* Let's see if the user has resolved it. */ - ret =handle_file (path,NULL ,NULL ); -if (ret) -continue ; - -copy_file (rerere_path (name,"postimage" ), path,0666 ); -fprintf (stderr,"Recorded resolution for '%s'.\n" , path); - mark_resolved: -free (rr->items[i].util); - rr->items[i].util =NULL ; - } +for (i =0 ; i < rr->nr; i++) +do_rerere_one_path (&rr->items[i], &update);if (update.nr)update_paths (&update);
Signature · 1 line
2.5.0-rc2-340-g0cccc16
PATCH v3 14/18 rerere: further de-dent do_plain_rerere()
In reply to Junio C HamanoIt's just easier to follow this way.
--- rerere.c | 69 ++++++++++++++++++++++++++++++++-------------------------------- 1 file changed, 35 insertions(+), 34 deletions(-)
rerere.c +35 -34
@@ -673,42 +673,43 @@ static int do_plain_rerere(struct string_list *rr, int fd)* initial run would catch all and register their preimages. */ for (i =0 ; i < conflict.nr; i++) { +unsigned char sha1[20 ]; +char *hex; +int ret;const char *path = conflict.items[i].string; -if (!string_list_has_string (rr, path)) { -unsigned char sha1[20 ]; -char *hex; -int ret; - -/* -* Ask handle_file() to scan and assign a -* conflict ID. No need to write anything out -* yet. -*/ - ret =handle_file (path, sha1,NULL ); -if (ret <1 ) -continue ; - hex =xstrdup (sha1_to_hex (sha1)); -string_list_insert (rr, path)->util = hex; - -/* -* If the directory does not exist, create -* it. mkdir_in_gitdir() will fail with -* EEXIST if there already is one. -* -* NEEDSWORK: make sure "gc" does not remove -* preimage without removing the directory. -*/ -if (mkdir_in_gitdir (git_path ("rr-cache/%s" , hex))) -continue ; -/* -* We are the first to encounter this -* conflict. Ask handle_file() to write the -* normalized contents to the "preimage" file. -*/ -handle_file (path,NULL ,rerere_path (hex,"preimage" )); -fprintf (stderr,"Recorded preimage for '%s'\n" , path); - } +if (string_list_has_string (rr, path)) +continue ; + +/* +* Ask handle_file() to scan and assign a +* conflict ID. No need to write anything out +* yet. +*/ + ret =handle_file (path, sha1,NULL ); +if (ret <1 ) +continue ; + hex =xstrdup (sha1_to_hex (sha1)); +string_list_insert (rr, path)->util = hex; + +/* +* If the directory does not exist, create +* it. mkdir_in_gitdir() will fail with +* EEXIST if there already is one. +* +* NEEDSWORK: make sure "gc" does not remove +* preimage without removing the directory. +*/ +if (mkdir_in_gitdir (git_path ("rr-cache/%s" , hex))) +continue ; + +/* +* We are the first to encounter this +* conflict. Ask handle_file() to write the +* normalized contents to the "preimage" file. +*/ +handle_file (path,NULL ,rerere_path (hex,"preimage" )); +fprintf (stderr,"Recorded preimage for '%s'\n" , path); }for (i =0 ; i < rr->nr; i++)
Signature · 1 line
2.5.0-rc2-340-g0cccc16
PATCH v3 15/18 rerere: further clarify do_rerere_one_path()
In reply to Junio C Hamano--- rerere.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
rerere.c +6 -9
@@ -644,16 +644,13 @@ static void do_rerere_one_path(struct string_list_item *rr_item,fprintf (stderr,"Resolved '%s' using previous resolution.\n" , path); - goto mark_resolved; + }else if (!handle_file (path,NULL ,NULL )) { +/* The user has resolved it. */ +copy_file (rerere_path (name,"postimage" ), path,0666 ); +fprintf (stderr,"Recorded resolution for '%s'.\n" , path); + }else { +return ; } - -/* Let's see if the user has resolved it. */ -if (handle_file (path,NULL ,NULL )) -return ;/* not yet resolved */ - -copy_file (rerere_path (name,"postimage" ), path,0666 ); -fprintf (stderr,"Recorded resolution for '%s'.\n" , path); -mark_resolved:free (rr_item->util); rr_item->util =NULL ; }
Signature · 1 line
2.5.0-rc2-340-g0cccc16
PATCH v3 16/18 rerere: call conflict-ids IDs
In reply to Junio C HamanoMost places we call conflict IDs "name" and some others we call them "hex"; update all of them to "id".
--- builtin/rerere.c | 4 +-- rerere.c | 76 ++++++++++++++++++++++++++++---------------------------- rerere.h | 2 +- 3 files changed, 41 insertions(+), 41 deletions(-)
builtin/rerere.c +2 -2
@@ -103,8 +103,8 @@ int cmd_rerere(int argc, const char **argv, const char *prefix) }else if (!strcmp (argv[0 ],"diff" ))for (i =0 ; i < merge_rr.nr; i++) {const char *path = merge_rr.items[i].string; -const char *name = (const char *)merge_rr.items[i].util; -diff_two (rerere_path (name,"preimage" ), path, path, path); +const char *id = (const char *)merge_rr.items[i].util; +diff_two (rerere_path (id,"preimage" ), path, path, path); }else usage_with_options (rerere_usage, options);
rerere.c +38 -38
205 lines not on this page · raw message
rerere.h +1 -1
@@ -17,7 +17,7 @@ extern void *RERERE_RESOLVED;extern int setup_rerere (struct string_list *,int );extern int rerere (int ); -extern const char *rerere_path (const char *hex,const char *file); +extern const char *rerere_path (const char *id,const char *file);extern int rerere_forget (struct pathspec *);extern int rerere_remaining (struct string_list *);extern void rerere_clear (struct string_list *);
Signature · 1 line
2.5.0-rc2-340-g0cccc16
PATCH v3 17/18 rerere: use "struct rerere_id" instead of "char *" for conflict ID
In reply to Junio C HamanoThis gives a thin abstraction between the conflict ID that is a hash value obtained by inspecting the conflicts and the name of the directory under $GIT_DIR/rr-cache/, in which the previous resolution is recorded to be replayed. The plan is to make sure that the presense of the directory does not imply the presense of a previous resolution and vice-versa, and later allow us to have more than one pair of <preimage, postimage> for a given conflict ID.
--- builtin/rerere.c | 2 +- rerere.c | 99 +++++++++++++++++++++++++++++++++++++++++--------------- rerere.h | 12 ++++++- 3 files changed, 85 insertions(+), 28 deletions(-)
builtin/rerere.c +1 -1
@@ -103,7 +103,7 @@ int cmd_rerere(int argc, const char **argv, const char *prefix) }else if (!strcmp (argv[0 ],"diff" ))for (i =0 ; i < merge_rr.nr; i++) {const char *path = merge_rr.items[i].string; -const char *id = (const char *)merge_rr.items[i].util; +const struct rerere_id *id = merge_rr.items[i].util;diff_two (rerere_path (id,"preimage" ), path, path, path); }else
rerere.c +73 -26
240 lines not on this page · raw message
rerere.h +11 -1
@@ -15,9 +15,19 @@ struct pathspec;*/ extern void *RERERE_RESOLVED ; +struct rerere_id { +char hex[41 ]; +}; +extern int setup_rerere (struct string_list *,int );extern int rerere (int ); -extern const char *rerere_path (const char *id,const char *file); +/* +* Given the conflict ID and the name of a "file" used for replaying +* the recorded resolution (e.g. "preimage", "postimage"), return the +* path to that filesystem entity. With "file" specified with NULL, +* return the path to the directory that houses these files. +*/ +extern const char *rerere_path (const struct rerere_id *,const char *file);extern int rerere_forget (struct pathspec *);extern int rerere_remaining (struct string_list *);extern void rerere_clear (struct string_list *);
Signature · 1 line
2.5.0-rc2-340-g0cccc16
Re: [PATCH v3 17/18] rerere: use "struct rerere_id" instead of "char *" for conflict ID
Quoted 5 lines · Junio C Hamano
This gives a thin abstraction between the conflict ID that is a hash value obtained by inspecting the conflicts and the name of the directory under $GIT_DIR/rr-cache/, in which the previous resolution is recorded to be replayed. The plan is to make sure that the presense of the directory does not imply the presense of a previous
s/presense/presence/g
Quoted 4 lines
resolution and vice-versa, and later allow us to have more than one pair of <preimage, postimage> for a given conflict ID.
Signed-off-by Junio C Hamano <[email protected]>
PATCH v3 18/18 rerere: un-nest merge() further
In reply to Junio C HamanoBy consistently using "upon failure, set 'ret' and jump to out" pattern, flatten the function further.
--- rerere.c | 50 ++++++++++++++++++++++++++------------------------ 1 file changed, 26 insertions(+), 24 deletions(-)
rerere.c +26 -24
@@ -571,6 +571,7 @@ int rerere_remaining(struct string_list *merge_rr)*/ static int merge (const struct rerere_id *id,const char *path) { +FILE *f;int ret;mmfile_t cur = {NULL ,0 }, base = {NULL ,0 }, other = {NULL ,0 };mmbuffer_t result = {NULL ,0 };
@@ -579,8 +580,10 @@ static int merge(const struct rerere_id *id, const char *path)* Normalize the conflicts in path and write it out to * "thisimage" temporary file. */ -if (handle_file (path,NULL ,rerere_path (id,"thisimage" )) <0 ) -return 1 ; +if (handle_file (path,NULL ,rerere_path (id,"thisimage" )) <0 ) { + ret =1 ; + goto out; + } if (read_mmfile(&cur, rerere_path(id, "thisimage")) || read_mmfile(&base, rerere_path(id, "preimage")) ||
@@ -594,29 +597,28 @@ static int merge(const struct rerere_id *id, const char *path)* low-level merge driver settings. */ ret =ll_merge (&result, path, &base,NULL , &cur,"" , &other,"" ,NULL ); -if (!ret) { -FILE *f; +if (ret) + goto out; -/* -* A successful replay of recorded resolution. -* Mark that "postimage" was used to help gc. -*/ -if (utime (rerere_path (id,"postimage" ),NULL ) <0 ) -warning ("failed utime() on %s: %s" , -rerere_path (id,"postimage" ), -strerror (errno)); - -/* Update "path" with the resolution */ - f =fopen (path,"w" ); -if (!f) -return error ("Could not open %s: %s" , path, -strerror (errno)); -if (fwrite (result.ptr, result.size,1 , f) !=1 ) -error ("Could not write %s: %s" , path,strerror (errno)); -if (fclose (f)) -return error ("Writing %s failed: %s" , path, -strerror (errno)); - } +/* +* A successful replay of recorded resolution. +* Mark that "postimage" was used to help gc. +*/ +if (utime (rerere_path (id,"postimage" ),NULL ) <0 ) +warning ("failed utime() on %s: %s" , +rerere_path (id,"postimage" ), +strerror (errno)); + +/* Update "path" with the resolution */ + f =fopen (path,"w" ); +if (!f) +return error ("Could not open %s: %s" , path, +strerror (errno)); +if (fwrite (result.ptr, result.size,1 , f) !=1 ) +error ("Could not write %s: %s" , path,strerror (errno)); +if (fclose (f)) +return error ("Writing %s failed: %s" , path, +strerror (errno)); out:free (cur.ptr);
Signature · 1 line
2.5.0-rc2-340-g0cccc16