* Re: 'git submodules update' ignores credential.helper config of the parent repository
From: Jeff King @ 2017-02-28 20:08 UTC (permalink / raw)
To: Stefan Beller; +Cc: Dmitry Neverov, Duy Nguyen, Junio C Hamano, Git List
In-Reply-To: <CAGZ79kb8F9_9fd9uhfPpHVPQj-zm99qt5Tr=3TUhpe=K6JknEg@mail.gmail.com>
On Tue, Feb 28, 2017 at 10:05:24AM -0800, Stefan Beller wrote:
> > I have a feeling that something like this would create unwelcome corner
> > cases in the config-writer, which is otherwise does not have to care
> > about which existing section of a file it adds a key to.
>
> Yeah the writer would become a lot more involved, if we're not going
> the stupid way (add these sections for nearly all keys. that would be
> a mess but easy to implement)
>
> So I guess then we rather settle with multiple config files or a white/blacklist
> of config options to propagate from the superproject to its submodules.
I'm still open to the idea that we simply improve the documentation to
make it clear that per-repo config really is per-repo, and is not shared
between super-projects and submodules. And then something like Duy's
proposed conditional config lets you set global config that flexibly
covers a set of repos.
-Peff
^ permalink raw reply
* Re: [PATCH 0/6] Use time_t
From: Junio C Hamano @ 2017-02-28 18:55 UTC (permalink / raw)
To: René Scharfe; +Cc: Jeff King, Johannes Schindelin, git
In-Reply-To: <f6b57868-0173-48d9-86cb-79780f7e301b@web.de>
René Scharfe <l.s.r@web.de> writes:
> Am 28.02.2017 um 15:28 schrieb Jeff King:
>
>> It looks from the discussion like the sanest path forward is our own
>> signed-64bit timestamp_t. That's unfortunate compared to using the
>> standard time_t, but hopefully it would reduce the number of knobs (like
>> TIME_T_IS_INT64) in the long run.
>
> Glibc will get a way to enable 64-bit time_t on 32-bit platforms
> eventually
> (https://sourceware.org/glibc/wiki/Y2038ProofnessDesign). Can
> platforms that won't provide a 64-bit time_t by 2038 be actually used
> at that point? How would we get time information on them? How would
> a custom timestamp_t help us?
That's a sensible "wait, let's step back a bit". I take it that you
are saying "time_t is just fine", and I am inclined to agree.
Right now, they may be able to have future timestamps ranging to
year 2100 and switching to time_t would limit their ability to
express future time to 2038 but they would be able to express
timestamp in the past to cover most of 20th century. Given that
these 32-bit time_t software platforms will die off before year 2038
(either by underlying hardware getting obsolete, or software updated
to handle 64-bit time_t), the (temporary) loss of 2038-2100 range
would not be too big a deal to warrant additional complexity.
> Regarding the need for knobs: We could let the compiler chose between
> strtoll() and strtol() based on the size of time_t, in an inline
> function. The maximum value can be calculated using its size as
> well. And we could use PRIdMAX and cast to intmax_t for printing.
Thanks.
^ permalink raw reply
* Re: SHA1 collisions found
From: Shawn Pearce @ 2017-02-28 19:52 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Jeff King, Joey Hess, Git Mailing List
In-Reply-To: <CA+55aFxTWqsTTiDKo4DBZT-8Z9t80bGMD3uijzKONa_bYEZABQ@mail.gmail.com>
On Tue, Feb 28, 2017 at 11:34 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Feb 28, 2017 at 11:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> In a way similar to 8415558f55 ("sha1dc: avoid c99
>> declaration-after-statement", 2017-02-24), we would want this on
>> top.
>
> There's a few other simplifications that could be done:
Yes, I found and did a number of these when I ported sha1dc to Java
for JGit[1], and it helped recover some of the lost throughput.
[1] https://git.eclipse.org/r/#/c/91852/
> (1) make the symbols static that aren't used.
>
> The sha1.h header ends up declaring several things that shouldn't
> have been exported.
>
> I suspect the code may have had some debug mode that got stripped
> out from it before making it public (or that was never there, and was
> just something the generating code could add).
>
> (2) get rid of the "safe mode" support.
>
> That one is meant for non-checking replacements where it
> generates a *different* hash for input with the collision fingerpring,
> but that's pointless for the git use when we abort on a collision
> fingerprint.
>
> I think the first one will show that the sha1_compression() function
> isn't actually used, and with the removal of safe-mode I think
> sha1_compression_W() also is unused.
Correct.
> Finally, only states 58 and 65 (out of all 80 states) are actually
> used,
Yes, at present only states 58 and 65 are used. I cut out support for
other states.
> and from what I can tell, the 'maski' value is always 0, so the
> looping over 80 state masks is really just a loop over two.
Actually, look closer at that loop:
for (i = 0; sha1_dvs[i].dvType != 0; ++i)
{
if ((0 == ctx->ubc_check) || (((uint32_t)(1) << sha1_dvs[i].maskb)
& ubc_dv_mask[sha1_dvs[i].maski]))
Its a loop over all 32 bits looking for which bits are set. Most of
the time few bits if any are set for most message blocks. Changing
this code to find the lowest 1 bit set in ubc_dv_mask[0] provided a
significant improvement in throughput.
The sha1_dvs array is indexed by maskb, so the code can be reduced to:
while (ubcDvMask != 0) {
int b = numberOfTrailingZeros(lowestOneBit(ubcDvMask));
UbcCheck.DvInfo dv = UbcCheck.DV[b];
Or something.
^ permalink raw reply
* Re: SHA1 collisions found
From: Linus Torvalds @ 2017-02-28 19:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Joey Hess, Git Mailing List
In-Reply-To: <xmqq60jukubq.fsf@gitster.mtv.corp.google.com>
On Tue, Feb 28, 2017 at 11:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> In a way similar to 8415558f55 ("sha1dc: avoid c99
> declaration-after-statement", 2017-02-24), we would want this on
> top.
There's a few other simplifications that could be done:
(1) make the symbols static that aren't used.
The sha1.h header ends up declaring several things that shouldn't
have been exported.
I suspect the code may have had some debug mode that got stripped
out from it before making it public (or that was never there, and was
just something the generating code could add).
(2) get rid of the "safe mode" support.
That one is meant for non-checking replacements where it
generates a *different* hash for input with the collision fingerpring,
but that's pointless for the git use when we abort on a collision
fingerprint.
I think the first one will show that the sha1_compression() function
isn't actually used, and with the removal of safe-mode I think
sha1_compression_W() also is unused.
Finally, only states 58 and 65 (out of all 80 states) are actually
used, and from what I can tell, the 'maski' value is always 0, so the
looping over 80 state masks is really just a loop over two.
The file has code top *generate* all the 80 sha1_recompression_step()
functions, and I don't think the compiler is smart enough to notice
that only two of them matter.
And because 'maski' is always zero, thisL
ubc_dv_mask[sha1_dvs[i].maski]
code looks like it might as well just use ubc_dv_mask[0] - in fact the
ubc_dv_mask[] "array" really is just a single-entry array anyway:
#define DVMASKSIZE 1
so that code has a few oddities in it. It's generated code, which is
probably why.
Basically, some of it could be improved. In particular, the "generate
code for 80 different recompression cases, but only ever use two of
them" really looks like it would blow up the code generation footprint
a lot.
I'm adding Marc Stevens and Dan Shumow to this email (bcc'd, so that
they don't get dragged into any unrelated email threads) in case they
want to comment.
I'm wondering if they perhaps have a cleaned-up version somewhere, or
maybe they can tell me that I'm just full of sh*t and missed
something.
Linus
^ permalink raw reply
* [PATCH] Travis: also test on 32-bit Linux
From: Johannes Schindelin @ 2017-02-28 19:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Lars Schneider
When Git v2.9.1 was released, it had a bug that showed only on Windows
and on 32-bit systems: our assumption that `unsigned long` can hold
64-bit values turned out to be wrong.
This could have been caught earlier if we had a Continuous Testing
set up that includes a build and test run on 32-bit Linux.
Let's do this (and take care of the Windows build later). This patch
asks Travis CI to install a Docker image with 32-bit libraries and then
goes on to build and test Git using this 32-bit setup.
A big thank you to Lars Schneider without whose help this patch would
not have happened.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/travis-32-bit-v1
Fetch-It-Via: git fetch https://github.com/dscho/git travis-32-bit-v1
.travis.yml | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/.travis.yml b/.travis.yml
index 9c63c8c3f68..87d9e9051a6 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -39,6 +39,17 @@ env:
matrix:
include:
+ - env: Linux32
+ os: linux
+ compiler: clang
+ sudo: required
+ services:
+ - docker
+ before_install:
+ - docker pull daald/ubuntu32:xenial
+ before_script:
+ script:
+ - "sudo docker run -i -v \"${PWD}:/usr/src/git\" daald/ubuntu32:xenial /bin/bash -c \"linux32 --32bit i386 sh -c 'apt update && apt install -y build-essential libcurl4-openssl-dev libssl-dev libexpat-dev gettext python && cd /usr/src/git && DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS=\\\"--timer --jobs 3 --state=failed,slow,save\\\" GIT_TEST_OPTS=--verbose-log GIT_TEST_CLONE_2GB=YesPlease make -j2 test'\""
- env: Documentation
os: linux
compiler: clang
base-commit: 3bc53220cb2dcf709f7a027a3f526befd021d858
--
2.12.0.windows.1.3.g8a117c48243
^ permalink raw reply related
* Re: SHA1 collisions found
From: Junio C Hamano @ 2017-02-28 19:07 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, Joey Hess, Git Mailing List
In-Reply-To: <xmqqefyikvin.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>> [1/3]: add collision-detecting sha1 implementation
>> [2/3]: sha1dc: adjust header includes for git
>> [3/3]: Makefile: add USE_SHA1DC knob
>
> I was lazy so I fetched the above and then added this on top before
> I start to play with it.
>
> -- >8 --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Tue, 28 Feb 2017 10:39:25 -0800
> Subject: [PATCH] sha1dc: resurrect LICENSE file
In a way similar to 8415558f55 ("sha1dc: avoid c99
declaration-after-statement", 2017-02-24), we would want this on
top.
-- >8 --
Subject: sha1dc: avoid 'for' loop initial decl
We write this:
type i;
for (i = initial; i < limit; i++)
instead of this:
for (type i = initial; i < limit; i++)
the latter of which is from c99.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
sha1dc/sha1.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index f4e261ae7a..6569b403e9 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -41,7 +41,8 @@
void sha1_message_expansion(uint32_t W[80])
{
- for (unsigned i = 16; i < 80; ++i)
+ unsigned i;
+ for (i = 16; i < 80; ++i)
W[i] = rotate_left(W[i - 3] ^ W[i - 8] ^ W[i - 14] ^ W[i - 16], 1);
}
@@ -49,9 +50,10 @@ void sha1_compression(uint32_t ihv[5], const uint32_t m[16])
{
uint32_t W[80];
uint32_t a, b, c, d, e;
+ unsigned i;
memcpy(W, m, 16 * 4);
- for (unsigned i = 16; i < 80; ++i)
+ for (i = 16; i < 80; ++i)
W[i] = rotate_left(W[i - 3] ^ W[i - 8] ^ W[i - 14] ^ W[i - 16], 1);
a = ihv[0];
^ permalink raw reply related
* Re: SHA1 collisions found
From: Jeff King @ 2017-02-28 19:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Dan Shumow, Linus Torvalds, Joey Hess, Git Mailing List
In-Reply-To: <xmqq60jukubq.fsf@gitster.mtv.corp.google.com>
On Tue, Feb 28, 2017 at 11:07:37AM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> >> [1/3]: add collision-detecting sha1 implementation
> >> [2/3]: sha1dc: adjust header includes for git
> >> [3/3]: Makefile: add USE_SHA1DC knob
> >
> > I was lazy so I fetched the above and then added this on top before
> > I start to play with it.
> >
> > -- >8 --
> > From: Junio C Hamano <gitster@pobox.com>
> > Date: Tue, 28 Feb 2017 10:39:25 -0800
> > Subject: [PATCH] sha1dc: resurrect LICENSE file
>
> In a way similar to 8415558f55 ("sha1dc: avoid c99
> declaration-after-statement", 2017-02-24), we would want this on
> top.
>
> -- >8 --
> Subject: sha1dc: avoid 'for' loop initial decl
Yeah, thanks, I had tweaked both that and the license thing locally but
had not pushed it out yet. Both are obvious improvements.
FWIW, I've been in touch with Dan Shumow, one of the authors, who has
been looking at whether we can speed up the sha1dc implementation, or
integrate the checks into the block-sha1 implementation.
Here are a few notes on the earlier timings I posted that came out in
our conversation:
- the timings I showed earlier were actually openssl versus sha1dc.
The block-sha1 timings fall somewhere in the middle:
[running test-sha1 on a fresh linux.git packfile]
1.347s openssl
2.079s block-sha1
4.983s sha1dc
[index-pack --verify on a fresh git.git packfile]
6.919s openssl
9.003s block-sha1
17.955s sha1dc
Those are the operations that show off sha1 performance the most.
The first one is really not even that interesting; it's raw
sha1 performance. The second one is an actual operation users might
notice (though not as --verify exactly, but as "index-pack --stdin"
when receiving a fetch or a push).
So there's room for improvement, but the gap between block-sha1
and sha1dc is not quite as big as I showed earlier.
- Dan timed the sha1dc implementation with and without the collision
detection enabled. The sha1 implementation is only 1.33x slower than
block-sha1 (for raw sha1 time). Adding in the detection makes it
2.6x slower.
So there's some potential gain from optimizing the sha1
implementation, but ultimately we may be looking at a 2x slowdown to
add in the collision detection.
It doesn't need to happen for _every_ sha1 we compute, but the
index-pack case is the one that almost certainly _does_ want it,
because that's when we're admitting remote objects into the
repository (ditto you'd probably want it for write_sha1_file(),
since you could be applying a patch from an untrusted source).
-Peff
^ permalink raw reply
* Re: SHA1 collisions found
From: Junio C Hamano @ 2017-02-28 18:41 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, Joey Hess, Git Mailing List
In-Reply-To: <20170223230507.kuxjqtg3ghcfskc6@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The first one is 98K. Mail headers may bump it over vger's 100K barrier.
> It's actually the _least_ interesting patch of the 3, because it just
> imports the code wholesale from the other project. But if it doesn't
> make it, you can fetch the whole series from:
>
> https://github.com/peff/git jk/sha1dc
>
> (By the way, I don't see your version on the list, Linus, which probably
> means it was eaten by the 100K filter).
>
> [1/3]: add collision-detecting sha1 implementation
> [2/3]: sha1dc: adjust header includes for git
> [3/3]: Makefile: add USE_SHA1DC knob
I was lazy so I fetched the above and then added this on top before
I start to play with it.
-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 28 Feb 2017 10:39:25 -0800
Subject: [PATCH] sha1dc: resurrect LICENSE file
The upstream releases the contents under the MIT license; the
initial import accidentally omitted its license file.
Add it back.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
sha1dc/LICENSE | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
create mode 100644 sha1dc/LICENSE
diff --git a/sha1dc/LICENSE b/sha1dc/LICENSE
new file mode 100644
index 0000000000..4a3e6a1b15
--- /dev/null
+++ b/sha1dc/LICENSE
@@ -0,0 +1,30 @@
+MIT License
+
+Copyright (c) 2017:
+ Marc Stevens
+ Cryptology Group
+ Centrum Wiskunde & Informatica
+ P.O. Box 94079, 1090 GB Amsterdam, Netherlands
+ marc@marc-stevens.nl
+
+ Dan Shumow
+ Microsoft Research
+ danshu@microsoft.com
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in all
+copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+SOFTWARE.
--
2.12.0-310-g733d1cbbe2
^ permalink raw reply related
* Re: format-patch subject-prefix gets truncated when using the --numbered flag
From: Junio C Hamano @ 2017-02-28 17:42 UTC (permalink / raw)
To: Adrian Dudau; +Cc: git@vger.kernel.org
In-Reply-To: <1488297556.2955.11.camel@enea.com>
Adrian Dudau <Adrian.Dudau@enea.com> writes:
> I noticed that the --subject-prefix string gets truncated sometimes,
> but only when using the --numbered flat. Here's an example:
>
> addu@sestofb11:/data/fb/addu/git$ export longm="very very very very
> very very very very very very very very very very long prefix"
This looks like "dr, my arm hurts when I twist it this way---don't
do that then" ;-). Truncation is designed and intended to preserve
space for the real title of commit.
Having said that...
> This is happening on the latest master branch, so I dug through the
> code and tracked the issue to this piece of code in log-tree.c:
>
> if (opt->total > 0) {
> static char buffer[64];
> snprintf(buffer, sizeof(buffer),
> "Subject: [%s%s%0*d/%d] ",
> opt->subject_prefix,
> *opt->subject_prefix ? " " : "",
> digits_in_number(opt->total),
> opt->nr, opt->total);
> subject = buffer;
> } else if (opt->total == 0 && opt->subject_prefix && *opt-
>>subject_prefix) {
> static char buffer[256];
> snprintf(buffer, sizeof(buffer),
> "Subject: [%s] ",
> opt->subject_prefix);
> subject = buffer;
> } else {
> subject = "Subject: ";
> }
>
> Apparently the size of the "buffer" var is different in the two
> situations. Anybody knows if this is by design or just an old
> oversight?
I think this is just an old oversight. The latter should have been
even shorter than the former one (or the former should be longer
than the latter) to account for the width of the counter e.g. 01/27.
^ permalink raw reply
* Re: [PATCH] http: attempt updating base URL only if no error
From: Jonathan Tan @ 2017-02-28 18:48 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170228132814.wp3cq4ilp7syinqy@sigill.intra.peff.net>
On 02/28/2017 05:28 AM, Jeff King wrote:
> Right, your patch makes sense. A real HTTP error should take precedence
> over the url-update trickery.
>
> Acked-by: Jeff King <peff@peff.net>
Thanks!
> Running your included test, we get:
>
> fatal: unable to access 'http://127.0.0.1:5550/redir-to/502/': The
> requested URL returned error: 502
>
> but the error really happened in the intermediate step. I wonder if we
> should show the effective_url in that case, as it's more likely to
> pinpoint the problem. OTOH, we do not mention the intermediate redirect
> at all, so they might be confused about where that URL came from. If you
> really want to debug HTTP confusion, you should use GIT_TRACE_CURL.
Yeah, if we mention the effective_url, I think that there would need to
be a lot more explaining to be done (e.g. why does my URL have
"info/refs?service=git-upload-pack" tacked on at the end). It might be
better to just recommend GIT_TRACE_CURL.
^ permalink raw reply
* Re: 'git submodules update' ignores credential.helper config of the parent repository
From: Stefan Beller @ 2017-02-28 18:05 UTC (permalink / raw)
To: Jeff King; +Cc: Dmitry Neverov, Duy Nguyen, Junio C Hamano, Git List
In-Reply-To: <20170228143710.smbzo6b7wefjc62r@sigill.intra.peff.net>
On Tue, Feb 28, 2017 at 6:37 AM, Jeff King <peff@peff.net> wrote:
>>
>> This would change the semantics of a config file as the attribute for
>> each setting depends on the location (was attribute.FOO.read =
>> {true, false} read before).
>
> I'm not enthused by this, just because there is a hidden dependency
> between attribute.* sections and other ones. They _look_ like regular
> config keys, but they really aren't.
True.
> I have a feeling that something like this would create unwelcome corner
> cases in the config-writer, which is otherwise does not have to care
> about which existing section of a file it adds a key to.
Yeah the writer would become a lot more involved, if we're not going
the stupid way (add these sections for nearly all keys. that would be
a mess but easy to implement)
So I guess then we rather settle with multiple config files or a white/blacklist
of config options to propagate from the superproject to its submodules.
^ permalink raw reply
* Re: [PATCH v5 14/24] path.c: move some code out of strbuf_git_path_submodule()
From: Michael Haggerty @ 2017-02-28 18:14 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-15-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> refs is learning to avoid path rewriting that is done by
> strbuf_git_path_submodule(). Factor out this code so it could be reused
> by refs*
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> path.c | 34 +++++++---------------------------
> submodule.c | 31 +++++++++++++++++++++++++++++++
> submodule.h | 1 +
> 3 files changed, 39 insertions(+), 27 deletions(-)
>
> diff --git a/path.c b/path.c
> index efcedafba..3451d2916 100644
> --- a/path.c
> +++ b/path.c
> @@ -475,35 +475,16 @@ const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...)
> static int do_submodule_path(struct strbuf *buf, const char *path,
> const char *fmt, va_list args)
> {
> - const char *git_dir;
> struct strbuf git_submodule_common_dir = STRBUF_INIT;
> struct strbuf git_submodule_dir = STRBUF_INIT;
> - const struct submodule *sub;
> - int err = 0;
> + int ret;
>
> - strbuf_addstr(buf, path);
> - strbuf_complete(buf, '/');
> - strbuf_addstr(buf, ".git");
> -
> - git_dir = read_gitfile(buf->buf);
> - if (git_dir) {
> - strbuf_reset(buf);
> - strbuf_addstr(buf, git_dir);
> - }
> - if (!is_git_directory(buf->buf)) {
> - gitmodules_config();
> - sub = submodule_from_path(null_sha1, path);
> - if (!sub) {
> - err = SUBMODULE_PATH_ERR_NOT_CONFIGURED;
I didn't read this patch too carefully, but where the old code used the
constant `SUBMODULE_PATH_ERR_NOT_CONFIGURED`, the new code returns -1.
In fact, now the constant is totally unused. It looks like there's some
more cleanup that could happen.
> - goto cleanup;
> - }
> - strbuf_reset(buf);
> - strbuf_git_path(buf, "%s/%s", "modules", sub->name);
> - }
> -
> - strbuf_addch(buf, '/');
> - strbuf_addbuf(&git_submodule_dir, buf);
> + ret = submodule_to_gitdir(&git_submodule_dir, path);
> + if (ret)
> + goto cleanup;
>
> + strbuf_complete(&git_submodule_dir, '/');
> + strbuf_addbuf(buf, &git_submodule_dir);
> strbuf_vaddf(buf, fmt, args);
>
> if (get_common_dir_noenv(&git_submodule_common_dir, git_submodule_dir.buf))
> @@ -514,8 +495,7 @@ static int do_submodule_path(struct strbuf *buf, const char *path,
> cleanup:
> strbuf_release(&git_submodule_dir);
> strbuf_release(&git_submodule_common_dir);
> -
> - return err;
> + return ret;
> }
>
> char *git_pathdup_submodule(const char *path, const char *fmt, ...)
> diff --git a/submodule.c b/submodule.c
> index 3b98766a6..36b8d1d11 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1514,3 +1514,34 @@ void absorb_git_dir_into_superproject(const char *prefix,
> strbuf_release(&sb);
> }
> }
> +
> +int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
> +{
> + const struct submodule *sub;
> + const char *git_dir;
> + int ret = 0;
> +
> + strbuf_reset(buf);
> + strbuf_addstr(buf, submodule);
> + strbuf_complete(buf, '/');
> + strbuf_addstr(buf, ".git");
> +
> + git_dir = read_gitfile(buf->buf);
> + if (git_dir) {
> + strbuf_reset(buf);
> + strbuf_addstr(buf, git_dir);
> + }
> + if (!is_git_directory(buf->buf)) {
> + gitmodules_config();
> + sub = submodule_from_path(null_sha1, submodule);
> + if (!sub) {
> + ret = -1;
> + goto cleanup;
> + }
> + strbuf_reset(buf);
> + strbuf_git_path(buf, "%s/%s", "modules", sub->name);
> + }
> +
> +cleanup:
> + return ret;
> +}
> diff --git a/submodule.h b/submodule.h
> index 05ab674f0..fc3d0303e 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -81,6 +81,7 @@ extern int push_unpushed_submodules(struct sha1_array *commits,
> int dry_run);
> extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
> extern int parallel_submodules(void);
> +int submodule_to_gitdir(struct strbuf *buf, const char *submodule);
A docstring is always nice :-)
>
> /*
> * Prepare the "env_array" parameter of a "struct child_process" for executing
>
Michael
^ permalink raw reply
* Re: [PATCH v5 00/24] Remove submodule from files-backend.c
From: Michael Haggerty @ 2017-02-28 18:20 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-1-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> v5 goes a bit longer than v4, basically:
I've read through patch 14/24 so far, and they all look good except for
the mostly superficial comments that I have sent so far. I like the way
this is heading!
I'll try to continue tomorrow.
Michael
^ permalink raw reply
* Re: git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Torsten Bögershausen @ 2017-02-28 18:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Mike Crowe
In-Reply-To: <xmqqefyjwfql.fsf@gitster.mtv.corp.google.com>
On 2017-02-27 21:17, Junio C Hamano wrote:
> Torsten, you've been quite active in fixing various glitches around
> the EOL conversion in the latter half of last year. Have any
> thoughts to share on this topic?
>
> Thanks.
Sorry for the delay, being too busy with other things.
I followed the discussion, but didn't have good things to contribute.
I am not an expert in diff.c, but there seems to be a bug, thanks everybody
for digging.
Back to business:
My understanding is that git diff --quiet should be quiet, when
git add will not do anything (but the file is "touched".
The touched means that Git will detect e.g a new mtime or inode
or file size when doing lstat().
mtime is tricky, inodes are problematic under Windows.
What is easy to change is the file length.
I don't thing that we need a test file with LF, nor do we need
the sleep, touch or anything.
Would the the following work ?
(This is copy-paste, so the TABs may be corrupted)
#!/bin/sh
#
# Copyright (c) 2017 Mike Crowe
#
# These tests ensure that files changing line endings in the presence
# of .gitattributes to indicate that line endings should be ignored
# don't cause 'git diff' or 'git diff --quiet' to think that they have
# been changed.
test_description='git diff with files that require CRLF conversion'
. ./test-lib.sh
test_expect_success setup '
echo "* text=auto" > .gitattributes &&
printf "Hello\r\nWorld\r\n" >crlf.txt &&
git add .gitattributes crlf.txt lf.txt &&
git commit -m "initial"
'
test_expect_success 'quiet diff works on file with line-ending change that has
no effect on repository' '
printf "Hello\r\nWorld\n" >crlf.txt &&
git status &&
git diff --quiet
'
test_done
^ permalink raw reply
* Re: [PATCH v5 13/24] refs.c: make get_main_ref_store() public and use it
From: Michael Haggerty @ 2017-02-28 18:06 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-14-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> get_ref_store() will soon be renamed to get_submodule_ref_store().
> Together with future get_worktree_ref_store(), the three functions
> provide an appropriate ref store for different operation modes. New APIs
> will be added to operate directly on ref stores.
Nice.
Michael
^ permalink raw reply
* Re: [PATCH v5 12/24] refs.c: kill register_ref_store(), add register_submodule_ref_store()
From: Michael Haggerty @ 2017-02-28 18:03 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-13-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> This is the last function in this code (besides public API) that takes
> submodule argument and handles both main/submodule cases. Break it down,
> move main store registration in get_main_ref_store() and keep the rest
> in register_submodule_ref_store().
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> refs.c | 50 ++++++++++++++++++++++++++------------------------
> 1 file changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index c284cb4f4..6adc38e42 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1412,29 +1412,6 @@ static struct ref_store *lookup_submodule_ref_store(const char *submodule)
> }
>
> /*
> - * Register the specified ref_store to be the one that should be used
> - * for submodule (or the main repository if submodule is NULL). It is
> - * a fatal error to call this function twice for the same submodule.
> - */
> -static void register_ref_store(struct ref_store *refs, const char *submodule)
> -{
> - if (!submodule) {
> - if (main_ref_store)
> - die("BUG: main_ref_store initialized twice");
> -
> - main_ref_store = refs;
> - } else {
> - if (!submodule_ref_stores.tablesize)
> - hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 0);
> -
> - if (hashmap_put(&submodule_ref_stores,
> - alloc_submodule_hash_entry(submodule, refs)))
> - die("BUG: ref_store for submodule '%s' initialized twice",
> - submodule);
> - }
> -}
> -
> -/*
> * Create, record, and return a ref_store instance for the specified
> * submodule (or the main repository if submodule is NULL).
> */
> @@ -1448,7 +1425,6 @@ static struct ref_store *ref_store_init(const char *submodule)
> die("BUG: reference backend %s is unknown", be_name);
>
> refs = be->init(submodule);
> - register_ref_store(refs, submodule);
> return refs;
> }
>
> @@ -1460,9 +1436,32 @@ static struct ref_store *get_main_ref_store(void)
> return main_ref_store;
>
> refs = ref_store_init(NULL);
> + if (refs) {
> + if (main_ref_store)
> + die("BUG: main_ref_store initialized twice");
> +
> + main_ref_store = refs;
> + }
> return refs;
Superfluous test: I don't think `ref_store_init()` ever returns NULL.
This also implies that you could check `main_ref_store` before calling
`ref_store_init()`, and eliminate a temporary.
> }
>
> +/*
> + * Register the specified ref_store to be the one that should be used
> + * for submodule. It is a fatal error to call this function twice for
> + * the same submodule.
> + */
> +static void register_submodule_ref_store(struct ref_store *refs,
> + const char *submodule)
> +{
> + if (!submodule_ref_stores.tablesize)
> + hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 0);
> +
> + if (hashmap_put(&submodule_ref_stores,
> + alloc_submodule_hash_entry(submodule, refs)))
> + die("BUG: ref_store for submodule '%s' initialized twice",
> + submodule);
> +}
> +
> struct ref_store *get_ref_store(const char *submodule)
> {
> struct strbuf submodule_sb = STRBUF_INIT;
> @@ -1480,6 +1479,9 @@ struct ref_store *get_ref_store(const char *submodule)
> if (is_nonbare_repository_dir(&submodule_sb))
> refs = ref_store_init(submodule);
> strbuf_release(&submodule_sb);
> +
> + if (refs)
I think `refs` should always be non-NULL here for the same reason.
> + register_submodule_ref_store(refs, submodule);
> return refs;
> }
Michael
^ permalink raw reply
* Re: [PATCH v5 08/24] files-backend: remove the use of git_path()
From: Michael Haggerty @ 2017-02-28 17:50 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-9-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> Given $GIT_DIR and $GIT_COMMON_DIR, files-backend is now in charge of
> deciding what goes where (*). The end goal is to pass $GIT_DIR only. A
> refs "view" of a linked worktree is a logical ref store that combines
> two files backends together.
>
> (*) Not entirely true since strbuf_git_path_submodule() still does path
> translation underneath. But that's for another patch.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> refs/files-backend.c | 43 ++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 72f4e1746..a390eaadf 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -923,7 +923,8 @@ struct files_ref_store {
> * store:
> */
> const char *submodule;
> -
> + char *gitdir;
> + char *gitcommondir;
> char *packed_refs_path;
>
> struct ref_entry *loose;
> @@ -985,6 +986,8 @@ static struct ref_store *files_ref_store_create(const char *submodule)
> {
> struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
> struct ref_store *ref_store = (struct ref_store *)refs;
> + struct strbuf sb = STRBUF_INIT;
> + const char *gitdir = get_git_dir();
>
> base_ref_store_init(ref_store, &refs_be_files);
>
> @@ -995,7 +998,11 @@ static struct ref_store *files_ref_store_create(const char *submodule)
> return ref_store;
> }
>
> - refs->packed_refs_path = git_pathdup("packed-refs");
> + refs->gitdir = xstrdup(gitdir);
> + get_common_dir_noenv(&sb, gitdir);
> + refs->gitcommondir = strbuf_detach(&sb, NULL);
> + strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
> + refs->packed_refs_path = strbuf_detach(&sb, NULL);
`git_path()` and friends avoid adding an extra `/` if `git_dir()`
already ends in a slash or if it is the empty string. Here you don't
have that functionality. Is that intentional?
Same thing below, too.
>
> return ref_store;
> }
> @@ -1173,11 +1180,26 @@ static void files_reflog_path(struct files_ref_store *refs,
> const char *refname)
> {
> if (!refname) {
> - strbuf_git_path(sb, "logs");
> + /*
> + * FIXME: of course this is wrong in multi worktree
> + * setting. To be fixed real soon.
> + */
> + strbuf_addf(sb, "%s/logs", refs->gitcommondir);
> return;
> }
>
> - strbuf_git_path(sb, "logs/%s", refname);
> + switch (ref_type(refname)) {
> + case REF_TYPE_PER_WORKTREE:
> + case REF_TYPE_PSEUDOREF:
> + strbuf_addf(sb, "%s/logs/%s", refs->gitdir, refname);
> + break;
> + case REF_TYPE_NORMAL:
> + strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname);
> + break;
> + default:
> + die("BUG: unknown ref type %d of ref %s",
> + ref_type(refname), refname);
> + }
> }
>
> static void files_refname_path(struct files_ref_store *refs,
> @@ -1189,7 +1211,18 @@ static void files_refname_path(struct files_ref_store *refs,
> return;
> }
>
> - strbuf_git_path(sb, "%s", refname);
> + switch (ref_type(refname)) {
> + case REF_TYPE_PER_WORKTREE:
> + case REF_TYPE_PSEUDOREF:
> + strbuf_addf(sb, "%s/%s", refs->gitdir, refname);
> + break;
> + case REF_TYPE_NORMAL:
> + strbuf_addf(sb, "%s/%s", refs->gitcommondir, refname);
> + break;
> + default:
> + die("BUG: unknown ref type %d of ref %s",
> + ref_type(refname), refname);
> + }
> }
>
> /*
>
Michael
^ permalink raw reply
* Re: [PATCH v5 09/24] refs.c: introduce get_main_ref_store()
From: Michael Haggerty @ 2017-02-28 17:51 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-10-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> refs.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 81b64b4ed..dab1a21ac 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1456,15 +1456,23 @@ static struct ref_store *ref_store_init(const char *submodule)
> return refs;
> }
>
> +static struct ref_store *get_main_ref_store(void)
> +{
> + struct ref_store *refs;
> +
> + if (main_ref_store)
> + return main_ref_store;
> +
> + refs = ref_store_init(NULL);
> + return refs;
Unnecessary temporary variable?
> [...]
Michael
^ permalink raw reply
* Re: gpg verify git sub modules useful?
From: Junio C Hamano @ 2017-02-28 17:36 UTC (permalink / raw)
To: Patrick Schleizer; +Cc: git, Whonix-devel
In-Reply-To: <a9da4416-6437-a491-f461-c5e61d805b2d@riseup.net>
Patrick Schleizer <patrick-mailinglists@whonix.org> writes:
> When using git submodules, is there value in iterating about the git
> submodules running "git verfiy-commit HEAD" or would that be already
> covered by the git submodule verification?
That depends on what you are referring to with the "git submodule
verification" and more importantly what threat you are guarding
against. "git -C <submodule-dir> verify-commit HEAD" may make sure
that the contents of that commit object is GPG signed by whoever you
trust--is that what you want to make sure? Or do you want all
commits in the submodule history to be similarly signed because the
tree of the superproject can switch to some other commit there?
^ permalink raw reply
* Re: [PATCH 1/2] docs/diffcore: fix grammar in diffcore-rename header
From: Junio C Hamano @ 2017-02-28 17:32 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Patrick Steinhardt
In-Reply-To: <2882e77a58e4219d60a39827c3ea8d4537d5178a.1488272203.git.patrick.steinhardt@elego.de>
Patrick Steinhardt <patrick.steinhardt@elego.de> writes:
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> Documentation/gitdiffcore.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt
> index 46bc6d077..cf009a187 100644
> --- a/Documentation/gitdiffcore.txt
> +++ b/Documentation/gitdiffcore.txt
> @@ -119,7 +119,7 @@ the original is used), and can be customized by giving a number
> after "-B" option (e.g. "-B75" to tell it to use 75%).
>
>
> -diffcore-rename: For Detection Renames and Copies
> +diffcore-rename: For Detecting Renames and Copies
> -------------------------------------------------
>
> This transformation is used to detect renames and copies, and is
Thanks for carefully reading. Both looks reasonable.
^ permalink raw reply
* Re: [PATCH v5 07/24] files-backend: add and use files_refname_path()
From: Michael Haggerty @ 2017-02-28 17:41 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-8-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> Keep repo-related path handling in one place. This will make it easier
> to add submodule/multiworktree support later.
>
> This automatically adds the "if submodule then use the submodule version
> of git_path" to other call sites too. But it does not mean those
> operations are sumodule-ready. Not yet.
s/sumodule/submodule/
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> refs/files-backend.c | 45 +++++++++++++++++++++++++--------------------
> 1 file changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 7b4ea4c56..72f4e1746 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1180,6 +1180,18 @@ static void files_reflog_path(struct files_ref_store *refs,
> strbuf_git_path(sb, "logs/%s", refname);
> }
>
> +static void files_refname_path(struct files_ref_store *refs,
> + struct strbuf *sb,
> + const char *refname)
> +{
> + if (refs->submodule) {
> + strbuf_git_path_submodule(sb, refs->submodule, "%s", refname);
> + return;
> + }
> +
> + strbuf_git_path(sb, "%s", refname);
> +}
Maybe it's just me, but I find it odd to exit early here when the first
exit isn't due to an error. For me, structuring this like `if ()
call1(); else call2();` would make it clearer that the two code paths
are equally-valid alternatives, and either one or the other will be
executed.
I had the same feeling when I read `files_reflog_path()`
> /*
> * Get the packed_ref_cache for the specified files_ref_store,
> * creating it if necessary.
> @@ -1251,10 +1263,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
> size_t path_baselen;
> int err = 0;
>
> - if (refs->submodule)
> - err = strbuf_git_path_submodule(&path, refs->submodule, "%s", dirname);
> - else
> - strbuf_git_path(&path, "%s", dirname);
> + files_refname_path(refs, &path, dirname);
It's so nice to see these ugly `if (refs->submodule)` code blocks
disappearing!
> [...]
Michael
^ permalink raw reply
* Re: [PATCH v5 24/24] t1406: new tests for submodule ref store
From: Michael Haggerty @ 2017-02-28 17:34 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-25-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> t/t1406-submodule-ref-store.sh (new +x) | 95 +++++++++++++++++++++++++++++++++
> 1 file changed, 95 insertions(+)
> create mode 100755 t/t1406-submodule-ref-store.sh
>
> diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
> new file mode 100755
> index 000000000..3b30ba62f
> --- /dev/null
> +++ b/t/t1406-submodule-ref-store.sh
> [...]
I haven't actually read this far in the patch series, but I noticed that
a test in this file fails:
t1406-submodule-ref-store.sh (Wstat: 256 Tests: 15
Failed: 1)
Failed test: 10
Non-zero exit status: 1
I didn't have time to look into it more; let me know if you can't
reproduce it.
Michael
^ permalink raw reply
* Re: [PATCH 0/6] Use time_t
From: Junio C Hamano @ 2017-02-28 17:26 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
In-Reply-To: <20170228142802.hu5esthnqdsgc2po@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I do not just agree, but I think the move to a signed timestamp is a big
> improvement. Git's object format is happy to represent times before
> 1970, but the code is not. I know this has been a pain for people who
> import ancient histories into Git.
>
> It looks from the discussion like the sanest path forward is our own
> signed-64bit timestamp_t. That's unfortunate compared to using the
> standard time_t, but hopefully it would reduce the number of knobs (like
> TIME_T_IS_INT64) in the long run.
Keeping it unsigned is safer in the short-term. There are some
places that uses 0 as "impossible time" (e.g. somebody tried to
parse a string as time and returns a failure) and these places need
to be found and be replaced with probably the most negative value
that timestamp_t cn represent. Another possible special value we
may use is for "expiring everything" but I think we tend to just use
the timestamp of the present time for that purpose and not UONG_MAX,
so we should be OK there.
But we need to cross the bridge to signed timestamp sometime, and I
do not see any reason why that somtime should not be now.
^ permalink raw reply
* Re: [PATCH v5 05/24] files-backend: move "logs/" out of TMP_RENAMED_LOG
From: Michael Haggerty @ 2017-02-28 17:19 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-6-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> This makes reflog path building consistent, always in the form of
>
> strbuf_git_path(sb, "logs/%s", refname);
>
> It reduces the mental workload a bit in the next patch when that
> function call is converted.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> refs/files-backend.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 435db1293..69946b0de 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2513,7 +2513,7 @@ static int files_delete_refs(struct ref_store *ref_store,
> * IOW, to avoid cross device rename errors, the temporary renamed log must
> * live into logs/refs.
> */
> -#define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log"
> +#define TMP_RENAMED_LOG "refs/.tmp-renamed-log"
The constant name feels a little bit misleading now that it is not the
name of a logfile but rather a reference name. OTOH "tmp-renamed-log" is
*in* the reference name so I guess it's not really wrong.
> struct rename_cb {
> const char *tmp_renamed_log;
> @@ -2549,7 +2549,7 @@ static int rename_tmp_log(const char *newrefname)
> int ret;
>
> strbuf_git_path(&path, "logs/%s", newrefname);
> - strbuf_git_path(&tmp, TMP_RENAMED_LOG);
> + strbuf_git_path(&tmp, "logs/%s", TMP_RENAMED_LOG);
> cb.tmp_renamed_log = tmp.buf;
> ret = raceproof_create_file(path.buf, rename_tmp_log_callback, &cb);
> if (ret) {
> @@ -2626,12 +2626,12 @@ static int files_rename_ref(struct ref_store *ref_store,
> return 1;
>
> strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
> - strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
> + strbuf_git_path(&tmp_renamed_log, "logs/%s", TMP_RENAMED_LOG);
> ret = log && rename(sb_oldref.buf, tmp_renamed_log.buf);
> strbuf_release(&sb_oldref);
> strbuf_release(&tmp_renamed_log);
> if (ret)
> - return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
> + return error("unable to move logfile logs/%s to logs/"TMP_RENAMED_LOG": %s",
> oldrefname, strerror(errno));
It seems like it would be preferable to use `sb_oldref.buf` and
`tmp.buf` when building the error message. But I guess that `tmp.buf`
might include some path preceding "logs/" that is unwanted in the error
message? But it's a shame to hardcode the file naming scheme here again.
Maybe we *do* want the path in the error message?
It just occurred to me: this temporary logfile lives in the main
repository, right? What if a worktree reference is being renamed? Part
of the advertised use of worktrees is that the worktree might live far
from the main directory, or even on removable media. But it's not
possible to rename files across partitions. Maybe this will come out in
the wash once worktrees are ref_stores themselves.
For that matter, what if a user tries to rename a worktree ref into a
common ref or vice versa?
> if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
> [...]
Michael
^ permalink raw reply
* Re: [PATCH v5 04/24] files-backend: convert git_path() to strbuf_git_path()
From: Michael Haggerty @ 2017-02-28 17:06 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-5-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> git_path() and friends are going to be killed in files-backend.c in near
> future. And because there's a risk with overwriting buffer in
> git_path(), let's convert them all to strbuf_git_path(). We'll have
> easier time killing/converting strbuf_git_path() then because we won't
> have to worry about memory management again.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> refs/files-backend.c | 139 +++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 106 insertions(+), 33 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 4676525de..435db1293 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> [...]
> @@ -2586,9 +2603,15 @@ static int files_rename_ref(struct ref_store *ref_store,
> int flag = 0, logmoved = 0;
> struct ref_lock *lock;
> struct stat loginfo;
> - int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
> + struct strbuf sb_oldref = STRBUF_INIT;
> + struct strbuf sb_newref = STRBUF_INIT;
> + struct strbuf tmp_renamed_log = STRBUF_INIT;
> + int log, ret;
> struct strbuf err = STRBUF_INIT;
>
> + strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
> + log = !lstat(sb_oldref.buf, &loginfo);
> + strbuf_release(&sb_oldref);
> if (log && S_ISLNK(loginfo.st_mode))
> return error("reflog for %s is a symlink", oldrefname);
>
> @@ -2602,7 +2625,12 @@ static int files_rename_ref(struct ref_store *ref_store,
> if (!rename_ref_available(oldrefname, newrefname))
> return 1;
>
> - if (log && rename(git_path("logs/%s", oldrefname), git_path(TMP_RENAMED_LOG)))
> + strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
> + strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
> + ret = log && rename(sb_oldref.buf, tmp_renamed_log.buf);
> + strbuf_release(&sb_oldref);
> + strbuf_release(&tmp_renamed_log);
> + if (ret)
> return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
> oldrefname, strerror(errno));
>
> @@ -2681,13 +2709,19 @@ static int files_rename_ref(struct ref_store *ref_store,
> log_all_ref_updates = flag;
>
> rollbacklog:
> - if (logmoved && rename(git_path("logs/%s", newrefname), git_path("logs/%s", oldrefname)))
> + strbuf_git_path(&sb_newref, "logs/%s", newrefname);
> + strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
> + if (logmoved && rename(sb_newref.buf, sb_oldref.buf))
> error("unable to restore logfile %s from %s: %s",
> oldrefname, newrefname, strerror(errno));
> + strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
> if (!logmoved && log &&
> - rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", oldrefname)))
> + rename(tmp_renamed_log.buf, sb_oldref.buf))
> error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
> oldrefname, strerror(errno));
It feels like you're writing, releasing, re-writing these strbufs more
than necessary. Maybe it would be clearer to set them once, and on
errors set `ret = error()` then jump to a label here...
> + strbuf_release(&sb_newref);
> + strbuf_release(&sb_oldref);
> + strbuf_release(&tmp_renamed_log);
>
...and change this to `return ret`?
> return 1;
> }
> [...]
> @@ -4108,18 +4171,28 @@ static int files_reflog_expire(struct ref_store *ref_store,
>
> static int files_init_db(struct ref_store *ref_store, struct strbuf *err)
> {
> + struct strbuf sb = STRBUF_INIT;
> +
> /* Check validity (but we don't need the result): */
> files_downcast(ref_store, 0, "init_db");
>
> /*
> * Create .git/refs/{heads,tags}
> */
> - safe_create_dir(git_path("refs/heads"), 1);
> - safe_create_dir(git_path("refs/tags"), 1);
> + strbuf_git_path(&sb, "refs/heads");
> + safe_create_dir(sb.buf, 1);
> + strbuf_reset(&sb);
> + strbuf_git_path(&sb, "refs/tags");
> + safe_create_dir(sb.buf, 1);
> + strbuf_reset(&sb);
> if (get_shared_repository()) {
> - adjust_shared_perm(git_path("refs/heads"));
> - adjust_shared_perm(git_path("refs/tags"));
> + strbuf_git_path(&sb, "refs/heads");
> + adjust_shared_perm(sb.buf);
> + strbuf_reset(&sb);
> + strbuf_git_path(&sb, "refs/tags");
> + adjust_shared_perm(sb.buf);
> }
> + strbuf_release(&sb);
> return 0;
> }
It looks to me like `safe_create_dir()` already has the ability to
`adjust_shared_perm()`, or am I missing something? (I realize that this
is preexisting code.)
Michael
^ 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