* [PATCH] revision: set rev_input_given in handle_revision_arg()
From: Jeff King @ 2020-08-26 20:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Bryan Turner, Git Users
In-Reply-To: <20200825195511.GD1419759@coredump.intra.peff.net>
On Tue, Aug 25, 2020 at 03:55:11PM -0400, Jeff King wrote:
> > You beat me to it while I was wondering what to do between the local
> > got_rev_arg variable and the revs->rev_input_given field.
>
> That makes me wonder why we need got_rev_arg at all if we have
> revs->rev_input_given. But I suspect an answer can be found by digging
> into git-blame. I probably won't do that immediately, so if you want to,
> you can do so without worrying that we're duplicating work. :)
This is all my fault, naturally. :) I added rev_input_given a while ago
but didn't go far enough. Here's what I think we should do:
-- >8 --
Subject: [PATCH] revision: set rev_input_given in handle_revision_arg()
Commit 7ba826290a (revision: add rev_input_given flag, 2017-08-02) added
a flag to rev_info to tell whether we got any revision arguments. As
explained there, this is necessary because some revision arguments may
not produce any pending traversal objects, but should still inhibit
default behaviors (e.g., a glob that matches nothing).
However, it only set the flag in the globbing code, but not for
revisions we get on the command-line or via stdin. This leads to two
problems:
- the command-line code keeps its own separate got_rev_arg flag; this
isn't wrong, but it's confusing and an extra maintenance burden
- even specifically-named rev arguments might end up not adding any
pending objects: if --ignore-missing is set, then specifying a
missing object is a noop rather than an error.
And that leads to some user-visible bugs:
- when deciding whether a default rev like "HEAD" should kick in, we
check both got_rev_arg and rev_input_given. That means that
"--ignore-missing $ZERO_OID" works on the command-line (where we set
got_rev_arg) but not on --stdin (where we don't)
- when rev-list decides whether it should complain that it wasn't
given a starting point, it relies on rev_input_given. So it can't
even get the command-line "--ignore-missing $ZERO_OID" right
Let's consistently set the flag if we got any revision argument. That
lets us clean up the redundant got_rev_arg, and fixes both of those bugs
(but note there are three new tests: we'll confirm the already working
git-log command-line case).
A few implementation notes:
- conceptually we want to set the flag whenever handle_revision_arg()
finds an actual revision arg ("handles" it, you might say). But it
covers a ton of cases with early returns. Rather than annotating
each one, we just wrap it and use its success exit-code to set the
flag in one spot.
- the new rev-list test is in t6018, which is titled to cover globs.
This isn't exactly a glob, but it made sense to stick it with the
other tests that handle the "even though we got a rev, we have no
pending objects" case, which are globs.
- the tests check for the oid of a missing object, which it's pretty
clear --ignore-missing should ignore. You can see the same behavior
with "--ignore-missing a-ref-that-does-not-exist", because
--ignore-missing treats them both the same. That's perhaps less
clearly correct, and we may want to change that in the future. But
the way the code and tests here are written, we'd continue to do the
right thing even if it does.
Reported-by: Bryan Turner <bturner@atlassian.com>
Signed-off-by: Jeff King <peff@peff.net>
---
revision.c | 16 +++++++++++-----
t/t4202-log.sh | 10 ++++++++++
t/t6018-rev-list-glob.sh | 5 +++++
3 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/revision.c b/revision.c
index 96630e3186..08c2ad23af 100644
--- a/revision.c
+++ b/revision.c
@@ -2017,7 +2017,7 @@ static int handle_dotdot(const char *arg,
return ret;
}
-int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)
+static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)
{
struct object_context oc;
char *mark;
@@ -2092,6 +2092,14 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
return 0;
}
+int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)
+{
+ int ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);
+ if (!ret)
+ revs->rev_input_given = 1;
+ return ret;
+}
+
static void read_pathspec_from_stdin(struct strbuf *sb,
struct strvec *prune)
{
@@ -2703,7 +2711,7 @@ static void NORETURN diagnose_missing_default(const char *def)
*/
int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *opt)
{
- int i, flags, left, seen_dashdash, got_rev_arg = 0, revarg_opt;
+ int i, flags, left, seen_dashdash, revarg_opt;
struct strvec prune_data = STRVEC_INIT;
const char *submodule = NULL;
int seen_end_of_options = 0;
@@ -2792,8 +2800,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
strvec_pushv(&prune_data, argv + i);
break;
}
- else
- got_rev_arg = 1;
}
if (prune_data.nr) {
@@ -2822,7 +2828,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
opt->tweak(revs, opt);
if (revs->show_merge)
prepare_show_merge(revs);
- if (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {
+ if (revs->def && !revs->pending.nr && !revs->rev_input_given) {
struct object_id oid;
struct object *object;
struct object_context oc;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index a0930599aa..56d34ed465 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1850,6 +1850,16 @@ test_expect_success 'log does not default to HEAD when rev input is given' '
test_must_be_empty actual
'
+test_expect_success 'do not default to HEAD with ignored object on cmdline' '
+ git log --ignore-missing $ZERO_OID >actual &&
+ test_must_be_empty actual
+'
+
+test_expect_success 'do not default to HEAD with ignored object on stdin' '
+ echo $ZERO_OID | git log --ignore-missing --stdin >actual &&
+ test_must_be_empty actual
+'
+
test_expect_success 'set up --source tests' '
git checkout --orphan source-a &&
test_commit one &&
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index bb5aeac07f..b31ff7eeec 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -345,6 +345,11 @@ test_expect_success 'rev-list should succeed with empty output with empty glob'
test_must_be_empty actual
'
+test_expect_success 'rev-list should succeed with empty output when ignoring missing' '
+ git rev-list --ignore-missing $ZERO_OID >actual &&
+ test_must_be_empty actual
+'
+
test_expect_success 'shortlog accepts --glob/--tags/--remotes' '
compare shortlog "subspace/one subspace/two" --branches=subspace &&
--
2.28.0.749.gf1242ce4bd
^ permalink raw reply related
* Re: [PATCH] clone: add remote.cloneDefault config option
From: Junio C Hamano @ 2020-08-26 19:59 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Sean Barag via GitGitGadget, git, Sean Barag
In-Reply-To: <eeebff99-d585-5575-009e-83bfef5294e3@gmail.com>
Derrick Stolee <stolee@gmail.com> writes:
> On 8/26/2020 2:46 PM, Junio C Hamano wrote:
>> "Sean Barag via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>> This commit implements
>>> `remote.cloneDefault` as a parallel to `remote.pushDefault`,
>>> with prioritized name resolution:
>>
>> I highly doubt that .cloneDefault is a good name. After reading
>> only the title of the patch e-mail, i.e. when the only available
>> information on the change available to me was the name of the
>> configuration variable and the fact that it pertains to the command
>> "git clone", I thought it is to specify a URL, from which "git
>> clone" without the URL would clone from that single repository.
>>
>> And the name will cause the same misunderstanding to normal users,
>> not just to reviewers of your patch, after this change hits a future
>> Git release.
>>
>> Taking a parallel from init.defaultBranchName, I would probably call
>> it clone.defaultUpstreamName if I were writing this feature.
>
> I was thinking "clone.defaultRemoteName" makes it clear we are naming
> the remote for the provided <url> in the command.
I 100% agree that defaultremotename is much better.
>> ... For example
>>
>> git -c remote.cloneDefault="bad.../...name" clone parent
>>
>> should fail, no?
>
> This is an important suggestion.
To be fair, the current code does not handle the "--origin" command
line option not so carefully.
Back when the command was scripted, e.g. 47874d6d (revamp git-clone
(take #2)., 2006-03-21), had both ref-format check and */*
multi-level check, and these checks been retained throughout its
life until 8434c2f1 (Build in clone, 2008-04-27) rewrote the whole
thing while discarding these checks for --origin=bad.../...name
It would make an excellent #leftoverbits or #microproject.
Thanks.
^ permalink raw reply
* [PATCH v2 2/2] cvsexportcommit: do not run git programs in dashed form
From: Junio C Hamano @ 2020-08-26 19:46 UTC (permalink / raw)
To: git
In-Reply-To: <20200826194650.4031087-1-gitster@pobox.com>
This ancient script runs "git-foo" all over the place, which is
OK for a scripted Porcelain in the Git suite, but asking "git" to
dispatch to subcommands is the usual way these days.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-cvsexportcommit.perl | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index 0ae8bce3fb..289d4bc684 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -30,7 +30,7 @@
# Remember where GIT_DIR is before changing to CVS checkout
unless ($ENV{GIT_DIR}) {
# No GIT_DIR set. Figure it out for ourselves
- my $gd =`git-rev-parse --git-dir`;
+ my $gd =`git rev-parse --git-dir`;
chomp($gd);
$ENV{GIT_DIR} = $gd;
}
@@ -66,7 +66,7 @@
# resolve target commit
my $commit;
$commit = pop @ARGV;
-$commit = safe_pipe_capture('git-rev-parse', '--verify', "$commit^0");
+$commit = safe_pipe_capture('git', 'rev-parse', '--verify', "$commit^0");
chomp $commit;
if ($?) {
die "The commit reference $commit did not resolve!";
@@ -76,7 +76,7 @@
my $parent;
if (@ARGV) {
$parent = pop @ARGV;
- $parent = safe_pipe_capture('git-rev-parse', '--verify', "$parent^0");
+ $parent = safe_pipe_capture('git', 'rev-parse', '--verify', "$parent^0");
chomp $parent;
if ($?) {
die "The parent reference did not resolve!";
@@ -84,7 +84,7 @@
}
# find parents from the commit itself
-my @commit = safe_pipe_capture('git-cat-file', 'commit', $commit);
+my @commit = safe_pipe_capture('git', 'cat-file', 'commit', $commit);
my @parents;
my $committer;
my $author;
@@ -162,9 +162,9 @@
close MSG;
if ($parent eq $noparent) {
- `git-diff-tree --binary -p --root $commit >.cvsexportcommit.diff`;# || die "Cannot diff";
+ `git diff-tree --binary -p --root $commit >.cvsexportcommit.diff`;# || die "Cannot diff";
} else {
- `git-diff-tree --binary -p $parent $commit >.cvsexportcommit.diff`;# || die "Cannot diff";
+ `git diff-tree --binary -p $parent $commit >.cvsexportcommit.diff`;# || die "Cannot diff";
}
## apply non-binary changes
@@ -178,7 +178,7 @@
print "Checking if patch will apply\n";
my @stat;
-open APPLY, "GIT_INDEX_FILE=$tmpdir/index git-apply $context --summary --numstat<.cvsexportcommit.diff|" || die "cannot patch";
+open APPLY, "GIT_INDEX_FILE=$tmpdir/index git apply $context --summary --numstat<.cvsexportcommit.diff|" || die "cannot patch";
@stat=<APPLY>;
close APPLY || die "Cannot patch";
my (@bfiles,@files,@afiles,@dfiles);
@@ -333,7 +333,7 @@
if ($opt_W) {
system("git checkout -q $commit^0") && die "cannot patch";
} else {
- `GIT_INDEX_FILE=$tmpdir/index git-apply $context --summary --numstat --apply <.cvsexportcommit.diff` || die "cannot patch";
+ `GIT_INDEX_FILE=$tmpdir/index git apply $context --summary --numstat --apply <.cvsexportcommit.diff` || die "cannot patch";
}
print "Patch applied successfully. Adding new files and directories to CVS\n";
--
2.28.0-454-g5f859b1948
^ permalink raw reply related
* [PATCH v2 1/2] transport-helper: do not run git-remote-ext etc. in dashed form
From: Junio C Hamano @ 2020-08-26 19:46 UTC (permalink / raw)
To: git
In-Reply-To: <20200826194650.4031087-1-gitster@pobox.com>
Running it as "git remote-ext" and letting "git" dispatch to
"remote-ext" would just be fine and is more idiomatic.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
transport-helper.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/transport-helper.c b/transport-helper.c
index defafbf4c1..c52c99d829 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -128,10 +128,10 @@ static struct child_process *get_helper(struct transport *transport)
helper->in = -1;
helper->out = -1;
helper->err = 0;
- strvec_pushf(&helper->args, "git-remote-%s", data->name);
+ strvec_pushf(&helper->args, "remote-%s", data->name);
strvec_push(&helper->args, transport->remote->name);
strvec_push(&helper->args, remove_ext_force(transport->url));
- helper->git_cmd = 0;
+ helper->git_cmd = 1;
helper->silent_exec_failure = 1;
if (have_git_dir())
--
2.28.0-454-g5f859b1948
^ permalink raw reply related
* [PATCH v2 0/2] avoid running "git-subcmd" in the dashed form
From: Junio C Hamano @ 2020-08-26 19:46 UTC (permalink / raw)
To: git
In-Reply-To: <xmqqd03dwe2x.fsf@gitster.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
> I think the cvsexportcommit and transport-helper changes are worth
> salvaging even if we don't remove builtin binaries, so I'll split
> them and whip them a bit more into a reasonable shape to be merged
> to 'next'. The "break those who say 'git-cat-file'" can be left for
> future.
And here it is.
These two patches are clean-ups that are worth doing whether we
decide to remove on-disk binaries for the builtin commands.
I droped the third one, that actually stops users from running
built-in commands using the dashed form, at least for now. It can
be resurrected later if we really wants to propose removal to the
users, but I am not inclined to make such a proposal right now.
Junio C Hamano (2):
transport-helper: do not run git-remote-ext etc. in dashed form
cvsexportcommit: do not run git programs in dashed form
git-cvsexportcommit.perl | 16 ++++++++--------
transport-helper.c | 4 ++--
2 files changed, 10 insertions(+), 10 deletions(-)
--
2.28.0-454-g5f859b1948
^ permalink raw reply
* Re: [PATCH] clone: add remote.cloneDefault config option
From: Derrick Stolee @ 2020-08-26 19:04 UTC (permalink / raw)
To: Junio C Hamano, Sean Barag via GitGitGadget; +Cc: git, Sean Barag
In-Reply-To: <xmqqlfi1utwi.fsf@gitster.c.googlers.com>
On 8/26/2020 2:46 PM, Junio C Hamano wrote:
> "Sean Barag via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> This commit implements
>> `remote.cloneDefault` as a parallel to `remote.pushDefault`,
>> with prioritized name resolution:
>
> I highly doubt that .cloneDefault is a good name. After reading
> only the title of the patch e-mail, i.e. when the only available
> information on the change available to me was the name of the
> configuration variable and the fact that it pertains to the command
> "git clone", I thought it is to specify a URL, from which "git
> clone" without the URL would clone from that single repository.
>
> And the name will cause the same misunderstanding to normal users,
> not just to reviewers of your patch, after this change hits a future
> Git release.
>
> Taking a parallel from init.defaultBranchName, I would probably call
> it clone.defaultUpstreamName if I were writing this feature.
I was thinking "clone.defaultRemoteName" makes it clear we are naming
the remote for the provided <url> in the command. Having
clone.defaultUpstreamName = upstream
may look a bit confusing, while
clone.defaultRemoteName = upstream
makes it a bit clearer that we will set
remote.upstream.url = <url>
>> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
>> index e69427f881..8aac67b385 100755
>> --- a/t/t5606-clone-options.sh
>> +++ b/t/t5606-clone-options.sh
>> @@ -19,6 +19,20 @@ test_expect_success 'clone -o' '
>>
>> '
>>
>> +test_expect_success 'clone respects remote.cloneDefault' '
>> +
>> + git -c remote.cloneDefault=bar clone parent clone-config &&
>> + (cd clone-config && git rev-parse --verify refs/remotes/bar/master)
>> +
>> +'
>> +
>> +test_expect_success 'clone chooses correct remote name' '
>> +
>> + git -c remote.cloneDefault=bar clone -o foo parent clone-o-and-config &&
>> + (cd clone-o-and-config && git rev-parse --verify refs/remotes/foo/master)
>> +
>> +'
>
> These two are "showing off my shiny new toy" tests, which are
> needed, but we also need negative tests where the shiny new toy does
> not kick in when it should not. For example
>
> git -c remote.cloneDefault="bad.../...name" clone parent
>
> should fail, no?
This is an important suggestion.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH] clone: add remote.cloneDefault config option
From: Junio C Hamano @ 2020-08-26 18:46 UTC (permalink / raw)
To: Sean Barag via GitGitGadget; +Cc: git, Sean Barag
In-Reply-To: <pull.710.git.1598456751674.gitgitgadget@gmail.com>
"Sean Barag via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Sean Barag <sean@barag.org>
>
> While the default remote name of `origin` can be overridden both
> pre-clone (with `git clone --origin foo`) and post-clone (with `git
> remote rename origin foo`), it'd be handy to have a configurable
> system-wide default for clones!
I doubt it is handy enough to deserve an explamation point. Replace
it with a plain-vanilla full-stop instead.
I however tend to agree that, even evidenced by the manual page
description of "git clone", i.e.
-o <name>::
--origin <name>::
Instead of using the remote name `origin` to keep track
of the upstream repository, use `<name>`.
that it is understandable if many users and projects wish to call it
"upstream".
> This commit implements
> `remote.cloneDefault` as a parallel to `remote.pushDefault`,
> with prioritized name resolution:
I highly doubt that .cloneDefault is a good name. After reading
only the title of the patch e-mail, i.e. when the only available
information on the change available to me was the name of the
configuration variable and the fact that it pertains to the command
"git clone", I thought it is to specify a URL, from which "git
clone" without the URL would clone from that single repository.
And the name will cause the same misunderstanding to normal users,
not just to reviewers of your patch, after this change hits a future
Git release.
Taking a parallel from init.defaultBranchName, I would probably call
it clone.defaultUpstreamName if I were writing this feature.
> diff --git a/builtin/clone.c b/builtin/clone.c
> index b087ee40c2..b0dbb848c6 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -941,6 +941,29 @@ static int path_exists(const char *path)
> return !stat(path, &sb);
> }
>
> +struct clone_default_info
> +{
> + enum config_scope scope;
> + struct strbuf remote_name;
> + int linenr;
> +};
> +
> +static int config_read_clone_default(const char *key, const char *value,
> + void *cb)
> +{
> + struct clone_default_info* info = cb;
> + if (strcmp(key, "remote.clonedefault") || !value) {
> + return 0;
> + }
> +
> + info->scope = current_config_scope();
> + strbuf_reset(&info->remote_name);
> + strbuf_addstr(&info->remote_name, value);
> + info->linenr = current_config_line();
> +
> + return 0;
> +}
This feels way overkill and insufficient at the same time. It does
not need scope, it does not need linenr, and we already have a place
to store end-user specified name for the upstream in the form of the
variable option_origin. And the code is not diagnosing any error.
static int git_clone_config(const char *k, const char *v, void *cb)
{
if (option_origin)
return 0; /* ignore -- the user gave us an option */
if (!strcmp(k, "clone.defaultupstreamname")) {
if (!v)
return config_error_nonbool(k);
if (strchr(v, '/') || check_refname_format(v, REFNAME_ALLOW_ONELEVEL))
return error(_("invalid upstream name '%s'"), v);
option_origin = xstrdup(v);
return 0;
}
return 0;
}
would be sufficient, and at the same time makes sure it rejects
names like 'o..ri..gin', 'o/ri/gin', etc.
> @@ -992,8 +1015,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> option_no_checkout = 1;
> }
>
> - if (!option_origin)
> - option_origin = "origin";
> + if (!option_origin) {
> + struct clone_default_info clone_default = { CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 };
> + git_config(config_read_clone_default, &clone_default);
> + if (strcmp("", (const char*) clone_default.remote_name.buf))
> + option_origin = clone_default.remote_name.buf;
> + else
> + option_origin = "origin";
> + }
> +
It is somewhat sad that we have the git_config(git_default_config)
call so late in the control flow. I wonder if we can update the
start-up sequence to match the usual flow, e.g. these things happen
in the following order in a normal program.
- variables are given their default value.
- call git_config(git_commandspecific_config, ...) early in the
program.
- git_commandspecific_config() interprets the command specific
variables and passes everything else to git_default_config()
variables like option_origin are given values of configuration
variable at this point.
- call parse_options() next, which may override the variables
from the value on the command line.
- main control flow uses the variable. "Command-line wins over
configuration which wins over the default" falls out naturally.
One oddity "git clone" has is that it wants to delay the reading of
configuration files (they are read only once, and second and
subsequent git_config() calls will reuse what was read before [*])
so that it can read what clone.c::write_config() wrote, so if we were
to "fix" the start-up sequence to match the usual flow, we need to
satisfy what that odd arrangement wanted to achieve in some other
way (e.g. feed what is in option_config to git_default_config
ourselves, without using git_config(), as part of the "main control
flow uses the variable" part), but it should be doable.
[*Side note*]. The above means that this patch, even when
the configuration variable does not give upstream name, may
break the option_config feature by breaking the second call
to git_config(). We need to have a test for that.
> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index e69427f881..8aac67b385 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -19,6 +19,20 @@ test_expect_success 'clone -o' '
>
> '
>
> +test_expect_success 'clone respects remote.cloneDefault' '
> +
> + git -c remote.cloneDefault=bar clone parent clone-config &&
> + (cd clone-config && git rev-parse --verify refs/remotes/bar/master)
> +
> +'
> +
> +test_expect_success 'clone chooses correct remote name' '
> +
> + git -c remote.cloneDefault=bar clone -o foo parent clone-o-and-config &&
> + (cd clone-o-and-config && git rev-parse --verify refs/remotes/foo/master)
> +
> +'
These two are "showing off my shiny new toy" tests, which are
needed, but we also need negative tests where the shiny new toy does
not kick in when it should not. For example
git -c remote.cloneDefault="bad.../...name" clone parent
should fail, no?
Thanks.
^ permalink raw reply
* Re: [PATCH] pretty-options.txt: fix --no-abbrev-commit description
From: Junio C Hamano @ 2020-08-26 17:56 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <20200826144923.16806-1-sorganov@gmail.com>
Sergey Organov <sorganov@gmail.com> writes:
> --no-abbrev-commit::
> Show the full 40-byte hexadecimal commit object name. This negates
> - `--abbrev-commit` and those options which imply it such as
> - "--oneline". It also overrides the `log.abbrevCommit` variable.
> + `--abbrev-commit`, either explicit or implied by other options such
> + as "--oneline". It also overrides the `log.abbrevCommit` variable.
I personally do not think it would be crazy to misread that one-line
format itself would be defeated when no abbreviation of object names
is requested, so from that point of view, the original text would be
OK enough, but as long as the updated text is easier to understand,
that is fine ;-)
Keeping the original sentence structure, e.g.
... and those options which imply abbreviating commit object
names such as ...
would have been what I wrote, instead of "either explicit or implied
by", though.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 2/7] maintenance: store the "last run" time in config
From: Junio C Hamano @ 2020-08-26 17:03 UTC (permalink / raw)
To: Derrick Stolee
Cc: Derrick Stolee via GitGitGadget, git, sandals, steadmon, jrnieder,
peff, congdanhqx, phillip.wood123, emilyshaffer, sluongng,
jonathantanmy, Derrick Stolee, Derrick Stolee
In-Reply-To: <4fe41132-c8f5-6b85-414f-5a1e5fcccdc9@gmail.com>
Derrick Stolee <stolee@gmail.com> writes:
>>> 1. The time the task takes to execute should not contribute to the
>>> interval between running the tasks.
>>
>> ... as long as the run time is sufficiently shorter than the
>> interval, that is. If a task takes 10-30 minutes depending on how
>> dirty the repository is, it does not make sense to even try to run
>> it every 15 minutes.
>
> Definitely. The lock on the object database from earlier prevents these
> longer-than-anticipated tasks from stacking.
Hmph, I actually was (anticipating|hoping) that you would give a
good argument for having maintenance subsystem in change of
scheduling rather than cron, as it can monitor how the already
running job is goind and skip one cycle if needed. The above is
instead a good argument that independent cron jobs can still
coordinate and there is no central and custom scheduler in the form
of 'maintenance run'.
>>> 2. If the task fails for some unforseen reason, it would be good to
>>> indicate that we _attempted_ the task at a certain timestamp. This
>>> will avoid spamming a repository that is in a bad state.
>>
>> Absolutely.
Somebody already mentioned that using the configuration file for
recordkeeping may not be a good idea, and I tend to agree, by the
way. I may want to periodically take a snapshot of my configuration
to notice and remember changes I made myself intentionally
(e.g. switched access method of a hosting site from ssh:// to
https://, added a new branch that builds on something else, etc.) by
comparing the snapshot with previous ones (and might even put it
under version-control) and mechanical noise would interfere with it.
^ permalink raw reply
* Re: [PATCH v2 1/7] maintenance: optionally skip --auto process
From: Junio C Hamano @ 2020-08-26 16:57 UTC (permalink / raw)
To: Derrick Stolee
Cc: Derrick Stolee via GitGitGadget, git, sandals, steadmon, jrnieder,
peff, congdanhqx, phillip.wood123, emilyshaffer, sluongng,
jonathantanmy, Derrick Stolee, Derrick Stolee
In-Reply-To: <1e107b2e-364a-4c17-90b8-0371e8e27878@gmail.com>
Derrick Stolee <stolee@gmail.com> writes:
> Before the maintenance builtin, it would be natural to see "gc.auto=0"
> and do this same behavior, but that will not be general enough in the
> future.
I do think it is an excellent change to move the check done in
the need_to_gc() to check the value of gc.auto to run_auto_gc()
for exactly the reason the log message of this patch gives. And
after the function is renamed to run_auto_maintenance(), at some
point the variable that gets checked would also be updated, and
we'd eventually reach the same state, I would think.
But it is so small a change that it probably is not worth the
book-keeping burden of remembering that the maintenance topic needs
to build on the patch to update auto-gc.
> If you prefer, I can pull this out into a series on its own to be
> tracked separately.
So let's leave it as-is.
Thanks.
^ permalink raw reply
* Re: [PATCH v1 0/3] War on dashed-git
From: Junio C Hamano @ 2020-08-26 16:45 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King
In-Reply-To: <nycvar.QRO.7.76.6.2008261007100.56@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> So maybe this patch series would be a good first step, but if we truly
> wanted to break that 12-year old promise, we might need to have another
> patch on top that _does_ install the dashed commands, but into a
> subdirectory of `libexec/git-core/` that is only added to the `PATH` when
> an "escape hatch"-style config setting is set.
Yes, I think that is a much more reasonable approach. Instead of
checking another environment variable that does not do anything but
bypass the "you used dashed form for builtin" check, we install non
builtins in libexec/git-core and leave the exec-cmd.c::setup_path()
as-is to add it to the PATH. A new location will have the buitlin
binaries, say in libexec/git-core/builtins, and it is not added to
the $PATH by us. The scripts that the users updated 12-years ago by
adding the former to the $PATH now needs to also add the latter,
too, and those users will loudly complain (which is what we want to
see happen [*1*]), but doing so is an easy way to unbreak them while
we reverse the course.
I think the cvsexportcommit and transport-helper changes are worth
salvaging even if we don't remove builtin binaries, so I'll split
them and whip them a bit more into a reasonable shape to be merged
to 'next'. The "break those who say 'git-cat-file'" can be left for
future.
Thanks.
[Footnote]
*1* In the ancient thread, I was sick of hearing complaints that
beat the dead horse, but in this particular case, we do want to hear
from them---that is the primary reason why we are doing it.
https://public-inbox.org/git/7vr68b8q9p.fsf@gitster.siamese.dyndns.org/
^ permalink raw reply
* Re: [PATCH 2/3] submodule: fix style in function definition
From: Junio C Hamano @ 2020-08-26 16:47 UTC (permalink / raw)
To: Shourya Shukla
Cc: Johannes.Schindelin, chriscool, christian.couder, git,
kaartic.sivaraam, liu.denton, peff
In-Reply-To: <20200826094506.GA311769@konoha>
Shourya Shukla <shouryashukla.oo@gmail.com> writes:
>>> The definitions of 'verify_submodule_committish()' and
>>> 'print_submodule_summary()' had wrong styling in terms of the asterisk
>>> placement. Amend them.
>
>> I pointed out only these two, but that does not necessarily mean
>> they are the only ones. Have you checked all the new code added by
>> the series?
>
> There is one more. It is not related to my patch series though.
Cleaning up the existing breakage is outside the scope your series,
but of course fixes as an independent patch is welcomed.
Thanks for checking.
^ permalink raw reply
* Re: [PATCH v1 3/3] git: catch an attempt to run "git-foo"
From: Junio C Hamano @ 2020-08-26 16:30 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King
In-Reply-To: <nycvar.QRO.7.76.6.2008261004120.56@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Junio,
>
> On Tue, 25 Aug 2020, Junio C Hamano wrote:
>
>> If we were to propose removing "git-foo" binaries from the
>> filesystem for built-in commands, we should first see if there are
>> users who will be affected by such a move. When cmd_main() detects
>> we were called not as "git", but as "git-foo", give an error message
>> to ask the user to let us know and stop our removal plan, unless we
>> are running a selected few programs that MUST be callable in the
>> dashed form (e.g. "git-upload-pack").
>>
>> Those who are always using "git foo" form will not be affected, but
>> those who trusted the promise we made to them 12 years ago that by
>> prepending the output of $(git --exec-path) to the list of
>> directories on their $PATH, they can safely keep writing
>> "git-cat-file" will be negatively affected as all their scripts
>> assuming the promise will be kept are now broken.
>
> It might be a good idea to also instrument the existing scripts, to show
> the same warning unless they were called through the `git` binary.
>
> _If_ we were to do this ;-)
Sure.
I am not the advocate for removing builtins from on-disk, though.
The burden of proof... ;-)
^ permalink raw reply
* Re: [PATCH v1 2/3] cvsexportcommit: do not run git programs in dashed form
From: Junio C Hamano @ 2020-08-26 16:28 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Johannes Schindelin, Jeff King
In-Reply-To: <xmqqa6yhxucq.fsf@gitster.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Tue, Aug 25, 2020 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>>> This ancient script runs "git-foo" all over the place. A strange
>>> thing is that it has t9200 tests successfully running, even though
>>> it does not seem to futz with PATH to prepend $(git --exec-path)
>>> output.
>>
>> t/test-lib.sh takes care of that for us, doesn't it?
>
> Actually, it is "git" itself.
>
> The tests spawn "git cvsexportcommit" via the "git" dispatcher. The
> dispatcher adds GIT_EXEC_PATH to PATH in exec-cmd.c::setup_path()
> and that is used to (1) locate "git-foo" from /usr/libexec/git-core/
> that are not builtin and (2) passed to any processes we invoke. I
> think the latter was originally done primarily for not breaking
> hooks, but that is what allows this script going in this particular
> case.
>
> If this were only a fluke in the test that kept otherwise unrunnable
> script passing, I'd say it is an evidence enough that lets us drop
> the cvsexportcommit immediately, but now I rediscovered how it was
> supposed to work and saw how it actually does work as designed, I
> would not be surprised if it is still used in the wild.
So, the conclusion: the proposed log message needs a large revamp.
Thanks.
^ permalink raw reply
* Re: [PATCH v1 1/3] transport-helper: do not run git-remote-ext etc. in dashed form
From: Junio C Hamano @ 2020-08-26 16:27 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Johannes Schindelin, Jeff King
In-Reply-To: <CAPig+cTvLaOD1idfB2M0-QSfXXKBe5-FnWSU9E0PaUMHAoGj1w@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Tue, Aug 25, 2020 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Runing them as "git remote-ext" and letting "git" dispatch to
>
> s/Runing/Running/
> s/them/it/
I wrote 'them' because it is not only 'ext' but other helpers are
also run with the same mechanism, but the sentence uses remote-ext
as a single concrete example, so 'it' would be more appropriate.
Thanks.
>> "remote-ext" would just be fine and is more idiomatic.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> diff --git a/transport-helper.c b/transport-helper.c
>> @@ -128,7 +128,8 @@ static struct child_process *get_helper(struct transport *transport)
>> - strvec_pushf(&helper->args, "git-remote-%s", data->name);
>> + strvec_push(&helper->args, "git");
>> + strvec_pushf(&helper->args, "remote-%s", data->name);
>
> Rather than pushing "git" as the first argument, would it instead be
> more idiomatic to set `helper->git_cmd = 1` (or would that not work
> correctly for some reason)?
I was aiming for minimum change and did not think too deeply.
If .git_cmd=1 works here (and offhand I do not see a reason why
not), then that would be simpler. Thanks.
^ permalink raw reply
* Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions
From: Junio C Hamano @ 2020-08-26 16:24 UTC (permalink / raw)
To: Johannes Schindelin
Cc: SZEDER Gábor, Johannes Schindelin via GitGitGadget, git
In-Reply-To: <xmqq5z95xu5f.fsf@gitster.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> Actually, `SKIP_DASHED_BUILT_INS` does not _only_ have an impact on `make
>> install`:
>> ...
>> See how `git-add.exe` is linked in the first, but not in the second run?
>
> OK, that is one more reason why we do want to have 3/3 applied not
> for all tasks in the CI , but for subset of tasks that includes the
> Windows task. If we had multiple Windows tasks, it may even be
> better to have only to some tasks, and allow other tasks build
> git-add.exe, so that both can be tested for the primary intended
> platform.
In other words, something like this squashed in.
ci/run-build-and-tests.sh | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 1df9402c3b..cfb841d981 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -5,12 +5,16 @@
. ${0%/*}/lib.sh
+BUILTINS_HOW=
case "$CI_OS_NAME" in
-windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";;
-*) ln -s "$cache_dir/.prove" t/.prove;;
+windows*)
+ BUILTINS_HOW=SKIP_DASHED_BUILT_INS=YesPlease
+ cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";;
+*)
+ ln -s "$cache_dir/.prove" t/.prove;;
esac
-make SKIP_DASHED_BUILT_INS=YesPlease
+make $BUILTINS_HOW
case "$jobname" in
linux-gcc)
make test
^ permalink raw reply related
* Re: [PATCH v3 0/8] Maintenance II: prefetch, loose-objects, incremental-repack tasks
From: Derrick Stolee @ 2020-08-26 16:21 UTC (permalink / raw)
To: Son Luong Ngoc, Derrick Stolee via GitGitGadget
Cc: git, sandals, steadmon, jrnieder, Jeff King, congdanhqx,
phillip.wood123, Emily Shaffer, Jonathan Tan, Derrick Stolee
In-Reply-To: <56C4015E-B431-4B16-8E05-5919BFA71E80@gmail.com>
On 8/26/2020 11:15 AM, Son Luong Ngoc wrote:
> Hi Derrick,
>
>> On Aug 25, 2020, at 20:36, Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:
>> * Dropped the "verify, and delete and rewrite on failure" logic from the
>> incremental-repack task. This might be added again later after it can be
>> tested more thoroughly.
>
> Perhaps I missed some conversations related to this change but
> why was this verify-rewrite strategy dropped?
>
> Was the problem such strategy were created to solve is now no longer a concern?
>
> I feel like it would be much better to add it in and then remove it using a separated commit?
> That way we can follow the reasoning behind these decisions via commit message.
The most-recent message was [1]
[1] https://lore.kernel.org/git/20200819174322.3087791-1-jonathantanmy@google.com/
For now, I'd rather move forward with this simpler task
and I will revisit the "verify and fix" situation when
it can be done in a focused way instead of being surrounded
by builtin boilerplate and other basics of the maintenance
feature. Specifically, it would help to have a way to test
the logic. In Scalar, I was able to mock the Git commands
and return failures in specific places. A similar approach
could be done here, or perhaps there is another way to be
confident that the "verify and fix" logic is actually
helpful.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v3 2/3] Optionally skip linking/copying the built-ins
From: Junio C Hamano @ 2020-08-26 16:20 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <52deafded50ef99d904d2af39c098028b8714e86.1598443012.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> For a long time already, the non-dashed form of the built-ins is the
> recommended way to write scripts, i.e. it is better to call `git merge
> [...]` than to call `git-merge [...]`.
>
> While Git still supports the dashed form (by hard-linking the `git`
> executable to the dashed name in `libexec/git-core/`), in practice, it
> is probably almost irrelevant.
Let's drop this paragraph. We do not have to defend this new opt-in
feature with "probably". Even if there are folks heavily depend on
the old promise of having all binaries on disk, giving the rest of
the world an option to have Git without that promise is a good thing
as long as there is a good reason why the rest of the world would
want to omit binaries for builtins. And we do have a good one below.
> However, we *do* care about keeping people's scripts working (even if
> they were written before the non-dashed form started to be recommended).
That misses the point. They were written in dashed form, and when
we started recommending non-dashed form, they were TOLD to tweak it
by adjusting PATH early in their script, and they did so to keep the
script working. So it is not "even if". We do care because we
promised them that we will *NOT* break them if they did such and
such, and they followed that recommendation.
> Keeping this backwards-compatibility is not necessarily cheap, though:
> ...
> introduce a Makefile knob to skip generating them.
I do not have anything add to the good argument above (elided) to
allow those who know they do not rely on the age old promise. Well
written.
^ permalink raw reply
* Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions
From: Junio C Hamano @ 2020-08-26 16:13 UTC (permalink / raw)
To: Johannes Schindelin
Cc: SZEDER Gábor, Johannes Schindelin via GitGitGadget, git
In-Reply-To: <nycvar.QRO.7.76.6.2008260615280.56@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Actually, `SKIP_DASHED_BUILT_INS` does not _only_ have an impact on `make
> install`:
> ...
> See how `git-add.exe` is linked in the first, but not in the second run?
OK, that is one more reason why we do want to have 3/3 applied not
for all tasks in the CI , but for subset of tasks that includes the
Windows task. If we had multiple Windows tasks, it may even be
better to have only to some tasks, and allow other tasks build
git-add.exe, so that both can be tested for the primary intended
platform.
Thanks.
^ permalink raw reply
* Re: [PATCH v1 2/3] cvsexportcommit: do not run git programs in dashed form
From: Junio C Hamano @ 2020-08-26 16:08 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Johannes Schindelin, Jeff King
In-Reply-To: <CAPig+cR-eYCVQLRa0rVhgJ8L60-zCS_aK6_nVERcrXSyApdihw@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Tue, Aug 25, 2020 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>> This ancient script runs "git-foo" all over the place. A strange
>> thing is that it has t9200 tests successfully running, even though
>> it does not seem to futz with PATH to prepend $(git --exec-path)
>> output.
>
> t/test-lib.sh takes care of that for us, doesn't it?
Actually, it is "git" itself.
The tests spawn "git cvsexportcommit" via the "git" dispatcher. The
dispatcher adds GIT_EXEC_PATH to PATH in exec-cmd.c::setup_path()
and that is used to (1) locate "git-foo" from /usr/libexec/git-core/
that are not builtin and (2) passed to any processes we invoke. I
think the latter was originally done primarily for not breaking
hooks, but that is what allows this script going in this particular
case.
If this were only a fluke in the test that kept otherwise unrunnable
script passing, I'd say it is an evidence enough that lets us drop
the cvsexportcommit immediately, but now I rediscovered how it was
supposed to work and saw how it actually does work as designed, I
would not be surprised if it is still used in the wild.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 4/7] for-each-repo: run subcommands on configured repos
From: Derrick Stolee @ 2020-08-26 16:03 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
In-Reply-To: <xmqq8se2z7uh.fsf@gitster.c.googlers.com>
On 8/25/2020 6:19 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +SYNOPSIS
>> +--------
>> +[verse]
>> +'git for-each-repo' --config=<config> [--] <arguments>
>> + ...
>> +--config=<config>::
>> + Use the given config variable as a multi-valued list storing
>> + absolute path names.
>
> Would it make sense to allow this config to be read from the current
> repository, I wonder. It is probably designed to be written to
> either ~/.gitconfig or /etc/gitconfig because it is probably a need
> that is not per-repository to list repositories for various purposes
> specified by the config key, but I suspect there _might_ be a good
> use case for storing some custom list of repositories in the
> configuration file local to a repository, but it is not quite
> obvious what it is.
>
> If we have a good example, we may want to spell it out---that would
> help future readers who wonder about this (just like I am doing now).
>
> Also, if we do read from local config, should there be a way to say
> "ah, you may have read values from /etc/gitconfig and ~/.gitconfig,
> but please forget them---I have a full list I care when you are
> running in this repository", i.e. clear the list. It is purely a
> convention and there is no built-in mechanism for this in the config
> API, but often it is signalled by giving an empty string as a value.
I guess I should test this, but if I ask for a multi-valued config,
will I not get _all_ of the results from /etc/gitconfig, ~/.gitconfig,
AND .git/config? That was my expectation, which is why I don't specify
"local" or "global" config anywhere in the discussion.
> By the way, I do not have a good concrete suggestion, but can we use
> something better than <config> as the placeholder? I first thought
> this was naming the name of a file that lists repositories, not the
> config variable name in our usual config namespace.
Sure. How about "<key>"?
>> +static int run_command_on_repo(const char *path,
>> + void *cbdata)
>
> Is that on repo or in repo? When I saw "-C" on the command line, I
> immediately thought of "in repo".
"in" is better.
>> +{
>> + int i;
>> + struct child_process child = CHILD_PROCESS_INIT;
>> + struct strvec *args = (struct strvec *)cbdata;
>> +
>> + child.git_cmd = 1;
>> + strvec_pushl(&child.args, "-C", path, NULL);
>> +
>> + for (i = 0; i < args->nr; i++)
>> + strvec_push(&child.args, args->v[i]);
>
> Would strvec_pushv() work, or is args->v[] not NULL terminated?
Yeah, pushv should work.
>> + return run_command(&child);
>> +}
>
>
>> + values = repo_config_get_value_multi(the_repository,
>> + config_key);
>
> Not your fault, but it is a bit unsatisfactory that we do not have
> special "type" meant for paths in the config API, unlike the
> parse-options API where there is a "filename" type that is a bit
> richer than a vanilla "string" type by allowing "prefix" handling.
> For the purposes of this, as the values are limited to absolute/full
> pathnames, it does not hurt as much, though.
Interesting. Noted.
Thanks,
-Stolee
^ permalink raw reply
* [PATCH] clone: add remote.cloneDefault config option
From: Sean Barag via GitGitGadget @ 2020-08-26 15:45 UTC (permalink / raw)
To: git; +Cc: Sean Barag, Sean Barag
From: Sean Barag <sean@barag.org>
While the default remote name of `origin` can be overridden both
pre-clone (with `git clone --origin foo`) and post-clone (with `git
remote rename origin foo`), it'd be handy to have a configurable
system-wide default for clones! This commit implements
`remote.cloneDefault` as a parallel to `remote.pushDefault`,
with prioritized name resolution:
1. (Highest priority) `git clone`'s `-o` option
2. `git config`'s `remote.cloneDefault` value
3. `origin`
There should be no impact for existing users, as it's pretty unlikely
that anyone's already configured `remote.cloneDefault` and the porcelain
hasn't changed (as best I can tell at least!).
Signed-off-by: Sean Barag <sean@barag.org>
---
clone: add remote.cloneDefault config option
While the default remote name of origin can be overridden both pre-clone
(with git clone --origin foo) and post-clone (with git remote rename
origin foo), it'd be handy to have a configurable system-wide default
for clones! This implementsremote.cloneDefault as a parallel to
remote.pushDefault, with prioritized name resolution:
1. (Highest priority) git clone's -o option
2. git config's remote.cloneDefault value
3. origin
There should be no impact for existing users, as it's pretty unlikely
that anyone's already configured remote.cloneDefault and the porcelain
hasn't changed (as best I can tell at least!).
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-710%2Fsjbarag%2Fadd-remote.cloneDefault-config-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-710/sjbarag/add-remote.cloneDefault-config-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/710
Documentation/config/remote.txt | 4 ++++
Documentation/git-clone.txt | 5 +++--
builtin/clone.c | 34 +++++++++++++++++++++++++++++++--
t/t5606-clone-options.sh | 14 ++++++++++++++
4 files changed, 53 insertions(+), 4 deletions(-)
diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt
index a8e6437a90..debb21ecbf 100644
--- a/Documentation/config/remote.txt
+++ b/Documentation/config/remote.txt
@@ -3,6 +3,10 @@ remote.pushDefault::
`branch.<name>.remote` for all branches, and is overridden by
`branch.<name>.pushRemote` for specific branches.
+remote.cloneDefault::
+ The name of the remote to create during `git clone <url>`.
+ Defaults to "origin".
+
remote.<name>.url::
The URL of a remote repository. See linkgit:git-fetch[1] or
linkgit:git-push[1].
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index c898310099..2e101ba4f4 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -183,8 +183,9 @@ objects from the source repository into a pack in the cloned repository.
-o <name>::
--origin <name>::
- Instead of using the remote name `origin` to keep track
- of the upstream repository, use `<name>`.
+ The remote name used to keep track of the upstream repository.
+ Overrides `remote.cloneDefault` from the config, and defaults
+ to `origin`.
-b <name>::
--branch <name>::
diff --git a/builtin/clone.c b/builtin/clone.c
index b087ee40c2..b0dbb848c6 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -941,6 +941,29 @@ static int path_exists(const char *path)
return !stat(path, &sb);
}
+struct clone_default_info
+{
+ enum config_scope scope;
+ struct strbuf remote_name;
+ int linenr;
+};
+
+static int config_read_clone_default(const char *key, const char *value,
+ void *cb)
+{
+ struct clone_default_info* info = cb;
+ if (strcmp(key, "remote.clonedefault") || !value) {
+ return 0;
+ }
+
+ info->scope = current_config_scope();
+ strbuf_reset(&info->remote_name);
+ strbuf_addstr(&info->remote_name, value);
+ info->linenr = current_config_line();
+
+ return 0;
+}
+
int cmd_clone(int argc, const char **argv, const char *prefix)
{
int is_bundle = 0, is_local;
@@ -992,8 +1015,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
option_no_checkout = 1;
}
- if (!option_origin)
- option_origin = "origin";
+ if (!option_origin) {
+ struct clone_default_info clone_default = { CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 };
+ git_config(config_read_clone_default, &clone_default);
+ if (strcmp("", (const char*) clone_default.remote_name.buf))
+ option_origin = clone_default.remote_name.buf;
+ else
+ option_origin = "origin";
+ }
+
repo_name = argv[0];
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index e69427f881..8aac67b385 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -19,6 +19,20 @@ test_expect_success 'clone -o' '
'
+test_expect_success 'clone respects remote.cloneDefault' '
+
+ git -c remote.cloneDefault=bar clone parent clone-config &&
+ (cd clone-config && git rev-parse --verify refs/remotes/bar/master)
+
+'
+
+test_expect_success 'clone chooses correct remote name' '
+
+ git -c remote.cloneDefault=bar clone -o foo parent clone-o-and-config &&
+ (cd clone-o-and-config && git rev-parse --verify refs/remotes/foo/master)
+
+'
+
test_expect_success 'redirected clone does not show progress' '
git clone "file://$(pwd)/parent" clone-redirected >out 2>err &&
base-commit: e9b77c84a0a0df029f2a3a8114e9f22186e7da24
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v2 3/7] maintenance: add --scheduled option and config
From: Derrick Stolee @ 2020-08-26 15:30 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
In-Reply-To: <xmqqd03ez8pp.fsf@gitster.c.googlers.com>
On 8/25/2020 6:01 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> A user may want to run certain maintenance tasks based on frequency, not
>> conditions given in the repository. For example, the user may want to
>> perform a 'prefetch' task every hour, or 'gc' task every day. To assist,
>> update the 'git maintenance run --scheduled' command to check the config
>> for the last run of that task and add a number of seconds. The task
>> would then run only if the current time is beyond that minimum
>> timestamp.
>>
>> Add a '--scheduled' option to 'git maintenance run' to only run tasks
>> that have had enough time pass since their last run. This is done for
>> each enabled task by checking if the current timestamp is at least as
>> large as the sum of 'maintenance.<task>.lastRun' and
>> 'maintenance.<task>.schedule' in the Git config. This second value is
>> new to this commit, storing a number of seconds intended between runs.
>>
>> A user could then set up an hourly maintenance run with the following
>> cron table:
>>
>> 0 * * * * git -C <repo> maintenance run --scheduled
>
> The scheme has one obvious drawback. An hourly crontab entry means
> your maintenance.*.schedule that is finer grained than an hour
> increment will not run as expected. You'd need to take all the
> schedule intervals and take their GCD to come up with the frequency
> of the single crontab entry.
My intention for the *.schedule is that it is not an _exact_ frequency,
but instead a lower bound on the frequency. That can be shelved for now
as we discuss this setup:
> Wouldn't it make more sense to have N crontab entries for N tasks
> you want to run periodically, each with their own frequency
> controlled by crontab? That way, you do not need to maintain
> maintenance.*.schedule configuration variables and the --scheduled
> option. It might make maintenance.*.lastrun timestamps unneeded,
> which would be an added plus to simplify the system quite
> drastically. Most importantly, that would be the way crontab users
> are most used to in order to schedule their periodical jobs, so it
> is one less thing to learn.
I had briefly considered setting up crontab entries for each task
(and possibly each repo) but ended up with these complications:
1. Maintenance frequency differs by task, so we need to split the
crontab by task. But we can't just split everything because we
do not want multiple tasks running at the same time on one
repository. We would need to group the tasks and have one entry
saying "git maintenance run --task=<task1> --task=<task2> ..."
for all tasks in the group.
2. Different repositories might want different tasks at different
frequencies, so we might need to split the crontab by repository.
Again, we likely want to group repositories by these frequencies
because a user could have 100 registered repositories and we don't
really want to launch 100 parallel processes.
3. If we want to stop maintenance, then restart it, we need to
clear the crontab and repopulate it, which would require iterating
through all "registered" repositories to read their config for
frequencies.
4. On macOS, editing the crontab doesn't require "sudo" but it _does_
create a pop-up(!) to get permission from the user. It would be
good to minimize how often we edit the crontab and instead use
config edits to change frequencies.
With these things in mind, here is a suggested alternative design:
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.
Then, "git maintenance (start|stop)" can be just as simple as we have
now: write a fixed schedule every time.
The problem here is that cron will launch these processes in parallel,
and then our object-database lock will cause some to fail! If anyone
knows a simple way to tell cron "run hourly _except_ not at midnight"
then we could let the "daily" schedule also run the "hourly" jobs, for
instance. Hopefully that pattern could be extended to the weekly and
monthly collisions.
Alternatively, we could run every hour and then interpret from config
if the current "hour" matches one of the schedules ourselves. So, the
crontab would be this simple:
0 * * * * git for-each-repo --config=maintenance.repos maintenance run --scheduled
and then we would internally decide "is this the midnight hour?" and
"is this the first day of the week?" and "is this the first day of the
month?" to detect if we should run the daily/weekly/monthly tasks. While
it adds more time-awareness into Git, it does avoid the parallel task
collisions. There are some concerns here related to long-running tasks
delaying sequential runs of "git -C <repo> maintenance run --scheduled"
causing the "is this the midnight hour?" queries to fail and having
nightly/weekly/monthly maintenance be skipped accidentally. This
motivates the *.lastRun config giving us some guarantee of _eventually_
running the tasks, just _not too frequently_.
I hope this launches a good discussion to help us find a good cron
schedule strategy. After we land on a suitable strategy, I'll summarize
all of these subtleties in the commit message for posterity.
Hopefully, the current way that I integrate with crontab and test that
integration (in PATCH 6/7) could also be reviewed in parallel with this
discussion. I'm very curious to see how that could be improved.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v3 0/8] Maintenance II: prefetch, loose-objects, incremental-repack tasks
From: Son Luong Ngoc @ 2020-08-26 15:15 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, sandals, steadmon, jrnieder, Jeff King, congdanhqx,
phillip.wood123, Emily Shaffer, Jonathan Tan, Derrick Stolee
In-Reply-To: <pull.696.v3.git.1598380599.gitgitgadget@gmail.com>
Hi Derrick,
> On Aug 25, 2020, at 20:36, Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:
>
...
>
> Updates since v2
> ================
>
> * Dropped "fetch: optionally allow disabling FETCH_HEAD update"
>
>
> * A lot of fallout from the change in the option parsing in v3 of
> Maintenance II.
>
>
> * Dropped the "verify, and delete and rewrite on failure" logic from the
> incremental-repack task. This might be added again later after it can be
> tested more thoroughly.
Perhaps I missed some conversations related to this change but
why was this verify-rewrite strategy dropped?
Was the problem such strategy were created to solve is now no longer a concern?
I feel like it would be much better to add it in and then remove it using a separated commit?
That way we can follow the reasoning behind these decisions via commit message.
>
...
>
> --
> gitgitgadget
Thanks,
Son Luong.
^ permalink raw reply
* Re: [PATCH v1 0/3] War on dashed-git
From: Johannes Schindelin @ 2020-08-26 8:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <20200826011718.3186597-1-gitster@pobox.com>
Hi Junio,
On Tue, 25 Aug 2020, Junio C Hamano wrote:
> If we were to propose breaking the 12-year old promise to our users
> that we will keep "git-foo" binaries on disk where "git --exec-path"
> tells them, we first need to gauge how big a population we would be
> hurting with such a move.
>
> So here is a miniseries towards that. The third one hooks to the
> codepath in git.c::cmd_main() where av[0] is of "git-foo" form and
> we dispatch to "foo" as a builtin command. In the original code, we
> will die() if "foo" is not a built-in command in this codepath, so
> it is exactly the place we want to catch remaining uses of "git-foo"
> invoking built-in commands.
>
> There are a few legitimate "git-foo" calls made even for built-ins
> and those exceptions are marked in the command-list mechanism, which
> is shared with the help subsystem. We might want to see if we can
> unify this exception list with what we have in the Makefile as
> BIN_PROGRAMS and what Dscho introduces as ALL_COMMANDS_TO_INSTALL
> in his series. These have large overlaps in what they mean, but
> they are not exactly identical.
As it happens, I discussed a scenario the other day where dashed
invocations might still happen, and the consensus was that nobody looks at
the output unless things are broken.
So maybe this patch series would be a good first step, but if we truly
wanted to break that 12-year old promise, we might need to have another
patch on top that _does_ install the dashed commands, but into a
subdirectory of `libexec/git-core/` that is only added to the `PATH` when
an "escape hatch"-style config setting is set.
Ciao,
Dscho
^ 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