* Re: What's cooking in git.git (Sep 2016, #07; Fri, 23)
From: Johannes Schindelin @ 2016-10-04 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqd1jpkkea.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 27 Sep 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > In your previous kitchen status ("What's cooking") you hinted at a
> > possible v2.10.1 soon. I have a couple of bugfixes lined up for Git
> > for Windows and would like to avoid unnecessarily frequent release
> > engineering... Any more concrete ideas on a date for this version?
>
> I scanned RelNotes for 2.11 and identified these topics that we'd want
> to have in 'maint'.
>
> bw/pathspec-remove-unused-extern-decl # 1 (6 days ago)
> rs/checkout-some-states-are-const # 1 (6 days ago)
> rs/strbuf-remove-fix # 1 (6 days ago)
> rs/unpack-trees-reduce-file-scope-global # 1 (6 days ago)
> mr/vcs-svn-printf-ulong # 1 (6 days ago)
> sy/git-gui-i18n-ja # 7 (12 days ago)
> jk/fix-remote-curl-url-wo-proto # 1 (12 days ago)
> js/git-gui-commit-gpgsign # 2 (12 days ago)
> jk/patch-ids-no-merges # 2 (6 days ago)
> ew/http-do-not-forget-to-call-curl-multi-remove-handle # 3 (6 days ago)
> rs/xdiff-merge-overlapping-hunks-for-W-context # 1 (6 days ago)
> ks/perf-build-with-autoconf # 1 (6 days ago)
> jt/format-patch-base-info-above-sig # 1 (6 days ago)
> jk/rebase-i-drop-ident-check # 1 (6 days ago)
> jk/reduce-gc-aggressive-depth # 1 (6 days ago)
> et/add-chmod-x # 1 (6 days ago)
> tg/add-chmod+x-fix # 7 (24 hours ago)
>
> Most are internal clean-ups that I do not mind leaving out, but I
> think we want to have that "add --chmod=+x" fix in. As it hasn't
> been enough time passed since the topic was merged to 'master', I'd
> say either
>
> (1) 2.10.1 with everything other than the last two in a few days
> and 2.10.2 late next week with "add --chmod=+x" fix, or
>
> (2) just a single 2.10.1 with everything late next week.
>
> I can go either way and welcome suggestions. I'd start merging
> older topics in the above list to 'maint' soonish, but not today.
Sorry for the delay in answering. By now, it was probably obvious to you
that (2) was my preference ;-)
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH 18/18] alternates: use fspathcmp to detect duplicates
From: Jacob Keller @ 2016-10-04 6:51 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203626.styj2vwcmgwnpx4v@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:36 PM, Jeff King <peff@peff.net> wrote:
> On a case-insensitive filesystem, we should realize that
> "a/objects" and "A/objects" are the same path. We already
> use fspathcmp() to check against the main object directory,
> but until recently we couldn't use it for comparing against
> other alternates (because their paths were not
> NUL-terminated strings). But now we can, so let's do so.
>
Yep, makes sense.
> Note that we also need to adjust count-objects to load the
> config, so that it can see the setting of core.ignorecase
> (this is required by the test, but is also a general bugfix
> for users of count-objects).
Also makes sense.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/count-objects.c | 2 ++
> sha1_file.c | 2 +-
> t/t5613-info-alternate.sh | 17 +++++++++++++++++
> 3 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/count-objects.c b/builtin/count-objects.c
> index a700409..a04b4f2 100644
> --- a/builtin/count-objects.c
> +++ b/builtin/count-objects.c
> @@ -97,6 +97,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
> OPT_END(),
> };
>
> + git_config(git_default_config, NULL);
> +
> argc = parse_options(argc, argv, prefix, opts, count_objects_usage, 0);
> /* we do not take arguments other than flags for now */
> if (argc)
> diff --git a/sha1_file.c b/sha1_file.c
> index b514167..b05ec9c 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -260,7 +260,7 @@ static int alt_odb_usable(struct strbuf *path, const char *normalized_objdir)
> * thing twice, or object directory itself.
> */
> for (alt = alt_odb_list; alt; alt = alt->next) {
> - if (!strcmp(path->buf, alt->path))
> + if (!fspathcmp(path->buf, alt->path))
> return 0;
> }
> if (!fspathcmp(path->buf, normalized_objdir))
> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
> index 76525a0..926fe14 100755
> --- a/t/t5613-info-alternate.sh
> +++ b/t/t5613-info-alternate.sh
> @@ -116,4 +116,21 @@ test_expect_success 'relative duplicates are eliminated' '
> test_cmp expect actual.alternates
> '
>
> +test_expect_success CASE_INSENSITIVE_FS 'dup finding can be case-insensitive' '
> + git init --bare insensitive.git &&
> + # the previous entry for "A" will have used uppercase
> + cat >insensitive.git/objects/info/alternates <<-\EOF &&
> + ../../C/.git/objects
> + ../../a/.git/objects
> + EOF
> + cat >expect <<-EOF &&
> + alternate: $(pwd)/C/.git/objects
> + alternate: $(pwd)/B/.git/objects
> + alternate: $(pwd)/A/.git/objects
> + EOF
> + git -C insensitive.git count-objects -v >actual &&
> + grep ^alternate: actual >actual.alternates &&
> + test_cmp expect actual.alternates
> +'
> +
> test_done
> --
> 2.10.0.618.g82cc264
^ permalink raw reply
* Re: [PATCH 17/18] sha1_file: always allow relative paths to alternates
From: Jacob Keller @ 2016-10-04 6:50 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203622.7uz76ay5f7bqqpfm@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:36 PM, Jeff King <peff@peff.net> wrote:
> We recursively expand alternates repositories, so that if A
> borrows from B which borrows from C, A can see all objects.
>
> For the root object database, we allow relative paths, so A
> can point to B as "../B/objects". However, we currently do
> not allow relative paths when recursing, so B must use an
> absolute path to reach C.
>
> That is an ancient protection from c2f493a (Transitively
> read alternatives, 2006-05-07) that tries to avoid adding
> the same alternate through two different paths. Since
> 5bdf0a8 (sha1_file: normalize alt_odb path before comparing
> and storing, 2011-09-07), we use a normalized absolute path
> for each alt_odb entry.
>
> This means that in most cases the protection is no longer
> necessary; we will detect the duplicate no matter how we got
> there (but see below). And it's a good idea to get rid of
> it, as it creates an unnecessary complication when setting
> up recursive alternates (B has to know that A is going to
> borrow from it and make sure to use an absolute path).
>
I think this makes sense. We already normalize a path, and if the
normalization is too complicated, then we (now) fail nicely so we
should always have an absolute path to the store.
> Note that our normalization doesn't actually look at the
> filesystem, so it can still be fooled by crossing symbolic
> links. But that's also true of absolute paths, so it's not a
> good reason to disallow only relative paths (it's
> potentially a reason to switch to real_path(), but that's a
> separate and non-trivial change).
Hmm, ya using real_path would fix that but I definitely agree that's
not trivial and can be done in the future if we think it is or becomes
necessary.
^ permalink raw reply
* Re: [PATCH 16/18] count-objects: report alternates via verbose mode
From: Jacob Keller @ 2016-10-04 6:46 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203618.m6kxd3b6h74jbmqz@sigill.intra.peff.net>
\On Mon, Oct 3, 2016 at 1:36 PM, Jeff King <peff@peff.net> wrote:
> There's no way to get the list of alternates that git
> computes internally; our tests only infer it based on which
> objects are available. In addition to testing, knowing this
> list may be helpful for somebody debugging their alternates
> setup.
>
> Let's add it to the "count-objects -v" output. We could give
> it a separate flag, but there's not really any need.
> "count-objects -v" is already a debugging catch-all for the
> object database, its output is easily extensible to new data
> items, and printing the alternates is not expensive (we
> already had to find them to count the objects).
>
Makes sense. Unless there's a compelling reason you'd want to print
out these alternates *without* anything else from -v, but you can just
use grep like the test does so this seems fine to me.
Thanks,
Jake
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Documentation/git-count-objects.txt | 5 +++++
> builtin/count-objects.c | 10 ++++++++++
> t/t5613-info-alternate.sh | 10 ++++++++++
> 3 files changed, 25 insertions(+)
>
> diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
> index 2ff3568..cb9b4d2 100644
> --- a/Documentation/git-count-objects.txt
> +++ b/Documentation/git-count-objects.txt
> @@ -38,6 +38,11 @@ objects nor valid packs
> +
> size-garbage: disk space consumed by garbage files, in KiB (unless -H is
> specified)
> ++
> +alternate: absolute path of alternate object databases; may appear
> +multiple times, one line per path. Note that if the path contains
> +non-printable characters, it may be surrounded by double-quotes and
> +contain C-style backslashed escape sequences.
>
> -H::
> --human-readable::
> diff --git a/builtin/count-objects.c b/builtin/count-objects.c
> index ba92919..a700409 100644
> --- a/builtin/count-objects.c
> +++ b/builtin/count-objects.c
> @@ -8,6 +8,7 @@
> #include "dir.h"
> #include "builtin.h"
> #include "parse-options.h"
> +#include "quote.h"
>
> static unsigned long garbage;
> static off_t size_garbage;
> @@ -73,6 +74,14 @@ static int count_cruft(const char *basename, const char *path, void *data)
> return 0;
> }
>
> +static int print_alternate(struct alternate_object_database *alt, void *data)
> +{
> + printf("alternate: ");
> + quote_c_style(alt->path, NULL, stdout, 0);
> + putchar('\n');
> + return 0;
> +}
> +
> static char const * const count_objects_usage[] = {
> N_("git count-objects [-v] [-H | --human-readable]"),
> NULL
> @@ -140,6 +149,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
> printf("prune-packable: %lu\n", packed_loose);
> printf("garbage: %lu\n", garbage);
> printf("size-garbage: %s\n", garbage_buf.buf);
> + foreach_alt_odb(print_alternate, NULL);
> strbuf_release(&loose_buf);
> strbuf_release(&pack_buf);
> strbuf_release(&garbage_buf);
> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
> index b393613..74f6770 100755
> --- a/t/t5613-info-alternate.sh
> +++ b/t/t5613-info-alternate.sh
> @@ -39,6 +39,16 @@ test_expect_success 'preparing third repository' '
> )
> '
>
> +test_expect_success 'count-objects shows the alternates' '
> + cat >expect <<-EOF &&
> + alternate: $(pwd)/B/.git/objects
> + alternate: $(pwd)/A/.git/objects
> + EOF
> + git -C C count-objects -v >actual &&
> + grep ^alternate: actual >actual.alternates &&
> + test_cmp expect actual.alternates
> +'
> +
> # Note: These tests depend on the hard-coded value of 5 as "too deep". We start
> # the depth at 0 and count links, not repositories, so in a chain like:
> #
> --
> 2.10.0.618.g82cc264
>
^ permalink raw reply
* Re: [PATCH 1/3] add QSORT
From: Kevin Bracey @ 2016-10-04 5:28 UTC (permalink / raw)
To: GIT Mailing-list; +Cc: René Scharfe
In-Reply-To: <9ff725eb-3536-638b-1ec0-ff9130478abc@web.de>
On 04/10/2016 01:00, René Scharfe wrote:
> Am 03.10.2016 um 19:09 schrieb Kevin Bracey:
>> As such, NULL checks can still be elided even with your change. If you
>> effectively change your example to:
>>
>> if (nmemb > 1)
>> qsort(array, nmemb, size, cmp);
>> if (!array)
>> printf("array is NULL\n");
>>
>> array may only be checked for NULL if nmemb <= 1. You can see GCC doing
>> that in the compiler explorer - it effectively turns that into "else
>> if".
>
> We don't support array == NULL together with nmemb > 1, so a segfault
> is to be expected in such cases, and thus NULL checks can be removed
> safely.
>
Possibly true in practice.
But technically wrong by the C standard - behaviour is undefined if the
qsort pointer is invalid. You can't formally expect the defined
behaviour of a segfault when sending NULL into qsort. (Hell, maybe the
qsort has its own NULL check and silently returns! cf printf - some
printfs will segfault when passed NULL, some print "(null)"). I've
worked on systems that don't fault reads to NULL, only writes, so those
might not segfault there, if NULL appeared sorted...
And obviously there's the language lawyer favourite possibility of the
call causing nasal flying monkeys or whatever.
So if it's not a program error for array to be NULL and nmemb to be zero
in your code, and you want a diagnostic for array=NULL, nmemb non-zero,
I think you should put that diagnostic into sane_qsort as an assert or
something, not rely on qsort's undefined behaviour being a segfault.
sane_qsort(blah)
{
if (nmemb >= 1) {
assert(array);
qsort(array, nmemb, ...);
}
}
Can't invoke undefined behaviour from NULL without triggering the
assert. (Could still have other invalid pointers, of course).
Usually I am on the side of "no NULL checks", as I make the assumption
that we will get a segfault as soon as NULL pointers are used, and those
are generally easy to diagnose. But seeing a compiler invoking this sort
of new trickery due to invoking undefined behaviour is making me more
nervous about doing so...
>> To make that check really work, you have to do:
>>
>> if (array)
>> qsort(array, nmemb, size, cmp);
>> else
>> printf("array is NULL\n");
>>
>> So maybe your "sane_qsort" should be checking array, not nmemb.
>
> It would be safe, but arguably too much so, because non-empty arrays
> with NULL wouldn't segfault anymore, and thus become harder to
> identify as the programming errors they are.
Well, you get the print. Although I guess you're worrying about the
second if being real code, not a debugging check.
I must say, this is quite a courageous new optimisation from GCC. It
strikes me as finding a language lawyer loophole that seems to have been
intended for something else (mapping library functions directly onto
CISCy CPU intrinsics), and using it to invent a whole new optimisation
that seems more likely to trigger bugs than optimise any significant
amount of code in a desirable way.
Doubly weird as there's no (standard) language support for this. I don't
know how you'd define "my_qsort" that triggered the same optimisations.
I've seen similar
library-knowledge-without-any-way-to-reproduce-in-user-code
optimisations like "malloc returns a new pointer that doesn't alias with
anything existing" (and no way to reproduce the optimisation with
my_malloc_wrapper). But those seemed to have a clear performance
benefit, without any obvious traps. Doubtful about this one.
Kevin
^ permalink raw reply
* Re: [PATCH 15/18] fill_sha1_file: write into a strbuf
From: Jacob Keller @ 2016-10-04 6:44 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203609.4hig3e24lyvswdcf@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:36 PM, Jeff King <peff@peff.net> wrote:
> It's currently the responsibility of the caller to give
> fill_sha1_file() enough bytes to write into, leading them to
> manually compute the required lengths. Instead, let's just
> write into a strbuf so that it's impossible to get this
> wrong.
Yea this makes sense.
>
> The alt_odb caller already has a strbuf, so this makes
> things strictly simpler. The other caller, sha1_file_name(),
> uses a static PATH_MAX buffer and dies when it would
> overflow. We can convert this to a static strbuf, which
> means our allocation cost is amortized (and as a bonus, we
> no longer have to worry about PATH_MAX being too short for
> normal use).
>
> This does introduce some small overhead in fill_sha1_file(),
> as each strbuf_addchar() will check whether it needs to
> grow. However, between the optimization in fec501d
> (strbuf_addch: avoid calling strbuf_grow, 2015-04-16) and
> the fact that this is not generally called in a tight loop
> (after all, the next step is typically to access the file!)
> this probably doesn't matter. And even if it did, the right
> place to micro-optimize is inside fill_sha1_file(), by
> calling a single strbuf_grow() there.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> sha1_file.c | 34 ++++++++++------------------------
> 1 file changed, 10 insertions(+), 24 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index efc8cee..80a3333 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -172,36 +172,28 @@ enum scld_error safe_create_leading_directories_const(const char *path)
> return result;
> }
>
> -static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)
> +static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)
> {
> int i;
> for (i = 0; i < 20; i++) {
> static char hex[] = "0123456789abcdef";
> unsigned int val = sha1[i];
> - *pathbuf++ = hex[val >> 4];
> - *pathbuf++ = hex[val & 0xf];
> + strbuf_addch(buf, hex[val >> 4]);
> + strbuf_addch(buf, hex[val & 0xf]);
> if (!i)
> - *pathbuf++ = '/';
> + strbuf_addch(buf, '/');
> }
> - *pathbuf = '\0';
> }
>
> const char *sha1_file_name(const unsigned char *sha1)
> {
> - static char buf[PATH_MAX];
> - const char *objdir;
> - int len;
> + static struct strbuf buf = STRBUF_INIT;
>
> - objdir = get_object_directory();
> - len = strlen(objdir);
> + strbuf_reset(&buf);
> + strbuf_addf(&buf, "%s/", get_object_directory());
>
> - /* '/' + sha1(2) + '/' + sha1(38) + '\0' */
> - if (len + 43 > PATH_MAX)
> - die("insanely long object directory %s", objdir);
> - memcpy(buf, objdir, len);
> - buf[len] = '/';
> - fill_sha1_path(buf + len + 1, sha1);
> - return buf;
I'm definitely a fan of seeing the magic number here go away.
> + fill_sha1_path(&buf, sha1);
> + return buf.buf;
> }
>
> struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)
> @@ -213,14 +205,8 @@ struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)
> static const char *alt_sha1_path(struct alternate_object_database *alt,
> const unsigned char *sha1)
> {
> - /* hex sha1 plus internal "/" */
> - size_t len = GIT_SHA1_HEXSZ + 1;
> struct strbuf *buf = alt_scratch_buf(alt);
Funny story.. While reviewing this code on my screen, my monitor has a
nice little bit of gunk just between the lines that made this one look
like it was being deleted. So I was really confused as to what strbuf
you were using and why you removed a call to alt_scratch_buf()..
Obviously this line just isn't being removed.
> -
> - strbuf_grow(buf, len);
> - fill_sha1_path(buf->buf + buf->len, sha1);
> - strbuf_setlen(buf, buf->len + len);
> -
> + fill_sha1_path(buf, sha1);
> return buf->buf;
> }
>
> --
> 2.10.0.618.g82cc264
>
^ permalink raw reply
* Re: [PATCH 13/18] fill_sha1_file: write "boring" characters
From: Jacob Keller @ 2016-10-04 6:13 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203555.6xadycotmmkuf34h@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:
> This function forms a sha1 as "xx/yyyy...", but skips over
> the slot for the slash rather than writing it, leaving it to
> the caller to do so. It also does not bother to put in a
> trailing NUL, even though every caller would want it (we're
> forming a path which by definition is not a directory, so
> the only thing to do with it is feed it to a system call).
>
> Let's make the lives of our callers easier by just writing
> out the internal "/" and the NUL.
>
Ya this makes sense.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> sha1_file.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 70c3e2f..c6308c1 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -178,10 +178,12 @@ static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)
> for (i = 0; i < 20; i++) {
> static char hex[] = "0123456789abcdef";
> unsigned int val = sha1[i];
> - char *pos = pathbuf + i*2 + (i > 0);
> - *pos++ = hex[val >> 4];
> - *pos = hex[val & 0xf];
> + *pathbuf++ = hex[val >> 4];
> + *pathbuf++ = hex[val & 0xf];
> + if (!i)
> + *pathbuf++ = '/';
> }
> + *pathbuf = '\0';
I think this makes a lot more sense than making the callers have to do this.
Thanks,
Jake
> }
>
> const char *sha1_file_name(const unsigned char *sha1)
> @@ -198,8 +200,6 @@ const char *sha1_file_name(const unsigned char *sha1)
> die("insanely long object directory %s", objdir);
> memcpy(buf, objdir, len);
> buf[len] = '/';
> - buf[len+3] = '/';
> - buf[len+42] = '\0';
> fill_sha1_path(buf + len + 1, sha1);
> return buf;
> }
> @@ -406,8 +406,6 @@ struct alternate_object_database *alloc_alt_odb(const char *dir)
>
> ent->name = ent->scratch + dirlen + 1;
> ent->scratch[dirlen] = '/';
> - ent->scratch[dirlen + 3] = '/';
> - ent->scratch[entlen-1] = 0;
>
> return ent;
> }
> --
> 2.10.0.618.g82cc264
>
^ permalink raw reply
* Re: [PATCH 12/18] alternates: use a separate scratch space
From: Jacob Keller @ 2016-10-04 6:12 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203551.tmqp5rll6nqkewxz@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:
> The alternate_object_database struct uses a single buffer
> both for storing the path to the alternate, and as a scratch
> buffer for forming object names. This is efficient (since
> otherwise we'd end up storing the path twice), but it makes
> life hard for callers who just want to know the path to the
> alternate. They have to remember to stop reading after
> "alt->name - alt->base" bytes, and to subtract one for the
> trailing '/'.
>
> It would be much simpler if they could simply access a
> NUL-terminated path string. We could encapsulate this in a
> function which puts a NUL in the scratch buffer and returns
> the string, but that opens up questions about the lifetime
> of the result. The first time another caller uses the
> alternate, the scratch buffer may get other data tacked onto
> it.
>
> Let's instead just store the root path separately from the
> scratch buffer. There aren't enough alternates being stored
> for the duplicated data to matter for performance, and this
> keeps things simple and safe for the callers.
>
Definitely agree here. The resulting code seems a lot easier to
follow, and making the callers simpler here is a very goo thing.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 10/18] alternates: provide helper for allocating alternate
From: Jacob Keller @ 2016-10-04 6:09 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203531.bppczvzmdfumtnb2@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:
> Allocating a struct alternate_object_database is tricky, as
> we must over-allocate the buffer to provide scratch space,
> and then put in particular '/' and NUL markers.
>
> Let's encapsulate this in a function so that the complexity
> doesn't leak into callers (and so that we can modify it
> later).
The overall way this was broken up is definitely a lot of patches to
follow but understanding the end goal this is a huge improvement in
code maintainability here. This original allocation is indeed very
tricky.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 09/18] alternates: provide helper for adding to alternates list
From: Jacob Keller @ 2016-10-04 6:07 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203503.omjwvg4ocz7pjyzt@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:35 PM, Jeff King <peff@peff.net> wrote:
> The submodule code wants to temporarily add an alternate
> object store to our in-memory alt_odb list, but does it
> manually. Let's provide a helper so it can reuse the code in
> link_alt_odb_entry().
>
> While we're adding our new add_to_alternates_memory(), let's
> document add_to_alternates_file(), as the two are related.
>
Ya the code used in the submodule area always felt a bit wrong to me.
It took me a bit to realize why we can just replace this all with a
call to link_alt_odb_entry, but the resulting code reduction is
definitely nice.
> - /* avoid adding it twice */
> - prepare_alt_odb();
> - for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
> - if (alt_odb->name - alt_odb->base == objects_directory.len &&
> - !strncmp(alt_odb->base, objects_directory.buf,
> - objects_directory.len))
> - goto done;
> -
> - alloc = st_add(objects_directory.len, 42); /* for "12/345..." sha1 */
> - alt_odb = xmalloc(st_add(sizeof(*alt_odb), alloc));
> - alt_odb->next = alt_odb_list;
> - xsnprintf(alt_odb->base, alloc, "%s", objects_directory.buf);
> - alt_odb->name = alt_odb->base + objects_directory.len;
> - alt_odb->name[2] = '/';
> - alt_odb->name[40] = '\0';
> - alt_odb->name[41] = '\0';
> - alt_odb_list = alt_odb;
> -
Getting rid of multiple places for this funky extra allocation is a
nice improvement.
Thanks,
Jake
> - /* add possible alternates from the submodule */
> - read_info_alternates(objects_directory.buf, 0);
> + add_to_alternates_memory(objects_directory.buf);
> done:
> strbuf_release(&objects_directory);
> return ret;
> --
> 2.10.0.618.g82cc264
>
^ permalink raw reply
* Re: [PATCH 08/18] link_alt_odb_entry: refactor string handling
From: Jacob Keller @ 2016-10-04 6:05 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203448.cdfbitl5jmhlpb5o@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:
> The string handling in link_alt_odb_entry() is mostly an
> artifact of the original version, which took the path as a
> ptr/len combo, and did not have a NUL-terminated string
> until we created one in the alternate_object_database
> struct. But since 5bdf0a8 (sha1_file: normalize alt_odb
> path before comparing and storing, 2011-09-07), the first
> thing we do is put the path into a strbuf, which gives us
> some easy opportunities for cleanup.
>
> In particular:
>
> - we call strlen(pathbuf.buf), which is silly; we can look
> at pathbuf.len.
Right. This makes obvious sense.
>
> - even though we have a strbuf, we don't maintain its
> "len" field when chomping extra slashes from the
> end, and instead keep a separate "pfxlen" variable. We
> can fix this and then drop "pfxlen" entirely.
>
Makes sense.
> - we don't check whether the path is usable until after we
> allocate the new struct, making extra cleanup work for
> ourselves. Since we have a NUL-terminated string, we can
> bump the "is it usable" checks higher in the function.
> While we're at it, we can move that logic to its own
> helper, which makes the flow of link_alt_odb_entry()
> easier to follow.
>
Also makes sense.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> And you can probably guess now how I found the issue in the last patch
> where pathbuf.len is totally bogus after calling normalize_path_copy. :)
>
> sha1_file.c | 83 +++++++++++++++++++++++++++++++++----------------------------
> 1 file changed, 45 insertions(+), 38 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 68571bd..f396823 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -234,6 +234,36 @@ char *sha1_pack_index_name(const unsigned char *sha1)
> struct alternate_object_database *alt_odb_list;
> static struct alternate_object_database **alt_odb_tail;
>
> +/*
> + * Return non-zero iff the path is usable as an alternate object database.
> + */
> +static int alt_odb_usable(struct strbuf *path, const char *normalized_objdir)
> +{
> + struct alternate_object_database *alt;
> +
> + /* Detect cases where alternate disappeared */
> + if (!is_directory(path->buf)) {
> + error("object directory %s does not exist; "
> + "check .git/objects/info/alternates.",
> + path->buf);
> + return 0;
> + }
> +
> + /*
> + * Prevent the common mistake of listing the same
> + * thing twice, or object directory itself.
> + */
> + for (alt = alt_odb_list; alt; alt = alt->next) {
> + if (path->len == alt->name - alt->base - 1 &&
> + !memcmp(path->buf, alt->base, path->len))
> + return 0;
> + }
> + if (!fspathcmp(path->buf, normalized_objdir))
> + return 0;
> +
> + return 1;
> +}
> +
This definitely makes reading the following function much easier,
though the diff is a bit funky. I think the end result is much
clearer.
Thanks,
Jake
> /*
> * Prepare alternate object database registry.
> *
> @@ -253,8 +283,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,
> int depth, const char *normalized_objdir)
> {
> struct alternate_object_database *ent;
> - struct alternate_object_database *alt;
> - size_t pfxlen, entlen;
> + size_t entlen;
> struct strbuf pathbuf = STRBUF_INIT;
>
> if (!is_absolute_path(entry) && relative_base) {
> @@ -270,47 +299,26 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,
> return -1;
> }
>
> - pfxlen = strlen(pathbuf.buf);
> -
> /*
> * The trailing slash after the directory name is given by
> * this function at the end. Remove duplicates.
> */
> - while (pfxlen && pathbuf.buf[pfxlen-1] == '/')
> - pfxlen -= 1;
> -
> - entlen = st_add(pfxlen, 43); /* '/' + 2 hex + '/' + 38 hex + NUL */
> - ent = xmalloc(st_add(sizeof(*ent), entlen));
> - memcpy(ent->base, pathbuf.buf, pfxlen);
> - strbuf_release(&pathbuf);
> -
> - ent->name = ent->base + pfxlen + 1;
> - ent->base[pfxlen + 3] = '/';
> - ent->base[pfxlen] = ent->base[entlen-1] = 0;
> + while (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')
> + strbuf_setlen(&pathbuf, pathbuf.len - 1);
>
> - /* Detect cases where alternate disappeared */
> - if (!is_directory(ent->base)) {
> - error("object directory %s does not exist; "
> - "check .git/objects/info/alternates.",
> - ent->base);
> - free(ent);
> + if (!alt_odb_usable(&pathbuf, normalized_objdir)) {
> + strbuf_release(&pathbuf);
> return -1;
> }
>
> - /* Prevent the common mistake of listing the same
> - * thing twice, or object directory itself.
> - */
> - for (alt = alt_odb_list; alt; alt = alt->next) {
> - if (pfxlen == alt->name - alt->base - 1 &&
> - !memcmp(ent->base, alt->base, pfxlen)) {
> - free(ent);
> - return -1;
> - }
> - }
> - if (!fspathcmp(ent->base, normalized_objdir)) {
> - free(ent);
> - return -1;
> - }
> + entlen = st_add(pathbuf.len, 43); /* '/' + 2 hex + '/' + 38 hex + NUL */
> + ent = xmalloc(st_add(sizeof(*ent), entlen));
> + memcpy(ent->base, pathbuf.buf, pathbuf.len);
> +
> + ent->name = ent->base + pathbuf.len + 1;
> + ent->base[pathbuf.len] = '/';
> + ent->base[pathbuf.len + 3] = '/';
> + ent->base[entlen-1] = 0;
>
> /* add the alternate entry */
> *alt_odb_tail = ent;
> @@ -318,10 +326,9 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,
> ent->next = NULL;
>
> /* recursively add alternates */
> - read_info_alternates(ent->base, depth + 1);
> -
> - ent->base[pfxlen] = '/';
> + read_info_alternates(pathbuf.buf, depth + 1);
>
> + strbuf_release(&pathbuf);
> return 0;
> }
>
> --
> 2.10.0.618.g82cc264
>
^ permalink raw reply
* Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors
From: Jacob Keller @ 2016-10-04 6:01 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203417.izcgwt4yz3yspdnm@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:
> When we add a new alternate to the list, we try to normalize
> out any redundant "..", etc. However, we do not look at the
> return value of normalize_path_copy(), and will happily
> continue with a path that could not be normalized. Worse,
> the normalizing process is done in-place, so we are left
> with whatever half-finished working state the normalizing
> function was in.
>
> Fortunately, this cannot cause us to read past the end of
> our buffer, as that working state will always leave the
> NUL from the original path in place. And we do tend to
> notice problems when we check is_directory() on the path.
> But you can see the nonsense that we feed to is_directory
> with an entry like:
>
> this/../../is/../../way/../../too/../../deep/../../to/../../resolve
>
> in your objects/info/alternates, which yields:
>
> error: object directory
> /to/e/deep/too/way//ects/this/../../is/../../way/../../too/../../deep/../../to/../../resolve
> does not exist; check .git/objects/info/alternates.
>
Yikes, that doesn't seem helpful.
> We can easily fix this just by checking the return value.
> But that makes it hard to generate a good error message,
> since we're normalizing in-place and our input value has
> been overwritten by cruft.
Right. Definitely want to check the return value here...
>
> Instead, let's provide a strbuf helper that does an in-place
> normalize, but restores the original contents on error. This
> uses a second buffer under the hood, which is slightly less
> efficient, but this is not a performance-critical code path.
>
I agree, I don't think this duplication is really a big deal, since it
helps ensure that the function doesn't modify its arguments on error.
> The strbuf helper can also properly set the "len" parameter
> of the strbuf before returning. Just doing:
>
> normalize_path_copy(buf.buf, buf.buf);
>
> will shorten the string, but leave buf.len at the original
> length. That may be confusing to later code which uses the
> strbuf.
>
Makes sense here. Properly setting the length will help prevent future issues.
Thanks,
Jake
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> sha1_file.c | 11 +++++++++--
> strbuf.c | 20 ++++++++++++++++++++
> strbuf.h | 8 ++++++++
> 3 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index b9c1fa3..68571bd 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -263,7 +263,12 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,
> }
> strbuf_addstr(&pathbuf, entry);
>
> - normalize_path_copy(pathbuf.buf, pathbuf.buf);
> + if (strbuf_normalize_path(&pathbuf) < 0) {
> + error("unable to normalize alternate object path: %s",
> + pathbuf.buf);
> + strbuf_release(&pathbuf);
> + return -1;
> + }
>
> pfxlen = strlen(pathbuf.buf);
>
> @@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
> }
>
> strbuf_add_absolute_path(&objdirbuf, get_object_directory());
> - normalize_path_copy(objdirbuf.buf, objdirbuf.buf);
> + if (strbuf_normalize_path(&objdirbuf) < 0)
> + die("unable to normalize object directory: %s",
> + objdirbuf.buf);
>
> alt_copy = xmemdupz(alt, len);
> string_list_split_in_place(&entries, alt_copy, sep, -1);
> diff --git a/strbuf.c b/strbuf.c
> index b839be4..8fec657 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -870,3 +870,23 @@ void strbuf_stripspace(struct strbuf *sb, int skip_comments)
>
> strbuf_setlen(sb, j);
> }
> +
> +int strbuf_normalize_path(struct strbuf *src)
> +{
> + struct strbuf dst = STRBUF_INIT;
> +
> + strbuf_grow(&dst, src->len);
> + if (normalize_path_copy(dst.buf, src->buf) < 0) {
> + strbuf_release(&dst);
> + return -1;
> + }
> +
> + /*
> + * normalize_path does not tell us the new length, so we have to
> + * compute it by looking for the new NUL it placed
> + */
And we can't correctly set the length inside normalize_path_copy
because it just takes C strings directly and not actually a strbuf. Ok
so it makes sense that we have to set it here.
Thanks,
Jake
> + strbuf_setlen(&dst, strlen(dst.buf));
> + strbuf_swap(src, &dst);
> + strbuf_release(&dst);
> + return 0;
> +}
> diff --git a/strbuf.h b/strbuf.h
> index ba8d5f1..2262b12 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -443,6 +443,14 @@ extern int strbuf_getcwd(struct strbuf *sb);
> */
> extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
>
^ permalink raw reply
* Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests
From: Jacob Keller @ 2016-10-04 5:57 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203412.bekizvlqtg4ls5fb@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:
> These tests are just trying to show that we allow recursion
> up to a certain depth, but not past it. But the counting is
> a bit non-intuitive, and rather than test at the edge of the
> breakage, we test "OK" cases in the middle of the chain.
> Let's explain what's going on, and explicitly test the
> switch between "OK" and "too deep".
>
Makes sense to actually test the edge case here instead of just in the middle.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t5613-info-alternate.sh | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
> index 7bc1c3c..b393613 100755
> --- a/t/t5613-info-alternate.sh
> +++ b/t/t5613-info-alternate.sh
> @@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '
> )
> '
>
> +# Note: These tests depend on the hard-coded value of 5 as "too deep". We start
> +# the depth at 0 and count links, not repositories, so in a chain like:
> +#
> +# A -> B -> C -> D -> E -> F -> G -> H
> +# 0 1 2 3 4 5 6
> +#
Ok so we count links, but wouldn't we have 5 links when we hit F, and
not G? Or am I missing something here?
> +# we are OK at "G", but break at "H".
> +#
Seems like from the wording of this comment that we'd break at G and
not H..? Obviously the test below shows G is ok. Aren't the numbers
here off by 1?
Regards,
Jake
> +# Note also that we must use "--bare -l" to make the link to H. The "-l"
> +# ensures we do not do a connectivity check, and the "--bare" makes sure
> +# we do not try to checkout the result (which needs objects), either of
> +# which would cause the clone to fail.
> test_expect_success 'creating too deep nesting' '
> git clone -l -s C D &&
> git clone -l -s D E &&
> @@ -47,16 +59,12 @@ test_expect_success 'creating too deep nesting' '
> git clone --bare -l -s G H
> '
>
> -test_expect_success 'invalidity of deepest repository' '
> - test_must_fail git -C H fsck
> -'
> -
> -test_expect_success 'validity of third repository' '
> - git -C C fsck
> +test_expect_success 'validity of fifth-deep repository' '
> + git -C G fsck
> '
>
> -test_expect_success 'validity of fourth repository' '
> - git -C D fsck
> +test_expect_success 'invalidity of sixth-deep repository' '
> + test_must_fail git -C H fsck
> '
>
> test_expect_success 'breaking of loops' '
> --
> 2.10.0.618.g82cc264
>
^ permalink raw reply
* Re: [PATCH 05/18] t5613: do not chdir in main process
From: Jacob Keller @ 2016-10-04 5:54 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203408.qnakqgcninzty3sr@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:
> Our usual style when working with subdirectories is to chdir
> inside a subshell or to use "git -C", which means we do not
> have to constantly return to the main test directory. Let's
> convert this old test, which does not follow that style.
>
More obvious cleanup for this test file. Seems like a lot of
intermediate steps to get this test file into a clean state.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 04/18] t5613: whitespace/style cleanups
From: Jacob Keller @ 2016-10-04 5:52 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203405.nzijl552nlqg63ab@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:
> Our normal test style these days puts the opening quote of
> the body on the description line, and indents the body with
> a single tab. This ancient test did not follow this.
>
I was surprised you didn't do this first, but it doesn't really make a
difference either way. This is also a pretty straight forward
improvement, and I can see why you'd want to split this out to review
separately.
Regards,
Jake
^ permalink raw reply
* Re: [PATCH 03/18] t5613: use test_must_fail
From: Jacob Keller @ 2016-10-04 5:51 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203401.d4awnljukgqbku2n@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:
> Besides being our normal style, this correctly checks for an
> error exit() versus signal death.
>
Another very simple but obvious improvement.
Regards,
Jake
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t5613-info-alternate.sh | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
> index 4548fb0..65074dd 100755
> --- a/t/t5613-info-alternate.sh
> +++ b/t/t5613-info-alternate.sh
> @@ -46,10 +46,9 @@ git clone -l -s F G &&
> git clone --bare -l -s G H'
>
> test_expect_success 'invalidity of deepest repository' \
> -'cd H && {
> - git fsck
> - test $? -ne 0
> -}'
> +'cd H &&
> +test_must_fail git fsck
> +'
>
> cd "$base_dir"
>
> @@ -75,7 +74,8 @@ cd "$base_dir"
> test_expect_success 'that info/alternates is necessary' \
> 'cd C &&
> rm -f .git/objects/info/alternates &&
> -! (git fsck)'
> +test_must_fail git fsck
> +'
>
> cd "$base_dir"
>
> @@ -89,7 +89,7 @@ cd "$base_dir"
> test_expect_success \
> 'that relative alternate is only possible for current dir' '
> cd D &&
> - ! (git fsck)
> + test_must_fail git fsck
> '
>
> cd "$base_dir"
> --
> 2.10.0.618.g82cc264
>
^ permalink raw reply
* Re: [PATCH 02/18] t5613: drop test_valid_repo function
From: Jacob Keller @ 2016-10-04 5:50 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203357.3cpeg2jyalzykm65@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:33 PM, Jeff King <peff@peff.net> wrote:
> This function makes sure that "git fsck" does not report any
> errors. But "--full" has been the default since f29cd39
> (fsck: default to "git fsck --full", 2009-10-20), and we can
> use the exit code (instead of counting the lines) since
> e2b4f63 (fsck: exit with non-zero status upon errors,
> 2007-03-05).
>
> So we can just use "git fsck", which is shorter and more
> flexible (e.g., we can use "git -C").
This seems obviously correct. I didn't understand your comment about
the use of "git -C" at first, because I was confused about why "git
-C" doesn't work with "git --full", but then I realized you can't use
"git -C" with the shell test_valid_repo function.
Thanks,
Jake
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t5613-info-alternate.sh | 19 +++++++------------
> 1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
> index e13f57d..4548fb0 100755
> --- a/t/t5613-info-alternate.sh
> +++ b/t/t5613-info-alternate.sh
> @@ -6,11 +6,6 @@
> test_description='test transitive info/alternate entries'
> . ./test-lib.sh
>
> -test_valid_repo() {
> - git fsck --full > fsck.log &&
> - test_line_count = 0 fsck.log
> -}
> -
> base_dir=$(pwd)
^ permalink raw reply
* Re: [PATCH 01/18] t5613: drop reachable_via function
From: Jacob Keller @ 2016-10-04 5:48 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203350.3u6ddr6ndr3jwr74@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:33 PM, Jeff King <peff@peff.net> wrote:
> This function was never used since its inception in dd05ea1
> (test case for transitive info/alternates, 2006-05-07).
> Which is just as well, since it mutates the repo state in a
> way that would invalidate further tests, without cleaning up
> after itself. Let's get rid of it so that nobody is tempted
> to use it.
>
Makes sense. It wouldn't be a good idea to leave this around since it
didn't clean up after itself. Curious why no test actually used it
though..
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 0/18] alternate object database cleanups
From: Jacob Keller @ 2016-10-04 5:47 UTC (permalink / raw)
To: Jeff King; +Cc: Git mailing list, René Scharfe
In-Reply-To: <20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net>
On Mon, Oct 3, 2016 at 1:33 PM, Jeff King <peff@peff.net> wrote:
> This series is the result of René nerd-sniping me with the claim that we
> could "easily" teach count-objects to print out the list of alternates
> in:
>
> http://public-inbox.org/git/c27dc1a4-3c7a-2866-d9d8-f5d3eb161650@web.de/
>
Hah. Nerd snipes are fun.
> My real goal is just patch 17, which is needed for the quarantine series
> in that thread. But along the way there were quite a few opportunities
> for cleanups along with a few minor bugfixes (in patches 7 and 18), and
> I think the count-objects change in patch 16 is a nice general debugging
> tool.
Yea there are a *lot* of cleanups here.
>
> The rest of it is "just" cleanup, but I'll note that it clears up some
> hairy allocation code. These were bits that I noticed in my big
> allocation-cleanup series last year, but were too nasty to fit any of
> the more general fixes. I think the end result is much better.
>
Definitely agreed. I read through all the patches, and each one seemed
reasonable.
> The number of patches is a little intimidating, but I tried hard to
> break the refactoring down into a sequence of obviously-correct steps.
> You can be the judge of my success.
>
I read through them once. I'm going to re-read through them again and
leave any comments I had.
Regards,
Jake
> [01/18]: t5613: drop reachable_via function
> [02/18]: t5613: drop test_valid_repo function
> [03/18]: t5613: use test_must_fail
> [04/18]: t5613: whitespace/style cleanups
> [05/18]: t5613: do not chdir in main process
> [06/18]: t5613: clarify "too deep" recursion tests
> [07/18]: link_alt_odb_entry: handle normalize_path errors
> [08/18]: link_alt_odb_entry: refactor string handling
> [09/18]: alternates: provide helper for adding to alternates list
> [10/18]: alternates: provide helper for allocating alternate
> [11/18]: alternates: encapsulate alt->base munging
> [12/18]: alternates: use a separate scratch space
> [13/18]: fill_sha1_file: write "boring" characters
> [14/18]: alternates: store scratch buffer as strbuf
> [15/18]: fill_sha1_file: write into a strbuf
> [16/18]: count-objects: report alternates via verbose mode
> [17/18]: sha1_file: always allow relative paths to alternates
> [18/18]: alternates: use fspathcmp to detect duplicates
>
> Documentation/git-count-objects.txt | 5 +
> builtin/count-objects.c | 12 +++
> builtin/fsck.c | 10 +-
> builtin/submodule--helper.c | 11 +-
> cache.h | 36 ++++++-
> sha1_file.c | 179 ++++++++++++++++++--------------
> sha1_name.c | 17 +--
> strbuf.c | 20 ++++
> strbuf.h | 8 ++
> submodule.c | 23 +---
> t/t5613-info-alternate.sh | 202 ++++++++++++++++++++----------------
> transport.c | 4 +-
> 12 files changed, 305 insertions(+), 222 deletions(-)
>
^ permalink raw reply
* Re: Feature Request: user defined suffix for temp files created by git-mergetool
From: Josef Ridky @ 2016-10-04 5:18 UTC (permalink / raw)
To: Anatoly Borodin; +Cc: git
In-Reply-To: <CACNzp2mANqmciMzvCahM_+=RnZYtU1SK9DRS2BmQru1fZY_wUQ@mail.gmail.com>
Hi Anatoly,
| Sent: Monday, October 3, 2016 5:18:44 PM
|
| Hi Josef,
|
|
| On Mon, Oct 3, 2016 at 8:36 AM, Josef Ridky <jridky@redhat.com> wrote:
| > In several projects, we are using git mergetool for comparing files from
| > different folders.
| > Unfortunately, when we have opened three files for comparing using meld
| > tool (e.q. Old_version -- Result -- New_version),
| > we can see only name of temporary files created by mergetool in the labels
| > (e.g. foo_REMOTE -- foo_BASE -- foo_LOCAL)
| > and users (and sometime even we) are confused, which of the files should
| > they edit and save.
|
| `git mergetool` just creates temporary files (with some temporary
| names) and calls `meld` (or `vimdiff`, etc) with the file names as
| parameters. So why wouldn't you call `meld` with the file names you
| want?
Because files, that we want, are temporary files created by git mergetool and we are not able to change their name.
Regards
Josef
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #01; Mon, 3)
From: Junio C Hamano @ 2016-10-04 4:01 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161004005247.sgeqgw3accn3whgi@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> * lt/abbrev-auto (2016-10-03) 3 commits
>
> I kind of expected this one to cook in next for a bit while people
> decided whether the larger hashes were irritating or not. Despite
> working on the implementation, I'm on the fence myself.
>
> I'd kind of hoped people would play with core.disambiguate and the hints
> and see if they still actually wanted to bump the default abbrev (and
> how aggressively to do so; if core.disambiguate means most of the
> ambiguity is just between commits, that cuts the number of
> collision-interesting objects by an order of magnitude).
Sure. Let's keep them cooking.
>> * jk/pack-objects-optim-mru (2016-08-11) 4 commits
>> (merged to 'next' on 2016-09-21 at 97b919bdbd)
>> + pack-objects: use mru list when iterating over packs
>> + pack-objects: break delta cycles before delta-search phase
>> + sha1_file: make packed_object_info public
>> + provide an initializer for "struct object_info"
>>
>> Originally merged to 'next' on 2016-08-11
>>
>> "git pack-objects" in a repository with many packfiles used to
>> spend a lot of time looking for/at objects in them; the accesses to
>> the packfiles are now optimized by checking the most-recently-used
>> packfile first.
>>
>> Will hold to see if people scream.
>
> This has been in next for 6 weeks. Is it time to consider graduating it?
Perhaps.
^ permalink raw reply
* Re: [PATCH 3/3] abbrev: auto size the default abbreviation
From: Junio C Hamano @ 2016-10-04 1:37 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20161003234728.s5sadekukxoppcmw@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> OK, as Linus's "count at the point of use" is already in 'next',
>> could you make it incremental with a log message?
>
> Sure. I wasn't sure if you actually liked my direction or not, so I was
> mostly just showing off what the completed one would look like.
To be quite honest, I am not just unsure if I liked your direction;
rather I am not sure if I actually understood what you perceived as
a difference that matters between the two approaches. I wanted to
hear you explain the difference in terms of "Linus's does this, but
it is bad in X and Y way, so let's avoid it and do it like Z
instead". One effective way to extract that out of you was to force
you to justify the "incremental" update.
And it seems that I succeeded ;-).
I am still not sure if I 100% agree with your first paragraph, but
at least now I think I see where you are coming from.
You probably will hear from Ramsay about extern-ness of msb().
> -- >8 --
> Subject: [PATCH] find_unique_abbrev: move logic out of get_short_sha1()
>
> The get_short_sha1() is only about reading short sha1s; we
> do call it in a loop to check "is this long enough" for each
> object, but otherwise it should not need to know about
> things like our default_abbrev setting.
>
> So instead of asking it to set default_automatic_abbrev as a
> side-effect, let's just have find_unique_abbrev() pick the
> right place to start its loop. This requires a separate
> approximate_object_count() function, but that naturally
> belongs with the rest of sha1_file.c.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> cache.h | 7 ++++++-
> sha1_file.c | 27 +++++++++++++++++++++++++++
> sha1_name.c | 60 +++++++++++++++++++++++++++++++++++-------------------------
> 3 files changed, 68 insertions(+), 26 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 0e2a059..f22ace5 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1204,7 +1204,6 @@ struct object_context {
> #define GET_SHA1_TREEISH 020
> #define GET_SHA1_BLOB 040
> #define GET_SHA1_FOLLOW_SYMLINKS 0100
> -#define GET_SHA1_AUTOMATIC 0200
> #define GET_SHA1_ONLY_TO_DIE 04000
>
> #define GET_SHA1_DISAMBIGUATORS \
> @@ -1456,6 +1455,12 @@ extern void prepare_packed_git(void);
> extern void reprepare_packed_git(void);
> extern void install_packed_git(struct packed_git *pack);
>
> +/*
> + * Give a rough count of objects in the repository. This sacrifices accuracy
> + * for speed.
> + */
> +unsigned long approximate_object_count(void);
> +
> extern struct packed_git *find_sha1_pack(const unsigned char *sha1,
> struct packed_git *packs);
>
> diff --git a/sha1_file.c b/sha1_file.c
> index b9c1fa3..4882440 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1381,6 +1381,32 @@ static void prepare_packed_git_one(char *objdir, int local)
> strbuf_release(&path);
> }
>
> +static int approximate_object_count_valid;
> +
> +/*
> + * Give a fast, rough count of the number of objects in the repository. This
> + * ignores loose objects completely. If you have a lot of them, then either
> + * you should repack because your performance will be awful, or they are
> + * all unreachable objects about to be pruned, in which case they're not really
> + * interesting as a measure of repo size in the first place.
> + */
> +unsigned long approximate_object_count(void)
> +{
> + static unsigned long count;
> + if (!approximate_object_count_valid) {
> + struct packed_git *p;
> +
> + prepare_packed_git();
> + count = 0;
> + for (p = packed_git; p; p = p->next) {
> + if (open_pack_index(p))
> + continue;
> + count += p->num_objects;
> + }
> + }
> + return count;
> +}
> +
> static void *get_next_packed_git(const void *p)
> {
> return ((const struct packed_git *)p)->next;
> @@ -1455,6 +1481,7 @@ void prepare_packed_git(void)
>
> void reprepare_packed_git(void)
> {
> + approximate_object_count_valid = 0;
> prepare_packed_git_run_once = 0;
> prepare_packed_git();
> }
> diff --git a/sha1_name.c b/sha1_name.c
> index beb7ab5..76e6885 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -15,7 +15,6 @@ typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);
>
> struct disambiguate_state {
> int len; /* length of prefix in hex chars */
> - unsigned int nrobjects;
> char hex_pfx[GIT_SHA1_HEXSZ + 1];
> unsigned char bin_pfx[GIT_SHA1_RAWSZ];
>
> @@ -119,14 +118,6 @@ static void find_short_object_filename(struct disambiguate_state *ds)
>
> if (strlen(de->d_name) != 38)
> continue;
> -
> - /*
> - * We only look at the one subdirectory, and we assume
> - * each subdirectory is roughly similar, so each
> - * object we find probably has 255 other objects in
> - * the other fan-out directories.
> - */
> - ds->nrobjects += 256;
> if (memcmp(de->d_name, ds->hex_pfx + 2, ds->len - 2))
> continue;
> memcpy(hex + 2, de->d_name, 38);
> @@ -160,7 +151,6 @@ static void unique_in_pack(struct packed_git *p,
>
> open_pack_index(p);
> num = p->num_objects;
> - ds->nrobjects += num;
> last = num;
> while (first < last) {
> uint32_t mid = (first + last) / 2;
> @@ -390,9 +380,6 @@ static int show_ambiguous_object(const unsigned char *sha1, void *data)
> return 0;
> }
>
> -/* start from our historical default before the automatic abbreviation */
> -static int default_automatic_abbrev = FALLBACK_DEFAULT_ABBREV;
> -
> static int get_short_sha1(const char *name, int len, unsigned char *sha1,
> unsigned flags)
> {
> @@ -439,14 +426,6 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
> for_each_abbrev(ds.hex_pfx, show_ambiguous_object, &ds);
> }
>
> - if (len < 16 && !status && (flags & GET_SHA1_AUTOMATIC)) {
> - unsigned int expect_collision = 1 << (len * 2);
> - if (ds.nrobjects > expect_collision) {
> - default_automatic_abbrev = len+1;
> - return SHORT_NAME_AMBIGUOUS;
> - }
> - }
> -
> return status;
> }
>
> @@ -476,22 +455,53 @@ int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
> return ret;
> }
>
> +/*
> + * Return the slot of the most-significant bit set in "val". There are various
> + * ways to do this quickly with fls() or __builtin_clzl(), but speed is
> + * probably not a big deal here.
> + */
> +unsigned msb(unsigned long val)
> +{
> + unsigned r = 0;
> + while (val >>= 1)
> + r++;
> + return r;
> +}
> +
> int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)
> {
> int status, exists;
> - int flags = GET_SHA1_QUIETLY;
>
> if (len < 0) {
> - flags |= GET_SHA1_AUTOMATIC;
> - len = default_automatic_abbrev;
> + unsigned long count = approximate_object_count();
> + /*
> + * Add one because the MSB only tells us the highest bit set,
> + * not including the value of all the _other_ bits (so "15"
> + * is only one off of 2^4, but the MSB is the 3rd bit.
> + */
> + len = msb(count) + 1;
> + /*
> + * We now know we have on the order of 2^len objects, which
> + * expects a collision at 2^(len/2). But we also care about hex
> + * chars, not bits, and there are 4 bits per hex. So all
> + * together we need to divide by 2; but we also want to round
> + * odd numbers up, hence adding one before dividing.
> + */
> + len = (len + 1) / 2;
> + /*
> + * For very small repos, we stick with our regular fallback.
> + */
> + if (len < FALLBACK_DEFAULT_ABBREV)
> + len = FALLBACK_DEFAULT_ABBREV;
> }
> +
> sha1_to_hex_r(hex, sha1);
> if (len == 40 || !len)
> return 40;
> exists = has_sha1_file(sha1);
> while (len < 40) {
> unsigned char sha1_ret[20];
> - status = get_short_sha1(hex, len, sha1_ret, flags);
> + status = get_short_sha1(hex, len, sha1_ret, GET_SHA1_QUIETLY);
> if (exists
> ? !status
> : status == SHORT_NAME_NOT_FOUND) {
^ permalink raw reply
* Re: [PATCH v2 6/6] git-gui: Update Japanese information
From: Junio C Hamano @ 2016-10-04 1:20 UTC (permalink / raw)
To: Pat Thoyts; +Cc: git
In-Reply-To: <87mvilt4jg.fsf@red.patthoyts.tk>
Pat Thoyts <patthoyts@users.sourceforge.net> writes:
> I've tried to merge in these branches as they appear in your version
> although I already had one patch on top of 0.20.0 for some time. I've
> tentatively pushed this up to http://github.com/patthoyts/git-gui as
> branch 'pu' with additional stuff on top of the patches you already
> have. If this looks ok to you I'll merge this to my master and send you
> a merge request to get it all synchronized.
Your 64c6b4c507 matches what I expected to see as the result of
merging the above four topics on top of your 'master'. I have no
opinion on the other topics that appear on top of it on the branch
you pushed out, other than that I trust the maintainer of the
subsystem and I'm fine to blindly pull them from you ;-)
I am not sure if f64a1a9311 ("git-gui: maintain backwards
compatibility for merge syntax", 2016-10-04) makes any practical
difference in the real world, though. You'd need to find somebody
who grabs the newer version of git-gui that includes b5f325cb4a
("git-gui: stop using deprecated merge syntax", 2016-09-24), without
having updated their copy of git-core for more than a year, given
that 2.5.0 is from July last year. It would not hurt, but I am not
sure if an extra invocation of "git version" is really worth it.
Thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #01; Mon, 3)
From: Jeff King @ 2016-10-04 0:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqtwct3w0p.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 03, 2016 at 03:31:02PM -0700, Junio C Hamano wrote:
> * lt/abbrev-auto (2016-10-03) 3 commits
> (merged to 'next' on 2016-10-03 at bb188d00f7)
> + abbrev: auto size the default abbreviation
> + abbrev: prepare for new world order
> + abbrev: add FALLBACK_DEFAULT_ABBREV to prepare for auto sizing
> (this branch uses jk/ambiguous-short-object-names.)
>
> Allow the default abbreviation length, which has historically been
> 7, to scale as the repository grows. The logic suggests to use 12
> hexdigits for the Linux kernel, and 9 to 10 for Git itself.
>
> Will merge to 'master'.
I kind of expected this one to cook in next for a bit while people
decided whether the larger hashes were irritating or not. Despite
working on the implementation, I'm on the fence myself.
I'd kind of hoped people would play with core.disambiguate and the hints
and see if they still actually wanted to bump the default abbrev (and
how aggressively to do so; if core.disambiguate means most of the
ambiguity is just between commits, that cuts the number of
collision-interesting objects by an order of magnitude).
> * jk/pack-objects-optim-mru (2016-08-11) 4 commits
> (merged to 'next' on 2016-09-21 at 97b919bdbd)
> + pack-objects: use mru list when iterating over packs
> + pack-objects: break delta cycles before delta-search phase
> + sha1_file: make packed_object_info public
> + provide an initializer for "struct object_info"
>
> Originally merged to 'next' on 2016-08-11
>
> "git pack-objects" in a repository with many packfiles used to
> spend a lot of time looking for/at objects in them; the accesses to
> the packfiles are now optimized by checking the most-recently-used
> packfile first.
>
> Will hold to see if people scream.
This has been in next for 6 weeks. Is it time to consider graduating it?
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 0/2] place cherry pick line below commit title
From: Jonathan Tan @ 2016-10-04 0:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <xmqq8tu55bel.fsf@gitster.mtv.corp.google.com>
On 10/03/2016 03:13 PM, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> There are other options like checking for indentation or checking for
>> balanced parentheses/brackets, but I think that these would lead to
>> surprising behavior for the user (this would mean that whitespace or
>> certain characters could turn a valid trailer into an invalid one or
>> vice versa, or change the behavior of trailer.ifexists, especially
>> "replace").
>
> Yes, that is exactly why I said that it may be necessary for the
> code to analize the lines in a block identified as "likely to be a
> trailing block" more carefully. We can afford to be loose as long
> as the only allowed operation is to append one at the end, but once
> we start removing/replacing an existing entry, etc., the definition
> of what an entry is becomes very much relevant.
I agree, and I was trying to discuss the possible alternatives for the
definition of what an entry is in my previous e-mail.
If you think that the alternatives are still too loose, I'm not sure if
we can make it any tighter. As far as I know, we're dealing with
trailers like the following:
Signed-off-by: A <author@example.com>
[This has nothing to do with the above line]
Signed-off-by: B <buthor@example.com>
and:
Link 1: a link
a continuation of the above
and:
Signed-off-by: Some body <some@body.xz> (comment
on two lines)
As I stated in the quoted paragraph, one possibility is to use
indentation and/or balanced parentheses/brackets to determine if a
trailer line continues onto the next line, and this would handle all the
above cases, but I still think that these would lead to surprising
behavior. Hence my suggestion to just simply define it as a single
physical line. But if you think that the pros (of the more complicated
approach) outweigh the cons, I'm OK with that.
One alternative is to postpone this decision by changing sequencer only
(and not trailer) to tolerate other lines in the trailer. This would
make them even more divergent (sequencer supports arbitrary lines while
trailer doesn't), but they were divergent already (sequencer supports
"(cherry picked by" but trailer doesn't).
^ 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