summaryrefslogtreecommitdiff
path: root/rerere.c
AgeCommit message (Collapse)AuthorFilesLines
2018-05-30Merge branch 'ma/lockfile-cleanup'Libravatar Junio C Hamano1-2/+1
Code clean-up to adjust to a more recent lockfile API convention that allows lockfile instances kept on the stack. * ma/lockfile-cleanup: lock_file: move static locks into functions lock_file: make function-local locks non-static refs.c: do not die if locking fails in `delete_pseudoref()` refs.c: do not die if locking fails in `write_pseudoref()` t/helper/test-write-cache: clean up lock-handling
2018-05-10lock_file: move static locks into functionsLibravatar Martin Ågren1-2/+1
Placing `struct lock_file`s on the stack used to be a bad idea, because the temp- and lockfile-machinery would keep a pointer into the struct. But after 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we can safely have lockfiles on the stack. (This applies even if a user returns early, leaving a locked lock behind.) Each of these `struct lock_file`s is used from within a single function. Move them into the respective functions to make the scope clearer and drop the staticness. For good measure, I have inspected these sites and come to believe that they always release the lock, with the possible exception of bailing out using `die()` or `exit()` or by returning from a `cmd_foo()`. As pointed out by Jeff King, it would be bad if someone held on to a `struct lock_file *` for some reason. After some grepping, I agree with his findings: no-one appears to be doing that. After this commit, the remaining occurrences of "static struct lock_file" are locks that are used from within different functions. That is, they need to remain static. (Short of more intrusive changes like passing around pointers to non-static locks.) Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-04-10Merge branch 'bc/object-id'Libravatar Junio C Hamano1-2/+2
Conversion from uchar[20] to struct object_id continues. * bc/object-id: (36 commits) convert: convert to struct object_id sha1_file: introduce a constant for max header length Convert lookup_replace_object to struct object_id sha1_file: convert read_sha1_file to struct object_id sha1_file: convert read_object_with_reference to object_id tree-walk: convert tree entry functions to object_id streaming: convert istream internals to struct object_id tree-walk: convert get_tree_entry_follow_symlinks internals to object_id builtin/notes: convert static functions to object_id builtin/fmt-merge-msg: convert remaining code to object_id sha1_file: convert sha1_object_info* to object_id Convert remaining callers of sha1_object_info_extended to object_id packfile: convert unpack_entry to struct object_id sha1_file: convert retry_bad_packed_offset to struct object_id sha1_file: convert assert_sha1_type to object_id builtin/mktree: convert to struct object_id streaming: convert open_istream to use struct object_id sha1_file: convert check_sha1_signature to struct object_id sha1_file: convert read_loose_object to use struct object_id builtin/index-pack: convert struct ref_delta_entry to object_id ...
2018-03-14sha1_file: convert read_sha1_file to struct object_idLibravatar brian m. carlson1-2/+2
Convert read_sha1_file to take a pointer to struct object_id and rename it read_object_file. Do the same for read_sha1_file_extended. Convert one use in grep.c to use the new function without any other code change, since the pointer being passed is a void pointer that is already initialized with a pointer to struct object_id. Update the declaration and definitions of the modified functions, and apply the following semantic patch to convert the remaining callers: @@ expression E1, E2, E3; @@ - read_sha1_file(E1.hash, E2, E3) + read_object_file(&E1, E2, E3) @@ expression E1, E2, E3; @@ - read_sha1_file(E1->hash, E2, E3) + read_object_file(E1, E2, E3) @@ expression E1, E2, E3, E4; @@ - read_sha1_file_extended(E1.hash, E2, E3, E4) + read_object_file_extended(&E1, E2, E3, E4) @@ expression E1, E2, E3, E4; @@ - read_sha1_file_extended(E1->hash, E2, E3, E4) + read_object_file_extended(E1, E2, E3, E4) Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-03-01write_locked_index(): add flag to avoid writing unchanged indexLibravatar Martin Ågren1-5/+3
We have several callers like if (active_cache_changed && write_locked_index(...)) handle_error(); rollback_lock_file(...); where the final rollback is needed because "!active_cache_changed" shortcuts the if-expression. There are also a few variants of this, including some if-else constructs that make it more clear when the explicit rollback is really needed. Teach `write_locked_index()` to take a new flag SKIP_IF_UNCHANGED and simplify the callers. Leave the most complicated of the callers (in builtin/update-index.c) unchanged. Rewriting it to use this new flag would end up duplicating logic. We could have made the new flag behave the other way round ("FORCE_WRITE"), but that could break existing users behind their backs. Let's take the more conservative approach. We can still migrate existing callers to use our new flag. Later we might even be able to flip the default, possibly without entirely ignoring the risk to in-flight or out-of-tree topics. Suggested-by: Jeff King <peff@peff.net> Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-01-22Use MOVE_ARRAYLibravatar SZEDER Gábor1-2/+2
Use the helper macro MOVE_ARRAY to move arrays. This is shorter and safer, as it automatically infers the size of elements. Patch generated by Coccinelle and contrib/coccinelle/array.cocci in Travis CI's static analysis build job. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-25Merge branch 'jk/write-in-full-fix'Libravatar Junio C Hamano1-1/+1
Many codepaths did not diagnose write failures correctly when disks go full, due to their misuse of write_in_full() helper function, which have been corrected. * jk/write-in-full-fix: read_pack_header: handle signed/unsigned comparison in read result config: flip return value of store_write_*() notes-merge: use ssize_t for write_in_full() return value pkt-line: check write_in_full() errors against "< 0" convert less-trivial versions of "write_in_full() != len" avoid "write_in_full(fd, buf, len) != len" pattern get-tar-commit-id: check write_in_full() return against 0 config: avoid "write_in_full(fd, buf, len) < len" pattern
2017-09-14avoid "write_in_full(fd, buf, len) != len" patternLibravatar Jeff King1-1/+1
The return value of write_in_full() is either "-1", or the requested number of bytes[1]. If we make a partial write before seeing an error, we still return -1, not a partial value. This goes back to f6aa66cb95 (write_in_full: really write in full or return error on disk full., 2007-01-11). So checking anything except "was the return value negative" is pointless. And there are a couple of reasons not to do so: 1. It can do a funny signed/unsigned comparison. If your "len" is signed (e.g., a size_t) then the compiler will promote the "-1" to its unsigned variant. This works out for "!= len" (unless you really were trying to write the maximum size_t bytes), but is a bug if you check "< len" (an example of which was fixed recently in config.c). We should avoid promoting the mental model that you need to check the length at all, so that new sites are not tempted to copy us. 2. Checking for a negative value is shorter to type, especially when the length is an expression. 3. Linus says so. In d34cf19b89 (Clean up write_in_full() users, 2007-01-11), right after the write_in_full() semantics were changed, he wrote: I really wish every "write_in_full()" user would just check against "<0" now, but this fixes the nasty and stupid ones. Appeals to authority aside, this makes it clear that writing it this way does not have an intentional benefit. It's a historical curiosity that we never bothered to clean up (and which was undoubtedly cargo-culted into new sites). So let's convert these obviously-correct cases (this includes write_str_in_full(), which is just a wrapper for write_in_full()). [1] A careful reader may notice there is one way that write_in_full() can return a different value. If we ask write() to write N bytes and get a return value that is _larger_ than N, we could return a larger total. But besides the fact that this would imply a totally broken version of write(), it would already invoke undefined behavior. Our internal remaining counter is an unsigned size_t, which means that subtracting too many byte will wrap it around to a very large number. So we'll instantly begin reading off the end of the buffer, trying to write gigabytes (or petabytes) of data. Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-08-22rerere: allow approxidate in gc.rerereResolved/gc.rerereUnresolvedLibravatar Junio C Hamano1-12/+2
These two configuration variables are described in the documentation to take an expiry period expressed in the number of days: gc.rerereResolved:: Records of conflicted merge you resolved earlier are kept for this many days when 'git rerere gc' is run. The default is 60 days. gc.rerereUnresolved:: Records of conflicted merge you have not resolved are kept for this many days when 'git rerere gc' is run. The default is 15 days. There is no strong reason not to allow a more general "approxidate" expiry specification, e.g. "5.days.ago", or "never". Rename the config_get_expiry() helper introduced in the previous step to git_config_get_expiry_in_days() and move it to a more generic place, config.c, and use date.c::parse_expiry_date() to do so. Give it an ability to allow the caller to tell among three cases (i.e. there is no "gc.rerereResolved" config, there is and it is correctly parsed into the *expiry variable, and there was an error in parsing the given value). The current caller can work correctly without using the return value, though. In the future, we may find other variables that only allow an integer that specifies "this many days" or other unit of time, and when it happens we may need to drop "_days" suffix from the name of the function and instead pass the "scale" value as another parameter. But this will do for now. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-08-22rerere: represent time duration in timestamp_t internallyLibravatar Junio C Hamano1-13/+23
The two configuration variables, gc.rerereResolved and gc.rerereUnresolved, are measured in days and are passed as such into the prune_one() helper function, which worked in time_t to see if an entry in the rerere database is past its expiry. Instead, have the caller turn the number of days into the expiry timestamp. Further, use timestamp_t instead of time_t. This will make it possible to extend the way the configuration variable is spelled by using date.c::parse_expiry_date() that gives the expiry timestamp in timestamp_t. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-06-24Merge branch 'ab/free-and-null'Libravatar Junio C Hamano1-2/+1
A common pattern to free a piece of memory and assign NULL to the pointer that used to point at it has been replaced with a new FREE_AND_NULL() macro. * ab/free-and-null: *.[ch] refactoring: make use of the FREE_AND_NULL() macro coccinelle: make use of the "expression" FREE_AND_NULL() rule coccinelle: add a rule to make "expression" code use FREE_AND_NULL() coccinelle: make use of the "type" FREE_AND_NULL() rule coccinelle: add a rule to make "type" code use FREE_AND_NULL() git-compat-util: add a FREE_AND_NULL() wrapper around free(ptr); ptr = NULL
2017-06-24Merge branch 'bw/config-h'Libravatar Junio C Hamano1-0/+1
Fix configuration codepath to pay proper attention to commondir that is used in multi-worktree situation, and isolate config API into its own header file. * bw/config-h: config: don't implicitly use gitdir or commondir config: respect commondir setup: teach discover_git_directory to respect the commondir config: don't include config.h by default config: remove git_config_iter config: create config.h
2017-06-16*.[ch] refactoring: make use of the FREE_AND_NULL() macroLibravatar Ævar Arnfjörð Bjarmason1-2/+1
Replace occurrences of `free(ptr); ptr = NULL` which weren't caught by the coccinelle rule. These fall into two categories: - free/NULL assignments one after the other which coccinelle all put on one line, which is functionally equivalent code, but very ugly. - manually spotted occurrences where the NULL assignment isn't right after the free() call. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-06-15config: don't include config.h by defaultLibravatar Brandon Williams1-0/+1
Stop including config.h by default in cache.h. Instead only include config.h in those files which require use of the config system. Signed-off-by: Brandon Williams <bmwill@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-05-26rerere.c: move error_errno() closer to the source system callLibravatar Nguyễn Thái Ngọc Duy1-1/+2
We are supposed to report errno from fopen(). fclose() between fopen() and the report function could either change errno or reset it. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-05-26print errno when reporting a system call errorLibravatar Nguyễn Thái Ngọc Duy1-2/+2
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-05-26wrapper.c: add and use fopen_or_warn()Libravatar Nguyễn Thái Ngọc Duy1-1/+1
When fopen() returns NULL, it could be because the given path does not exist, but it could also be some other errors and the caller has to check. Add a wrapper so we don't have to repeat the same error check everywhere. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-12-07hold_locked_index(): align error handling with hold_lockfile_for_update()Libravatar Junio C Hamano1-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.
2016-09-07cache: convert struct cache_entry to use struct object_idLibravatar brian m. carlson1-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>
2016-05-23Merge branch 'jc/rerere-multi'Libravatar Junio C Hamano1-8/+17
* jc/rerere-multi: rerere: remove an null statement rerere: plug memory leaks upon "rerere forget" failure
2016-05-19rerere: remove an null statementLibravatar Junio C Hamano1-1/+1
J6t spotted that previous commit added an empty statement by mistake. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-05-17Merge branch 'nd/error-errno'Libravatar Junio C Hamano1-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() ...
2016-05-11rerere: plug memory leaks upon "rerere forget" failureLibravatar Junio C Hamano1-8/+17
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-05-09rerere.c: use error_errno() and warning_errno()Libravatar Nguyễn Thái Ngọc Duy1-11/+7
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-04-25Merge branch 'jc/rerere-multi'Libravatar Junio C Hamano1-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
2016-04-06rerere: adjust 'forget' to multi-variant world orderLibravatar Junio C Hamano1-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>
2016-04-06rerere: split code to call ll_merge() furtherLibravatar Junio C Hamano1-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>
2016-04-06rerere: move code related to "forget" togetherLibravatar Junio C Hamano1-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>
2016-04-06rerere: gc and clearLibravatar Junio C Hamano1-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>
2016-03-15rerere: do use multiple variantsLibravatar Junio C Hamano1-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>
2016-03-15rerere: allow multiple variants to existLibravatar Junio C Hamano1-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>
2016-03-15rerere: delay the recording of preimageLibravatar Junio C Hamano1-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>
2016-03-15rerere: handle leftover rr-cache/$ID directory and postimage filesLibravatar Junio C Hamano1-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>
2016-02-17Merge branch 'jk/rerere-xsnprintf'Libravatar Junio C Hamano1-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
2016-02-08rerere: scan $GIT_DIR/rr-cache/$ID when instantiating a rerere_idLibravatar Junio C Hamano1-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>
2016-02-08rerere: split conflict ID furtherLibravatar Junio C Hamano1-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>
2016-02-08rerere: replace strcpy with xsnprintfLibravatar Jeff King1-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>
2015-10-05Merge branch 'jc/rerere'Libravatar Junio C Hamano1-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 ...
2015-09-01rerere: release lockfile in non-writing functionsLibravatar Jeff King1-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>
2015-08-10memoize common git-path "constant" filesLibravatar Jeff King1-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>
2015-07-24rerere: un-nest merge() furtherLibravatar Junio C Hamano1-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>
2015-07-24rerere: use "struct rerere_id" instead of "char *" for conflict IDLibravatar Junio C Hamano1-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>
2015-07-24rerere: call conflict-ids IDsLibravatar Junio C Hamano1-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>
2015-07-24rerere: further clarify do_rerere_one_path()Libravatar Junio C Hamano1-9/+6
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-07-24rerere: further de-dent do_plain_rerere()Libravatar Junio C Hamano1-32/+33
It's just easier to follow this way. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-07-24rerere: refactor "replay" part of do_plain_rerere()Libravatar Junio C Hamano1-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>
2015-07-24rerere: explain the remainderLibravatar Junio C Hamano1-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>
2015-07-24rerere: explain "rerere forget" codepathLibravatar Junio C Hamano1-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>
2015-07-24rerere: explain the primary codepathLibravatar Junio C Hamano1-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>
2015-07-24rerere: explain MERGE_RR management helpersLibravatar Junio C Hamano1-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>