* Re: [RFC/PATCH] Configurable hyperlinking in gitk
From: Jakub Narebski @ 2011-09-18 18:50 UTC (permalink / raw)
To: Chris Packham; +Cc: Jeff Epler, git
In-Reply-To: <4E7467B7.1090201@gmail.com>
Chris Packham <judge.packham@gmail.com> writes:
> On 17/09/11 14:29, Jeff Epler wrote:
> > Some time ago I hardcoded this into gitk for $DAY_JOB and find it very
> > useful. I made it configurable in the hopes that it might be adopted
> > upstream. (unfortunately, the configurable version is radically
> > different from the original hard-coded version, so I can't say this
> > has had much testing yet)
>
> This is definitely something folks at my $dayjob would be interested in.
> We've already done some customisation of gitweb to do something similar.
> I'm not actually sure what the changes where or how configurable they
> are. I'll see if I can dig them out on Monday someone else might want to
> polish them into something suitable (I might do it myself if I get some
> tuits).
That would be nice. So called "committags" support was long planned
for gitweb, and even some preliminary work exists...
> > There are probably better names for the configuration options, too.
>
> It'd be nice if the config variables weren't gitk specific. .re and .sub
> could be applied to gitweb and maybe other git viewers outside of
> gig.git might decide to use them. My bikeshedding suggestion would be to
> just drop the gitk prefix and have linkify.re and linkify.sub.
Perhaps more descriptive name, i.e.
linkify.<name>.regexp
linkify.<name>.subst
would be better?
I guess that regexp is an extended regular expression, isn't it?
--
Jakub Narębski
^ permalink raw reply
* [PATCH 0/3] fast-import: fix pack_id corner cases
From: Dmitry Ivankov @ 2011-09-18 19:01 UTC (permalink / raw)
To: git; +Cc: Jonathan Nieder, Shawn O. Pearce, David Barr, Dmitry Ivankov
[1/3] is just a precaution unlikely to happen as having 65536+ packs
produced in fast-import looks extremely rare.
[2/2] is more real bug. Shouldn't be too hard to reproduce, but I'm
currently too lazy to as it is quite rare and not likely to get broken
again.
[3/3] is pure cosmetic change while I'm on pack_id topic.
Dmitry Ivankov (3):
fast-import: die if we produce too many (MAX_PACK_ID) packs
fast-import: fix corner case for checkpoint
fast-import: rename object_count to pack_object_count
fast-import.c | 29 +++++++++++++++--------------
1 files changed, 15 insertions(+), 14 deletions(-)
--
1.7.3.4
^ permalink raw reply
* [PATCH 1/3] fast-import: die if we produce too many (MAX_PACK_ID) packs
From: Dmitry Ivankov @ 2011-09-18 19:01 UTC (permalink / raw)
To: git; +Cc: Jonathan Nieder, Shawn O. Pearce, David Barr, Dmitry Ivankov
In-Reply-To: <1316372508-7173-1-git-send-email-divanorama@gmail.com>
In fast-import pack_id is 16-bit with MAX_PACK_ID reserved to identify
pre-existing objects. It is unlikely to wrap under reasonable settings
but still things in fast-import will break once it happens.
Add a check and immediate die() as the simplest reaction to being unable
to continue the import.
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
fast-import.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 742e7da..907cb05 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1009,6 +1009,8 @@ static void end_packfile(void)
static void cycle_packfile(void)
{
end_packfile();
+ if (pack_id >= MAX_PACK_ID)
+ die("too many (%u) packs produced", pack_id);
start_packfile();
}
--
1.7.3.4
^ permalink raw reply related
* [PATCH 3/3] fast-import: rename object_count to pack_object_count
From: Dmitry Ivankov @ 2011-09-18 19:01 UTC (permalink / raw)
To: git; +Cc: Jonathan Nieder, Shawn O. Pearce, David Barr, Dmitry Ivankov
In-Reply-To: <1316372508-7173-1-git-send-email-divanorama@gmail.com>
object_count is used to count objects that'll go to the current pack.
While object_count_by_* are used to count total amount of objects and
are not used to determine if current packfile is empty.
Rename (and move declaration) object_count to pack_object_count to
avoid possible confusion.
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
fast-import.c | 20 ++++++++++----------
1 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 014a807..8f2411b 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -290,7 +290,6 @@ static uintmax_t object_count_by_type[1 << TYPE_BITS];
static uintmax_t duplicate_count_by_type[1 << TYPE_BITS];
static uintmax_t delta_count_by_type[1 << TYPE_BITS];
static uintmax_t delta_count_attempts_by_type[1 << TYPE_BITS];
-static unsigned long object_count;
static unsigned long branch_count;
static unsigned long branch_load_count;
static int failure;
@@ -316,6 +315,7 @@ static struct sha1file *pack_file;
static struct packed_git *pack_data;
static struct packed_git **all_packs;
static off_t pack_size;
+static unsigned long pack_object_count;
/* Table of objects we've written. */
static unsigned int object_entry_alloc = 5000;
@@ -880,7 +880,7 @@ static void start_packfile(void)
pack_data = p;
pack_size = sizeof(hdr);
- object_count = 0;
+ pack_object_count = 0;
all_packs = xrealloc(all_packs, sizeof(*all_packs) * (pack_id + 1));
all_packs[pack_id] = p;
@@ -894,17 +894,17 @@ static const char *create_index(void)
struct object_entry_pool *o;
/* Build the table of object IDs. */
- idx = xmalloc(object_count * sizeof(*idx));
+ idx = xmalloc(pack_object_count * sizeof(*idx));
c = idx;
for (o = blocks; o; o = o->next_pool)
for (e = o->next_free; e-- != o->entries;)
if (pack_id == e->pack_id)
*c++ = &e->idx;
- last = idx + object_count;
+ last = idx + pack_object_count;
if (c != last)
die("internal consistency error creating the index");
- tmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts, pack_data->sha1);
+ tmpfile = write_idx_file(NULL, idx, pack_object_count, &pack_idx_opts, pack_data->sha1);
free(idx);
return tmpfile;
}
@@ -953,7 +953,7 @@ static void end_packfile(void)
struct packed_git *old_p = pack_data, *new_p;
clear_delta_base_cache();
- if (object_count) {
+ if (pack_object_count) {
unsigned char cur_pack_sha1[20];
char *idx_name;
int i;
@@ -963,7 +963,7 @@ static void end_packfile(void)
close_pack_windows(pack_data);
sha1close(pack_file, cur_pack_sha1, 0);
fixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1,
- pack_data->pack_name, object_count,
+ pack_data->pack_name, pack_object_count,
cur_pack_sha1, pack_size);
close(pack_data->pack_fd);
idx_name = keep_pack(create_index());
@@ -1103,7 +1103,7 @@ static int store_object(
e->type = type;
e->pack_id = pack_id;
e->idx.offset = pack_size;
- object_count++;
+ pack_object_count++;
object_count_by_type[type]++;
crc32_begin(pack_file);
@@ -1267,7 +1267,7 @@ static void stream_blob(uintmax_t len, unsigned char *sha1out, uintmax_t mark)
e->pack_id = pack_id;
e->idx.offset = offset;
e->idx.crc32 = crc32_end(pack_file);
- object_count++;
+ pack_object_count++;
object_count_by_type[OBJ_BLOB]++;
}
@@ -3025,7 +3025,7 @@ static void parse_ls(struct branch *b)
static void checkpoint(void)
{
checkpoint_requested = 0;
- if (object_count)
+ if (pack_object_count)
cycle_packfile();
dump_branches();
dump_tags();
--
1.7.3.4
^ permalink raw reply related
* [PATCH 2/3] fast-import: fix corner case for checkpoint
From: Dmitry Ivankov @ 2011-09-18 19:01 UTC (permalink / raw)
To: git; +Cc: Jonathan Nieder, Shawn O. Pearce, David Barr, Dmitry Ivankov
In-Reply-To: <1316372508-7173-1-git-send-email-divanorama@gmail.com>
checkpoint command makes fast-import finish current pack and write out
branches/tags and marks. In case no new objects are added in current
pack fast-import falls back to no-op. While it is possible that refs
or marks need to be updated (to point to old objects).
Make fast-import always dump them on checkpoint. But as before do not
cycle_packfile if there are no objects to write.
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
fast-import.c | 9 ++++-----
1 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 907cb05..014a807 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3025,12 +3025,11 @@ static void parse_ls(struct branch *b)
static void checkpoint(void)
{
checkpoint_requested = 0;
- if (object_count) {
+ if (object_count)
cycle_packfile();
- dump_branches();
- dump_tags();
- dump_marks();
- }
+ dump_branches();
+ dump_tags();
+ dump_marks();
}
static void parse_checkpoint(void)
--
1.7.3.4
^ permalink raw reply related
* Re: [PATCH 1/3] fast-import: die if we produce too many (MAX_PACK_ID) packs
From: Jonathan Nieder @ 2011-09-18 19:17 UTC (permalink / raw)
To: Dmitry Ivankov; +Cc: git, Shawn O. Pearce, David Barr
In-Reply-To: <1316372508-7173-2-git-send-email-divanorama@gmail.com>
Dmitry Ivankov wrote:
> In fast-import pack_id is 16-bit with MAX_PACK_ID reserved to identify
> pre-existing objects. It is unlikely to wrap under reasonable settings
> but still things in fast-import will break once it happens.
>
> Add a check and immediate die() as the simplest reaction to being unable
> to continue the import.
Makes a lot of sense. A few possible minor clarity improvements:
- missing commas after "In fast-import" and before "with MAX_PACK_ID
reserved"
- "pre-existing objects": it would be clearer to say something like
"objects this fast-import process instance did not write out to a
packfile", like the comment before gfi_unpack_entry() does
- I suppose "under reasonable settings" means "with a reasonable
max-pack-size setting"?
- "things will break" is a bit vague.
- "immediate" -> "immediately"
Maybe:
In fast-import, pack_id is a 16-bit unsigned integer, with MAX_PACK_ID
(2^16 - 1) reserved for use by objects that are not in a packfile that
this fast-import process instance wrote. It is unusual for pack_id to
hit MAX_PACK_ID with a reasonable --max-pack-size setting, but when it
does, the pack_id stored in each "struct object_entry" wraps and
fast-import gets utterly confused.
Add a check and immediately die() so the operator can at least see what
went wrong instead of experiencing an unexplained broken import.
With or without that clarification,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks! A test would still be nice, if someone has time to write one.
^ permalink raw reply
* Re: Maint-only commits
From: Enrico Weigelt @ 2011-09-18 19:11 UTC (permalink / raw)
To: git
In-Reply-To: <6416457.30612.1305580526325.JavaMail.root@mail.hq.genarts.com>
* Stephen Bash <bash@genarts.com> wrote:
> Hi all-
>
> In my office we've recently run into three separate fixes
> required on our maintenance branch that should not be
> included in master (our normal workflow is to make changes
> on maint, tag, release, and then merge to master). Normally
> these "maint only" fixes are interspersed with commits that
> should go back into master. In the past the "maint only"
> commits were rare, so I'd carefully use "merge -s ours"
> to avoid including the "maint only" changes in master.
> But now I'm wondering if there's a better process/workflow?
Of course, there is: use topic branches and rebase.
Assuming you've found a bug in maint, which is also still
in master.
#1: for off a topic branch (for that bug) from maint
#2: fix the bug there
#3: rebase to latest maint (if changed meanwhile) and test carefully
#4: (ff-)merge your bugfix branch to maint
#5: rebase bugfix branch to master (maybe incremental, if they
went too far away from another) and test carefully
#6: (ff-)merge bugfix branch to master
#7: drop that topic branch, as you're done now.
cu
--
----------------------------------------------------------------------
Enrico Weigelt, metux IT service -- http://www.metux.de/
phone: +49 36207 519931 email: weigelt@metux.de
mobile: +49 151 27565287 icq: 210169427 skype: nekrad666
----------------------------------------------------------------------
Embedded-Linux / Portierung / Opensource-QM / Verteilte Systeme
----------------------------------------------------------------------
^ permalink raw reply
* Re: [PATCH 2/3] fast-import: fix corner case for checkpoint
From: Jonathan Nieder @ 2011-09-18 19:28 UTC (permalink / raw)
To: Dmitry Ivankov; +Cc: git, Shawn O. Pearce, David Barr
In-Reply-To: <1316372508-7173-3-git-send-email-divanorama@gmail.com>
Dmitry Ivankov wrote:
> checkpoint command makes fast-import finish current pack and write out
> branches/tags and marks. In case no new objects are added in current
> pack fast-import falls back to no-op. While it is possible that refs
> or marks need to be updated (to point to old objects).
>
> Make fast-import always dump them on checkpoint. But as before do not
> cycle_packfile if there are no objects to write.
Yeah, that would be annoying to run into. Rearranging the description
a little for clarity and brevity:
fast-import: update refs on checkpoint even if there are no new objects
During an import using the fast-import command, it is possible for
no new objects to have been added between two checkpoints requested
with the SIGUSR1 signal or the "checkpoint" command. Even in this
case, fast-import should write out any updated refs and marks to
fulfill the second checkpoint request.
As before, fast-import will not write an empty pack and start a new
one when there are no new objects to write out.
With that change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH 3/3] fast-import: rename object_count to pack_object_count
From: Jonathan Nieder @ 2011-09-18 19:32 UTC (permalink / raw)
To: Dmitry Ivankov; +Cc: git, Shawn O. Pearce, David Barr
In-Reply-To: <1316372508-7173-4-git-send-email-divanorama@gmail.com>
Dmitry Ivankov wrote:
> object_count is used to count objects that'll go to the current pack.
> While object_count_by_* are used to count total amount of objects and
> are not used to determine if current packfile is empty.
>
> Rename (and move declaration) object_count to pack_object_count to
> avoid possible confusion.
No strong opinion on this one. I guess the important thing is that
you are moving the declaration to the group of declarations labelled as
/* The .pack file being generated */
. Is it important to rename the variable while at it (which will
disrupt other patches in flight using that variable if they exist)?
^ permalink raw reply
* Re: Recovering from a bad object
From: Enrico Weigelt @ 2011-09-18 19:31 UTC (permalink / raw)
To: Git Mailing List
In-Reply-To: <7vy61ywjpa.fsf@alter.siamese.dyndns.org>
* Junio C Hamano <gitster@pobox.com> wrote:
> > The object is likely at kernel.org.
> > Can git go fetch it somehow?
>
> The protocol is designed specifically to disallow "I guessed that the
> trade secret is contained within an object whose object name is this,
> please feed me that object" kind of requests, so in general no.
Maybe the local side (in some special repair mode) could check if
some of the remote refs (and their referenced graphs) are broken
and refetch them completely. If the remote side(s) got the objects
you're missing (visibile to you), you'll be done after that.
For example:
A local branch "foo" is based on origin/master, somewhere in the
line behind origin/master some objects are broken:
It would find out that origin/master points to broken/missing objects
and refetch it completely afresh (same as it would if they had never
been fetched yet).
Perhaps it's not that hard to implement: just requires a special
git-fetch operation mode that ignores locally existing objects,
and a few lines of shell code that simply fetches the whole remote
to some temporary namespaces (and drop that when done). Ends up in
the same traffic as a fresh clone, but at least runs automatically.
cu
--
----------------------------------------------------------------------
Enrico Weigelt, metux IT service -- http://www.metux.de/
phone: +49 36207 519931 email: weigelt@metux.de
mobile: +49 151 27565287 icq: 210169427 skype: nekrad666
----------------------------------------------------------------------
Embedded-Linux / Portierung / Opensource-QM / Verteilte Systeme
----------------------------------------------------------------------
^ permalink raw reply
* Re: Recovering from a bad object
From: jonsmirl @ 2011-09-18 19:48 UTC (permalink / raw)
To: weigelt, Git Mailing List
In-Reply-To: <20110918193116.GB6334@nibiru.local>
On Sun, Sep 18, 2011 at 3:31 PM, Enrico Weigelt <weigelt@metux.de> wrote:
> * Junio C Hamano <gitster@pobox.com> wrote:
>
>> > The object is likely at kernel.org.
>> > Can git go fetch it somehow?
>>
>> The protocol is designed specifically to disallow "I guessed that the
>> trade secret is contained within an object whose object name is this,
>> please feed me that object" kind of requests, so in general no.
>
> Maybe the local side (in some special repair mode) could check if
> some of the remote refs (and their referenced graphs) are broken
> and refetch them completely. If the remote side(s) got the objects
> you're missing (visibile to you), you'll be done after that.
>
> For example:
>
> A local branch "foo" is based on origin/master, somewhere in the
> line behind origin/master some objects are broken:
>
> It would find out that origin/master points to broken/missing objects
> and refetch it completely afresh (same as it would if they had never
> been fetched yet).
>
That's describes the lines I was thinking along.
>
> Perhaps it's not that hard to implement: just requires a special
> git-fetch operation mode that ignores locally existing objects,
> and a few lines of shell code that simply fetches the whole remote
> to some temporary namespaces (and drop that when done). Ends up in
> the same traffic as a fresh clone, but at least runs automatically.
>
>
> cu
> --
> ----------------------------------------------------------------------
> Enrico Weigelt, metux IT service -- http://www.metux.de/
>
> phone: +49 36207 519931 email: weigelt@metux.de
> mobile: +49 151 27565287 icq: 210169427 skype: nekrad666
> ----------------------------------------------------------------------
> Embedded-Linux / Portierung / Opensource-QM / Verteilte Systeme
> ----------------------------------------------------------------------
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [PATCH 3/3] fast-import: rename object_count to pack_object_count
From: Dmitry Ivankov @ 2011-09-18 19:51 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Shawn O. Pearce, David Barr
In-Reply-To: <20110918193205.GF2308@elie>
On Mon, Sep 19, 2011 at 1:32 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Dmitry Ivankov wrote:
>
>> object_count is used to count objects that'll go to the current pack.
>> While object_count_by_* are used to count total amount of objects and
>> are not used to determine if current packfile is empty.
>>
>> Rename (and move declaration) object_count to pack_object_count to
>> avoid possible confusion.
>
> No strong opinion on this one. I guess the important thing is that
> you are moving the declaration to the group of declarations labelled as
>
> /* The .pack file being generated */
>
> . Is it important to rename the variable while at it (which will
> disrupt other patches in flight using that variable if they exist)?
Not that important. Maybe a huge comment will do more and better.
object_count++ still appears near object_count_by_type[type]++, but
hopefully one will look for their declarations and thus avoid the confusion.
--- a/fast-import.c
+++ b/fast-import.c
@@ -290,7 +290,6 @@ static uintmax_t object_count_by_type[1 << TYPE_BITS];
static uintmax_t duplicate_count_by_type[1 << TYPE_BITS];
static uintmax_t delta_count_by_type[1 << TYPE_BITS];
static uintmax_t delta_count_attempts_by_type[1 << TYPE_BITS];
-static unsigned long object_count;
static unsigned long branch_count;
static unsigned long branch_load_count;
static int failure;
@@ -310,8 +309,16 @@ static unsigned int atom_cnt;
static struct atom_str **atom_table;
/* The .pack file being generated */
+/*
+ * objects that are being written to the current pack
+ * all *must* have current pack_id in struct object_entry.
+ * And object_count *must* be a count of object_entry's
+ * having current pack_id. This data is used to create
+ * index file once current pack_file is finished.
+ */
static struct pack_idx_option pack_idx_opts;
static unsigned int pack_id;
+static unsigned long object_count;
static struct sha1file *pack_file;
static struct packed_git *pack_data;
static struct packed_git **all_packs;
^ permalink raw reply
* Re: Pull From Mirrored Repository
From: Jean Sacren @ 2011-09-18 20:06 UTC (permalink / raw)
To: git
In-Reply-To: <CAH5451m1WJ0H9G8uHbCqZ5acO-xEynGKjMk=6g75buJy1S7iSw@mail.gmail.com>
From: Andrew Ardill <andrew.ardill@gmail.com>
Date: Mon, 19 Sep 2011 01:15:47 +1000
>
> Am I correct in thinking that you have done the following:
>
> $ git clone original-server/repo.git
> $ cd repo
> $ git pull mirror-server/repo.git
Essentially that's what I did.
> and now you want the remote ref 'origin/master' to point to the same
> thing as 'mirror/master' (assuming that you create the ref for the
> mirror)?
The idea is to update the local tree from multiple sites so long as one
of them is still running. And the update shall make no difference
whether it is from the master site or not.
> This can be done (by editing the file .git/refs/remotes/origin/master
> and specifying the correct sha1) however this is probably not the best
> idea. It is best for git to know the actual state of the remote the
> last time you synced with it so that it can act intelligently the next
> time you sync with it.
It turns out to be more of a bug with git. After a successful pull from
a master or mirror site, the SHA1 shall be updated among all the master
files in .git/refs/remotes/origin or .git/refs/remotes/[WHATEVER_MIRROR]
as the tip of the local tree is updated.
> If you intend to sync with the mirror, you might as well set it as the
> upstream ref of you master branch. That way you will not get the 'Your
> branch is ahead' message when you pull new updates (unless you have
> local changes, of course).
> This can be done by:
> $ git checkout master
> $ git branch --set-upstream mirror
This is a nice hack, but we want all the changes stay with the master
branch. It's easier to use one master branch for tracking all updates
from the upstream.
Thank you for all your help!
--
Jean Sacren
^ permalink raw reply
* [RFC] fast-import: note deletion command
From: Dmitry Ivankov @ 2011-09-18 20:07 UTC (permalink / raw)
To: Git List, Jonathan Nieder, David Barr, Shawn O. Pearce
Hi,
fast-import is able to write notes provided a command
N <note_content_data_specification> <commit_specification>
There is no documented command to delete a note. It can't be
easily replaced with Delete command on notes tree because
of notes fanout.
But fast-import interprets 0{40} data_specification as a note
deletion command. It is consistent with fast-import internals
where null_sha1 is used for absent/unknown object. But is
not consistent with file Modify command nor looks like a common
convention in git ui.
So the questions are:
- should 0{40} not be treated as deletion toggle? Downside is that
it is used as one in t/t9301-fast-import-notes.sh.
- how should a documented way to delete notes look like?
ND commit_sha1 # One may think there are also NC, NR, NM
N delete commit_sha1
# Only :mark, full-40-byte-sha1 and 'inline' are allowed currently.
# So no clashes arise, but then one may also want M 'delete' path
# command to work too.
^ permalink raw reply
* Re: [RFC] fast-import: note deletion command
From: Jonathan Nieder @ 2011-09-18 20:35 UTC (permalink / raw)
To: Dmitry Ivankov
Cc: Git List, David Barr, Shawn O. Pearce, Thomas Rast, Johan Herland,
Sverre Rabbelier
In-Reply-To: <CA+gfSn9sdTzQghqQp6hcO-9kN9mPx2JLRig79Rgx2FqGWXXp=A@mail.gmail.com>
(+cc: Sverre for fast-import, Johan for notes, Thomas as a user of
the note import feature, snipping less for their benefit)
Dmitry Ivankov wrote:
> Hi,
>
> fast-import is able to write notes provided a command
> N <note_content_data_specification> <commit_specification>
>
> There is no documented command to delete a note. It can't be
> easily replaced with Delete command on notes tree because
> of notes fanout.
>
> But fast-import interprets 0{40} data_specification as a note
> deletion command. It is consistent with fast-import internals
> where null_sha1 is used for absent/unknown object. But is
> not consistent with file Modify command nor looks like a common
> convention in git ui.
Right.
> So the questions are:
> - should 0{40} not be treated as deletion toggle? Downside is that
> it is used as one in t/t9301-fast-import-notes.sh.
> - how should a documented way to delete notes look like?
> ND commit_sha1 # One may think there are also NC, NR, NM
> N delete commit_sha1
> # Only :mark, full-40-byte-sha1 and 'inline' are allowed currently.
> # So no clashes arise, but then one may also want M 'delete' path
> # command to work too.
If I were doing it:
- advertise "N 0{40} <commit>" as the historical way to delete a
note, and make sure it keeps working
- introduce 'ND' or 'notedelete <commit>' as a human-friendly
synonym, keeping in mind that NC and NR could be introduced in the
future if there is demand for them.
- don't add any new magic "N <magic keyword> <commit>" commands,
since it's hard to know when they will clash with some VCS's
convention for object names.
I imagine there are plenty of other acceptable ways to accomplish
these things, though. :) Thanks for looking into it.
^ permalink raw reply
* Re: [RFC] fast-import: note deletion command
From: Sverre Rabbelier @ 2011-09-18 20:39 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Dmitry Ivankov, Git List, David Barr, Shawn O. Pearce,
Thomas Rast, Johan Herland
In-Reply-To: <20110918203506.GG2308@elie>
Heya,
On Sun, Sep 18, 2011 at 22:35, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> ND commit_sha1 # One may think there are also NC, NR, NM
>> N delete commit_sha1
>> # Only :mark, full-40-byte-sha1 and 'inline' are allowed currently.
>> # So no clashes arise, but then one may also want M 'delete' path
>> # command to work too.
>
> If I were doing it:
>
> - advertise "N 0{40} <commit>" as the historical way to delete a
> note, and make sure it keeps working
>
> - introduce 'ND' or 'notedelete <commit>' as a human-friendly
> synonym, keeping in mind that NC and NR could be introduced in the
> future if there is demand for them.
Is this perhaps a good moment to also think about branch deletion?
That came up earlier as well, and thinking about that might give us
some insights in how to deal with deletions uniformly.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [RFC] fast-import: note deletion command
From: Dmitry Ivankov @ 2011-09-18 20:59 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Jonathan Nieder, Git List, David Barr, Shawn O. Pearce,
Thomas Rast, Johan Herland
In-Reply-To: <CAGdFq_hpA95Kj4eMr4e1duuXTpr+OkkwF4K5bTapXEi9UjWcSA@mail.gmail.com>
On Mon, Sep 19, 2011 at 2:39 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Heya,
>
>>
>> If I were doing it:
>>
>> - advertise "N 0{40} <commit>" as the historical way to delete a
>> note, and make sure it keeps working
>>
>> - introduce 'ND' or 'notedelete <commit>' as a human-friendly
>> synonym, keeping in mind that NC and NR could be introduced in the
>> future if there is demand for them.
>
> Is this perhaps a good moment to also think about branch deletion?
> That came up earlier as well, and thinking about that might give us
> some insights in how to deal with deletions uniformly.
Also maybe marks deletion, getting mark sha1, resetting a mark with
explicit sha1. But most probably not tags deletion.
And going much further on commands, following look nice to have:
- 'ls' storing result to a mark (to allow us not to compute sha1/store
object if we don't want to)
- marks namespaces (to keep ls mark separately, mark deletion will
do too, if it's only a single temporary mark). Maybe like ::nr:mark.
>
> --
> Cheers,
>
> Sverre Rabbelier
>
^ permalink raw reply
* fast-import wishlist (Re: [RFC] fast-import: note deletion command)
From: Jonathan Nieder @ 2011-09-18 21:03 UTC (permalink / raw)
To: Dmitry Ivankov
Cc: Sverre Rabbelier, Git List, David Barr, Shawn O. Pearce,
Thomas Rast, Johan Herland
In-Reply-To: <CA+gfSn9CsL4tz30B62mDzALdyy1RTFiRT4a1zdJ8pR8aTdcpXA@mail.gmail.com>
Dmitry Ivankov wrote:
> On Mon, Sep 19, 2011 at 2:39 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
>> Is this perhaps a good moment to also think about branch deletion?
It might be a good temporal moment, but it's not quite the right
thread for it. :) If someone wants to maintain a fast-import wishlist
in a wiki somewhere, it sounds like it could be useful to some people
(I prefer to read patches and stories about particular use cases,
myself :)).
>> That came up earlier as well, and thinking about that might give us
>> some insights in how to deal with deletions uniformly.
>
> Also maybe marks deletion, getting mark sha1, resetting a mark with
> explicit sha1. But most probably not tags deletion.
>
> And going much further on commands, following look nice to have:
> - 'ls' storing result to a mark (to allow us not to compute sha1/store
> object if we don't want to)
> - marks namespaces (to keep ls mark separately, mark deletion will
> do too, if it's only a single temporary mark). Maybe like ::nr:mark.
Regards,
Jonathan
^ permalink raw reply
* [PATCH/RFC 0/2] fast-import: commit from null_sha1
From: Dmitry Ivankov @ 2011-09-18 21:20 UTC (permalink / raw)
To: git
Cc: Jonathan Nieder, Shawn O. Pearce, David Barr, Sverre Rabbelier,
Dmitry Ivankov
Not so sure how null_sha1 parent should be treated in fast-import.
Absent parent is represented as null_sha1 to the user in reflog,
but isn't allowed as an argument for porcelain nor shows in most
plumbing commands afaik.
These patches make fast-import treat
commit refs/heads/master
...
from `null_sha1`
like any other missing parent sha1 - reject such input.
Note: if we'll want this input to be valid, some other adjustments
to fast-import logic may be needed for consistency.
Dmitry Ivankov (2):
fast-import: add 'commit from 0{40}' failing test
fast-import: fix 'from 0{40}' test
fast-import.c | 17 ++++++-----------
t/t9300-fast-import.sh | 12 ++++++++++++
2 files changed, 18 insertions(+), 11 deletions(-)
--
1.7.3.4
^ permalink raw reply
* [PATCH 1/2] fast-import: add 'commit from 0{40}' failing test
From: Dmitry Ivankov @ 2011-09-18 21:20 UTC (permalink / raw)
To: git
Cc: Jonathan Nieder, Shawn O. Pearce, David Barr, Sverre Rabbelier,
Dmitry Ivankov
In-Reply-To: <1316380846-15845-1-git-send-email-divanorama@gmail.com>
Following shouldn't be allowed, while it is:
commit refs/heads/some
committer ...
data ...
from `null_sha1`
It is treated as if 'from' was omitted. But it is allowed to just
omit 'from' actually. And `null_sha1` being special in fast-import
is an internal implementation detail.
Add a test as described.
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
t/t9300-fast-import.sh | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 1a6c066..8cc3f16 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -375,6 +375,18 @@ test_expect_success 'B: fail on invalid branch name "bad[branch]name"' '
rm -f .git/objects/pack_* .git/objects/index_*
cat >input <<INPUT_END
+commit refs/heads/zeromaster
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data 0
+
+from 0000000000000000000000000000000000000000
+INPUT_END
+test_expect_failure 'B: fail on "from 0{40}"' '
+ test_must_fail git fast-import <input
+'
+rm -f .git/objects/pack_* .git/objects/index_*
+
+cat >input <<INPUT_END
commit TEMP_TAG
committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
data <<COMMIT
--
1.7.3.4
^ permalink raw reply related
* [PATCH 2/2] fast-import: fix 'from 0{40}' test
From: Dmitry Ivankov @ 2011-09-18 21:20 UTC (permalink / raw)
To: git
Cc: Jonathan Nieder, Shawn O. Pearce, David Barr, Sverre Rabbelier,
Dmitry Ivankov
In-Reply-To: <1316380846-15845-1-git-send-email-divanorama@gmail.com>
parse_from_existing() has a special case for null_sha1 treating it
as a start of an orphaned branch. It is how null_sha1 parent is
treated in fast-import. For example parse_reset_branch() clears
sha1 of a branch but leaves it in a lookup table.
Looking at parse_from_existing() call sites, we can seen that it is
only called for sha1's that come from get_sha1() or an existing
object. So fast-import internals don't give it null_sha1 explicitly
and the only way for it to appear is direct '0{40}' in the input.
Don't treat null_sha1 as a magic sha1 in parse_from_existing thus
making 'from 0{40}' an invalid input. (Unless there is a commit
object having null_sha1, of course. And object with null_sha1 would
be a lot of trouble in fast-import regardless of this patch.)
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
fast-import.c | 17 ++++++-----------
t/t9300-fast-import.sh | 2 +-
2 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 742e7da..827434a 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2488,18 +2488,13 @@ static void parse_from_commit(struct branch *b, char *buf, unsigned long size)
static void parse_from_existing(struct branch *b)
{
- if (is_null_sha1(b->sha1)) {
- hashclr(b->branch_tree.versions[0].sha1);
- hashclr(b->branch_tree.versions[1].sha1);
- } else {
- unsigned long size;
- char *buf;
+ unsigned long size;
+ char *buf;
- buf = read_object_with_reference(b->sha1,
- commit_type, &size, b->sha1);
- parse_from_commit(b, buf, size);
- free(buf);
- }
+ buf = read_object_with_reference(b->sha1,
+ commit_type, &size, b->sha1);
+ parse_from_commit(b, buf, size);
+ free(buf);
}
static int parse_from(struct branch *b)
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 8cc3f16..0784d50 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -381,7 +381,7 @@ data 0
from 0000000000000000000000000000000000000000
INPUT_END
-test_expect_failure 'B: fail on "from 0{40}"' '
+test_expect_success 'B: fail on "from 0{40}"' '
test_must_fail git fast-import <input
'
rm -f .git/objects/pack_* .git/objects/index_*
--
1.7.3.4
^ permalink raw reply related
* Branch deletion (Re: [RFC] fast-import: note deletion command)
From: Jonathan Nieder @ 2011-09-18 21:18 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Dmitry Ivankov, Git List, David Barr, Shawn O. Pearce,
Thomas Rast, Johan Herland
In-Reply-To: <CAGdFq_hpA95Kj4eMr4e1duuXTpr+OkkwF4K5bTapXEi9UjWcSA@mail.gmail.com>
Sverre Rabbelier wrote:
> Is this perhaps a good moment to also think about branch deletion?
> That came up earlier as well, and thinking about that might give us
> some insights in how to deal with deletions uniformly.
Sorry, my earlier reply missed the point. To answer your question:
Dmitry's RFC was about deleting individual notes rather than entire
notes refs, so it is not too closely related to branch deletion.
A new "deleteref" command would be a very simple addition, and the UI
seems pretty obvious. The main concern in adding such a thing is that
it destroys history, while currently "git fast-import" without -f does
not provide any commands that can do that. So presumably one would
want to do one of the following:
- only allow deletion of refs that were not present at the start of
the import, or
- add a command-line option to permit deletion of refs, so it doesn't
happen by mistake when misparsing a stream.
It would also be interesting to see an example use (do you remember
the earlier thread where it came up?) to make sure the UI fits it
well. In most typical cases I can imagine, direct use of "git
update-ref -d" would work better. In the case of remote helpers, IIRC
there was already a need for the transport-helper to handle the final
ref updates so "git fetch" can write a nice notice about them to the
console.
^ permalink raw reply
* Re: [PATCH/RFC 0/2] fast-import: commit from null_sha1
From: Jonathan Nieder @ 2011-09-18 21:30 UTC (permalink / raw)
To: Dmitry Ivankov; +Cc: git, Shawn O. Pearce, David Barr, Sverre Rabbelier
In-Reply-To: <1316380846-15845-1-git-send-email-divanorama@gmail.com>
Dmitry Ivankov wrote:
> These patches make fast-import treat
> commit refs/heads/master
> ...
> from `null_sha1`
> like any other missing parent sha1 - reject such input.
Are you sure the existing support for "from 0{40}" is not deliberate
and that no one is relying on it? If and only if you are, then this
seems like a good idea (a single patch that both makes the behavior
change and adds a test for it should be easier to review).
^ permalink raw reply
* Re: Branch deletion (Re: [RFC] fast-import: note deletion command)
From: Sverre Rabbelier @ 2011-09-18 21:38 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Dmitry Ivankov, Git List, David Barr, Shawn O. Pearce,
Thomas Rast, Johan Herland
In-Reply-To: <20110918211836.GI2308@elie>
Heya,
On Sun, Sep 18, 2011 at 23:18, Jonathan Nieder <jrnieder@gmail.com> wrote:
> In the case of remote helpers, IIRC there was already a need for
> the transport-helper to handle the final ref updates so "git fetch"
> can write a nice notice about them to the console.
That book-keeping is trivial to do. The problem currently is that when
you try to "git push origin :experimental-branch", there is no way for
the transport-helper code to tell the helper to delete the ref.
Something like 'deleteref' sounds sane I suppose, and I agree that
there should be some sort of safety switch :). I think we need a new
feature announcing that we require support for it, and then the
importer can abort right away if the user doesn't want to have their
refs deleted.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH 3/3] fast-import: rename object_count to pack_object_count
From: Jonathan Nieder @ 2011-09-18 21:40 UTC (permalink / raw)
To: Dmitry Ivankov; +Cc: git, Shawn O. Pearce, David Barr
In-Reply-To: <CA+gfSn8aOWPm=xmTE9WzuXsQY0EfYypFxRAyVb-x3_kmhNUb-Q@mail.gmail.com>
Dmitry Ivankov wrote:
> --- a/fast-import.c
> +++ b/fast-import.c
[...]
> @@ -310,8 +309,16 @@ static unsigned int atom_cnt;
> static struct atom_str **atom_table;
>
> /* The .pack file being generated */
> +/*
> + * objects that are being written to the current pack
> + * all *must* have current pack_id in struct object_entry.
> + * And object_count *must* be a count of object_entry's
> + * having current pack_id. This data is used to create
> + * index file once current pack_file is finished.
> + */
> static struct pack_idx_option pack_idx_opts;
> static unsigned int pack_id;
> +static unsigned long object_count;
> static struct sha1file *pack_file;
Closer. Now I am tempted to nitpick and say that this should be
a single comment, formatted in complete sentences, and written to
be descriptive rather than normative when possible (since norms
will inevitably change over time, and future readers should not
have an excuse to be afraid to adjust the comment to match code
changes).
/*
* The .pack file being generated
*
* Objects that are being written to the current pack store the
* current value of "pack_id" in struct object_entry.
* "object_count" counts the object_entrys with the current
* pack_id. These values are used to create the pack index
* file when the current pack is finished.
*/
static struct pack_idx_option pack_idx_opts;
static unsigned int pack_id;
...
^ 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