* RE: git commit results in many lstat()s
From: Gumbel, Matthew K @ 2017-02-01 22:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <xmqq8tppo92x.fsf@gitster.mtv.corp.google.com>
"Junio C Hamano" <jch2355@gmail.com> writes:
> There probably are other things that can be optimized.
Yes, I think that when the user passes --only flag to git-commit, then git does not
need to call refresh_cache() in prepare_index() in builtin/commit.c.
I may experiment with that. Do you see any downside or negative side-effects?
Matt
^ permalink raw reply
* Re: [PATCH] doc: add note about ignoring --no-create-reflog
From: Junio C Hamano @ 2017-02-01 22:30 UTC (permalink / raw)
To: cornelius.weig; +Cc: git, peff
In-Reply-To: <20170201220727.18070-1-cornelius.weig@tngtech.com>
cornelius.weig@tngtech.com writes:
> From: Cornelius Weig <cornelius.weig@tngtech.com>
>
> The commands git-branch and git-tag accept a `--create-reflog` argument.
For the purpose of contrasting the above with "--no-create-reflog",
I find it a bit too weak to just say "accept". How about
The commands git-branch and git-tag accept a `--create-reflog`
option, and creates reflog even in a repository where
core.logallrefupdates configuration is set not to.
or something? After all "--no-create-reflog" is accepted. It just
does not override the configured (or unconfigured) default.
> On the other hand, the negated form `--no-create-reflog` is accepted as
> a valid option but has no effect. This silent noop may puzzle users.
True, very true.
> To communicate that this behavior is intentional, add a short note in
> the manuals for git-branch and git-tag.
Hmph. The added "short note" merely states the fact; it does not
hint that it is intentional or it explains what reasoning is behind
that intention.
> Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
> ---
>
> Notes:
> In a previous discussion (<xmqqbmunrwbf.fsf@gitster.mtv.corp.google.com>) it
> was found that git-branch and git-tag accept a "--no-create-reflog" argument,
> but it has no effect, does not produce a warning, and is undocumented.
Reading what Peff said in the thread, I do not think we actively
wanted this behaviour; we agreed that it is merely acceptable.
So perhaps s/this behaviour is intentional/this is known/ to weaken
the log message? That way, when somebody else who really cares
comes later and finds this commit that adds explicit notes to these
manual pages via "git blame", s/he would not be dissuaded from
making things better. Such an update may make it warn when
core.logallrefupdates is not set to false (and continue to ignore
the command line option), or it may make the command line option
actually override the configured default.
With such an update to the log message, I think the patch looks
good.
Thanks.
> Documentation/git-branch.txt | 1 +
> Documentation/git-tag.txt | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 1fae4ee..fca3754 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -91,6 +91,7 @@ OPTIONS
> based sha1 expressions such as "<branchname>@\{yesterday}".
> Note that in non-bare repositories, reflogs are usually
> enabled by default by the `core.logallrefupdates` config option.
> + The negated form `--no-create-reflog` is silently ignored.
>
> -f::
> --force::
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 5b2288c..b0b933e 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -152,6 +152,7 @@ This option is only applicable when listing tags without annotation lines.
> --create-reflog::
> Create a reflog for the tag. To globally enable reflogs for tags, see
> `core.logAllRefUpdates` in linkgit:git-config[1].
> + The negated form `--no-create-reflog` is silently ignored.
>
> <tagname>::
> The name of the tag to create, delete, or describe.
^ permalink raw reply
* Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
From: Junio C Hamano @ 2017-02-01 22:33 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer, Jeff King
In-Reply-To: <alpine.DEB.2.20.1702012319460.3496@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> That leaves the "putty" case in handle_ssh_variant(), does it not? Was it
> not your specific objection that that is the case?
Yup, you can remove that while you reroll.
> No worries, I will let this simmer for a while. Your fixup has a lot of
> duplicated code (so much for maintainability as an important goal... ;-))
> and I will have to think about it. My immediate thinking is to *not*
> duplicate code,...
You need to realize that the namespaces of the configuration and the
command names are distinct. There is no code duplication.
^ permalink raw reply
* Re: [PATCH] doc: add note about ignoring --no-create-reflog
From: Jeff King @ 2017-02-01 22:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: cornelius.weig, git
In-Reply-To: <xmqq4m0do86p.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 01, 2017 at 02:30:38PM -0800, Junio C Hamano wrote:
> > Notes:
> > In a previous discussion (<xmqqbmunrwbf.fsf@gitster.mtv.corp.google.com>) it
> > was found that git-branch and git-tag accept a "--no-create-reflog" argument,
> > but it has no effect, does not produce a warning, and is undocumented.
>
> Reading what Peff said in the thread, I do not think we actively
> wanted this behaviour; we agreed that it is merely acceptable.
>
> So perhaps s/this behaviour is intentional/this is known/ to weaken
> the log message? That way, when somebody else who really cares
> comes later and finds this commit that adds explicit notes to these
> manual pages via "git blame", s/he would not be dissuaded from
> making things better. Such an update may make it warn when
> core.logallrefupdates is not set to false (and continue to ignore
> the command line option), or it may make the command line option
> actually override the configured default.
Yeah, I'd consider it more of a "known bug" or "known limitation" than
anything.
Those can go in a separate section, but they're probably more likely to
be read when supplied next to the actual option.
> With such an update to the log message, I think the patch looks
> good.
> [...]
> > @@ -91,6 +91,7 @@ OPTIONS
> > based sha1 expressions such as "<branchname>@\{yesterday}".
> > Note that in non-bare repositories, reflogs are usually
> > enabled by default by the `core.logallrefupdates` config option.
> > + The negated form `--no-create-reflog` is silently ignored.
This might be nitpicking, but it's _not_ ignored. It still negates an
earlier "--create-reflog". It is only that it does not override the
decision to create a reflog caused by the setting of
core.logallrefupdates.
-Peff
^ permalink raw reply
* Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
From: Johannes Schindelin @ 2017-02-01 22:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Segev Finer, Jeff King
In-Reply-To: <xmqqzii5mthp.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 1 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > That leaves the "putty" case in handle_ssh_variant(), does it not? Was it
> > not your specific objection that that is the case?
>
> Yup, you can remove that while you reroll.
>
> > No worries, I will let this simmer for a while. Your fixup has a lot of
> > duplicated code (so much for maintainability as an important goal... ;-))
> > and I will have to think about it. My immediate thinking is to *not*
> > duplicate code,...
>
> You need to realize that the namespaces of the configuration and the
> command names are distinct. There is no code duplication.
Since your 2/4 did away with the "plink" and "tortoiseplink" values in
favor of "port_option" and "batch_option", there is a duplication of logic
which I tried to undo in v3 and which you reintroduce in your fixup.
Maybe that refactoring was not so smart to begin with, and we should have
instead modified the code to use an enum instead and keep the original
conditionals instead of setting a port_option and a batch_option
explicitly.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
From: Junio C Hamano @ 2017-02-01 22:43 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer, Jeff King
In-Reply-To: <xmqqzii5mthp.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> That leaves the "putty" case in handle_ssh_variant(), does it not? Was it
>> not your specific objection that that is the case?
>
> Yup, you can remove that while you reroll.
>
>> No worries, I will let this simmer for a while. Your fixup has a lot of
>> duplicated code (so much for maintainability as an important goal... ;-))
>> and I will have to think about it. My immediate thinking is to *not*
>> duplicate code,...
>
> You need to realize that the namespaces of the configuration and the
> command names are distinct. There is no code duplication.
To explain this a bit, there is no reason why allowed values for
SSH_VARIANT must be "putty" and "tortoiseplink". An alternative
design could be "port_option=-p,needs_batch=yes" and it may be more
logical and futureproof if a variant of tortoiseplink decides to use
"-p" instead of "-P" and still require "-batch".
Prematurely attempting to share code, only because the current
vocabularies for two distinct concepts happen to overlap, is not
de-duplicating the code for maintainability. It is adding
unnecessary work other people need to do in the future when they
want to extend the system.
^ permalink raw reply
* Re: [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND
From: Junio C Hamano @ 2017-02-01 22:55 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King
In-Reply-To: <alpine.DEB.2.20.1702012311440.3496@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Compared to the correctness issue, these are much harder to spot by
>> the submitter alone, who focused so intensely to get his code
>> "correct". The review process is of greater value to spot these
>> issues.
>
> We will never agree on this.
That's too bad.
> From my perspective, design, explanation and maintainability are a
> consequence of making it easy for reviewers to spot where the code is
> incorrect.
>
> And correctness is not covered by "the submitter tested this". Correctness
> includes all the corner cases, where the "many eyes make bugs shallow"
> really shines.
>
> I'd rather have reviewers find bugs than users.
I'd rather have submitters find bugs than reviewers.
> I will *never* be a fan of a review process that pushes correctness to a
> back seat (yes, it is much harder than spotting typos or lines longer than
> 80 columns per row, but the ultimate goal is to deliver value to the end
> user, not to make life easy for the maintainer).
Did I ever say correctness is pushed to a back seat?
I said that it is easier to spot correctness issues for you as a
submitter than other kinds of issues without outside help (and
implied that if you are a diligent contributor, you should aim for,
and you should be able to achieve, a patch series where correctness
issues do not need to be pointed out). But other higher level
issues are harder for any submitter to spot (regardless of
experience and competence of the submitter), because one gets so
married to one's own code, design and worldview. And that is why
"review is primarily to spot bugs" can never be a correct viewpoint.
A reviewee needs to be prepared to accept review comments on higher
level issues, even more readily than comments on correctness issues,
because it is too easy to be constrained by early decisions one has
already made while preparing a patch series and become blind to
bigger picture after staring one's own new code for number of hours.
Higher level issues can be more easily spotted by reviewers, whose
eyes are still fresh to the series.
^ permalink raw reply
* Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
From: Johannes Schindelin @ 2017-02-01 23:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Segev Finer, Jeff King
In-Reply-To: <xmqqvastmt09.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 1 Feb 2017, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> >> That leaves the "putty" case in handle_ssh_variant(), does it not? Was it
> >> not your specific objection that that is the case?
> >
> > Yup, you can remove that while you reroll.
> >
> >> No worries, I will let this simmer for a while. Your fixup has a lot of
> >> duplicated code (so much for maintainability as an important goal... ;-))
> >> and I will have to think about it. My immediate thinking is to *not*
> >> duplicate code,...
> >
> > You need to realize that the namespaces of the configuration and the
> > command names are distinct. There is no code duplication.
>
> To explain this a bit, there is no reason why allowed values for
> SSH_VARIANT must be "putty" and "tortoiseplink". An alternative
> design could be "port_option=-p,needs_batch=yes" and it may be more
> logical and futureproof if a variant of tortoiseplink decides to use
> "-p" instead of "-P" and still require "-batch".
>
> Prematurely attempting to share code, only because the current
> vocabularies for two distinct concepts happen to overlap, is not
> de-duplicating the code for maintainability. It is adding
> unnecessary work other people need to do in the future when they
> want to extend the system.
Except, of course, that your hypothetical port_option and needs_batch
settings would be handled at a different point altogether.
I sense very strongly that this discussion has taken a very emotional
turn, which is detrimental to the quality. So let's take a break here.
Ciao,
Johannes
^ permalink raw reply
* [PATCH 1/2] doc: add doc for git-push --recurse-submodules=only
From: cornelius.weig @ 2017-02-01 23:07 UTC (permalink / raw)
To: git; +Cc: bmwill, sbeller, Cornelius Weig
From: Cornelius Weig <cornelius.weig@tngtech.com>
Add documentation for the `--recurse-submodules=only` option of
git-push. The feature was added in commit 225e8bf (add option to
push only submodules).
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
Notes:
This feature is already in 'next' but was undocumented. Unless somebody reads
the release notes, there is no way of knowing about it.
Documentation/git-push.txt | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 8eefabd..1624a35 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -272,7 +272,7 @@ origin +master` to force a push to the `master` branch). See the
standard error stream is not directed to a terminal.
--no-recurse-submodules::
---recurse-submodules=check|on-demand|no::
+--recurse-submodules=check|on-demand|only|no::
May be used to make sure all submodule commits used by the
revisions to be pushed are available on a remote-tracking branch.
If 'check' is used Git will verify that all submodule commits that
@@ -280,11 +280,12 @@ origin +master` to force a push to the `master` branch). See the
remote of the submodule. If any commits are missing the push will
be aborted and exit with non-zero status. If 'on-demand' is used
all submodules that changed in the revisions to be pushed will be
- pushed. If on-demand was not able to push all necessary revisions
- it will also be aborted and exit with non-zero status. A value of
- 'no' or using `--no-recurse-submodules` can be used to override the
- push.recurseSubmodules configuration variable when no submodule
- recursion is required.
+ pushed. If on-demand was not able to push all necessary revisions it will
+ also be aborted and exit with non-zero status. If 'only' is used all
+ submodules will be recursively pushed while the superproject is left
+ unpushed. A value of 'no' or using `--no-recurse-submodules` can be used
+ to override the push.recurseSubmodules configuration variable when no
+ submodule recursion is required.
--[no-]verify::
Toggle the pre-push hook (see linkgit:githooks[5]). The
--
2.10.2
^ permalink raw reply related
* [PATCH 2/2] completion: add completion for --recurse-submodules=only
From: cornelius.weig @ 2017-02-01 23:07 UTC (permalink / raw)
To: git; +Cc: bmwill, sbeller, Cornelius Weig
In-Reply-To: <20170201230753.19462-1-cornelius.weig@tngtech.com>
From: Cornelius Weig <cornelius.weig@tngtech.com>
Command completion for 'git-push --recurse-submodules' already knows to
complete some modes. However, the recently added mode 'only' is missing.
Adding 'only' to the recognized modes completes the list of non-trivial
modes.
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
contrib/completion/git-completion.bash | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ff7072a..fe3b0f8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1675,7 +1675,7 @@ _git_pull ()
__git_complete_remote_or_refspec
}
-__git_push_recurse_submodules="check on-demand"
+__git_push_recurse_submodules="check on-demand only"
__git_complete_force_with_lease ()
{
--
2.10.2
^ permalink raw reply related
* Re: [PATCH] doc: add note about ignoring --no-create-reflog
From: Junio C Hamano @ 2017-02-01 23:11 UTC (permalink / raw)
To: Jeff King; +Cc: cornelius.weig, git
In-Reply-To: <20170201223520.b4er3av67ev5m3ls@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This might be nitpicking, but it's _not_ ignored. It still negates an
> earlier "--create-reflog". It is only that it does not override the
> decision to create a reflog caused by the setting of
> core.logallrefupdates.
OK, rolling them all into one, how about this as an amend?
-- >8 --
From: Cornelius Weig <cornelius.weig@tngtech.com>
Date: Wed, 1 Feb 2017 23:07:27 +0100
Subject: [PATCH] doc: add note about ignoring '--no-create-reflog'
The commands git-branch and git-tag accept the '--create-reflog'
option, and create reflog even when core.logallrefupdates
configuration is explicitly set not to.
On the other hand, the negated form '--no-create-reflog' is accepted
as a valid option but has no effect (other than overriding an
earlier '--create-reflog' on the command line). This silent noop may
puzzle users. To communicate that this is a known limitation, add a
short note in the manuals for git-branch and git-tag.
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/git-branch.txt | 3 +++
Documentation/git-tag.txt | 3 +++
2 files changed, 6 insertions(+)
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 5516a47b54..102e426fd8 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -91,6 +91,9 @@ OPTIONS
based sha1 expressions such as "<branchname>@\{yesterday}".
Note that in non-bare repositories, reflogs are usually
enabled by default by the `core.logallrefupdates` config option.
+ The negated form `--no-create-reflog` does not override the
+ default, even though it overrides `--create-reflog` that appears
+ earlier on the command line.
-f::
--force::
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 2ac25a9bb3..fd7eeae075 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -152,6 +152,9 @@ This option is only applicable when listing tags without annotation lines.
--create-reflog::
Create a reflog for the tag. To globally enable reflogs for tags, see
`core.logAllRefUpdates` in linkgit:git-config[1].
+ The negated form `--no-create-reflog` does not override the
+ default, even though it overrides `--create-reflog` that appears
+ earlier on the command line.
<tagname>::
The name of the tag to create, delete, or describe.
--
2.11.0-800-g4bf73cb6b2
^ permalink raw reply related
* Re: [PATCH 1/2] doc: add doc for git-push --recurse-submodules=only
From: Junio C Hamano @ 2017-02-01 23:16 UTC (permalink / raw)
To: cornelius.weig; +Cc: git, bmwill, sbeller
In-Reply-To: <20170201230753.19462-1-cornelius.weig@tngtech.com>
cornelius.weig@tngtech.com writes:
> From: Cornelius Weig <cornelius.weig@tngtech.com>
>
> Add documentation for the `--recurse-submodules=only` option of
> git-push. The feature was added in commit 225e8bf (add option to
> push only submodules).
>
> Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
> ---
>
> Notes:
> This feature is already in 'next' but was undocumented. Unless somebody reads
> the release notes, there is no way of knowing about it.
Good eyes; the topic bw/push-submodule-only is already in 'master'.
Looks good to me; Brandon?
>
> Documentation/git-push.txt | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 8eefabd..1624a35 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -272,7 +272,7 @@ origin +master` to force a push to the `master` branch). See the
> standard error stream is not directed to a terminal.
>
> --no-recurse-submodules::
> ---recurse-submodules=check|on-demand|no::
> +--recurse-submodules=check|on-demand|only|no::
> May be used to make sure all submodule commits used by the
> revisions to be pushed are available on a remote-tracking branch.
> If 'check' is used Git will verify that all submodule commits that
> @@ -280,11 +280,12 @@ origin +master` to force a push to the `master` branch). See the
> remote of the submodule. If any commits are missing the push will
> be aborted and exit with non-zero status. If 'on-demand' is used
> all submodules that changed in the revisions to be pushed will be
> - pushed. If on-demand was not able to push all necessary revisions
> - it will also be aborted and exit with non-zero status. A value of
> - 'no' or using `--no-recurse-submodules` can be used to override the
> - push.recurseSubmodules configuration variable when no submodule
> - recursion is required.
> + pushed. If on-demand was not able to push all necessary revisions it will
> + also be aborted and exit with non-zero status. If 'only' is used all
> + submodules will be recursively pushed while the superproject is left
> + unpushed. A value of 'no' or using `--no-recurse-submodules` can be used
> + to override the push.recurseSubmodules configuration variable when no
> + submodule recursion is required.
>
> --[no-]verify::
> Toggle the pre-push hook (see linkgit:githooks[5]). The
^ permalink raw reply
* Re: [PATCH] doc: add note about ignoring --no-create-reflog
From: Cornelius Weig @ 2017-02-01 23:19 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: git
In-Reply-To: <xmqqmve5mrpe.fsf@gitster.mtv.corp.google.com>
On 02/02/2017 12:11 AM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> This might be nitpicking, but it's _not_ ignored. It still negates an
>> earlier "--create-reflog". It is only that it does not override the
>> decision to create a reflog caused by the setting of
>> core.logallrefupdates.
This corner case is quite important. Glad you thought about it!
> -- >8 --
> From: Cornelius Weig <cornelius.weig@tngtech.com>
> Date: Wed, 1 Feb 2017 23:07:27 +0100
> Subject: [PATCH] doc: add note about ignoring '--no-create-reflog'
>
> The commands git-branch and git-tag accept the '--create-reflog'
> option, and create reflog even when core.logallrefupdates
> configuration is explicitly set not to.
>
> On the other hand, the negated form '--no-create-reflog' is accepted
> as a valid option but has no effect (other than overriding an
> earlier '--create-reflog' on the command line). This silent noop may
> puzzle users. To communicate that this is a known limitation, add a
> short note in the manuals for git-branch and git-tag.
>
> Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Documentation/git-branch.txt | 3 +++
> Documentation/git-tag.txt | 3 +++
> 2 files changed, 6 insertions(+)
>
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 5516a47b54..102e426fd8 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -91,6 +91,9 @@ OPTIONS
> based sha1 expressions such as "<branchname>@\{yesterday}".
> Note that in non-bare repositories, reflogs are usually
> enabled by default by the `core.logallrefupdates` config option.
> + The negated form `--no-create-reflog` does not override the
> + default, even though it overrides `--create-reflog` that appears
> + earlier on the command line.
>
> -f::
> --force::
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 2ac25a9bb3..fd7eeae075 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -152,6 +152,9 @@ This option is only applicable when listing tags without annotation lines.
> --create-reflog::
> Create a reflog for the tag. To globally enable reflogs for tags, see
> `core.logAllRefUpdates` in linkgit:git-config[1].
> + The negated form `--no-create-reflog` does not override the
> + default, even though it overrides `--create-reflog` that appears
> + earlier on the command line.
>
> <tagname>::
> The name of the tag to create, delete, or describe.
>
Your amended version is quite concise and says everything there is to
say. Thanks
^ permalink raw reply
* Re: [PATCH] doc: add note about ignoring --no-create-reflog
From: Jeff King @ 2017-02-01 23:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: cornelius.weig, git
In-Reply-To: <xmqqmve5mrpe.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 01, 2017 at 03:11:57PM -0800, Junio C Hamano wrote:
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 5516a47b54..102e426fd8 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -91,6 +91,9 @@ OPTIONS
> based sha1 expressions such as "<branchname>@\{yesterday}".
> Note that in non-bare repositories, reflogs are usually
> enabled by default by the `core.logallrefupdates` config option.
> + The negated form `--no-create-reflog` does not override the
> + default, even though it overrides `--create-reflog` that appears
> + earlier on the command line.
Should this perhaps say "currently" or "this may change in the future",
so that people (including those who might want to fix it later) know
that it's a limitation and not intentional?
I'd also probably say it a little shorter, like:
The negated form `--no-create-reflog` only overrides an earlier
`--create-reflog`, but currently does not negate the setting of
`core.logallrefupdates`.
I guess that really isn't much shorter (I wondered if you could cut out
the "overrides --create-reflog" part, since that is the normal and
expected behavior, but I had trouble wording it to do so).
-Peff
^ permalink raw reply
* Re: [PATCH] doc: add note about ignoring --no-create-reflog
From: Cornelius Weig @ 2017-02-01 23:23 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
In-Reply-To: <20170201231939.hxhhujpzyb2cqq7a@sigill.intra.peff.net>
> The negated form `--no-create-reflog` only overrides an earlier
> `--create-reflog`, but currently does not negate the setting of
> `core.logallrefupdates`.
Even better than Junio's version. I especially like that it mentions
where the default setting comes from.
^ permalink raw reply
* Re: [PATCH] merge-recursive: make "CONFLICT (rename/delete)" message show both paths
From: Matt McCutchen @ 2017-02-01 23:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqh94dockc.fsf@gitster.mtv.corp.google.com>
On Wed, 2017-02-01 at 12:56 -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Matt McCutchen <matt@mattmccutchen.net> writes:
> > ...
> > > Please check that my refactoring is indeed correct! All the
> > > existing tests pass
> > > for me, but the existing test coverage of these conflict messages
> > > looks poor.
> >
> > This unfortunately is doing a bit too many things at once from that
> > point of view. I need to reserve a solid quiet 20-minutes without
> > distraction to check it, which I am hoping to do tonight.
>
> Let me make sure if I understood your changes correctly:
>
> * conflict_rename_delete() knew which one is renamed and which one
> is deleted (even though the deleted one was called "other"), but
> because in the original code handle_change_delete() wants to
> always see tree A first and then tree B in its parameter list,
> the original code swapped a/b before calling it. In the original
> code, handle_change_delete() needed to figure out which one is
> the deleted one by looking at a_oid or b_oid.
>
> * In the updated code, the knowledge of which branch survives and
> which branch is deleted is passed from the caller to
> handle_change_delete(), which no longer needs to figure out by
> looking at a_oid/b_oid. The updated API only passes the oid for
> surviving branch (as deleted one would have been 0{40} anyway).
>
> * In the updated code, handle_change_delete() is told the names of
> both branches (the one that survives and the other that was
> deleted). It no longer has to switch between o->branch[12]
> depending on the NULLness of a_oid/b_oid; it knows both names and
> which one is which.
>
> * handle_modify_delete() also calls handle_change_delete(). Unlike
> conflict_rename_delete(), it is not told by its caller which
> branch keeps the path and which branch deletes the path, and
> instead relies on handle_change_delete() to figure it out.
> Because of the above change to the API, now it needs to sort it
> out before calling handle_change_delete().
>
> It all makes sense to me.
>
> The single call to update_file() that appears near the end of
> handle_change_delete() in the updated code corresponds to calls to
> the same function in 3 among 4 codepaths in the function in the
> original code. It is a bit tricky to reason about, though.
>
> In the original code, update_file() was omitted when we didn't have
> to come up with a unique alternate filename and the one that is left
> is a_oid (i.e. our side). The way to tell if we did not come up
> with a unique alternate filename used to be to see the "renamed"
> variable but now it is the NULL-ness of "alt_path".
"alt_path" is the same variable that used to be "renamed". I just
renamed it to be less confusing.
> And the way to
> tell if the side that is left is ours, we check to see o->branch1
> is the change_branch, not delete_branch.
>
> So the condition to guard the call to update_file() also looks
> correct to me.
All of the above matches my understanding. Would it have saved you
time if I had included some of this explanation in the patch "cover
letter"?
Matt
^ permalink raw reply
* Re: [PATCH] doc: add note about ignoring --no-create-reflog
From: Junio C Hamano @ 2017-02-01 23:27 UTC (permalink / raw)
To: Jeff King; +Cc: cornelius.weig, git
In-Reply-To: <20170201231939.hxhhujpzyb2cqq7a@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Should this perhaps say "currently" or "this may change in the future",
> so that people (including those who might want to fix it later) know
> that it's a limitation and not intentional?
Good point.
> I'd also probably say it a little shorter, like:
>
> The negated form `--no-create-reflog` only overrides an earlier
> `--create-reflog`, but currently does not negate the setting of
> `core.logallrefupdates`.
>
> I guess that really isn't much shorter (I wondered if you could cut out
> the "overrides --create-reflog" part, since that is the normal and
> expected behavior, but I had trouble wording it to do so).
I had the same trouble wording. Another thing I noticed was that I
deliberately left it vague what "default" this does not override,
because it appears to me that those who do not set logallrefupdates
will get the compiled-in default and that is also not overriden.
IOW, "does not negate the setting of core.logallrefupdates" will
open us to reports "I do not have the configuration set, but I still
get reflog even when --no-create-reflog is given".
The negated form `--no-create-reflog` currently does not negate
the default; it overrides an earlier `--create-reflog`, though.
perhaps?
^ permalink raw reply
* Re: [PATCH] merge-recursive: make "CONFLICT (rename/delete)" message show both paths
From: Junio C Hamano @ 2017-02-01 23:28 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git
In-Reply-To: <1485991441.28767.2.camel@mattmccutchen.net>
Matt McCutchen <matt@mattmccutchen.net> writes:
> On Wed, 2017-02-01 at 12:56 -0800, Junio C Hamano wrote:
>
>> Let me make sure if I understood your changes correctly:
>> ...
>> So the condition to guard the call to update_file() also looks
>> correct to me.
>
> All of the above matches my understanding. Would it have saved you
> time if I had included some of this explanation in the patch "cover
> letter"?
The fact that I arrived at the same understanding by reading the
change without peeking at such a cheat-sheet gives me more peace of
mind ;-)
Thanks.
^ permalink raw reply
* Re: [PATCH] doc: add note about ignoring --no-create-reflog
From: Jeff King @ 2017-02-01 23:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: cornelius.weig, git
In-Reply-To: <xmqqefzhmr02.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 01, 2017 at 03:27:09PM -0800, Junio C Hamano wrote:
> I had the same trouble wording. Another thing I noticed was that I
> deliberately left it vague what "default" this does not override,
> because it appears to me that those who do not set logallrefupdates
> will get the compiled-in default and that is also not overriden.
>
> IOW, "does not negate the setting of core.logallrefupdates" will
> open us to reports "I do not have the configuration set, but I still
> get reflog even when --no-create-reflog is given".
>
> The negated form `--no-create-reflog` currently does not negate
> the default; it overrides an earlier `--create-reflog`, though.
>
> perhaps?
True. I thought the default was "off", and that we merely set the config
when initializing a repo. But looking again, it really is checking
is_bare_repository() at runtime.
I still think it is OK to mention, as the description of
core.logallrefupdates is where we document the behavior and the
defaults. So even with "I do not have it set", that is still the key to
find more information.
I do not care that strongly either way, though. This is a minor issue,
and I suspect just about any note would be helpful.
-Peff
^ permalink raw reply
* Re: git commit results in many lstat()s
From: Junio C Hamano @ 2017-02-01 23:50 UTC (permalink / raw)
To: Gumbel, Matthew K; +Cc: git@vger.kernel.org
In-Reply-To: <DA0A42D68346B1469147552440A645039A9C57D6@ORSMSX115.amr.corp.intel.com>
"Gumbel, Matthew K" <matthew.k.gumbel@intel.com> writes:
> "Junio C Hamano" <jch2355@gmail.com> writes:
>> There probably are other things that can be optimized.
>
> Yes, I think that when the user passes --only flag to git-commit, then git does not
> need to call refresh_cache() in prepare_index() in builtin/commit.c.
>
> I may experiment with that. Do you see any downside or negative side-effects?
There may be other fallouts, but one that immediately comes to mind
is that it may break pre-commit hook.
When we get "--only", we prepare an temporary index to create the
commit out of, and give it to the pre-commit hook. The hook expects
that the cached stat information is up-to-date, iow, it does not
have to do 'update-index --refresh' before using plumbing commands
like "diff-index" to do its own inspection of the working tree.
^ permalink raw reply
* Re: [PATCH] doc: add note about ignoring --no-create-reflog
From: Junio C Hamano @ 2017-02-01 23:54 UTC (permalink / raw)
To: Jeff King; +Cc: cornelius.weig, git
In-Reply-To: <20170201233202.p462dggidiiyx6s6@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Feb 01, 2017 at 03:27:09PM -0800, Junio C Hamano wrote:
>
>> I had the same trouble wording. Another thing I noticed was that I
>> deliberately left it vague what "default" this does not override,
>> because it appears to me that those who do not set logallrefupdates
>> will get the compiled-in default and that is also not overriden.
>>
>> IOW, "does not negate the setting of core.logallrefupdates" will
>> open us to reports "I do not have the configuration set, but I still
>> get reflog even when --no-create-reflog is given".
>>
>> The negated form `--no-create-reflog` currently does not negate
>> the default; it overrides an earlier `--create-reflog`, though.
>>
>> perhaps?
>
> True. I thought the default was "off", and that we merely set the config
> when initializing a repo. But looking again, it really is checking
> is_bare_repository() at runtime.
>
> I still think it is OK to mention, as the description of
> core.logallrefupdates is where we document the behavior and the
> defaults. So even with "I do not have it set", that is still the key to
> find more information.
OK, let's take yours as the final and merge it down to 'next'
soonish.
^ permalink raw reply
* RE: git commit results in many lstat()s
From: Gumbel, Matthew K @ 2017-02-02 0:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <xmqq60ktmpwl.fsf@gitster.mtv.corp.google.com>
"Junio C Hamano <mailto:jch2355@gmail.com> writes:
"Gumbel, Matthew K" <matthew.k.gumbel@intel.com> writes:
>> Yes, I think that when the user passes --only flag to git-commit, then git does not
>> need to call refresh_cache() in prepare_index() in builtin/commit.c.
>>
>> I may experiment with that. Do you see any downside or negative side-effects?
> There may be other fallouts, but one that immediately comes to mind
> is that it may break pre-commit hook.
If pre-commit hook exists, we can fall-back to original behavior and call
refresh_cache(). Many repos will not have pre-commit hook and can
benefit from the speedup.
I'm testing such a change locally. Git test suite seems to be running for quite
a while. Do you know any way to run it in parallel or otherwise speed it
up?
Thanks,
Matt
^ permalink raw reply
* Re: git commit results in many lstat()s
From: brian m. carlson @ 2017-02-02 0:25 UTC (permalink / raw)
To: Gumbel, Matthew K; +Cc: Junio C Hamano, git@vger.kernel.org
In-Reply-To: <DA0A42D68346B1469147552440A645039A9C5929@ORSMSX115.amr.corp.intel.com>
[-- Attachment #1: Type: text/plain, Size: 631 bytes --]
On Thu, Feb 02, 2017 at 12:14:30AM +0000, Gumbel, Matthew K wrote:
> I'm testing such a change locally. Git test suite seems to be running for quite
> a while. Do you know any way to run it in parallel or otherwise speed it
> up?
I usually do something like the following:
make -j3 all && (cd t && GIT_PROVE_OPTS=-j3 make prove)
This, of course, requires that you have Perl's prove installed, which
has been part of core Perl since 5.10.1.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [PATCH 2/7] completion: add subcommand completion for rerere
From: SZEDER Gábor @ 2017-02-02 0:57 UTC (permalink / raw)
To: Cornelius Weig
Cc: SZEDER Gábor, bitte.keine.werbung.einwerfen, thomas.braun,
john, git
In-Reply-To: <20170122225724.19360-3-cornelius.weig@tngtech.com>
> Managing recorded resolutions requires command-line usage of git-rerere.
> Added subcommand completion for rerere and path completion for its
> subcommand forget.
> ---
> contrib/completion/git-completion.bash | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c54a557..8329f09 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2401,6 +2401,17 @@ _git_replace ()
> __gitcomp_nl "$(__git_refs)"
> }
>
> +_git_rerere ()
> +{
> + local subcommands="clear forget diff remaining status gc"
> + local subcommand="$(__git_find_on_cmdline "$subcommands")"
> + if test -z "$subcommand"
> + then
> + __gitcomp "$subcommands"
> + return
> + fi
> +}
> +
> _git_reset ()
> {
> __git_has_doubledash && return
> --
> 2.10.2
You didn't add 'rerere' to the list of porcelain commands, i.e. it
won't be listed after 'git <TAB><TAB>'. I'm not saying it should be
listed there, because I can't decide whether 'rerere' is considered
porcelain or plumbing... Just wanted to make sure that this omission
was intentional.
^ permalink raw reply
* Re: [PATCH 6/7] completion: teach remote subcommands option completion
From: SZEDER Gábor @ 2017-02-02 1:37 UTC (permalink / raw)
To: Cornelius Weig
Cc: SZEDER Gábor, bitte.keine.werbung.einwerfen, thomas.braun,
john, git
In-Reply-To: <20170122225724.19360-7-cornelius.weig@tngtech.com>
> Git-remote needs to complete remote names, its subcommands, and options
> thereof. In addition to the existing subcommand and remote name
> completion, do also complete the options
>
> - add: --track --master --fetch --tags --no-tags --mirror=
Oh, '--track' and '--master' are not even in the manpage or in 'git
remote -h', I could only find them after looking at the source code...
Good eyes :)
> - set-url: --push --add --delete
> - get-url: --push --all
> - prune: --dry-run
> ---
> contrib/completion/git-completion.bash | 36 +++++++++++++++++++++++++++-------
> 1 file changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index e76cbd7..0e09519 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2384,24 +2384,46 @@ _git_config ()
>
> _git_remote ()
> {
> - local subcommands="add rename remove set-head set-branches set-url show prune update"
> + local subcommands="
> + add rename remove set-head set-branches
> + get-url set-url show prune update
> + "
> local subcommand="$(__git_find_on_cmdline "$subcommands")"
> if [ -z "$subcommand" ]; then
> - __gitcomp "$subcommands"
> + case "$cur" in
> + --*)
> + __gitcomp "--verbose"
> + ;;
> + *)
> + __gitcomp "$subcommands"
> + ;;
> + esac
> return
> fi
>
> - case "$subcommand" in
> - rename|remove|set-url|show|prune)
> - __gitcomp_nl "$(__git_remotes)"
> + case "$subcommand,$cur" in
> + add,--*)
> + __gitcomp "--track --master --fetch --tags --no-tags --mirror="
> ;;
> - set-head|set-branches)
> + add,*)
> + ;;
> + set-head,*|set-branches,*)
The 'set-head' subcommand has '--auto' and '--delete' options, and
'set-branches' has '--add'.
> __git_complete_remote_or_refspec
> ;;
> - update)
> + update,*)
The 'update' subcommand has a '--prune' option.
Otherwise it all looks good.
> __gitcomp "$(__git_get_config_variables "remotes")"
> ;;
> + set-url,--*)
> + __gitcomp "--push --add --delete"
> + ;;
> + get-url,--*)
> + __gitcomp "--push --all"
> + ;;
> + prune,--*)
> + __gitcomp "--dry-run"
> + ;;
> *)
> + __gitcomp_nl "$(__git_remotes)"
> ;;
> esac
> }
> --
> 2.10.2
^ 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