* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Jeff King @ 2016-10-31 13:56 UTC (permalink / raw)
To: Linus Torvalds
Cc: Johannes Schindelin, Junio C Hamano, Git Mailing List,
Lars Schneider, Eric Wong
In-Reply-To: <CA+55aFw93vkraxBvFCXFSYJqn836tXW+OCOFuToN+HaxTcJ7cg@mail.gmail.com>
On Fri, Oct 28, 2016 at 09:13:41AM -0700, Linus Torvalds wrote:
> On Fri, Oct 28, 2016 at 4:11 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > You guys. I mean: You guys! You sure make my life hard. A brief look at
> > mingw.h could have answered your implicit question:
>
> So here's what you guys should do:
>
> - leave O_NOATIME damn well alone. It works. It has worked for 10+
> years. Stop arguing against it, people who do.
For some definition of worked, perhaps.
If you set a probe on touch_atime() in the kernel (which is called for
every attempt to smudge the atime, regardless of mount options, but is
skipped when the descriptor was opened with O_NOATIME), you can see the
impact. Here's a command I picked because it reads a lot of objects (run
on my git.git clone):
$ perf stat -e probe:touch-atime git log -Sfoo >/dev/null
And the probe:touch_atime counts before (stock git) and after (a patch
to drop O_NOATIME):
before: 22,235
after: 22,362
So that's only half a percent difference. And it's on a reasonably messy
clone that is partway to triggering an auto-repack:
$ git count-objects -v
count: 6167
size: 61128
in-pack: 275773
packs: 18
size-pack: 86857
prune-packable: 25
garbage: 0
size-garbage: 0
Running "git gc" drops the probe count to 21,733.
It makes a bigger difference for some commands (it's more like 10% for
git-status). And smaller for others ("git log -p" triggers it over
100,000 times).
One thing missing in that count is how many of those calls would have
resulted in an actual disk write. Looking at strace, most of the
filesystem activity is opening .gitattributes files, and we end up
opening the same ones repeatedly (e.g., t/.gitattributes in git.git).
Multiple hits for a given inode in the same second get coalesced into at
most a single disk write.
So I guess it's possible that it produces a noticeable effect in some
cases, but I'm still somewhat doubtful. And actually repacking your
repository had a greater effect in every case I measured (in addition to
providing other speedups).
Like I said, I'm OK keeping O_NOATIME. It's just not that much code. But
if you really care about the issue of dirtying inodes via atime, you
should look into vastly increasing our use of O_NOATIME. Or possibly
looking at caching more in the attribute code.
-Peff
^ permalink raw reply
* Re: Is the entire working copy “at one branch”?
From: Stefan Monov @ 2016-10-31 10:41 UTC (permalink / raw)
To: Alexei Lozovsky; +Cc: git
In-Reply-To: <CALhvvbYE6Tt3eByDVMB3a4t=nm3dScVZSea0Z1SsKVgwFSiQ-w@mail.gmail.com>
Thanks Alexei!
On Sat, Oct 29, 2016 at 12:47 PM, Alexei Lozovsky <a.lozovsky@gmail.com> wrote:
> Hi Stefan,
>
> Generally with git, your entire working copy will have the same
> revision (set to current branch, aka HEAD). The idea behind this
> is that your working copy of a repository should always be in
> consistent state.
>
> You can check out specific files or directories from another
> revision (mimicking "svn update -r1234 filename"):
>
> $ git checkout branch-or-sha-hash -- filename
>
> However, SVN tracks the 'revision' thing on per-file basis, while
> in git this is a property of the working copy. So if you do like
> above then git will be telling you that the 'filename' has been
> changed (as it is certainly different from its pristine version
> in HEAD):
>
> $ git status
> On branch master
> Changes to be committed:
> (use "git reset HEAD <file>..." to unstage)
>
> modified: filename
>
> So it's generally not recommended to do such a thing.
>
> Another thing that you _can do_ in git to mimick SVN is the
> 'standard layout'. There is a feature called "git worktree" which
> allows you to have SVN-like directory structure with multiple
> directories linked to different working copies:
>
> $ mkdir my-project
> $ cd my-project
> $ git clone my-project-repository master
> $ mkdir branches
> $ cd master
> $ git worktree add -b branch-1 ../branches/branch-1
> $ git worktree add -b branch-2 ../branches/branch-2
>
> After that you will have directory structure like this:
>
> $ tree my-project
> my-project
> ├── branches
> │ ├── branch-1
> │ │ ├── 1
> │ │ ├── 2
> │ │ └── 3
> │ └── branch-2
> │ ├── 1
> │ ├── 2
> │ └── banana
> └── master
> ├── 1
> └── 2
> You can work with these working copies separately, like you
> would be working with SVN. Commits in 'master' will go to the
> 'master' branch, commits made in 'branches/branch-1' will go
> to the 'branch-1' branch.
^ permalink raw reply
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Anders Kaseorg @ 2016-10-31 0:30 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jeff King, Philip Oakley, Junio C Hamano, 842477,
Git Mailing List, Vasco Almeida
In-Reply-To: <CACBZZX6ArQdG202n-SouwDhoTE1LF=69mKjWQv8HPKJ+K_0fJQ@mail.gmail.com>
On Sun, 30 Oct 2016, Ævar Arnfjörð Bjarmason wrote:
> This did break in v2.10.0, and it's taken a couple of months to notice
> this, so clearly it's not very widely used, which says something about
> the cost-benefit of maintaining this for external users.
For the record, in case this affects the calculation, it was noticed that
guilt was broken a just couple of days after the first git 2.10.x upload
to Debian, which was last weekend.
https://bugs.debian.org/842477
http://repo.or.cz/guilt.git/blob/v0.36:/guilt#l28
(I have no further opinion; I trust that Junio has all the information
needed to decide one way or the other.)
Anders
^ permalink raw reply
* Re: [PATCH] commit: simplify building parents list
From: Junio C Hamano @ 2016-10-30 23:03 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <30afdbeb-7a45-70b2-495e-35fd3b62419a@web.de>
René Scharfe <l.s.r@web.de> writes:
> Push pptr down into the FROM_MERGE branch of the if/else statement,
> where it's actually used, and call commit_list_append() for appending
> elements instead of playing tricks with commit_list_insert(). Call
> copy_commit_list() in the amend branch instead of open-coding it. Don't
> bother setting pptr in the final branch as it's not used thereafter.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> ...
> @@ -1729,7 +1727,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> reflog_msg = (whence == FROM_CHERRY_PICK)
> ? "commit (cherry-pick)"
> : "commit";
> - pptr = &commit_list_insert(current_head, pptr)->next;
> + commit_list_insert(current_head, &parents);
> }
I needed to read the full preimage to determine why this hunk is
equivalent to the original. Which is a good demonstration that what
motivated this patch is a valid issue to tackle---initializing the
pptr variable to point at &parents too early and have the long
if/elseif/... cascade work with it made the code unnecessarily
harder to understand and this update untangles that.
Thanks.
^ permalink raw reply
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Junio C Hamano @ 2016-10-30 22:53 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jeff King, Philip Oakley, Anders Kaseorg, 842477,
Git Mailing List, Vasco Almeida
In-Reply-To: <CACBZZX6ArQdG202n-SouwDhoTE1LF=69mKjWQv8HPKJ+K_0fJQ@mail.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
(commenting out of order)
> It's probably worthwhile to split off git-sh-setup into git-sh-setup &
> git-sh-setup-internal along with a documentation fix. A lot of what
> it's doing (e.g. git_broken_path_fix(), and adding a die() function)
> is probably only needed internally by git itself. The
> git-sh-setup-internal should be the thing sourcing "git-sh-i18n", I
> don't see how anyone out-of-tree could make use of that. Surely nobody
> needs to re-emit the exact message we shipped with our *.po files.
My reading of d323c6b641 ("i18n: git-sh-setup.sh: mark strings for
translation", 2016-06-17) is abit different. It needs to dot-source
the i18n stuff because the shell library functions it contains need
the localization support in the messages they emit. IOW, I do not
think i18n belongs to -internal at all.
> I don't see why we shouldn't have some stable shellscript function API
> if that's needed either.
>
> I just wanted to point out that currently git-sh-setup isn't
> documented as such. So at least a follow-up patch to the documentation
> seems in order.
>
> This did break in v2.10.0, and it's taken a couple of months to notice
> this, so clearly it's not very widely used, which says something about
> the cost-benefit of maintaining this for external users.
I am not sure if "stable API" in sh-setup is a good thing for the
ecosystem in the longer term.
As more and more in-tree scripted Porcelain commands migrate to C,
many helper functions in sh-setup will lose their in-tree users.
For example, get_author_ident_from_commit used to have three in-tree
customers (git-commit.sh, git-am.sh and git-rebase--interactive.sh),
but the first two is long gone and the third one may soon lose its
need to call it. Once a helper function in setup-sh loses all
in-tree users, we may no longer _break_ that helper, but that is
simply because we feel no need to touch it. The in-tree Porcelain
commands that migrated to C however will enhance the counterpart
they use in C to be more featureful or fix longstanding bugs in the
C version they use, while sh-setup version bitrot and making old
practice obsolete for "modern" use of Git of the day.
Keeping such a stale version that we do not use, or even we attempt
to update it without having a good vehicle to test the change
ourselves (because we no longer have any in-tree users) will be
disservice to third-party scripts---the only thing they are getting
by using the stale one, instead of reinventing their own that they
may be responsible to keep up to date, is that they share the same
staleness as everybody else that use the sh-setup version as a
third-party.
I am not arguing that we should remove what loses all in-tree users
immediately. At least not yet. But I wanted to point out that it
may not be a good use of our brain cycles to keep the API "stable"
by keeping what in-tree users do not use anymore, especially if it
does not help third-party users in the long run.
^ permalink raw reply
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Junio C Hamano @ 2016-10-30 22:25 UTC (permalink / raw)
To: Anders Kaseorg; +Cc: 842477, git, Vasco Almeida
In-Reply-To: <alpine.DEB.2.10.1610292153300.60842@buzzword-bingo.mit.edu>
Anders Kaseorg <andersk@mit.edu> writes:
> v2.10.0-rc0~45^2~2 “i18n: git-sh-setup.sh: mark strings for
> translation” broke outside scripts such as guilt that source
> git-sh-setup as described in the documentation:
>
> $ . "$(git --exec-path)/git-sh-setup"
> sh: 6: .: git-sh-i18n: not found
>
> This also affects contrib/convert-grafts-to-replace-refs.sh and
> contrib/rerere-train.sh in tree. Fix this by using git --exec-path to
> find git-sh-i18n.
>
> While we’re here, move the sourcing of git-sh-i18n below the shell
> portability fixes.
>
> Signed-off-by: Anders Kaseorg <andersk@mit.edu>
> ---
Looks good.
Our in-tree scripts rely on the fact that $PATH is adjusted to have
$GIT_EXEC_PATH early (either by getting invoked indirectly by "git"
potty, or the requirement to do so for people and scripts that still
run our in-tree scripts with dashed e.g. "git-rebase" form) by the
time they run. But when sh-setup dot-sources git-sh-i18n for its
own use, it should be explicit to name which one of the many copies
that may appear in directories on user's $PATH (one among which is
the one in $GIT_EXEC_PATH) it wants to use. And this patch does the
right thing by not relying on the $PATH, but instead naming the
exact path using $(git --exec-path)/ prefix, to the included file.
In other words, I think this patch is a pure bugfix, even if there
is no third-party script that includes it. We may want to have the
above as the rationale to apply this patch in the proposed log
message, though.
> Is this a supported use of git-sh-setup? Although the documentation is
> clear that the end user should not invoke it directly, it seems to imply
> that scripts may do this, and in practice it has worked until v2.10.0.
It is correct for the documentation to say that this is not a
"command" end users would want to run; they cannot invoke it as a
standalone command as it is written as a dot-sourced shell library.
Even though it is intended solely for internal use, so far we have
not removed things from there, which would have signalled people
that third-party scripts can also dot-source it. We may want to
reserve the right to break them in the future, but because this is a
pure bugfix, "can third-party rely on the interface not changing?"
is not a question we need to answer in this thread---there is no
reason to leave this broken.
Thanks.
> git-sh-setup.sh | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index a8a4576..240c7eb 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -2,9 +2,6 @@
> # to set up some variables pointing at the normal git directories and
> # a few helper shell functions.
>
> -# Source git-sh-i18n for gettext support.
> -. git-sh-i18n
> -
> # Having this variable in your environment would break scripts because
> # you would cause "cd" to be taken to unexpected places. If you
> # like CDPATH, define it for your interactive shell sessions without
> @@ -46,6 +43,9 @@ git_broken_path_fix () {
>
> # @@BROKEN_PATH_FIX@@
>
> +# Source git-sh-i18n for gettext support.
> +. "$(git --exec-path)/git-sh-i18n"
> +
> die () {
> die_with_status 1 "$@"
> }
^ permalink raw reply
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Ævar Arnfjörð Bjarmason @ 2016-10-30 22:11 UTC (permalink / raw)
To: Jeff King
Cc: Philip Oakley, Anders Kaseorg, Junio C Hamano, 842477,
Git Mailing List, Vasco Almeida
In-Reply-To: <20161030211227.4gqovv7mt7mtnpy7@sigill.intra.peff.net>
,On Sun, Oct 30, 2016 at 10:12 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Oct 30, 2016 at 08:09:21PM -0000, Philip Oakley wrote:
>
>> > It is documented (Documentation/git-sh-setup.txt), and this is not the
>> > internal Documentation/technical section of the documentation, so my
>> > default assumption would be that everything shown there is intended as
>> > public. I only bring this up as a question because it was apparently
>> > allowed to break. If I’m wrong and it isn’t public, other patches are
>> > needed (to the documentation and to its users in contrib).
>> >
>> But the Documenation does say ::
>>
>> - This is not a command the end user would want to run. Ever.
>>
>> - This documentation is meant for people who are studying the Porcelain-ish
>> scripts and/or are writing new ones.
>> --
>
> Historically speaking, porcelain-ish scripts were carried both in and
> out of git.git. These days what we consider porcelain is usually carried
> in-tree, but I don't think it's unreasonable for people building their
> own scripts to want to make use of git-sh-setup. And we've generally
> tried to retain backwards compatibility in the functions it provides,
> even to out-of-tree scripts.
>
> So I think it is worth applying the fix at the start of this thread to
> keep that working.
>
> As for a documentation change for "do not use this for out-of-tree
> scripts", I am mildly negative, as I don't think that matches historical
> practice.
I don't see why we shouldn't have some stable shellscript function API
if that's needed either.
I just wanted to point out that currently git-sh-setup isn't
documented as such. So at least a follow-up patch to the documentation
seems in order.
This did break in v2.10.0, and it's taken a couple of months to notice
this, so clearly it's not very widely used, which says something about
the cost-benefit of maintaining this for external users.
It's probably worthwhile to split off git-sh-setup into git-sh-setup &
git-sh-setup-internal along with a documentation fix. A lot of what
it's doing (e.g. git_broken_path_fix(), and adding a die() function)
is probably only needed internally by git itself. The
git-sh-setup-internal should be the thing sourcing "git-sh-i18n", I
don't see how anyone out-of-tree could make use of that. Surely nobody
needs to re-emit the exact message we shipped with our *.po files.
^ permalink raw reply
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Jeff King @ 2016-10-30 21:12 UTC (permalink / raw)
To: Philip Oakley
Cc: Anders Kaseorg, Ævar Arnfjörð Bjarmason,
Junio C Hamano, 842477, Git Mailing List, Vasco Almeida
In-Reply-To: <223121D101D844DEBF086AC40A5AF4CB@PhilipOakley>
On Sun, Oct 30, 2016 at 08:09:21PM -0000, Philip Oakley wrote:
> > It is documented (Documentation/git-sh-setup.txt), and this is not the
> > internal Documentation/technical section of the documentation, so my
> > default assumption would be that everything shown there is intended as
> > public. I only bring this up as a question because it was apparently
> > allowed to break. If I’m wrong and it isn’t public, other patches are
> > needed (to the documentation and to its users in contrib).
> >
> But the Documenation does say ::
>
> - This is not a command the end user would want to run. Ever.
>
> - This documentation is meant for people who are studying the Porcelain-ish
> scripts and/or are writing new ones.
> --
Historically speaking, porcelain-ish scripts were carried both in and
out of git.git. These days what we consider porcelain is usually carried
in-tree, but I don't think it's unreasonable for people building their
own scripts to want to make use of git-sh-setup. And we've generally
tried to retain backwards compatibility in the functions it provides,
even to out-of-tree scripts.
So I think it is worth applying the fix at the start of this thread to
keep that working.
As for a documentation change for "do not use this for out-of-tree
scripts", I am mildly negative, as I don't think that matches historical
practice.
-Peff
^ permalink raw reply
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Philip Oakley @ 2016-10-30 20:09 UTC (permalink / raw)
To: Anders Kaseorg, Ævar Arnfjörð Bjarmason
Cc: Junio C Hamano, 842477, Git Mailing List, Vasco Almeida
In-Reply-To: <alpine.DEB.2.10.1610301503280.60842@buzzword-bingo.mit.edu>
From: "Anders Kaseorg" <andersk@mit.edu>
> On Sun, 30 Oct 2016, Ævar Arnfjörð Bjarmason wrote:
>> This seems like a reasonable fix for this issue. However as far as I
>> can tell git-sh-setup was never meant to be used by outside scripts
>> that didn't ship as part of git itself.
>>
>> If that's the case any change in the API which AFAICT is now
>> considered internal might break them, so should some part of that be
>> made public & documented as such?
>
> It is documented (Documentation/git-sh-setup.txt), and this is not the
> internal Documentation/technical section of the documentation, so my
> default assumption would be that everything shown there is intended as
> public. I only bring this up as a question because it was apparently
> allowed to break. If I’m wrong and it isn’t public, other patches are
> needed (to the documentation and to its users in contrib).
>
But the Documenation does say ::
- This is not a command the end user would want to run. Ever.
- This documentation is meant for people who are studying the Porcelain-ish
scripts and/or are writing new ones.
--
So there is a cautionary word or two there...
The question would then become: what (if anything) was missing in the
documentation?...
maybe the inclusion of Ævar's "[Not] to be used by outside scripts that
didn't ship as part of git itself."?
Or a comment that it may change in newer versions.
Though the code fix may still be reasonable..
Philip
^ permalink raw reply
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Anders Kaseorg @ 2016-10-30 19:21 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Junio C Hamano, 842477, Git Mailing List, Vasco Almeida
In-Reply-To: <CACBZZX4SnJj6ZYK-Ha3EtiWUf_n=+LZ=UeS=7vxgsj8s=bi3Sg@mail.gmail.com>
On Sun, 30 Oct 2016, Ævar Arnfjörð Bjarmason wrote:
> This seems like a reasonable fix for this issue. However as far as I
> can tell git-sh-setup was never meant to be used by outside scripts
> that didn't ship as part of git itself.
>
> If that's the case any change in the API which AFAICT is now
> considered internal might break them, so should some part of that be
> made public & documented as such?
It is documented (Documentation/git-sh-setup.txt), and this is not the
internal Documentation/technical section of the documentation, so my
default assumption would be that everything shown there is intended as
public. I only bring this up as a question because it was apparently
allowed to break. If I’m wrong and it isn’t public, other patches are
needed (to the documentation and to its users in contrib).
Anders
^ permalink raw reply
* [PATCH] doc: fix missing "::" in config list
From: Jeff King @ 2016-10-30 18:46 UTC (permalink / raw)
To: Alexei Lozovsky
Cc: Michael Rappazzo, Junio C Hamano, ryenus, Git mailing list
In-Reply-To: <CALhvvbYJ8G12Lbe2FgP8PWKZ-LABcw2M-M-zWPkT12UUqq1vaw@mail.gmail.com>
On Sun, Oct 30, 2016 at 07:21:41PM +0200, Alexei Lozovsky wrote:
> > It would help especially when the commit message was written badly.
> >
> > Or it might be possible to customize just like "git log --format"?
>
> It is possible to change the format globally via config option
> rebase.instructionFormat:
>
> $ git config rebase.instructionFormat "%an (%ad): %s"
I had totally forgotten we added this option. When I went to look at its
documentation, I found the formatting a bit funny. This should fix it.
-- >8 --
Subject: [PATCH] doc: fix missing "::" in config list
The rebase.instructionFormat option is missing its "::" to
tell AsciiDoc that it's a list entry. As a result, the
option name gets lumped into the description in one big
paragraph.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/config.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27069ac03..a0ab66aae 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2450,7 +2450,7 @@ rebase.missingCommitsCheck::
command in the todo-list.
Defaults to "ignore".
-rebase.instructionFormat
+rebase.instructionFormat::
A format string, as specified in linkgit:git-log[1], to be used for
the instruction list during an interactive rebase. The format will automatically
have the long commit hash prepended to the format.
--
2.10.2.870.g7471999
^ permalink raw reply related
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Ævar Arnfjörð Bjarmason @ 2016-10-30 17:55 UTC (permalink / raw)
To: Anders Kaseorg; +Cc: Junio C Hamano, 842477, Git Mailing List, Vasco Almeida
In-Reply-To: <alpine.DEB.2.10.1610292153300.60842@buzzword-bingo.mit.edu>
On Sun, Oct 30, 2016 at 3:10 AM, Anders Kaseorg <andersk@mit.edu> wrote:
> v2.10.0-rc0~45^2~2 “i18n: git-sh-setup.sh: mark strings for
> translation” broke outside scripts such as guilt that source
> git-sh-setup as described in the documentation:
>
> $ . "$(git --exec-path)/git-sh-setup"
> sh: 6: .: git-sh-i18n: not found
This seems like a reasonable fix for this issue. However as far as I
can tell git-sh-setup was never meant to be used by outside scripts
that didn't ship as part of git itself.
If that's the case any change in the API which AFAICT is now
considered internal might break them, so should some part of that be
made public & documented as such?
^ permalink raw reply
* Re: [git rebase -i] show time and author besides commit hash and message?
From: Alexei Lozovsky @ 2016-10-30 17:21 UTC (permalink / raw)
To: ryenus; +Cc: Git mailing list
In-Reply-To: <CAKkAvazX1gDzwhQLTbRvxc84sjz72ONy2-P7qWijQUnQqJ+K8g@mail.gmail.com>
> It would help especially when the commit message was written badly.
>
> Or it might be possible to customize just like "git log --format"?
It is possible to change the format globally via config option
rebase.instructionFormat:
$ git config rebase.instructionFormat "%an (%ad): %s"
The format is the same as for 'git log'. This one outputs author
name, date, and the first line of commit message.
This option is supported since Git 2.6.
Or are you interested in a command-line option that can change
the format on per-invocation basis? I think there isn't one.
It can be interesting to add it, but I don't think it has much
utility...
^ permalink raw reply
* [git rebase -i] show time and author besides commit hash and message?
From: ryenus @ 2016-10-30 17:03 UTC (permalink / raw)
To: Git mailing list
It would help especially when the commit message was written badly.
Or it might be possible to customize just like "git log --format"?
Thanks
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Jeff King @ 2016-10-30 12:54 UTC (permalink / raw)
To: Aaron Pelly; +Cc: Junio C Hamano, git
In-Reply-To: <cc23eece-d693-9e40-78fe-3bafe6bcad3a@pelly.co>
On Fri, Oct 28, 2016 at 10:32:07PM +1300, Aaron Pelly wrote:
> On 28/10/16 15:54, Junio C Hamano wrote:
> > Jeff King <peff@peff.net> writes:
> >
> >> However, as I said elsewhere, I'm not convinced this feature is all that
> >> helpful for in-repository .gitignore files, and I think it does
> >> introduce compatibility complications. People with older git will not
> >> respect your .gitignore.d files. Whereas $GIT_DIR/info is purely a local
> >> matter.
> >
> > As I do not see the point of making in-tree .gitignore to a forest
> > of .gitignore.d/ at all, compatibility complications is not worth
> > even thinking about, I would have to say.
>
> Well; that saves some work. :)
>
> I do not suggesting making this mandatory. I think it adds value and it
> is a common and understood mechanism. But, if it is abhorrent, consider:
>
> There is precedent for including files in git-config. This could be
> extended to ignore files. The code is not similar, but the concept is. I
> could live with it.
Yes, but note that we don't have in-tree config files, either (to large
degree because of the security implications).
Perhaps Junio can clarify himself, but I took his statement to mean only
that in-tree .gitignore.d is not worth worrying about, but that
$GIT_DIR/info/exclude.d or similar would be OK (but perhaps I
interpreted that way because that's my own position :) ).
> Or how about a new githook that can intelligently create or return the
> details? This would be my least favourite option unless it was
> configured in an obvious place.
That seems more complicated than is really merited, and probably doesn't
perform great either (it's an extra forked process for almost every git
operation). And obviously would not work for an in-tree solution anyway,
as we do not want to run arbitrary code.
-Peff
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Junio C Hamano @ 2016-10-30 8:16 UTC (permalink / raw)
To: Jon Loeliger; +Cc: Matt McCutchen, git
In-Reply-To: <E1c0XaZ-0007Ab-QI@mylo.jdl.com>
Jon Loeliger <jdl@jdl.com> writes:
> Is there an existing protocol provision, or an extension to
> the protocol that would allow a distrustful client to say to
> the server, "Really, you have Y2? Prove it."
There is not, but I do not think it would be an effective solution.
The issue is not the lack of protocol support, but how to determine
that the other side needs such a proof for Y2 but not for other
commits. How does your side know what makes Y2 special and why does
yout side think they should not have Y2?
Once you know how to determine Y2 is special, that knowledge can be
used to abort the "push" before even starting. When you are pushing
back the 'master' and that 'master' reaches Y2, which must be kept
secret, you shouldn't be pushing that 'master' to them, whether they
claim to have Y2 or not.
I think the above is just a different way to say what Peff just said
(paraphrasing, do not push what is secret).
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Junio C Hamano @ 2016-10-30 8:03 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git
In-Reply-To: <1477757268.1524.20.camel@mattmccutchen.net>
Matt McCutchen <matt@mattmccutchen.net> writes:
> On Fri, 2016-10-28 at 22:31 -0700, Junio C Hamano wrote:
>> Not sending to the list, where mails from Gmail/phone is known to get
>> rejected.
>
> [I guess I can go ahead and quote this to the list.]
>
>> No. I'm saying that the scenario you gave is bad and people should be
>> taught not to connect to untrustworthy sites.
>
> To clarify, are you saying:
>
> (1) don't connect to an untrusted server ever (e.g., we don't promise
> that the server can't execute arbitrary code on the client), or
>
> (2) don't connect to an untrusted server if the client repository has
> data that needs to be kept secret from the server?
You sneaked "arbitrary code execution" into the discussion but I do
not know where it came from. In any case, "don't pull from or push
to untrustworthy place" would be a common sense advice that would
make sense in any scenario ;-)
Just for future reference, when you have ideas/issues that might
have possible security ramifications, I'd prefer to see it first
discussed on a private list we created for that exact purpose, until
we can assess the impact (if any). Right now MaintNotes says this:
If you think you found a security-sensitive issue and want to disclose
it to us without announcing it to wider public, please contact us at
our security mailing list <git-security@googlegroups.com>. This is
a closed list that is limited to people who need to know early about
vulnerabilities, including:
- people triaging and fixing reported vulnerabilities
- people operating major git hosting sites with many users
- people packaging and distributing git to large numbers of people
where these issues are discussed without risk of the information
leaking out before we're ready to make public announcements.
We may want to tweak the description from "disclose it to us" to
"have a discussion on it with us" (the former makes it sound as if
the topic has to be a definite problem, the latter can include an
idle speculation that may not be realistic attack vector).
^ permalink raw reply
* Re: Fetch/push lets a malicious server steal the targets of "have" lines
From: Junio C Hamano @ 2016-10-30 7:53 UTC (permalink / raw)
To: Jeff King; +Cc: Matt McCutchen, git
In-Reply-To: <20161029191023.ztrfe76u4gi4l3ci@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... It is not thinking about what secret things are hitting the
> master that you are pushing, no matter how they got there.
>
> I agree there is a potential workflow (that you have laid out) where
> such lying can cause an innocent-looking sequence of events to disclose
> the secret commits. And again, I don't mind a note in the documentation
> mentioning that. I just have trouble believing it's a common one in
> practice.
I'd say I agree with the above. I am not sure how easy people
employing common workflows can be tricked into the scenario Matt
presented, either, but I do not think it would hurt to warn people
that they need to be careful not to pull from or push to an
untrustworthy place or push things you are not sure that are clean.
> The reason I brought up the delta thing, even though it's a much harder
> attack to execute, is that it comes up in much more common workflows,
> like simply fetching from a private security-sensitive repo into your
> "main" public repo (which is an example you brought up, and something I
> know that I have personally done in the past for git.git).
Yup.
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Duy Nguyen @ 2016-10-30 3:16 UTC (permalink / raw)
To: Aaron Pelly; +Cc: Junio C Hamano, Jeff King, Git Mailing List
In-Reply-To: <cc23eece-d693-9e40-78fe-3bafe6bcad3a@pelly.co>
On Fri, Oct 28, 2016 at 4:32 PM, Aaron Pelly <aaron@pelly.co> wrote:
> Or how about a new githook that can intelligently create or return the
> details? This would be my least favourite option unless it was
> configured in an obvious place.
I wonder if smudge/clean filters can be used to recreate in-tree
.gitignore from .gitignore.d/*.
--
Duy
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Duy Nguyen @ 2016-10-30 3:09 UTC (permalink / raw)
To: Jeff King; +Cc: Jacob Keller, Aaron Pelly, Git mailing list
In-Reply-To: <20161027210458.ptzh4y75dkfaixeo@sigill.intra.peff.net>
On Fri, Oct 28, 2016 at 4:04 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Oct 27, 2016 at 12:48:34PM -0700, Jacob Keller wrote:
>
>> > I think the normal behavior in such "foo.d" directory is to just sort
>> > the contents lexically and read them in order, as if they were all
>> > concatenated together, and with no recursion. I.e., behave "as if" the
>> > user had run "cat $dir/*".
>>
>> Yea, this is the normal behavior, and the user is expected to order
>> their files lexically such as "00-name", "50-name" and so on. Pretty
>> traditional for a lot of newer configurations.
>
> One thing I will say about this approach is that you can implement it
> without any changes in git by doing:
>
> path=.git/info/exclude
> cat $path.d/* >$path
>
> and I have seen several config mechanisms basically do that (e.g.,
> Debian packaging for a program that doesn't have its own ".d" mechanism,
> but needs to grab config provided by several separate packages).
My first thought at this .git/info/exclude.d was "oh no I have to
teach untracked cache about new dependencies, or at least disable it
until it can deal with exclude.d", but this "cat" approach simplifies
things and should keep untracked cache unchanged.
There may be complication with negative patterns though. The user may
want to limit the effect of negative patterns within individual
exclude files in exclude.d so a negative pattern in exclude.d/a won't
influence anything in exclude.d/b (easier to reason, safer to compose
different exclude sets). The plain "cat" would lose file boundary info
that we need. I'm not sure. But I'll dig more into it when patches
show up.
--
Duy
^ permalink raw reply
* [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Anders Kaseorg @ 2016-10-30 2:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: 842477, git, Vasco Almeida
v2.10.0-rc0~45^2~2 “i18n: git-sh-setup.sh: mark strings for
translation” broke outside scripts such as guilt that source
git-sh-setup as described in the documentation:
$ . "$(git --exec-path)/git-sh-setup"
sh: 6: .: git-sh-i18n: not found
This also affects contrib/convert-grafts-to-replace-refs.sh and
contrib/rerere-train.sh in tree. Fix this by using git --exec-path to
find git-sh-i18n.
While we’re here, move the sourcing of git-sh-i18n below the shell
portability fixes.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
Is this a supported use of git-sh-setup? Although the documentation is
clear that the end user should not invoke it directly, it seems to imply
that scripts may do this, and in practice it has worked until v2.10.0.
git-sh-setup.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index a8a4576..240c7eb 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -2,9 +2,6 @@
# to set up some variables pointing at the normal git directories and
# a few helper shell functions.
-# Source git-sh-i18n for gettext support.
-. git-sh-i18n
-
# Having this variable in your environment would break scripts because
# you would cause "cd" to be taken to unexpected places. If you
# like CDPATH, define it for your interactive shell sessions without
@@ -46,6 +43,9 @@ git_broken_path_fix () {
# @@BROKEN_PATH_FIX@@
+# Source git-sh-i18n for gettext support.
+. "$(git --exec-path)/git-sh-i18n"
+
die () {
die_with_status 1 "$@"
}
--
2.10.1
^ permalink raw reply related
* Re: [PATCH v1 10/19] read-cache: regenerate shared index if necessary
From: Christian Couder @ 2016-10-29 22:58 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8CfQ6d3Q74W4rm=rJD69EAzuUe7PdrW-5NDo0vHuDSNpw@mail.gmail.com>
On Tue, Oct 25, 2016 at 12:16 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> @@ -2233,7 +2263,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
>> if ((v & 15) < 6)
>> istate->cache_changed |= SPLIT_INDEX_ORDERED;
>> }
>> - if (istate->cache_changed & SPLIT_INDEX_ORDERED) {
>> + if (istate->cache_changed & SPLIT_INDEX_ORDERED ||
>> + too_many_not_shared_entries(istate)) {
>
> It's probably safer to keep this piece unchanged and add this
> somewhere before it
>
> if (too_many_not_shared_entries(istate))
> istate->cache_changed |= SPLIT_INDEX_ORDERED;
>
> We could keep cache_changed consistent until the end this way.
Ok, it will be in the next version.
>> test_expect_success 'enable split index' '
>> + git config splitIndex.maxPercentChange 100 &&
>
> An alternative name might be splitThreshold. I don't know, maybe
> maxPercentChange is better.
I think it is important to say that it is a percent in the name, so I
prefer maxPercentChange.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v1 10/19] read-cache: regenerate shared index if necessary
From: Christian Couder @ 2016-10-29 22:40 UTC (permalink / raw)
To: Ramsay Jones
Cc: git, Junio C Hamano, Nguyen Thai Ngoc Duy,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <c30212bd-c454-f1a1-d01c-d6a12d20d17d@ramsayjones.plus.com>
On Sun, Oct 23, 2016 at 6:07 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>>
>> +int too_many_not_shared_entries(struct index_state *istate)
>
> This function is a file-loacal symbol; could you please make it
> a static function.
Ok, it will be in the next version.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v1 09/19] config: add git_config_get_max_percent_split_change()
From: Christian Couder @ 2016-10-29 22:24 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8A0djR6=s0AY0tzVehYY5b1-o11uRsFdGtOUCeu4Z6Xjw@mail.gmail.com>
On Tue, Oct 25, 2016 at 12:06 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> This new function will be used in a following commit to get the
>> +int git_config_get_max_percent_split_change(void)
>> +{
>> + int val = -1;
>> +
>> + if (!git_config_get_int("splitindex.maxpercentchange", &val)) {
>> + if (0 <= val && val <= 100)
>> + return val;
>> +
>> + error("splitindex.maxpercentchange value '%d' "
>
> We should keep camelCase form for easy reading. And wrap this string with _().
Ok, it will be in the next version.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency
From: Christian Couder @ 2016-10-29 22:19 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8Br2q0aadTFjkNgb=oN8nSzbkWJEK7bCCgr7v-oOZtrSA@mail.gmail.com>
On Tue, Oct 25, 2016 at 12:00 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> When users are using `git update-index --(no-)split-index`, they
>> may expect the split-index feature to be used or not according to
>> the option they just used, but this might not be the case if the
>> new "core.splitIndex" config variable has been set. In this case
>> let's warn about what will happen and why.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>> builtin/update-index.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/update-index.c b/builtin/update-index.c
>> index b75ea03..a14dbf2 100644
>> --- a/builtin/update-index.c
>> +++ b/builtin/update-index.c
>> @@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>> }
>>
>> if (split_index > 0) {
>> + if (git_config_get_split_index() == 0)
>> + warning("core.splitIndex is set to false; "
>> + "remove or change it, if you really want to "
>> + "enable split index");
>
> Wrap this string and the one below with _() so they can be translated.
Ok, it will be in the next version.
^ 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