* Re: [PATCH 07/21] Copy the remaining differences from verify_tag() to parse_tag_buffer_internal()
From: Johan Herland @ 2007-06-09 21:39 UTC (permalink / raw)
To: git; +Cc: Alex Riesen, Junio C Hamano, Johannes Schindelin
In-Reply-To: <81b0412b0706091431h3a786aaew342693a667938d9@mail.gmail.com>
On Saturday 09 June 2007, Alex Riesen wrote:
> On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > + /* Verify the tag-name: we don't allow control characters or spaces in it */
> > + for (i = 4;;) {
> > + unsigned char c = tag_line[i++];
> > + if (c == '\n')
> > + break;
> > + if (c > ' ')
> > + continue;
> > + return error("char" PD_FMT ": could not verify tag name", tag_line + i - data);
> > + }
>
> This looks very familiar. Haven't you just made a very useless patch
> which had this very same code? How about putting it in its own
> function and just call it from these two places? And what problem
> do you have with pointers?!
I just answered your comment on the previous patch, and that answer should
apply here as well.
I'm probably splitting this up into too small pieces, since I keep getting
comments that fail to see the overall picture of what I'm trying to do,
namely taking two similar pieces of code and slowly unifying them to the
point where I can replace one of them by a call to the other (see the two
next patches).
Hope this helps,
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: [PATCH 12/21] Use prefixcmp() instead of memcmp() for cleaner code with less magic numbers
From: Alex Riesen @ 2007-06-09 21:42 UTC (permalink / raw)
To: Johan Herland; +Cc: git, Junio C Hamano, Johannes Schindelin
In-Reply-To: <200706090217.49818.johan@herland.net>
On 6/9/07, Johan Herland <johan@herland.net> wrote:
> +/*
> + * Perform parsing and verification of tag object data.
> + *
> + * The 'item' parameter may be set to NULL if only verification is desired.
> + *
> + * The given data _must_ be null-terminated.
> + */
> int parse_and_verify_tag_buffer(struct tag *item,
> const char *data, const unsigned long size, int thorough_verify)
This hunk really belongs into commit which introduced the function
parse_and_verify_tag_buffer.
^ permalink raw reply
* Re: [PATCH 09/21] Remove unneeded code from mktag.c
From: Johan Herland @ 2007-06-09 21:42 UTC (permalink / raw)
To: git; +Cc: Alex Riesen, Junio C Hamano, Johannes Schindelin
In-Reply-To: <81b0412b0706091439q7be180celb939f694b80df159@mail.gmail.com>
On Saturday 09 June 2007, Alex Riesen wrote:
> On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > Signed-off-by: Johan Herland <johan@herland.net>
> > ---
> > mktag.c | 94 ---------------------------------------------------------------
> > 1 files changed, 0 insertions(+), 94 deletions(-)
>
> So, you do some useless changes just to remove the
> function completely afterwards?
Yes. Basically so that people can follow my process. If you don't want the
intermediary/useless states, just look at my first patch series that was
replaced by this series because it was too bulky and disruptive to follow.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: [PATCH 10/21] Free mktag's buffer before dying
From: Johan Herland @ 2007-06-09 21:46 UTC (permalink / raw)
To: git; +Cc: Alex Riesen, Junio C Hamano, Johannes Schindelin
In-Reply-To: <81b0412b0706091437j50070e9aw7c258f136f4d5cde@mail.gmail.com>
On Saturday 09 June 2007, Alex Riesen wrote:
> On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > + if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {
> > + free(buffer);
> > + die("invalid tag data file");
>
> This, and the similar one below are useless. You're destroying the
> process, what do you free that buffer for? Either handle the error
> case or do not needlessly complicate your change, which really
> also absolutely unneeded.
Well, I was taught to treat my memory with care.
Right now it doesn't make any difference in practice (except that
Valgrind might be a bit happier with it), but in the future -- with
the libifaction effort and whatnot -- you never know what might happen
to this piece of code, and I'd like to stay on the safe side.
Feel free to drop this patch from the series if I'm the only one thinking
like this.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: [PATCH 12/21] Use prefixcmp() instead of memcmp() for cleaner code with less magic numbers
From: Johan Herland @ 2007-06-09 21:47 UTC (permalink / raw)
To: git; +Cc: Alex Riesen, Junio C Hamano, Johannes Schindelin
In-Reply-To: <81b0412b0706091442m6594260btd3b898bfb63e1fbb@mail.gmail.com>
On Saturday 09 June 2007, Alex Riesen wrote:
> On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > +/*
> > + * Perform parsing and verification of tag object data.
> > + *
> > + * The 'item' parameter may be set to NULL if only verification is desired.
> > + *
> > + * The given data _must_ be null-terminated.
> > + */
> > int parse_and_verify_tag_buffer(struct tag *item,
> > const char *data, const unsigned long size, int thorough_verify)
>
> This hunk really belongs into commit which introduced the function
> parse_and_verify_tag_buffer.
Yes. I'm sorry it slipped out of that patch and into this one.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: [PATCH 16/21] Introduce optional "keywords" on tag objects
From: Alex Riesen @ 2007-06-09 21:52 UTC (permalink / raw)
To: Johan Herland; +Cc: git, Junio C Hamano, Johannes Schindelin
In-Reply-To: <200706090219.37289.johan@herland.net>
On 6/9/07, Johan Herland <johan@herland.net> wrote:
> This patch introduces a new optional header line to the tag object, called
> "keywords". The "keywords" line may contain a comma-separated list of
> custom keywords associated with the tag object.
What is the character set for the keywords?
> + for (i = 0; i < keywords_len; ++i) {
> + unsigned char c = keywords_line[i];
> + if (c == ',' && keywords_line[i + 1] == ',')
> + /* consecutive commas */
> + return error("Tag object (@ char "
> + PD_FMT "): Found empty keyword",
> + keywords_line + i - data);
> + if (c > ' ' && c != 0x7f)
> + continue;
And what is so special about 0x7f?
> + if (keywords_len) { /* optional keywords string was given */
> + item->keywords = xmalloc(keywords_len + 1);
Who frees the keywords and what's wrong with strndup?
^ permalink raw reply
* Re: [PATCH 10/21] Free mktag's buffer before dying
From: Alex Riesen @ 2007-06-09 22:00 UTC (permalink / raw)
To: Johan Herland; +Cc: git, Junio C Hamano, Johannes Schindelin
In-Reply-To: <200706092346.12359.johan@herland.net>
On 6/9/07, Johan Herland <johan@herland.net> wrote:
> On Saturday 09 June 2007, Alex Riesen wrote:
> > On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > > + if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {
> > > + free(buffer);
> > > + die("invalid tag data file");
> >
> > This, and the similar one below are useless. You're destroying the
> > process, what do you free that buffer for? Either handle the error
> > case or do not needlessly complicate your change, which really
> > also absolutely unneeded.
>
> Well, I was taught to treat my memory with care.
>
How do you treat your performance?
Besides, was that systems with common address space
where you were taught? Like DOS or MacOS, perhaps?
> Right now it doesn't make any difference in practice (except that
> Valgrind might be a bit happier with it), but in the future -- with
> the libifaction effort and whatnot -- you never know what might happen
> to this piece of code, and I'd like to stay on the safe side.
So that people have to check your free as well (they will have to,
they come looking for die-calls). You just made more work for them.
^ permalink raw reply
* Re: [PATCH 16/21] Introduce optional "keywords" on tag objects
From: Johan Herland @ 2007-06-09 22:00 UTC (permalink / raw)
To: Alex Riesen; +Cc: git, Junio C Hamano, Johannes Schindelin
In-Reply-To: <81b0412b0706091452q2957540dy95fbf13ebd89ca1f@mail.gmail.com>
On Saturday 09 June 2007, Alex Riesen wrote:
> On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > This patch introduces a new optional header line to the tag object, called
> > "keywords". The "keywords" line may contain a comma-separated list of
> > custom keywords associated with the tag object.
>
> What is the character set for the keywords?
Hmm. Same as for the "tag" line, and the rest of the tag object, I guess.
I agree this should probably be specified. What are the rules for other
git objects?
> > + for (i = 0; i < keywords_len; ++i) {
> > + unsigned char c = keywords_line[i];
> > + if (c == ',' && keywords_line[i + 1] == ',')
> > + /* consecutive commas */
> > + return error("Tag object (@ char "
> > + PD_FMT "): Found empty keyword",
> > + keywords_line + i - data);
> > + if (c > ' ' && c != 0x7f)
> > + continue;
>
> And what is so special about 0x7f?
Isn't DEL a control char?
> > + if (keywords_len) { /* optional keywords string was given */
> > + item->keywords = xmalloc(keywords_len + 1);
>
> Who frees the keywords and what's wrong with strndup?
The code that should free the tag name should also free the keywords.
No code appear to do this at the moment (regardless of whether you use my patch
series or not), which is a shame. We should make sure all objects are properly
deallocated. This is currently a general problem in git, isn't it?
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: [PATCH 10/21] Free mktag's buffer before dying
From: Johan Herland @ 2007-06-09 22:05 UTC (permalink / raw)
To: git; +Cc: Alex Riesen, Junio C Hamano, Johannes Schindelin
In-Reply-To: <81b0412b0706091500o2c0a8f88xd3808d6ee790d777@mail.gmail.com>
On Sunday 10 June 2007, Alex Riesen wrote:
> On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > On Saturday 09 June 2007, Alex Riesen wrote:
> > > On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > > > + if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {
> > > > + free(buffer);
> > > > + die("invalid tag data file");
> > >
> > > This, and the similar one below are useless. You're destroying the
> > > process, what do you free that buffer for? Either handle the error
> > > case or do not needlessly complicate your change, which really
> > > also absolutely unneeded.
> >
> > Well, I was taught to treat my memory with care.
>
> How do you treat your performance?
Hopefully with care, as well. However, I tend to look at performance _after_
correctness.
> Besides, was that systems with common address space
> where you were taught? Like DOS or MacOS, perhaps?
Nope. Never programmed on either. I thought care with memory was generally
considered a good principle. If I'm wrong, please point me at the relevant
documentation.
> > Right now it doesn't make any difference in practice (except that
> > Valgrind might be a bit happier with it), but in the future -- with
> > the libifaction effort and whatnot -- you never know what might happen
> > to this piece of code, and I'd like to stay on the safe side.
>
> So that people have to check your free as well (they will have to,
> they come looking for die-calls). You just made more work for them.
Ok. Drop it. This isn't particularily important to me. I just try to
follow good principles when I can.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* [PATCH] Whitespace: replace spaces with tabs.
From: William Pursell @ 2007-06-09 22:25 UTC (permalink / raw)
To: git
Two lines of alloc.c contain indentation using spaces, while the
remainder of the file uses tabs. This change brings those 2 lines
into conformance with the predominant indentation style.
---
alloc.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/alloc.c b/alloc.c
index 216c23a..baf9fce 100644
--- a/alloc.c
+++ b/alloc.c
@@ -59,13 +59,13 @@ DEFINE_ALLOCATOR(object, union any_object)
static void report(const char* name, unsigned int count, size_t size)
{
- fprintf(stderr, "%10s: %8u (" SZ_FMT " kB)\n", name, count, size);
+ fprintf(stderr, "%10s: %8u (" SZ_FMT " kB)\n", name, count, size);
}
#undef SZ_FMT
#define REPORT(name) \
- report(#name, name##_allocs, name##_allocs*sizeof(struct name) >> 10)
+ report(#name, name##_allocs, name##_allocs*sizeof(struct name)
>> 10)
void alloc_report(void)
{
--
1.4.4.4
^ permalink raw reply related
* [PATCH] Use xstrndup() instead of xmalloc() and memcpy(); fix buglet with generating default item->keywords.
From: Johan Herland @ 2007-06-09 22:36 UTC (permalink / raw)
To: git; +Cc: Alex Riesen, Junio C Hamano, Johannes Schindelin
In-Reply-To: <81b0412b0706091452q2957540dy95fbf13ebd89ca1f@mail.gmail.com>
Using xstrndup() yields more compact and readable code than using
xmalloc(), memcpy() and manual NUL termination.
Thanks to Alex Riesen <raa.lkml@gmail.com> for suggesting this.
Also fixes a buglet where item->keywords would always be set to "tag",
even if item->tag was empty.
Signed-off-by: Johan Herland <johan@herland.net>
---
On Saturday 09 June 2007, Alex Riesen wrote:
> ... and what's wrong with strndup?
Nothing.
...Johan
tag.c | 27 ++++++++++-----------------
1 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/tag.c b/tag.c
index c3a2855..2307ec9 100644
--- a/tag.c
+++ b/tag.c
@@ -219,26 +219,19 @@ int parse_and_verify_tag_buffer(struct tag *item,
}
if (item) { /* Store parsed information into item */
- if (tag_len) { /* optional tag name was given */
- item->tag = xmalloc(tag_len + 1);
- memcpy(item->tag, tag_line, tag_len);
- item->tag[tag_len] = '\0';
- }
- else { /* optional tag name not given */
- item->tag = xmalloc(1);
- item->tag[0] = '\0';
- }
+ if (tag_len) /* optional tag name was given */
+ item->tag = xstrndup(tag_line, tag_len);
+ else /* optional tag name not given */
+ item->tag = xstrndup("", 0);
- if (keywords_len) { /* optional keywords string was given */
- item->keywords = xmalloc(keywords_len + 1);
- memcpy(item->keywords, keywords_line, keywords_len);
- item->keywords[keywords_len] = '\0';
- }
+ if (keywords_len) /* optional keywords string was given */
+ item->keywords = xstrndup(keywords_line, keywords_len);
else { /* optional keywords string not given. Set default */
/* if tag name is set, use "tag"; else use "note" */
- const char *default_kw = item->tag ? "tag" : "note";
- item->keywords = xmalloc(strlen(default_kw) + 1);
- memcpy(item->keywords, default_kw, strlen(default_kw) + 1);
+ if (*(item->tag))
+ item->keywords = xstrndup("tag", 3);
+ else
+ item->keywords = xstrndup("note", 4);
}
if (!strcmp(type, blob_type)) {
--
1.5.2.1.144.gabc40
^ permalink raw reply related
* Re: [RFC][PATCH 00/10] Sparse: Git's "make check" target
From: Sam Ravnborg @ 2007-06-09 22:56 UTC (permalink / raw)
To: Josh Triplett
Cc: Ramsay Jones, Junio C Hamano, GIT Mailing-list, linux-sparse
In-Reply-To: <466A5204.6060200@freedesktop.org>
>
> Also, you might consider just using cgcc to run both GCC and Sparse. That
> would handle the issue of target-specific CFLAGS, by ensuring that Sparse and
> GCC always see the same CFLAGS.
Is this the recommended way?
I that case I suggest that someone looks into the linux kernel part
and change it to use this method.
Sam
^ permalink raw reply
* Re: Refactoring the tag object; Introducing soft references (softrefs); Git 'notes' (take 2)
From: Steven Grimm @ 2007-06-09 22:57 UTC (permalink / raw)
To: Johan Herland; +Cc: git, Junio C Hamano
In-Reply-To: <200706040251.05286.johan@herland.net>
(Resending this in plaintext; sorry to those who got it twice.)
Being able to specify relationships between commits after the fact seems
like a very useful facility.
Does it make sense to have type information to record what the
relationship between two objects means? Without that, it seems like
it'll be hard to build much of a tool set on top of this feature, since
no two tools that made use of it could unambiguously query just their
own softrefs.
A few use cases for relationships-after-the-fact come to mind in
addition to the one the patch itself mentions:
A facility like this could replace the info/grafts file, or at least
provide another way to turn a regular commit into a merge commit. Just
put a "manually specified merge parent" ref between the target revision
and the one you want git to think you've merged from. That would scale a
lot better than info/grafts does, I suspect, if only by virtue of being
O(log n) searchable thanks to the sorting.
One could easily imagine recording a "cherry picked" softref, which
could, e.g., be the rebase machinery to skip over an already-applied
revision. IMO the lack of any tool-readable history about cherry picking
-- which is, after all, a sort of merge, at least conceptually -- is a
shortcoming in present-day git. (Not a huge one, but if nothing else
it'd be great to see cherry picking represented in, e.g., the gitk
history display.)
It might be possible to annotate rebases to make pulling from rebased
branches less troublesome. If you have
A--B--C--D
\
-E--F--G
and you rebase E onto D, a "rebased from" softref could be recorded
between E and E':
A--B--C--D
\ \
-E....E'--F'--G'
Then a pulling client could potentially use that information to cleanly
replay the rebase operation to keep its history straight. Perhaps if you
could record historical rebases like that, you could do away with the
current gotchas involving rebasing shared repositories. One negative
side effect would be that you'd end up needing to keep E around where
before you'd have been able to throw it away, but it should delta
compress well, and you can, I think, still prune revisions F and G in
the above picture. Or maybe it's enough to just keep E's SHA1 around
without actually retaining its contents.
But in any event, this seems like the start of a useful new set of
capabilities for git.
-Steve
^ permalink raw reply
* Re: Refactoring the tag object; Introducing soft references (softrefs); Git 'notes' (take 2)
From: Johan Herland @ 2007-06-09 23:16 UTC (permalink / raw)
To: Steven Grimm; +Cc: git, Junio C Hamano
In-Reply-To: <466B305A.5080802@midwinter.com>
On Sunday 10 June 2007, Steven Grimm wrote:
> Being able to specify relationships between commits after the fact seems
> like a very useful facility.
>
> Does it make sense to have type information to record what the
> relationship between two objects means? Without that, it seems like
> it'll be hard to build much of a tool set on top of this feature, since
> no two tools that made use of it could unambiguously query just their
> own softrefs.
Actually MadCoder/Pierre had a similar idea on IRC. He wanted to separate
softrefs into namespaces, so that softrefs for tags could live in a
different place than softrefs associated with his "gits" bug tracker.
I haven't thought very much about this, but it's certainly possible to do
something like this. What do the rest of y'all think?
> A few use cases for relationships-after-the-fact come to mind in
> addition to the one the patch itself mentions:
>
> A facility like this could replace the info/grafts file, or at least
> provide another way to turn a regular commit into a merge commit. Just
> put a "manually specified merge parent" ref between the target revision
> and the one you want git to think you've merged from. That would scale a
> lot better than info/grafts does, I suspect, if only by virtue of being
> O(log n) searchable thanks to the sorting.
Yes, I _knew_ this was similar to grafts in some way :) While working on
this, I tried to see if I could leverage grafts somewhere in my design, but
I found them to be too commit-bound and specific. But when you look at it
the other way it seems to make more sense.
> One could easily imagine recording a "cherry picked" softref, which
> could, e.g., be the rebase machinery to skip over an already-applied
> revision. IMO the lack of any tool-readable history about cherry picking
> -- which is, after all, a sort of merge, at least conceptually -- is a
> shortcoming in present-day git. (Not a huge one, but if nothing else
> it'd be great to see cherry picking represented in, e.g., the gitk
> history display.)
>
> It might be possible to annotate rebases to make pulling from rebased
> branches less troublesome. If you have
>
> A--B--C--D
> \
> -E--F--G
>
> and you rebase E onto D, a "rebased from" softref could be recorded
> between E and E':
>
> A--B--C--D
> \ \
> -E....E'--F'--G'
>
> Then a pulling client could potentially use that information to cleanly
> replay the rebase operation to keep its history straight. Perhaps if you
> could record historical rebases like that, you could do away with the
> current gotchas involving rebasing shared repositories. One negative
> side effect would be that you'd end up needing to keep E around where
> before you'd have been able to throw it away, but it should delta
> compress well, and you can, I think, still prune revisions F and G in
> the above picture. Or maybe it's enough to just keep E's SHA1 around
> without actually retaining its contents.
Whoa. I hadn't even imagined this, but I guess you're right. I actually
thought about solving the same problem (using a much worse method) way back
in May [1], but I'd since totally forgotten about it.
> But in any event, this seems like the start of a useful new set of
> capabilities for git.
Thanks a lot for sharing your ideas. :)
Have fun!
...Johan
[1] http://article.gmane.org/gmane.comp.version-control.git/46137
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: [PATCH] Port git-tag.sh to C.
From: Kristian Høgsberg @ 2007-06-09 23:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7iqdf0gn.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano wrote:
> Kristian Høgsberg <krh@redhat.com> writes:
>
>> Content-Type: TEXT/PLAIN; charset=ISO-8859-1
>>
>> From: Kristian Høgsberg <krh@redhat.com>
>>
>> A more or less straight-forward port of git-tag.sh to C.
>>
>> Signed-off-by: Kristian Høgsberg <krh@redhat.com>
>> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>
> I think your name in your commit message is in UTF-8 but munged your
> mail was mismarked as iso-8859-1.
That's odd both the email I cc'ed to my redhat.com address and the one I got
on gmail.com through the list have
Content-Type: text/plain; charset=utf-8
and saving the raw message and asking /usr/bin/file, it tells me its
/home/krh/Desktop/hep: UTF-8 Unicode mail text
>> +static int launch_editor(const char *path, const char *template,
>> + char *buffer, size_t size)
>> +{
>
> It would have been nicer to have this in editor.c or somesuch,
> as other commands will be redone in C in the future.
>
> We could do the moving later, but the problem is that later is
> conditional: "if we are lucky enough to remember that we already
> have this function in builtin-tag when doing so".
Yeah, true. I did write it as a generally usable "launch editor" functions,
but I didn't want to move it until there was a second user. Is there anything
else that git-commit that will use this, btw?
>> + fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0644);
>
> I would understand an argument to use 0666 (honor umask) or 0600
> (this is a temporary file and others have no business looking at
> it while an edit is in progress), but I cannot justify 0644.
Oh hehe, yeah, I didn't think much about the permission bits... 0666 sounds fine.
>> + fd = open(path, O_RDONLY, 0644);
>
> Open for reading with mode ;-)?
Even less thinking here :)
>> + if (fd == -1)
>> + die("could not read %s.", path);
>> + len = read_in_full(fd, buffer, size);
>> + if (len < 0)
>> + die("failed to read '%s', %m", path);
>> + close(fd);
>> +
>> + blank_lines = 1;
>> + for (i = 0, j = 0; i < len; i++) {
>> ...
>> + }
>> +
>> + if (buffer[j - 1] != '\n')
>> + buffer[j++] = '\n';
>> +
>> + unlink(path);
>> +
>> + return j;
>> +}
>
> I really think this function needs to be refactored into three.
>
> * A generic "spawn an editor with this initial seed template,
> return the result of editing in memory and also give exit
> status of the editor" function that does not take path
> parameter (instead perhaps mkstemp a temporary file on your
> own);
>
> * A function that does what git-stripspace does in core;
>
> * A function for builtin-tag to use, that calls the above two
> and uses the result (e.g. "did the user kill the editor?
> does the resulting buffer have any nonempty line?") to decide
> what it does.
and that last function should be useful for git-commit too, no? The reason I
didn't split it up was that I don't see any use for the two parts on their
own, and the functions is a total of 70 lines. And wouldn't returning 0 bytes
for either an empty buffer or editor quit be sufficient? Do we need to handle
the two cases differently?
>> +static void create_tag(const unsigned char *object, const char *tag,
>> + const char *message, int sign, unsigned char *result)
>> +{
>> + enum object_type type;
>> + char buffer[4096];
>> + int header, body, total;
>> +
>> + type = sha1_object_info(object, NULL);
>> + if (type <= 0)
>> + die("bad object type.");
>> +
>> + header = snprintf(buffer, sizeof buffer,
>> + "object %s\n"
>> + "type %s\n"
>> + "tag %s\n"
>> + "tagger %s\n\n",
>> + sha1_to_hex(object),
>> + typename(type),
>> + tag,
>> + git_committer_info(1));
>> +
>> + if (message == NULL)
>> + body = launch_editor(git_path("TAGMSG"), tag_template,
>> + buffer + header, sizeof buffer - header);
>> + else
>> + body = snprintf(buffer + header, sizeof buffer - header,
>> + "%s\n", message);
>> +
>> + if (body == 0)
>> + die("no tag message?");
>> +
>> + if (header + body > sizeof buffer)
>> + die("tag message too big.");
>
> Two issues:
>
> * It used to be a tag had limit of 8kB which was lifted some
> time ago; now it is limited to 4kB. Fixing this implies that
> the "launch editor and get results in core" function I
> mentioned above may need to realloc, and probably the buffer
> is better passed as (char *, ulong) pair as done everywhere
> else (although we know this is text so you can pass only a
> pointer and have the user run strlen() when needed).
Oh... read_pipe() reallocs... OK. I took the limit from mktag.c and didn't
realize that the buffer could get realloced. I did wonder about the xmalloc()
for a fixed buffer though...
> * I do not see any validation on the value of "tag". Do we want
> to allow passing "" to it? What about "my\ntag"?
I do
if (check_ref_format(ref))
die("'%s' is not a valid tag name.", tag);
on the ref, which is what git-tag.sh ends up doing, and it catches the two
examples you mention:
[krh@dinky git]$ ./git tag "hello
> world"
fatal: 'hello
world' is not a valid tag name.
[krh@dinky git]$ ./git tag ""
fatal: '' is not a valid tag name.
so I think it's fine.
But I think I'll leave it to Carlos to finish his builtin-tag work, and he can
cherry pick the bits from my patch that work for him. This was more of a
friday afternoon hacking sprint, for my part.
cheers,
Kristian
^ permalink raw reply
* Re: [RFC][PATCH 00/10] Sparse: Git's "make check" target
From: Josh Triplett @ 2007-06-09 23:50 UTC (permalink / raw)
To: Sam Ravnborg; +Cc: Ramsay Jones, Junio C Hamano, GIT Mailing-list, linux-sparse
In-Reply-To: <20070609225630.GC3008@uranus.ravnborg.org>
[-- Attachment #1: Type: text/plain, Size: 553 bytes --]
Sam Ravnborg wrote:
>> Also, you might consider just using cgcc to run both GCC and Sparse. That
>> would handle the issue of target-specific CFLAGS, by ensuring that Sparse and
>> GCC always see the same CFLAGS.
>
> Is this the recommended way?
> I that case I suggest that someone looks into the linux kernel part
> and change it to use this method.
The approach taken by Linux allows running sparse on files without recompiling
them. Using CC=cgcc just makes for less work, but the kernel has that work
done now.
- Josh Triplett
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]
^ permalink raw reply
* Comment on weak refs
From: Junio C Hamano @ 2007-06-09 23:55 UTC (permalink / raw)
To: Johan Herland; +Cc: git, Linus Torvalds, Pierre Habouzit
In-Reply-To: <200706092019.13185.johan@herland.net>
The patch series to look-up and maintain "softref" relationship
is trivially clean. Although I probably would have many nits to
pick, I do not think it is _wrong_ in a major way per-se. I
would not even mind saying that I liked the basic concept, until
I thought things a bit deeper.
Here are some initial notes I took while reading your patches.
Semantics
---------
Not all "softref" relationship is equal. "This object is
referred to by these tags" is one obvious application, and only
because we already try to follow tags when git-fetch happens
anyway, it looks natural to make everybody follow such a softref
relationship.
However, as Pierre Habouzit wants to, we may want to make a bug
tracking sheet (the details of the implementation of such a bug
tracking sheet does not matter in this discussion -- it could be
a blob, a commit, or a tag) refer to commits using this
mechanism, by pointing at the blob from commits after the fact
(i.e. "later it was verified that this commit fixes the bug
described in this bug entry").
Side note: I am assuming a simplest implementation where
one blob would always capture the latest status of the
bug. refs/bugs/12127 would point at the latest version
of bug 12127's tracking sheet. An alternative
implementation would be to represent each entry of the
tracking sheet for a single bug as a blob, and have
multiple blobs associated to a commit on the main
project, or the other way around, but then you would
need a way to give order between referers to a single
referent, which I do not find in your proposed
"softref".
Most users who want to download and compile the main project do
not care about bug tracker objects. You would need to have a
way to describe what kind of relationship a particular softlink
represents, and adjust the definition of reachability for the
purposes of traversal of objects.
It gets worse when you actually start using softrefs. I do not
think you would have a limited set of softrefs, such as
"reverse-tag-lookup-softref", "bug-tracker-softref". For
example, a typical bug tracking sheet may look like this:
- Hey I found a bug, you can reproduce like so... I am
testing commit _A_.
- It appears that commit _B_ introduced it; it does not
reproduce with anything older.
- Here is a patch to fix it; please try.
- Oh, it seems to fix. Committed as _C_.
- No, it does not work for me, and the approach to fix is
wrong; reopening the bug.
- Ok, take 2 with different approach. Committed as _D_.
please try.
- Thanks, it finally fixes it. Closing the bug.
The bug will be associated with commits A, B, C and D. The
questions maintainers would want to ask are:
- What caused this bug?
- Which versions (or earlier) have this bug?
- Which versions (or later) have proper fix?
- What alternate approaches were taken to fix this bug?
- In this upcoming release, which bugs have been fixed?
- What bugs are still open after this release?
Depending on what you want to find out, you would need to ask
which commits are related to this bug tracking sheet object, and
the answer has to be different. Some "softref" relation should
extend to its ancestry (when "this fixes" is attached to a
commit, its children ought to inherit that property), some
shouldn't ("this is what broke it" should not propagate to its
parent nor child).
It is also unclear how relationship "softref" introduces is
propagated across repositories (not objects the softref binds,
but the fact that such a binding between two objects exists need
to be propagated). I would imagine that your assumption is
simply "to take union". IOW, if you say A refers to B and
transfer object A to the other side, in addition to transfering
object B (if the other side does not have it yet), you would
tell the other side that B is related to A and have the other
side add that to its set of softrefs. Techinically it is a
simple and easy to implement semantics, but I suspect that would
not necessarily be useful in practice. Maybe two people would
disagree if A and B are related or not. Maybe you first think A
and B are related and then later change your mind. Should
"softref" relationships be versioned?
Reachability
------------
The association brought in between referent and referer by
softref is "weak", in that referer needs to exist only if
referent need to be there. This has the following
consequences.
Fsck/prune/lost-found
.....................
The current object traversal starts from "known tip objects"
(i.e. refs, HEAD, index, and reflog entries when not doing
lost-found) and follows the reachability link embedded in
referer objects (i.e. tag to tagged object, commit to tree, tree
to tree and blob). We only need to extend this "reachability"
with softref. If a referer object refers to another object via
a softref, we consider referent reachable and we are done.
This should be reasonably straightforward, except that we
probably would need to worry about circular references that
softlink makes possible.
push/fetch/rev-list --objects
.............................
We walk the revision range (object transfers essentially starts
traversal from the tips of the sender until it meets what the
receiver already has), enumerating the reachable objects. I
suspect that adding reachability with softref to this scheme has
consequences on performance.
Imagine:
A---B---C---D---E
The sender's tip is at E and the receiver claims to have C. In
the sender's repository, E is associated with A (somebody
noticed that E fixes regression introduced by A, and added a
softlink to make A reachable from E). Currently we only need to
know C is reachable from E to decide that we do not have to send
A again, but with softlink we would need to.
The ancestry chain of referent and referer do not have to share
any common commits. Imagine a bug tracking system where each
bug's tracking sheet is represented as a DAG of commits (this
will allow you to merge and split bugs easily). This history
would not share any tree nor blob with the history of the source
code of the project. And you would make a commit in the main
project associated with objects in the bug tracking project
using softrefs. As sender and receiver exchanges the commit
ancestry information on the main project, both ends may need to
negotiate which objects in the bug tracking history are already
present in the reciever.
One attractive point of softref is that you do not have to
anchor referents with explicit refs. E.g. if a commit in the
main project is associated with bug tracking entries in the "bug
tracking" project via softrefs, that is enough to keep the bug
tracking objects alive. But I suspect this property makes the
enumeration of "what do we have on this end" costly. I dunno.
Come to think of it, and this is off topic for "softref", I
think using an isolated commit DAG for each bug is probably a
very natural way to implement a bug tracking system. If you
want to refer to commits on the mainline, you can refer to them
by their object names, just like mainline commit log messages
would refer to earlier commits in the text (e.g. "This fixes the
regression introduced by commit ABC"). The new text entry for
traditional BTS will go to the commit message (a bug "project"
commit does not have to have anything in its tree), and you can
use "git log" to view what the ordinary BTS would provide, and
it will distribute the tracking. You do not necessarily
need/benefit from "softref" for this, though. It could be that
such a bug "project" commit might have a commit from the main
project in its tree (it would look like a subproject) whose tree
entry name may be something like "fixes/yyyy-mm-yy-author".
^ permalink raw reply
* Re: [PATCH 16/21] Introduce optional "keywords" on tag objects
From: Junio C Hamano @ 2007-06-10 0:05 UTC (permalink / raw)
To: Alex Riesen; +Cc: Johan Herland, git, Junio C Hamano, Johannes Schindelin
In-Reply-To: <81b0412b0706091452q2957540dy95fbf13ebd89ca1f@mail.gmail.com>
"Alex Riesen" <raa.lkml@gmail.com> writes:
> On 6/9/07, Johan Herland <johan@herland.net> wrote:
>> This patch introduces a new optional header line to the tag object, called
>> "keywords". The "keywords" line may contain a comma-separated list of
>> custom keywords associated with the tag object.
>
> What is the character set for the keywords?
>
>> + for (i = 0; i < keywords_len; ++i) {
>> + unsigned char c = keywords_line[i];
>> + if (c == ',' && keywords_line[i + 1] == ',')
>> + /* consecutive commas */
>> + return error("Tag object (@ char "
>> + PD_FMT "): Found empty keyword",
>> + keywords_line + i - data);
>> + if (c > ' ' && c != 0x7f)
>> + continue;
>
> And what is so special about 0x7f?
It is DEL, but as the code uses uchar, it probably also error on
0x80 or higher, if the intent is "printable ASCII".
^ permalink raw reply
* Re: [PATCH 1/3] git-submodule: allow submodule name and path to differ
From: Junio C Hamano @ 2007-06-10 0:27 UTC (permalink / raw)
To: Lars Hjemli; +Cc: Johannes Schindelin, Sven Verdoolaege, git
In-Reply-To: <1181425132294-git-send-email-hjemli@gmail.com>
Lars Hjemli <hjemli@gmail.com> writes:
> This teaches git-submodule to check module.*.path when looking for the
> config for a submodule path. If no match is found it falls back to the
> current behaviour (module.$path).
I have a feeling that it might be much less troublesome in the
longer term to admit that module.$path was a mistake and support
only one format; wouldn't trying to support both leave ambiguity
and confusion?
^ permalink raw reply
* Re: [PATCH 2/3] Add gitmodules(5)
From: Frank Lichtenheld @ 2007-06-10 0:28 UTC (permalink / raw)
To: Lars Hjemli; +Cc: Junio C Hamano, Johannes Schindelin, Sven Verdoolaege, git
In-Reply-To: <11814251322779-git-send-email-hjemli@gmail.com>
The unofficial git documentation nitpicker at work ;)
On Sat, Jun 09, 2007 at 11:38:51PM +0200, Lars Hjemli wrote:
> +The `.gitmodules` file, located in the top-level directory of a
> +gitlink:git[7] working tree, is a text file with a layout matching the
That link seems superfluous to me.
I would have used "syntax" instead of "layout".
> +requirements of gitlink:git-config[1].
> +
> +The file consists of sections named `module`, divided into one subsection
> +per submodule. The subsections are named with the logical name of the
> +submodule it describes.
"sections named module" sounds confusing to me. Why are there multiple
sections named module? (for the record: I know what you mean, I just
don't know if it couldn't be said simpler)
Maybe better "subsections of section `module`, one per submodule"?
Hmm, sounds ugly too.
> +Each submodule can contain the following keys.
> +
> +module.$name.path::
> + Define a path, relative to the top-level directory of the git
> + working tree, where the submodule is expected to be checked out.
> +
> +module.$name.url::
> + Define a url from where the submodule repository can be cloned.
For .path a "Defaults to name of submodule" probably wouldn't hurt.
For the sake of documentation consistency I would suggest
module.<name>.path. You can compare the output of
$ grep "\.<[a-z]" Documentation/*.txt
with
$ grep "\.\$[a-z]" Documentation/*.txt
to see what I mean.
> + [module 'libfoo']
> + path = include/foo
> + url = git://example1.com/git/libfoo.git
> +
> + [module 'libbar']
> + url = git://example2.com/pub/git/libbar.git
This would actually be a syntax error in a git config file
(subsection names can be enclosed in "" but not '').
Gruesse,
--
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/
^ permalink raw reply
* Re: fatal: serious inflate inconsistency
From: Linus Torvalds @ 2007-06-10 0:30 UTC (permalink / raw)
To: Thomas Glanzmann; +Cc: Marco Roeland, Nicolas Pitre, GIT, Michael Gernoth
In-Reply-To: <20070609211213.GA18761@cip.informatik.uni-erlangen.de>
On Sat, 9 Jun 2007, Thomas Glanzmann wrote:
>
> the machine that had the issues just got down because of a broken power
> supply and took half the rack with it. So it was definitive a hardware
> fault. Sorry for all the trouble. And the bottom line is: It was _not_
> git, it was broken hardware!
Good. That was one of the design goals for git (ie the "you know you can
trust the data" thing relies on very strong protection at all levels,
even in the presense of disk/memory/cpu corruption).
I think we have a pretty good record on this: the only real git-induced
corruption I can remember was the embarrassing bug with use-after-free of
the CRLF auto-translation. But let's not get complacent.
Linus
^ permalink raw reply
* [PATCH] Fail if tag name and keywords is not within "printable ASCII"
From: Johan Herland @ 2007-06-10 0:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alex Riesen, git, Johannes Schindelin
In-Reply-To: <7vfy50d67q.fsf@assigned-by-dhcp.cox.net>
Signed-off-by: Johan Herland <johan@herland.net>
---
On Sunday 10 June 2007, Junio C Hamano wrote:
> "Alex Riesen" <raa.lkml@gmail.com> writes:
> > And what is so special about 0x7f?
>
> It is DEL, but as the code uses uchar, it probably also error on
> 0x80 or higher, if the intent is "printable ASCII".
Is this better?
...Johan
tag.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tag.c b/tag.c
index 2307ec9..2b92465 100644
--- a/tag.c
+++ b/tag.c
@@ -183,8 +183,8 @@ int parse_and_verify_tag_buffer(struct tag *item,
/* Verify tag name: disallow control characters or spaces */
if (tag_len) { /* tag name was given */
for (i = 0; i < tag_len; ++i) {
- unsigned char c = tag_line[i];
- if (c > ' ' && c != 0x7f)
+ char c = tag_line[i];
+ if (c > ' ' && c < 0x7f)
continue;
return FAIL("Tag object (@ char " PD_FMT "): "
"Could not verify tag name",
@@ -198,13 +198,13 @@ int parse_and_verify_tag_buffer(struct tag *item,
*/
if (keywords_len) { /* keywords line was given */
for (i = 0; i < keywords_len; ++i) {
- unsigned char c = keywords_line[i];
+ char c = keywords_line[i];
if (c == ',' && keywords_line[i + 1] == ',')
/* consecutive commas */
return FAIL("Tag object (@ char "
PD_FMT "): Found empty keyword",
keywords_line + i - data);
- if (c > ' ' && c != 0x7f)
+ if (c > ' ' && c < 0x7f)
continue;
return FAIL("Tag object (@ char " PD_FMT "): "
"Could not verify keywords",
--
1.5.2.1.144.gabc40
^ permalink raw reply related
* [PATCH] Move check for already parsed tag object to parse_tag_buffer() wrapper function
From: Johan Herland @ 2007-06-10 0:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin
In-Reply-To: <7vlketf1m6.fsf@assigned-by-dhcp.cox.net>
This effectively disables the check when creating and fscking tag objects,
which is desirable because we don't want the fact that the object is
already parsed to stop us from verifying thoroughly.
Signed-off-by: Johan Herland <johan@herland.net>
---
On Saturday 09 June 2007, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > diff --git a/tag.c b/tag.c
> > index 19c66cd..b134967 100644
> > --- a/tag.c
> > +++ b/tag.c
> > @@ -46,9 +46,11 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u
> > const char *type_line, *tag_line, *tagger_line;
> > unsigned long type_len, tag_len;
> >
> > - if (item->object.parsed)
> > - return 0;
> > - item->object.parsed = 1;
> > + if (item) {
> > + if (item->object.parsed)
> > + return 0;
> > + item->object.parsed = 1;
> > + }
>
> Passing both item and data does not feel right. If you are
> trying to make the factored out function to do the verification
> of data, then perhaps the caller should do the "don't handle the
> same data twice" optimization using item?
You mean, like this?
...Johan
tag.c | 10 ++++------
1 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/tag.c b/tag.c
index 2b92465..4e79943 100644
--- a/tag.c
+++ b/tag.c
@@ -75,12 +75,6 @@ int parse_and_verify_tag_buffer(struct tag *item,
unsigned long type_len, tag_len, keywords_len, tagger_len;
const char *header_end;
- if (item) {
- if (item->object.parsed)
- return 0;
- item->object.parsed = 1;
- }
-
if (size < 65)
return FAIL("Tag object failed preliminary size check");
@@ -264,6 +258,10 @@ int parse_and_verify_tag_buffer(struct tag *item,
int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
{
+ if (item->object.parsed)
+ return 0;
+ item->object.parsed = 1;
+
return parse_and_verify_tag_buffer(item, (const char *) data, size, 0);
}
--
1.5.2.1.144.gabc40
^ permalink raw reply related
* Re: fatal: serious inflate inconsistency
From: Martin Langhoff @ 2007-06-10 0:50 UTC (permalink / raw)
To: Thomas Glanzmann; +Cc: Linus Torvalds, Nicolas Pitre, GIT, Michael Gernoth
In-Reply-To: <20070608193319.GS2335@cip.informatik.uni-erlangen.de>
On 6/9/07, Thomas Glanzmann <thomas@glanzmann.de> wrote:
> I have a very similar machine. For this one it works. It has 32 bit kernel and
> userland. Debian etch again. v2.6.20.
Wanted to mention -- we've been using git on debian sarge/etch on
several arches a lot and never seen any lib compatiblity problem.
> Does someone know a command for debian that checks the checksums
apt-get install debsums
man debsums # sorry, I don't use it often enough to remember ;-)
cheers,
m
^ permalink raw reply
* Re: Comment on weak refs
From: Johan Herland @ 2007-06-10 1:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Linus Torvalds, Pierre Habouzit
In-Reply-To: <7vk5ucd6of.fsf_-_@assigned-by-dhcp.cox.net>
On Sunday 10 June 2007, Junio C Hamano wrote:
> The patch series to look-up and maintain "softref" relationship
> is trivially clean. Although I probably would have many nits to
> pick, I do not think it is _wrong_ in a major way per-se. I
> would not even mind saying that I liked the basic concept, until
> I thought things a bit deeper.
>
> Here are some initial notes I took while reading your patches.
>
>
> Semantics
> ---------
>
> Not all "softref" relationship is equal. "This object is
> referred to by these tags" is one obvious application, and only
> because we already try to follow tags when git-fetch happens
> anyway, it looks natural to make everybody follow such a softref
> relationship.
>
> However, as Pierre Habouzit wants to, we may want to make a bug
> tracking sheet (the details of the implementation of such a bug
> tracking sheet does not matter in this discussion -- it could be
> a blob, a commit, or a tag) refer to commits using this
> mechanism, by pointing at the blob from commits after the fact
> (i.e. "later it was verified that this commit fixes the bug
> described in this bug entry").
>
> Side note: I am assuming a simplest implementation where
> one blob would always capture the latest status of the
> bug. refs/bugs/12127 would point at the latest version
> of bug 12127's tracking sheet. An alternative
> implementation would be to represent each entry of the
> tracking sheet for a single bug as a blob, and have
> multiple blobs associated to a commit on the main
> project, or the other way around, but then you would
> need a way to give order between referers to a single
> referent, which I do not find in your proposed
> "softref".
>
> Most users who want to download and compile the main project do
> not care about bug tracker objects. You would need to have a
> way to describe what kind of relationship a particular softlink
> represents, and adjust the definition of reachability for the
> purposes of traversal of objects.
Yes, I'm starting to see that it's not a good idea to put _all_ softrefs in
one bag.
> It gets worse when you actually start using softrefs. I do not
> think you would have a limited set of softrefs, such as
> "reverse-tag-lookup-softref", "bug-tracker-softref". For
> example, a typical bug tracking sheet may look like this:
>
> - Hey I found a bug, you can reproduce like so... I am
> testing commit _A_.
> - It appears that commit _B_ introduced it; it does not
> reproduce with anything older.
> - Here is a patch to fix it; please try.
> - Oh, it seems to fix. Committed as _C_.
> - No, it does not work for me, and the approach to fix is
> wrong; reopening the bug.
> - Ok, take 2 with different approach. Committed as _D_.
> please try.
> - Thanks, it finally fixes it. Closing the bug.
>
> The bug will be associated with commits A, B, C and D. The
> questions maintainers would want to ask are:
>
> - What caused this bug?
> - Which versions (or earlier) have this bug?
> - Which versions (or later) have proper fix?
> - What alternate approaches were taken to fix this bug?
> - In this upcoming release, which bugs have been fixed?
> - What bugs are still open after this release?
>
> Depending on what you want to find out, you would need to ask
> which commits are related to this bug tracking sheet object, and
> the answer has to be different. Some "softref" relation should
> extend to its ancestry (when "this fixes" is attached to a
> commit, its children ought to inherit that property), some
> shouldn't ("this is what broke it" should not propagate to its
> parent nor child).
We're getting a little ahead of ourselves, aren't we? IMHO, it would be up
to the bug system to determine which (and how many) connections to make
between the bug reports and the commits (or even if softrefs would be the
correct mechanism for these connections at all). We shouldn't necessarily
base the softrefs design on how we imagine a hypothetical bug system to
work. But Pierre might have something to say on how he would want to use
softrefs, and his system is hopefully _less_ hypothetical. :)
But I can see the use of letting the user/porcelain/bugtracker define
classes/namespaces of softrefs (at runtime).
> It is also unclear how relationship "softref" introduces is
> propagated across repositories (not objects the softref binds,
> but the fact that such a binding between two objects exists need
> to be propagated). I would imagine that your assumption is
> simply "to take union". IOW, if you say A refers to B and
> transfer object A to the other side, in addition to transfering
> object B (if the other side does not have it yet), you would
> tell the other side that B is related to A and have the other
> side add that to its set of softrefs. Techinically it is a
> simple and easy to implement semantics, but I suspect that would
> not necessarily be useful in practice. Maybe two people would
> disagree if A and B are related or not.
Yes, I see that different classes of softrefs would have different semantics
for propagation. we could probably try to set up some sane defaults, and
then let users put rules in their configs for how they would want to
propagate the various softrefs classes.
> Maybe you first think A
> and B are related and then later change your mind. Should
> "softref" relationships be versioned?
Intriguing idea. Not immediately sure how we would implement it though...
> Reachability
> ------------
>
> The association brought in between referent and referer by
> softref is "weak", in that referer needs to exist only if
> referent need to be there. This has the following
> consequences.
>
> Fsck/prune/lost-found
> .....................
>
> The current object traversal starts from "known tip objects"
> (i.e. refs, HEAD, index, and reflog entries when not doing
> lost-found) and follows the reachability link embedded in
> referer objects (i.e. tag to tagged object, commit to tree, tree
> to tree and blob). We only need to extend this "reachability"
> with softref. If a referer object refers to another object via
> a softref, we consider referent reachable and we are done.
Agreed.
> This should be reasonably straightforward, except that we
> probably would need to worry about circular references that
> softlink makes possible.
Isn't there a .used flag on objects we could easily check to see if we've
seen an object before, thus preventing us from following the circular
reference?
> push/fetch/rev-list --objects
> .............................
>
> We walk the revision range (object transfers essentially starts
> traversal from the tips of the sender until it meets what the
> receiver already has), enumerating the reachable objects. I
> suspect that adding reachability with softref to this scheme has
> consequences on performance.
>
> Imagine:
>
> A---B---C---D---E
>
> The sender's tip is at E and the receiver claims to have C. In
> the sender's repository, E is associated with A (somebody
> noticed that E fixes regression introduced by A, and added a
> softlink to make A reachable from E). Currently we only need to
> know C is reachable from E to decide that we do not have to send
> A again, but with softlink we would need to.
Hmm. First of all, I'm not sure it would be useful to add a _direct_ link
between E and A, but even so...
I'm thinking we can do the regular/current reachability calculation first,
and after it's done, we analyze the softrefs to see if there are more
objects to be fetched. In that scenario, we wouldn't need to send A again,
since it's already in our repo.
> The ancestry chain of referent and referer do not have to share
> any common commits. Imagine a bug tracking system where each
> bug's tracking sheet is represented as a DAG of commits (this
> will allow you to merge and split bugs easily). This history
> would not share any tree nor blob with the history of the source
> code of the project. And you would make a commit in the main
> project associated with objects in the bug tracking project
> using softrefs. As sender and receiver exchanges the commit
> ancestry information on the main project, both ends may need to
> negotiate which objects in the bug tracking history are already
> present in the reciever.
As above, if the receving side gets the list of involved softrefs, it can
make the decision on which objects it needs to fetch.
Hmm. Thinking about it, this process would of course need to be recursive,
which could potentially adversely affect the runtime of fetch...
> One attractive point of softref is that you do not have to
> anchor referents with explicit refs. E.g. if a commit in the
> main project is associated with bug tracking entries in the "bug
> tracking" project via softrefs, that is enough to keep the bug
> tracking objects alive. But I suspect this property makes the
> enumeration of "what do we have on this end" costly. I dunno.
Yeah, there are still may open questions. But I'm glad to see that most
people seem to find the basic concept useful, at least.
Thanks for taking the time and effort to comment.
Have fun!
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ 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