kernel/git - git - PowerEL Git System

Commit Graph

Author	SHA1	Message	Date
Junio C Hamano	c6da34a610	Revert "Merge branch 'ps/avoid-unnecessary-hook-invocation-with-packed-refs'" This reverts commit `991b4d47f0`, reversing changes made to `bcd020f88e`.	3 years ago
René Scharfe	84792322ed	commit, sequencer: turn off break_opt for commit summary `dc6b1d92ca` (wt-status: use settings from git_diff_ui_config, 2018-05-04) disabled diffopt.break_opt for diffstats shown by git status and in commit templates. For git status there isn't even a way to enable it. Make the commit summary (shown after the commit) consistent by disabling it there as well. Reported-by: Laurent Lyaudet <laurent.lyaudet@gmail.com> Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Jayati Shrivastava	5327d8982a	sequencer: use reverse_commit_list() helper Instead of creating a new allocation, reverse the original list in-place by calling the reverse_commit_list() helper. The original code discards the list "bases" after storing its reverse copy in a newly created list "reversed". If the code that followed from here used both "bases" and "reversed", the modification would not have worked, but since the original list "bases" gets discarded, we can simply reverse "bases" in-place with the reverse_commit_list() helper and reuse the same variable in the code that follows. builtin/merge.c has been left unmodified, since in its case, the original list is needed separately from its reverse copy by the code. Signed-off-by: Jayati Shrivastava <gaurijove@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Ævar Arnfjörð Bjarmason	a8cc594333	hooks: fix an obscure TOCTOU "did we just run a hook?" race Fix a Time-of-check to time-of-use (TOCTOU) race in code added in `680ee550d7` (commit: skip discarding the index if there is no pre-commit hook, 2017-08-14). This obscure race condition can occur if we e.g. ran the "pre-commit" hook and it modified the index, but hook_exists() returns false later on (e.g., because the hook itself went away, the directory became unreadable, etc.). Then we won't call discard_cache() when we should have. The race condition itself probably doesn't matter, and users would have been unlikely to run into it in practice. This problem has been noted on-list when `680ee550d7` was discussed[1], but had not been fixed. This change is mainly intended to improve the readability of the code involved, and to make reasoning about it more straightforward. It wasn't as obvious what we were trying to do here, but by having an "invoked_hook" it's clearer that e.g. our discard_cache() is happening because of the earlier hook execution. Let's also change this for the push-to-checkout hook. Now instead of checking if the hook exists and either doing a push to checkout or a push to deploy we'll always attempt a push to checkout. If the hook doesn't exist we'll fall back on push to deploy. The same behavior as before, without the TOCTOU race. See `0855331941` (receive-pack: support push-to-checkout hook, 2014-12-01) for the introduction of the previous behavior. This leaves uses of hook_exists() in two places that matter. The "reference-transaction" check in refs.c, see `6754159767` (refs: implement reference transaction hook, 2020-06-19), and the "prepare-commit-msg" hook, see `66618a50f9` (sequencer: run 'prepare-commit-msg' hook, 2018-01-24). In both of those cases we're saving ourselves CPU time by not preparing data for the hook that we'll then do nothing with if we don't have the hook. So using this "invoked_hook" pattern doesn't make sense in those cases. The "reference-transaction" and "prepare-commit-msg" hook also aren't racy. In those cases we'll skip the hook runs if we race with a new hook being added, whereas in the TOCTOU races being fixed here we were incorrectly skipping the required post-hook logic. 1. https://lore.kernel.org/git/20170810191613.kpmhzg4seyxy3cpq@sigill.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Jean-Noël Avila	1a8aea857e	i18n: factorize "invalid value" messages Use the same message when an invalid value is passed to a command line option or a configuration variable. Signed-off-by: Jean-Noël Avila <jn.avila@free.fr> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Ævar Arnfjörð Bjarmason	ce14de03db	refs API: remove "failure_errno" from refs_resolve_ref_unsafe() Remove the now-unused "failure_errno" parameter from the refs_resolve_ref_unsafe() signature. In my recent `96f6623ada` (Merge branch 'ab/refs-errno-cleanup', 2021-11-29) series we made all of its callers explicitly request the errno via an output parameter. As that series shows all but one caller ended up passing in a boilerplate "ignore_errno", since they only cared about whether the return value was NULL or not, i.e. if the ref could be resolved. There was one small issue with that series fixed with a follow-up in `31e3912369` (Merge branch 'ab/refs-errno-cleanup', 2022-01-14) a small bug in that series was fixed. After those two there was one caller left in sequencer.c that used the "failure_errno', but as of the preceding commit it uses a boilerplate "ignore_errno" instead. This leaves the public refs API without any use of "failure_errno" at all. We could still do with a bit of cleanup and generalization between refs.c and refs/files-backend.c before the "reftable" integration lands, but that's all internal to the reference code itself. So let's remove this output parameter. Not only isn't it used now, but it's unlikely that we'll want it again in the future. We'd like to slowly move the refs API to a more file-backend independent way of communicating error codes, having it use a "failure_errno" was only the first step in that direction. If this or any other function needs to communicate what specifically is wrong with the requested "refname" it'll be better to have the function set some output enum of well-defined error states than piggy-backend on "errno". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Ævar Arnfjörð Bjarmason	09444e74e3	sequencer: don't use die_errno() on refs_resolve_ref_unsafe() failure Change code that was faithfully migrated to the new "resolve_errno" API in `ed90f04155` (refs API: make resolve_ref_unsafe() not set errno, 2021-10-16) to stop caring about the errno at all. When we fail to resolve "HEAD" after the sequencer runs it doesn't really help to say what the "errno" value is, since the fake backend errno may or may not reflect anything real about the state of the ".git/HEAD". With the upcoming reftable backend this fakery will become even more pronounced. So let's just die() instead of die_errno() here. This will also help simplify the refs_resolve_ref_unsafe() API. This was the only user of it that wasn't ignoring the "failure_errno" output parameter. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Phillip Wood	38c541ce94	rebase -m: don't fork git checkout Now that reset_head() can handle the initial checkout of onto correctly use it in the "merge" backend instead of forking "git checkout". This opens the way for us to stop calling the post-checkout hook in the future. Not running "git checkout" means that "rebase -i/m" no longer recurse submodules when checking out "onto" (thanks to Philippe Blain for pointing this out). As the rest of rebase does not know what to do with submodules this is probably a good thing. When using merge-ort rebase ought be able to handle submodules correctly if it parsed the submodule config, such a change is left for a future patch series. The "apply" based rebase has avoided forking git checkout since `ac7f467fef` ("builtin/rebase: support running "git rebase <upstream>"", 2018-08-07). The code that handles the checkout was moved into libgit by `b309a97108` ("reset: extract reset_head() from rebase", 2020-04-07). Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Phillip Wood	6ae8086161	reset_head(): take struct rebase_head_opts This function takes a confusingly large number of parameters which makes it difficult to remember which order to pass them in. The following commits will add a couple more parameters which makes the problem worse. To address this change the function to take a struct of options. Using a struct means that it is no longer necessary to remember which order to pass the parameters in and anyone reading the code can easily see which value is passed to each parameter. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Phillip Wood	b7de153bd9	create_autostash(): remove unneeded parameter The default_reflog parameter of create_autostash() is passed to reset_head(). However as creating a stash does not involve updating any refs the parameter is not used by reset_head(). Removing the parameter from create_autostash() simplifies the callers. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Phillip Wood	1946d45844	reset_head(): remove action parameter The only use of the action parameter is to setup the error messages for unpack_trees(). All but two cases pass either "checkout" or "reset". The case that passes "reset --hard" would be better passing "reset" so that the error messages match the builtin reset command like all the other callers that are doing a reset. The case that passes "Fast-forwarded" is only updating HEAD and so the parameter is unused in that case as it does not call unpack_trees(). The value to pass to setup_unpack_trees_porcelain() can be determined by checking whether flags contains RESET_HEAD_HARD without the caller having to specify it. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Elijah Newren	ff5b7913f0	sequencer, stash: fix running from worktree subdir In commits `bc3ae46b42` ("rebase: do not attempt to remove startup_info->original_cwd", 2021-12-09) and `0fce211ccc` ("stash: do not attempt to remove startup_info->original_cwd", 2021-12-09), we wanted to allow the subprocess to know which directory the parent process was running from, so that the subprocess could protect it. However... When run from a non-main worktree, setup_git_directory() will note that the discovered git directory (/PATH/TO/.git/worktree/non-main-worktree) does not match DEFAULT_GIT_DIR_ENVIRONMENT (see setup_discovered_git_dir()), and decide to set GIT_DIR in the environment. This matters because... Whenever git is run with the GIT_DIR environment variable set, and GIT_WORK_TREE not set, it presumes that '.' is the working tree. So... This combination results in the subcommand being very confused about the working tree. Fix it by also setting the GIT_WORK_TREE environment variable along with setting cmd.dir. A possibly more involved fix we could consider for later would be to make setup.c set GIT_WORK_TREE whenever (a) it discovers both the git directory and the working tree and (b) it decides to set GIT_DIR in the environment. I did not attempt that here as such would be too big of a change for a 2.35.1 release. Test-case-by: Glen Choo <chooglen@google.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Patrick Steinhardt	fbe73f61cb	refs: allow passing flags when beginning transactions We do not currently have any flags when creating reference transactions, but we'll add one to disable execution of the reference transaction hook in some cases. Allow passing flags to `ref_store_transaction_begin()` to prepare for this change. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Elijah Newren	bc3ae46b42	rebase: do not attempt to remove startup_info->original_cwd Since rebase spawns a `checkout` subprocess, make sure we run that from the startup_info->original_cwd directory, so that the checkout process knows to protect that directory. Acked-by: Derrick Stolee <stolee@gmail.com> Acked-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Elijah Newren	434e0636db	sequencer: do not export GIT_DIR and GIT_WORK_TREE for 'exec' Commands executed from `git rebase --exec` can give different behavior from within that environment than they would outside of it, due to the fact that sequencer.c exports both GIT_DIR and GIT_WORK_TREE. For example, if the relevant script calls something like git -C ../otherdir log --format=%H --no-walk the user may be surprised to find that the command above does not show a commit hash from ../otherdir, because $GIT_DIR prevents automatic gitdir detection and makes the -C option useless. This is a regression in behavior from the original legacy implemented-in-shell rebase. It is perhaps rare for it to cause problems in practice, especially since most small problems that were caused by this area of bugs has been fixed-up in the past in a way that masked the particular bug observed without fixing the real underlying problem. An explanation of how we arrived at the current situation is perhaps merited. The setting of GIT_DIR and GIT_WORK_TREE done by sequencer.c arose from a sequence of historical accidents: * When rebase was implemented as a shell command, it would call git-sh-setup, which among other things would set GIT_DIR -- but not export it. This meant that when rebase --exec commands were run via /bin/sh -c "$COMMAND" they would not inherit the GIT_DIR setting. The fact that GIT_DIR was not set in the run $COMMAND is the behavior we'd like to restore. * When the rebase--helper builtin was introduced to allow incrementally replacing shell with C code, we had an implementation that was half shell, half C. In particular, commit `18633e1a22` ("rebase -i: use the rebase--helper builtin", 2017-02-09) added calls to exec git rebase--helper ... which caused rebase--helper to inherit the GIT_DIR environment variable from the shell. git's setup would change the environment variable from an absolute path to a relative one (".git"), but would leave it set. This meant that when rebase --exec commands were run via run_command_v_opt(...) they would inherit the GIT_DIR setting. * In commit `09d7b6c6fa` ("sequencer: pass absolute GIT_DIR to exec commands", 2017-10-31), it was noted that the GIT_DIR caused problems with some commands; e.g. git rebase --exec 'cd subdir && git describe' ... would have GIT_DIR=.git which was invalid due to the change to the subdirectory. Instead of questioning why GIT_DIR was set, that commit instead made sequencer change GIT_DIR to be an absolute path and explicitly export it via argv_array_pushf(&child_env, "GIT_DIR=%s", absolute_path(get_git_dir())); run_command_v_opt_cd_env(..., child_env.argv) * In commit `ab5e67d751` ("sequencer: pass absolute GIT_WORK_TREE to exec commands", 2018-07-14), it was noted that when GIT_DIR is set but GIT_WORK_TREE is not, that we do not discover GIT_WORK_TREE but just assume it is '.'. That is incorrect if trying to run commands from a subdirectory. However, rather than question why GIT_DIR was set, that commit instead also added GIT_WORK_TREE to the list of things to export. Each of the above problems would have been fixed automatically when git-rebase became a full builtin, had it not been for the fact that sequencer.c started exporting GIT_DIR and GIT_WORK_TREE in the interim. Stop exporting them now. Signed-off-by: Elijah Newren <newren@gmail.com> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de> Acked-by: Johannes Altmanninger <aclopte@gmail.com> Acked-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Elijah Newren	cc9dcdee61	sequencer: avoid adding exec commands for non-commit creating commands The `--exec <cmd>` is documented as Append "exec <cmd>" after each line creating a commit in the final history. ... If --autosquash is used, "exec" lines will not be appended for the intermediate commits, and will only appear at the end of each squash/fixup series. Unfortunately, it would also add exec commands after non-pick operations, such as 'no-op', which could be seen for example with git rebase -i --exec true HEAD todo_list_add_exec_commands() intent was to insert exec commands after each logical pick, while trying to consider a chains of fixup and squash commits to be part of the pick before it. So it would keep an 'insert' boolean tracking if it had seen a pick or merge, but not write the exec command until it saw the next non-fixup/squash command. Since that would make it miss the final exec command, it had some code that would check whether it still needed to insert one at the end, but instead of a simple if (insert) it had a if (insert \|\| <condition that is always true>) That's buggy; as per the docs, we should only add exec commands for lines that create commits, i.e. only if insert is true. Fix the conditional. There was one testcase in the testsuite that we tweak for this change; it was introduced in `54fd3243da` ("rebase -i: reread the todo list if `exec` touched it", 2017-04-26), and was merely testing that after an exec had fired that the todo list would be re-read. The test at the time would have worked given any revision at all, though it would only work with 'HEAD' as a side-effect of this bug. Since we're fixing this bug, choose something other than 'HEAD' for that test. Finally, add a testcase that verifies when we have no commits to pick, that we get no exec lines in the generated todo list. Reported-by: Nikita Bobko <nikitabobko@gmail.com> Signed-off-by: Elijah Newren <newren@gmail.com> Acked-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Ævar Arnfjörð Bjarmason	2b7098936c	run-command API users: use strvec_pushl(), not argv construction Change a pattern of hardcoding an "argv" array size, populating it and assigning to the "argv" member of "struct child_process" to instead use "strvec_pushl()" to add data to the "args" member. This implements the same behavior as before in fewer lines of code, and moves us further towards being able to remove the "argv" member in a subsequent commit. Since we've entirely removed the "argv" variable(s) we can be sure that no potential logic errors of the type discussed in a preceding commit are being introduced here, i.e. ones where the local "argv" was being modified after the assignment to "struct child_process"'s "argv". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Phillip Wood	9d6b9df128	rebase -i: fix rewording with --committer-date-is-author-date `baf8ec8d3a` (rebase -r: don't write .git/MERGE_MSG when fast-forwarding, 2021-08-20) stopped reading the author script in run_git_commit() when rewording a commit. This is normally safe because "git commit --amend" preserves the authorship. However if the user passes "--committer-date-is-author-date" then we need to read the author date from the author script when rewording. Fix this regression by tightening the check for when it is safe to skip reading the author script. Reported-by: Jonas Kittner <jonas.kittner@ruhr-uni-bochum.de> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Ævar Arnfjörð Bjarmason	f1da24ca5e	refs API: post-migration API renaming [2/2] Rename the transitory refs_werrres_ref_unsafe() function to refs_resolve_ref_unsafe(), now that all callers of the old function have learned to pass in a "failure_errno" parameter. The coccinelle semantic patch added in the preceding commit works, but I couldn't figure out how to get spatch(1) to re-flow these argument lists (and sometimes make lines way too long), so this rename was done with: perl -pi -e 's/refs_werrres_ref_unsafe/refs_resolve_ref_unsafe/g' \ $(git grep -l refs_werrres_ref_unsafe -- '*.c') But after that "make contrib/coccinelle/refs.cocci.patch" comes up empty, so the result would have been the same. Let's remove that transitory semantic patch file, we won't need to retain it for any other in-flight changes, refs_werrres_ref_unsafe() only existed within this patch series. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Ævar Arnfjörð Bjarmason	ed90f04155	refs API: make resolve_ref_unsafe() not set errno Change the resolve_ref_unsafe() wrapper function to use the underlying refs_werrres_ref_unsafe() directly. From a reading of the callers I determined that the only one who cared about errno was a sequencer.c caller added in `e47c6cafcb` (commit: move print_commit_summary() to libgit, 2017-11-24), I'm migrating it to using refs_werrres_ref_unsafe() directly. This adds another "set errno" instance, but in this case it's OK and idiomatic. We are setting it just before calling die_errno(). We could have some hypothetical die_errno_var(&saved_errno, ...) here, but I don't think it's worth it. The problem with errno is subtle action at distance, not this sort of thing. We already use this pattern in a couple of places in wrapper.c Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Ævar Arnfjörð Bjarmason	6e658547d3	sequencer: fix a memory leak in do_reset() Fix a memory leak introduced in `9055e401dd` (sequencer: introduce new commands to reset the revision, 2018-04-25), which called setup_unpack_trees_porcelain() without a corresponding call to clear_unpack_trees_porcelain(). This introduces a change in behavior in that we now start calling clear_unpack_trees_porcelain() even without having called the setup_unpack_trees_porcelain(). That's OK, that clear function, like most others, will accept a zero'd out struct. This inches us closer to passing various tests in "t34*.sh" (e.g. "t3434-rebase-i18n.sh"), but because they have so many other memory leaks in revisions.c this doesn't make any test file or even a single test pass. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Ævar Arnfjörð Bjarmason	0c52cf8e00	sequencer: add a "goto cleanup" to do_reset() Restructure code that's mostly added in `9055e401dd` (sequencer: introduce new commands to reset the revision, 2018-04-25) to avoid code duplication, and to make freeing other resources easier in a subsequent commit. It's safe to initialize "tree_desc" to be zero'd out in order to unconditionally free desc.buffer, it won't be initialized on the first couple of "goto"'s. There are three earlier "return"'s in this function which should probably be made to use this new "cleanup" too, per [1] it looks like they're leaving behind stale locks. But let's not try to fix every potential bug here now, I'm just trying to narrowly plug a memory leak. 1. https://lore.kernel.org/git/CABPp-BH=3DP-dXRCphY53-3eZd1TU8h5GY_M12nnbEGm-UYB9Q@mail.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Elijah Newren	1b5f37334a	Remove ignored files by default when they are in the way Change several commands to remove ignored files by default when they are in the way. Since some commands (checkout, merge) take a --no-overwrite-ignore option to allow the user to configure this, and it may make sense to add that option to more commands (and in the case of merge, actually plumb that configuration option through to more of the backends than just the fast-forwarding special case), add little comments about where such flags would be used. Incidentally, this fixes a test failure in t7112. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Elijah Newren	04988c8d18	unpack-trees: introduce preserve_ignored to unpack_trees_options Currently, every caller of unpack_trees() that wants to ensure ignored files are overwritten by default needs to: * allocate unpack_trees_options.dir * flip the DIR_SHOW_IGNORED flag in unpack_trees_options.dir->flags * call setup_standard_excludes AND then after the call to unpack_trees() needs to * call dir_clear() * deallocate unpack_trees_options.dir That's a fair amount of boilerplate, and every caller uses identical code. Make this easier by instead introducing a new boolean value where the default value (0) does what we want so that new callers of unpack_trees() automatically get the appropriate behavior. And move all the handling of unpack_trees_options.dir into unpack_trees() itself. While preserve_ignored = 0 is the behavior we feel is the appropriate default, we defer fixing commands to use the appropriate default until a later commit. So, this commit introduces several locations where we manually set preserve_ignored=1. This makes it clear where code paths were previously preserving ignored files when they should not have been; a future commit will flip these to instead use a value of 0 to get the behavior we want. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Ævar Arnfjörð Bjarmason	07a348e746	hook.c users: use "hook_exists()" instead of "find_hook()" Use the new hook_exists() function instead of find_hook() where the latter was called in boolean contexts. This make subsequent changes in a series where we further refactor the hook API clearer, as we won't conflate wanting to get the path of the hook with checking for its existence. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Ævar Arnfjörð Bjarmason	5e3aba33da	hook.[ch]: move find_hook() from run-command.c to hook.c Move the find_hook() function from run-command.c to a new hook.c library. This change establishes a stub library that's pretty pointless right now, but will see much wider use with Emily Shaffer's upcoming "configuration-based hooks" series. Eventually all the hook related code will live in hook.[ch]. Let's start that process by moving the simple find_hook() function over as-is. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Phillip Wood	2b88fe0603	rebase: fix todo-list rereading `54fd3243da` ("rebase -i: reread the todo list if `exec` touched it", 2017-04-26) sought to reread the todo list after running an exec command only if it had been changed. To accomplish this it checks the stat data of the todo list after running an exec command to see if it has changed. Unfortunately there are two problems, firstly the implementation is buggy we actually reread the list after each exec which is quadratic in the number of commit lookups and secondly the design is predicated on using nanosecond time stamps which are not the default. The implementation bug stems from the fact that we write a new todo list to disk before running each command but do not update the stat data to reflect this[1]. The design problem is that it is possible for the user to edit the todo list without changing its size or inode which means we have to rely on the mtime to tell us if it has changed. Unfortunately unless git is built with USE_NSEC it is possible for the original and edited list to share the same mtime. Ideally "git rebase --edit-todo" would set a flag that we would then check in sequencer.c. Unfortunately this is approach will not work as there are scripts in the wild that write to the todo list directly without running "git rebase --edit-todo". Instead of relying on stat data this patch simply reads the possibly edited todo list and compares it to the original with memcmp(). This is much faster than reparsing the todo list each time. This patch reduces the time to run git rebase -r -xtrue v2.32.0~100 v2.32.0 which runs 419 exec commands by 6.6%. For comparison fixing the implementation bug in stat based approach reduces the time by a further 1.4% and is indistinguishable from never rereading the todo list. [1] https://lore.kernel.org/git/20191125131833.GD23183@szeder.dev/ Reported-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Phillip Wood	dfa8bae5a2	sequencer.c: factor out a function This code is heavily indented and obscures the high level logic within the loop. Let's move it to its own function before modifying it in the next commit. Note that there is a subtle change in behavior if the todo list cannot be reread. Previously todo_list->current was incremented before returning, now it returns immediately. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Derrick Stolee	5d9c9349bd	sequencer: ensure full index if not ORT strategy The sequencer is used by 'cherry-pick' and 'rebase' to sequence a list of operations that modify the index. Since we intend to remove the need for 'command_requires_full_index', we need to ensure the sparse index is expanded every time it is written to disk between these steps. That is, unless the merge strategy is 'ort' where the index can remain sparse throughout. There are two main places to be extra careful about a full index: 1. Right before calling merge_trees(), ensure the index is full. This happens within an 'else' where the 'if' block checks if the 'ort' strategy is selected. 2. During read_and_refresh_cache(), the index might be written to disk and converted to sparse in the process. Ensure it expands back to full afterwards by checking if the strategy is _not_ 'ort'. This 'if' statement is the logical negation of the 'if' in item (1). Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Johannes Schindelin	17919c3585	sequencer: restrict scope of a formerly public function The function to add the `exec` commands to the todo list only needed to be public API because it was not only used internally by the sequencer, but also by `git rebase --preserve-merges`. Now that that mode has been removed, we no longer need that function to be scoped publicly. Helped-by: Alban Gruin <alban.gruin@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Josh Steadmon	767a4ca648	sequencer: advise if skipping cherry-picked commit Silently skipping commits when rebasing with --no-reapply-cherry-picks (currently the default behavior) can cause user confusion. Issue warnings when this happens, as well as advice on how to preserve the skipped commits. These warnings and advice are displayed only when using the (default) "merge" rebase backend. Update the git-rebase docs to mention the warnings and advice. Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Ben Boeckel	ed9bff0817	advice: remove read uses of most global `advice_` variables In `c4a09cc9cc` (Merge branch 'hw/advise-ng', 2020-03-25), a new API for accessing advice variables was introduced and deprecated `advice_config` in favor of a new array, `advice_setting`. This patch ports all but two uses which read the status of the global `advice_` variables over to the new `advice_enabled` API. We'll deal with advice_add_embedded_repo and advice_graft_file_deprecated separately. Signed-off-by: Ben Boeckel <mathstuf@gmail.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
ZheNing Hu	f172556b89	cherry-pick: use better advice message "git cherry-pick", upon seeing a conflict, says: hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' hint: and commit the result with 'git commit' as if running "git commit" to conclude the resolution of this single step were the end of the story. This stems from the fact that the command originally was to pick a single commit and not a range of commits, and the message was written back then and has not been adjusted. When picking a range of commits and the command stops with a conflict in the middle of the range, however, after resolving the conflict and (optionally) recording the result with "git commit", the user has to run "git cherry-pick --continue" to have the rest of the range dealt with, "--skip" to drop the current commit, or "--abort" to discard the series. Suggest use of "git cherry-pick --continue/--skip/--abort" so that the message also covers the case where a range of commits are being picked. Similarly, this optimization can be applied to git revert, suggest use of "git revert --continue/--skip/--abort" so that the message also covers the case where a range of commits are being reverted. It is worth mentioning that now we use advice() to print the content of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will start with "hint: ". Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk> Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: ZheNing Hu <adlternative@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Phillip Wood	f2563c9ef3	rebase -r: fix merge -c with a merge strategy If a rebase is started with a --strategy option other than "ort" or "recursive" then "merge -c" does not allow the user to reword the commit message. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Phillip Wood	baf8ec8d3a	rebase -r: don't write .git/MERGE_MSG when fast-forwarding When fast-forwarding we do not create a new commit so .git/MERGE_MSG is not removed and can end up seeding the message of a commit made after the rebase has finished. Avoid writing .git/MERGE_MSG when we are fast-forwarding by writing the file after the fast-forward checks. Note that there are no changes to the fast-forward code, it is simply moved. Note that the way this change is implemented means we no longer write the author script when fast-forwarding either. I believe this is safe for the reasons below but it is a departure from what we do when fast-forwarding a non-merge commit. If we reword the merge then 'git commit --amend' will keep the authorship of the commit we're rewording as it ignores GIT_AUTHOR_* unless --reset-author is passed. It will also export the correct GIT_AUTHOR_* variables to any hooks and we already test the authorship of the reworded commit. If we are not rewording then we no longer call spilt_ident() which means we are no longer checking the commit author header looks sane. However this is what we already do when fast-forwarding non-merge commits in skip_unnecessary_picks() so I don't think we're breaking any promises by not checking the author here. Reported-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Phillip Wood	2be6b6f411	rebase -r: make 'merge -c' behave like reword If the user runs git log while rewording a commit it is confusing if sometimes we're amending the commit that's being reworded and at other times we're creating a new commit depending on whether we could fast-forward or not[1]. For this reason the reword command ensures that there are no uncommitted changes when rewording. The reword command also allows the user to edit the todo list while the rebase is paused. As 'merge -c' also rewords commits make it behave like reword and add a test. [1] https://lore.kernel.org/git/xmqqlfvu4be3.fsf@gitster-ct.c.googlers.com/T/#m133009cb91cf0917bcf667300f061178be56680a Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Phillip Wood	e5ee33e855	rebase --continue: remove .git/MERGE_MSG If the user skips the final commit by removing all the changes from the index and worktree with 'git restore' (or read-tree) and then runs 'git rebase --continue' .git/MERGE_MSG is left behind. This will seed the commit message the next time the user commits which is not what we want to happen. Reported-by: Victor Gambier <vgambier@excilys.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Elijah Newren	6a5fb96672	Change default merge backend from recursive to ort There are a few reasons to switch the default: * Correctness * Extensibility * Performance I'll provide some summaries about each. === Correctness === The original impetus for a new merge backend was to fix issues that were difficult to fix within recursive's design. The success with this goal is perhaps most easily demonstrated by running the following: $ git grep -2 KNOWN_FAILURE t/ \| grep -A 4 GIT_TEST_MERGE_ALGORITHM $ git grep test_expect_merge_algorithm.failure.success t/ $ git grep test_expect_merge_algorithm.success.failure t/ In order, these greps show: * Seven sets of submodule tests (10 total tests) that fail with recursive but succeed with ort * 22 other tests that fail with recursive, but succeed with ort * 0 tests that pass with recursive, but fail with ort === Extensibility === Being able to perform merges without touching the working tree or index makes it possible to create new features that were difficult with the old backend: * Merging, cherry-picking, rebasing, reverting in bare repositories... or just on branches that aren't checked out. * `git diff AUTO_MERGE` -- ability to see what changes the user has made to resolve conflicts so far (see commit `5291828df8` ("merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict", 2021-03-20) * A --remerge-diff option for log/show, used to show diffs for merges that display the difference between what an automatic merge would have created and what was recorded in the merge. (This option will often result in an empty diff because many merges are clean, but for the non-clean ones it will show how conflicts were fixed including the removal of conflict markers, and also show additional changes made outside of conflict regions to e.g. fix semantic conflicts.) * A --remerge-diff-only option for log/show, similar to --remerge-diff but also showing how cherry-picks or reverts differed from what an automatic cherry-pick or revert would provide. The last three have been implemented already (though only one has been submitted upstream so far; the others were waiting for performance work to complete), and I still plan to implement the first one. === Performance === I'll quote from the summary of my final optimization for merge-ort (while fixing the testcase name from 'no-renames' to 'few-renames'): Timings Infinite merge- merge- Parallelism recursive recursive of rename merge-ort v2.30.0 current detection current ---------- --------- ----------- --------- few-renames: 18.912 s 18.030 s 11.699 s 198.3 ms mega-renames: 5964.031 s 361.281 s 203.886 s 661.8 ms just-one-mega: 149.583 s 11.009 s 7.553 s 264.6 ms Speedup factors Infinite merge- merge- Parallelism recursive recursive of rename v2.30.0 current detection merge-ort ---------- --------- ----------- --------- few-renames: 1 1.05 1.6 95 mega-renames: 1 16.5 29 9012 just-one-mega: 1 13.6 20 565 And, for partial clone users: Factor reduction in number of objects needed Infinite merge- merge- Parallelism recursive recursive of rename v2.30.0 current detection merge-ort ---------- --------- ----------- --------- mega-renames: 1 1 1 181.3 Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Elijah Newren	81483fe613	Update error message and code comment There were two locations in the code that referred to 'merge-recursive' but which were also applicable to 'merge-ort'. Update them to more general wording. Acked-by: Derrick Stolee <dstolee@microsoft.com> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Johannes Schindelin	a03b097d63	Use a better name for the function interpolating paths It is not immediately clear what `expand_user_path()` means, so let's rename it to `interpolate_path()`. This also opens the path for interpolating more than just a home directory. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Ævar Arnfjörð Bjarmason	48ca53cac4	*.c static functions: add missing __attribute__((format)) Add missing __attribute__((format)) function attributes to various "static" functions that take printf arguments. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Ævar Arnfjörð Bjarmason	d4ac305073	sequencer.c: move static function to avoid forward decl Move the reflog_message() function added in `96e832a5fd` (sequencer (rebase -i): refactor setting the reflog message, 2017-01-02), it gained another user in `9055e401dd` (sequencer: introduce new commands to reset the revision, 2018-04-25). Let's move it around and remove the forward declaration added in the latter commit. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
brian m. carlson	14228447c9	hash: provide per-algorithm null OIDs Up until recently, object IDs did not have an algorithm member, only a hash. Consequently, it was possible to share one null (all-zeros) object ID among all hash algorithms. Now that we're going to be handling objects from multiple hash algorithms, it's important to make sure that all object IDs have a correct algorithm field. Introduce a per-algorithm null OID, and add it to struct hash_algo. Introduce a wrapper function as well, and use it everywhere we used to use the null_oid constant. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Ævar Arnfjörð Bjarmason	e5b32bffd1	rebase: don't override --no-reschedule-failed-exec with config Fix a bug in how --no-reschedule-failed-exec interacts with rebase.rescheduleFailedExec=true being set in the config. Before this change the --no-reschedule-failed-exec config option would be overridden by the config. This bug happened because of the particulars of how "rebase" works v.s. most other git commands when it comes to parsing options and config: When we read the config and parse the CLI options we correctly prefer the --no-reschedule-failed-exec option over rebase.rescheduleFailedExec=true in the config. So far so good. However the --reschedule-failed-exec option doesn't take effect when the rebase starts (we'd just create a ".git/rebase-merge/reschedule-failed-exec" file if it was true). It only takes effect when the exec command fails, at which point we'll reschedule the failed "exec" command. Since we only wrote out the positive ".git/rebase-merge/reschedule-failed-exec" under --reschedule-failed-exec, but nothing with --no-reschedule-failed-exec we'll forget that we asked not to reschedule failed "exec", and would happily re-read the config and see that rebase.rescheduleFailedExec=true is set. So the config will effectively override the user having explicitly disabled the option on the command-line. Even more confusingly: Since rebase accepts different options based on its state there wasn't even a way to get around this with "rebase --continue --no-reschedule-failed-exec" (but you could of course set the config with "rebase -c ..."). I think the least bad way out of this is to declare that for such options and config whatever we decide at the beginning of the rebase goes. So we'll now always create either a "reschedule-failed-exec" or a "no-reschedule-failed-exec file at the start, not just the former if we decided we wanted the feature. With this new worldview you can no longer change the setting once a rebase has started except by manually removing the state files discussed above. I think making it work like that is the the least confusing thing we can do. In the future we might want to learn to change the setting in the middle by combining "--edit-todo" with "--[no-]reschedule-failed-exec", we currently don't support combining those options, or any other way to change the state in the middle of the rebase short of manually editing the files in ".git/rebase-merge/*". The bug being fixed here originally came about because of a combination of the behavior of the code added in `d421afa0c6` (rebase: introduce --reschedule-failed-exec, 2018-12-10) and the addition of the config variable in `969de3ff0e` (rebase: add a config option to default to --reschedule-failed-exec, 2018-12-10). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Elijah Newren	39edfd5cbc	sequencer: fix edit handling for cherry-pick and revert messages save_opts() should save any non-default values. It was intended to do this, but since most options in struct replay_opts default to 0, it only saved non-zero values. Unfortunately, this does not always work for options.edit. Roughly speaking, options.edit had a default value of 0 for cherry-pick but a default value of 1 for revert. Make save_opts() record a value whenever it differs from the default. options.edit was also overly simplistic; we had more than two cases. The behavior that previously existed was as follows: Non-conflict commits Right after Conflict revert Edit iff isatty(0) Edit (ignore isatty(0)) cherry-pick No edit See above Specify --edit Edit (ignore isatty(0)) See above Specify --no-edit () See above () Before stopping for conflicts, No edit is the behavior. After stopping for conflicts, the --no-edit flag is not saved so see the first two rows. However, the expected behavior is: Non-conflict commits Right after Conflict revert Edit iff isatty(0) Edit iff isatty(0) cherry-pick No edit Edit iff isatty(0) Specify --edit Edit (ignore isatty(0)) Edit (ignore isatty(0)) Specify --no-edit No edit No edit In order to get the expected behavior, we need to change options.edit to a tri-state: unspecified, false, or true. When specified, we follow what it says. When unspecified, we need to check whether the current commit being created is resolving a conflict as well as consulting options.action and isatty(0). While at it, add a should_edit() utility function that compresses options.edit down to a boolean based on the additional information for the non-conflict case. continue_single_pick() is the function responsible for resuming after conflict cases, regardless of whether there is one commit being picked or many. Make this function stop assuming edit behavior in all cases, so that it can correctly handle !isatty(0) and specific requests to not edit the commit message. Reported-by: Renato Botelho <garga@freebsd.org> Signed-off-by: Elijah Newren <newren@gmail.com> Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Elijah Newren	5291828df8	merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict There are a variety of questions users might ask while resolving conflicts: * What changes have been made since the previous (first) parent? * What changes are staged? * What is still unstaged? (or what is still conflicted?) * What changes did I make to resolve conflicts so far? The first three of these have simple answers: * git diff HEAD * git diff --cached * git diff There was no way to answer the final question previously. Adding one is trivial in merge-ort, since it works by creating a tree representing what should be written to the working copy complete with conflict markers. Simply write that tree to .git/AUTO_MERGE, allowing users to answer the fourth question with * git diff AUTO_MERGE I avoided using a name like "MERGE_AUTO", because that would be merge-specific (much like MERGE_HEAD, REBASE_HEAD, REVERT_HEAD, CHERRY_PICK_HEAD) and I wanted a name that didn't change depending on which type of operation the merge was part of. Ensure that paths which clean out other temporary operation-specific files (e.g. CHERRY_PICK_HEAD, MERGE_MSG, rebase-merge/ state directory) also clean out this AUTO_MERGE file. Signed-off-by: Elijah Newren <newren@gmail.com> Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Charvi Mendiratta	6e0e288779	sequencer: export and rename subject_length() This function can be used in other parts of git. Let's move the function to commit.c and also rename it to make the name of the function more generic. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
René Scharfe	ca56dadb4b	use CALLOC_ARRAY Add and apply a semantic patch for converting code that open-codes CALLOC_ARRAY to use it instead. It shortens the code and infers the element size automatically. Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Charvi Mendiratta	1f9696019a	sequencer: rename a few functions Rename functions to make them more descriptive and while at it, remove unnecessary 'inline' of the skip_fixupish() function. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Charvi Mendiratta	a25314c1ec	sequencer: fixup the datatype of the 'flag' argument As 'flag' is a combination of bits, so change its datatype from 'enum todo_item_flags' to 'unsigned'. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago

1 2 3 4 5 ...

785 Commits (2088a0c0cd61ab6c98064e68619e1d931a4619e2)