* Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing
From: Christian Couder @ 2020-08-26 6:22 UTC (permalink / raw)
To: Hariom verma
Cc: Eric Sunshine, Junio C Hamano, Hariom Verma via GitGitGadget,
Git List, Heba Waly
In-Reply-To: <CAP8UFD03Am94_84FvRPxEdt_AG74864eQ4TimggKtUYWjJYqCg@mail.gmail.com>
On Wed, Aug 26, 2020 at 8:18 AM Christian Couder
<christian.couder@gmail.com> wrote:
> I think we could also get rid of the "match_and_" part of the
> suggestion, in the same way as skip_prefix() is not called
> match_and_skip_prefix(). Readers can just expect that if there is no
> match the function will return 0.
>
> So maybe "extract_field_option()".
If we want to hint more that it works in the way as skip_prefix(), we
could call it "skip_field()".
^ permalink raw reply
* Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing
From: Christian Couder @ 2020-08-26 6:18 UTC (permalink / raw)
To: Hariom verma
Cc: Eric Sunshine, Junio C Hamano, Hariom Verma via GitGitGadget,
Git List, Heba Waly
In-Reply-To: <CA+CkUQ_eRqOB8Ushg-BcEmjRxEZSs7tmPnZcb8GUTwz3R55Xhg@mail.gmail.com>
Hi,
On Tue, Aug 25, 2020 at 1:32 AM Hariom verma <hariom18599@gmail.com> wrote:
> On Mon, Aug 24, 2020 at 9:19 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> >
> > On Sun, Aug 23, 2020 at 8:56 PM Hariom verma <hariom18599@gmail.com> wrote:
> > > Recently, I sent a patch series "Improvements to ref-filter"[1]. A
> > > patch in this patch series introduced "sanitize" modifier to "subject"
> > > atom. i.e "%(subject:sanitize)".
> > >
> > > What if in the future we also want "%(contents:subject:sanitize)" to work?
> > > We can use this helper any number of times, whenever there is a need.
> > >
> > > Sorry, I missed saying this earlier. But I don't prefer duplicating
> > > the code here.
> >
> > Pushing back on a reviewer suggestion is fine. Explaining the reason
> > for your position -- as you do here -- helps reviewers understand why
> > you feel the way you do. My review suggestion about making it easier
> > to reason about the code while avoiding a brand new function, at the
> > cost of a minor amount of duplication, was made in the context of this
> > one-off case in which the function increased cognitive load and was
> > used just once (not knowing that you envisioned future callers). If
> > you expect the new function to be re-used by upcoming changes, then
> > that may be a good reason to keep it. Stating so in the commit message
> > will help reviewers see beyond the immediate patch or patch series.
>
> Yeah. I should have mentioned this in the commit message.
I agree.
> > Aside from a couple minor style violations[1,2], I don't particularly
> > oppose the helper function, though I have a quibble with the name
> > check_format_field(), which I don't find helpful, and which (at least
> > for me) increases the cognitive load. The increased cognitive load, I
> > think, comes not only from the function name not spelling out what the
> > function actually does, but also because the function is dual-purpose:
> > it's both checking that the argument matches a particular token
> > ("trailers", in this case) and extracting the sub-argument. Perhaps
> > naming it match_and_extract_subarg() or something similar would help,
> > though that's a mouthful.
>
> I will fix those violations.
> Also, "match_and_extract_subarg()" looks good to me.
I am not sure about the "subarg" part of the name. In the for-each-ref
doc, names inside %(...) are called "field names", and parts after ":"
are called "options". So it might be better to have "field_option"
instead of "subarg" in the name.
I think we could also get rid of the "match_and_" part of the
suggestion, in the same way as skip_prefix() is not called
match_and_skip_prefix(). Readers can just expect that if there is no
match the function will return 0.
So maybe "extract_field_option()".
> > But the observation about the function being dual-purpose (thus
> > potentially confusing) brings up other questions. For instance, is it
> > too special-purpose? If you foresee more callers in the future with
> > multiple-token arguments such as `%(content:subject:sanitize)`, should
> > the function provide more assistance by splitting out each of the
> > sub-arguments rather than stopping at the first? Taking that even
> > further, a generalized helper for "splitting" arguments like that
> > might be useful at the top-level of contents_atom_parser() too, rather
> > than only for specific arguments, such as "trailers". Of course, this
> > may all be way too ambitious for this little bug fix series or even
> > for whatever upcoming changes you're planning, thus not worth
> > pursuing.
>
> Splitting sub-arguments is done at "<atomname>_atom_parser()".
> If you mean pre-splitting every argument...
> something like: ['contents', 'subject', 'sanitize'] for
> `%(content:subject:sanitize)` in `contents_atom_parser()` ? I'm not
> able to see how it can be useful.
Yeah, it seems to me that such a splitting would require a complete
rewrite of the current code, so I am not sure it's an interesting way
forward for now. And anyway adding extract_field_option() goes in the
right direction of abstracting the parsing and making the code
simpler, more efficient and likely more correct.
> Sorry, If I got your concerned wrong.
>
> > As for the helper's implementation, I might have written it like this:
> >
> > static int check_format_field(...)
> > {
> > const char *opt
> > if (!strcmp(arg, field))
> > *option = NULL;
> > else if (skip_prefix(arg, field, opt) && *opt == ':')
> > *option = opt + 1;
> > else
> > return 0;
> > return 1;
> > }
> >
> > which is more compact and closer to what I suggested earlier for
> > avoiding the helper function in the first place. But, of course,
> > programming is quite subjective, and you may find your implementation
> > easier to reason about. Plus, your version has the benefit of being
> > slightly more optimal since it avoids an extra string scan, although
> > that probably is mostly immaterial considering that
> > contents_atom_parser() itself contains a long chain of potentially
> > sub-optimal strcmp() and skip_prefix() calls.
>
> "programming is quite subjective"
> Yeah, I couldn't agree more.
>
> The change you suggested looks good too. But I'm little inclined to my
> keeping my changes. I'm curious, what others have to say on this.
I also prefer a slightly more optimal one even if it's a bit less compact.
Thanks,
Christian.
^ permalink raw reply
* Re: Temporary credentials timeout during long operations
From: brian m. carlson @ 2020-08-26 1:44 UTC (permalink / raw)
To: Peterson, Alex; +Cc: git
In-Reply-To: <1f95e9da5e734dd3a8f94c1337f8c756@EX13D10UWA004.ant.amazon.com>
[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]
[Please keep the list in CC. Other people may have important
contributions to the discussion, and due to weather, I may be offline at
some point in the future and be unable to respond.]
On 2020-08-26 at 00:18:05, Peterson, Alex wrote:
> Hi Brian,
>
> Unfortunately, even if the server returns a 401, git will retry but with the old expired credentials which will fail. I believe it is because of this line that checks if a username/password already exists (which it does)
> https://github.com/git/git/blob/07d8ea56f2ecb64b75b92264770c0a664231ce17/credential.c#L338
>
> In my test I cleared the username and password to force it to re-request credentials and that worked OK.
Ah, yes. In that case, it looks like we call credential_reject and then
return HTTP_NOAUTH. I think the assumption is that the credential
helper returns a consistent set of credentials and once we've told the
credential helper to reject them, then the user can push again and be
prompted for new credentials.
I would be open to seeing a patch which, the first time through,
returned HTTP_REAUTH. We wouldn't want to do that indefinitely, since
that would mean that the user would get stuck in a loop if the
credentials were wrong.
I will say that my gut tells me that it's generally a reasonable
assumption that credentials are valid for the life of a push, whatever
that is, so while I'm not opposed to seeing a patch to improve this, I'm
not especially sympathetic to using credentials that have such a short
lifetime that this occurs, even if I am in general in support of
short-lived credentials.
--
brian m. carlson: Houston, Texas, US
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: [PATCH v1 2/3] cvsexportcommit: do not run git programs in dashed form
From: Junio C Hamano @ 2020-08-26 1:42 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?
It generally shouldn't, I think.
We have bin-wrappers directory and we shouldn't be copying things we
would install in libexec/git-core/ in real life.
^ permalink raw reply
* Re: [PATCH v1 2/3] cvsexportcommit: do not run git programs in dashed form
From: Eric Sunshine @ 2020-08-26 1:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Johannes Schindelin, Jeff King
In-Reply-To: <20200826011718.3186597-3-gitster@pobox.com>
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?
> It is tempting to declare that the command must be unused,
> but that is left to another topic.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply
* Re: [PATCH v1 1/3] transport-helper: do not run git-remote-ext etc. in dashed form
From: Eric Sunshine @ 2020-08-26 1:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Johannes Schindelin, Jeff King
In-Reply-To: <20200826011718.3186597-2-gitster@pobox.com>
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/
> "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)?
^ permalink raw reply
* Re: [PATCH v1 3/3] git: catch an attempt to run "git-foo"
From: Junio C Hamano @ 2020-08-26 1:19 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Jeff King
In-Reply-To: <20200826011718.3186597-4-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> 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.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
The same idea as the one for pack-redundant. I do not use the
technique to inspect $PATH and see if $GIT_EXEC_PATH is on it, as
that would mean we will *not* bug users with legitimate need to keep
the feature working, hence will not get "don't do that" objections.
We may want to ensure command_list[] is sorted by name and run
binary search on it if running find_cmdname_help() for each and
every dashed "git-foo" invocations turns out to be costly. Our
conjecture behind this patch is that the form is rarely if ever
used, so it may not matter at all, though.
^ permalink raw reply
* [PATCH v1 3/3] git: catch an attempt to run "git-foo"
From: Junio C Hamano @ 2020-08-26 1:17 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Jeff King
In-Reply-To: <20200826011718.3186597-1-gitster@pobox.com>
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.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
command-list.txt | 11 +++++++----
git.c | 2 ++
help.c | 34 ++++++++++++++++++++++++++++++++++
help.h | 3 +++
4 files changed, 46 insertions(+), 4 deletions(-)
diff --git a/command-list.txt b/command-list.txt
index e5901f2213..1238f6ec8b 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -39,6 +39,9 @@
# mainporcelain commands are completable so you don't need this
# attribute.
#
+# "onpath" attribute is used to mark that the command MUST appear
+# on $PATH (typically in /usr/bin) due to protocol requirement.
+#
# As part of the Git man page list, the man(5/7) guides are also
# specified here, which can only have "guide" attribute and nothing
# else.
@@ -144,7 +147,7 @@ git-quiltimport foreignscminterface
git-range-diff mainporcelain
git-read-tree plumbingmanipulators
git-rebase mainporcelain history
-git-receive-pack synchelpers
+git-receive-pack synchelpers onpath
git-reflog ancillarymanipulators complete
git-remote ancillarymanipulators complete
git-repack ancillarymanipulators complete
@@ -159,7 +162,7 @@ git-rev-parse plumbinginterrogators
git-rm mainporcelain worktree
git-send-email foreignscminterface complete
git-send-pack synchingrepositories
-git-shell synchelpers
+git-shell synchelpers onpath
git-shortlog mainporcelain
git-show mainporcelain info
git-show-branch ancillaryinterrogators complete
@@ -182,8 +185,8 @@ git-unpack-objects plumbingmanipulators
git-update-index plumbingmanipulators
git-update-ref plumbingmanipulators
git-update-server-info synchingrepositories
-git-upload-archive synchelpers
-git-upload-pack synchelpers
+git-upload-archive synchelpers onpath
+git-upload-pack synchelpers onpath
git-var plumbinginterrogators
git-verify-commit ancillaryinterrogators
git-verify-pack plumbinginterrogators
diff --git a/git.c b/git.c
index 8bd1d7551d..927018bda7 100644
--- a/git.c
+++ b/git.c
@@ -839,6 +839,8 @@ int cmd_main(int argc, const char **argv)
* that one cannot handle it.
*/
if (skip_prefix(cmd, "git-", &cmd)) {
+ warn_on_dashed_git(argv[0]);
+
argv[0] = cmd;
handle_builtin(argc, argv);
die(_("cannot handle %s as a builtin"), cmd);
diff --git a/help.c b/help.c
index d478afb2af..490d2bc3ae 100644
--- a/help.c
+++ b/help.c
@@ -720,3 +720,37 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd,
string_list_clear(&suggested_refs, 0);
exit(1);
}
+
+static struct cmdname_help *find_cmdname_help(const char *name)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(command_list); i++) {
+ if (!strcmp(command_list[i].name, name))
+ return &command_list[i];
+ }
+ return NULL;
+}
+
+void warn_on_dashed_git(const char *cmd)
+{
+ struct cmdname_help *cmdname;
+ static const char *still_in_use_var = "GIT_I_STILL_USE_DASHED_GIT";
+ static const char *still_in_use_msg =
+ N_("Use of '%s' in the dashed-form is nominated for removal.\n"
+ "If you still use it, export '%s=true'\n"
+ "and send an e-mail to <git@vger.kernel.org>\n"
+ "to let us know and stop our removal plan. Thanks.\n");
+
+ if (!cmd)
+ return; /* git-help is OK */
+
+ cmdname = find_cmdname_help(cmd);
+ if (cmdname && (cmdname->category & CAT_onpath))
+ return; /* git-upload-pack and friends are OK */
+
+ if (!git_env_bool(still_in_use_var, 0)) {
+ fprintf(stderr, _(still_in_use_msg), cmd, still_in_use_var);
+ exit(1);
+ }
+}
diff --git a/help.h b/help.h
index dc02458855..d3de5e0d69 100644
--- a/help.h
+++ b/help.h
@@ -45,6 +45,9 @@ void get_version_info(struct strbuf *buf, int show_build_options);
*/
NORETURN void help_unknown_ref(const char *ref, const char *cmd, const char *error);
+/* When the cmd_main() sees "git-foo", check if the user intended */
+void warn_on_dashed_git(const char *);
+
static inline void list_config_item(struct string_list *list,
const char *prefix,
const char *str)
--
2.28.0-454-g5f859b1948
^ permalink raw reply related
* [PATCH v1 2/3] cvsexportcommit: do not run git programs in dashed form
From: Junio C Hamano @ 2020-08-26 1:17 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Jeff King
In-Reply-To: <20200826011718.3186597-1-gitster@pobox.com>
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. It is tempting to declare that the command must be unused,
but that is left to another topic.
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 v1 1/3] transport-helper: do not run git-remote-ext etc. in dashed form
From: Junio C Hamano @ 2020-08-26 1:17 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Jeff King
In-Reply-To: <20200826011718.3186597-1-gitster@pobox.com>
Runing them 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 | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/transport-helper.c b/transport-helper.c
index defafbf4c1..fae3d99500 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -128,7 +128,8 @@ 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_push(&helper->args, "git");
+ 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;
--
2.28.0-454-g5f859b1948
^ permalink raw reply related
* [PATCH v1 0/3] War on dashed-git
From: Junio C Hamano @ 2020-08-26 1:17 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Jeff King
In-Reply-To: <xmqq1rjuz6n3.fsf_-_@gitster.c.googlers.com>
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.
Junio C Hamano (3):
transport-helper: do not run git-remote-ext etc. in dashed form
cvsexportcommit: do not run git programs in dashed form
git: catch an attempt to run "git-foo"
command-list.txt | 11 +++++++----
git-cvsexportcommit.perl | 16 ++++++++--------
git.c | 2 ++
help.c | 34 ++++++++++++++++++++++++++++++++++
help.h | 3 +++
transport-helper.c | 3 ++-
6 files changed, 56 insertions(+), 13 deletions(-)
--
2.28.0-454-g5f859b1948
^ permalink raw reply
* Re: [PATCH] pack-redundant: gauge the usage before proposing its removal
From: Junio C Hamano @ 2020-08-25 23:22 UTC (permalink / raw)
To: Taylor Blau; +Cc: Jeff King, git, dstolee
In-Reply-To: <20200825230904.GA23203@syl.lan>
Taylor Blau <me@ttaylorr.com> writes:
> On Tue, Aug 25, 2020 at 03:45:52PM -0700, Junio C Hamano wrote:
>> The subcommand is unusably slow and the reason why nobody reports it
>> as a performance bug is suspected to be the absense of users. Let's
>> show a big message that asks the user to tell us that they still
>> care about the command when an attempt is made to run the command,
>> with an escape hatch to override it with a command line option.
>>
>> In a few releases, we may turn it into an error and keep it for a
>> few more releases before finally removing it (during the whole time,
>> the plan to remove it would be interrupted by end user raising hand).
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>
> Thanks. Peff's plan seems reasonable to me (and I'd like to add that I
> am a frequent reader of the release notes ;-)), as does this patch.
>
> Reviewed-by: Taylor Blau <me@ttaylorr.com>
Thanks. It needs updates to a test script, though.
diff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh
index 6b4d1ca353..2dd2d67b9e 100755
--- a/t/t5323-pack-redundant.sh
+++ b/t/t5323-pack-redundant.sh
@@ -39,6 +39,8 @@ relationship between packs and objects is as follows:
master_repo=master.git
shared_repo=shared.git
+git_pack_redundant='git pack-redundant --i-still-use-this'
+
# Create commits in <repo> and assign each commit's oid to shell variables
# given in the arguments (A, B, and C). E.g.:
#
@@ -154,7 +156,7 @@ test_expect_success 'master: no redundant for pack 1, 2, 3' '
EOF
(
cd "$master_repo" &&
- git pack-redundant --all >out &&
+ $git_pack_redundant --all >out &&
test_must_be_empty out
)
'
@@ -192,7 +194,7 @@ test_expect_success 'master: one of pack-2/pack-3 is redundant' '
cat >expect <<-EOF &&
P3:$P3
EOF
- git pack-redundant --all >out &&
+ $git_pack_redundant --all >out &&
format_packfiles <out >actual &&
test_cmp expect actual
)
@@ -231,7 +233,7 @@ test_expect_success 'master: pack 2, 4, and 6 are redundant' '
P4:$P4
P6:$P6
EOF
- git pack-redundant --all >out &&
+ $git_pack_redundant --all >out &&
format_packfiles <out >actual &&
test_cmp expect actual
)
@@ -266,7 +268,7 @@ test_expect_success 'master: pack-8 (subset of pack-1) is also redundant' '
P6:$P6
P8:$P8
EOF
- git pack-redundant --all >out &&
+ $git_pack_redundant --all >out &&
format_packfiles <out >actual &&
test_cmp expect actual
)
@@ -284,9 +286,9 @@ test_expect_success 'master: clean loose objects' '
test_expect_success 'master: remove redundant packs and pass fsck' '
(
cd "$master_repo" &&
- git pack-redundant --all | xargs rm &&
+ $git_pack_redundant --all | xargs rm &&
git fsck &&
- git pack-redundant --all >out &&
+ $git_pack_redundant --all >out &&
test_must_be_empty out
)
'
@@ -304,7 +306,7 @@ test_expect_success 'setup shared.git' '
test_expect_success 'shared: all packs are redundant, but no output without --alt-odb' '
(
cd "$shared_repo" &&
- git pack-redundant --all >out &&
+ $git_pack_redundant --all >out &&
test_must_be_empty out
)
'
@@ -343,7 +345,7 @@ test_expect_success 'shared: show redundant packs in stderr for verbose mode' '
P5:$P5
P7:$P7
EOF
- git pack-redundant --all --verbose >out 2>out.err &&
+ $git_pack_redundant --all --verbose >out 2>out.err &&
test_must_be_empty out &&
grep "pack$" out.err | format_packfiles >actual &&
test_cmp expect actual
@@ -356,9 +358,9 @@ test_expect_success 'shared: remove redundant packs, no packs left' '
cat >expect <<-EOF &&
fatal: Zero packs found!
EOF
- git pack-redundant --all --alt-odb | xargs rm &&
+ $git_pack_redundant --all --alt-odb | xargs rm &&
git fsck &&
- test_must_fail git pack-redundant --all --alt-odb >actual 2>&1 &&
+ test_must_fail $git_pack_redundant --all --alt-odb >actual 2>&1 &&
test_cmp expect actual
)
'
@@ -386,7 +388,7 @@ test_expect_success 'shared: create new objects and packs' '
test_expect_success 'shared: no redundant without --alt-odb' '
(
cd "$shared_repo" &&
- git pack-redundant --all >out &&
+ $git_pack_redundant --all >out &&
test_must_be_empty out
)
'
@@ -417,7 +419,7 @@ test_expect_success 'shared: no redundant without --alt-odb' '
test_expect_success 'shared: one pack is redundant with --alt-odb' '
(
cd "$shared_repo" &&
- git pack-redundant --all --alt-odb >out &&
+ $git_pack_redundant --all --alt-odb >out &&
format_packfiles <out >actual &&
test_line_count = 1 actual
)
@@ -454,7 +456,7 @@ test_expect_success 'shared: ignore unique objects and all two packs are redunda
Px1:$Px1
Px2:$Px2
EOF
- git pack-redundant --all --alt-odb >out <<-EOF &&
+ $git_pack_redundant --all --alt-odb >out <<-EOF &&
$X
$Y
$Z
^ permalink raw reply related
* Re: [PATCH] pack-redundant: gauge the usage before proposing its removal
From: Taylor Blau @ 2020-08-25 23:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Taylor Blau, git, dstolee
In-Reply-To: <xmqq1rjuz6n3.fsf_-_@gitster.c.googlers.com>
Hi Junio,
On Tue, Aug 25, 2020 at 03:45:52PM -0700, Junio C Hamano wrote:
> The subcommand is unusably slow and the reason why nobody reports it
> as a performance bug is suspected to be the absense of users. Let's
> show a big message that asks the user to tell us that they still
> care about the command when an attempt is made to run the command,
> with an escape hatch to override it with a command line option.
>
> In a few releases, we may turn it into an error and keep it for a
> few more releases before finally removing it (during the whole time,
> the plan to remove it would be interrupted by end user raising hand).
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
Thanks. Peff's plan seems reasonable to me (and I'd like to add that I
am a frequent reader of the release notes ;-)), as does this patch.
Reviewed-by: Taylor Blau <me@ttaylorr.com>
Thanks,
Taylor
^ permalink raw reply
* [PATCH] pack-redundant: gauge the usage before proposing its removal
From: Junio C Hamano @ 2020-08-25 22:45 UTC (permalink / raw)
To: Jeff King; +Cc: Taylor Blau, git, dstolee
In-Reply-To: <20200825182745.GA1417288@coredump.intra.peff.net>
The subcommand is unusably slow and the reason why nobody reports it
as a performance bug is suspected to be the absense of users. Let's
show a big message that asks the user to tell us that they still
care about the command when an attempt is made to run the command,
with an escape hatch to override it with a command line option.
In a few releases, we may turn it into an error and keep it for a
few more releases before finally removing it (during the whole time,
the plan to remove it would be interrupted by end user raising hand).
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Jeff King <peff@peff.net> writes:
> A more gentle transition would I guess be:
>
> 1. Mention deprecation in release notes (hah, as if anybody reads
> them).
>
> 2. Issue a warning but continue to behave as normal. That might break
> scripts that care a lot about stderr, but otherwise is harmless. No
> clue if anybody would actually see the message or not.
OK, so here is an update for the above.
builtin/pack-redundant.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 178e3409b7..b94c2f2423 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -554,6 +554,7 @@ static void load_all(void)
int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
{
int i;
+ int i_still_use_this = 0;
struct pack_list *min = NULL, *red, *pl;
struct llist *ignore;
struct object_id *oid;
@@ -580,12 +581,24 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
alt_odb = 1;
continue;
}
+ if (!strcmp(arg, "--i-still-use-this")) {
+ i_still_use_this = 1;
+ continue;
+ }
if (*arg == '-')
usage(pack_redundant_usage);
else
break;
}
+ if (!i_still_use_this) {
+ fputs(_("'git pack-redundant' is nominated for removal.\n"
+ "If you still use this command, please add an extra\n"
+ "option, '--i-still-use-this', on the command line\n"
+ "and let us know you still use it by sending an e-mail\n"
+ "to <git@vger.kernel.org>. Thanks.\n"), stderr);
+ }
+
if (load_all_packs)
load_all();
else
--
2.28.0-454-g5f859b1948
^ permalink raw reply related
* Re: [PATCH v2 4/7] for-each-repo: run subcommands on configured repos
From: Junio C Hamano @ 2020-08-25 22:19 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, sandals, steadmon, jrnieder, peff, congdanhqx,
phillip.wood123, emilyshaffer, sluongng, jonathantanmy,
Derrick Stolee, Derrick Stolee
In-Reply-To: <0314258c5cbb8fd771c35e433bf6be95297c4597.1598380805.git.gitgitgadget@gmail.com>
"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.
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.
> +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".
> +{
> + 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?
> + 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.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 3/7] maintenance: add --scheduled option and config
From: Junio C Hamano @ 2020-08-25 22:01 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, sandals, steadmon, jrnieder, peff, congdanhqx,
phillip.wood123, emilyshaffer, sluongng, jonathantanmy,
Derrick Stolee, Derrick Stolee
In-Reply-To: <c728c57d85b17035d42313260620a7de5756b0c3.1598380805.git.gitgitgadget@gmail.com>
"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.
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.
^ permalink raw reply
* Re: [PATCH v2 2/7] maintenance: store the "last run" time in config
From: Junio C Hamano @ 2020-08-25 21:52 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, sandals, steadmon, jrnieder, peff, congdanhqx,
phillip.wood123, emilyshaffer, sluongng, jonathantanmy,
Derrick Stolee, Derrick Stolee
In-Reply-To: <e3ef0b9bea4a31c72ce88841639e88355408f0c1.1598380805.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> I selected the timestamp before the task, as opposed to after the task,
> for a couple reasons:
>
> 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.
> If a daily task takes 10 minutes
> to run, then every day the execution will drift by at least 10
> minutes.
That is not incorrect per-se, but it does not tell us why drifting
by 10 minutes is a bad thing.
> 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.
> +static void update_last_run(struct maintenance_task *task)
> +{
> + timestamp_t now = approxidate("now");
> + struct strbuf config = STRBUF_INIT;
> + struct strbuf value = STRBUF_INIT;
> + strbuf_addf(&config, "maintenance.%s.lastrun", task->name);
> + strbuf_addf(&value, "%"PRItime"", now);
So is this essentially meant as a human-unreadable opaque value,
like we have in the commit object header lines? I do not have a
strong opinion, but it would be nice to allow curious to casually
read it. Perhaps "git config --type=timestamp maintenance.lastrun"
can be taught to pretty print its value?
^ permalink raw reply
* Re: [PATCH 0/7] Better threaded delta resolution in index-pack (another try)
From: Jeff King @ 2020-08-25 21:46 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, steadmon
In-Reply-To: <20200825211836.GA1448402@coredump.intra.peff.net>
On Tue, Aug 25, 2020 at 05:18:36PM -0400, Jeff King wrote:
> Still maxes out at the number of physical cores (not unexpected, but
> that was the thing I was most curious about ;) ). I may run it on the
> 40-core machine, too. It's possible that with the new threading we're
> able to do better going past 20-threads. I doubt it, because I think
> it's mostly a function of Git's locking granularity, but worth checking.
For the curious (I know you were all on the edge of your seat):
5302.3: index-pack 0 threads 519.23(479.10+40.10)
5302.4: index-pack 1 threads 525.94(476.27+49.64)
5302.5: index-pack 2 threads 271.82(458.93+55.52)
5302.6: index-pack 5 threads 115.88(463.84+50.69)
5302.7: index-pack 10 threads 67.26(478.37+57.38)
5302.8: index-pack 20 threads 43.02(524.01+77.33)
5302.9: index-pack 40 threads 33.42(709.86+100.24)
5302.10: index-pack 80 threads 32.02(1030.75+228.28)
5302.11: index-pack default number of threads 43.58(551.13+68.92)
So it actually does do a slight improvement to go from 20 to 40 threads
on this repository/machine combo. Not enough to make me revise the code
I sent the other day, though. And we still get nothing from going past
the number of physical cores.
-Peff
^ permalink raw reply
* Re: [PATCH v2 1/7] maintenance: optionally skip --auto process
From: Junio C Hamano @ 2020-08-25 21:44 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, sandals, steadmon, jrnieder, peff, congdanhqx,
phillip.wood123, emilyshaffer, sluongng, jonathantanmy,
Derrick Stolee, Derrick Stolee
In-Reply-To: <5fdd8188b1d9b6efc2803b557b3ba344e184d22e.1598380805.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Some commands run 'git maintenance run --auto --[no-]quiet' after doing
> their normal work, as a way to keep repositories clean as they are used.
> Currently, users who do not want this maintenance to occur would set the
> 'gc.auto' config option to 0 to avoid the 'gc' task from running.
> However, this does not stop the extra process invocation.
OK, that is because the configuration is checked on the other side,
and the new check is implemented on this side before we decide to
spawn the maintenance task.
It sounds like a change worth having without even waiting for the
"git maintenance" to materialize ;-).
> @@ -1868,8 +1869,13 @@ int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task,
>
> int run_auto_maintenance(int quiet)
> {
> + int enabled;
> struct child_process maint = CHILD_PROCESS_INIT;
>
> + if (!git_config_get_bool("maintenance.auto", &enabled) &&
> + !enabled)
> + return 0;
So in a repository without this configuration, get_bool would fail
and we do not short-circuit. Only if the value get_bool sees is
false, we return without running the command. Makes sense.
^ permalink raw reply
* Paid Post
From: Russell Fenton @ 2020-08-25 21:19 UTC (permalink / raw)
To: git
Hello there,
Hope you are fine and doing well.
I want to make a long-haul business association with you.
I will give you elegantly composed and one of a kind articles identified with your site.
I will give you 2 to 3 articles each month. The links would be the do-follow and relevant to your website. No gambling or adult.
I will pay Via PayPal or Payoneer.
Much obliged.
^ permalink raw reply
* Re: [PATCH 0/7] Better threaded delta resolution in index-pack (another try)
From: Jeff King @ 2020-08-25 21:18 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, steadmon
In-Reply-To: <20200825181145.1091378-1-jonathantanmy@google.com>
On Tue, Aug 25, 2020 at 11:11:45AM -0700, Jonathan Tan wrote:
> > There may be other cases that get better, though. A 3% increase here is
> > probably OK if we get something for it. But if our primary goal here is
> > increasing multithread efficiency, then we should be able to show some
> > benchmark that improves. :)
>
> Ah...good question. Cloning from
> https://fuchsia.googlesource.com/third_party/vulkan-cts (mentioned in
> patch 7), cd-ing to the pack dir, and running:
>
> git index-pack --stdin -o foo <*.pack
>
> I got 8m2.878s with my patches and 12m6.365s without. But I ran this on
> a cloud virtual machine (what I have access to right now) so the numbers
> might look different on a dedicated machine.
Thanks, that's a much more interesting example. Here's what I get on my
8-core machine:
5302.9: index-pack default number of threads 167.70(546.19+12.00) 83.69(585.61+6.95) -50.1%
So that's a considerable improvement. And hardly surprising given the
repository structure. I used the script below to show the size of the
delta families, and the vk-master ones really dominate in size and
object number (the biggest is 50GB in one delta family).
I also ran my PERF_EXTRA tests on them to see if it behaved differently
as the threads increased. Nope:
5302.3: index-pack 0 threads 434.13(425.90+8.16)
5302.4: index-pack 1 threads 428.65(421.82+6.77)
5302.5: index-pack 2 threads 224.05(424.13+6.21)
5302.6: index-pack 4 threads 125.43(457.68+5.77)
5302.7: index-pack 8 threads 82.60(579.10+7.78)
5302.8: index-pack 16 threads 82.89(1147.82+9.66)
5302.9: index-pack default number of threads 83.91(576.92+8.52)
Still maxes out at the number of physical cores (not unexpected, but
that was the thing I was most curious about ;) ). I may run it on the
40-core machine, too. It's possible that with the new threading we're
able to do better going past 20-threads. I doubt it, because I think
it's mostly a function of Git's locking granularity, but worth checking.
-Peff
-- >8 --
#!/bin/sh
# script to output size, count, and filenames for each delta family
git rev-list --objects --all |
git cat-file --buffer \
--batch-check='%(objectname) %(deltabase) %(objectsize) %(rest)' |
perl -alne '
if ($F[1] =~ /[^0]/) {
push @{$children{$F[1]}}, $F[0];
} else {
push @bases, $F[0];
}
$size{$F[0]} = $F[2];
$name{$F[0]} = $F[3];
END {
sub add_to_component {
my ($oid, $data) = @_;
$data->{names}->{$name{$oid}}++;
$data->{size} += $size{$oid};
$data->{nr}++;
add_to_component($_, $data) for @{$children{$oid}};
}
for my $b (@bases) {
my $data = { size => 0, nr => 0, names => {} };
add_to_component($b, $data);
print join(" ",
$data->{size}, $data->{nr},
sort keys(%{$data->{names}})
), "\n";
}
}
' |
sort -rn
^ permalink raw reply
* Re: [PATCH v3 0/8] Maintenance II: prefetch, loose-objects, incremental-repack tasks
From: Junio C Hamano @ 2020-08-25 20:59 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, sandals, steadmon, jrnieder, peff, congdanhqx,
phillip.wood123, emilyshaffer, sluongng, jonathantanmy,
Derrick Stolee
In-Reply-To: <pull.696.v3.git.1598380599.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This series is based on v3 of part I (ds/maintenance-part-1) [2].
>
> This patch series contains 9 patches that were going to be part of v4 of
> ds/maintenance [1], but the discussion has gotten really long. To help, I'm
> splitting out the portions that create and test the 'maintenance' builtin
> from the additional tasks (prefetch, loose-objects, incremental-repack) that
> can be brought in later.
I gave it a quick look but the changes mostly are fallout from
renaming the options structure to maitenance_RUN_opts and loss of
midx verify while a task rewrites midx; iow, no significant change
that are likely to become controversial.
^ permalink raw reply
* Re: [PATCH 2/3] submodule: fix style in function definition
From: Junio C Hamano @ 2020-08-25 20:45 UTC (permalink / raw)
To: Shourya Shukla
Cc: git, christian.couder, kaartic.sivaraam, Johannes.Schindelin,
peff, liu.denton, Christian Couder
In-Reply-To: <20200825113020.71801-3-shouryashukla.oo@gmail.com>
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?
> Also, the warning printed in case of an unexpected file mode printed the
> mode in decimal. Print it in octal for enhanced readability.
I actually did check this side ;-) and am reasonably sure that there
aren't any other irrational choice of format specifiers.
Thanks.
^ permalink raw reply
* Re: Mismatched HEAD default behavior from git log
From: Jeff King @ 2020-08-25 19:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Bryan Turner, Git Users
In-Reply-To: <xmqq4koq1p28.fsf@gitster.c.googlers.com>
On Tue, Aug 25, 2020 at 12:51:59PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> As the intent for adding the "--stdin" option to any subcommand has
> >> always been "we may need to feed many many things, that may bust the
> >> command line length limit, hence we let you feed these things from
> >> the standard input, but otherwise there should be no change in
> >> behaviour or semantics", when the behaviour of command line and
> >> "--stdin" differ, it is a bug in the latter.
> >
> > Agreed. It also helps in this case that the command-line behavior is
> > sensible and the --stdin one is not. :)
> >
> > I think the solution is probably something like:
>
> 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. :)
-Peff
^ permalink raw reply
* Re: Mismatched HEAD default behavior from git log
From: Junio C Hamano @ 2020-08-25 19:51 UTC (permalink / raw)
To: Jeff King; +Cc: Bryan Turner, Git Users
In-Reply-To: <20200825194619.GB1419759@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> As the intent for adding the "--stdin" option to any subcommand has
>> always been "we may need to feed many many things, that may bust the
>> command line length limit, hence we let you feed these things from
>> the standard input, but otherwise there should be no change in
>> behaviour or semantics", when the behaviour of command line and
>> "--stdin" differ, it is a bug in the latter.
>
> Agreed. It also helps in this case that the command-line behavior is
> sensible and the --stdin one is not. :)
>
> I think the solution is probably something like:
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.
> diff --git a/revision.c b/revision.c
> index 96630e3186..f5bbefa091 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2099,12 +2099,13 @@ static void read_pathspec_from_stdin(struct strbuf *sb,
> strvec_push(prune, sb->buf);
> }
>
> -static void read_revisions_from_stdin(struct rev_info *revs,
> - struct strvec *prune)
> +static int read_revisions_from_stdin(struct rev_info *revs,
> + struct strvec *prune)
> {
> struct strbuf sb;
> int seen_dashdash = 0;
> int save_warning;
> + int got_rev_arg = 0;
>
> save_warning = warn_on_object_refname_ambiguity;
> warn_on_object_refname_ambiguity = 0;
> @@ -2124,12 +2125,14 @@ static void read_revisions_from_stdin(struct rev_info *revs,
> if (handle_revision_arg(sb.buf, revs, 0,
> REVARG_CANNOT_BE_FILENAME))
> die("bad revision '%s'", sb.buf);
> + got_rev_arg = 1;
> }
> if (seen_dashdash)
> read_pathspec_from_stdin(&sb, prune);
>
> strbuf_release(&sb);
> warn_on_object_refname_ambiguity = save_warning;
> + return got_rev_arg;
> }
>
> static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)
> @@ -2754,7 +2757,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
> }
> if (revs->read_from_stdin++)
> die("--stdin given twice?");
> - read_revisions_from_stdin(revs, &prune_data);
> + if (read_revisions_from_stdin(revs, &prune_data))
> + got_rev_arg = 1;
> continue;
> }
>
>
> Possibly it would make sense to push that flag into rev_info, though,
> and let handle_revision_arg() set it. That would fix this bug and
> prevent similar ones in other code paths (though we're not likely to get
> revisions from anywhere else, I suppose).
>
> -Peff
^ 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