summaryrefslogtreecommitdiff
path: root/rerere.c
Commit message (Collapse)AuthorAgeFilesLines
* hold_locked_index(): align error handling with hold_lockfile_for_update()Junio C Hamano2016-12-071-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Callers of the hold_locked_index() function pass 0 when they want to prepare to write a new version of the index file without wishing to die or emit an error message when the request fails (e.g. somebody else already held the lock), and pass 1 when they want the call to die upon failure. This option is called LOCK_DIE_ON_ERROR by the underlying lockfile API, and the hold_locked_index() function translates the paramter to LOCK_DIE_ON_ERROR when calling the hold_lock_file_for_update(). Replace these hardcoded '1' with LOCK_DIE_ON_ERROR and stop translating. Callers other than the ones that are replaced with this change pass '0' to the function; no behaviour change is intended with this patch. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Among the callers of hold_locked_index() that passes 0: - diff.c::refresh_index_quietly() at the end of "git diff" is an opportunistic update; it leaks the lockfile structure but it is just before the program exits and nobody should care. - builtin/describe.c::cmd_describe(), builtin/commit.c::cmd_status(), sequencer.c::read_and_refresh_cache() are all opportunistic updates and they are OK. - builtin/update-index.c::cmd_update_index() takes a lock upfront but we may end up not needing to update the index (i.e. the entries may be fully up-to-date), in which case we do not need to issue an error upon failure to acquire the lock. We do diagnose and die if we indeed need to update, so it is OK. - wt-status.c::require_clean_work_tree() IS BUGGY. It asks silence, does not check the returned value. Compare with callsites like cmd_describe() and cmd_status() to notice that it is wrong to call update_index_if_able() unconditionally.
* cache: convert struct cache_entry to use struct object_idbrian m. carlson2016-09-071-1/+2
| | | | | | | | | | | | | | | | | | | | | Convert struct cache_entry to use struct object_id by applying the following semantic patch and the object_id transforms from contrib, plus the actual change to the struct: @@ struct cache_entry E1; @@ - E1.sha1 + E1.oid.hash @@ struct cache_entry *E1; @@ - E1->sha1 + E1->oid.hash Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* Merge branch 'jc/rerere-multi'Junio C Hamano2016-05-231-8/+17
|\ | | | | | | | | | | * jc/rerere-multi: rerere: remove an null statement rerere: plug memory leaks upon "rerere forget" failure
| * rerere: remove an null statementjc/rerere-multiJunio C Hamano2016-05-191-1/+1
| | | | | | | | | | | | | | J6t spotted that previous commit added an empty statement by mistake. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: plug memory leaks upon "rerere forget" failureJunio C Hamano2016-05-111-8/+17
| | | | | | | | Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | Merge branch 'nd/error-errno'Junio C Hamano2016-05-171-11/+7
|\ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The code for warning_errno/die_errno has been refactored and a new error_errno() reporting helper is introduced. * nd/error-errno: (41 commits) wrapper.c: use warning_errno() vcs-svn: use error_errno() upload-pack.c: use error_errno() unpack-trees.c: use error_errno() transport-helper.c: use error_errno() sha1_file.c: use {error,die,warning}_errno() server-info.c: use error_errno() sequencer.c: use error_errno() run-command.c: use error_errno() rerere.c: use error_errno() and warning_errno() reachable.c: use error_errno() mailmap.c: use error_errno() ident.c: use warning_errno() http.c: use error_errno() and warning_errno() grep.c: use error_errno() gpg-interface.c: use error_errno() fast-import.c: use error_errno() entry.c: use error_errno() editor.c: use error_errno() diff-no-index.c: use error_errno() ...
| * | rerere.c: use error_errno() and warning_errno()Nguyễn Thái Ngọc Duy2016-05-091-11/+7
| | | | | | | | | | | | | | | Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | Merge branch 'jc/rerere-multi'Junio C Hamano2016-04-251-194/+432
|\ \ \ | |/ / |/| / | |/ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | "git rerere" can encounter two or more files with the same conflict signature that have to be resolved in different ways, but there was no way to record these separate resolutions. * jc/rerere-multi: rerere: adjust 'forget' to multi-variant world order rerere: split code to call ll_merge() further rerere: move code related to "forget" together rerere: gc and clear rerere: do use multiple variants t4200: rerere a merge with two identical conflicts rerere: allow multiple variants to exist rerere: delay the recording of preimage rerere: handle leftover rr-cache/$ID directory and postimage files rerere: scan $GIT_DIR/rr-cache/$ID when instantiating a rerere_id rerere: split conflict ID further
| * rerere: adjust 'forget' to multi-variant world orderJunio C Hamano2016-04-061-1/+27
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Because conflicts with the same contents inside conflict blocks enclosed by "<<<<<<<" and ">>>>>>>" can now have multiple variants to help three-way merge to adjust to the differences outside the conflict blocks, "rerere forget $path" needs to be taught that there may be multiple recorded resolutions that share the same conflict hash (which groups the conflicts with "the same contents inside conflict blocks"), among which there are some that would not be relevant to the conflict we are looking at. These "other variants" that happen to share the same conflict hash should not be cleared, and the variant that would apply to the current conflict may not be the zero-th one (which is the only one that is cleared by the current code). After finding the conflict hash, iterate over the existing variants and try to resolve the conflict using each of them to find the one that "cleanly" resolves the current conflict. That is the one we want to forget and record the preimage for, so that the user can record the corrected resolution. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: split code to call ll_merge() furtherJunio C Hamano2016-04-061-16/+31
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The merge() helper function is given an existing rerere ID (i.e. the name of the .git/rr-cache/* subdirectory, and the variant number) that identifies one <preimage, postimage> pair, try to see if the conflicted state in the given path can be resolved by using the pair, and if this succeeds, then update the conflicted path with the result in the working tree. To implement rerere_forget() in the multiple variant world, we'd need a helper to do the "see if a <preimage, postimage> pair cleanly resolves a conflicted state we have in-core" part, without actually touching any file in the working tree, in order to identify which variant(s) to remove. Split the logic to do so into a separate helper function try_merge() out of merge(). Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: move code related to "forget" togetherJunio C Hamano2016-04-061-97/+97
| | | | | | | | | | | | | | | | | | "rerere forget" is the only user of handle_cache() helper, which in turn is the only user of rerere_io that reads from an in-core buffer whose getline method is implemented as rerere_mem_getline(). Gather them together. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: gc and clearJunio C Hamano2016-04-061-39/+48
| | | | | | | | | | | | | | | | Adjust "git rerere gc" and "git rerere clear" to the new world order with rerere database with multiple variants for the same shape of conflicts. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: do use multiple variantsJunio C Hamano2016-03-151-38/+60
| | | | | | | | | | | | | | | | | | | | | | | | | | | | This enables the multiple-variant support for real. Multiple conflicts of the same shape can have differences in contexts where they appear, interfering the replaying of recorded resolution of one conflict to another, and in such a case, their resolutions are recorded as different variants under the same conflict ID. We still need to adjust garbage collection codepaths for this change, but the basic "replay" functionality is functional with this change. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: allow multiple variants to existJunio C Hamano2016-03-151-22/+105
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The shape of the conflict in a path determines the conflict ID. The preimage and postimage pair that was recorded for the conflict ID previously may or may not replay well for the conflict we just saw. Currently, we punt when the previous resolution does not cleanly replay, but ideally we should then be able to record the currently conflicted path by assigning a new 'variant', and then record the resolution the user is going to make. Introduce a mechanism to have more than one variant for a given conflict ID; we do not actually assign any variant other than 0th variant yet at this step. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: delay the recording of preimageJunio C Hamano2016-03-151-27/+25
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | We record the preimage only when there is no directory to record the conflict we encountered, i.e. when $GIT_DIR/rr-cache/$ID does not exist. As the plan is to allow multiple <preimage,postimage> pairs as variants for the same conflict ID eventually, this logic needs to go. As the first step in that direction, stop the "did we create the directory? Then we record the preimage" logic. Instead, we record if a preimage does not exist when we saw a conflict in a path. Also make sure that we remove a stale postimage, which most likely is totally unrelated to the resolution of this new conflict, when we create a new preimage under $ID when $GIT_DIR/rr-cache/$ID already exists. In later patches, we will further update this logic to be "do we have <preimage,postimage> pair that cleanly resolve the current conflicts? If not, record a new preimage as a new variant", but that does not happen at this stage yet. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: handle leftover rr-cache/$ID directory and postimage filesJunio C Hamano2016-03-151-17/+25
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | If by some accident there is only $GIT_DIR/rr-cache/$ID directory existed, we wouldn't have recorded a preimage for a conflict that is newly encountered, which would mean after a manual resolution, we wouldn't have recorded it by storing the postimage, because the logic used to be "if there is no rr-cache/$ID directory, then we are the first so record the preimage". Instead, record preimage if we do not have one. In addition, if there is only $GIT_DIR/rr-cache/$ID/postimage without corresponding preimage, we would have tried to call into merge() and punted. These would have been a situation frustratingly hard to recover from. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: scan $GIT_DIR/rr-cache/$ID when instantiating a rerere_idJunio C Hamano2016-02-081-3/+29
| | | | | | | | | | | | | | | | This will help fixing bootstrap corner-case issues, e.g. having an empty $GIT_DIR/rr-cache/$ID directory would fail to record a preimage, in later changes in this series. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: split conflict ID furtherJunio C Hamano2016-02-081-5/+56
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The plan is to keep assigning the backward compatible conflict ID based on the hash of the (normalized) text of conflicts, keep using that conflict ID as the directory name under $GIT_DIR/rr-cache/, but allow each conflicted path to use a separate "variant" to record resolutions, i.e. having more than one <preimage,postimage> pairs under $GIT_DIR/rr-cache/$ID/ directory. As the first step in that direction, separate the shared "conflict ID" out of the rerere_id structure. The plan is to keep information per $ID in rerere_dir, that can be shared among rerere_id that is per conflicted path. When we are done with rerere(), which can be directly called from other programs like "git apply", "git commit" and "git merge", the shared rerere_dir structures can be freed entirely, so they are not reference-counted and they are not freed when we release rerere_id's that reference them. Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | Merge branch 'jk/rerere-xsnprintf'Junio C Hamano2016-02-171-2/+2
|\ \ | |/ | | | | | | | | | | | | | | | | | | Some calls to strcpy(3) triggers a false warning from static analysers that are less intelligent than humans, and reducing the number of these false hits helps us notice real issues. A few calls to strcpy(3) in "git rerere" that are already safe has been rewritten to avoid false wanings. * jk/rerere-xsnprintf: rerere: replace strcpy with xsnprintf
| * rerere: replace strcpy with xsnprintfjk/rerere-xsnprintfJeff King2016-02-081-2/+2
| | | | | | | | | | | | | | | | | | | | This shouldn't overflow, as we are copying a sha1 hex into a 41-byte buffer. But it does not hurt to use a bound-checking function, which protects us and makes auditing for overflows easier. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | Merge branch 'jc/rerere'Junio C Hamano2015-10-051-151/+404
|\ \ | |/ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Code clean-up and minor fixes. * jc/rerere: (21 commits) rerere: un-nest merge() further rerere: use "struct rerere_id" instead of "char *" for conflict ID rerere: call conflict-ids IDs rerere: further clarify do_rerere_one_path() rerere: further de-dent do_plain_rerere() rerere: refactor "replay" part of do_plain_rerere() rerere: explain the remainder rerere: explain "rerere forget" codepath rerere: explain the primary codepath rerere: explain MERGE_RR management helpers rerere: fix benign off-by-one non-bug and clarify code rerere: explain the rerere I/O abstraction rerere: do not leak mmfile[] for a path with multiple stage #1 entries rerere: stop looping unnecessarily rerere: drop want_sp parameter from is_cmarker() rerere: report autoupdated paths only after actually updating them rerere: write out each record of MERGE_RR in one go rerere: lift PATH_MAX limitation rerere: plug conflict ID leaks rerere: handle conflicts with multiple stage #1 entries ...
| * rerere: un-nest merge() furtherjc/rerereJunio C Hamano2015-07-241-24/+26
| | | | | | | | | | | | | | By consistently using "upon failure, set 'ret' and jump to out" pattern, flatten the function further. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: use "struct rerere_id" instead of "char *" for conflict IDJunio C Hamano2015-07-241-26/+73
| | | | | | | | | | | | | | | | | | | | | | | | 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 presence 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. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: call conflict-ids IDsJunio C Hamano2015-07-241-38/+38
| | | | | | | | | | | | | | Most places we call conflict IDs "name" and some others we call them "hex"; update all of them to "id". Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: further clarify do_rerere_one_path()Junio C Hamano2015-07-241-9/+6
| | | | | | | | Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: further de-dent do_plain_rerere()Junio C Hamano2015-07-241-32/+33
| | | | | | | | | | | | It's just easier to follow this way. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: refactor "replay" part of do_plain_rerere()Junio C Hamano2015-07-241-35/+40
| | | | | | | | | | | | | | | | | | Extract 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. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: explain the remainderJunio C Hamano2015-07-241-0/+20
| | | | | | | | | | | | | | | | | | | | | | Explain 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". Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: explain "rerere forget" codepathJunio C Hamano2015-07-241-0/+24
| | | | | | | | | | | | | | | | | | | | Explain 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". Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: explain the primary codepathJunio C Hamano2015-07-241-13/+82
| | | | | | | | | | | | | | | | | | | | | | Explain 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. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: explain MERGE_RR management helpersJunio C Hamano2015-07-241-0/+41
| | | | | | | | | | | | | | | | | | | | | | | | Explain 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. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: fix benign off-by-one non-bug and clarify codeJunio C Hamano2015-07-241-1/+8
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | rerere_io_putconflict() wants to use a limited fixed-sized buf[] on stack repeatedly to formulate a longer string, but its implementation is doubly confusing: * When it knows that the whole thing fits in buf[], it wants to fill early part of buf[] with conflict marker characters, followed by a LF and a NUL. It miscounts the size of the buffer by 1 and does not use the last byte of buf[]. * When it needs to show only the early part of a long conflict marker string (because the whole thing does not fit in buf[]), it adjusts the number of bytes shown in the current round in a strange-looking way. It makes sure that this round does not emit all bytes and leaves at least one byte to the next round, so that "it all fits" case will pick up the rest and show the terminating LF. While this is correct, one needs to stop and think for a while to realize why it is correct without an explanation. Fix the benign off-by-one, and add comments to explain the strange-looking size adjustment. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: explain the rerere I/O abstractionJunio C Hamano2015-07-241-7/+31
| | | | | | | | | | | | | | | | | | Explain 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. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: do not leak mmfile[] for a path with multiple stage #1 entriesJunio C Hamano2015-07-241-2/+4
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | A conflicted index can have multiple stage #1 entries when dealing with a criss-cross merge and using the "resolve" merge strategy. Plug the leak by reading only the first one of the same stage entries. Strictly speaking, this fix 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 is not a grave backward compatibility breakage. Either way, we are arbitrarily picking one of multiple stage #1 entries and using it, ignoring others, and there is no meaning in the ordering of these stage #1 entries. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: stop looping unnecessarilyJunio C Hamano2015-07-241-10/+7
| | | | | | | | | | | | | | | | | | | | | | | | 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 <gitster@pobox.com>
| * rerere: drop want_sp parameter from is_cmarker()Junio C Hamano2015-07-241-5/+22
| | | | | | | | | | | | | | | | As the nature of the conflict marker line determines if there should be a SP and label after it, the caller shouldn't have to pass the parameter redundantly. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: report autoupdated paths only after actually updating themJunio C Hamano2015-07-241-10/+12
| | | | | | | | Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: write out each record of MERGE_RR in one goJunio C Hamano2015-07-241-7/+9
| | | | | | | | | | | | | | | | | | | | | | | | | | Instead 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). Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: lift PATH_MAX limitationJunio C Hamano2015-07-241-20/+15
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The 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. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: plug conflict ID leaksJunio C Hamano2015-07-241-2/+5
| | | | | | | | | | | | | | | | | | | | | | | | | | | | The 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. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: handle conflicts with multiple stage #1 entriesJunio C Hamano2015-07-241-1/+1
| | | | | | | | | | | | | | A conflicted index can have multiple stage #1 entries when dealing with a criss-cross merge and using the "resolve" merge strategy. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * rerere: fix an off-by-one non-bugJunio C Hamano2015-06-281-4/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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. Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * Merge branch 'jn/rerere-fail-on-auto-update-failure' into maintJunio C Hamano2015-02-241-10/+6
| |\ | | | | | | | | | | | | | | | | | | | | | | | | "git rerere" (invoked internally from many mergy operations) did not correctly signal errors when told to update the working tree files and failed to do so for whatever reason. * jn/rerere-fail-on-auto-update-failure: rerere: error out on autoupdate failure
* | | rerere: release lockfile in non-writing functionsjk/am-rerere-lock-fixJeff King2015-09-011-2/+15
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | There's a bug in builtin/am.c in which we take a lock on MERGE_RR recursively. But rather than fix am.c, this patch fixes the confusing interface from rerere.c that caused the bug. Read on for the gory details. The setup_rerere() function both reads the existing MERGE_RR file, and takes MERGE_RR.lock. In the rerere() and rerere_forget() functions, we end up in write_rr(), which will then commit the lock file. But for functions like rerere_clear() that do not write to MERGE_RR, we expect the caller to have handled setup_rerere(). That caller would then need to release the lockfile, but it can't; the lock struct is local to rerere.c. For builtin/rerere.c, this is OK. We run a single rerere operation and then exit immediately, which has the side effect of rolling back the lockfile. But in builtin/am.c, this is actively wrong. If we run "git am -3 --skip", we call setup-rerere twice without releasing the lock: 1. The "--skip" causes us to call am_rerere_clear(), which calls setup_rerere(), but never drops the lock. 2. We then proceed to the next patch. 3. The "--3way" may cause us to call rerere() to handle conflicts in that patch, but we are already holding the lock. The lockfile code dies with: BUG: prepare_tempfile_object called for active object We could fix this by having rerere_clear() call rollback_lock_file(). But it feels a bit odd for it to roll back a lockfile that it did not itself take. So let's simplify the interface further, and handle setup_rerere in the function itself, taking away the question from the caller over whether they need to do so. We can give rerere_gc() the same treatment, as well (even though it doesn't have any callers besides builtin/rerere.c at this point). Note that these functions don't take flags from their callers to pass along to setup_rerere; that's OK, because the flags would not be meaningful for what they are doing. Both of those functions need to hold the lock because even though they do not write to MERGE_RR, they are still writing and should be protected from a simultaneous "rerere" run. But rerere_remaining(), "rerere diff", and "rerere status" are all read-only operations. They want to setup_rerere(), but do not care about taking the lock in the first place. Since our update of MERGE_RR is the usual atomic rename done by commit_lock_file, they can just do a lockless read. For that, we teach setup_rerere a READONLY flag to avoid the lock. As a bonus, this pushes builtin/rerere.c's setup_rerere call closer to the functions that use it. Which means that "git rerere totally-bogus-command" will no longer silently exit(0) in a repository without rerere enabled. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | memoize common git-path "constant" filesjk/git-pathJeff King2015-08-101-11/+8
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | One of the most common uses of git_path() is to pass a constant, like git_path("MERGE_MSG"). This has two drawbacks: 1. The return value is a static buffer, and the lifetime is dependent on other calls to git_path, etc. 2. There's no compile-time checking of the pathname. This is OK for a one-off (after all, we have to spell it correctly at least once), but many of these constant strings appear throughout the code. This patch introduces a series of functions to "memoize" these strings, which are essentially globals for the lifetime of the program. We compute the value once, take ownership of the buffer, and return the cached value for subsequent calls. cache.h provides a helper macro for defining these functions as one-liners, and defines a few common ones for global use. Using a macro is a little bit gross, but it does nicely document the purpose of the functions. If we need to touch them all later (e.g., because we learned how to change the git_dir variable at runtime, and need to invalidate all of the stored values), it will be much easier to have the complete list. Note that the shared-global functions have separate, manual declarations. We could do something clever with the macros (e.g., expand it to a declaration in some places, and a declaration _and_ a definition in path.c). But there aren't that many, and it's probably better to stay away from too-magical macros. Likewise, if we abandon the C preprocessor in favor of generating these with a script, we could get much fancier. E.g., normalizing "FOO/BAR-BAZ" into "git_path_foo_bar_baz". But the small amount of saved typing is probably not worth the resulting confusion to readers who want to grep for the function's definition. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | Merge branch 'jk/rerere-forget-check-enabled'Junio C Hamano2015-05-261-0/+2
|\ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | "git rerere forget" in a repository without rerere enabled gave a cryptic error message; it should be a silent no-op instead. * jk/rerere-forget-check-enabled: rerere: exit silently on "forget" when rerere is disabled
| * | | rerere: exit silently on "forget" when rerere is disabledjk/rerere-forget-check-enabledJeff King2015-05-141-0/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | If you run "git rerere forget foo" in a repository that does not have rerere enabled, git hits an internal error: $ git init -q $ git rerere forget foo fatal: BUG: attempt to commit unlocked object The problem is that setup_rerere() will not actually take the lock if the rerere system is disabled. We should notice this and return early. We can return with a success code here, because we know there is nothing to forget. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | | Merge branch 'jn/rerere-fail-on-auto-update-failure'Junio C Hamano2015-02-111-10/+6
|\ \ \ \ | |_|/ / |/| | / | | |/ | |/| | | | | | | | | | | | | "git rerere" (invoked internally from many mergy operations) did not correctly signal errors when told to update the working tree files and failed to do so for whatever reason. * jn/rerere-fail-on-auto-update-failure: rerere: error out on autoupdate failure
| * | rerere: error out on autoupdate failurejn/rerere-fail-on-auto-update-failureJonathan Nieder2015-01-081-10/+6
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | We have been silently tolerating errors by returning early with an error that the caller ignores since rerere.autoupdate was introduced in v1.6.0-rc0~120^2 (2008-06-22). So on error (for example if the index is already locked), rerere can return success silently without updating the index or with only some items in the index updated. Better to treat such failures as a fatal error so the operator can figure out what is wrong and fix it. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | lockfile.h: extract new header file for the functions in lockfile.cmh/lockfileMichael Haggerty2014-10-011-0/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Move the interface declaration for the functions in lockfile.c from cache.h to a new file, lockfile.h. Add #includes where necessary (and remove some redundant includes of cache.h by files that already include builtin.h). Move the documentation of the lock_file state diagram from lockfile.c to the new header file. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>