* Re: [WIP RFC 3/5] upload-pack: refactor reading of pack-objects out
From: Junio C Hamano @ 2018-12-05 6:30 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <e19f294df9ff999d30a47339a7848c7104bfae7d.1543879256.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> +struct output_state {
> + char buffer[8193];
> + int used;
> +};
> +
> +static int read_pack_objects_stdout(int outfd, struct output_state *os)
> +{
The naming feels quite odd. We are downstream of pack-objects and
reading its standard output stream, so "read stdout-of-pack-objects"
is not wrong per-se, but it may be just me. Stepping back and what
the function is really about often helps us to understand what we
are doing.
I think the function you extracted from the existing logic is
responsible for relaying the data read from pack-objects to the
fetch-pack client on the other side of the wire. So from that point
of view, singling out "read" as if it is a primary thing the
function does is already suboptimal. Perhaps
static int relay_pack_data(int in, struct output_state *os)
because the fd is what we "read" from (hence, 'in', not 'out'), and
relaying is what we do and reading is only one half of doing so?
> + /* Data ready; we keep the last byte to ourselves
> + * in case we detect broken rev-list, so that we
> + * can leave the stream corrupted. This is
> + * unfortunate -- unpack-objects would happily
> + * accept a valid packdata with trailing garbage,
> + * so appending garbage after we pass all the
> + * pack data is not good enough to signal
> + * breakage to downstream.
> + */
Yeah, I recall writing this funky and unfortunate logic in 2006.
Perhaps we want to update the comment style to make it a bit more
modern?
The "Data ready;" part of the comment applies more to what the
caller of this logic does (i.e. poll returned and revents indicates
we can read, and that is why we are calling into this logic). The
remainder is the comment that is relevat to this logic.
> + ssize_t readsz;
> +
> + readsz = xread(outfd, os->buffer + os->used,
> + sizeof(os->buffer) - os->used);
So we read what we could read in the remaining buffer.
I notice that you alloated 8k+1 bytes; would this code end up
reading that 8k+1 bytes in full under the right condition? I am
mainly wondering where the need for +1 comes from.
> + if (readsz < 0) {
> + return readsz;
> + }
OK, report an error to the caller by returning negative. I am
guessing that you'd have more code in this block that currently have
only one statement in the future steps, perhaps.
> + os->used += readsz;
> +
> + if (os->used > 1) {
> + send_client_data(1, os->buffer, os->used - 1);
> + os->buffer[0] = os->buffer[os->used - 1];
OK, this corresponds to the "*cp++ = buffered" in the original just
before xread().
> + os->used = 1;
> + } else {
> + send_client_data(1, os->buffer, os->used);
> + os->used = 0;
I am not sure if the code is correct when os->used happens to be 1
(shouldn't we hold the byte, skip the call to send_client_data(),
and go back to poll() to expect more data?), but this is a faithful
code movement and rewrite of the original.
I've read the caller (below, snipped) and the conversion looked
sensible there as well. The final flushing of the byte(s) held back
in *os also looks good.
^ permalink raw reply
* Re: [PATCH 8/9] sha1-file: use loose object cache for quick existence check
From: Jeff King @ 2018-12-05 6:51 UTC (permalink / raw)
To: René Scharfe
Cc: Ævar Arnfjörð Bjarmason, Geert Jansen,
Junio C Hamano, git@vger.kernel.org, Takuto Ikuta
In-Reply-To: <54e7a97d-501c-1aa7-55cd-83f070d05a8c@web.de>
On Wed, Dec 05, 2018 at 07:02:17AM +0100, René Scharfe wrote:
> > I actually wrote it that way initially, but doing the hashcpy() in the
> > caller is a bit more awkward. My thought was to punt on that until the
> > rest of the surrounding code starts handling oids.
>
> The level of awkwardness is the same to me, but sha1_loose_object_info()
> is much longer already, so it might feel worse to add it there.
Right, what I meant was that in sha1_loose_object_info():
if (special_case)
handle_special_case();
is easier to follow than a block setting up the special case and then
calling the function.
> This
> function is easily converted to struct object_id, though, as its single
> caller can pass one on -- this makes the copy unnecessary.
If you mean modifying sha1_loose_object_info() to take an oid, then
sure, I agree that is a good step forward (and that is exactly the "punt
until" moment I meant).
> > This patch looks sane. How do you want to handle it? I'd assumed your
> > earlier one would be for applying on top, but this one doesn't have a
> > commit message. Did you want me to squash down the individual hunks?
>
> I'm waiting for the first one (object-store: use one oid_array per
> subdirectory for loose cache) to be accepted, as it aims to solve a
> user-visible performance regression, i.e. that's where the meat is.
> I'm particularly interested in performance numbers from Ævar for it.
>
> I can send the last one properly later, and add patches for converting
> quick_has_loose() to struct object_id. Those are just cosmetic..
Great, thanks. I just wanted to make sure these improvements weren't
going to slip through the cracks.
-Peff
^ permalink raw reply
* Re: [PATCH v3 07/14] checkout: split into switch-branch and restore-files
From: Junio C Hamano @ 2018-12-05 6:56 UTC (permalink / raw)
To: Elijah Newren
Cc: Nguyễn Thái Ngọc, Ævar Arnfjörð,
Git Mailing List, Stefan Beller, Thomas Gummerer, sxenos
In-Reply-To: <CABPp-BGZF8s=ReiC=jTKKQbt1LLO72K7a_2pYbQHrw0ZeA9J5w@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> What depends on stage#2 coming from the commit that will become the
> first parent?
How about "git diff --cc" for a starter? What came from HEAD's
ancestry should appear first and then what came from the side branch
that is merged into.
^ permalink raw reply
* Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences
From: Johannes Sixt @ 2018-12-05 6:57 UTC (permalink / raw)
To: Elijah Newren, Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <CABPp-BG=4K9VCc8zuUm0KTRG5cHPijtvQTK4QXWRVbSFu3o_fQ@mail.gmail.com>
Am 05.12.18 um 07:20 schrieb Elijah Newren:
> On Tue, Dec 4, 2018 at 7:54 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Elijah Newren <newren@gmail.com> writes:
>>
>>> Gah, when I was rebasing on your patch I adopted your sentence rewrite
>>> but forgot to remove the "sometimes". Thanks for catching; correction:
>>
>>>
>>> -- 8< --
>>> Subject: [PATCH v2] git-rebase.txt: update note about directory rename
>>> detection and am
>>>
>>> In commit 6aba117d5cf7 ("am: avoid directory rename detection when
>>> calling recursive merge machinery", 2018-08-29), the git-rebase manpage
>>> probably should have also been updated to note the stronger
>>> incompatibility between git-am and directory rename detection. Update
>>> it now.
>>>
>>> Signed-off-by: Elijah Newren <newren@gmail.com>
>>> ---
>>> Documentation/git-rebase.txt | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>>> index 41631df6e4..ef76cccf3f 100644
>>> --- a/Documentation/git-rebase.txt
>>> +++ b/Documentation/git-rebase.txt
>>> @@ -569,8 +569,12 @@ it to keep commits that started empty.
>>> Directory rename detection
>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> -The merge and interactive backends work fine with
>>> -directory rename detection. The am backend sometimes does not.
>>> +The merge and interactive backends work fine with directory rename
>>
>> I am not sure "work fine" a fair and correct label, as rename is
>> always heuristic.
>>
>> The "directory rename detection" heuristic in "merge" and the
>> "interactive" backends can take what happened to paths in the
>> same directory into account when deciding if a disappeared path
>> was "renamed" and to which other path. The heuristic produces
>> incorrect result when the information given is only about
>> changed paths, which is why it is disabled when using the "am"
>> backend.
>>
>> perhaps.
>
> The general idea sounds good. Does adding a few more details help
> with understanding, or is it more of an information overload? I'm
> thinking of something like:
>
> The "directory rename detection" heuristic in the "merge" and
> "interactive" backends can take what happened to paths in the
> same directory on the other side of history into account when
> deciding whether a new path in that directory should instead be
> moved elsewhere. The heuristic produces incorrect results when
> the only information available is about files which were changed
> on the side of history being rebased, which is why directory
> rename detection is disabled when using the "am" backend.
Please let me deposit my objection. This paragraph is not the right
place to explain what directory renme detection is and how it works
under the hood. "works fine" in the original text is the right phrase
here; if there is concern that this induces expectations that cannot be
met, throw in the word "heuristics".
Such as:
Directory rename heuristics work fine in the merge and interactive
backends. It does not in the am backend because...
-- Hannes
^ permalink raw reply
* Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences
From: Junio C Hamano @ 2018-12-05 7:40 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Elijah Newren, Git Mailing List
In-Reply-To: <76537e8b-3b66-e1f1-eb4d-e9e1c18012df@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Please let me deposit my objection. This paragraph is not the right
> place to explain what directory renme detection is and how it works
> under the hood. "works fine" in the original text is the right phrase
> here; if there is concern that this induces expectations that cannot
> be met, throw in the word "heuristics".
>
> Such as:
> Directory rename heuristics work fine in the merge and interactive
> backends. It does not in the am backend because...
OK, good enough, I guess. Or just s/work fine/is enabled/.
^ permalink raw reply
* Re: [PATCH 8/9] sha1-file: use loose object cache for quick existence check
From: Jeff King @ 2018-12-05 8:15 UTC (permalink / raw)
To: René Scharfe
Cc: Ævar Arnfjörð Bjarmason, Geert Jansen,
Junio C Hamano, git@vger.kernel.org, Takuto Ikuta
In-Reply-To: <20181205065136.GA27263@sigill.intra.peff.net>
On Wed, Dec 05, 2018 at 01:51:36AM -0500, Jeff King wrote:
> > This
> > function is easily converted to struct object_id, though, as its single
> > caller can pass one on -- this makes the copy unnecessary.
>
> If you mean modifying sha1_loose_object_info() to take an oid, then
> sure, I agree that is a good step forward (and that is exactly the "punt
> until" moment I meant).
So the simple thing is to do that, and then have it pass oid->hash to
the other functions it uses. If we start to convert those, there's a
little bit of a rabbit hole, but it's actually not too bad.
Most of the spill-over is into the dumb-http code. Note that it actually
uses sha1 itself! That probably needs to be the_hash_algo (though I'm
not even sure how we'd negotiate the algorithm across a dumb fetch). At
any rate, I don't think this patch makes anything _worse_ in that
respect.
diff --git a/http-push.c b/http-push.c
index cd48590912..0141b0ad53 100644
--- a/http-push.c
+++ b/http-push.c
@@ -255,7 +255,7 @@ static void start_fetch_loose(struct transfer_request *request)
struct active_request_slot *slot;
struct http_object_request *obj_req;
- obj_req = new_http_object_request(repo->url, request->obj->oid.hash);
+ obj_req = new_http_object_request(repo->url, &request->obj->oid);
if (obj_req == NULL) {
request->state = ABORTED;
return;
diff --git a/http-walker.c b/http-walker.c
index 0a392c85b6..29b59e2fe0 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -58,7 +58,7 @@ static void start_object_request(struct walker *walker,
struct active_request_slot *slot;
struct http_object_request *req;
- req = new_http_object_request(obj_req->repo->base, obj_req->oid.hash);
+ req = new_http_object_request(obj_req->repo->base, &obj_req->oid);
if (req == NULL) {
obj_req->state = ABORTED;
return;
@@ -543,11 +543,11 @@ static int fetch_object(struct walker *walker, unsigned char *sha1)
} else if (req->zret != Z_STREAM_END) {
walker->corrupt_object_found++;
ret = error("File %s (%s) corrupt", hex, req->url);
- } else if (!hasheq(obj_req->oid.hash, req->real_sha1)) {
+ } else if (!oideq(&obj_req->oid, &req->real_oid)) {
ret = error("File %s has bad hash", hex);
} else if (req->rename < 0) {
struct strbuf buf = STRBUF_INIT;
- loose_object_path(the_repository, &buf, req->sha1);
+ loose_object_path(the_repository, &buf, &req->oid);
ret = error("unable to write sha1 filename %s", buf.buf);
strbuf_release(&buf);
}
diff --git a/http.c b/http.c
index 7cfa7a16e0..e95b5b9be0 100644
--- a/http.c
+++ b/http.c
@@ -2298,9 +2298,9 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,
}
struct http_object_request *new_http_object_request(const char *base_url,
- unsigned char *sha1)
+ const struct object_id *oid)
{
- char *hex = sha1_to_hex(sha1);
+ char *hex = oid_to_hex(oid);
struct strbuf filename = STRBUF_INIT;
struct strbuf prevfile = STRBUF_INIT;
int prevlocal;
@@ -2311,10 +2311,10 @@ struct http_object_request *new_http_object_request(const char *base_url,
freq = xcalloc(1, sizeof(*freq));
strbuf_init(&freq->tmpfile, 0);
- hashcpy(freq->sha1, sha1);
+ oidcpy(&freq->oid, oid);
freq->localfile = -1;
- loose_object_path(the_repository, &filename, sha1);
+ loose_object_path(the_repository, &filename, oid);
strbuf_addf(&freq->tmpfile, "%s.temp", filename.buf);
strbuf_addf(&prevfile, "%s.prev", filename.buf);
@@ -2456,16 +2456,16 @@ int finish_http_object_request(struct http_object_request *freq)
}
git_inflate_end(&freq->stream);
- git_SHA1_Final(freq->real_sha1, &freq->c);
+ git_SHA1_Final(freq->real_oid.hash, &freq->c);
if (freq->zret != Z_STREAM_END) {
unlink_or_warn(freq->tmpfile.buf);
return -1;
}
- if (!hasheq(freq->sha1, freq->real_sha1)) {
+ if (!oideq(&freq->oid, &freq->real_oid)) {
unlink_or_warn(freq->tmpfile.buf);
return -1;
}
- loose_object_path(the_repository, &filename, freq->sha1);
+ loose_object_path(the_repository, &filename, &freq->oid);
freq->rename = finalize_object_file(freq->tmpfile.buf, filename.buf);
strbuf_release(&filename);
diff --git a/http.h b/http.h
index d305ca1dc7..66c52b2e1e 100644
--- a/http.h
+++ b/http.h
@@ -224,8 +224,8 @@ struct http_object_request {
CURLcode curl_result;
char errorstr[CURL_ERROR_SIZE];
long http_code;
- unsigned char sha1[20];
- unsigned char real_sha1[20];
+ struct object_id oid;
+ struct object_id real_oid;
git_SHA_CTX c;
git_zstream stream;
int zret;
@@ -234,7 +234,7 @@ struct http_object_request {
};
extern struct http_object_request *new_http_object_request(
- const char *base_url, unsigned char *sha1);
+ const char *base_url, const struct object_id *oid);
extern void process_http_object_request(struct http_object_request *freq);
extern int finish_http_object_request(struct http_object_request *freq);
extern void abort_http_object_request(struct http_object_request *freq);
diff --git a/object-store.h b/object-store.h
index fecbb7e094..265d0d8e1f 100644
--- a/object-store.h
+++ b/object-store.h
@@ -151,11 +151,13 @@ void raw_object_store_clear(struct raw_object_store *o);
/*
* Put in `buf` the name of the file in the local object database that
- * would be used to store a loose object with the specified sha1.
+ * would be used to store a loose object with the specified oid.
*/
-const char *loose_object_path(struct repository *r, struct strbuf *buf, const unsigned char *sha1);
+const char *loose_object_path(struct repository *r, struct strbuf *buf,
+ const struct object_id *oid);
-void *map_sha1_file(struct repository *r, const unsigned char *sha1, unsigned long *size);
+void *map_loose_object(struct repository *r, const struct object_id *oid,
+ unsigned long *size);
extern void *read_object_file_extended(const struct object_id *oid,
enum object_type *type,
diff --git a/sha1-file.c b/sha1-file.c
index 3ddf4c9426..0705709036 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -333,12 +333,12 @@ int raceproof_create_file(const char *path, create_file_fn fn, void *cb)
return ret;
}
-static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)
+static void fill_loose_path(struct strbuf *buf, const struct object_id *oid)
{
int i;
for (i = 0; i < the_hash_algo->rawsz; i++) {
static char hex[] = "0123456789abcdef";
- unsigned int val = sha1[i];
+ unsigned int val = oid->hash[i];
strbuf_addch(buf, hex[val >> 4]);
strbuf_addch(buf, hex[val & 0xf]);
if (!i)
@@ -348,19 +348,19 @@ static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)
static const char *odb_loose_path(struct object_directory *odb,
struct strbuf *buf,
- const unsigned char *sha1)
+ const struct object_id *oid)
{
strbuf_reset(buf);
strbuf_addstr(buf, odb->path);
strbuf_addch(buf, '/');
- fill_sha1_path(buf, sha1);
+ fill_loose_path(buf, oid);
return buf->buf;
}
const char *loose_object_path(struct repository *r, struct strbuf *buf,
- const unsigned char *sha1)
+ const struct object_id *oid)
{
- return odb_loose_path(r->objects->odb, buf, sha1);
+ return odb_loose_path(r->objects->odb, buf, oid);
}
/*
@@ -721,7 +721,7 @@ static int check_and_freshen_odb(struct object_directory *odb,
int freshen)
{
static struct strbuf path = STRBUF_INIT;
- odb_loose_path(odb, &path, oid->hash);
+ odb_loose_path(odb, &path, oid);
return check_and_freshen_file(path.buf, freshen);
}
@@ -879,15 +879,15 @@ int git_open_cloexec(const char *name, int flags)
* Note that it may point to static storage and is only valid until another
* call to stat_sha1_file().
*/
-static int stat_sha1_file(struct repository *r, const unsigned char *sha1,
- struct stat *st, const char **path)
+static int stat_loose_object(struct repository *r, const struct object_id *oid,
+ struct stat *st, const char **path)
{
struct object_directory *odb;
static struct strbuf buf = STRBUF_INIT;
prepare_alt_odb(r);
for (odb = r->objects->odb; odb; odb = odb->next) {
- *path = odb_loose_path(odb, &buf, sha1);
+ *path = odb_loose_path(odb, &buf, oid);
if (!lstat(*path, st))
return 0;
}
@@ -900,7 +900,7 @@ static int stat_sha1_file(struct repository *r, const unsigned char *sha1,
* descriptor. See the caveats on the "path" parameter above.
*/
static int open_sha1_file(struct repository *r,
- const unsigned char *sha1, const char **path)
+ const struct object_id *oid, const char **path)
{
int fd;
struct object_directory *odb;
@@ -909,7 +909,7 @@ static int open_sha1_file(struct repository *r,
prepare_alt_odb(r);
for (odb = r->objects->odb; odb; odb = odb->next) {
- *path = odb_loose_path(odb, &buf, sha1);
+ *path = odb_loose_path(odb, &buf, oid);
fd = git_open(*path);
if (fd >= 0)
return fd;
@@ -922,19 +922,16 @@ static int open_sha1_file(struct repository *r,
}
static int quick_has_loose(struct repository *r,
- const unsigned char *sha1)
+ const struct object_id *oid)
{
- int subdir_nr = sha1[0];
- struct object_id oid;
+ int subdir_nr = oid->hash[0];
struct object_directory *odb;
- hashcpy(oid.hash, sha1);
-
prepare_alt_odb(r);
for (odb = r->objects->odb; odb; odb = odb->next) {
odb_load_loose_cache(odb, subdir_nr);
if (oid_array_lookup(&odb->loose_objects_cache[subdir_nr],
- &oid) >= 0)
+ oid) >= 0)
return 1;
}
return 0;
@@ -944,8 +941,8 @@ static int quick_has_loose(struct repository *r,
* Map the loose object at "path" if it is not NULL, or the path found by
* searching for a loose object named "sha1".
*/
-static void *map_sha1_file_1(struct repository *r, const char *path,
- const unsigned char *sha1, unsigned long *size)
+static void *map_loose_object_1(struct repository *r, const char *path,
+ const struct object_id *oid, unsigned long *size)
{
void *map;
int fd;
@@ -953,7 +950,7 @@ static void *map_sha1_file_1(struct repository *r, const char *path,
if (path)
fd = git_open(path);
else
- fd = open_sha1_file(r, sha1, &path);
+ fd = open_sha1_file(r, oid, &path);
map = NULL;
if (fd >= 0) {
struct stat st;
@@ -972,10 +969,11 @@ static void *map_sha1_file_1(struct repository *r, const char *path,
return map;
}
-void *map_sha1_file(struct repository *r,
- const unsigned char *sha1, unsigned long *size)
+void *map_loose_object(struct repository *r,
+ const struct object_id *oid,
+ unsigned long *size)
{
- return map_sha1_file_1(r, NULL, sha1, size);
+ return map_loose_object_1(r, NULL, oid, size);
}
static int unpack_sha1_short_header(git_zstream *stream,
@@ -1045,7 +1043,9 @@ static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
return -1;
}
-static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long size, const unsigned char *sha1)
+static void *unpack_loose_rest(git_zstream *stream,
+ void *buffer, unsigned long size,
+ const struct object_id *oid)
{
int bytes = strlen(buffer) + 1;
unsigned char *buf = xmallocz(size);
@@ -1082,10 +1082,10 @@ static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s
}
if (status < 0)
- error(_("corrupt loose object '%s'"), sha1_to_hex(sha1));
+ error(_("corrupt loose object '%s'"), oid_to_hex(oid));
else if (stream->avail_in)
error(_("garbage at end of loose object '%s'"),
- sha1_to_hex(sha1));
+ oid_to_hex(oid));
free(buf);
return NULL;
}
@@ -1164,9 +1164,9 @@ int parse_sha1_header(const char *hdr, unsigned long *sizep)
return parse_sha1_header_extended(hdr, &oi, 0);
}
-static int sha1_loose_object_info(struct repository *r,
- const unsigned char *sha1,
- struct object_info *oi, int flags)
+static int loose_object_info(struct repository *r,
+ const struct object_id *oid,
+ struct object_info *oi, int flags)
{
int status = 0;
unsigned long mapsize;
@@ -1191,15 +1191,15 @@ static int sha1_loose_object_info(struct repository *r,
const char *path;
struct stat st;
if (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))
- return quick_has_loose(r, sha1) ? 0 : -1;
- if (stat_sha1_file(r, sha1, &st, &path) < 0)
+ return quick_has_loose(r, oid) ? 0 : -1;
+ if (stat_loose_object(r, oid, &st, &path) < 0)
return -1;
if (oi->disk_sizep)
*oi->disk_sizep = st.st_size;
return 0;
}
- map = map_sha1_file(r, sha1, &mapsize);
+ map = map_loose_object(r, oid, &mapsize);
if (!map)
return -1;
@@ -1211,22 +1211,22 @@ static int sha1_loose_object_info(struct repository *r,
if ((flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE)) {
if (unpack_sha1_header_to_strbuf(&stream, map, mapsize, hdr, sizeof(hdr), &hdrbuf) < 0)
status = error(_("unable to unpack %s header with --allow-unknown-type"),
- sha1_to_hex(sha1));
+ oid_to_hex(oid));
} else if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
status = error(_("unable to unpack %s header"),
- sha1_to_hex(sha1));
+ oid_to_hex(oid));
if (status < 0)
; /* Do nothing */
else if (hdrbuf.len) {
if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, flags)) < 0)
status = error(_("unable to parse %s header with --allow-unknown-type"),
- sha1_to_hex(sha1));
+ oid_to_hex(oid));
} else if ((status = parse_sha1_header_extended(hdr, oi, flags)) < 0)
- status = error(_("unable to parse %s header"), sha1_to_hex(sha1));
+ status = error(_("unable to parse %s header"), oid_to_hex(oid));
if (status >= 0 && oi->contentp) {
- *oi->contentp = unpack_sha1_rest(&stream, hdr,
- *oi->sizep, sha1);
+ *oi->contentp = unpack_loose_rest(&stream, hdr,
+ *oi->sizep, oid);
if (!*oi->contentp) {
git_inflate_end(&stream);
status = -1;
@@ -1292,7 +1292,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,
return -1;
/* Most likely it's a loose object. */
- if (!sha1_loose_object_info(r, real->hash, oi, flags))
+ if (!loose_object_info(r, real, oi, flags))
return 0;
/* Not a loose object; someone else may have just packed it. */
@@ -1420,7 +1420,7 @@ void *read_object_file_extended(const struct object_id *oid,
die(_("replacement %s not found for %s"),
oid_to_hex(repl), oid_to_hex(oid));
- if (!stat_sha1_file(the_repository, repl->hash, &st, &path))
+ if (!stat_loose_object(the_repository, repl, &st, &path))
die(_("loose object %s (stored in %s) is corrupt"),
oid_to_hex(repl), path);
@@ -1620,7 +1620,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,
static struct strbuf tmp_file = STRBUF_INIT;
static struct strbuf filename = STRBUF_INIT;
- loose_object_path(the_repository, &filename, oid->hash);
+ loose_object_path(the_repository, &filename, oid);
fd = create_tmpfile(&tmp_file, filename.buf);
if (fd < 0) {
@@ -2196,7 +2196,7 @@ static int check_stream_sha1(git_zstream *stream,
/*
* This size comparison must be "<=" to read the final zlib packets;
- * see the comment in unpack_sha1_rest for details.
+ * see the comment in unpack_loose_rest for details.
*/
while (total_read <= size &&
(status == Z_OK ||
@@ -2245,7 +2245,7 @@ int read_loose_object(const char *path,
*contents = NULL;
- map = map_sha1_file_1(the_repository, path, NULL, &mapsize);
+ map = map_loose_object_1(the_repository, path, NULL, &mapsize);
if (!map) {
error_errno(_("unable to mmap %s"), path);
goto out;
@@ -2267,7 +2267,7 @@ int read_loose_object(const char *path,
if (check_stream_sha1(&stream, hdr, *size, path, expected_oid->hash) < 0)
goto out;
} else {
- *contents = unpack_sha1_rest(&stream, hdr, *size, expected_oid->hash);
+ *contents = unpack_loose_rest(&stream, hdr, *size, expected_oid);
if (!*contents) {
error(_("unable to unpack contents of %s"), path);
git_inflate_end(&stream);
diff --git a/streaming.c b/streaming.c
index ac7c7a22f9..9049146bc1 100644
--- a/streaming.c
+++ b/streaming.c
@@ -338,8 +338,8 @@ static struct stream_vtbl loose_vtbl = {
static open_method_decl(loose)
{
- st->u.loose.mapped = map_sha1_file(the_repository,
- oid->hash, &st->u.loose.mapsize);
+ st->u.loose.mapped = map_loose_object(the_repository,
+ oid, &st->u.loose.mapsize);
if (!st->u.loose.mapped)
return -1;
if ((unpack_sha1_header(&st->z,
^ permalink raw reply related
* Re: [RFC PATCH 3/3] test-lib: add the '--stress' option to run a test repeatedly under load
From: SZEDER Gábor @ 2018-12-05 10:34 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20181205054408.GE12284@sigill.intra.peff.net>
Just a quick reply to this one point for now:
On Wed, Dec 05, 2018 at 12:44:09AM -0500, Jeff King wrote:
> On Tue, Dec 04, 2018 at 05:34:57PM +0100, SZEDER Gábor wrote:
> > + job_nr=0
> > + while test $job_nr -lt "$job_count"
> > + do
> > + wait
> > + job_nr=$(($job_nr + 1))
> > + done
>
> Do we need to loop? Calling "wait" with no arguments should wait for all
> children.
It should, but in dash, ksh, ksh93, Bash v4.2 or older it doesn't seem
to do so, at least not on my machine, and I get back my shell prompt
right after hitting ctrl-C or the first failed test, and then get the
progress output from the remaining jobs later.
Bash 4.3 or later are strange: I get back the shell prompt immediately
after ctrl-C as well, so it doesn't appear to be waiting for all
remaining jobs to finish either, but! I don't get any of the progress
output from those jobs to mess up my next command.
And mksh and zsh can't run our tests, and I don't have any more shells
at hand to try.
^ permalink raw reply
* Re: [RFC PATCH 3/3] test-lib: add the '--stress' option to run a test repeatedly under load
From: SZEDER Gábor @ 2018-12-05 12:07 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King
In-Reply-To: <87muplyxfn.fsf@evledraar.gmail.com>
On Tue, Dec 04, 2018 at 07:11:08PM +0100, Ævar Arnfjörð Bjarmason wrote:
> It's a frequent annoyance of mine in the test suite that I'm
> e.g. running t*.sh with some parallel "prove" in one screen, and then I
> run tABCD*.sh manually, and get unlucky because they use the same trash
> dir, and both tests go boom.
>
> You can fix that with --root, which is much of what this patch does. My
> one-liner for doing --stress has been something like:
>
> perl -E 'say ++$_ while 1' | parallel --jobs=100% --halt-on-error soon,fail=1 './t0000-basic.sh --root=trash-{} -v'
>
> But it would be great if I didn't have to worry about that and could
> just run two concurrent:
>
> ./t0000-basic.sh
>
> So I think we could just set some env variable where instead of having
> the predictable trash directory we have a $TRASHDIR.$N as this patch
> does, except we pick $N by locking some test-runs/tABCD.Nth file with
> flock() during the run.
Is 'flock' portable? It doesn't appear so.
> Then a stress mode like this would just be:
>
> GIT_TEST_FLOCKED_TRASH_DIRS=1 perl -E 'say ++$_ while 1' | parallel --jobs=100% --halt-on-error soon,fail=1 './t0000-basic.sh'
This doesn't address the issue of TCP ports for various daemons.
> And sure, we could ship a --stress option too, but it wouldn't be
> magical in any way, just another "spawn N in a loop" implementation, but
> you could also e.g. use GNU parallel to drive it, and without needing to
GNU 'parallel' may or may not be available on the platform, that's why
I stuck with the barebones shell-loops-in-the-background approach.
> decide to stress test in advance, since we'd either flock() the trash
> dir, or just mktemp(1)-it.
While 'mktemp' seems to be more portable than 'flock', it doesn't seem
to be portable enough; at least it's not in POSIX.
And in general I'd prefer deterministic trash directory names. If I
re-run a failed test after fixing the bug, then currently the trash
directory will be overwritten, and that's good. With 'mktemp's junk
in the trash direcory's name it won't be overwritten, and if my bugfix
doesn't work, then I'll start accumulating trash directories and won't
even know which one is from the last run.
^ permalink raw reply
* Re: [PATCH 2/3] test-lib-functions: introduce the 'test_set_port' helper function
From: SZEDER Gábor @ 2018-12-05 12:20 UTC (permalink / raw)
To: Jeff King; +Cc: git, Luke Diamand
In-Reply-To: <20181205051709.GD12284@sigill.intra.peff.net>
On Wed, Dec 05, 2018 at 12:17:09AM -0500, Jeff King wrote:
> On Tue, Dec 04, 2018 at 05:34:56PM +0100, SZEDER Gábor wrote:
>
> > Several test scripts run daemons like 'git-daemon' or Apache, and
> > communicate with them through TCP sockets. To have unique ports where
> > these daemons are accessible, the ports are usually the number of the
> > corresponding test scripts, unless the user overrides them via
> > environment variables, and thus all those tests and test libs contain
> > more or less the same bit of one-liner boilerplate code to find out
> > the port. The last patch in this series will make this a bit more
> > complicated.
> >
> > Factor out finding the port for a daemon into the common helper
> > function 'test_set_port' to avoid repeating ourselves.
>
> OK. Looks like this should keep the same port numbers for all of the
> existing tests, which I think is good.
Not for all existing tests, though: t0410 and the 'git p4' tests do
get different ports.
Hrm, speaking of affecting 'git p4' tests... I should have put Luke
on Cc, so doing it now.
^ permalink raw reply
* Re: [RFC PATCH 3/3] test-lib: add the '--stress' option to run a test repeatedly under load
From: SZEDER Gábor @ 2018-12-05 14:01 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20181205054408.GE12284@sigill.intra.peff.net>
On Wed, Dec 05, 2018 at 12:44:09AM -0500, Jeff King wrote:
> On Tue, Dec 04, 2018 at 05:34:57PM +0100, SZEDER Gábor wrote:
>
> > To prevent the several parallel invocations of the same test from
> > interfering with each other:
> >
> > - Include the parallel job's number in the name of the trash
> > directory and the various output files under 't/test-results/' as
> > a '.stress-<Nr>' suffix.
>
> Makes sense.
>
> > - Add the parallel job's number to the port number specified by the
> > user or to the test number, so even tests involving daemons
> > listening on a TCP socket can be stressed.
>
> In my script I just use an arbitrary sequence of ports. That means two
> stress runs may stomp on each other, but stress runs tend not to
> interfere with regular runs (whereas with your scheme, a stress run of
> t5561 is going to stomp on t5562). It probably doesn't matter much
> either way, as I tend not to do too much with the machine during a
> stress run.
A custom port can always be set via environment variables, though the
user has to remember to set it (I doubt that I would remember it until
reminded by test failures...)
> > - Make '--stress' imply '--verbose-log' and discard the test's
> > standard ouput and error; dumping the output of several parallel
> > tests to the terminal would create a big ugly mess.
>
> Makes sense. My script just redirects the output to a file, but it
> predates --verbose-log.
>
> My script always runs with "-x". I guess it's not that hard to add it if
> you want, but it is annoying to finally get a failure after several
> minutes only to realize that your are missing some important
> information. ;)
>
> Ditto for "-i", which leaves more readable output (you know the
> interesting part is at the end of the log), and leaves a trash directory
> state that is more likely to be useful for inspecting.
I wanted to imply only the bare minimum of options that are necessary
to prevent the tests from stomping on each other. Other than that I
agree and wouldn't run '--stress' without '-x -i' myself, and at one
point considered to recommend '-x -i' in the description of
'--stress'.
> My script also implies "--root". That's not strictly necessary, though I
> suspect it is much more likely to catch races when it's run on a ram
> disk (simply because it leaves the CPU as the main bottleneck).
>
> > 'wait' for all parallel jobs before exiting (either because a failure
> > was found or because the user lost patience and aborted the stress
> > test), allowing the still running tests to finish. Otherwise the "OK
> > X.Y" progress output from the last iteration would likely arrive after
> > the user got back the shell prompt, interfering with typing in the
> > next command. OTOH, this waiting might induce a considerable delay
> > between hitting ctrl-C and the test actually exiting; I'm not sure
> > this is the right tradeoff.
>
> If there is a delay, it's actually quite annoying to _not_ wait; you
> start doing something in the terminal, and then a bunch of extra test
> statuses print at a random time.
At least the INT/TERM/etc. handler should say something like "Waiting
for background jobs to finish", to let the user know why it doesn't
exit right away.
An alternative would be to exit right away, and make the background
loops a tad bit smarter to not print these progress lines when
aborting.
> > + job_nr=0
> > + while test $job_nr -lt "$job_count"
> > + do
> > + (
> > + GIT_TEST_STRESS_STARTED=done
> > + GIT_TEST_STRESS_JOB_NR=$job_nr
> > + export GIT_TEST_STRESS_STARTED GIT_TEST_STRESS_JOB_NR
> > +
> > + cnt=0
> > + while ! test -e "$stressfail"
> > + do
> > + if $TEST_SHELL_PATH "$0" "$@" >/dev/null 2>&1
> > + then
> > + printf >&2 "OK %2d.%d\n" $GIT_TEST_STRESS_JOB_NR $cnt
>
> OK, this adds a counter for each job number (compared to my script).
> Seems helpful.
>
> > + elif test -f "$stressfail" &&
> > + test "$(cat "$stressfail")" = "aborted"
> > + then
> > + printf >&2 "ABORTED %2d.%d\n" $GIT_TEST_STRESS_JOB_NR $cnt
> > + else
> > + printf >&2 "FAIL %2d.%d\n" $GIT_TEST_STRESS_JOB_NR $cnt
> > + echo $GIT_TEST_STRESS_JOB_NR >>"$stressfail"
> > + fi
>
> Hmm. What happens if we see a failure _and_ there's an abort? That's
> actually pretty plausible if you see a failure whiz by, and you want to
> abort the remaining scripts because they take a long time to run.
>
> If the abort happens first, then the goal is to assume any errors are
> due to the abort.
Yeah, that's why I added this ABORTED case. When I hit ctrl-C, a
couple of the tests tend to fail, but that's not the rare failure we
are hoping to find when stress testing, so printing FAIL would be
misleading. The test script exits with 1 all the same, so we can't
tell the difference, but since rare failures are, well, rare, this
failure is most likely due to the abort.
> But there's a race between the individual jobs reading
> $stressfail and the parent signal handler writing it. So you may end up
> with "aborted\n5" or similar (after which any other jobs would fail to
> match "aborted" and declare themselves failures, as well). I think my
> script probably also has this race, too (it doesn't check the abort case
> explicitly, but it would race in checking "test -e fail").
>
> If the fail happens first (which I think is the more likely case), then
> the abort handler will overwrite the file with "aborted", and we'll
> forget that there was a real failure. This works in my script because of
> the symlinking (if a real failure symlinked $fail to a directory, then
> the attempt to write "aborted" will just be a noop).
OK, I think the abort handler should append, not overwrite, and then
the file's contents should be checked e.g. with a case statement
looking for *aborted*.
Or use two separate files for signaling abort and test failure.
In any case, you could always grab the job number from the "FAIL X.Y"
line, and then look at its logs and trash direcory.
> > + job_nr=0
> > + while test $job_nr -lt "$job_count"
> > + do
> > + wait
> > + job_nr=$(($job_nr + 1))
> > + done
>
> Do we need to loop? Calling "wait" with no arguments should wait for all
> children.
>
> > + if test -f "$stressfail" && test "$(cat "$stressfail")" != "aborted"
> > + then
> > + echo "Log(s) of failed test run(s) be found in:"
> > + for f in $(cat "$stressfail")
> > + do
> > + echo " $TEST_RESULTS_BASE.stress-$f.out"
> > + done
> > + fi
>
> In my experience, outputting the failed log saves a step (especially
> with "-i"), since seeing the failed test and its output is often
> sufficient.
I just don't like when test output, especially with '-x', is dumped on
my terminal :)
> I'm also sad to lose the symlink to the failed trash directory, which
> saves cutting and pasting when you have to inspect it. But I guess we
> can't rely on symlinks everywhere.
You can tab-complete most of the trash directory, and then there are
only those 1-2-3 digits of the job number that you have to type. That
worked out well enough for me so far (but I've only used it for a few
days, so...).
We could rename the trash directory of the failed test to something
short, sweet, and common, but that could be racy in the unlikely case
that two tests fail at the same time. And I like the 'trash
directory.' prefix, because then 'make clean' will delete that one,
too, without modifying 't/Makefile' (which is also the reason for
adding the 'stress-N' suffix instead of a prefix, and putting the fail
file into the 'test-results' directory).
^ permalink raw reply
* Re: [RFC PATCH 3/3] test-lib: add the '--stress' option to run a test repeatedly under load
From: Ævar Arnfjörð Bjarmason @ 2018-12-05 14:01 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Jeff King
In-Reply-To: <20181205120725.GK30222@szeder.dev>
On Wed, Dec 05 2018, SZEDER Gábor wrote:
> On Tue, Dec 04, 2018 at 07:11:08PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> It's a frequent annoyance of mine in the test suite that I'm
>> e.g. running t*.sh with some parallel "prove" in one screen, and then I
>> run tABCD*.sh manually, and get unlucky because they use the same trash
>> dir, and both tests go boom.
>>
>> You can fix that with --root, which is much of what this patch does. My
>> one-liner for doing --stress has been something like:
>>
>> perl -E 'say ++$_ while 1' | parallel --jobs=100% --halt-on-error soon,fail=1 './t0000-basic.sh --root=trash-{} -v'
>>
>> But it would be great if I didn't have to worry about that and could
>> just run two concurrent:
>>
>> ./t0000-basic.sh
>>
>> So I think we could just set some env variable where instead of having
>> the predictable trash directory we have a $TRASHDIR.$N as this patch
>> does, except we pick $N by locking some test-runs/tABCD.Nth file with
>> flock() during the run.
>
> Is 'flock' portable? It doesn't appear so.
Portable enough, and since it's just an alias for "grab lock atomically"
it can devolve into other more basic FS functions on systems that don't
have it.
>> Then a stress mode like this would just be:
>>
>> GIT_TEST_FLOCKED_TRASH_DIRS=1 perl -E 'say ++$_ while 1' | parallel --jobs=100% --halt-on-error soon,fail=1 './t0000-basic.sh'
>
> This doesn't address the issue of TCP ports for various daemons.
Once we have a test ABCD and slot offset N (for a fixed size of N) we
can pick a port from that.
>> And sure, we could ship a --stress option too, but it wouldn't be
>> magical in any way, just another "spawn N in a loop" implementation, but
>> you could also e.g. use GNU parallel to drive it, and without needing to
>
> GNU 'parallel' may or may not be available on the platform, that's why
> I stuck with the barebones shell-loops-in-the-background approach.
Yes, my point is not that you should drop that part of your patch for
using GNU parallel, but just to demonstrate that we can get pretty close
to this for most tests with an external tool, and that it would make
this sort of thing work for concurrent tests without needing to decide
to concurrently test in advance.
>> decide to stress test in advance, since we'd either flock() the trash
>> dir, or just mktemp(1)-it.
>
> While 'mktemp' seems to be more portable than 'flock', it doesn't seem
> to be portable enough; at least it's not in POSIX.>
We are already relying on stuff like mktemp() being reliable for
e.g. the split index.
> And in general I'd prefer deterministic trash directory names. If I
> re-run a failed test after fixing the bug, then currently the trash
> directory will be overwritten, and that's good. With 'mktemp's junk
> in the trash direcory's name it won't be overwritten, and if my bugfix
> doesn't work, then I'll start accumulating trash directories and won't
> even know which one is from the last run.
In general you wouldn't need to set GIT_TEST_FLOCKED_TRASH_DIRS=1 and
would get the same monolithic trash names as now, and for something like
the flock() version it could use a job number as a suffix like your
patch does.
^ permalink raw reply
* Any way to make git-log to enumerate commits?
From: Konstantin Kharlamov @ 2018-12-05 14:22 UTC (permalink / raw)
To: git
It would be great if git-log has a formatting option to insert an index of the current commit since HEAD.
It would allow after quitting the git-log to immediately fire up "git rebase -i HEAD~index" instead of "git rebase -i go-copy-paste-this-long-number-id".
^ permalink raw reply
* Re: [RFC PATCH 3/3] test-lib: add the '--stress' option to run a test repeatedly under load
From: SZEDER Gábor @ 2018-12-05 14:39 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King
In-Reply-To: <87wooof4xm.fsf@evledraar.gmail.com>
On Wed, Dec 05, 2018 at 03:01:41PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> decide to stress test in advance, since we'd either flock() the trash
> >> dir, or just mktemp(1)-it.
> >
> > While 'mktemp' seems to be more portable than 'flock', it doesn't seem
> > to be portable enough; at least it's not in POSIX.>
>
> We are already relying on stuff like mktemp() being reliable for
> e.g. the split index.
That's the mktemp() function from libc; I meant the 'mktemp' command
that we would use this early in 'test-lib.sh', where PATH has not been
set up for testing yet.
^ permalink raw reply
* Re: Any way to make git-log to enumerate commits?
From: Konstantin Khomoutov @ 2018-12-05 14:54 UTC (permalink / raw)
To: Konstantin Kharlamov; +Cc: git
In-Reply-To: <5e5c6d1c-6b3e-c94a-17be-a2af518c1607@yandex.ru>
On Wed, Dec 05, 2018 at 05:22:14PM +0300, Konstantin Kharlamov wrote:
> It would be great if git-log has a formatting option to insert an
> index of the current commit since HEAD.
>
> It would allow after quitting the git-log to immediately fire up "git
> rebase -i HEAD~index" instead of "git rebase -i
> go-copy-paste-this-long-number-id".
This may have little sense in a general case as the history maintained
by Git is a graph, not a single line. Hence your prospective approach
would only work for cases like `git log` called with the
"--first-parent" command-line option.
Still, for a simple approach you may code it right away yourself.
Say, let's create an alias:
$ git config alias.foo '!git log "$@" --pretty=oneline --source | {
n=0;
while read sha ref rest; do
printf "%s\t%s~%s\t%s\n" "$sha" "$ref" $n "$rest"
n=$((n+1))
done
}'
Now calling `git foo --abbrev=commit` would output something like
9be8e297d HEAD~7 Frobincated fizzle
where "7" is what you're looking for.
A more roubst solution may need to use the `git rev-list` command.
^ permalink raw reply
* Re: Any way to make git-log to enumerate commits?
From: Elijah Newren @ 2018-12-05 15:31 UTC (permalink / raw)
To: kostix; +Cc: hi-angel, Git Mailing List
In-Reply-To: <20181205145419.vbbaghzzrnceez45@tigra>
On Wed, Dec 5, 2018 at 6:56 AM Konstantin Khomoutov <kostix@bswap.ru> wrote:
>
> On Wed, Dec 05, 2018 at 05:22:14PM +0300, Konstantin Kharlamov wrote:
>
> > It would be great if git-log has a formatting option to insert an
> > index of the current commit since HEAD.
> >
> > It would allow after quitting the git-log to immediately fire up "git
> > rebase -i HEAD~index" instead of "git rebase -i
> > go-copy-paste-this-long-number-id".
>
> This may have little sense in a general case as the history maintained
> by Git is a graph, not a single line. Hence your prospective approach
> would only work for cases like `git log` called with the
> "--first-parent" command-line option.
>
> Still, for a simple approach you may code it right away yourself.
> Say, let's create an alias:
>
> $ git config alias.foo '!git log "$@" --pretty=oneline --source | {
> n=0;
> while read sha ref rest; do
> printf "%s\t%s~%s\t%s\n" "$sha" "$ref" $n "$rest"
> n=$((n+1))
> done
> }'
>
> Now calling `git foo --abbrev=commit` would output something like
>
> 9be8e297d HEAD~7 Frobincated fizzle
>
> where "7" is what you're looking for.
>
> A more roubst solution may need to use the `git rev-list` command.
Or, just use name-rev so it works with non-linear histories too:
git log | git name-rev --refs=$(git symbolic-ref HEAD) --stdin | less
That'll add things like "master~7" for stuff in the first parent
history and output like "master~7^2~3" for commits on side-branches,
either of which can be fed to other git commands.
^ permalink raw reply
* Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences
From: Elijah Newren @ 2018-12-05 15:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Git Mailing List
In-Reply-To: <xmqqr2ewbevt.fsf@gitster-ct.c.googlers.com>
On Tue, Dec 4, 2018 at 11:40 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Johannes Sixt <j6t@kdbg.org> writes:
>
> > Please let me deposit my objection. This paragraph is not the right
> > place to explain what directory renme detection is and how it works
> > under the hood. "works fine" in the original text is the right phrase
> > here; if there is concern that this induces expectations that cannot
> > be met, throw in the word "heuristics".
> >
> > Such as:
> > Directory rename heuristics work fine in the merge and interactive
> > backends. It does not in the am backend because...
>
> OK, good enough, I guess. Or just s/work fine/is enabled/.
So...
Directory rename heuristics are enabled in the merge and interactive
backends. They are not in the am backend because it operates on
"fake ancestors" that involve trees containing only the files modified
in the patch. Due to the lack of accurate tree information, directory
rename detection is disabled for the am backend.
?
^ permalink raw reply
* Re: Any way to make git-log to enumerate commits?
From: Andreas Schwab @ 2018-12-05 15:43 UTC (permalink / raw)
To: Elijah Newren; +Cc: kostix, hi-angel, Git Mailing List
In-Reply-To: <CABPp-BHhYgBndaOjTC9-YQ5gNJXqaw21Hf=FZzyA7AKDQvN+0A@mail.gmail.com>
On Dez 05 2018, Elijah Newren <newren@gmail.com> wrote:
> Or, just use name-rev so it works with non-linear histories too:
>
> git log | git name-rev --refs=$(git symbolic-ref HEAD) --stdin | less
That wouldn't work for a detached HEAD, though, and you need to use
--no-abbrev.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply
* Re: [RFC] cherry-pick notes to find out cherry-picks from the origin
From: Tejun Heo @ 2018-12-05 16:20 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, kernel-team
In-Reply-To: <20181115144044.GA16450@sigill.intra.peff.net>
Hello, Jeff.
On Thu, Nov 15, 2018 at 09:40:44AM -0500, Jeff King wrote:
> Sorry for the slow reply. This was on my to-look-at pile, but for
> some reason I accidentally put in my done pile.
No worries and sorry about my late reply too. Things were a bit
hectic.
> > * A new built-in command note-cherry-picks, which walks the specified
> > commits and if they're marked with the cherry-pick trailer, adds the
> > backlink to the origin commit using Cherry-picked-to tag in a
> > cherry-picks note.
>
> That makes sense. I think this could also be an option to cherry-pick,
> to instruct it to create the note when the cherry-pick is made.
>
> But you may still want a command to backfill older cherry-picks, or
> those done by other people who do not care themselves about maintaining
> the notes tree.
So, I wanted to do both with the same command. git-cherry-pick knows
which commits are new, so it can just pass those commits to
note-cherry-picks. When backfilling the whole tree or newly pulled
commits, the appropriate command can invoke note-cherry-picks with the
new commits which should be super cheap.
> It _feels_ like this is something that should be do-able by plugging a
> few commands together, rather than writing a new C program. I.e.,
> something like:
>
> git rev-list --format='%(trailers)' HEAD |
> perl -lne '
> /^commit ([0-9]+)/ and $commit = $1;
> /^\(cherry picked from commit ([0-9]+)/
> and print "$commit $1";
> ' |
> while read from to; do
> # One process per note isn't very efficient. Ideally there would
> # be an "append --stdin" mode. Double points if it understands
> # how to avoid adding existing lines.
> git notes append -m "Cherry-picked-to: $to" $from
> done
>
> which is roughly what your program is doing. Not that I'm entirely
> opposed to doing something in C (we've been moving away from shell
> scripts anyway). But mostly I am wondering if we can leverage existing
But the above wouldn't clean up stale commits, which could happen with
e.g. abandoned releases, and would be prone to creating duplicates.
We sure can add all those to shell / perl scripts but it's difficult
for me to see the upsides of doing it that way.
> tools, and fill in their gaps in a way that lets people easily do
> similar things.
I'll respond to this together below.
> And on that note...
>
> > * When formatting a cherry-picks note for display, nested cherry-picks
> > are followed from each Cherry-picked-to tag and printed out with
> > matching indentations.
>
> That makes sense to me, but does this have to be strictly related to
> cherry-picks? I.e., in the more generic form, could we have a way of
> marking a note as "transitive" for display, and the notes-display code
> would automatically recognize and walk hashes?
>
> That would serve your purpose, but would also allow similar things to
> easily be done in the future.
Below.
> > Combined with name-rev --stdin, it can produce outputs like the following.
> > [...]
>
> Yeah, that looks pretty good.
>
> > Locally, the notes can be kept up-to-date with a trivial post-commit
> > hook which invokes note-cherry-picks on the new commit; however, I'm
> > having a bit of trouble figuring out a way to keep it up-to-date when
> > multiple trees are involved. AFAICS, there are two options.
> >
> > 1. Ensuring that the notes are always generated on local commits and
> > whenever new commits are received through fetch/pulls.
> >
> > 2. Ensuring that the notes are always generated on local commits and
> > transported with push/pulls.
> >
> > 3. A hybrid approach - also generate notes on the receiving end and
> > ensure that fetch/pulls receives the notes together (ie. similar to
> > --tags option to git-fetch).
> >
> > #1 seems simpler and more robust to me. Unfortunately, I can't see a
> > way to implement any of the three options with the existing hooks.
> > For #1, there's no post-fetch hook. For #2 and #3, there doesn't seem
> > to be a fool-proof way to make sure that the notes are transported
> > together. Any suggestions would be greatly appreciated.
>
> Yeah, I think (1) is the simplest: it becomes a purely local thing that
> you've generated these annotations. Unfortunately, no, I don't think
> there's anything like a post-fetch hook. This might be a good reason to
> have one. One can always do "git fetch && update-notes" of course, but
> having fetch feed the script the set of updated ref tips would be very
> helpful (so you know you can traverse from $old..$new looking for
> cherry-picks).
This sounds great to me.
> I only looked briefly over your implementation, but didn't see anything
> obviously wrong. I do think it would be nice to make it more generic, as
> much as possible. I think the most generic form is really:
>
> traverse-and-show-trailers | invert-trailers | add-notes
>
> In theory I should be able to do the same inversion step on any trailer
> which mentions another commit.
Hmm... yeah, I can definitely separate out the first part into a
separate function, move dedup of notes to add-notes, and then glue
them together.
I'm a bit hesitant to expose the first part as a separate command
without knowing what other use cases would actually look like. I'll
make it so that it's easy to do so when a new use case arises.
> If it is going to stay in C and be cherry-pick-specific, one obvious
> improvement would be to use the notes API directly, rather than spawning
> subprocesses. That should be much more efficient if you have a lot of
> notes to write.
I'll try to make the components more generic and then glue them
together in C and make it use the APIs directly.
Thanks.
--
tejun
^ permalink raw reply
* gitweb: local configuration not found
From: Jonathan Nieder @ 2018-12-05 18:44 UTC (permalink / raw)
To: Martin Mares; +Cc: git, Ævar Arnfjörð Bjarmason
In-Reply-To: <154401401074.29584.11169979442731329694.reportbug@gimli.ms.mff.cuni.cz>
Martin Mares wrote[1]:
> After upgrade to Stretch, gitweb no longer finds the configuration file
> "gitweb_config.perl" in the current directory. However, "man gitweb" still
> mentions this as one of the possible locations of the config file (and
> indeed a useful one when using multiple instances of gitweb).
>
> It was probably broken by Perl dropping "." from the default search path
> for security reasons.
Indeed, perldelta(1) tells me that in 5.24.1 (and 5.26, etc),
Core modules and tools no longer search "." for optional modules
gitweb.perl contains
sub read_config_file {
my $filename = shift;
return unless defined $filename;
# die if there are errors parsing config file
if (-e $filename) {
do $filename;
which implies an @INC search but it is silly --- as the "-e" test
illustrates, this never intended to search @INC.
Documentation says "If you are absolutely certain that you want your
script to load and execute a file from the current directory, then use
a ./ prefix". We can do that, like so:
diff --git i/gitweb/Makefile w/gitweb/Makefile
index cd194d057f..3160b6cc5d 100644
--- i/gitweb/Makefile
+++ w/gitweb/Makefile
@@ -18,7 +18,7 @@ RM ?= rm -f
INSTALL ?= install
# default configuration for gitweb
-GITWEB_CONFIG = gitweb_config.perl
+GITWEB_CONFIG = ./gitweb_config.perl
GITWEB_CONFIG_SYSTEM = /etc/gitweb.conf
GITWEB_CONFIG_COMMON = /etc/gitweb-common.conf
GITWEB_HOME_LINK_STR = projects
but that does not help if someone overrides GITWEB_CONFIG, and besides,
it would be nicer to avoid the possibility of an @INC search altogether.
Another alternative would be to use
local @INC = ('.');
Would that be better?
Advice from someone more versed than I am in perl would be very welcome
(hence the cc to Ævar).
Thanks for reporting and hope that helps,
Jonathan
[1] https://bugs.debian.org/915632
^ permalink raw reply related
* Re: [PATCH 8/9] sha1-file: use loose object cache for quick existence check
From: René Scharfe @ 2018-12-05 18:41 UTC (permalink / raw)
To: Jeff King
Cc: Ævar Arnfjörð Bjarmason, Geert Jansen,
Junio C Hamano, git@vger.kernel.org, Takuto Ikuta
In-Reply-To: <20181205081544.GA9271@sigill.intra.peff.net>
Am 05.12.2018 um 09:15 schrieb Jeff King:
> On Wed, Dec 05, 2018 at 01:51:36AM -0500, Jeff King wrote:
>
>>> This
>>> function is easily converted to struct object_id, though, as its single
>>> caller can pass one on -- this makes the copy unnecessary.
>>
>> If you mean modifying sha1_loose_object_info() to take an oid, then
>> sure, I agree that is a good step forward (and that is exactly the "punt
>> until" moment I meant).
>
> So the simple thing is to do that, and then have it pass oid->hash to
> the other functions it uses.
Yes.
> If we start to convert those, there's a
> little bit of a rabbit hole, but it's actually not too bad.
You don't need to crawl in just for quick_has_loose(), but eventually
everything has to be converted. It seems a bit much for one patch, but
perhaps that's just my ever-decreasing attention span speaking.
Converting one function prototype or struct member at a time seems
about the right amount of change per patch to me. That's not always
possible due to entanglement, of course.
> Most of the spill-over is into the dumb-http code. Note that it actually
> uses sha1 itself! That probably needs to be the_hash_algo (though I'm
> not even sure how we'd negotiate the algorithm across a dumb fetch). At
> any rate, I don't think this patch makes anything _worse_ in that
> respect.
Right.
> diff --git a/sha1-file.c b/sha1-file.c
> index 3ddf4c9426..0705709036 100644
> --- a/sha1-file.c
> +++ b/sha1-file.c
> @@ -333,12 +333,12 @@ int raceproof_create_file(const char *path, create_file_fn fn, void *cb)
> return ret;
> }
>
> -static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)
> +static void fill_loose_path(struct strbuf *buf, const struct object_id *oid)
The new name fits.
> @@ -879,15 +879,15 @@ int git_open_cloexec(const char *name, int flags)
> * Note that it may point to static storage and is only valid until another
> * call to stat_sha1_file().
> */
> -static int stat_sha1_file(struct repository *r, const unsigned char *sha1,
> - struct stat *st, const char **path)
> +static int stat_loose_object(struct repository *r, const struct object_id *oid,
> + struct stat *st, const char **path)
Hmm, read_sha1_file() was renamed to read_object_file() earlier this
year, and I'd have expected this to become stat_object_file(). Names..
Anyway, the comment above and one a few lines below should be updated
as well.
> {
> struct object_directory *odb;
> static struct strbuf buf = STRBUF_INIT;
>
> prepare_alt_odb(r);
> for (odb = r->objects->odb; odb; odb = odb->next) {
> - *path = odb_loose_path(odb, &buf, sha1);
> + *path = odb_loose_path(odb, &buf, oid);
> if (!lstat(*path, st))
> return 0;
> }
> @@ -900,7 +900,7 @@ static int stat_sha1_file(struct repository *r, const unsigned char *sha1,
> * descriptor. See the caveats on the "path" parameter above.
> */
> static int open_sha1_file(struct repository *r,
> - const unsigned char *sha1, const char **path)
> + const struct object_id *oid, const char **path)
That function should lose the "sha1" in its name as well.
> -static void *map_sha1_file_1(struct repository *r, const char *path,
> - const unsigned char *sha1, unsigned long *size)
> +static void *map_loose_object_1(struct repository *r, const char *path,
> + const struct object_id *oid, unsigned long *size)
Similarly, map_object_file_1()?
> -void *map_sha1_file(struct repository *r,
> - const unsigned char *sha1, unsigned long *size)
> +void *map_loose_object(struct repository *r,
> + const struct object_id *oid,
> + unsigned long *size)
Similar.
> @@ -1045,7 +1043,9 @@ static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
> return -1;
> }
>
> -static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long size, const unsigned char *sha1)
> +static void *unpack_loose_rest(git_zstream *stream,
> + void *buffer, unsigned long size,
> + const struct object_id *oid)
Hmm, both old and new name here look weird to me at this point.
> -static int sha1_loose_object_info(struct repository *r,
> - const unsigned char *sha1,
> - struct object_info *oi, int flags)
> +static int loose_object_info(struct repository *r,
> + const struct object_id *oid,
> + struct object_info *oi, int flags)
And nothing of value was lost. :)
René
^ permalink raw reply
* Re: gitweb: local configuration not found
From: Ævar Arnfjörð Bjarmason @ 2018-12-05 19:11 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Martin Mares, git
In-Reply-To: <20181205184404.GC246451@google.com>
On Wed, Dec 05 2018, Jonathan Nieder wrote:
> Martin Mares wrote[1]:
>
>> After upgrade to Stretch, gitweb no longer finds the configuration file
>> "gitweb_config.perl" in the current directory. However, "man gitweb" still
>> mentions this as one of the possible locations of the config file (and
>> indeed a useful one when using multiple instances of gitweb).
>>
>> It was probably broken by Perl dropping "." from the default search path
>> for security reasons.
>
> Indeed, perldelta(1) tells me that in 5.24.1 (and 5.26, etc),
>
> Core modules and tools no longer search "." for optional modules
>
> gitweb.perl contains
>
> sub read_config_file {
> my $filename = shift;
> return unless defined $filename;
> # die if there are errors parsing config file
> if (-e $filename) {
> do $filename;
>
> which implies an @INC search but it is silly --- as the "-e" test
> illustrates, this never intended to search @INC.
>
> Documentation says "If you are absolutely certain that you want your
> script to load and execute a file from the current directory, then use
> a ./ prefix". We can do that, like so:
>
> diff --git i/gitweb/Makefile w/gitweb/Makefile
> index cd194d057f..3160b6cc5d 100644
> --- i/gitweb/Makefile
> +++ w/gitweb/Makefile
> @@ -18,7 +18,7 @@ RM ?= rm -f
> INSTALL ?= install
>
> # default configuration for gitweb
> -GITWEB_CONFIG = gitweb_config.perl
> +GITWEB_CONFIG = ./gitweb_config.perl
> GITWEB_CONFIG_SYSTEM = /etc/gitweb.conf
> GITWEB_CONFIG_COMMON = /etc/gitweb-common.conf
> GITWEB_HOME_LINK_STR = projects
>
> but that does not help if someone overrides GITWEB_CONFIG, and besides,
> it would be nicer to avoid the possibility of an @INC search altogether.
> Another alternative would be to use
>
> local @INC = ('.');
>
> Would that be better?
>
> Advice from someone more versed than I am in perl would be very welcome
> (hence the cc to Ævar).
It seems most sensible to follow the ./FILE.pl advice per
https://metacpan.org/pod/distribution/perl/pod/perl5260delta.pod#Removal-of-the-current-directory-(%22.%22)-from-@INC
Just:
local @INC = '.';
do 'FILE.pl';
Would do the same thing, but seems like a more indirect way to do it if
all we want is ./ anyway. FWIW to be pedantically bug-compatible with
the old version (we should not do this) it's:
local @INC = (@INC, ".");
do 'FILE.pl';
I.e. before our behavior was implicitly to check whether we had a local
FILE.pl, then loop through all of @INC to see if we found it there, and
finally come back to the file we did the -e check for.
^ permalink raw reply
* Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
From: Frank Schäfer @ 2018-12-05 19:29 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, brian m. carlson, git
In-Reply-To: <3572b619-0603-d16d-392c-4cc8e0bc4614@kdbg.org>
Am 02.12.18 um 22:22 schrieb Johannes Sixt:
> Am 02.12.18 um 20:31 schrieb Frank Schäfer:
>> With other words:
>> "If CR comes immediately before a LF, do the following with *all* lines:
>> <RESET> after the CR if eol=lf but do not <RESET> after the CR if
>> eol=crlf."
>
> Why? It is the pager's duty to highlight CR, IMO. If it doesn't, but
> the user wants to see them, then they are using the wrong pager or the
> wrong pager settings.
AFAIU Junios explanation it's not the pagers fault.
>
> As far as I am concerned, I do not have any of my files marked as
> eol=crlf, but I do *not* want to see ^M in the pager. I.e., having git
> insert <RESET> between CR and LF would do the wrong thing for me.
>
But doing the same thing in added lines is doing the right thing for you ?
Or are you suggesting to fix the behavior of added lines instead ?
In any case, inconsistent behavior is not what we want.
Regards,
Frank
^ permalink raw reply
* Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
From: Frank Schäfer @ 2018-12-05 19:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, brian m. carlson, git
In-Reply-To: <xmqqmupnh0lo.fsf@gitster-ct.c.googlers.com>
Am 03.12.18 um 02:15 schrieb Junio C Hamano:
> Frank Schäfer <fschaefer.oss@googlemail.com> writes:
>
>> Hi Junio,
>>
>> Am 29.11.18 um 03:11 schrieb Junio C Hamano:
>> [...]
>>> This was misspoken a bit. Let's revise it to
>>>
>>> When producing a colored output (not limited to whitespace
>>> error coloring of diff output) for contents that are not
>>> marked as eol=crlf (and other historical means), insert
>>> <RESET> before a CR that comes immediately before a LF.
>> You mean
>> ...
>> <RESET> *after* a CR that comes immediately before a LF."
>>
>> OK, AFAICS this produces the desired output in all cases if eol=lf.
> OK, yeah, I think I meant "after", i.e. ... CR <RESET> LF, in order
> to force CR to be separated from LF.
>
>> Now what about the case eol=crlf ?
> I have no strong opinions, other than that "LF in repository, CRLF
> in working tree" would make the issue go away (when it is solved for
> EOL=LF like the above, that is).
>
>> Keeping the current behavior of '-' lines is correct.
>> But shouldn't ^M now be suppressed in '+' lines for a consistent behavior ?
> If "LF in repository, CRLF in working tree" is done, there would not
> be ^M at the end of the line, not just for removed lines, but also
> for added lines, no?
Just to be sure that I'm not missing anything here:
What's your definition of "LF in repository, CRLF in working tree" in
terms of config parameters ?
Regards,
Frank
^ permalink raw reply
* Re: [RFC PATCH 3/3] test-lib: add the '--stress' option to run a test repeatedly under load
From: Ævar Arnfjörð Bjarmason @ 2018-12-05 19:59 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Jeff King
In-Reply-To: <20181205143919.GN30222@szeder.dev>
On Wed, Dec 05 2018, SZEDER Gábor wrote:
> On Wed, Dec 05, 2018 at 03:01:41PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> >> decide to stress test in advance, since we'd either flock() the trash
>> >> dir, or just mktemp(1)-it.
>> >
>> > While 'mktemp' seems to be more portable than 'flock', it doesn't seem
>> > to be portable enough; at least it's not in POSIX.>
>>
>> We are already relying on stuff like mktemp() being reliable for
>> e.g. the split index.
>
> That's the mktemp() function from libc; I meant the 'mktemp' command
> that we would use this early in 'test-lib.sh', where PATH has not been
> set up for testing yet.
Ah, if that ever becomes an issue it's a one-line test-tool wrapper.
^ permalink raw reply
* git, monorepos, and access control
From: Coiner, John @ 2018-12-05 20:13 UTC (permalink / raw)
To: git@vger.kernel.org
Hi,
I'm an engineer with AMD. I'm looking at whether we could switch our
internal version control to a monorepo, possibly one based on git and
VFSForGit.
One obstacle to moving AMD to git/VFSForGit is the lack of access
control support in git. AMD has a lot of data whose distribution must be
limited. Sometimes it's a legal requirement, eg. CPU core designs are
covered by US export control laws and not all employees may see them.
Sometimes it's a contractual obligation, as when a third party shares
data with us and we agree only to share this data with certain
employees. Any hypothetical AMD monorepo should be able to securely deny
read access in certain subtrees to users without required permissions.
Has anyone looked at adding access control to git, at a per-directory
granularity? Is this a feature that the git community would possibly
welcome?
Here's my rough thinking about how it might work:
- an administrator can designate that a tree object requires zero or
more named privileges to read
- when a mortal user attempts to retrieve the tree object, a hook
allows the server to check if the user has a given privilege. The hook
can query an arbitrary user/group data base, LDAP or whatever. The
details of this check are mostly in the hook; git only knows about
abstract named privileges.
- if the user has permission, everything goes as normal.
- if the user lacks permission, they get a DeniedTree object which
might carry some metadata about what permissions would be needed to see
more. The DeniedTree lacks the real tree's entries. (TBD, how do we
render a denied tree in the workspace? An un-writable directory
containing only a GITDENIED file with some user friendly error message?)
- hashes are secret. If the hashes from a protected tree leak, the
data also leaks. No check on the server prevents it from handing out
contents for correctly-guessed hashes.
- mortal users shouldn't be able to alter permissions. Of course,
mortal users will often modify tree objects that carry permissions. So
the server should enforce that a user isn't pushing updates that alter
permissions on the same logical directory.
I would welcome your feedback on whether this idea makes technical
sense, and whether the feature could ever be a fit for git.
You might ask what alternatives we are looking at. At our scale, we'd
really want a version control system that implements a virtual
filesystem. That already limits us to ClearCase, VFSForGit, and maybe
Vesta among public ones. Am I missing any? We would also want one that
permits branching enormous numbers of files without creating enormous
amounts of data in the repo -- git gets that right, and perforce (our
status quo) does not. That's how I got onto the idea of adding read
authorization to git.
Thanks,
John
^ 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