* Re: [PATCH v2 3/7] maintenance: add --scheduled option and config
From: Derrick Stolee @ 2020-08-27 15:47 UTC (permalink / raw)
To: Junio C Hamano, Derrick Stolee via GitGitGadget
Cc: git, sandals, steadmon, jrnieder, peff, congdanhqx,
phillip.wood123, emilyshaffer, sluongng, jonathantanmy,
Derrick Stolee, Derrick Stolee, Jeff Hostetler
In-Reply-To: <bd4e18b7-6265-73e7-bc1a-a7d647eafd0a@gmail.com>
On 8/26/2020 11:30 AM, Derrick Stolee wrote:
> Let users specify a schedule frequency among this list: hourly, daily,
> weekly, monthly. We then set the following* crontab:
>
> 0 * * * * git for-each-repo --config=maintenance.repos maintenance run --scheduled=hourly
> 0 0 * * * git for-each-repo --config=maintenance.repos maintenance run --scheduled=daily
> 0 0 * * 0 git for-each-repo --config=maintenance.repos maintenance run --scheduled=weekly
> 0 0 0 * * git for-each-repo --config=maintenance.repos maintenance run --scheduled=monthly
>
> *Of course, there is some care around "$path/git --exec-path=$path"
> that I drop for ease here.
Jeff Hostetler pointed out the following details in the crontab
documentation [1]:
Ranges of numbers are allowed. Ranges are two numbers separated with
a hyphen. The specified range is inclusive. For example, 8-11 for
an 'hours' entry specifies execution at hours 8, 9, 10, and 11. The
first number must be less than or equal to the second one.
[1] https://man7.org/linux/man-pages/man5/crontab.5.html
This means we could try this schedule:
0 1-23 * * * git for-each-repo --config=maintenance.repos maintenance run --scheduled=hourly
0 0 * * 1-6 git for-each-repo --config=maintenance.repos maintenance run --scheduled=daily
0 0 1-30 * 0 git for-each-repo --config=maintenance.repos maintenance run --scheduled=weekly
0 0 0 * * git for-each-repo --config=maintenance.repos maintenance run --scheduled=monthly
And it should behave this way:
Run --scheduled=hourly every hour, except at midnight. This runs
all "hourly" tasks.
Run --scheduled=daily at midnight, except on Sunday. This runs all
"hourly" and "daily" tasks.
Run --scheduled=weekly at midnight Sunday, except on the first day
of the month. This runs all "hourly", "daily", and "weekly" tasks.
Run --scheduled=monthly at midnight on the first day of the month.
This runs all scheduled tasks.
There is some subtlety between whether the "weekly" runs should be a
subset of "monthly" and maybe the easiest way to handle that would
be to not support "monthly" and have only "hourly", "daily", and "weekly"
options for now.
This should get around all of the parallel issues and allow us to drop
the *.lastRun config option.
Thoughts?
Thanks,
-Stolee
^ permalink raw reply
* Re: post-checkout hook aborts rebase
From: Junio C Hamano @ 2020-08-27 15:51 UTC (permalink / raw)
To: Chris Torek; +Cc: Tom Rutherford, Git List
In-Reply-To: <CAPx1GvfSt=s5VP9_+ZtndHWaBZ5W7nFxAf8bTF2tXnJkS95Dfg@mail.gmail.com>
Chris Torek <chris.torek@gmail.com> writes:
> This might be intended as a feature, but if so, the documentation
> needs a tweak: the githooks docs say in part
>
> This hook cannot affect the outcome of git checkout.
>
> If "outcome" includes exit status -- to me, it does -- either the docs
> are wrong or the code is wrong.
I suspected that this was a case of adding the hook that just runs
without affecting the exit code and then somebody later changed the
behaviour (either deliberately or by mistake) and forgot to update
the documentation. But it seems that ever since the hook support
was introduced at 1abbe475 (post-checkout hook, tests, and docs,
2007-09-26), the command was made to exit with the same status from
the hook.
So I agree that depending on reader's view on "the outcome", this is
documented incorrectly (if the exit code is part of "the outcome")
or just fine (otherwise). Apparently, the author of the patch and
the reviewers back then thought the latter.
I still suspect that the checkout run, merely as an implementation
detail of rebase (or any other git subcommand), should not trigger
any hook, but before any such code change, at least let's update the
documentation to clarify what we mean by "the outcome".
Hopefully something like the below may be a good starting point?
Documentation/githooks.txt | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 31b601e4bc..cf95d6d02b 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -193,7 +193,9 @@ worktree. The hook is given three parameters: the ref of the previous HEAD,
the ref of the new HEAD (which may or may not have changed), and a flag
indicating whether the checkout was a branch checkout (changing branches,
flag=1) or a file checkout (retrieving a file from the index, flag=0).
-This hook cannot affect the outcome of `git switch` or `git checkout`.
+This hook cannot affect the outcome of `git switch` or `git checkout`,
+other than that the hook's exit status becomes the exit status of
+these two commands.
It is also run after linkgit:git-clone[1], unless the `--no-checkout` (`-n`) option is
used. The first parameter given to the hook is the null-ref, the second the
^ permalink raw reply related
* Cleaning up files reported by size-garbage
From: Konstantin Ryabitsev @ 2020-08-27 15:55 UTC (permalink / raw)
To: git
Hello:
Running git count-objects -v reports garbage files:
$ git count-objects -v
warning: garbage found: ./objects/pack/tmp_pack_XSv8MO
warning: garbage found: ./objects/pack/tmp_pack_2uOuMg
warning: garbage found: ./objects/pack/tmp_pack_KzP1ja
count: 19
size: 84
in-pack: 172456
packs: 6
size-pack: 63907
prune-packable: 0
garbage: 3
size-garbage: 1911
Is there a way to tell git to clean those up? I'm not finding anything
and would rather avoid having to parse stderr in these cases.
Best,
-K
^ permalink raw reply
* Re: feature request - add --only-author option to git push
From: Junio C Hamano @ 2020-08-27 15:58 UTC (permalink / raw)
To: Toni Brkic; +Cc: Pratyush Yadav, git
In-Reply-To: <CAF2SHyD2EO7o7RhKRGmMaffAa2=6z_rkO+CeBPWREObD=4HmuQ@mail.gmail.com>
Toni Brkic <brkict@gmail.com> writes:
> ... If this would be
> something that would be of interest to be added I could take a look
> myself and try
> to write a patch.
I am personally not interested with "all my commits must be from me
and nobody else", but it might be a useful thing to have if we can
come up with an easy way to give a "git shortlog [-s]" (and other
"history summary" command the user can ask with configuration
variables and such) over the range of commits that will be pushed
(which theoretically cannot be known before "git push" starts
running) before the actual push happens.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/5] worktree: add skeleton "repair" command
From: Junio C Hamano @ 2020-08-27 16:08 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Henré Botha, Jeff King
In-Reply-To: <20200827082129.56149-2-sunshine@sunshineco.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> Worktree administrative files can become corrupted or outdated due to
> external factors. Although, it is often possible to recover from such
> situations by hand-tweaking these files, doing so requires intimate
> knowledge of worktree internals. While information necessary to make
> such repairs manually can be obtained from git-worktree.txt and
> gitrepository-layout.txt, we can assist users more directly by teaching
> git-worktree how to repair its administrative files itself (at least to
> some extent). Therefore, add a "git worktree repair" command which
> attempts to correct common problems which may arise due to factors
> beyond Git's control.
>
> At this stage, the "repair" command is a mere skeleton; subsequent
> commits will flesh out the functionality.
Sounds good. It looked a bit odd to have skeleton test script only
to reserve its test number, but it is just odd and not wrong.
Let's read on.
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
> Documentation/git-worktree.txt | 6 ++++++
> builtin/worktree.c | 15 +++++++++++++++
> t/t2406-worktree-repair.sh | 11 +++++++++++
> 3 files changed, 32 insertions(+)
> create mode 100755 t/t2406-worktree-repair.sh
>
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 6ee6ec7982..ae432d39a8 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -15,6 +15,7 @@ SYNOPSIS
> 'git worktree move' <worktree> <new-path>
> 'git worktree prune' [-n] [-v] [--expire <expire>]
> 'git worktree remove' [-f] <worktree>
> +'git worktree repair'
> 'git worktree unlock' <worktree>
>
> DESCRIPTION
> @@ -110,6 +111,11 @@ and no modification in tracked files) can be removed. Unclean working
> trees or ones with submodules can be removed with `--force`. The main
> working tree cannot be removed.
>
> +repair::
> +
> +Repair working tree administrative files, if possible, if they have
> +become corrupted or outdated due to external factors.
> +
> unlock::
>
> Unlock a working tree, allowing it to be pruned, moved or deleted.
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 378f332b5d..88af412d4f 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -1030,6 +1030,19 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
> return ret;
> }
>
> +static int repair(int ac, const char **av, const char *prefix)
> +{
> + struct option options[] = {
> + OPT_END()
> + };
> + int rc = 0;
> +
> + ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> + if (ac)
> + usage_with_options(worktree_usage, options);
> + return rc;
> +}
> +
> int cmd_worktree(int ac, const char **av, const char *prefix)
> {
> struct option options[] = {
> @@ -1056,5 +1069,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
> return move_worktree(ac - 1, av + 1, prefix);
> if (!strcmp(av[1], "remove"))
> return remove_worktree(ac - 1, av + 1, prefix);
> + if (!strcmp(av[1], "repair"))
> + return repair(ac - 1, av + 1, prefix);
> usage_with_options(worktree_usage, options);
> }
> diff --git a/t/t2406-worktree-repair.sh b/t/t2406-worktree-repair.sh
> new file mode 100755
> index 0000000000..cc679e1a21
> --- /dev/null
> +++ b/t/t2406-worktree-repair.sh
> @@ -0,0 +1,11 @@
> +#!/bin/sh
> +
> +test_description='test git worktree repair'
> +
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> + test_commit init
> +'
> +
> +test_done
^ permalink raw reply
* [PATCH] gitk: use the 'reference' format for the commit reference
From: Beat Bolli @ 2020-08-27 16:12 UTC (permalink / raw)
To: Paul Mackerras; +Cc: git, Beat Bolli
Since 1f0fc1db85 (pretty: implement 'reference' format, 2019-11-19) in
the main Git repository, there's an officially blessed format for commit
references. Update gitk to also use this format.
Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
gitk | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/gitk b/gitk
index da84e22..b26996c 100755
--- a/gitk
+++ b/gitk
@@ -9433,8 +9433,7 @@ proc mktaggo {} {
proc copyreference {} {
global rowmenuid autosellen
- set format "%h (\"%s\", %ad)"
- set cmd [list git show -s --pretty=format:$format --date=short]
+ set cmd [list git show -s --pretty=reference]
if {$autosellen < 40} {
lappend cmd --abbrev=$autosellen
}
--
2.26.0.277.gb8618d28a9
^ permalink raw reply related
* Re: [PATCH] gitk: use the 'reference' format for the commit reference
From: Eric Sunshine @ 2020-08-27 16:26 UTC (permalink / raw)
To: Beat Bolli; +Cc: Paul Mackerras, Git List
In-Reply-To: <20200827161224.824365-1-dev+git@drbeat.li>
On Thu, Aug 27, 2020 at 12:19 PM Beat Bolli <dev+git@drbeat.li> wrote:
> Since 1f0fc1db85 (pretty: implement 'reference' format, 2019-11-19) in
> the main Git repository, there's an officially blessed format for commit
> references. Update gitk to also use this format.
>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
See this thread:
https://lore.kernel.org/git/da9321b1bd56aafd16c8dcb99d5d628b79e2244e.1576100147.git.liu.denton@gmail.com/T/
^ permalink raw reply
* Re: post-checkout hook aborts rebase
From: Elijah Newren @ 2020-08-27 16:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tom Rutherford, Git Mailing List
In-Reply-To: <xmqq3649szs8.fsf@gitster.c.googlers.com>
On Wed, Aug 26, 2020 at 5:24 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > ... If "git rebase" or whatever
> > command wanted to place files and the index into some state by using
> > "git checkout" command, and if the post-checkout hook mucked with
> > the state in such a way that contradicts with what the "git rebase"
> > command wanted them to be in, it is not surprising the hook's behavior
> > broke "git rebase"'s operation.
>
> Having said all that, I actually think that "rebase" shouldn't be
> invoking "git checkout" (and its equivalent) internally when
> switching to a specific version, in such a way that it would trigger
> any end-user specified hooks and allow them to muck with the working
> tree and the index state.
>
> I haven't checked the actual implementation of "git rebase" for
> quite some time to be sure, but we have lower-level plumbing
> commands that are not affected by the end-user hooks for exactly
> that kind of "build higher-level commands by synthesis of
> lower-level machinery", and it is very possible that what we are
> looking at is actually a bug that needs to be fixed. I dunno.
>
> Thanks.
Yes, and I think we should also make rebase stop invoking "git commit" too.
^ permalink raw reply
* Re: [PATCH 2/5] worktree: teach "repair" to fix worktree back-links to main worktree
From: Junio C Hamano @ 2020-08-27 17:05 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Henré Botha, Jeff King
In-Reply-To: <20200827082129.56149-3-sunshine@sunshineco.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> The .git file in a linked worktree is a "gitlink" which points back to
Please never call it a "gitlink" (which is a word reserved for a
cache entry with 160000 mode that typically point at a commit
object, an implementation detail of the mechanism to bind a
submodule to its superproject) to avoid confusing future readers of
our code and documentation. The setup.c code calls it as "gitfile",
since it was introduced at b44ebb19 (Add platform-independent .git
"symlink", 2008-02-20), I think, and that is how it has been described
in the glossary.
> the .git/worktrees/<id> entry in the main worktree or bare repository.
> If a worktree's .git file is deleted or becomes corrupted or outdated,
> then the linked worktree won't know how to find the repository or any of
> its own administrative files (such as 'index', 'HEAD', etc.). An easy
> way for the .git file to become outdated is for the user to move the
> main worktree or bare repository.
I am not sure if a directory that used to be a secondary worktree
would still appear to be a Git controlled worktree after its ".git"
removed, but I guess a ".git" pointing at a wrong place won't,
either. I am just curious how much more involved in dealing with
the "is deleted" situation than "becomes corrupted" situation.
An obvious way is to ask the users to say "Here in directory D
should be a gitfile that points at the primary copy X---D/.git may
be missing or corrupt so please go create or fix it" without letting
any "auto repository discovery" logic to kick in. For that, I suspect
you may have to disable RUN_SETUP in git.c for "worktree" built-in,
and run the setup sequence manually for all the existing subcommands
except for this new "repair" subcommand. We'll see.
> Although it is possible to manually
> update each linked worktree's .git file to reflect the new repository
> location, doing so requires a level of knowledge about worktree
> internals beyond what a user should be expected to know offhand.
>
> Therefore, teach "git worktree repair" how to repair broken or outdated
> worktree .git files automatically. (For this to work, the command must
> be invoked from within the main worktree or bare repository, or from
> within a worktree which has not become disconnected from the repository
> -- such as one which was created after the repository was moved.)
Hmph, ok, that is not quite satisfactory, but it would work. So
instead of
user goes to a directory D, thinking it is a valid secondary
worktree and wanting to work in it, and finds that Git does not
think it is.
user tells "worktree repair" that X is the location of the
primary copy, and cwd is supposed to be the top of the working
tree of a secondary worktree of it.
"worktree repair" creates/updates ./.git gitfile to point at X.
user starts working.
sequence, the user does
user goes to a directory D, thinking it is a valid secondary
worktree and wanting to work in it, and finds that Git does not
think it is.
user goes to X, which is the location of the primary copy, and
tells "worktree repair" that the directory D ought to be the top
of the working tree for a secondary worktree.
"worktree repair" creates/updates D/.git gitfile to point at X.
user comes back to D and starts working.
because "git worktree repair" wants to do the usual setup sequence
anyway.
OK.
> Move a working tree to a new location. Note that the main working tree
> -or linked working trees containing submodules cannot be moved.
> +or linked working trees containing submodules cannot be moved with this
> +command. (The `git worktree repair` command, however, can reestablish
> +the connection with linked working trees if you move the main working
> +tree manually.)
... meaning after moving the main working tree, repair can be used
to touch-up the submodule directories?
Ah, no. You are saying "git worktree move" still cannot be used to
move the main working tree (the ones with submodules in it we do not
even care), but as a workaround, the user can "mv" it manually and
run "repair" to fix all the secondary worktrees.
Hopefully somewhere in the rest of this series it would become
automatic? After all, instead of letting users "mv" things without
us knowing what is going on, if we let them say "worktree move" the
primary working tree, we know both the source and the destination
directories of such a move, and because we know all the secondary
worktrees, we can run "worktree repair" on them as part of this
"worktree move" of the primary working tree.
It is perfectly fine that it is not happening here in this step to
keep each step digestible, of course.
Let's read on.
> @@ -115,6 +118,11 @@ repair::
>
> Repair working tree administrative files, if possible, if they have
> become corrupted or outdated due to external factors.
> ++
> +For instance, if the main working tree (or bare repository) is moved,
> +linked working trees will be unable to locate it. Running `repair` in
> +the recently-moved main working tree will reestablish the connection
> +from linked working trees back to the main working tree.
Does "recently-" have a positive value in this paragraph? It makes
readers wonder how long the zombie would stay resurrectable, which
would encourage to run this command while s/he still remembers that
the primary copy got moved, but I am not sure if that is a plus.
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 88af412d4f..62e33eb7f5 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -1030,6 +1030,16 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
> return ret;
> }
>
> +static void repair_cb(int iserr, const char *path, const char *msg, void *cb_data)
repair_cb as the name for a parameter to repair_worktrees() API
function is an excellent name, but as the name for a concrete
instance of such a callback function, it is a horrible one. Name it
after what it does or what it is for in the context of this
particular use of that API function, e.g. "report repair status".
> +{
> + if (!iserr)
> + printf_ln(_("repair: %s: %s"), msg, path);
> + else {
> + fprintf_ln(stderr, _("error: %s: %s"), msg, path);
> + *(int *)cb_data = 1;
> + }
Spend a local variable would pay for readability, e.g.
{
int *exit_status = (int *)cb_data;
if (!iserr) {
... report success ...
} else {
... report failure ...
*exit_status = 1;
}
}
would make it clearer that the number '1' is assigned to update the
value used as the exit status. With the same callback API, the
caller could be counting the number of secondaries failed to be
resurrected, in which case assignment of 1 is a potential bug
that needs to be updated to
*(int *)cb_data += 1;
but the reader cannot tell from the generic name cb_data.
> +}
> +
> static int repair(int ac, const char **av, const char *prefix)
> {
> struct option options[] = {
> @@ -1040,6 +1050,7 @@ static int repair(int ac, const char **av, const char *prefix)
> ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> if (ac)
> usage_with_options(worktree_usage, options);
> + repair_worktrees(repair_cb, &rc);
> return rc;
> }
>
> diff --git a/t/t2406-worktree-repair.sh b/t/t2406-worktree-repair.sh
> index cc679e1a21..9379a63130 100755
> --- a/t/t2406-worktree-repair.sh
> +++ b/t/t2406-worktree-repair.sh
> @@ -8,4 +8,76 @@ test_expect_success setup '
> test_commit init
> '
>
> +test_expect_success 'skip missing worktree' '
> + test_when_finished "git worktree prune" &&
> + git worktree add --detach missing &&
> + rm -rf missing &&
> + git worktree repair >out 2>err &&
> + test_must_be_empty out &&
> + test_must_be_empty err
> +'
> +
> +test_expect_success "don't clobber .git repo" '
> + test_when_finished "rm -rf repo && git worktree prune" &&
> + git worktree add --detach repo &&
> + rm -rf repo &&
> + test_create_repo repo &&
> + test_must_fail git worktree repair >out 2>err &&
> + test_must_be_empty out &&
> + test_i18ngrep ".git is not a file" err
> +'
> +
> +test_corrupt_gitlink () {
See gitglossary::gitfile. We'd need to find all references to
"gitlink" and "git link" in this series and decide which ones need
to be fixed (there might be genuine references to gitlink---I
haven't read the series through).
> diff --git a/worktree.c b/worktree.c
> index 62217b4a6b..029ce91fdf 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -571,3 +571,56 @@ int other_head_refs(each_ref_fn fn, void *cb_data)
> free_worktrees(worktrees);
> return ret;
> }
> +
> +/*
> + * Repair worktree's /path/to/worktree/.git link if missing, corrupt, or not
> + * pointing at <repo>/worktrees/<id>.
> + */
> +static void repair_dotgit(struct worktree *wt,
> + worktree_repair_cb *cb, void *cb_data)
"dotgit" is an OK name in this context, I would think. repair_gitfile
is also fine, but "dotgit" may be more explicit.
It is customary to call the callback function "fn", not "cb", which
sometimes used as a short-hand for "cb_data".
> +{
> + struct strbuf dotgit = STRBUF_INIT;
> + struct strbuf repo = STRBUF_INIT;
> + char *backlink;
> + const char *repair = NULL;
> + int err;
> +
> + /* missing worktree can't be repaired */
> + if (!file_exists(wt->path))
> + return;
Shouldn't it be a bit stronger? if wt->path is not a directory, it
cannot be the top of the working tree of a secondary worktree.
> + strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
OK, the need to use git_common_path() is a good justification why it
is easier to implement if we initialize ourselves using the primary
copy's repository data (i.e. require the user to start "worktree
repair" in the primary copy or any working secondary worktrees, and
let the normal setup.c sequence to prime us in the repository),
instead of allowing the user to start at the secondary worktree
whose gitfile got broken.
> + strbuf_addf(&dotgit, "%s/.git", wt->path);
> + backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
> +
> + if (err == READ_GITFILE_ERR_NOT_A_FILE)
> + cb(1, wt->path, _(".git is not a file"), cb_data);
> + else if (err)
> + repair = _(".git link broken");
> + else if (fspathcmp(backlink, repo.buf))
> + repair = _(".git link incorrect");
> + if (repair) {
> + cb(0, wt->path, repair, cb_data);
> + write_file(dotgit.buf, "gitdir: %s", repo.buf);
Shouldn't write_file() be monitored for its failure, and the failure
reported back to the callback function?
> + }
> +
> + free(backlink);
> + strbuf_release(&repo);
> + strbuf_release(&dotgit);
> +}
> +
> +static void repair_noop_cb(int iserr, const char *path, const char *msg,
> + void *cb_data) {}
Even the body is empty, just follow the coding guidelines, i.e.
static void function_name(parameter list)
{
/* nothing */
}
> +void repair_worktrees(worktree_repair_cb *cb, void *cb_data)
> +{
> + struct worktree **worktrees = get_worktrees();
> + struct worktree **wt = worktrees + 1; /* +1 skips main worktree */
> +
> + if (!cb)
> + cb = repair_noop_cb;
> + for (; *wt; wt++)
> + repair_dotgit(*wt, cb, cb_data);
> + free_worktrees(worktrees);
> +}
This "repair"s only in one direction. Manual movement of secondary
worktrees, if we want to support repairing, needs the user to tell
where the new location of the secondary is, and we need a code to
update the record of the location of the secondary kept at the main
worktree, which is not needed in the repair implemented by this
step.
> diff --git a/worktree.h b/worktree.h
> index 516744c433..7d085c7b91 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -89,6 +89,17 @@ int validate_worktree(const struct worktree *wt,
> void update_worktree_location(struct worktree *wt,
> const char *path_);
>
> +typedef void worktree_repair_cb(int iserr, const char *path, const char *msg,
> + void *cb_data);
> +
> +/*
> + * Visit each registered linked worktree and repair corruptions. For each
> + * repair made or error encountered while attempting a repair, the callback, if
> + * non-NULL, is called with the path of the worktree and a description of the
> + * repair or error, along with the callback user-data.
> + */
> +void repair_worktrees(worktree_repair_cb *, void *cb_data);
> +
> /*
> * Free up the memory for worktree(s)
> */
Looking good.
Thanks.
^ permalink raw reply
* Re: [PATCH 3/5] worktree: teach "repair" to fix outgoing links to worktrees
From: Junio C Hamano @ 2020-08-27 17:14 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Henré Botha, Jeff King
In-Reply-To: <20200827082129.56149-4-sunshine@sunshineco.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> The .git/worktrees/<id>/gitdir file points at the location of a linked
> worktree's .git file. Its content must be of the form
> /path/to/worktree/.git (from which the location of the worktree itself
> can be derived by stripping the "/.git" suffix). If the gitdir file is
> deleted or becomes corrupted or outdated, then Git will be unable to
> find the linked worktree. An easy way for the gitdir file to become
> outdated is for the user to move the worktree manually (without using
> "git worktree move"). Although it is possible to manually update the
> gitdir file to reflect the new linked worktree location, doing so
> requires a level of knowledge about worktree internals beyond what a
> user should be expected to know offhand.
>
> Therefore, teach "git worktree repair" how to repair broken or outdated
> .git/worktrees/<id>/gitdir files automatically. (For this to work, the
> command must either be invoked from within the worktree whose gitdir
> file requires repair, or from within the main or any linked worktree by
> providing the path of the broken worktree as an argument to "git
> worktree repair".)
Would git "work" in a corrupt worktree whose gitfile is broken, in
the sense that it notices that the cwd is the top of the working
tree of a secondary worktree? I can imagine how it would work,
starting in one of the functioning worktrees so that git can locate
where the primary copy is, with end-user supplied path to a
directory that is supposed to be the top of the working tree of a
secondary worktree.
Hmph, if the secondary is _moved_, how would "worktree repair $path"
would know which <id> the $path corresponds to? Would we just cull
all the <id> that do not point at working secondary worktrees and
add the $path as if it were a new one by allocating a new <id>, or
reusing a randomly chosen <id> that points at a non-existing
location?
^ permalink raw reply
* Re: Cleaning up files reported by size-garbage
From: Jeff King @ 2020-08-27 17:19 UTC (permalink / raw)
To: Konstantin Ryabitsev; +Cc: git
In-Reply-To: <20200827155529.3vtmrec7nn3mqgpl@chatter.i7.local>
On Thu, Aug 27, 2020 at 11:55:29AM -0400, Konstantin Ryabitsev wrote:
> Running git count-objects -v reports garbage files:
>
> $ git count-objects -v
> warning: garbage found: ./objects/pack/tmp_pack_XSv8MO
> warning: garbage found: ./objects/pack/tmp_pack_2uOuMg
> warning: garbage found: ./objects/pack/tmp_pack_KzP1ja
> count: 19
> size: 84
> in-pack: 172456
> packs: 6
> size-pack: 63907
> prune-packable: 0
> garbage: 3
> size-garbage: 1911
>
> Is there a way to tell git to clean those up? I'm not finding anything
> and would rather avoid having to parse stderr in these cases.
I think that git-gc will clean them up (via git-prune). It will also
check that their mtimes are older than the expiration time, which avoids
accidentally cleaning up the pack for an incoming fetch or push.
The default gc expiration time is 2 weeks, though, so you might want
something like:
git gc --prune=5.minutes.ago
if you're trying to get rid of them immediately. Likewise use git-prune
directly if you don't want to incur the cost of a full gc/repack.
-Peff
^ permalink raw reply
* Re: post-checkout hook aborts rebase
From: Junio C Hamano @ 2020-08-27 17:27 UTC (permalink / raw)
To: Elijah Newren; +Cc: Tom Rutherford, Git Mailing List
In-Reply-To: <CABPp-BF=ydfHE2XqN4L9qfeAg3AZL9yNJhs4rykGj0baT1Eh6g@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> On Wed, Aug 26, 2020 at 5:24 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > ... If "git rebase" or whatever
>> > command wanted to place files and the index into some state by using
>> > "git checkout" command, and if the post-checkout hook mucked with
>> > the state in such a way that contradicts with what the "git rebase"
>> > command wanted them to be in, it is not surprising the hook's behavior
>> > broke "git rebase"'s operation.
>>
>> Having said all that, I actually think that "rebase" shouldn't be
>> invoking "git checkout" (and its equivalent) internally when
>> switching to a specific version, in such a way that it would trigger
>> any end-user specified hooks and allow them to muck with the working
>> tree and the index state.
>>
>> I haven't checked the actual implementation of "git rebase" for
>> quite some time to be sure, but we have lower-level plumbing
>> commands that are not affected by the end-user hooks for exactly
>> that kind of "build higher-level commands by synthesis of
>> lower-level machinery", and it is very possible that what we are
>> looking at is actually a bug that needs to be fixed. I dunno.
>>
>> Thanks.
>
> Yes, and I think we should also make rebase stop invoking "git commit" too.
Note that I didn't say we should make it stop invoking "git
checkout".
We could invent a mechanism that disables all the hook invocations
and other customizations [*1*] (done e.g. via the configuration
variables) for internal use of the Porcelain commands, and use it
when "rebase" invokes Porcelains like "checkout", "commit" as its
implementation detail, for example (some "invocations" I think
bypass the run_command() inteface and instead done by directly
calling the implementation detail of "checkout" and "commit", but
the principles are the same).
[Footnote]
*1* of course, it becomes a balancing act to decide what kind of
customizations are OK to honor under such a mode.
^ permalink raw reply
* Re: Cleaning up files reported by size-garbage
From: Konstantin Ryabitsev @ 2020-08-27 17:36 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20200827171954.GA3970312@coredump.intra.peff.net>
On Thu, Aug 27, 2020 at 01:19:54PM -0400, Jeff King wrote:
> I think that git-gc will clean them up (via git-prune). It will also
> check that their mtimes are older than the expiration time, which avoids
> accidentally cleaning up the pack for an incoming fetch or push.
>
> The default gc expiration time is 2 weeks, though, so you might want
> something like:
>
> git gc --prune=5.minutes.ago
>
> if you're trying to get rid of them immediately. Likewise use git-prune
> directly if you don't want to incur the cost of a full gc/repack.
Oh, you're totally right. For some reason I was convinced that git-prune
didn't touch them, but I stand corrected.
Thanks for the pointer!
-K
^ permalink raw reply
* [GSoC][PATCH v2 0/3] submodule: fixup to summary-v3
From: Shourya Shukla @ 2020-08-27 17:44 UTC (permalink / raw)
To: git
Cc: kaartic.sivaraam, christian.couder, gitster, Johannes.Schindelin,
liu.denton, peff, Shourya Shukla
Greetings,
This is the v2 of the previous patch series with the same title. The v1
received some comments from Junio and Kaartic. The following changes
were made:
PATCH[3/3] received the comment that it had an unnecessary
'char *dst_abbrev = NULL' which had to be reverted to 'char
*dst_abbrev' since the assignment was pretty much useless.
The commit message also needed some changes in the sense that it
stated that the change of guarding the
'verify_submodule_committish()' call was made for dst_abbrev as well
which wasn't the case. Junio also suggested to clarify the reason
for not having the guard in case of 'dst_abbrev'.
Another thing which came up was the cleanup of 'submodule--helper.c'. IT
started with Junio commenting on PATCH[2/3] 'submodule: fix style in
function definition'. He asked me to verify if there are any other
similar faults regarding function or variable defintions which had a
faulty asterisk placement. I did some digging and found a fault here in
submodule--helper.c:
static char *compute_rev_name(const char *sub_path, const char* object_id)
As yiou may notice, the correction should be 's/static char */static
char* /. I also did some further digging and found that there we some
other minor faults in the option descriptions of various subcommands.
For instance in module_foreach:
struct option module_foreach_options[] = {
OPT__QUIET(&info.quiet, N_("Suppress output of entering each submodule command")),
OPT_BOOL(0, "recursive", &info.recursive,
N_("Recurse into nested submodules")),
OPT_END()
};
The option descriptions should start in lowercase but they start with a
capital letter. This convention is mentioned in L267-270 of
'api-parse-options.txt'. There are other such small violations such as
die() messages starting with a captial letter.
I will do this cleanup after some time when I am a bit free since I have
some personal engagements right now. Or something even better could be
to add this as a 'good first issue' on gitgitgadget/git so that a
newcomer can be something relatively easy and get familiar with the way
work is done at Git. Please do tell what seems more fitting to you.
Also, to be clear, I am not suggesting the latter out of laziness.
I am attaching a range-diff b/w v1 and v2 below for ease of review.
Feedback and reviews are appreciated.
Regards,
Shourya Shukla
-----
range-diff:
1: a22ffa950f = 1: 768f24de95 submodule: eliminate unused parameters from print_submodule_summary()
2: 32934998ee = 2: 35664360ac submodule: fix style in function definition
3: 82e0956cd2 ! 3: f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
@@ Commit message
fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
- Tighten up the check to compute '{src,dst}_abbrev' by guarding the
+ Tighten up the check to compute 'src_abbrev' by guarding the
'verify_submodule_committish()' call using `p->status !='D'`, so that
the former isn't called in case of non-existent submodule directory,
consequently, there is no such error message on any execution
- environment.
+ environment. The same need not be implemented for 'dst_abbrev' and is
+ rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`
+ already guards the `verify_submodule_committish` when we have a status
+ of 'D'.
Therefore, eliminate the 'grep' check in t7421. Instead, verify the
1: a22ffa950f = 1: 768f24de95 submodule: eliminate unused parameters from print_submo
dule_summary()
2: 32934998ee = 2: 35664360ac submodule: fix style in function definition
3: 82e0956cd2 ! 3: f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw comp
atibility
@@ Commit message
fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
- Tighten up the check to compute '{src,dst}_abbrev' by guarding the
:...skipping...
1: a22ffa950f = 1: 768f24de95 submodule: eliminate unused parameters from print_submodule_summary()
2: 32934998ee = 2: 35664360ac submodule: fix style in function definition
3: 82e0956cd2 ! 3: f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
@@ Commit message
fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
- Tighten up the check to compute '{src,dst}_abbrev' by guarding the
+ Tighten up the check to compute 'src_abbrev' by guarding the
1: a22ffa950f = 1: 768f24de95 submodule: eliminate unused parameters from print_submodule_summary()
2: 32934998ee = 2: 35664360ac submodule: fix style in function definition
3: 82e0956cd2 ! 3: f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
@@ Commit message
fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
- Tighten up the check to compute '{src,dst}_abbrev' by guarding the
+ Tighten up the check to compute 'src_abbrev' by guarding the
'verify_submodule_committish()' call using `p->status !='D'`, so that
the former isn't called in case of non-existent submodule directory,
consequently, there is no such error message on any execution
- environment.
+ environment. The same need not be implemented for 'dst_abbrev' and is
+ rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`
+ already guards the `verify_submodule_committish` when we have a status
+ of 'D'.
Therefore, eliminate the 'grep' check in t7421. Instead, verify the
1: a22ffa950f = 1: 768f24de95 submodule: eliminate unused parameters from print_submo
dule_summary()
2: 32934998ee = 2: 35664360ac submodule: fix style in function definition
3: 82e0956cd2 ! 3: f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw comp
atibility
@@ Commit message
fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
- Tighten up the check to compute '{src,dst}_abbrev' by guarding the
:...skipping...
1: a22ffa950f = 1: 768f24de95 submodule: eliminate unused parameters from print_submodule_summary()
2: 32934998ee = 2: 35664360ac submodule: fix style in function definition
3: 82e0956cd2 ! 3: f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
@@ Commit message
fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
- Tighten up the check to compute '{src,dst}_abbrev' by guarding the
+ Tighten up the check to compute 'src_abbrev' by guarding the
'verify_submodule_committish()' call using `p->status !='D'`, so that
the former isn't called in case of non-existent submodule directory,
consequently, there is no such error message on any execution
- environment.
+ environment. The same need not be implemented for 'dst_abbrev' and is
+ rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`
+ already guards the `verify_submodule_committish` when we have a status
+ of 'D'.
Therefore, eliminate the 'grep' check in t7421. Instead, verify the
absence of an error message by doing a 'test_must_be_empty' on the
@@ builtin/submodule--helper.c: static void print_submodule_summary(struct summary_
struct module_cb *p)
{
- char *displaypath, *src_abbrev, *dst_abbrev;
-+ char *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL;
++ char *displaypath, *src_abbrev = NULL, *dst_abbrev;
int missing_src = 0, missing_dst = 0;
char *errmsg = NULL;
int total_commits = -1;
~
~
~
~
~
~
~
~
~ 'verify_submodule_committish()' call using `p->status !='D'`, so that
the former isn't called in case of non-existent submodule directory,
consequently, there is no such error message on any execution
- environment.
+ environment. The same need not be implemented for 'dst_abbrev' and is
+ rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`
+ already guards the `verify_submodule_committish` when we have a status
+ of 'D'.
Therefore, eliminate the 'grep' check in t7421. Instead, verify the
absence of an error message by doing a 'test_must_be_empty' on the
@@ builtin/submodule--helper.c: static void print_submodule_summary(struct summary_
struct module_cb *p)
{
- char *displaypath, *src_abbrev, *dst_abbrev;
-+ char *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL;
++ char *displaypath, *src_abbrev = NULL, *dst_abbrev;
int missing_src = 0, missing_dst = 0;
char *errmsg = NULL;
int total_commits = -1;
~
~
~
~
~
~
~
~
~
-----
Shourya Shukla (3):
submodule: eliminate unused parameters from print_submodule_summary()
submodule: fix style in function definition
t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
builtin/submodule--helper.c | 17 ++++++++---------
t/t7421-submodule-summary-add.sh | 2 +-
2 files changed, 9 insertions(+), 10 deletions(-)
--
2.28.0
^ permalink raw reply
* [PATCH v2 1/3] submodule: eliminate unused parameters from print_submodule_summary()
From: Shourya Shukla @ 2020-08-27 17:44 UTC (permalink / raw)
To: git
Cc: kaartic.sivaraam, christian.couder, gitster, Johannes.Schindelin,
liu.denton, peff, Shourya Shukla, Christian Couder
In-Reply-To: <20200827174501.7103-1-shouryashukla.oo@gmail.com>
Eliminate the parameters 'missing_{src,dst}' from the
'print_submodule_summary()' function call since they are not used
anywhere in the function.
Reported-by: Jeff King <peff@peff.net>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
builtin/submodule--helper.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 63ea39025d..b83f840251 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -982,7 +982,6 @@ static char* verify_submodule_committish(const char *sm_path,
static void print_submodule_summary(struct summary_cb *info, char* errmsg,
int total_commits, const char *displaypath,
const char *src_abbrev, const char *dst_abbrev,
- int missing_src, int missing_dst,
struct module_cb *p)
{
if (p->status == 'T') {
@@ -1154,8 +1153,7 @@ static void generate_submodule_summary(struct summary_cb *info,
print_submodule_summary(info, errmsg, total_commits,
displaypath, src_abbrev,
- dst_abbrev, missing_src,
- missing_dst, p);
+ dst_abbrev, p);
free(displaypath);
free(src_abbrev);
--
2.28.0
^ permalink raw reply related
* [PATCH v2 2/3] submodule: fix style in function definition
From: Shourya Shukla @ 2020-08-27 17:45 UTC (permalink / raw)
To: git
Cc: kaartic.sivaraam, christian.couder, gitster, Johannes.Schindelin,
liu.denton, peff, Shourya Shukla, Christian Couder
In-Reply-To: <20200827174501.7103-1-shouryashukla.oo@gmail.com>
The definitions of 'verify_submodule_committish()' and
'print_submodule_summary()' had wrong styling in terms of the asterisk
placement. Amend them.
Also, the warning printed in case of an unexpected file mode printed the
mode in decimal. Print it in octal for enhanced readability.
Reported-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
builtin/submodule--helper.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b83f840251..93d0700891 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -959,7 +959,7 @@ enum diff_cmd {
DIFF_FILES
};
-static char* verify_submodule_committish(const char *sm_path,
+static char *verify_submodule_committish(const char *sm_path,
const char *committish)
{
struct child_process cp_rev_parse = CHILD_PROCESS_INIT;
@@ -979,7 +979,7 @@ static char* verify_submodule_committish(const char *sm_path,
return strbuf_detach(&result, NULL);
}
-static void print_submodule_summary(struct summary_cb *info, char* errmsg,
+static void print_submodule_summary(struct summary_cb *info, char *errmsg,
int total_commits, const char *displaypath,
const char *src_abbrev, const char *dst_abbrev,
struct module_cb *p)
@@ -1056,7 +1056,7 @@ static void generate_submodule_summary(struct summary_cb *info,
} else {
/* for a submodule removal (mode:0000000), don't warn */
if (p->mod_dst)
- warning(_("unexpected mode %d\n"), p->mod_dst);
+ warning(_("unexpected mode %o\n"), p->mod_dst);
}
}
--
2.28.0
^ permalink raw reply related
* [PATCH v2 3/3] t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
From: Shourya Shukla @ 2020-08-27 17:45 UTC (permalink / raw)
To: git
Cc: kaartic.sivaraam, christian.couder, gitster, Johannes.Schindelin,
liu.denton, peff, Shourya Shukla, Christian Couder
In-Reply-To: <20200827174501.7103-1-shouryashukla.oo@gmail.com>
The 'grep' check in test 4 of t7421 resulted in the failure of t7421 on
Windows due to a different error message
error: cannot spawn git: No such file or directory
instead of
fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
Tighten up the check to compute 'src_abbrev' by guarding the
'verify_submodule_committish()' call using `p->status !='D'`, so that
the former isn't called in case of non-existent submodule directory,
consequently, there is no such error message on any execution
environment. The same need not be implemented for 'dst_abbrev' and is
rather redundant since the conditional 'if (S_ISGITLINK(p->mod_dst))'
already guards the 'verify_submodule_committish()' when we have a
status of 'D'.
Therefore, eliminate the 'grep' check in t7421. Instead, verify the
absence of an error message by doing a 'test_must_be_empty' on the
file containing the error.
Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Helped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
builtin/submodule--helper.c | 7 ++++---
t/t7421-submodule-summary-add.sh | 2 +-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 93d0700891..1db1176e48 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1035,7 +1035,7 @@ static void print_submodule_summary(struct summary_cb *info, char *errmsg,
static void generate_submodule_summary(struct summary_cb *info,
struct module_cb *p)
{
- char *displaypath, *src_abbrev, *dst_abbrev;
+ char *displaypath, *src_abbrev = NULL, *dst_abbrev;
int missing_src = 0, missing_dst = 0;
char *errmsg = NULL;
int total_commits = -1;
@@ -1061,8 +1061,9 @@ static void generate_submodule_summary(struct summary_cb *info,
}
if (S_ISGITLINK(p->mod_src)) {
- src_abbrev = verify_submodule_committish(p->sm_path,
- oid_to_hex(&p->oid_src));
+ if (p->status != 'D')
+ src_abbrev = verify_submodule_committish(p->sm_path,
+ oid_to_hex(&p->oid_src));
if (!src_abbrev) {
missing_src = 1;
/*
diff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh
index 59a9b00467..b070f13714 100755
--- a/t/t7421-submodule-summary-add.sh
+++ b/t/t7421-submodule-summary-add.sh
@@ -58,7 +58,7 @@ test_expect_success 'submodule summary output for submodules with changed paths'
git commit -m "change submodule path" &&
rev=$(git -C sm rev-parse --short HEAD^) &&
git submodule summary HEAD^^ -- my-subm >actual 2>err &&
- grep "fatal:.*my-subm" err &&
+ test_must_be_empty err &&
cat >expected <<-EOF &&
* my-subm ${rev}...0000000:
--
2.28.0
^ permalink raw reply related
* Re: post-checkout hook aborts rebase
From: Elijah Newren @ 2020-08-27 17:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tom Rutherford, Git Mailing List
In-Reply-To: <xmqqd03cq9r5.fsf@gitster.c.googlers.com>
On Thu, Aug 27, 2020 at 10:28 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > On Wed, Aug 26, 2020 at 5:24 PM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> Junio C Hamano <gitster@pobox.com> writes:
> >>
> >> > ... If "git rebase" or whatever
> >> > command wanted to place files and the index into some state by using
> >> > "git checkout" command, and if the post-checkout hook mucked with
> >> > the state in such a way that contradicts with what the "git rebase"
> >> > command wanted them to be in, it is not surprising the hook's behavior
> >> > broke "git rebase"'s operation.
> >>
> >> Having said all that, I actually think that "rebase" shouldn't be
> >> invoking "git checkout" (and its equivalent) internally when
> >> switching to a specific version, in such a way that it would trigger
> >> any end-user specified hooks and allow them to muck with the working
> >> tree and the index state.
> >>
> >> I haven't checked the actual implementation of "git rebase" for
> >> quite some time to be sure, but we have lower-level plumbing
> >> commands that are not affected by the end-user hooks for exactly
> >> that kind of "build higher-level commands by synthesis of
> >> lower-level machinery", and it is very possible that what we are
> >> looking at is actually a bug that needs to be fixed. I dunno.
> >>
> >> Thanks.
> >
> > Yes, and I think we should also make rebase stop invoking "git commit" too.
>
> Note that I didn't say we should make it stop invoking "git
> checkout".
Understood that you didn't say that, but I am of the opinion that we
should do that.
Invoking "git checkout" and "git commit" were convenient
implementation details when rebase was written as a script. When it
was rewritten in C, forking out to these processes made for an easy
conversion path (even if slightly ugly). But forking other processes
is costly, it has given us multiple reports of unwanted side-effects
from hooks[1], it makes the code more difficult to debug in a
debugger, etc. I think these are all problems we could avoid by no
longer calling these external commands.
> We could invent a mechanism that disables all the hook invocations
> and other customizations [*1*] (done e.g. via the configuration
> variables) for internal use of the Porcelain commands, and use it
> when "rebase" invokes Porcelains like "checkout", "commit" as its
> implementation detail, for example (some "invocations" I think
> bypass the run_command() inteface and instead done by directly
> calling the implementation detail of "checkout" and "commit", but
> the principles are the same).
>
> [Footnote]
>
> *1* of course, it becomes a balancing act to decide what kind of
> customizations are OK to honor under such a mode.
That'd be one way to solve it, but it feels like it'd push maintenance
burden onto future folks that people touching the commit and checkout
builtins have to be aware of what other commands are using them under
the covers and tweak them appropriately. I'd rather checkout and
commit just shared the relevant bits of important code with sequencer,
and then if people want to add configuration bits (be that hooks or
config settings or whatever), then it only gets added to the commands
that people explicitly add them to, rather than them getting added to
rebase via accident.
^ permalink raw reply
* [PATCH] send-email: do not prompt for In-Reply-To
From: Drew DeVault @ 2020-08-27 17:55 UTC (permalink / raw)
To: git, Junio C Hamano; +Cc: Drew DeVault
Most mailing lists prefer that new patchsets and their revisions are
placed into a new thread. Additionally, knowledge of what In-Reply-To
means and where to find the Message-Id to fill in are domain-specific
and confusing to new users. In the niche situations where this is called
for, the --in-reply-to flag is sufficient.
A config option, sendemail.promptInReplyTo, has been added to re-enable
the old behavior.
---
Documentation/config/sendemail.txt | 7 +++++++
git-send-email.perl | 5 ++++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt
index cbc5af42fd..34ca8263d0 100644
--- a/Documentation/config/sendemail.txt
+++ b/Documentation/config/sendemail.txt
@@ -66,3 +66,10 @@ sendemail.forbidSendmailVariables::
To avoid common misconfiguration mistakes, linkgit:git-send-email[1]
will abort with a warning if any configuration options for "sendmail"
exist. Set this variable to bypass the check.
+
+sendemail.promptInReplyTo::
+ Previous versions of linkgit:git-send-email[1] would prompt the user to
+ input an In-Reply-To header for every patchset sent. This was removed as
+ the default behavior, being generally undesirable on most mailing lists,
+ and confusing for new users. Set this variable to re-enable the old
+ behavior.
diff --git a/git-send-email.perl b/git-send-email.perl
index 1f425c0809..f3abccef6d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -251,6 +251,7 @@ sub do_edit {
my $validate = 1;
my $target_xfer_encoding = 'auto';
my $forbid_sendmail_variables = 1;
+my $prompt_in_reply_to = 0;
my %config_bool_settings = (
"thread" => \$thread,
@@ -265,6 +266,7 @@ sub do_edit {
"annotate" => \$annotate,
"xmailer" => \$use_xmailer,
"forbidsendmailvariables" => \$forbid_sendmail_variables,
+ "promptinreplyto" => \$prompt_in_reply_to,
);
my %config_settings = (
@@ -467,6 +469,7 @@ sub read_config {
"batch-size=i" => \$batch_size,
"relogin-delay=i" => \$relogin_delay,
"git-completion-helper" => \$git_completion_helper,
+ "prompt-in-reply-to" => \$prompt_in_reply_to,
);
# Munge any "either config or getopt, not both" variables
@@ -978,7 +981,7 @@ sub expand_one_alias {
@initial_cc = process_address_list(@initial_cc);
@initial_bcc = process_address_list(@initial_bcc);
-if ($thread && !defined $initial_in_reply_to && $prompting) {
+if ($thread && !defined $initial_in_reply_to && $prompting && $prompt_in_reply_to) {
$initial_in_reply_to = ask(
__("Message-ID to be used as In-Reply-To for the first email (if any)? "),
default => "",
--
2.28.0
^ permalink raw reply related
* Re: [PATCH] send-email: do not prompt for In-Reply-To
From: Junio C Hamano @ 2020-08-27 19:04 UTC (permalink / raw)
To: Drew DeVault; +Cc: git
In-Reply-To: <20200827175552.132193-1-sir@cmpwn.com>
Drew DeVault <sir@cmpwn.com> writes:
> Most mailing lists prefer that new patchsets and their revisions are
> placed into a new thread. Additionally, knowledge of what In-Reply-To
> means and where to find the Message-Id to fill in are domain-specific
> and confusing to new users. In the niche situations where this is called
> for, the --in-reply-to flag is sufficient.
>
> A config option, sendemail.promptInReplyTo, has been added to re-enable
> the old behavior.
We do not break existing users' habits without a good reason, and a
subjective "this is the way I prefer" is *not* a good reason.
If/when the claim "most mailing lists prefer" can be substantiated,
we'd need to devise a transition plan to flip the default over
several releases. Here is how I would envision the plan should go.
(0) Introduce sendemail.promptInReplyTo that defaults to true; this
can be done today and it would be a genuine improvement for
those who want the new behaviour, without hurting any existing
users.
(1) Substantiate the "most mailing list prefer" claim. If we
cannot, we stop here. Otherwise we would move to the next
step.
(2) Teach "git send-email" to issue a warning message when the
telling the user that the default will be flipped in some
future version of Git and optionally ask them to tell us to
stop on the mailing list, when sendemail.promptInReplyTo
configuration variable is not set.
Advertise the future flip of the default in other channels,
too.
(3) Wait for at least a few releases. Monitor the mailing list and
other channels for objections, and if it becomes clear that we
misjudged in step (1), stop the transition plan by reverting to
the state before step (2) (i.e. not to before step (0)).
(4) Flip the default and tweak the message to tell those users who
still do not have sendemail.promptInReplyTo variable set that
the default have changed, and if they want to get prompted,
they must set the variable to true. Also stop asking them to
tell us to stop---at this point we are committed and will not
go back.
(5) Waiting for several releases
(6) Remove the code to give messages for users who do not have the
configuration variable.
^ permalink raw reply
* Re: post-checkout hook aborts rebase
From: Chris Torek @ 2020-08-27 19:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tom Rutherford, Git List
In-Reply-To: <xmqqzh6gqe7i.fsf@gitster.c.googlers.com>
On Thu, Aug 27, 2020 at 8:51 AM Junio C Hamano <gitster@pobox.com> wrote:
> I still suspect that the checkout run, merely as an implementation
> detail of rebase (or any other git subcommand), should not trigger
> any hook ...
The *last* checkout from the finished rebase perhaps *should*, but
other than that one, that seems logically correct.
> but before any such code change, at least let's update the
> documentation to clarify what we mean by "the outcome".
>
> Hopefully something like the below may be a good starting point?
>
> Documentation/githooks.txt | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 31b601e4bc..cf95d6d02b 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -193,7 +193,9 @@ worktree. The hook is given three parameters: the ref of the previous HEAD,
> the ref of the new HEAD (which may or may not have changed), and a flag
> indicating whether the checkout was a branch checkout (changing branches,
> flag=1) or a file checkout (retrieving a file from the index, flag=0).
> -This hook cannot affect the outcome of `git switch` or `git checkout`.
> +This hook cannot affect the outcome of `git switch` or `git checkout`,
> +other than that the hook's exit status becomes the exit status of
> +these two commands.
>
> It is also run after linkgit:git-clone[1], unless the `--no-checkout` (`-n`) option is
> used. The first parameter given to the hook is the null-ref, the second the
This looks good to me, and can either be independent of, or the first part of,
any rebase update.
Chris
^ permalink raw reply
* Re: [PATCH] send-email: do not prompt for In-Reply-To
From: Junio C Hamano @ 2020-08-27 19:08 UTC (permalink / raw)
To: Drew DeVault; +Cc: git
In-Reply-To: <xmqq7dtjrjut.fsf@gitster.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
> Drew DeVault <sir@cmpwn.com> writes:
>
>> Most mailing lists prefer that new patchsets and their revisions are
>> placed into a new thread. Additionally, knowledge of what In-Reply-To
>> means and where to find the Message-Id to fill in are domain-specific
>> and confusing to new users. In the niche situations where this is called
>> for, the --in-reply-to flag is sufficient.
>>
>> A config option, sendemail.promptInReplyTo, has been added to re-enable
>> the old behavior.
>
> We do not break existing users' habits without a good reason, and a
> subjective "this is the way I prefer" is *not* a good reason.
Having said that (and I am not retracting anything I said in the
message I am responding to), I haven't seen this prompt triggering
for me when I use send-email, with or without --in-reply-to option
on the command line.
Admittedly I use a wrapper around "git send-email" to add minimum
set of command line options that are always used (they are --from,
--envelope-sender, and --smtp-server) but I do not think they have
effect on the use of in-reply-to prompt.
What are we doing differently, I wonder?
^ permalink raw reply
* Re: [PATCH] send-email: do not prompt for In-Reply-To
From: Drew DeVault @ 2020-08-27 19:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq3647rjnt.fsf@gitster.c.googlers.com>
Do you have sendemail.to set in your local git config?
^ permalink raw reply
* Re: [PATCH] send-email: do not prompt for In-Reply-To
From: Drew DeVault @ 2020-08-27 19:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq7dtjrjut.fsf@gitster.c.googlers.com>
Okay, thanks for the info! I'll take this back to the drawing board and
come up with a plan which follows more closely to that.
^ permalink raw reply
* Re: [PATCH] send-email: do not prompt for In-Reply-To
From: Junio C Hamano @ 2020-08-27 19:21 UTC (permalink / raw)
To: Drew DeVault; +Cc: git
In-Reply-To: <C580P9BS4VYA.15I6SHXQQ35HF@homura>
"Drew DeVault" <sir@cmpwn.com> writes:
> Do you have sendemail.to set in your local git config?
Yes, and I'd assume most mailing list have a single to: address, and
people use a repository to interact with just a single project, so I
think it would be natural to have it in the per-repository
configuration file. Is it true that you do not set it and that the
lack of the configuration is why you are getting prompted?
I suspect that it may not make much sense for the presense of
sendemail.to to affect prompting for other header fields (iow, my
not getting prompted might be a bug).
Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox