* [PATCH v2 0/2] Let the sequencer handle the grunt work of rebase -i
From: Johannes Schindelin @ 2017-02-09 22:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1472805251.git.johannes.schindelin@gmx.de>
After all of these patch series y'all had to review, this is finally the
one that switches things over.
Please note that it does not (yet) handle the `git rebase -i --root`
invocation; I tried to focus on the common case, and I rarely use --root
myself.
Please note also that --preserve-merges is *not* handled.
The way I designed --preserve-merges is totally stupid and idiotic and I
do not want to spend any further time on it. You cannot "pick" merges
and hope to be able to reorder commits, for example. I may work on
porting Git garden shears' way to recreate the branch structure into
rebase -i proper at some stage.
And please finally note that this pair of patches does not yet yield the
full speed improvement that I promised earlier. After these patches, the
time is dominated by pre- and post-processing the todo script, at least
on Windows, so there is another patch series that ports those bits and
pieces into the rebase--helper, too.
Changes since v1:
- rebased to current master
- this required a change in t3404 because I was bullied^Wasked to change
some messages (which should not have been conflated with the work I
actually wanted to do, but whatevs)
Johannes Schindelin (2):
Add a builtin helper for interactive rebases
rebase -i: use the rebase--helper builtin
.gitignore | 1 +
Makefile | 1 +
builtin.h | 1 +
builtin/rebase--helper.c | 40 ++++++++++++++++++++++++++++++++++++++++
git-rebase--interactive.sh | 13 +++++++++++++
git.c | 1 +
t/t3404-rebase-interactive.sh | 2 +-
7 files changed, 58 insertions(+), 1 deletion(-)
create mode 100644 builtin/rebase--helper.c
base-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8
Based-On: sequencer-i at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git sequencer-i
Published-As: https://github.com/dscho/git/releases/tag/rebase--helper-v2
Fetch-It-Via: git fetch https://github.com/dscho/git rebase--helper-v2
Interdiff vs v1:
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e2f18d11f6..33d392ba11 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -556,7 +556,7 @@ test_expect_success 'clean error after failed "exec"' '
echo "edited again" > file7 &&
git add file7 &&
test_must_fail git rebase --continue 2>error &&
- test_i18ngrep "You have staged changes in your working tree." error
+ test_i18ngrep "you have staged changes in your working tree" error
'
test_expect_success 'rebase a detached HEAD' '
--
2.11.1.windows.1
^ permalink raw reply
* [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Johannes Schindelin @ 2017-02-09 22:27 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Junio C Hamano
From: Jeff Hostetler <jeffhost@microsoft.com>
Use OpenSSL's SHA-1 routines rather than builtin block-sha1 routines.
This improves performance on SHA1 operations on Intel processors.
OpenSSL 1.0.2 has made considerable performance improvements and
support the Intel hardware acceleration features. See:
https://software.intel.com/en-us/articles/improving-openssl-performance
https://software.intel.com/en-us/articles/intel-sha-extensions
To test this I added/staged a single file in a gigantic
repository having a 450MB index file. The code in read-cache.c
verifies the header SHA as it reads the index and computes a new
header SHA as it writes out the new index. Therefore, in this test
the SHA code must process 900MB of data. Testing was done on an
Intel I7-4770 CPU @ 3.40GHz (Intel64, Family 6, Model 60) CPU.
The block-sha1 version averaged 5.27 seconds.
The OpenSSL version averaged 4.50 seconds.
================================================================
$ echo xxx >> project.mk
$ time /e/blk_sha/bin/git.exe add project.mk
real 0m5.207s
user 0m0.000s
sys 0m0.250s
$ echo xxx >> project.mk
$ time /e/blk_sha/bin/git.exe add project.mk
real 0m5.362s
user 0m0.015s
sys 0m0.234s
$ echo xxx >> project.mk
$ time /e/blk_sha/bin/git.exe add project.mk
real 0m5.300s
user 0m0.016s
sys 0m0.250s
$ echo xxx >> project.mk
$ time /e/blk_sha/bin/git.exe add project.mk
real 0m5.216s
user 0m0.000s
sys 0m0.250s
================================================================
$ echo xxx >> project.mk
$ time /e/openssl/bin/git.exe add project.mk
real 0m4.431s
user 0m0.000s
sys 0m0.250s
$ echo xxx >> project.mk
$ time /e/openssl/bin/git.exe add project.mk
real 0m4.478s
user 0m0.000s
sys 0m0.265s
$ echo xxx >> project.mk
$ time /e/openssl/bin/git.exe add project.mk
real 0m4.690s
user 0m0.000s
sys 0m0.250s
$ echo xxx >> project.mk
$ time /e/openssl/bin/git.exe add project.mk
real 0m4.420s
user 0m0.000s
sys 0m0.234s
================================================================
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/mingw-openssl-sha1-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-openssl-sha1-v1
config.mak.uname | 1 -
1 file changed, 1 deletion(-)
diff --git a/config.mak.uname b/config.mak.uname
index 447f36ac2e..a07936da8b 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -515,7 +515,6 @@ ifneq (,$(findstring MINGW,$(uname_S)))
OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
NO_REGEX = YesPlease
NO_PYTHON = YesPlease
- BLK_SHA1 = YesPlease
ETAGS_TARGET = ETAGS
NO_INET_PTON = YesPlease
NO_INET_NTOP = YesPlease
base-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8
--
2.11.1.windows.1
^ permalink raw reply related
* Re: What's cooking in git.git (Feb 2017, #02; Mon, 6)
From: Johannes Schindelin @ 2017-02-09 22:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqk28znhtx.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 9 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > ... and even go so far as calling your patch reverting my refactoring
> > a hot-fix, why don't you just go ahead and merge the result over my
> > objections?
>
> At this point, you are simply being silly.
How is it that this patch cannot be applied when, and if, that
hypothetical config setting is introduced?
Maybe I am dense here, but I would really like to know why this
preparatory patch must be applied *now*, when there is nothing to prepare
for.
> Isn't "Putty is not a command but is also handled as if it is a valid
> implementation of SSH" a bug?
If you think it is a bug to handle an ssh command called "putty" as if it
were plink, sure. I do not think there is a valid use case, but hey.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v2 0/2] Let the sequencer handle the grunt work of rebase -i
From: Junio C Hamano @ 2017-02-09 22:40 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <cover.1486678952.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> After all of these patch series y'all had to review, this is finally the
> one that switches things over.
>
> Please note that it does not (yet) handle the `git rebase -i --root`
> invocation; I tried to focus on the common case, and I rarely use --root
> myself.
As long as the longer-term goal is clear enough and the short-term
approach does not conflict with the goal, solving the most common
problem that yields the largest payback first is absolutely the
right thing to do, and omitting "--root" and/or "-p" and getting the
main use of "-i" right is a great way to start.
> .gitignore | 1 +
> Makefile | 1 +
> builtin.h | 1 +
> builtin/rebase--helper.c | 40 ++++++++++++++++++++++++++++++++++++++++
> git-rebase--interactive.sh | 13 +++++++++++++
> git.c | 1 +
> t/t3404-rebase-interactive.sh | 2 +-
> 7 files changed, 58 insertions(+), 1 deletion(-)
> create mode 100644 builtin/rebase--helper.c
And it is surprisingly short and sweet ;-)
Will queue as js/rebase-helper topic, forked at 6e3a7b3398 ("Git
2.12-rc0", 2017-02-03).
Thanks.
PS. in case if anybody is wondering after reading [*1*], at this
point, I _have_ read the patches not just the cover letter, looked
at the branch name the original author gave to the topic, chose the
local topic name I use, and chose where to fork the topic from, but
have not applied the patches (so I may later end up saying "the
patch does not apply cleanly", "the compiler complains on this
line", or "the new code is inconsistent with this existing code that
is a bit beyond the context of the patch that I did not notice when
I reviewed the patches alone" in a separate message). I do not have
a new entry for this topic in the draft of "What's cooking" report
yet, or have not decided if the topic would hit 'jch' or 'pu' yet
either.
[Reference]
*1* http://public-inbox.org/git/xmqq7f4zqiyj.fsf@gitster.mtv.corp.google.com
^ permalink raw reply
* Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory
From: Junio C Hamano @ 2017-02-09 22:54 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Duy Nguyen, Git Mailing List
In-Reply-To: <alpine.DEB.2.20.1702092304250.3496@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> I have no strong opinion for or against a "longer term" solution
>> that makes "rev-parse --git-path" behave differently from how it
>> behaves today, but I am not yet convinced that we can reach that
>> longer term goal without a transition period, as I suspect there are
>> existing users that know and came to expect how it behaves, based on
>> its today's behaviour. Other than that I do not have suggestion on
>> this topic at the moment.
I think I was simply being silly (not merely "overcautious", but
just "silly") here.
There is no reason for people to use "--git-path" if they are not
preparing to work with secondary worktrees, because the whole point
of the feature is so that cases where "$(rev-parse --git-dir)/path"
does a wrong thing (e.g. end up referring to the main worktree thing
when you need to refer to your own, or vice versa).
> Given that
> ...
> it should be safe to assume that a transitional period is more likely to
> do more harm to our users than bring benefit.
In short, "--git-path as currently exposed to the end-users is
utterly broken and cannot have been used for anything sensible". If
that is the case, let's just change that with an entry in the
release notes that states so (iow, there is no need for even a
backward compatibility notice, we just have an entry that says "this
was totally broken in such and such way, and now it is fixed to
behave this way").
That leaves what the right single-step behaviour change should be.
As I recall Duy said something about --common-dir and other things
Mike's earlier change also covered, I'd prefer to leave it to three
of you to figure out what the final patch should be.
Thanks.
^ permalink raw reply
* Re: [RFC-PATCHv2] submodules: add a background story
From: Junio C Hamano @ 2017-02-09 23:32 UTC (permalink / raw)
To: Stefan Beller
In-Reply-To: <20170209020855.23486-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Just like gitmodules(5), gitattributes(5), gitcredentials(7),
> gitnamespaces(7), gittutorial(7), we'd like to provide some background
> on submodules, which is not specific to the `submodule` command, but
> elaborates on the background and its intended usage.
>
> Add gitsubmodules(7), that explains the states, structure and usage of
> submodules.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> This would replace the last patch of sb/submodule-doc, though it's still
> RFC. In this revision I took care of the technical details (i.e. proper
> formatting, spelling), and only slight rewording of the text.
>
> The main issue persists; see bottom of the patch:
>
> SAMPLE WORKFLOWS (RFC/TODO)
> ---------------------------
>
> Do we need
>
> * an opinionated way to check for a specific state of a submodule
> * (submodule helper to be plumbing?)
> * expose the design mistake of having the (name->path) mapping inside the
> working tree, i.e. never remove a name from the submodule config even when
> the submodule doesn't exist any more.
I am not sure about the last item.
Are you talking about a case where submodule comes and goes (think:
"git checkout v1.0" that would make submodules added since that
version disappar)? .gitmodules that is checked out would not have
any entry, but .git/config needs to record the end-user preference
for the module, so that the user can do "git checkout -" to come
back, no? IOW .git/config that mentions all the submodule the user
ever showed interests in is not a design mistake, so you must be
talking about something else, but I do not know what it is.
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index 4a4cede144..d38aa2d53a 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -24,37 +24,7 @@ DESCRIPTION
> -----------
> Inspects, updates and manages submodules.
>
> -A submodule allows you to keep another Git repository in a subdirectory
> ...
> -if you choose to go that route.
> +For more information about submodules, see linkgit:gitsubmodules[5]
OK.
> @@ -420,6 +390,10 @@ This file should be formatted in the same way as `$GIT_DIR/config`. The key
> to each submodule url is "submodule.$name.url". See linkgit:gitmodules[5]
> for details.
>
> +SEE ALSO
> +--------
> +linkgit:gitsubmodules[1], linkgit:gitmodules[1].
Are they both in section (1)? I think the former (concepts) belongs
to section 7 and the latter (file formats) belongs to section 5.
> diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt
> new file mode 100644
> index 0000000000..3369d55ae9
> --- /dev/null
> +++ b/Documentation/gitsubmodules.txt
> @@ -0,0 +1,194 @@
> +gitsubmodules(7)
> +================
> +
> +NAME
> +----
> +gitsubmodules - information about submodules
> +
> +SYNOPSIS
> +--------
> +$GIT_DIR/config, .gitmodules
> +
> +------------------
> +git submodule
> +------------------
> +
> +DESCRIPTION
> +-----------
> +
> +A submodule allows you to keep another Git repository in a subdirectory
> +...
> +When cloning or pulling a repository containing submodules however,
> +the submodules will not be checked out by default; You need to instruct
> +'clone' to recurse into submodules. The 'init' and 'update' subcommands
I think this is not "You need to", but rather "You can, if you want
to have each and every submodules."
> +of 'git submodule' will maintain submodules checked out and at an
> +appropriate revision in your working tree.
> +
> +WHEN TO USE
> +-----------
> +
> +Submodules, repositories inside other repositories,
> +can be used for different use cases:
> +
> +* To have finer grained access control.
> + The design principles of Git do not allow for partial repositories to be
> + checked out or transferred. A repository is the smallest unit that a user
> + can be given access to. Submodules are separate repositories, such that
> + you can restrict access to parts of your project via the use of submodules.
> +
> +* To decouple Git histories.
> + Decoupling histories has different benefits.
> +
> +** When you want to use a (third party) library tied to a specific version.
> + Using submodules for a library allows you to have a clean history for
> + your own project and only updating the library in the submodule when needed.
I somehow do not see this as decoupling; it is keeping what is
originally separate separate, isn't it?
> +** In its current form Git scales up poorly for very large repositories that
> + change a lot, as the history grows very large. For that you may want to look
> + at shallow clone, sparse checkout or git-lfs.
> + However you can also use submodules to e.g. hold large binary assets
> + and these repositories are then shallowly cloned such that you do not
> + have a large history locally.
In other words, a better way to list these may be
1. using another project that stands on its own.
2. artificially split a (logically single) project into multiple
repositories and tying them back together.
The access control and performance reasons are subclasses of 2.
IOW, if Git had per-path ACL and infinite scaling, you wouldn't be
splitting your project into submodules for 2. You would still want
to use somebody else's project by binding it as a subproject, instead
of merging its history into yours.
> +When working with submodules, you can think of them as in a state machine.
> +So each submodule can be in a different state, the following indicators are used:
> +
> +* the existence of the setting of 'submodule.<name>.url' in the
> + superprojects configuration
> +* the existence of the submodules working tree within the
> + working tree of the superproject
> +* the existence of the submodules git directory within the superprojects
> + git directory at $GIT_DIR/modules/<name> or within the submodules working
> + tree
> +
> + State URL config working tree git dir
> + -----------------------------------------------------
> + uninitialized no no no
> + initialized yes no no
> + populated yes yes yes
> + depopulated yes no yes
> + deinitialized no no yes
> + uninteresting no yes yes
> +
> + invalid no yes no
> + invalid yes yes no
I do not have strong opinions on these labels; are submodule folks
happy with the above vocabulary?
"uninteresting" is not explained in the below?
> ...
> +SEE ALSO
> +--------
> +linkgit:git-submodule[1], linkgit:gitmodules[1].
Ditto.
^ permalink raw reply
* Re: Bug with fixup and autosquash
From: Philip Oakley @ 2017-02-09 23:31 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano
Cc: Ashutosh Bapat, git, Michael Haggerty, Michael J Gruber,
Matthieu Moy
In-Reply-To: <alpine.DEB.2.20.1702092142020.3496@virtualbox>
From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Sent: Thursday, February 09, 2017 8:55 PM
> Hi Ashutosh and Junio,
>
> On Wed, 8 Feb 2017, Junio C Hamano wrote:
>
>> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>>
>> > I have been using git rebase heavily these days and seem to have found
>> > a bug.
>> >
>> > If there are two commit messages which have same prefix e.g.
>> > yyyyyy This is prefix
>> > xxxxxx This is prefix and message
>> >
>> > xxxxxx comitted before yyyyyy
>> >
>> > Now I commit a fixup to yyyyyy using git commit --fixup yyyyyy
>> > zzzzzz fixup! This is prefix
>> >
>> > When I run git rebase -i --autosquash, the script it shows me looks
>> > like
>> > pick xxxxxx This is prefix and message
>> > fixup zzzzzz fixup! This is prefix
>> > pick yyyyyy This is prefix
>> >
>> > I think the correct order is
>> > pick xxxxxx This is prefix and message
>> > pick yyyyyy This is prefix
>> > fixup zzzzzz fixup! This is prefix
>> >
>> > Is that right?
>>
>> [...]
>>
>> Unfortunately, "rebase -i --autosquash" reorders the entries by
>> identifying the commit by its title, and it goes with prefix match so
>> that fix-up commits created without using --fixup option but manually
>> records the title's prefix substring can also work.
>
> This prefix match also happens to introduce a serious performance problem,
> which is why I "fixed" this issue in the rebase--helper already (which is
> the case if you are using Git for Windows, whose master branch builds on
> Linux and MacOSX as well). I quoted "fix" because my motivation was to fix
> the performance problem, not the "incorrect match" problem.
>
> The rebase--helper code (specifically, the patch moving autosquash logic
> into it: https://github.com/dscho/git/commit/7d0831637f) tries to match
> exact onelines first,
While I think this is an improvement, and will strongly support the `git
commit --fixup=<commit>` option which will, if the sha1/oid is given, create
the exact commit subject line.
However it would also be useful if the actual commit subject line could have
a similar format option, so that those who use say the git gui (rather than
the cli) for the commit message, could easily create the `!fixup <commit>`
message which would allow a broader range of ways of spelling the commit
(e.g. giving a sha1(min length) that is within the rebase todo list).
> and falls back to prefix matching only after that.
>
> Now that the sequencer-i patch series is in `master`, the next step is to
> send the patch series introducing the rebase--helper. The patch series
> including the fix discussed above relies on that one. Meaning that it will
> take a while to get through the mill.
>
> So please do not hold your breath until this feature/fix hits an official
> Git version. If you need it[*1*] faster, feel free to build Git for
> Windows' master and run with that for a while.
>
> Ciao,
> Johannes
>
> Footnote: By "it" I mean "the feature/fix", not "an official Git version"
> nor "your breath".
>
--
Philip
^ permalink raw reply
* Re: [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Junio C Hamano @ 2017-02-09 23:41 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff Hostetler
In-Reply-To: <6a29f8c60d315a24292c1fa9f5e84df4dfdbf813.1486679254.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Use OpenSSL's SHA-1 routines rather than builtin block-sha1 routines.
> This improves performance on SHA1 operations on Intel processors.
> ...
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
Nice. Will queue as jh/mingw-openssl-sha1 topic; it is a bit too
late for today's integration cycle to be merged to 'next', but let's
have this by the end of the week in 'master'.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] git-p4: fix git-p4.pathEncoding for removed files
From: Junio C Hamano @ 2017-02-09 23:39 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, luke
In-Reply-To: <20170209150656.9070-1-larsxschneider@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
> unfortunately, I missed to send this v2. I agree with Luke's review and
> I moved the re-encode of the path name to the `streamOneP4File` and
> `streamOneP4Deletion` explicitly.
>
> Discussion:
> http://public-inbox.org/git/CAE5ih7-=bD_ZoL5pFYfD2Qvy-XE24V_cgge0XoAvuoTK02EDfg@mail.gmail.com/
>
> Thanks,
> Lars
Thanks. Will replace but will not immediately merge to 'next' yet,
just in case Luke wants to tell me add his "Reviewed-by:".
^ permalink raw reply
* Re: [PATCH] gc: ignore old gc.log files
From: Philip Oakley @ 2017-02-09 23:57 UTC (permalink / raw)
To: Jeff King, David Turner; +Cc: git, pclouds
In-Reply-To: <20170209032325.bspll66ux6n2pj4n@sigill.intra.peff.net>
From: "Jeff King" <peff@peff.net>
> On Wed, Feb 08, 2017 at 09:02:22PM -0500, David Turner wrote:
>
>> The intent of automatic gc is to have a git repository be relatively
>> low-maintenance from a server-operator perspective. Of course, large
>> operators like GitHub will need a more complicated management strategy,
>> but for ordinary usage, git should just work.
>>
>> In this commit, git learns to ignore gc.log files which are older than
>> (by default) one day old. It also learns about a config, gc.maxLogAge
>> to manage this.
>>
>> So git should never get itself into a state where it refuses to do any
>> maintenance, just because at some point some piece of the maintenance
>> didn't make progress. That might still happen (e.g. because the repo
>> is corrupt), but at the very least it won't be because Git is too dumb
>> to try again.
>
> Sounds like a good goal and approach.
>
>> +gc.maxLogAge::
>> + If the file gc.log exists, then `git gc --auto` won't run
>> + unless that file is more than maxLogAge seconds old. Default
>> + is 86400, one day.
Is there a reason why one day is chosen? If maintenance staff are available
24/7 then a shorter time would be appropriate, but if it's a 5 day work week
then they may want longer. Is there a particular case it targets?
>
> For other time-based config, we use approxidate with a relative time,
> like "1 day ago". I think it would make sense for this to match, as it
> makes the config a little more readable.
>
> You can follow the prune_expire example which is right below your new
> config variable in all of the hunks of your patch. Though I think
> ultimately that isn't parsed inside gc, so you'd eventually look at how
> "prune --expire" is handled (which I think is via parse_expiry_date()).
>
[...]
Philip
^ permalink raw reply
* Re: [PATCH] gc: ignore old gc.log files
From: Jeff King @ 2017-02-10 0:36 UTC (permalink / raw)
To: Philip Oakley; +Cc: David Turner, git, pclouds
In-Reply-To: <1637154470724DABAC99AF6199707A4B@PhilipOakley>
On Thu, Feb 09, 2017 at 11:57:12PM -0000, Philip Oakley wrote:
> > > +gc.maxLogAge::
> > > + If the file gc.log exists, then `git gc --auto` won't run
> > > + unless that file is more than maxLogAge seconds old. Default
> > > + is 86400, one day.
>
> Is there a reason why one day is chosen? If maintenance staff are available
> 24/7 then a shorter time would be appropriate, but if it's a 5 day work week
> then they may want longer. Is there a particular case it targets?
I'm pretty sure the one-day time limit isn't scientific. It's just a
number we've been throwing around.
I'm not sure what maintenance staff matters, though. It basically needs
long enough that we're not doing _too_ many fruitless gc's, because it
wastes resources. But you'd prefer to not go too long without a gc for a
repository that needs it.
The root cause of the error could be any number of issues. But for the
case that David cares about most, you basically want to keep trying
until the too-many-objects condition goes away. That's usually on a
2-week timer. So trying once per day to see if the 2-week timer feels
about right.
That's certainly not science, but hopefully it at least frames the
general ballpark.
One possible option would be to auto-scale it with the pruneExpire time.
I don't know if people actually tweak that value or not.
-Peff
^ permalink raw reply
* Re: [PATCH 0/5] Store submodules in a hash, not a linked list
From: Jeff King @ 2017-02-10 0:40 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Stefan Beller, Johannes Schindelin, David Turner, git
In-Reply-To: <9ba16176-b388-4c70-a479-fda4c9244e67@alum.mit.edu>
On Thu, Feb 09, 2017 at 10:23:35PM +0100, Michael Haggerty wrote:
> >> So push the submodule attribute down to the `files_ref_store` class
> >> (but continue to let the `ref_store`s be looked up by submodule).
> >
> > I'm not sure I understand all of the ramifications here. It _sounds_ like
> > pushing this down into the files-backend code would make it harder to
> > have mixed ref-backends for different submodules. Or is this just
> > pushing down an implementation detail of the files backend, and future
> > code is free to have as many different ref_stores as it likes?
>
> I don't understand how this would make it harder, aside from the fact
> that a new backend class might also need a path member and have to
> maintain its own copy rather than using one that the base class provides.
Probably the answer is "I'm really confused".
But here's how my line of reasoning went:
Right now we have a main ref-store that points to the submodule
ref-stores. I don't know the current state of it, but in theory those
could all use different backends.
This seems like it's pushing that submodule linkage down into the
backend.
But I think from your response that the answer is no, the thing that is
being pushed down is not the right way for the main ref store and the
submodules to be linked. In fact, there is no reason at all for the
main ref store to know or care about submodules. Anybody who wants to
know about a submodule's refs should ask the hashmap.
-Peff
^ permalink raw reply
* [PATCH] help: show help for aliases
From: Tom Kunze @ 2017-02-10 0:17 UTC (permalink / raw)
To: git
If an alias is a single git command show the man page of the
aliased git command with --help.
Signed-off-by: Tom Kunze <mail@tom-kunze.de>
---
Hi,
I noticed that when I pass --help to an alias which is only a git
command it tells me a information about the alias. But it would be
nice if it instead opens the corresponding man page of the command.
There is a memory leak but in my opinion it can be ignored because
the process will be replaced anyway.
Regards,
Tom Kunze
PS: Please add me to cc as I am not subscribed.
builtin/help.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/builtin/help.c b/builtin/help.c
index 49f7a07..655ed49 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -437,6 +437,10 @@ static const char *check_git_cmd(const char* cmd)
alias = alias_lookup(cmd);
if (alias) {
+ if (alias[0] != '!') {
+ strtok(alias, " \t\n");
+ return alias;
+ }
printf_ln(_("`git %s' is aliased to `%s'"), cmd, alias);
free(alias);
exit(0);
--
2.1.4
^ permalink raw reply related
* Re: [PATCH] help: show help for aliases
From: Junio C Hamano @ 2017-02-10 1:46 UTC (permalink / raw)
To: Tom Kunze; +Cc: git
In-Reply-To: <137f35a4-ec2e-b8aa-c6a5-b17688eca61a@tomabrafix.de>
Tom Kunze <mail@tom-kunze.de> writes:
> If an alias is a single git command show the man page of the
> aliased git command with --help.
>
> Signed-off-by: Tom Kunze <mail@tom-kunze.de>
> ...
> diff --git a/builtin/help.c b/builtin/help.c
> index 49f7a07..655ed49 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -437,6 +437,10 @@ static const char *check_git_cmd(const char* cmd)
>
> alias = alias_lookup(cmd);
> if (alias) {
> + if (alias[0] != '!') {
> + strtok(alias, " \t\n");
> + return alias;
> + }
While I understand where you come from, I am moderately negative,
especially with that strtok() to ignore options.
For a truly simple alias, e.g.
$ git co --help
`git co' is aliased to `checkout'
I do not think I would mind the updated behaviour given by this
patch that much.
But most of the time, when I do "help" on an alias, I am primarily
interested in what default customization I am using over the base
command, i.e.
$ git lgf --help
`git lgf' is aliased to `log --oneline --boundary --first-parent'
is my way to remind me that I am using these three options to "git
log" in the alias I very often use (and forgot what they were).
Jumping directly to the "git log" manual page is the last thing I
want "help" to do.
^ permalink raw reply
* Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory
From: Mike Rappazzo @ 2017-02-10 3:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Duy Nguyen, Git Mailing List
In-Reply-To: <xmqqwpczm0vj.fsf@gitster.mtv.corp.google.com>
On Thu, Feb 9, 2017 at 5:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> That leaves what the right single-step behaviour change should be.
> As I recall Duy said something about --common-dir and other things
> Mike's earlier change also covered, I'd prefer to leave it to three
> of you to figure out what the final patch should be.
>
The tests which I covered in my previous patch [1] addressed the
places where we identified similar problems. We should try to include
some form of those tests. As far as implementation goes in rev-parse,
the version in this thread is probably better that what I had, but it
would need to also be applied to --git-common-dir and
--shared-index-path.
[1] http://public-inbox.org/git/1464261556-89722-2-git-send-email-rappazzo@gmail.com/
^ permalink raw reply
* Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory
From: Jeff King @ 2017-02-10 4:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, git, Nguyễn Thái Ngọc Duy
In-Reply-To: <xmqqo9ybnie0.fsf@gitster.mtv.corp.google.com>
On Thu, Feb 09, 2017 at 01:50:31PM -0800, Junio C Hamano wrote:
> > There is just no way you can "fix" this otherwise. As an occasional shell
> > scripter, you may be tempted to use `$(git rev-parse --show-cdup)$(git
> > rev-parse --git-path filename)`, but of course that breaks in worktrees
> > and if you do not use worktrees you would not even know that your
> > workaround introduced another bug.
>
> In case it is not clear, I understand all of the above.
>
> I was just worried about the people who do *NOT* use worktrees and
> did the obvious "concatenate --cdup with --git-path" and thought
> their script were working happily and well. By prepending the path
> to the (real) location of the .git in the updated --git-path output
> ourselves, they will complain, our update broke their script.
That concatenating approach is broken in other ways, too. For example,
if you have $GIT_DIR set to an absolute path, then --git-path will use
that. I don't think we have ever promised that the output of --git-path
(or --git-dir) would ever be absolute or relative (in fact, the
--git-dir documentation implies that you may get either).
So yes, there could be somebody who is doing this concatenation
workaround, but only ever calls their script in a certain way that never
triggers the absolute-path variant. They're happy now, and may not be
after Dscho's patch. But I really think they are relying on a bogus
assumption, and their scripts are already buggy.
-Peff
^ permalink raw reply
* Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap
From: Jeff King @ 2017-02-10 4:04 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Haggerty, Nguyễn Thái Ngọc Duy,
Stefan Beller, Johannes Schindelin, David Turner, git
In-Reply-To: <xmqqh943p0hv.fsf@gitster.mtv.corp.google.com>
On Thu, Feb 09, 2017 at 12:34:04PM -0800, Junio C Hamano wrote:
> > +static struct submodule_hash_entry *alloc_submodule_hash_entry(
> > + const char *submodule, struct ref_store *refs)
> > +{
> > + size_t len = strlen(submodule);
> > + struct submodule_hash_entry *entry = malloc(sizeof(*entry) + len + 1);
>
> I think this (and the later memcpy) is what FLEX_ALLOC_MEM() was
> invented for.
Yes, it was. Though since the length comes from a strlen() call, it can
actually use the _STR variant, like:
FLEX_ALLOC_STR(entry, submodule, submodule);
Besides being shorter, this does integer-overflow checks on the final
length.
> > @@ -1373,16 +1405,17 @@ void base_ref_store_init(struct ref_store *refs,
> > die("BUG: main_ref_store initialized twice");
> >
> > refs->submodule = "";
> > - refs->next = NULL;
> > main_ref_store = refs;
> > } else {
> > - if (lookup_ref_store(submodule))
> > + refs->submodule = xstrdup(submodule);
> > +
> > + if (!submodule_ref_stores.tablesize)
> > + hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);
>
> Makes me wonder what "20" stands for. Perhaps the caller should be
> allowed to say "I do not quite care what initial size is" by passing
> 0 or some equally but more clealy meaningless value (which of course
> would be outside the scope of this series).
I think this is what "0" already does (grep for HASHMAP_INITIAL_SIZE).
In fact, that constant is 64. The 20 we pass in goes through some magic
load-factor computation and ends up as 25. That being smaller than the
INITIAL_SIZE constant, I believe that we end up allocating 64 entries
either way (that's just from reading the code, though; I didn't run it
to double check).
-Peff
^ permalink raw reply
* Re: [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Jeff King @ 2017-02-10 5:02 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff Hostetler, Junio C Hamano
In-Reply-To: <6a29f8c60d315a24292c1fa9f5e84df4dfdbf813.1486679254.git.johannes.schindelin@gmx.de>
On Thu, Feb 09, 2017 at 11:27:49PM +0100, Johannes Schindelin wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Use OpenSSL's SHA-1 routines rather than builtin block-sha1 routines.
> This improves performance on SHA1 operations on Intel processors.
>
> OpenSSL 1.0.2 has made considerable performance improvements and
> support the Intel hardware acceleration features. See:
> https://software.intel.com/en-us/articles/improving-openssl-performance
> https://software.intel.com/en-us/articles/intel-sha-extensions
>
> To test this I added/staged a single file in a gigantic
> repository having a 450MB index file. The code in read-cache.c
> verifies the header SHA as it reads the index and computes a new
> header SHA as it writes out the new index. Therefore, in this test
> the SHA code must process 900MB of data. Testing was done on an
> Intel I7-4770 CPU @ 3.40GHz (Intel64, Family 6, Model 60) CPU.
I think this is only half the story. A heavy-sha1 workload is faster,
which is good. But one of the original reasons to prefer blk-sha1 (at
least on Linux) is that resolving libcrypto.so symbols takes a
non-trivial amount of time. I just timed it again, and it seems to be
consistently 1ms slower to run "git rev-parse --git-dir" on my machine
(from the top-level of a repo).
1ms is mostly irrelevant, but it adds up on scripted workloads that
start a lot of git processes. Whether it's a net win or not depends on
how much sha1 computation you do in your workload versus how many
processes you start.
I don't know what that means for Windows, though. My impression is that
process startup is so painfully slow there that the link time may just
be lost in the noise. It may just always be a win there. So not really
an objection to your patch, but something you may want to consider.
(Of course, it would in theory be possible to have the best of both
worlds either by static-linking openssl, or by teaching block-sha1 the
same optimizations, but both of those are obviously much more complex).
-Peff
^ permalink raw reply
* git subtree add fails in new repository
From: Daurnimator @ 2017-02-10 6:03 UTC (permalink / raw)
To: Git Mailing List
I'm trying to make a series of repository that only contain subtrees
for various other projects.
However git subtree does not like being on a newly created branch:
$ git init
$ git subtree add --prefix=git https://github.com/git/git.git master
fatal: ambiguous argument 'HEAD': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
Working tree has modifications. Cannot add.
^ permalink raw reply
* [PATCH v2] gc: ignore old gc.log files
From: David Turner @ 2017-02-09 19:17 UTC (permalink / raw)
To: git; +Cc: peff, pclouds, David Turner
The intent of automatic gc is to have a git repository be relatively
low-maintenance from a server-operator perspective. Of course, large
operators like GitHub will need a more complicated management strategy,
but for ordinary usage, git should just work.
In this commit, git learns to ignore gc.log files which are older than
(by default) one day old. It also learns about a config, gc.logExpiry
to manage this. There is also some cleanup: a successful manual gc,
or a warning-free auto gc with an old log file, will remove any old
gc.log files.
So git should never get itself into a state where it refuses to do any
maintenance, just because at some point some piece of the maintenance
didn't make progress. That might still happen (e.g. because the repo
is corrupt), but at the very least it won't be because Git is too dumb
to try again.
Signed-off-by: David Turner <dturner@twosigma.com>
Helped-by: Jeff King <peff@peff.net>
---
Documentation/config.txt | 5 +++++
builtin/gc.c | 42 +++++++++++++++++++++++++++++++++++-------
t/t6500-gc.sh | 13 +++++++++++++
3 files changed, 53 insertions(+), 7 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index fc5a28a32..2c2c9c75c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1402,6 +1402,11 @@ gc.autoDetach::
Make `git gc --auto` return immediately and run in background
if the system supports it. Default is true.
+gc.logExpiry::
+ If the file gc.log exists, then `git gc --auto` won't run
+ unless that file is more than 'gc.logExpiry' old. Default is
+ "1.day". See `gc.pruneExpire` for more possible values.
+
gc.packRefs::
Running `git pack-refs` in a repository renders it
unclonable by Git versions prior to 1.5.1.2 over dumb
diff --git a/builtin/gc.c b/builtin/gc.c
index 331f21926..46edcff30 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -33,6 +33,7 @@ static int aggressive_window = 250;
static int gc_auto_threshold = 6700;
static int gc_auto_pack_limit = 50;
static int detach_auto = 1;
+static unsigned long gc_log_expire_time;
static const char *prune_expire = "2.weeks.ago";
static const char *prune_worktrees_expire = "3.months.ago";
@@ -76,10 +77,12 @@ static void git_config_date_string(const char *key, const char **output)
static void process_log_file(void)
{
struct stat st;
- if (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size)
+ if (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size) {
commit_lock_file(&log_lock);
- else
+ } else {
+ unlink(git_path("gc.log"));
rollback_lock_file(&log_lock);
+ }
}
static void process_log_file_at_exit(void)
@@ -111,6 +114,11 @@ static void gc_config(void)
git_config_get_int("gc.auto", &gc_auto_threshold);
git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit);
git_config_get_bool("gc.autodetach", &detach_auto);
+
+ if (!git_config_get_value("gc.logexpiry", &value)) {
+ parse_expiry_date(value, &gc_log_expire_time);
+ }
+
git_config_date_string("gc.pruneexpire", &prune_expire);
git_config_date_string("gc.worktreepruneexpire", &prune_worktrees_expire);
git_config(git_default_config, NULL);
@@ -290,19 +298,34 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
static int report_last_gc_error(void)
{
struct strbuf sb = STRBUF_INIT;
- int ret;
+ int ret = 0;
+ struct stat st;
+ char *gc_log_path = git_pathdup("gc.log");
- ret = strbuf_read_file(&sb, git_path("gc.log"), 0);
+ if (stat(gc_log_path, &st)) {
+ if (errno == ENOENT)
+ goto done;
+
+ ret = error(_("Can't read %s"), gc_log_path);
+ goto done;
+ }
+
+ if (st.st_mtime < gc_log_expire_time)
+ goto done;
+
+ ret = strbuf_read_file(&sb, gc_log_path, 0);
if (ret > 0)
- return error(_("The last gc run reported the following. "
+ ret = error(_("The last gc run reported the following. "
"Please correct the root cause\n"
"and remove %s.\n"
"Automatic cleanup will not be performed "
"until the file is removed.\n\n"
"%s"),
- git_path("gc.log"), sb.buf);
+ gc_log_path, sb.buf);
strbuf_release(&sb);
- return 0;
+done:
+ free(gc_log_path);
+ return ret;
}
static int gc_before_repack(void)
@@ -349,6 +372,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
argv_array_pushl(&prune_worktrees, "worktree", "prune", "--expire", NULL);
argv_array_pushl(&rerere, "rerere", "gc", NULL);
+ /* default expiry time, overwritten in gc_config */
+ parse_expiry_date("1.day", &gc_log_expire_time);
gc_config();
if (pack_refs < 0)
@@ -448,5 +473,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
warning(_("There are too many unreachable loose objects; "
"run 'git prune' to remove them."));
+ if (!daemonized)
+ unlink(git_path("gc.log"));
+
return 0;
}
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 1762dfa6a..84ad07eb2 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -67,5 +67,18 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre
test_line_count = 2 new # There is one new pack and its .idx
'
+test_expect_success 'background auto gc does not run if gc.log is present and recent but does if it is old' '
+ keep=$(ls .git/objects/pack/*.pack|head -1|sed -e "s/pack$/keep/") &&
+ test_commit foo &&
+ test_commit bar &&
+ git repack &&
+ test_config gc.autopacklimit 1 &&
+ test_config gc.autodetach true &&
+ echo fleem >.git/gc.log &&
+ test_must_fail git gc --auto 2>err &&
+ test_i18ngrep "^error:" err &&
+ test-chmtime =-86401 .git/gc.log &&
+ git gc --auto
+'
test_done
--
2.11.GIT
^ permalink raw reply related
* Re: [PATCH v2] gc: ignore old gc.log files
From: Junio C Hamano @ 2017-02-10 9:16 UTC (permalink / raw)
To: David Turner; +Cc: git, peff, pclouds
In-Reply-To: <20170209191724.3987-1-dturner@twosigma.com>
David Turner <dturner@twosigma.com> writes:
> The intent of automatic gc is to have a git repository be relatively
> low-maintenance from a server-operator perspective.
This is diametrically opposite from how I recall the auto-gc came
about in Sep 2007. The primary purpose was to help desktop clients
that never runs repack.
By pointing this out, I do not mean that we shouldn't make auto-gc
work well in the server settings. I however do not want our log
messages to distort history in order to justify a change that is
worth making, and I do not think this change needs to do that.
For example, a paragraph like this:
It would be really nice if the auto gc mechanism can be used to
help server operators, even though the original purpose it was
introduced was primarily to help desktop clients that never
repacks.
followed by a description of what makes it not exactly helpful for
server operators in the current behaviour (iow, "what is it that you
are fixing?"), would be a useful justification that is faithful to
the history. Of course, ", even though..." part is irrelevant
and/or unnecessary in that description of the motivation, and if you
omit it, I wouldn't call that is distorting the history.
> Of course, large
> operators like GitHub will need a more complicated management strategy,
> but for ordinary usage, git should just work.
True. "should just work" may want to be replaced by what exactly
are the things it currently does that you view as its problems.
Once you say that, "git learns to do x and y" in the next paragraph,
i.e. the description of the solution to the problem, starts making
sense.
>
> In this commit, git learns to ignore gc.log files which are older than
> (by default) one day old. It also learns about a config, gc.logExpiry
> to manage this. There is also some cleanup: a successful manual gc,
> or a warning-free auto gc with an old log file, will remove any old
> gc.log files.
>
> So git should never get itself into a state where it refuses to do any
> maintenance, just because at some point some piece of the maintenance
> didn't make progress. That might still happen (e.g. because the repo
> is corrupt), but at the very least it won't be because Git is too dumb
> to try again.
IOW, what you wrote in this last paragraph can come earlier to
explain what you perceive as problems the current behaviour has.
> Signed-off-by: David Turner <dturner@twosigma.com>
> Helped-by: Jeff King <peff@peff.net>
> ---
A v2 patch is unfriendly to reviewers unless it is sent with summary
of what got changed since v1, taking input from the discussion on
the previous round, and here before the diffstat is a good place to
do so.
> +gc.logExpiry::
> + If the file gc.log exists, then `git gc --auto` won't run
> + unless that file is more than 'gc.logExpiry' old. Default is
> + "1.day". See `gc.pruneExpire` for more possible values.
> +
Micronit. Perhaps you meant by "more possible values" "more ways to
specify its values", IOW, you didn't mean to say "instead of 1.day,
you can say 2.days".
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 331f21926..46edcff30 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -33,6 +33,7 @@ static int aggressive_window = 250;
> static int gc_auto_threshold = 6700;
> static int gc_auto_pack_limit = 50;
> static int detach_auto = 1;
> +static unsigned long gc_log_expire_time;
> static const char *prune_expire = "2.weeks.ago";
> static const char *prune_worktrees_expire = "3.months.ago";
>
> @@ -76,10 +77,12 @@ static void git_config_date_string(const char *key, const char **output)
> static void process_log_file(void)
> {
> struct stat st;
> - if (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size)
> + if (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size) {
> commit_lock_file(&log_lock);
> - else
> + } else {
> + unlink(git_path("gc.log"));
> rollback_lock_file(&log_lock);
After we grab a lock by creating gc.log.lock, if we fail to fstat(2),
we remove gc.log? That does not sound quite right, as the failure
to fstat(2) sounds like a log-worthy event. Removing the log after
noticing that we didn't write anything (i.e. st.st_size being 0) is
quite sensible, though.
> @@ -111,6 +114,11 @@ static void gc_config(void)
> git_config_get_int("gc.auto", &gc_auto_threshold);
> git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit);
> git_config_get_bool("gc.autodetach", &detach_auto);
> +
> + if (!git_config_get_value("gc.logexpiry", &value)) {
> + parse_expiry_date(value, &gc_log_expire_time);
> + }
Drop {}?
> @@ -290,19 +298,34 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
> static int report_last_gc_error(void)
> {
> struct strbuf sb = STRBUF_INIT;
> - int ret;
> + int ret = 0;
> + struct stat st;
> + char *gc_log_path = git_pathdup("gc.log");
>
> - ret = strbuf_read_file(&sb, git_path("gc.log"), 0);
> + if (stat(gc_log_path, &st)) {
> + if (errno == ENOENT)
> + goto done;
> +
> + ret = error(_("Can't read %s"), gc_log_path);
You probably want to use error_errno() instead here. This is not
"can't read"; your stat() noticed there is something wrong and you
gave up before you even attempted to read.
> + goto done;
> + }
> +
> + if (st.st_mtime < gc_log_expire_time)
> + goto done;
OK.
> + ret = strbuf_read_file(&sb, gc_log_path, 0);
> if (ret > 0)
> - return error(_("The last gc run reported the following. "
> + ret = error(_("The last gc run reported the following. "
> "Please correct the root cause\n"
> "and remove %s.\n"
> "Automatic cleanup will not be performed "
> "until the file is removed.\n\n"
> "%s"),
> - git_path("gc.log"), sb.buf);
> + gc_log_path, sb.buf);
> strbuf_release(&sb);
> - return 0;
> +done:
> + free(gc_log_path);
> + return ret;
> }
OK.
> @@ -349,6 +372,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
> argv_array_pushl(&prune_worktrees, "worktree", "prune", "--expire", NULL);
> argv_array_pushl(&rerere, "rerere", "gc", NULL);
>
> + /* default expiry time, overwritten in gc_config */
> + parse_expiry_date("1.day", &gc_log_expire_time);
Alternatively, we can mimick the way in which prune_expire and
prune_worktrees_expire are set up (i.e. they are kept as strings,
configuration overwrites the string), and then turn the final string
into value after gc_config() returns. I think what you wrote here
may be simpler. Nice.
> gc_config();
>
> if (pack_refs < 0)
> @@ -448,5 +473,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
> warning(_("There are too many unreachable loose objects; "
> "run 'git prune' to remove them."));
>
> + if (!daemonized)
> + unlink(git_path("gc.log"));
> +
OK. We want to remove "gc.log" after running a successful
foreground gc and this does exactly that.
^ permalink raw reply
* Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap
From: Michael Haggerty @ 2017-02-10 10:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, git, Jeff King
In-Reply-To: <xmqqh943p0hv.fsf@gitster.mtv.corp.google.com>
On 02/09/2017 09:34 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> [...]
>> +static int submodule_hash_cmp(const void *entry, const void *entry_or_key,
>> + const void *keydata)
>> +{
>> + const struct submodule_hash_entry *e1 = entry, *e2 = entry_or_key;
>> + const char *submodule = keydata;
>> +
>> + return strcmp(e1->submodule, submodule ? submodule : e2->submodule);
>
> I would have found it more readable if it were like so:
>
> const char *submodule = keydata ? keydata : e2->submodule;
>
> return strcmp(e1->submodule, submodule);
>
> but I suspect the difference is not that huge.
Yes, that's better. I'll change it.
On 02/10/2017 05:04 AM, Jeff King wrote:
> On Thu, Feb 09, 2017 at 12:34:04PM -0800, Junio C Hamano wrote:
>
>>> +static struct submodule_hash_entry *alloc_submodule_hash_entry(
>>> + const char *submodule, struct ref_store *refs)
>>> +{
>>> + size_t len = strlen(submodule);
>>> + struct submodule_hash_entry *entry = malloc(sizeof(*entry) + len + 1);
>>
>> I think this (and the later memcpy) is what FLEX_ALLOC_MEM() was
>> invented for.
>
> Yes, it was. Though since the length comes from a strlen() call, it can
> actually use the _STR variant, like:
>
> FLEX_ALLOC_STR(entry, submodule, submodule);
>
> Besides being shorter, this does integer-overflow checks on the final
> length.
Nice. TIL. Will fix.
>>> @@ -1373,16 +1405,17 @@ void base_ref_store_init(struct ref_store *refs,
>>> die("BUG: main_ref_store initialized twice");
>>>
>>> refs->submodule = "";
>>> - refs->next = NULL;
>>> main_ref_store = refs;
>>> } else {
>>> - if (lookup_ref_store(submodule))
>>> + refs->submodule = xstrdup(submodule);
>>> +
>>> + if (!submodule_ref_stores.tablesize)
>>> + hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);
>>
>> Makes me wonder what "20" stands for. Perhaps the caller should be
>> allowed to say "I do not quite care what initial size is" by passing
>> 0 or some equally but more clealy meaningless value (which of course
>> would be outside the scope of this series).
>
> I think this is what "0" already does (grep for HASHMAP_INITIAL_SIZE).
> In fact, that constant is 64. The 20 we pass in goes through some magic
> load-factor computation and ends up as 25. That being smaller than the
> INITIAL_SIZE constant, I believe that we end up allocating 64 entries
> either way (that's just from reading the code, though; I didn't run it
> to double check).
I guess I might as well change it to zero, then.
Thanks for the feedback!
Michael
^ permalink raw reply
* Re: [PATCH 0/5] Store submodules in a hash, not a linked list
From: Michael Haggerty @ 2017-02-10 10:27 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Stefan Beller, Johannes Schindelin, David Turner, git
In-Reply-To: <20170210004033.cgqmovhvoylad5cf@sigill.intra.peff.net>
On 02/10/2017 01:40 AM, Jeff King wrote:
> On Thu, Feb 09, 2017 at 10:23:35PM +0100, Michael Haggerty wrote:
>
>>>> So push the submodule attribute down to the `files_ref_store` class
>>>> (but continue to let the `ref_store`s be looked up by submodule).
>>>
>>> I'm not sure I understand all of the ramifications here. It _sounds_ like
>>> pushing this down into the files-backend code would make it harder to
>>> have mixed ref-backends for different submodules. Or is this just
>>> pushing down an implementation detail of the files backend, and future
>>> code is free to have as many different ref_stores as it likes?
>>
>> I don't understand how this would make it harder, aside from the fact
>> that a new backend class might also need a path member and have to
>> maintain its own copy rather than using one that the base class provides.
>
> Probably the answer is "I'm really confused".
>
> But here's how my line of reasoning went:
>
> Right now we have a main ref-store that points to the submodule
> ref-stores. I don't know the current state of it, but in theory those
> could all use different backends.
>
> This seems like it's pushing that submodule linkage down into the
> backend.
>
> But I think from your response that the answer is no, the thing that is
> being pushed down is not the right way for the main ref store and the
> submodules to be linked. In fact, there is no reason at all for the
> main ref store to know or care about submodules. Anybody who wants to
> know about a submodule's refs should ask the hashmap.
That's correct; the main ref store and submodule ref stores know nothing
of each other.
Michael
^ permalink raw reply
* [PATCH v2 2/9] refs: make some ref_store lookup functions private
From: Michael Haggerty @ 2017-02-10 11:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, Jeff King, git,
Michael Haggerty
In-Reply-To: <cover.1486724698.git.mhagger@alum.mit.edu>
The following functions currently don't need to be exposed:
* ref_store_init()
* lookup_ref_store()
That might change in the future, but for now make them private.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 19 +++++++++++++++++--
refs/refs-internal.h | 19 -------------------
2 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index 707092f..d7265cc 100644
--- a/refs.c
+++ b/refs.c
@@ -1358,7 +1358,15 @@ static struct ref_store *main_ref_store;
/* A linked list of ref_stores for submodules: */
static struct ref_store *submodule_ref_stores;
-struct ref_store *lookup_ref_store(const char *submodule)
+/*
+ * Return the ref_store instance for the specified submodule (or the
+ * main repository if submodule is NULL). If that ref_store hasn't
+ * been initialized yet, return NULL.
+ *
+ * For backwards compatibility, submodule=="" is treated the same as
+ * submodule==NULL.
+ */
+static struct ref_store *lookup_ref_store(const char *submodule)
{
struct ref_store *refs;
@@ -1373,7 +1381,14 @@ struct ref_store *lookup_ref_store(const char *submodule)
return NULL;
}
-struct ref_store *ref_store_init(const char *submodule)
+/*
+ * Create, record, and return a ref_store instance for the specified
+ * submodule (or the main repository if submodule is NULL).
+ *
+ * For backwards compatibility, submodule=="" is treated the same as
+ * submodule==NULL.
+ */
+static struct ref_store *ref_store_init(const char *submodule)
{
const char *be_name = "files";
struct ref_storage_be *be = find_ref_storage_backend(be_name);
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 708b260..d8a7eb1 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -653,25 +653,6 @@ void base_ref_store_init(struct ref_store *refs,
const char *submodule);
/*
- * Create, record, and return a ref_store instance for the specified
- * submodule (or the main repository if submodule is NULL).
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
- */
-struct ref_store *ref_store_init(const char *submodule);
-
-/*
- * Return the ref_store instance for the specified submodule (or the
- * main repository if submodule is NULL). If that ref_store hasn't
- * been initialized yet, return NULL.
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
- */
-struct ref_store *lookup_ref_store(const char *submodule);
-
-/*
* Return the ref_store instance for the specified submodule. For the
* main repository, use submodule==NULL; such a call cannot fail. For
* a submodule, the submodule must exist and be a nonbare repository,
--
2.9.3
^ permalink raw reply related
* [PATCH v2 0/9] Store submodules in a hash, not a linked list
From: Michael Haggerty @ 2017-02-10 11:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, Jeff King, git,
Michael Haggerty
This is v2 of the patch series, considerably reorganized but not that
different codewise. Thanks to Stefan, Junio, and Peff for their
feedback about v1 [1]. I think I have addressed all of your comments.
Changes since v1:
* Rebase from `master` onto `maint` (to match Junio).
* Reorder some commits to make the presentation more logical.
* Added an explicit preparatory commit that just reorders some
function definitions, because it makes the diff for the subsequent
commit a lot easier to read.
* Make some preexisting functions private:
* lookup_ref_store()
* ref_store_init()
* Remove some unnecessary handling of `submodule == ""` when it is
already known to have been converted to `NULL`. (Some of the
purported handling also happened to be broken.)
* Introduce function `register_ref_store()` in a separate step, before
switching to hashmaps.
* Don't initialize hashmap in `lookup_ref_store()`. (Just return
`NULL`; the hashmap will be initialized in `register_ref_store()` a
moment later.)
* Make code in `submodule_hash_cmp()` clearer.
* Use `FLEX_ALLOC_STR()` in `alloc_submodule_hash_entry()`.
* Don't specify an initial size for the submodule hashmap (the default
is OK).
This patch series is also available from my fork on GitHub [2] as
branch "submodule-hash".
Michael
[1] http://public-inbox.org/git/cover.1486629195.git.mhagger@alum.mit.edu/T/#u
[2] https://github.com/mhagger/git
Michael Haggerty (9):
refs: reorder some function definitions
refs: make some ref_store lookup functions private
refs: remove some unnecessary handling of submodule == ""
register_ref_store(): new function
refs: store submodule ref stores in a hashmap
refs: push the submodule attribute down
base_ref_store_init(): remove submodule argument
files_ref_store::submodule: use NULL for the main repository
read_loose_refs(): read refs using resolve_ref_recursively()
refs.c | 107 +++++++++++++++++++++++++++++++++++----------------
refs/files-backend.c | 77 +++++++++++++++++++++---------------
refs/refs-internal.h | 48 ++++-------------------
3 files changed, 127 insertions(+), 105 deletions(-)
--
2.9.3
^ 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