[PATCH] dir: remove dead code
Remove dead code around remove_dir_recursively().
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
dir.c | 21 -
1 file changed, 4 insertions(+), 17 deletions(-)
diff --git a/dir.c b/dir.c
index 910bfcd..2b31241 100644
--- a/dir.c
+++ b/dir.c
@@ -1464,11 +1464,11 @@ int is_empty_dir(const char *path)
return ret;
}
-static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
+int remove_dir_recursively(struct strbuf *path, int flag)
{
DIR *dir;
struct dirent *e;
- int ret = 0, original_len = path-len, len, kept_down = 0;
+ int ret = 0, original_len = path-len, len;
int only_empty = (flag REMOVE_DIR_EMPTY_ONLY);
int keep_toplevel = (flag REMOVE_DIR_KEEP_TOPLEVEL);
unsigned char submodule_head[20];
@@ -1476,8 +1476,6 @@ static int remove_dir_recurse(struct strbuf *path, int
flag, int *kept_up)
if ((flag REMOVE_DIR_KEEP_NESTED_GIT)
!resolve_gitlink_ref(path-buf, HEAD, submodule_head)) {
/* Do not descend and nuke a nested git work tree. */
- if (kept_up)
- *kept_up = 1;
return 0;
}
@@ -1504,7 +1502,7 @@ static int remove_dir_recurse(struct strbuf *path, int
flag, int *kept_up)
if (lstat(path-buf, st))
; /* fall thru */
else if (S_ISDIR(st.st_mode)) {
- if (!remove_dir_recurse(path, flag, kept_down))
+ if (!remove_dir_recursively(path, flag))
continue; /* happy */
} else if (!only_empty !unlink(path-buf))
continue; /* happy, too */
@@ -1516,22 +1514,11 @@ static int remove_dir_recurse(struct strbuf *path, int
flag, int *kept_up)
closedir(dir);
strbuf_setlen(path, original_len);
- if (!ret !keep_toplevel !kept_down)
+ if (!ret !keep_toplevel)
ret = rmdir(path-buf);
- else if (kept_up)
- /*
-* report the uplevel that it is not an error that we
-* did not rmdir() our directory.
-*/
- *kept_up = !ret;
return ret;
}
-int remove_dir_recursively(struct strbuf *path, int flag)
-{
- return remove_dir_recurse(path, flag, NULL);
-}
-
void setup_standard_excludes(struct dir_struct *dir)
{
const char *path;
--
1.8.4.100.gde18f6d.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
On Sun, Sep 8, 2013 at 12:21 AM, Jeff King p...@peff.net wrote: On Sun, Sep 08, 2013 at 12:09:34AM -0500, Felipe Contreras wrote: It's not if you understand the difference between merge-then-commit and commit-then-merge. But for a clueless user who has been told replace svn commit with git commit git push and replace svn update with git pull, it is quite similar. Well, yeah, but if they are so clueless they have to be told what to do, they can be told to do 'git pull --merge' instead, no? I think it's fine to tell them to do git pull --merge. What I'd worry more about is somebody who is suddenly presented with the choice between --rebase and --merge and doesn't know which to choose. We've created a cognitive load on the user, and even more load if they choose --rebase and don't quite understand what it means. If that happens they will go back to the guy that told them to run those commands. Fortunately there probably are very few of these users. The current warning message in jc/pull-training-wheel is quite neutral between the two options. Perhaps we should lean more towards merging? I don't like that message. I would like this for the deprecation period: The pull was not fast-forward, in the future you would have to choose a merge or a rebase, merging automatically for now. Read 'man git pull' for more help. Then when obsolete: The pull was not fast-forward, please either merge or rebase. Any more babysitting with essay long messages is counter-productive to the vast majority of Git users. I guess that works against John's case, though, which is clueless people working on a project that _does_ care about the shape of history. At least they would have to stop and think for a moment, though, which might help (and maybe convince them to ask more clueful project members). I don't know. Or google 'git pull' 'git merge' 'git rebase' or 'git non-fast-forward'. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
On 2013-09-07 22:41, Felipe Contreras wrote: On Wed, Sep 4, 2013 at 5:59 PM, Junio C Hamano gits...@pobox.com wrote: Which can be solved by adding the above fail option, and then renaming them to pull.integrate and branch.name.integrate to clarify what these variables are about (it is no longer do you rebase or not---if you choose not to rebase, by definition you are going to merge, as there is a third choice to fail), while retaining pull.rebase and branch.name.rebase as a deprecated synonym. All these names are completely unintuitive. First of all, why integrate? Integrate what to what? And then, why fail? Fail on what circumstances? Always? My proposal that does: pull.mode = merge/rebase/merge-ff-only Is way more intuitive. +1 What about something like: pull.mergeoptions (defaults to --ff-only) pull.rebaseoptions (defaults to empty? --preserve-merges?) branch.name.pull.mergeoptions (defaults to pull.mergeoptions) branch.name.pull.rebaseoptions (defaults to pull.rebaseoptions) -Richard -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] diffcore-break: fix typo in comment
The parameter is called break_score, not minimum_edit. Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- diffcore-break.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/diffcore-break.c b/diffcore-break.c index 1d9e530..34dd1e0 100644 --- a/diffcore-break.c +++ b/diffcore-break.c @@ -37,7 +37,7 @@ static int should_break(struct diff_filespec *src, * back together. The break operation itself happens * according to the former definition. * -* The minimum_edit parameter tells us when to break (the +* The break_score parameter tells us when to break (the * amount of edit required for us to consider breaking the * pair). We leave the amount of deletion in *merge_score_p * when we return. -- 1.8.4.100.gde18f6d.dirty -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] index-pack: add --unpack-limit to unpack objects
On Sun, Sep 8, 2013 at 11:44 AM, Jeff King p...@peff.net wrote: On Fri, Sep 06, 2013 at 07:46:01AM +0700, Nguyen Thai Ngoc Duy wrote: --- I had something that could unpack without writing to temp pack file but I scraped it and chose this way because it follows closely how index-pack works. It's a good thing imo because .pack v4 is coming and I don't know how v4 may impact this unpack code path. Once things are settled, we can revisit and open a separate code path if it's still a good idea. From a cursory read, this seems fine. If it were done in complete isolation, I'd say it was a slight regression, just because we are doing more I/O for the unpack case, and it is not really saving us any code (it is not like we can throw away unpack-objects, as I think we would want to keep it as a last resort for getting data out of malformed or otherwise non-indexable packs). I can see unpack-objects is more tolerable on corrupt/incomplete packs. If index-pack finds something wrong, it aborts the whole process. I think we can make index-pack stop at the first bad object, adjust nr_objects, and try to recover as much as possible out of the good part. Any other reasons to keep unpack-objects (because I still want to kill it)? But I can also see it making pack v4 handling easier. So it would make sense to me to put it at the start of a series adding pack v4 indexing. By the end of the series you would be able to see the benefits of the reduced code complexity. Until then, it is a probably this will help later change. Agreed. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] index-pack: add --unpack-limit to unpack objects
On Sun, Sep 08, 2013 at 01:28:47PM +0700, Nguyen Thai Ngoc Duy wrote: From a cursory read, this seems fine. If it were done in complete isolation, I'd say it was a slight regression, just because we are doing more I/O for the unpack case, and it is not really saving us any code (it is not like we can throw away unpack-objects, as I think we would want to keep it as a last resort for getting data out of malformed or otherwise non-indexable packs). I can see unpack-objects is more tolerable on corrupt/incomplete packs. If index-pack finds something wrong, it aborts the whole process. I think we can make index-pack stop at the first bad object, adjust nr_objects, and try to recover as much as possible out of the good part. Any other reasons to keep unpack-objects (because I still want to kill it)? No, I don't think there is another reason. The command name may hang around for historical reasons, but we can always make it an alias for index-pack --unpack-limit=0 or whatever. I do not think index-pack even needs to be particularly clever about trying to recover. It is mainly that we may do some extra sanity checks when writing the index (e.g., the recent discussion on avoiding duplicates in the index), that do not even come up when simply exploding the pack in a linear fashion. But I don't think there is any fundamental reason why index-pack could not learn to be as robust when operating in unpack mode. As an aside, you cannot just drop the nr_objects that index-pack's generated index says it contains. Packfile readers double-check that the .idx and .pack agree on the number of objects (I tried it as a method for working around duplicate entries :) ). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
On Sun, Sep 08, 2013 at 01:17:42AM -0500, Felipe Contreras wrote: I think it's fine to tell them to do git pull --merge. What I'd worry more about is somebody who is suddenly presented with the choice between --rebase and --merge and doesn't know which to choose. We've created a cognitive load on the user, and even more load if they choose --rebase and don't quite understand what it means. If that happens they will go back to the guy that told them to run those commands. I think the guy may be git itself. For example, here is a possible session with jc/pull-training-wheel: $ git push To ... ! [rejected]master - master (non-fast-forward) error: failed to push some refs to '...' hint: Updates were rejected because the tip of your current branch is behind hint: its remote counterpart. Integrate the remote changes (e.g. hint: 'git pull ...') before pushing again. hint: See the 'Note about fast-forwards' in 'git push --help' for details. $ git pull The pull does not fast-forward; please specify if you want to merge or rebase. Use either git pull --rebase git pull --merge You can also use 'git config pull.rebase true' (if you want --rebase) or 'git config pull.rebase false' (if you want --merge) to set this once for this project and forget about it. The user is pointed at pull from push, and then gets presented with the merge or rebase choice. It may be that the advice you can find by googling merge vs rebase is enough to then help the person along (and/or we may need to improve the manpages in that respect). I am genuinely curious what people in favor of this feature would want to say in the documentation to a user encountering this choice for the first time. In my experience, rebasing introduces more complications, specifically: 1. the merge is backwards with respect to ours/theirs 2. you may end up with difficult conflict resolution due to repeated changes over the same section of code. E.g., you write some buggy code and then fix it, but upstream has changed the same area. Rebasing involves first resolving your buggy version with the upstream code, and then resolving the fix on top of the previous resolution. 3. rewriting of commits found in other branches, which then need rebased on top of the branch you just rebased 4. a previously bug-free commit can show a bug after the rebase if other parts of the project changed (whereas with a merge, the bug would be attributable to the merge) I know those are all balanced by some advantages of rebasing, but I also think they are things that can be troublesome for a user who does not fully grok the rebase process. I'm just wondering if we should mention both, but steer people towards merging as the safer alternative (you might have ugly history, but you are less likely to create a mess with duplicate commits or badly-resolved conflicts). Fortunately there probably are very few of these users. Maybe. I am not sure how one would measure. If you are interested, I can ask the opinion of some of the GitHub trainers. They see a lot of new users and have a sense of what kinds of confusion come up most frequently, what kinds of workflows they tend to see, etc. Their experience may be biased towards corporate-ish users, though, because those are the people who pay for training. The current warning message in jc/pull-training-wheel is quite neutral between the two options. Perhaps we should lean more towards merging? I don't like that message. I would like this for the deprecation period: The pull was not fast-forward, in the future you would have to choose a merge or a rebase, merging automatically for now. Read 'man git pull' for more help. Then when obsolete: The pull was not fast-forward, please either merge or rebase. A deprecation message helps people who are making the transition from an older behavior to a newer one. It cannot help new users who start with a git version after the deprecation period. Any more babysitting with essay long messages is counter-productive to the vast majority of Git users. I think that is what we have advice.* for. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] upload-pack: send the HEAD information
On Fri, Sep 06, 2013 at 10:46:24AM -0700, Junio C Hamano wrote: I think it is perfectly fine to expose _only_ HEAD now, and wait until we find a good reason that we should send this information for other symbolic refs in the repository. Yeah, I agree with that. However, because we already anticipate that we may find such a good reason later, on-the-wire format should be prepared to support such later enhancement. I think sending symref=HEAD:refs/heads/master is probably one good way to do so, as Peff suggested in that old thread ($gmane/102070; note that back then this wasn't suggested as a proper capability so the exact format he suggests in the message is different). Then we could later add advertisements for other symbolic refs if we find it necessary to do so, e.g. symref=HEAD:refs/heads/master symref=refs/remotes/origin/HEAD:refs/remotes/origin/master (all on one line together with other capabilities separated with a SP in between). It somehow feels a little weird to me that we would output the information about refs/foo on the HEAD line. A few possible issues (and I am playing devil's advocate to some degree here): 1. What if we have a large number of symrefs? Would we run afoul of pkt-line length limits? 2. What's the impact of having to display all symrefs on the first line, before we output other refs? Right now we can just stream out refs as we read them, but we would have to make two passes (and/or cache them all) to find all of the symrefs before we start outputting. Will the extra latency ever matter? What do you think about teaching git to read extra data after \0 for _every_ ref line? And then ref advertisement might look something like: sha1 HEAD\0multi_ack thin-pack ... symref=refs/heads/master\n sha1 refs/heads/master\n sha1 refs/heads/my-alias\0symref=refs/heads/master That would leave us future room for more ref annotations if we should want them, and I think (but haven't tested) that existing receivers should ignore everything after the NUL. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
On Sun, Sep 8, 2013 at 1:54 AM, Jeff King p...@peff.net wrote: On Sun, Sep 08, 2013 at 01:17:42AM -0500, Felipe Contreras wrote: I think it's fine to tell them to do git pull --merge. What I'd worry more about is somebody who is suddenly presented with the choice between --rebase and --merge and doesn't know which to choose. We've created a cognitive load on the user, and even more load if they choose --rebase and don't quite understand what it means. If that happens they will go back to the guy that told them to run those commands. I think the guy may be git itself. For example, here is a possible session with jc/pull-training-wheel: $ git push Who told him to use 'git push'? Certainly not git. To ... ! [rejected]master - master (non-fast-forward) error: failed to push some refs to '...' hint: Updates were rejected because the tip of your current branch is behind hint: its remote counterpart. Integrate the remote changes (e.g. hint: 'git pull ...') before pushing again. hint: See the 'Note about fast-forwards' in 'git push --help' for details. $ git pull The pull does not fast-forward; please specify if you want to merge or rebase. Use either git pull --rebase git pull --merge You can also use 'git config pull.rebase true' (if you want --rebase) or 'git config pull.rebase false' (if you want --merge) to set this once for this project and forget about it. Why stop there? Post the whole man page already. Moreover, it's overly verbose on all the wrong and irrelevant information. If you are going to waste precious screen state, explain wth a non fast-forward is; people can figure out what a merge is, and maybe a rebase, but a non fast-forward definitely not. The user is pointed at pull from push, and then gets presented with the merge or rebase choice. It may be that the advice you can find by googling merge vs rebase is enough to then help the person along (and/or we may need to improve the manpages in that respect). Yes, but that's not the use-case we are talking about. You mentioned specifically a svn-like worfklow where the guy was told by somebody else to replace the svn commands with git ones. If we are talking about a guy that is learning git, that's and entirely different case. I am genuinely curious what people in favor of this feature would want to say in the documentation to a user encountering this choice for the first time. In my experience, rebasing introduces more complications, specifically: Yes, but it's what the user might want. I know those are all balanced by some advantages of rebasing, but I also think they are things that can be troublesome for a user who does not fully grok the rebase process. I'm just wondering if we should mention both, but steer people towards merging as the safer alternative (you might have ugly history, but you are less likely to create a mess with duplicate commits or badly-resolved conflicts). The purpose of this change in the code is not to change the user behavior. The choice of merge vs. rebase is entirely up to the user, and we are not changing that. The purpose of this change is to avoid doing a merge when the user wanted a rebase, or maybe something more complicated. So a rebase being complicated is not an issue, because we know that's what the user wants, that's the whole reason we are trying to avoid the automated merge. Fortunately there probably are very few of these users. Maybe. I am not sure how one would measure. If you are interested, I can ask the opinion of some of the GitHub trainers. They see a lot of new users and have a sense of what kinds of confusion come up most frequently, what kinds of workflows they tend to see, etc. Their experience may be biased towards corporate-ish users, though, because those are the people who pay for training. Ask. I'm sure they will tell you doing merges by mistake with 'git pull' is an issue. The current warning message in jc/pull-training-wheel is quite neutral between the two options. Perhaps we should lean more towards merging? I don't like that message. I would like this for the deprecation period: The pull was not fast-forward, in the future you would have to choose a merge or a rebase, merging automatically for now. Read 'man git pull' for more help. Then when obsolete: The pull was not fast-forward, please either merge or rebase. A deprecation message helps people who are making the transition from an older behavior to a newer one. It cannot help new users who start with a git version after the deprecation period. The new users are told to either merge or rebase, if they don't know what that means, they will go on look it up, just like they looked up the 'git pull' command in the first place. Any more babysitting with essay long messages is counter-productive to the vast majority of Git users. I think that is what we have advice.* for. I don't
[PATCH v2 00/14] pack v4 support in index-pack
Mostly cleanups after Nico's comments. The diff against v2 is
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 4a24bc3..88340b5 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -22,8 +22,8 @@ struct object_entry {
struct pack_idx_entry idx;
unsigned long size;
unsigned int hdr_size;
- enum object_type type;
- enum object_type real_type;
+ enum object_type type; /* type as written in pack */
+ enum object_type real_type; /* type after delta resolving */
unsigned delta_depth;
int base_object_no;
int nr_bases; /* only valid for v4 trees */
@@ -194,8 +194,10 @@ static int mark_link(struct object *obj, int type, void
*data)
return 0;
}
-/* The content of each linked object must have been checked
- or it must be already present in the object database */
+/*
+ * The content of each linked object must have been checked or it must
+ * be already present in the object database
+ */
static unsigned check_object(struct object *obj)
{
if (!obj)
@@ -289,6 +291,19 @@ static inline void *fill_and_use(int bytes)
return p;
}
+static void check_against_sha1table(const unsigned char *sha1)
+{
+ const unsigned char *found;
+ if (!packv4)
+ return;
+
+ found = bsearch(sha1, sha1_table, nr_objects, 20,
+ (int (*)(const void *, const void *))hashcmp);
+ if (!found)
+ die(_(object %s not found in SHA-1 table),
+ sha1_to_hex(sha1));
+}
+
static NORETURN void bad_object(unsigned long offset, const char *format,
...) __attribute__((format (printf, 2, 3)));
@@ -325,15 +340,8 @@ static const unsigned char *read_sha1ref(void)
static const unsigned char *read_sha1table_ref(void)
{
const unsigned char *sha1 = read_sha1ref();
- if (sha1 sha1_table || sha1 = sha1_table + nr_objects * 20) {
- unsigned char *found;
- found = bsearch(sha1, sha1_table, nr_objects, 20,
- (int (*)(const void *, const void *))hashcmp);
- if (!found)
- bad_object(consumed_bytes,
- _(SHA-1 %s not found in SHA-1 table),
- sha1_to_hex(sha1));
- }
+ if (sha1 sha1_table || sha1 = sha1_table + nr_objects * 20)
+ check_against_sha1table(sha1);
return sha1;
}
@@ -346,21 +354,6 @@ static const unsigned char *read_dictref(struct
packv4_dict *dict)
return dict-data + dict-offsets[index];
}
-static void *read_data(int size)
-{
- const int max = sizeof(input_buffer);
- void *buf;
- char *p;
- p = buf = xmalloc(size);
- while (size) {
- int to_fill = size max ? max : size;
- memcpy(p, fill_and_use(to_fill), to_fill);
- p += to_fill;
- size -= to_fill;
- }
- return buf;
-}
-
static const char *open_pack_file(const char *pack_name)
{
if (from_stdin) {
@@ -532,8 +525,7 @@ static void read_and_inflate(unsigned long offset,
git_SHA1_Final(sha1, ctx);
}
-static void *unpack_commit_v4(unsigned int offset,
- unsigned long size,
+static void *unpack_commit_v4(unsigned int offset, unsigned long size,
unsigned char *sha1)
{
unsigned int nb_parents;
@@ -622,7 +614,8 @@ static void add_tree_delta_base(struct object_entry *obj,
* v4 trees are actually kind of deltas and we don't do delta in the
* first pass. This function only walks through a tree object to find
* the end offset, register object dependencies and performs limited
- * validation.
+ * validation. For v4 trees that have no dependencies, we do
+ * uncompress and calculate their SHA-1.
*/
static void *unpack_tree_v4(struct object_entry *obj,
unsigned int offset, unsigned long size,
@@ -641,9 +634,9 @@ static void *unpack_tree_v4(struct object_entry *obj,
add_tree_delta_base(obj, last_base,
delta_start);
} else if (!last_base)
bad_object(offset,
- _(bad copy count index in
unpack_tree_v4));
+ _(missing delta base
unpack_tree_v4));
copy_count = 1;
- if (!copy_count)
+ if (!copy_count || copy_count nr)
bad_object(offset,
_(bad copy count index in
unpack_tree_v4));
nr -= copy_count;
@@ -657,6 +650,13 @@ static void *unpack_tree_v4(struct object_entry *obj,
entry_sha1 = read_sha1ref();
nr--;
+ /*
+
[PATCH v2 04/14] index-pack: split out varint decoding code
---
builtin/index-pack.c | 82
1 file changed, 45 insertions(+), 37 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 1dbabe0..5fbd517 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -277,6 +277,31 @@ static void use(int bytes)
consumed_bytes += bytes;
}
+static inline void *fill_and_use(int bytes)
+{
+ void *p = fill(bytes);
+ use(bytes);
+ return p;
+}
+
+static NORETURN void bad_object(unsigned long offset, const char *format,
+ ...) __attribute__((format (printf, 2, 3)));
+
+static uintmax_t read_varint(void)
+{
+ unsigned char c = *(char*)fill_and_use(1);
+ uintmax_t val = c 127;
+ while (c 128) {
+ val += 1;
+ if (!val || MSB(val, 7))
+ bad_object(consumed_bytes,
+ _(offset overflow in read_varint));
+ c = *(char*)fill_and_use(1);
+ val = (val 7) + (c 127);
+ }
+ return val;
+}
+
static const char *open_pack_file(const char *pack_name)
{
if (from_stdin) {
@@ -317,9 +342,6 @@ static void parse_pack_header(void)
use(sizeof(struct pack_header));
}
-static NORETURN void bad_object(unsigned long offset, const char *format,
- ...) __attribute__((format (printf, 2, 3)));
-
static NORETURN void bad_object(unsigned long offset, const char *format, ...)
{
va_list params;
@@ -462,55 +484,41 @@ static void *unpack_entry_data(unsigned long offset,
unsigned long size,
return buf == fixed_buf ? NULL : buf;
}
+static void read_typesize_v2(struct object_entry *obj)
+{
+ unsigned char c = *(char*)fill_and_use(1);
+ unsigned shift;
+
+ obj-type = (c 4) 7;
+ obj-size = (c 15);
+ shift = 4;
+ while (c 128) {
+ c = *(char*)fill_and_use(1);
+ obj-size += (c 0x7f) shift;
+ shift += 7;
+ }
+}
+
static void *unpack_raw_entry(struct object_entry *obj,
union delta_base *delta_base,
unsigned char *sha1)
{
- unsigned char *p;
- unsigned long size, c;
- off_t base_offset;
- unsigned shift;
void *data;
+ uintmax_t val;
obj-idx.offset = consumed_bytes;
input_crc32 = crc32(0, NULL, 0);
- p = fill(1);
- c = *p;
- use(1);
- obj-type = (c 4) 7;
- size = (c 15);
- shift = 4;
- while (c 0x80) {
- p = fill(1);
- c = *p;
- use(1);
- size += (c 0x7f) shift;
- shift += 7;
- }
- obj-size = size;
+ read_typesize_v2(obj);
switch (obj-type) {
case OBJ_REF_DELTA:
- hashcpy(delta_base-sha1, fill(20));
- use(20);
+ hashcpy(delta_base-sha1, fill_and_use(20));
break;
case OBJ_OFS_DELTA:
memset(delta_base, 0, sizeof(*delta_base));
- p = fill(1);
- c = *p;
- use(1);
- base_offset = c 127;
- while (c 128) {
- base_offset += 1;
- if (!base_offset || MSB(base_offset, 7))
- bad_object(obj-idx.offset, _(offset value
overflow for delta base object));
- p = fill(1);
- c = *p;
- use(1);
- base_offset = (base_offset 7) + (c 127);
- }
- delta_base-offset = obj-idx.offset - base_offset;
+ val = read_varint();
+ delta_base-offset = obj-idx.offset - val;
if (delta_base-offset = 0 || delta_base-offset =
obj-idx.offset)
bad_object(obj-idx.offset, _(delta base offset is out
of bound));
break;
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 02/14] pack v4: add pv4_free_dict()
---
packv4-parse.c | 8
packv4-parse.h | 1 +
sha1_file.c| 2 ++
3 files changed, 11 insertions(+)
diff --git a/packv4-parse.c b/packv4-parse.c
index 82661ba..d515bb9 100644
--- a/packv4-parse.c
+++ b/packv4-parse.c
@@ -63,6 +63,14 @@ struct packv4_dict *pv4_create_dict(const unsigned char
*data, int dict_size)
return dict;
}
+void pv4_free_dict(struct packv4_dict *dict)
+{
+ if (dict) {
+ free((void*)dict-data);
+ free(dict);
+ }
+}
+
static struct packv4_dict *load_dict(struct packed_git *p, off_t *offset)
{
struct pack_window *w_curs = NULL;
diff --git a/packv4-parse.h b/packv4-parse.h
index 0b2405a..e6719f6 100644
--- a/packv4-parse.h
+++ b/packv4-parse.h
@@ -8,6 +8,7 @@ struct packv4_dict {
};
struct packv4_dict *pv4_create_dict(const unsigned char *data, int dict_size);
+void pv4_free_dict(struct packv4_dict *dict);
void *pv4_get_commit(struct packed_git *p, struct pack_window **w_curs,
off_t offset, unsigned long size);
diff --git a/sha1_file.c b/sha1_file.c
index c7bf677..1528e28 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -763,6 +763,8 @@ void free_pack_by_name(const char *pack_name)
}
close_pack_index(p);
free(p-bad_object_sha1);
+ pv4_free_dict(p-ident_dict);
+ pv4_free_dict(p-path_dict);
*pp = p-next;
if (last_found_pack == p)
last_found_pack = NULL;
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 07/14] index-pack: parse v4 header and dictionaries
---
builtin/index-pack.c | 49 -
1 file changed, 48 insertions(+), 1 deletion(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3389262..83e6e79 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -11,6 +11,7 @@
#include exec_cmd.h
#include streaming.h
#include thread-utils.h
+#include packv4-parse.h
static const char index_pack_usage[] =
git index-pack [-v] [-o index-file] [--keep | --keep=msg] [--verify]
[--strict] (pack-file | --stdin [--fix-thin] [pack-file]);
@@ -70,6 +71,8 @@ struct delta_entry {
static struct object_entry *objects;
static struct delta_entry *deltas;
static struct thread_local nothread_data;
+static unsigned char *sha1_table;
+static struct packv4_dict *name_dict, *path_dict;
static int nr_objects;
static int nr_deltas;
static int nr_resolved_deltas;
@@ -81,6 +84,7 @@ static int do_fsck_object;
static int verbose;
static int show_stat;
static int check_self_contained_and_connected;
+static int packv4;
static struct progress *progress;
@@ -334,7 +338,9 @@ static void parse_pack_header(void)
/* Header consistency check */
if (hdr-hdr_signature != htonl(PACK_SIGNATURE))
die(_(pack signature mismatch));
- if (!pack_version_ok(hdr-hdr_version))
+ if (hdr-hdr_version == htonl(4))
+ packv4 = 1;
+ else if (!pack_version_ok(hdr-hdr_version))
die(_(pack version %PRIu32 unsupported),
ntohl(hdr-hdr_version));
@@ -1035,6 +1041,40 @@ static void *threaded_second_pass(void *data)
}
#endif
+static struct packv4_dict *read_dict(void)
+{
+ unsigned long size;
+ unsigned char *data;
+ struct packv4_dict *dict;
+
+ size = read_varint();
+ data = xmallocz(size);
+ read_and_inflate(consumed_bytes, data, size, 0, NULL, NULL);
+ dict = pv4_create_dict(data, size);
+ if (!dict)
+ die(unable to parse dictionary);
+ return dict;
+}
+
+static void parse_dictionaries(void)
+{
+ int i;
+ if (!packv4)
+ return;
+
+ sha1_table = xmalloc(20 * nr_objects);
+ hashcpy(sha1_table, fill_and_use(20));
+ for (i = 1; i nr_objects; i++) {
+ unsigned char *p = sha1_table + i * 20;
+ hashcpy(p, fill_and_use(20));
+ if (hashcmp(p - 20, p) = 0)
+ die(_(wrong order in SHA-1 table at entry %d), i);
+ }
+
+ name_dict = read_dict();
+ path_dict = read_dict();
+}
+
/*
* First pass:
* - find locations of all objects;
@@ -1673,6 +1713,7 @@ int cmd_index_pack(int argc, const char **argv, const
char *prefix)
parse_pack_header();
objects = xcalloc(nr_objects + 1, sizeof(struct object_entry));
deltas = xcalloc(nr_objects, sizeof(struct delta_entry));
+ parse_dictionaries();
parse_pack_objects(pack_sha1);
resolve_deltas();
conclude_pack(fix_thin_pack, curr_pack, pack_sha1);
@@ -1683,6 +1724,9 @@ int cmd_index_pack(int argc, const char **argv, const
char *prefix)
if (show_stat)
show_pack_info(stat_only);
+ if (packv4)
+ die(we're not there yet);
+
idx_objects = xmalloc((nr_objects) * sizeof(struct pack_idx_entry *));
for (i = 0; i nr_objects; i++)
idx_objects[i] = objects[i].idx;
@@ -1699,6 +1743,9 @@ int cmd_index_pack(int argc, const char **argv, const
char *prefix)
free(objects);
free(index_name_buf);
free(keep_name_buf);
+ free(sha1_table);
+ pv4_free_dict(name_dict);
+ pv4_free_dict(path_dict);
if (pack_name == NULL)
free((void *) curr_pack);
if (index_name == NULL)
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 06/14] index-pack: split inflate/digest code out of unpack_entry_data
---
builtin/index-pack.c | 62 +++-
1 file changed, 37 insertions(+), 25 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 78554d0..3389262 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -431,6 +431,40 @@ static int is_delta_type(enum object_type type)
return (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);
}
+static void read_and_inflate(unsigned long offset,
+void *buf, unsigned long size,
+unsigned long wraparound,
+git_SHA_CTX *ctx,
+unsigned char *sha1)
+{
+ git_zstream stream;
+ int status;
+
+ memset(stream, 0, sizeof(stream));
+ git_inflate_init(stream);
+ stream.next_out = buf;
+ stream.avail_out = wraparound ? wraparound : size;
+
+ do {
+ unsigned char *last_out = stream.next_out;
+ stream.next_in = fill(1);
+ stream.avail_in = input_len;
+ status = git_inflate(stream, 0);
+ use(input_len - stream.avail_in);
+ if (sha1)
+ git_SHA1_Update(ctx, last_out, stream.next_out -
last_out);
+ if (wraparound) {
+ stream.next_out = buf;
+ stream.avail_out = wraparound;
+ }
+ } while (status == Z_OK);
+ if (stream.total_out != size || status != Z_STREAM_END)
+ bad_object(offset, _(inflate returned %d), status);
+ git_inflate_end(stream);
+ if (sha1)
+ git_SHA1_Final(sha1, ctx);
+}
+
/*
* Unpack an entry data in the streamed pack, calculate the object
* SHA-1 if it's not a large blob. Otherwise just try to inflate the
@@ -440,8 +474,6 @@ static void *unpack_entry_data(unsigned long offset,
unsigned long size,
enum object_type type, unsigned char *sha1)
{
static char fixed_buf[8192];
- int status;
- git_zstream stream;
void *buf;
git_SHA_CTX c;
char hdr[32];
@@ -459,29 +491,9 @@ static void *unpack_entry_data(unsigned long offset,
unsigned long size,
else
buf = xmalloc(size);
- memset(stream, 0, sizeof(stream));
- git_inflate_init(stream);
- stream.next_out = buf;
- stream.avail_out = buf == fixed_buf ? sizeof(fixed_buf) : size;
-
- do {
- unsigned char *last_out = stream.next_out;
- stream.next_in = fill(1);
- stream.avail_in = input_len;
- status = git_inflate(stream, 0);
- use(input_len - stream.avail_in);
- if (sha1)
- git_SHA1_Update(c, last_out, stream.next_out -
last_out);
- if (buf == fixed_buf) {
- stream.next_out = buf;
- stream.avail_out = sizeof(fixed_buf);
- }
- } while (status == Z_OK);
- if (stream.total_out != size || status != Z_STREAM_END)
- bad_object(offset, _(inflate returned %d), status);
- git_inflate_end(stream);
- if (sha1)
- git_SHA1_Final(sha1, c);
+ read_and_inflate(offset, buf, size,
+buf == fixed_buf ? sizeof(fixed_buf) : 0,
+c, sha1);
return buf == fixed_buf ? NULL : buf;
}
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 03/14] index-pack: add more comments on some big functions
---
builtin/index-pack.c | 30 ++
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 9c1cfac..1dbabe0 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -19,8 +19,8 @@ struct object_entry {
struct pack_idx_entry idx;
unsigned long size;
unsigned int hdr_size;
- enum object_type type;
- enum object_type real_type;
+ enum object_type type; /* type as written in pack */
+ enum object_type real_type; /* type after delta resolving */
unsigned delta_depth;
int base_object_no;
};
@@ -187,8 +187,10 @@ static int mark_link(struct object *obj, int type, void
*data)
return 0;
}
-/* The content of each linked object must have been checked
- or it must be already present in the object database */
+/*
+ * The content of each linked object must have been checked or it must
+ * be already present in the object database
+ */
static unsigned check_object(struct object *obj)
{
if (!obj)
@@ -407,6 +409,11 @@ static int is_delta_type(enum object_type type)
return (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);
}
+/*
+ * Unpack an entry data in the streamed pack, calculate the object
+ * SHA-1 if it's not a large blob. Otherwise just try to inflate the
+ * object to /dev/null to determine the end of the entry in the pack.
+ */
static void *unpack_entry_data(unsigned long offset, unsigned long size,
enum object_type type, unsigned char *sha1)
{
@@ -522,6 +529,11 @@ static void *unpack_raw_entry(struct object_entry *obj,
return data;
}
+/*
+ * Unpack entry data in the second pass when the pack is already
+ * stored on disk. consume call back is used for large-blob case. Must
+ * be thread safe.
+ */
static void *unpack_data(struct object_entry *obj,
int (*consume)(const unsigned char *, unsigned long,
void *),
void *cb_data)
@@ -875,6 +887,11 @@ static void resolve_delta(struct object_entry *delta_obj,
counter_unlock();
}
+/*
+ * Given a base object, search for all objects depending on the base,
+ * try to unpack one of those object. The function will be called
+ * repeatedly until all objects are unpacked.
+ */
static struct base_data *find_unresolved_deltas_1(struct base_data *base,
struct base_data *prev_base)
{
@@ -958,6 +975,10 @@ static int compare_delta_entry(const void *a, const void
*b)
objects[delta_b-obj_no].type);
}
+/*
+ * Unpack all objects depending directly or indirectly on the given
+ * object
+ */
static void resolve_base(struct object_entry *obj)
{
struct base_data *base_obj = alloc_base_data();
@@ -967,6 +988,7 @@ static void resolve_base(struct object_entry *obj)
}
#ifndef NO_PTHREADS
+/* Call resolve_base() in multiple threads */
static void *threaded_second_pass(void *data)
{
set_thread_data(data);
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 01/14] pack v4: split pv4_create_dict() out of load_dict()
---
packv4-parse.c | 63 --
packv4-parse.h | 8
2 files changed, 43 insertions(+), 28 deletions(-)
diff --git a/packv4-parse.c b/packv4-parse.c
index 63bba03..82661ba 100644
--- a/packv4-parse.c
+++ b/packv4-parse.c
@@ -30,11 +30,38 @@ const unsigned char *get_sha1ref(struct packed_git *p,
return sha1;
}
-struct packv4_dict {
- const unsigned char *data;
- unsigned int nb_entries;
- unsigned int offsets[FLEX_ARRAY];
-};
+struct packv4_dict *pv4_create_dict(const unsigned char *data, int dict_size)
+{
+ struct packv4_dict *dict;
+ int i;
+
+ /* count number of entries */
+ int nb_entries = 0;
+ const unsigned char *cp = data;
+ while (cp data + dict_size - 3) {
+ cp += 2; /* prefix bytes */
+ cp += strlen((const char *)cp); /* entry string */
+ cp += 1; /* terminating NUL */
+ nb_entries++;
+ }
+ if (cp - data != dict_size) {
+ error(dict size mismatch);
+ return NULL;
+ }
+
+ dict = xmalloc(sizeof(*dict) + nb_entries * sizeof(dict-offsets[0]));
+ dict-data = data;
+ dict-nb_entries = nb_entries;
+
+ cp = data;
+ for (i = 0; i nb_entries; i++) {
+ dict-offsets[i] = cp - data;
+ cp += 2;
+ cp += strlen((const char *)cp) + 1;
+ }
+
+ return dict;
+}
static struct packv4_dict *load_dict(struct packed_git *p, off_t *offset)
{
@@ -45,7 +72,7 @@ static struct packv4_dict *load_dict(struct packed_git *p,
off_t *offset)
const unsigned char *cp;
git_zstream stream;
struct packv4_dict *dict;
- int nb_entries, i, st;
+ int st;
/* get uncompressed dictionary data size */
src = use_pack(p, w_curs, curpos, avail);
@@ -77,32 +104,12 @@ static struct packv4_dict *load_dict(struct packed_git *p,
off_t *offset)
return NULL;
}
- /* count number of entries */
- nb_entries = 0;
- cp = data;
- while (cp data + dict_size - 3) {
- cp += 2; /* prefix bytes */
- cp += strlen((const char *)cp); /* entry string */
- cp += 1; /* terminating NUL */
- nb_entries++;
- }
- if (cp - data != dict_size) {
- error(dict size mismatch);
+ dict = pv4_create_dict(data, dict_size);
+ if (!dict) {
free(data);
return NULL;
}
- dict = xmalloc(sizeof(*dict) + nb_entries * sizeof(dict-offsets[0]));
- dict-data = data;
- dict-nb_entries = nb_entries;
-
- cp = data;
- for (i = 0; i nb_entries; i++) {
- dict-offsets[i] = cp - data;
- cp += 2;
- cp += strlen((const char *)cp) + 1;
- }
-
*offset = curpos;
return dict;
}
diff --git a/packv4-parse.h b/packv4-parse.h
index 5f9d809..0b2405a 100644
--- a/packv4-parse.h
+++ b/packv4-parse.h
@@ -1,6 +1,14 @@
#ifndef PACKV4_PARSE_H
#define PACKV4_PARSE_H
+struct packv4_dict {
+ const unsigned char *data;
+ unsigned int nb_entries;
+ unsigned int offsets[FLEX_ARRAY];
+};
+
+struct packv4_dict *pv4_create_dict(const unsigned char *data, int dict_size);
+
void *pv4_get_commit(struct packed_git *p, struct pack_window **w_curs,
off_t offset, unsigned long size);
void *pv4_get_tree(struct packed_git *p, struct pack_window **w_curs,
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 13/14] index-pack: skip looking for ofs-deltas in v4 as they are not allowed
---
builtin/index-pack.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index e903a49..ce06473 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1173,10 +1173,13 @@ static struct base_data
*find_unresolved_deltas_1(struct base_data *base,
find_delta_children(base_spec,
base-ref_first, base-ref_last,
OBJ_REF_DELTA);
- memset(base_spec, 0, sizeof(base_spec));
- base_spec.offset = base-obj-idx.offset;
- find_delta_children(base_spec,
- base-ofs_first, base-ofs_last,
OBJ_OFS_DELTA);
+ if (!packv4) {
+ memset(base_spec, 0, sizeof(base_spec));
+ base_spec.offset = base-obj-idx.offset;
+ find_delta_children(base_spec,
+ base-ofs_first, base-ofs_last,
+ OBJ_OFS_DELTA);
+ }
if (base-ref_last == -1 base-ofs_last == -1) {
free(base-data);
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 11/14] index-pack: move delta base queuing code to unpack_raw_entry
For v2, ofs-delta and ref-delta can only have queue one delta base at
a time. A v4 tree can have more than one delta base. Move the queuing
code up to unpack_raw_entry() and give unpack_tree_v4() more
flexibility to add its bases.
---
builtin/index-pack.c | 46 ++
1 file changed, 30 insertions(+), 16 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index dcb6409..8f2d929 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -568,6 +568,25 @@ static void *unpack_commit_v4(unsigned int offset,
unsigned long size,
return dst.buf;
}
+static void add_sha1_delta(struct object_entry *obj,
+ const unsigned char *sha1)
+{
+ struct delta_entry *delta = deltas + nr_deltas;
+ delta-obj_no = obj - objects;
+ hashcpy(delta-base.sha1, sha1);
+ nr_deltas++;
+}
+
+static void add_ofs_delta(struct object_entry *obj,
+ off_t offset)
+{
+ struct delta_entry *delta = deltas + nr_deltas;
+ delta-obj_no = obj - objects;
+ memset(delta-base, 0, sizeof(delta-base));
+ delta-base.offset = offset;
+ nr_deltas++;
+}
+
/*
* v4 trees are actually kind of deltas and we don't do delta in the
* first pass. This function only walks through a tree object to find
@@ -703,17 +722,16 @@ static void read_typesize_v2(struct object_entry *obj)
}
static void *unpack_raw_entry(struct object_entry *obj,
- union delta_base *delta_base,
unsigned char *sha1)
{
void *data;
- uintmax_t val;
+ off_t offset;
obj-idx.offset = consumed_bytes;
input_crc32 = crc32(0, NULL, 0);
if (packv4) {
- val = read_varint();
+ uintmax_t val = read_varint();
obj-type = val 15;
obj-size = val 4;
} else
@@ -722,14 +740,14 @@ static void *unpack_raw_entry(struct object_entry *obj,
switch (obj-type) {
case OBJ_REF_DELTA:
- hashcpy(delta_base-sha1, fill_and_use(20));
+ add_sha1_delta(obj, fill_and_use(20));
break;
case OBJ_OFS_DELTA:
- memset(delta_base, 0, sizeof(*delta_base));
- val = read_varint();
- delta_base-offset = obj-idx.offset - val;
- if (delta_base-offset = 0 || delta_base-offset =
obj-idx.offset)
- bad_object(obj-idx.offset, _(delta base offset is out
of bound));
+ offset = obj-idx.offset - read_varint();
+ if (offset = 0 || offset = obj-idx.offset)
+ bad_object(obj-idx.offset,
+ _(delta base offset is out of bound));
+ add_ofs_delta(obj, offset);
break;
case OBJ_COMMIT:
case OBJ_TREE:
@@ -1282,7 +1300,6 @@ static void parse_dictionaries(void)
static void parse_pack_objects(unsigned char *sha1)
{
int i, nr_delays = 0;
- struct delta_entry *delta = deltas;
struct stat st;
if (verbose)
@@ -1291,12 +1308,9 @@ static void parse_pack_objects(unsigned char *sha1)
nr_objects);
for (i = 0; i nr_objects; i++) {
struct object_entry *obj = objects[i];
- void *data = unpack_raw_entry(obj, delta-base, obj-idx.sha1);
- if (is_delta_type(obj-type)) {
- nr_deltas++;
- delta-obj_no = i;
- delta++;
- } else if (!data obj-type == OBJ_PV4_TREE) {
+ void *data = unpack_raw_entry(obj, obj-idx.sha1);
+ if (is_delta_type(obj-type) ||
+ (!data obj-type == OBJ_PV4_TREE)) {
/* delay sha1_object() until second pass */
} else if (!data) {
/* large blobs, check later */
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 09/14] index-pack: parse v4 commit format
---
builtin/index-pack.c | 94 ++--
1 file changed, 91 insertions(+), 3 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index efb969a..473514a 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -319,6 +319,30 @@ static uintmax_t read_varint(void)
return val;
}
+static const unsigned char *read_sha1ref(void)
+{
+ unsigned int index = read_varint();
+ if (!index) {
+ static unsigned char sha1[20];
+ hashcpy(sha1, fill_and_use(20));
+ return sha1;
+ }
+ index--;
+ if (index = nr_objects)
+ bad_object(consumed_bytes,
+ _(bad index in read_sha1ref));
+ return sha1_table + index * 20;
+}
+
+static const unsigned char *read_dictref(struct packv4_dict *dict)
+{
+ unsigned int index = read_varint();
+ if (index = dict-nb_entries)
+ bad_object(consumed_bytes,
+ _(bad index in read_dictref));
+ return dict-data + dict-offsets[index];
+}
+
static const char *open_pack_file(const char *pack_name)
{
if (from_stdin) {
@@ -484,6 +508,58 @@ static void read_and_inflate(unsigned long offset,
git_SHA1_Final(sha1, ctx);
}
+static void *unpack_commit_v4(unsigned int offset, unsigned long size,
+ unsigned char *sha1)
+{
+ unsigned int nb_parents;
+ const unsigned char *committer, *author, *ident;
+ unsigned long author_time, committer_time;
+ git_SHA_CTX ctx;
+ char hdr[32];
+ int hdrlen;
+ int16_t committer_tz, author_tz;
+ struct strbuf dst;
+
+ strbuf_init(dst, size);
+
+ strbuf_addf(dst, tree %s\n, sha1_to_hex(read_sha1ref()));
+ nb_parents = read_varint();
+ while (nb_parents--)
+ strbuf_addf(dst, parent %s\n, sha1_to_hex(read_sha1ref()));
+
+ committer_time = read_varint();
+ ident = read_dictref(name_dict);
+ committer_tz = (ident[0] 8) | ident[1];
+ committer = ident + 2;
+
+ author_time = read_varint();
+ ident = read_dictref(name_dict);
+ author_tz = (ident[0] 8) | ident[1];
+ author = ident + 2;
+
+ if (author_time 1)
+ author_time = committer_time + (author_time 1);
+ else
+ author_time = committer_time - (author_time 1);
+
+ strbuf_addf(dst,
+ author %s %lu %+05d\n
+ committer %s %lu %+05d\n,
+ author, author_time, author_tz,
+ committer, committer_time, committer_tz);
+
+ if (dst.len size)
+ bad_object(offset, _(bad commit));
+
+ hdrlen = sprintf(hdr, commit %lu, size) + 1;
+ git_SHA1_Init(ctx);
+ git_SHA1_Update(ctx, hdr, hdrlen);
+ git_SHA1_Update(ctx, dst.buf, dst.len);
+ read_and_inflate(offset, dst.buf + dst.len, size - dst.len,
+0, ctx, sha1);
+ return dst.buf;
+}
+
/*
* Unpack an entry data in the streamed pack, calculate the object
* SHA-1 if it's not a large blob. Otherwise just try to inflate the
@@ -498,6 +574,9 @@ static void *unpack_entry_data(unsigned long offset,
unsigned long size,
char hdr[32];
int hdrlen;
+ if (type == OBJ_PV4_COMMIT)
+ return unpack_commit_v4(offset, size, sha1);
+
if (!is_delta_type(type)) {
hdrlen = sprintf(hdr, %s %lu, typename(type), size) + 1;
git_SHA1_Init(c);
@@ -541,7 +620,13 @@ static void *unpack_raw_entry(struct object_entry *obj,
obj-idx.offset = consumed_bytes;
input_crc32 = crc32(0, NULL, 0);
- read_typesize_v2(obj);
+ if (packv4) {
+ val = read_varint();
+ obj-type = val 15;
+ obj-size = val 4;
+ } else
+ read_typesize_v2(obj);
+ obj-real_type = obj-type;
switch (obj-type) {
case OBJ_REF_DELTA:
@@ -559,6 +644,10 @@ static void *unpack_raw_entry(struct object_entry *obj,
case OBJ_BLOB:
case OBJ_TAG:
break;
+
+ case OBJ_PV4_COMMIT:
+ obj-real_type = OBJ_COMMIT;
+ break;
default:
bad_object(obj-idx.offset, _(unknown object type %d),
obj-type);
}
@@ -1108,7 +1197,6 @@ static void parse_pack_objects(unsigned char *sha1)
for (i = 0; i nr_objects; i++) {
struct object_entry *obj = objects[i];
void *data = unpack_raw_entry(obj, delta-base, obj-idx.sha1);
- obj-real_type = obj-type;
if (is_delta_type(obj-type)) {
nr_deltas++;
delta-obj_no = i;
@@ -1120,7 +1208,7 @@ static void parse_pack_objects(unsigned char *sha1)
check_against_sha1table(obj-idx.sha1);
} else {
[PATCH v2 08/14] index-pack: make sure all objects are registered in v4's SHA-1 table
---
builtin/index-pack.c | 22 --
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 83e6e79..efb969a 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -288,6 +288,19 @@ static inline void *fill_and_use(int bytes)
return p;
}
+static void check_against_sha1table(const unsigned char *sha1)
+{
+ const unsigned char *found;
+ if (!packv4)
+ return;
+
+ found = bsearch(sha1, sha1_table, nr_objects, 20,
+ (int (*)(const void *, const void *))hashcmp);
+ if (!found)
+ die(_(object %s not found in SHA-1 table),
+ sha1_to_hex(sha1));
+}
+
static NORETURN void bad_object(unsigned long offset, const char *format,
...) __attribute__((format (printf, 2, 3)));
@@ -907,6 +920,7 @@ static void resolve_delta(struct object_entry *delta_obj,
bad_object(delta_obj-idx.offset, _(failed to apply delta));
hash_sha1_file(result-data, result-size,
typename(delta_obj-real_type), delta_obj-idx.sha1);
+ check_against_sha1table(delta_obj-idx.sha1);
sha1_object(result-data, NULL, result-size, delta_obj-real_type,
delta_obj-idx.sha1);
counter_lock();
@@ -1103,8 +1117,12 @@ static void parse_pack_objects(unsigned char *sha1)
/* large blobs, check later */
obj-real_type = OBJ_BAD;
nr_delays++;
- } else
- sha1_object(data, NULL, obj-size, obj-type,
obj-idx.sha1);
+ check_against_sha1table(obj-idx.sha1);
+ } else {
+ check_against_sha1table(obj-idx.sha1);
+ sha1_object(data, NULL, obj-size, obj-type,
+ obj-idx.sha1);
+ }
free(data);
display_progress(progress, i+1);
}
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 12/14] index-pack: record all delta bases in v4 (tree and ref-delta)
---
builtin/index-pack.c | 33 ++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 8f2d929..e903a49 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -24,6 +24,7 @@ struct object_entry {
enum object_type real_type; /* type after delta resolving */
unsigned delta_depth;
int base_object_no;
+ int nr_bases; /* only valid for v4 trees */
};
union delta_base {
@@ -482,6 +483,11 @@ static int is_delta_type(enum object_type type)
return (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);
}
+static int is_delta_tree(const struct object_entry *obj)
+{
+ return obj-type == OBJ_PV4_TREE obj-nr_bases 0;
+}
+
static void read_and_inflate(unsigned long offset,
void *buf, unsigned long size,
unsigned long wraparound,
@@ -587,6 +593,20 @@ static void add_ofs_delta(struct object_entry *obj,
nr_deltas++;
}
+static void add_tree_delta_base(struct object_entry *obj,
+ const unsigned char *base,
+ int delta_start)
+{
+ int i;
+
+ for (i = delta_start; i nr_deltas; i++)
+ if (!hashcmp(base, deltas[i].base.sha1))
+ return;
+
+ add_sha1_delta(obj, base);
+ obj-nr_bases++;
+}
+
/*
* v4 trees are actually kind of deltas and we don't do delta in the
* first pass. This function only walks through a tree object to find
@@ -601,12 +621,14 @@ static void *unpack_tree_v4(struct object_entry *obj,
unsigned int nr = read_varint();
const unsigned char *last_base = NULL;
struct strbuf sb = STRBUF_INIT;
+ int delta_start = nr_deltas;
while (nr) {
unsigned int copy_start_or_path = read_varint();
if (copy_start_or_path 1) { /* copy_start */
unsigned int copy_count = read_varint();
if (copy_count 1) { /* first delta */
last_base = read_sha1table_ref();
+ add_tree_delta_base(obj, last_base,
delta_start);
} else if (!last_base)
bad_object(offset,
_(missing delta base
unpack_tree_v4));
@@ -740,9 +762,15 @@ static void *unpack_raw_entry(struct object_entry *obj,
switch (obj-type) {
case OBJ_REF_DELTA:
- add_sha1_delta(obj, fill_and_use(20));
+ if (packv4)
+ add_sha1_delta(obj, read_sha1table_ref());
+ else
+ add_sha1_delta(obj, fill_and_use(20));
break;
case OBJ_OFS_DELTA:
+ if (packv4)
+ die(_(pack version 4 does not support ofs-delta type
(offset %lu)),
+ obj-idx.offset);
offset = obj-idx.offset - read_varint();
if (offset = 0 || offset = obj-idx.offset)
bad_object(obj-idx.offset,
@@ -1309,8 +1337,7 @@ static void parse_pack_objects(unsigned char *sha1)
for (i = 0; i nr_objects; i++) {
struct object_entry *obj = objects[i];
void *data = unpack_raw_entry(obj, obj-idx.sha1);
- if (is_delta_type(obj-type) ||
- (!data obj-type == OBJ_PV4_TREE)) {
+ if (is_delta_type(obj-type) || is_delta_tree(obj)) {
/* delay sha1_object() until second pass */
} else if (!data) {
/* large blobs, check later */
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 14/14] index-pack: resolve v4 one-base trees
This is the most common case for delta trees. In fact it's the only
kind that's produced by packv4-create. It fits well in the way
index-pack resolves deltas and benefits from threading (the set of
objects depending on this base does not overlap with the set of
objects depending on another base)
Multi-base trees will be probably processed differently.
---
builtin/index-pack.c | 195 ++-
1 file changed, 179 insertions(+), 16 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index ce06473..88340b5 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -12,6 +12,8 @@
#include streaming.h
#include thread-utils.h
#include packv4-parse.h
+#include varint.h
+#include tree-walk.h
static const char index_pack_usage[] =
git index-pack [-v] [-o index-file] [--keep | --keep=msg] [--verify]
[--strict] (pack-file | --stdin [--fix-thin] [pack-file]);
@@ -38,8 +40,8 @@ struct base_data {
struct object_entry *obj;
void *data;
unsigned long size;
- int ref_first, ref_last;
- int ofs_first, ofs_last;
+ int ref_first, ref_last, tree_first;
+ int ofs_first, ofs_last, tree_last;
};
#if !defined(NO_PTHREADS) defined(NO_THREAD_SAFE_PREAD)
@@ -430,6 +432,7 @@ static struct base_data *alloc_base_data(void)
memset(base, 0, sizeof(*base));
base-ref_last = -1;
base-ofs_last = -1;
+ base-tree_last = -1;
return base;
}
@@ -670,6 +673,8 @@ static void *unpack_tree_v4(struct object_entry *obj,
}
if (last_base) {
+ if (nr_deltas - delta_start 1)
+ die(sorry guys, multi-base trees are not supported
yet);
strbuf_release(sb);
return NULL;
} else {
@@ -800,6 +805,84 @@ static void *unpack_raw_entry(struct object_entry *obj,
}
/*
+ * Some checks are skipped because they are already done by
+ * unpack_tree_v4() in the first pass.
+ */
+static void *patch_one_base_tree(const struct object_entry *src,
+const unsigned char *src_buf,
+const unsigned char *delta_buf,
+unsigned long delta_size,
+unsigned long *dst_size)
+{
+ int nr;
+ const unsigned char *last_base = NULL;
+ struct strbuf sb = STRBUF_INIT;
+ const unsigned char *p = delta_buf;
+
+ nr = decode_varint(p);
+ while (nr 0 p delta_buf + delta_size) {
+ unsigned int copy_start_or_path = decode_varint(p);
+ if (copy_start_or_path 1) { /* copy_start */
+ struct tree_desc desc;
+ struct name_entry entry;
+ unsigned int copy_count = decode_varint(p);
+ unsigned int copy_start = copy_start_or_path 1;
+ if (!src)
+ die(we are not supposed to copy from another
tree!);
+ if (copy_count 1) { /* first delta */
+ unsigned int id = decode_varint(p);
+ if (!id) {
+ last_base = p;
+ p += 20;
+ } else
+ last_base = sha1_table + (id - 1) * 20;
+ if (hashcmp(last_base, src-idx.sha1))
+ die(_(bad tree base in
patch_one_base_tree));
+ }
+
+ copy_count = 1;
+ nr -= copy_count;
+
+ init_tree_desc(desc, src_buf, src-size);
+ while (tree_entry(desc, entry)) {
+ if (copy_start)
+ copy_start--;
+ else if (copy_count) {
+ strbuf_addf(sb, %o %s%c,
+ entry.mode, entry.path,
'\0');
+ strbuf_add(sb, entry.sha1, 20);
+ copy_count--;
+ } else
+ break;
+ }
+ } else {/* path */
+ unsigned int path_idx = copy_start_or_path 1;
+ const unsigned char *path;
+ unsigned mode;
+ unsigned int id;
+ const unsigned char *entry_sha1;
+
+ id = decode_varint(p);
+ if (!id) {
+ entry_sha1 = p;
+ p += 20;
+ } else
+ entry_sha1 = sha1_table + (id - 1) * 20;
+ nr--;
+
+
[PATCH v2 10/14] index-pack: parse v4 tree format
---
builtin/index-pack.c | 105 +--
1 file changed, 101 insertions(+), 4 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 473514a..dcb6409 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -334,6 +334,14 @@ static const unsigned char *read_sha1ref(void)
return sha1_table + index * 20;
}
+static const unsigned char *read_sha1table_ref(void)
+{
+ const unsigned char *sha1 = read_sha1ref();
+ if (sha1 sha1_table || sha1 = sha1_table + nr_objects * 20)
+ check_against_sha1table(sha1);
+ return sha1;
+}
+
static const unsigned char *read_dictref(struct packv4_dict *dict)
{
unsigned int index = read_varint();
@@ -561,21 +569,105 @@ static void *unpack_commit_v4(unsigned int offset,
unsigned long size,
}
/*
+ * v4 trees are actually kind of deltas and we don't do delta in the
+ * first pass. This function only walks through a tree object to find
+ * the end offset, register object dependencies and performs limited
+ * validation. For v4 trees that have no dependencies, we do
+ * uncompress and calculate their SHA-1.
+ */
+static void *unpack_tree_v4(struct object_entry *obj,
+ unsigned int offset, unsigned long size,
+ unsigned char *sha1)
+{
+ unsigned int nr = read_varint();
+ const unsigned char *last_base = NULL;
+ struct strbuf sb = STRBUF_INIT;
+ while (nr) {
+ unsigned int copy_start_or_path = read_varint();
+ if (copy_start_or_path 1) { /* copy_start */
+ unsigned int copy_count = read_varint();
+ if (copy_count 1) { /* first delta */
+ last_base = read_sha1table_ref();
+ } else if (!last_base)
+ bad_object(offset,
+ _(missing delta base
unpack_tree_v4));
+ copy_count = 1;
+ if (!copy_count || copy_count nr)
+ bad_object(offset,
+ _(bad copy count index in
unpack_tree_v4));
+ nr -= copy_count;
+ } else {/* path */
+ unsigned int path_idx = copy_start_or_path 1;
+ const unsigned char *entry_sha1;
+
+ if (path_idx = path_dict-nb_entries)
+ bad_object(offset,
+ _(bad path index in
unpack_tree_v4));
+ entry_sha1 = read_sha1ref();
+ nr--;
+
+ /*
+* Attempt to rebuild a canonical (base) tree.
+* If last_base is set, this tree depends on
+* another tree, which we have no access at this
+* stage, so reconstruction must be delayed until
+* the second pass.
+*/
+ if (!last_base) {
+ const unsigned char *path;
+ unsigned mode;
+
+ path = path_dict-data +
path_dict-offsets[path_idx];
+ mode = (path[0] 8) | path[1];
+ strbuf_addf(sb, %o %s%c, mode, path+2, '\0');
+ strbuf_add(sb, entry_sha1, 20);
+ if (sb.len size)
+ bad_object(offset,
+ _(tree larger than
expected));
+ }
+ }
+ }
+
+ if (last_base) {
+ strbuf_release(sb);
+ return NULL;
+ } else {
+ git_SHA_CTX ctx;
+ char hdr[32];
+ int hdrlen;
+
+ if (sb.len != size)
+ bad_object(offset, _(tree size mismatch));
+
+ hdrlen = sprintf(hdr, tree %lu, size) + 1;
+ git_SHA1_Init(ctx);
+ git_SHA1_Update(ctx, hdr, hdrlen);
+ git_SHA1_Update(ctx, sb.buf, size);
+ git_SHA1_Final(sha1, ctx);
+ return strbuf_detach(sb, NULL);
+ }
+}
+
+/*
* Unpack an entry data in the streamed pack, calculate the object
* SHA-1 if it's not a large blob. Otherwise just try to inflate the
* object to /dev/null to determine the end of the entry in the pack.
*/
-static void *unpack_entry_data(unsigned long offset, unsigned long size,
- enum object_type type, unsigned char *sha1)
+static void *unpack_entry_data(struct object_entry *obj, unsigned char *sha1)
{
static char fixed_buf[8192];
void *buf;
git_SHA_CTX c;
char hdr[32];
int hdrlen;
+
Re: [PATCH 1/3] upload-pack: send the HEAD information
On Sun, Sep 08, 2013 at 03:13:59AM -0400, Jeff King wrote: What do you think about teaching git to read extra data after \0 for _every_ ref line? And then ref advertisement might look something like: sha1 HEAD\0multi_ack thin-pack ... symref=refs/heads/master\n sha1 refs/heads/master\n sha1 refs/heads/my-alias\0symref=refs/heads/master That would leave us future room for more ref annotations if we should want them, and I think (but haven't tested) that existing receivers should ignore everything after the NUL. Meh, elsewhere you said: The capability list _could_ be sent more than once, and the receiving end is prepared to accept such a stream. Everything learned from an older capability list needs to be forgot and only the last one is honored, I think. and I think you are right. We simply keep a copy of the string the server sent, and when we see a new one, we free the old and replace it. So each subsequent ref would have to re-send the whole capability string (only if it is a symref, but still, it is somewhat ugly). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Officially start moving to the term 'staging area'
From: Felipe Contreras felipe.contre...@gmail.com Sent: Sunday, September 08, 2013 2:33 AM [snip...] The snapshot concept is totally orthogonal from the staging area concept. Git works in snapshots, which are frozen images of how the content tree was at a certain point in time; IOW; a commit. (I feel that) In most peoples minds the need for a staging area, and the use of snapshots, are related. Part of that relationship, often not noticed by those folks, is that they are 'orthogonal' to *each other*. Thus orthogonality means both un-related, and related at the same time (aren't we humans peculiar!). They are cleaved to each other. When trying to explain staging/index I tend to use the analogy of an old style office (I am almost that old) where one has an In tray and an Out tray on one's desk (and one parked WIP for lunch time desk tidy), and the staging area is the basket at the end marked 'For Filing'. When the 'For Filing' basket is ready, one called the filing clerk to dictate the cover note and away it went, commited to some remote filing repository. Oh how things have changed ;-) _How_ that snapshot is created is an entirely different topic, and the staging area is a tool to create the desired snapshots. The user might decide to never use that tool (i.e. always run git commit -a), but the concept of snapshots remain. So, clearly, one concept has absolutely nothing to do with the other. The point would be that we allow a particular snapshot to be selected, and that the git commit -a is but one (common) method. Commit -a is like jumping in the car for a quick trip to the shops, while the selective staging of content is like packing for a holiday. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
On Sun, Sep 08, 2013 at 02:15:17AM -0500, Felipe Contreras wrote: I think the guy may be git itself. For example, here is a possible session with jc/pull-training-wheel: $ git push Who told him to use 'git push'? Certainly not git. Any of the hundreds of existing tutorials that teach basic git commands like push? To ... ! [rejected]master - master (non-fast-forward) error: failed to push some refs to '...' hint: Updates were rejected because the tip of your current branch is behind hint: its remote counterpart. Integrate the remote changes (e.g. hint: 'git pull ...') before pushing again. hint: See the 'Note about fast-forwards' in 'git push --help' for details. [...] Why stop there? Post the whole man page already. Moreover, it's overly verbose on all the wrong and irrelevant information. If you are going to waste precious screen state, explain wth a non fast-forward is; people can figure out what a merge is, and maybe a rebase, but a non fast-forward definitely not. Note that I was not trying to defend any of the messages, but only showing a plausible mechanism by which a user with basic knowledge that he wants to push may arrive at the question what is the difference between merge and rebase?. If you want to suggest revisions for the push message, go ahead. The push advice _is_ an attempt to define non-fast-forwards in plain language without taking up too much space, but perhaps you can do better. You could even suggest omitting it entirely, but I'm not sure if that is a good idea. It was not added in a vacuum; we lacked that advice for many years, and people complained about it quite a bit until it was added. The user is pointed at pull from push, and then gets presented with the merge or rebase choice. It may be that the advice you can find by googling merge vs rebase is enough to then help the person along (and/or we may need to improve the manpages in that respect). Yes, but that's not the use-case we are talking about. You mentioned specifically a svn-like worfklow where the guy was told by somebody else to replace the svn commands with git ones. No, I mentioned an svn-like workflow. I didn't say anything about how they were told. They might have been told by a co-worker, or read a brief tutorial on git, or read something like Git-SVN Crash Course. If we are talking about a guy that is learning git, that's and entirely different case. That is certainly what I meant to be talking about. The purpose of this change in the code is not to change the user behavior. The choice of merge vs. rebase is entirely up to the user, and we are not changing that. Right, but by not doing anything by default, you are forcing the user to make a decision. Right now, we strongly encourage merging by making it the default, and you have to learn about rebasing separately. But a message that mentions them both as equals is going to lead to extra work for the user; they have to figure out which one is most appropriate. My concern is that this is non-trivial for new users, and that they may end up arbitrarily picking rebase, which is probably not doing them any favors if they do not understand it. For clueful users, choosing between the two is not hard. But some people seem to have trouble understanding the DAG. I don't know how large a group that is, and how any pain caused by this change might compare to the times it will help. If you are interested, I can ask the opinion of some of the GitHub trainers. They see a lot of new users and have a sense of what kinds of confusion come up most frequently, what kinds of workflows they tend to see, etc. Their experience may be biased towards corporate-ish users, though, because those are the people who pay for training. Ask. I'm sure they will tell you doing merges by mistake with 'git pull' is an issue. I've sent an email. I'll post the response when I get it. Any more babysitting with essay long messages is counter-productive to the vast majority of Git users. I think that is what we have advice.* for. I don't understand what that means. It means that some time ago, after many people complained that git did not give enough hints, we added many hints. Some people who did not need these hints would want to disable them, and we have the advice.* config options to do so. So we can have a longer message for new users, and a shorter one for people who do not want to be bothered with the long advice. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
From: Felipe Contreras felipe.contre...@gmail.com
Sent: Sunday, September 08, 2013 3:34 AM
On Thu, Sep 5, 2013 at 3:06 AM, John Keeping j...@keeping.me.uk
wrote:
On Wed, Sep 04, 2013 at 03:59:18PM -0700, Junio C Hamano wrote:
Are there cases where you do not want to either rebase nor merge?
If so what do you want to do after git pull fetches from the other
side? Nothing?
One other thing that I can see being useful occasionally is:
git rebase @{u}@{1} --onto @{u}
which allows local commits to be replayed onto a rewritten upstream
branch.
Although I agree with your side note below that people doing this may
be
better off fetching and then updating their local branch,
particularly
if @{1} is not the correct reflog entry for the upstream when they
created the branch.
That's why after recognizing the fact the you can't find the branch
point of a branch in Git, I decided to write patches to support the
@{tail} shorthand, which is basically the point where the branch was
created, or rebased to:
https://github.com/felipec/git/commits/fc/base
And if 'git rebase' was fixed to ignore the commits already in the
rebased onto branch, almost always what you would want to do is 'git
rebase @{tail} --onto @{upstream}'.
The use case that trips me up (i.e. doesn't fit the above) is when I
have a branch that may need rebasing on (onto) pu, or may need rebasing
on master, or next, depending on what others have been doing.
As a Distributed VCS (i.e. others doing work independently), a rebase
always has the possibility that the world has moved on and one has to
adapt to the new world order by moving location (--onto somewhere new),
not just fixing up the house (patch conflicts). When the update order is
unknown there is no guaranteed solution (IIUC).
You are right that mostly what one wants to do is stick with ones
current location and patch up conflicts, it's just that one din't want
any conflicts in the first place (i.e. the fast forward check).
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git tag output order is incorrect (IMHO)
On Tue, Aug 20, 2013 at 05:12:47PM +0200, Antoine Pelisse wrote: On Sat, Jul 20, 2013 at 2:22 AM, Jeff King p...@peff.net wrote: I do plan to finish it eventually, but if anyone else feels like picking it up, I'd be glad to review patches and/or share my work-in-progress as a starting point. I have some free time to come, and would like to work on that feature. Does the offer still hold ? If it does, I would be interested in your patches. I'm sorry I have taken so long to get back to you on this. I was hoping to revisit the topic and make sure the patches were in a sensible state for showing to somebody. But it took me some time to get around to it, and now that I have, they're really not looking very good. My general strategy was to factor out all of the which refs to select code from git-tag (which knows --contains and --points-at) and git-branch (which knows --merged, --no-merged, and --contains), and then make them all available in a library-ish way to both commands, as well as for-each-ref (which also knows name matching, which all 3 should know, too). You can see my messy in-progress commit (that does not even compile) at: git://github.com/peff/git.git jk/contains-wip Part of the complication is that the filters have to happen at different times (you can efficiently ask --contains for each ref as you see it, but asking --merged must happen after you have collected each one). I do not recall at this point what other issues led me to stop working on it (it may simply have been time for dinner, and I never came back to it). So the patches there may or may not actually be helpful to you. Sorry I can't be more helpful. I'd be happy to discuss or review if you want to work on it. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
From: Jeff King p...@peff.net Sent: Sunday, September 08, 2013 5:06 AM Was there some objective argument made that I missed? Here's more; human semantics: Isn't this one of those pick any two from three tasks: 'human', 'objective', 'argument'. Only 1/6 of the time is an 'objective' answer the result. Philip Between thee and me there's nowt so queer as fowk, and I ain't so sure about thee. old Yorkshire saying. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
On Sun, Sep 8, 2013 at 3:01 AM, Philip Oakley philipoak...@iee.org wrote:
From: Felipe Contreras felipe.contre...@gmail.com
Sent: Sunday, September 08, 2013 3:34 AM
On Thu, Sep 5, 2013 at 3:06 AM, John Keeping j...@keeping.me.uk wrote:
On Wed, Sep 04, 2013 at 03:59:18PM -0700, Junio C Hamano wrote:
Are there cases where you do not want to either rebase nor merge?
If so what do you want to do after git pull fetches from the other
side? Nothing?
One other thing that I can see being useful occasionally is:
git rebase @{u}@{1} --onto @{u}
which allows local commits to be replayed onto a rewritten upstream
branch.
Although I agree with your side note below that people doing this may be
better off fetching and then updating their local branch, particularly
if @{1} is not the correct reflog entry for the upstream when they
created the branch.
That's why after recognizing the fact the you can't find the branch
point of a branch in Git, I decided to write patches to support the
@{tail} shorthand, which is basically the point where the branch was
created, or rebased to:
https://github.com/felipec/git/commits/fc/base
And if 'git rebase' was fixed to ignore the commits already in the
rebased onto branch, almost always what you would want to do is 'git
rebase @{tail} --onto @{upstream}'.
The use case that trips me up (i.e. doesn't fit the above) is when I have a
branch that may need rebasing on (onto) pu, or may need rebasing on master,
or next, depending on what others have been doing.
Yes, so you would do:
% git rebase --onto pu
Which would be translated to:
% git rebase @{tail} --onto pu
What's the problem?
As a Distributed VCS (i.e. others doing work independently), a rebase always
has the possibility that the world has moved on and one has to adapt to the
new world order by moving location (--onto somewhere new), not just fixing
up the house (patch conflicts). When the update order is unknown there is no
guaranteed solution (IIUC).
Yeah, but almost always you want to rebase onto @{upstream}.
--
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 0/5] git-config and large integers
Here's a re-roll (and re-design) of my series to help with git config and large integers. It takes the different approach we discussed of simply removing (well, increasing) the range limits for git config --int. I also improved the error messages for bogus config (patches 3-4). [1/5]: config: factor out integer parsing from range checks [2/5]: config: properly range-check integer values [3/5]: config: set errno in numeric git_parse_* functions [4/5]: config: make numeric parsing errors more clear [5/5]: git-config: always treat --int as 64-bit internally -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/5] config: properly range-check integer values
When we look at a config value as an integer using the
git_config_int function, we carefully range-check the value
we get and complain if it is out of our range. But the range
we compare to is that of a long, which we then cast to an
int in the function's return value. This means that on
systems where int and long have different sizes (e.g.,
LP64 systems), we may pass the range check, but then return
nonsense by truncating the value as we cast it to an int.
We can solve this by converting git_parse_long into
git_parse_int, and range-checking the int range. Nobody
actually cared that we used a long internally, since the
result was truncated anyway. And the only other caller of
git_parse_long is git_config_maybe_bool, which should be
fine to just use int (though we will now forbid out-of-range
nonsense like setting merge.ff to 10g to mean true,
which is probably a good thing).
Signed-off-by: Jeff King p...@peff.net
---
I tested this with:
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index b1a6365..5444952 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -25,4 +25,8 @@ test_expect_success 'gc -h with invalid
configuration' '
test_i18ngrep [Uu]sage broken/usage
'
+test_expect_success 'gc complains about out-of-range integers' '
+ test_must_fail git -c gc.auto=3g gc --auto
+'
+
test_done
but that test is horrible for a few reasons:
1. It depends on knowing that gc.auto uses git_config_int internally.
2. It passes without the patch on 32-bit platforms.
3. It won't pass with the patch on ILP64 systems.
We can't use git config --int to test it, because it's going to stop
using git_config_int later in the series. So I think we should just go
without a test.
config.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/config.c b/config.c
index ee7c1f8..0a65ac2 100644
--- a/config.c
+++ b/config.c
@@ -515,10 +515,10 @@ static int git_parse_long(const char *value, long *ret)
return 0;
}
-static int git_parse_long(const char *value, long *ret)
+static int git_parse_int(const char *value, int *ret)
{
intmax_t tmp;
- if (!git_parse_signed(value, tmp, maximum_signed_value_of_type(long)))
+ if (!git_parse_signed(value, tmp, maximum_signed_value_of_type(int)))
return 0;
*ret = tmp;
return 1;
@@ -542,8 +542,8 @@ int git_config_int(const char *name, const char *value)
int git_config_int(const char *name, const char *value)
{
- long ret = 0;
- if (!git_parse_long(value, ret))
+ int ret;
+ if (!git_parse_int(value, ret))
die_bad_config(name);
return ret;
}
@@ -575,10 +575,10 @@ int git_config_maybe_bool(const char *name, const char
*value)
int git_config_maybe_bool(const char *name, const char *value)
{
- long v = git_config_maybe_bool_text(name, value);
+ int v = git_config_maybe_bool_text(name, value);
if (0 = v)
return v;
- if (git_parse_long(value, v))
+ if (git_parse_int(value, v))
return !!v;
return -1;
}
--
1.8.4.2.g87d4a77
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/5] config: set errno in numeric git_parse_* functions
When we are parsing an integer or unsigned long, we use
the strto*max functions, which properly set errno to ERANGE
if we get a large value. However, we also do further range
checks after applying our multiplication factor, but do not
set ERANGE. This means that a caller cannot tell if an error
was caused by ERANGE or if the input was simply not a valid
number.
This patch teaches git_parse_signed and git_parse_unsigned
reliably set ERANGE for range errors, and EINVAL for other
errors.
Signed-off-by: Jeff King p...@peff.net
---
I'm a little iffy on using errno like this. The normal way in git
would be to return an enum like:
enum config_parse_error {
CONFIG_PARSE_SUCCESS = 0,
CONFIG_PARSE_RANGE = -1,
CONFIG_PARSE_INVALID_UNIT = -2
};
But that would be changing the return semantics of git_parse_ulong
(which currently is 0 for fail, 1 for success) without changing its
signature. I figured this was the path of least resistance since we
already get ERANGE out of strtoimax, but I'm happy to switch it, too.
config.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/config.c b/config.c
index 0a65ac2..7332b06 100644
--- a/config.c
+++ b/config.c
@@ -480,16 +480,21 @@ static int git_parse_signed(const char *value, intmax_t
*ret, intmax_t max)
val = strtoimax(value, end, 0);
if (errno == ERANGE)
return 0;
- if (!parse_unit_factor(end, factor))
+ if (!parse_unit_factor(end, factor)) {
+ errno = EINVAL;
return 0;
+ }
uval = abs(val);
uval *= factor;
- if (uval max || abs(val) uval)
+ if (uval max || abs(val) uval) {
+ errno = ERANGE;
return 0;
+ }
val *= factor;
*ret = val;
return 1;
}
+ errno = EINVAL;
return 0;
}
@@ -505,13 +510,18 @@ int git_parse_unsigned(const char *value, uintmax_t *ret,
uintmax_t max)
if (errno == ERANGE)
return 0;
oldval = val;
- if (!parse_unit_factor(end, val))
+ if (!parse_unit_factor(end, val)) {
+ errno = EINVAL;
return 0;
- if (val max || oldval val)
+ }
+ if (val max || oldval val) {
+ errno = ERANGE;
return 0;
+ }
*ret = val;
return 1;
}
+ errno = EINVAL;
return 0;
}
--
1.8.4.2.g87d4a77
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/5] config: make numeric parsing errors more clear
If we try to parse an integer config argument and get a
number outside of the representable range, we die with the
cryptic message: bad config value for '%s'.
We can improve two things:
1. Show the value that produced the error (e.g., bad
config value '3g' for 'foo.bar').
2. Mention the reason the value was rejected (e.g.,
invalid unit versus out of range).
A few tests need to be updated with the new output, but that
should not be representative of real-world breakage, as
scripts should not be depending on the exact text of our
stderr output, which is subject to i18n anyway.
Signed-off-by: Jeff King p...@peff.net
---
I was tempted to clean up the tortured string construction in
die_bad_number(), but my understanding is that translators prefer to see
the string in as whole a piece as we can get it, rather than building it
piecemeal from ifs. And though this isn't marked for translation, it
probably should be.
config.c| 17 -
t/t1300-repo-config.sh | 6 +++---
t/t4055-diff-context.sh | 2 +-
3 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/config.c b/config.c
index 7332b06..87f87ba 100644
--- a/config.c
+++ b/config.c
@@ -543,18 +543,25 @@ int git_config_int(const char *name, const char *value)
return 1;
}
-static void die_bad_config(const char *name)
+static void die_bad_number(const char *name, const char *value)
{
+ const char *reason = errno == ERANGE ?
+out of range :
+invalid unit;
+ if (!value)
+ value = ;
+
if (cf cf-name)
- die(bad config value for '%s' in %s, name, cf-name);
- die(bad config value for '%s', name);
+ die(bad numeric config value '%s' for '%s' in %s: %s,
+ value, name, cf-name, reason);
+ die(bad numeric config value '%s' for '%s': %s, value, name, reason);
}
int git_config_int(const char *name, const char *value)
{
int ret;
if (!git_parse_int(value, ret))
- die_bad_config(name);
+ die_bad_number(name, value);
return ret;
}
@@ -562,7 +569,7 @@ unsigned long git_config_ulong(const char *name, const char
*value)
{
unsigned long ret;
if (!git_parse_ulong(value, ret))
- die_bad_config(name);
+ die_bad_number(name, value);
return ret;
}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index c4a7d84..20aee6e 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -657,11 +657,11 @@ test_expect_success 'invalid unit' '
echo 1auto expect
git config aninvalid.unit actual
test_cmp expect actual
- cat expect -\EOF
- fatal: bad config value for '\''aninvalid.unit'\'' in .git/config
+ cat expect -\EOF
+ fatal: bad numeric config value '\''1auto'\'' for
'\''aninvalid.unit'\'' in .git/config: invalid unit
EOF
test_must_fail git config --int --get aninvalid.unit 2actual
- test_cmp actual expect
+ test_i18ncmp expect actual
'
cat expect EOF
diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh
index 97172b4..cd04543 100755
--- a/t/t4055-diff-context.sh
+++ b/t/t4055-diff-context.sh
@@ -73,7 +73,7 @@ test_expect_success 'non-integer config parsing' '
test_expect_success 'non-integer config parsing' '
git config diff.context no
test_must_fail git diff 2output
- test_i18ngrep bad config value output
+ test_i18ngrep bad numeric config value output
'
test_expect_success 'negative integer config parsing' '
--
1.8.4.2.g87d4a77
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/5] git-config: always treat --int as 64-bit internally
When you run git config --int, the maximum size of integer
you get depends on how git was compiled, and what it
considers to be an int.
This is almost useful, because your scripts calling git
config will behave similarly to git internally. But relying
on this is dubious; you have to actually know how git treats
each value internally (e.g., int versus unsigned long),
which is not documented and is subject to change. And even
if you know it is unsigned long, we do not have a
git-config option to match that behavior.
Furthermore, you may simply be asking git to store a value
on your behalf (e.g., configuration for a hook). In that
case, the relevant range check has nothing at all to do with
git, but rather with whatever scripting tools you are using
(and git has no way of knowing what the appropriate range is
there).
Not only is the range check useless, but it is actively
harmful, as there is no way at all for scripts to look
at config variables with large values. For instance, one
cannot reliably get the value of pack.packSizeLimit via
git-config. On an LP64 system, git happily uses a 64-bit
unsigned long internally to represent the value, but the
script cannot read any value over 2G.
Ideally, the --int option would simply represent an
arbitrarily large integer. For practical purposes, however,
a 64-bit integer is large enough, and is much easier to
implement (and if somebody overflows it, we will still
notice the problem, and not simply return garbage).
Signed-off-by: Jeff King p...@peff.net
---
I kind of picked 64-bit as big enough. But I suppose we could also go
with intmax_t. The only thing we can't do is an unsigned value.
Also, this is the first use of PRId64. I notice we have compatibility
macros for PRIu*, but I'm not sure what to put in one for PRId64.
builtin/config.c | 7 ---
cache.h| 1 +
config.c | 17 +
t/t1300-repo-config.sh | 7 +++
4 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 4010c43..8b182d2 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -128,7 +128,8 @@ static int collect_config(const char *key_, const char
*value_, void *cb)
must_print_delim = 1;
}
if (types == TYPE_INT)
- sprintf(value, %d, git_config_int(key_, value_?value_:));
+ sprintf(value, %PRId64,
+ git_config_int64(key_, value_ ? value_ : ));
else if (types == TYPE_BOOL)
vptr = git_config_bool(key_, value_) ? true : false;
else if (types == TYPE_BOOL_OR_INT) {
@@ -265,8 +266,8 @@ static char *normalize_value(const char *key, const char
*value)
else {
normalized = xmalloc(64);
if (types == TYPE_INT) {
- int v = git_config_int(key, value);
- sprintf(normalized, %d, v);
+ int64_t v = git_config_int64(key, value);
+ sprintf(normalized, %PRId64, v);
}
else if (types == TYPE_BOOL)
sprintf(normalized, %s,
diff --git a/cache.h b/cache.h
index 85b544f..ac4525a 100644
--- a/cache.h
+++ b/cache.h
@@ -1190,6 +1190,7 @@ extern int git_config_int(const char *, const char *);
extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
extern int git_parse_ulong(const char *, unsigned long *);
extern int git_config_int(const char *, const char *);
+extern int64_t git_config_int64(const char *, const char *);
extern unsigned long git_config_ulong(const char *, const char *);
extern int git_config_bool_or_int(const char *, const char *, int *);
extern int git_config_bool(const char *, const char *);
diff --git a/config.c b/config.c
index 87f87ba..6588cf5 100644
--- a/config.c
+++ b/config.c
@@ -534,6 +534,15 @@ static int git_parse_int(const char *value, int *ret)
return 1;
}
+static int git_parse_int64(const char *value, int64_t *ret)
+{
+ intmax_t tmp;
+ if (!git_parse_signed(value, tmp,
maximum_signed_value_of_type(int64_t)))
+ return 0;
+ *ret = tmp;
+ return 1;
+}
+
int git_parse_ulong(const char *value, unsigned long *ret)
{
uintmax_t tmp;
@@ -565,6 +574,14 @@ int git_config_int(const char *name, const char *value)
return ret;
}
+int64_t git_config_int64(const char *name, const char *value)
+{
+ int64_t ret;
+ if (!git_parse_int64(value, ret))
+ die_bad_number(name, value);
+ return ret;
+}
+
unsigned long git_config_ulong(const char *name, const char *value)
{
unsigned long ret;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 20aee6e..b66c632 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -652,6 +652,13 @@ test_expect_success numbers '
test_cmp expect actual
'
+test_expect_success '--int is at least 64 bits' '
+ git
Re: [PATCH 0/3] Reject non-ff pulls by default
From: Felipe Contreras felipe.contre...@gmail.com
To: Philip Oakley philipoak...@iee.org
Cc: John Keeping j...@keeping.me.uk; Junio C Hamano
gits...@pobox.com; git@vger.kernel.org; Andreas Krey
a.k...@gmx.de
Sent: Sunday, September 08, 2013 9:16 AM
Subject: Re: [PATCH 0/3] Reject non-ff pulls by default
On Sun, Sep 8, 2013 at 3:01 AM, Philip Oakley philipoak...@iee.org
wrote:
From: Felipe Contreras felipe.contre...@gmail.com
Sent: Sunday, September 08, 2013 3:34 AM
On Thu, Sep 5, 2013 at 3:06 AM, John Keeping j...@keeping.me.uk
wrote:
On Wed, Sep 04, 2013 at 03:59:18PM -0700, Junio C Hamano wrote:
Are there cases where you do not want to either rebase nor merge?
If so what do you want to do after git pull fetches from the
other
side? Nothing?
One other thing that I can see being useful occasionally is:
git rebase @{u}@{1} --onto @{u}
which allows local commits to be replayed onto a rewritten upstream
branch.
Although I agree with your side note below that people doing this
may be
better off fetching and then updating their local branch,
particularly
if @{1} is not the correct reflog entry for the upstream when they
created the branch.
That's why after recognizing the fact the you can't find the branch
point of a branch in Git, I decided to write patches to support the
@{tail} shorthand, which is basically the point where the branch was
created, or rebased to:
https://github.com/felipec/git/commits/fc/base
And if 'git rebase' was fixed to ignore the commits already in the
rebased onto branch, almost always what you would want to do is 'git
rebase @{tail} --onto @{upstream}'.
The use case that trips me up (i.e. doesn't fit the above) is when I
have a
branch that may need rebasing on (onto) pu, or may need rebasing on
master,
or next, depending on what others have been doing.
Yes, so you would do:
% git rebase --onto pu
Which would be translated to:
% git rebase @{tail} --onto pu
What's the problem?
The 'problem' is (would be) that I don't yet know that I would need
the --onto pu until I discover (how?) that the default rebase would
result in conflicts.
As a Distributed VCS (i.e. others doing work independently), a rebase
always
has the possibility that the world has moved on and one has to adapt
to the
new world order by moving location (--onto somewhere new), not just
fixing
up the house (patch conflicts). When the update order is unknown
there is no
guaranteed solution (IIUC).
Yeah, but almost always you want to rebase onto @{upstream}.
Yeah, but almost always you want to check first *before* starting.
That is, 'git rebase --abort' should not be required from the (user's
selected /git's) default invocation.
--
Philip Oakley
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
On Sun, Sep 8, 2013 at 2:50 AM, Jeff King p...@peff.net wrote: On Sun, Sep 08, 2013 at 02:15:17AM -0500, Felipe Contreras wrote: I think the guy may be git itself. For example, here is a possible session with jc/pull-training-wheel: $ git push Who told him to use 'git push'? Certainly not git. Any of the hundreds of existing tutorials that teach basic git commands like push? You can't use a tutorial out there that just tells you to replace svn commands with git alternatives, go to work and mess up the repository history. I'm trying to take the point of view of your hypothetical user working on a repository where history is not important, but it seems more and more than this person is just not real. If it's OK to push crappy merges, somebody must have told him that was OK and provided him with the commands. If it's just some random person that read some random tutorial from 'svn' - 'git' working on a random repository that happens to accept merges all over the place. Well I think that's a very very exceptional situation. And this person still wouldn't have a problem finding another tutorial explaining what a merge is. To ... ! [rejected]master - master (non-fast-forward) error: failed to push some refs to '...' hint: Updates were rejected because the tip of your current branch is behind hint: its remote counterpart. Integrate the remote changes (e.g. hint: 'git pull ...') before pushing again. hint: See the 'Note about fast-forwards' in 'git push --help' for details. [...] Why stop there? Post the whole man page already. Moreover, it's overly verbose on all the wrong and irrelevant information. If you are going to waste precious screen state, explain wth a non fast-forward is; people can figure out what a merge is, and maybe a rebase, but a non fast-forward definitely not. Note that I was not trying to defend any of the messages, but only showing a plausible mechanism by which a user with basic knowledge that he wants to push may arrive at the question what is the difference between merge and rebase?. Yes, and this person would have to read it online, like everything else, because clearly Git documentation would do a bad job at it. That's why the online documentation was needed in the first place. The first hits of 'git merge vs rebase' are rather useful: http://mislav.uniqpath.com/2013/02/merge-vs-rebase/ http://stackoverflow.com/questions/16336014/git-merge-vs-rebase http://www.derekgourlay.com/archives/428 http://blog.sourcetreeapp.com/2012/08/21/merge-or-rebase/ http://git-scm.com/book/en/Git-Branching-Rebasing Notice how none of the results point to official documentation, precisely because it's not really useful. If you want to suggest revisions for the push message, go ahead. The push advice _is_ an attempt to define non-fast-forwards in plain language without taking up too much space, but perhaps you can do better. I definitely can, but you would disagree. But anyway, you are relying on the user having pushed first, what if he is pulling first, or what if he doesn't have write access and is only pulling? You could even suggest omitting it entirely, but I'm not sure if that is a good idea. It was not added in a vacuum; we lacked that advice for many years, and people complained about it quite a bit until it was added. I would have to see the evidence, as I have never seen any complaints about that. The complains are about the UI, and they still remain. The user is pointed at pull from push, and then gets presented with the merge or rebase choice. It may be that the advice you can find by googling merge vs rebase is enough to then help the person along (and/or we may need to improve the manpages in that respect). Yes, but that's not the use-case we are talking about. You mentioned specifically a svn-like worfklow where the guy was told by somebody else to replace the svn commands with git ones. No, I mentioned an svn-like workflow. I didn't say anything about how they were told. They might have been told by a co-worker, or read a brief tutorial on git, or read something like Git-SVN Crash Course. Once again, this doesn't make any sense. People can't just push crap merges to any repository. If we are talking about a guy that is learning git, that's and entirely different case. That is certainly what I meant to be talking about. If he is learning Git, then he will be looking for the meaning of a merge and a rebase. The fact that the repository accepts crappy merges wouldn't be relevant. The purpose of this change in the code is not to change the user behavior. The choice of merge vs. rebase is entirely up to the user, and we are not changing that. Right, but by not doing anything by default, you are forcing the user to make a decision. No, it would be a warning first, he wouldn't be *forced* to make a decision, only after the deprecation period is over. Then yes, if by then he
Re: [PATCH 0/3] Reject non-ff pulls by default
On Sun, Sep 8, 2013 at 3:42 AM, Philip Oakley philipoak...@iee.org wrote:
The 'problem' is (would be) that I don't yet know that I would need the
--onto pu until I discover (how?) that the default rebase would result in
conflicts.
I don't see what that has to do with an invocation of 'git rebase'
without arguments, and @{tail}. There's absolutely no way Git can
figure out for you which is the appropriate place for you to rebase
onto.
However, it shouldn't be too difficult to write a tool that checks
multiple commits and tells you on top of which ones a rebase could
work, but I don't think 'git rebase' is the right place.
--
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] upload-pack keepalive
I've gotten complaints that cloning from github.com with -q will sometimes abort before sending any data. The problem is that during a quiet clone, upload-pack may be silent for a long time while preparing the pack (i.e., the counting objects and compressing phases). We have load balancers and reverse proxies sitting in front of the git machines, and these machines may sometimes think the connection has hung and drop it, even though if they had waited a few more seconds, the pack would have started coming. We mitigated it somewhat by bumping the timeouts on our side, but that's only one piece of the puzzle. Clients may be going through http proxies or stateful firewalls on their end, and neither we nor they have any control. This series teaches upload-pack to periodically send an empty sideband data packet when pack-objects is being quiet. That keeps a small amount of data flowing, and should be backwards compatible. I hand-tested it against JGit, dulwich (via the mercurial git plugin), libgit2, and old versions of git, and all worked fine. It has also been running on github.com for about a week and a half, and nobody has reported any problems. [1/2]: upload-pack: send keepalive packets during pack computation [2/2]: upload-pack: bump keepalive default to 5 seconds -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] upload-pack: bump keepalive default to 5 seconds
From: Jeff King p...@peff.net There is no reason not to turn on keepalives by default. They take very little bandwidth, and significantly less than the progress reporting they are replacing. And in the case that progress reporting is on, we should never need to send a keepalive anyway, as we will constantly be showing progress and resetting the keepalive timer. We do not necessarily know what the client's idea of a reasonable timeout is, so let's keep this on the low side of 5 seconds. That is high enough that we will always prefer our normal 1-second progress reports to sending a keepalive packet, but low enough that no sane client should consider the connection hung. Signed-off-by: Jeff King p...@peff.net --- Documentation/config.txt | 2 +- upload-pack.c| 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 5d748f7..6b35578 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2225,7 +2225,7 @@ uploadpack.keepalive:: the server to be hung and give up. Setting this option instructs `upload-pack` to send an empty keepalive packet every `uploadpack.keepalive` seconds. Setting this option to 0 - disables keepalive packets entirely. The default is 0. + disables keepalive packets entirely. The default is 5 seconds. url.base.insteadOf:: Any URL that starts with this value will be rewritten to diff --git a/upload-pack.c b/upload-pack.c index c3e4a20..04a8707 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -40,7 +40,7 @@ static unsigned int timeout; static struct object_array want_obj; static struct object_array extra_edge_obj; static unsigned int timeout; -static int keepalive = -1; +static int keepalive = 5; /* 0 for no sideband, * otherwise maximum packet size (up to 65520 bytes). */ -- 1.8.4.2.g87d4a77 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] upload-pack: send keepalive packets during pack computation
Whenn upload-pack has started pack-objects, there may be a
quiet period while pack-objects prepares the pack (i.e.,
counting objects and delta compression). Normally we would
see (and send to the client) progress information, but if
--quiet is in effect, pack-objects will produce nothing at
all until the pack data is ready. On a large repository,
this can take tens of seconds (or even minutes if the system
is loaded or the repository is badly packed). Clients or
intermediate proxies can sometimes give up in this
situation, assuming that the server or connection has hung.
This patch introduces a keepalive option; if upload-pack
sees no data from pack-objects for a certain number of
seconds, it will send an empty sideband data packet to let
the other side know that we are still working on it.
Signed-off-by: Jeff King p...@peff.net
---
Documentation/config.txt | 11 +++
upload-pack.c| 25 -
2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ec57a15..5d748f7 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2216,6 +2216,17 @@ uploadpack.allowtipsha1inwant::
of a hidden ref (by default, such a request is rejected).
see also `uploadpack.hiderefs`.
+uploadpack.keepalive::
+ When `upload-pack` has started `pack-objects`, there may be a
+ quiet period while `pack-objects` prepares the pack. Normally
+ it would output progress information, but if `--quiet` was used
+ for the fetch, `pack-objects` will output nothing at all until
+ the pack data begins. Some clients and networks may consider
+ the server to be hung and give up. Setting this option instructs
+ `upload-pack` to send an empty keepalive packet every
+ `uploadpack.keepalive` seconds. Setting this option to 0
+ disables keepalive packets entirely. The default is 0.
+
url.base.insteadOf::
Any URL that starts with this value will be rewritten to
start, instead, with base. In cases where some site serves a
diff --git a/upload-pack.c b/upload-pack.c
index 127e59a..c3e4a20 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -40,6 +40,7 @@ static unsigned int timeout;
static struct object_array want_obj;
static struct object_array extra_edge_obj;
static unsigned int timeout;
+static int keepalive = -1;
/* 0 for no sideband,
* otherwise maximum packet size (up to 65520 bytes).
*/
@@ -200,6 +201,7 @@ static void create_pack_file(void)
while (1) {
struct pollfd pfd[2];
int pe, pu, pollsize;
+ int ret;
reset_timeout();
@@ -222,7 +224,8 @@ static void create_pack_file(void)
if (!pollsize)
break;
- if (poll(pfd, pollsize, -1) 0) {
+ ret = poll(pfd, pollsize, 1000 * keepalive);
+ if (ret 0) {
if (errno != EINTR) {
error(poll failed, resuming: %s,
strerror(errno));
@@ -284,6 +287,21 @@ static void create_pack_file(void)
if (sz 0)
goto fail;
}
+
+ /*
+* We hit the keepalive timeout without saying anything; send
+* an empty message on the data sideband just to let the other
+* side know we're still working on it, but don't have any data
+* yet.
+*
+* If we don't have a sideband channel, there's no room in the
+* protocol to say anything, so those clients are just out of
+* luck.
+*/
+ if (!ret use_sideband) {
+ static const char buf[] = 0005\1;
+ write_or_die(1, buf, 5);
+ }
}
if (finish_command(pack_objects)) {
@@ -785,6 +803,11 @@ static int upload_pack_config(const char *var, const char
*value, void *unused)
{
if (!strcmp(uploadpack.allowtipsha1inwant, var))
allow_tip_sha1_in_want = git_config_bool(var, value);
+ else if (!strcmp(uploadpack.keepalive, var)) {
+ keepalive = git_config_int(var, value);
+ if (!keepalive)
+ keepalive = -1;
+ }
return parse_hide_refs_config(var, value, uploadpack);
}
--
1.8.4.2.g87d4a77
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Officially start moving to the term 'staging area'
On Sun, Sep 8, 2013 at 2:49 AM, Philip Oakley philipoak...@iee.org wrote: From: Felipe Contreras felipe.contre...@gmail.com Sent: Sunday, September 08, 2013 2:33 AM [snip...] The snapshot concept is totally orthogonal from the staging area concept. Git works in snapshots, which are frozen images of how the content tree was at a certain point in time; IOW; a commit. (I feel that) In most peoples minds the need for a staging area, and the use of snapshots, are related. Part of that relationship, often not noticed by those folks, is that they are 'orthogonal' to *each other*. Thus orthogonality means both un-related, and related at the same time (aren't we humans peculiar!). They are cleaved to each other. Not really. You can argue that a movie is a sequence of snapshots, 24 of them per second, but most people don't think of it that way. You can also argue that every change you do on a file is a snapshot, but again, people don't think of it that way. Yes, you can argue that the index is a snapshot, but it doesn't help to think of it that way. And if you try to argue that way, then every waking moment is a snapshot, what is NOT a snapshot? The useful concept of snapshot is a moment in time stored for posterity, in that sense a photo is a snapshot, a backup is a snapshot, and a commit is a snapshot, but the staging area is not, because it will be gone. In fact, I just thought of another concept; a draft. When trying to explain staging/index I tend to use the analogy of an old style office (I am almost that old) where one has an In tray and an Out tray on one's desk (and one parked WIP for lunch time desk tidy), and the staging area is the basket at the end marked 'For Filing'. When the 'For Filing' basket is ready, one called the filing clerk to dictate the cover note and away it went, commited to some remote filing repository. Oh how things have changed ;-) But that doesn't work well. You didn't add and remove things for the 'For Filing' as you worked, did you? Even if it did work, I don't think it would be particularly useful to most people today. I think a much more fitting analogy is a mail draft; you add and remove things, change them, review, you can take as much time as you want, and at the end of the day you can discard the whole thing. Nothing gets done until you press 'send', which is analogous to 'commit'. _How_ that snapshot is created is an entirely different topic, and the staging area is a tool to create the desired snapshots. The user might decide to never use that tool (i.e. always run git commit -a), but the concept of snapshots remain. So, clearly, one concept has absolutely nothing to do with the other. The point would be that we allow a particular snapshot to be selected, and that the git commit -a is but one (common) method. Commit -a is like jumping in the car for a quick trip to the shops, while the selective staging of content is like packing for a holiday. I still don't see it. When you do 'git commit' you don't have an array of snapshots to choose from, you just have one, and it's not particularly static, as you can add and remove things from it, so it's not really a snapshot of your working directory even, because you can add things that where never there. And then if the staging area is a snapshot, then what is a commit? Another snapshot? Then what is the difference between the two? One is permanent and the other temporary? Well, then saying snapshot wouldn't be enough to describe the staging area, you would need to say preliminary snapshot, which doesn't really make sense, as those are called previews, not snapshots. But why bother with snapshot, we can use preliminary commit. But again, it sounds weird using the word commit to something that can and is meant to change, unlike git commits, and incidentally, unlike snapshots. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
From: Felipe Contreras felipe.contre...@gmail.com
Sent: Sunday, September 08, 2013 9:49 AM
On Sun, Sep 8, 2013 at 3:42 AM, Philip Oakley philipoak...@iee.org
wrote:
The 'problem' is (would be) that I don't yet know that I would need
the
--onto pu until I discover (how?) that the default rebase would
result in
conflicts.
I don't see what that has to do with an invocation of 'git rebase'
without arguments, and @{tail}.
There's absolutely no way Git can
figure out for you which is the appropriate place for you to rebase
onto.
.. which was my point. I may not have explained it that well.
Given that Git can't figure it out, we should stop trying in such cases.
However, it shouldn't be too difficult to write a tool that checks
multiple commits and tells you on top of which ones a rebase could
work, but I don't think 'git rebase' is the right place.
That's an SOS approach (Success Oriented Script)[1] that presumes the
user is already better than they are - The Kruger Dunning paper [2]
offers some insight into capability misconceptions (at all levels).
--
regards
Philip
--
[1] in the original it was a Success Oriented Schedule - one of those
plans that hopeful managers put together on late running projects that
amount to wishful thinking that hopefully garners them enough time to
make a little progress and update their 'success stories'.
[2] http://dx.doi.org/10.%2F1467-8721.01235 Why People Fail to
Recognize Their Own Incompetence. Though the corollaries (People fail
to recognise their own skills, and hence they/we mishandle our
communications) are just as (IMHO more) important.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
On Sun, Sep 08, 2013 at 02:54:20AM -0400, Jeff King wrote:
I am genuinely curious what people in favor of this feature would want
to say in the documentation to a user encountering this choice for the
first time. In my experience, rebasing introduces more complications,
specifically:
1. the merge is backwards with respect to ours/theirs
2. you may end up with difficult conflict resolution due to repeated
changes over the same section of code. E.g., you write some buggy
code and then fix it, but upstream has changed the same area.
Rebasing involves first resolving your buggy version with the
upstream code, and then resolving the fix on top of the previous
resolution.
3. rewriting of commits found in other branches, which then need
rebased on top of the branch you just rebased
4. a previously bug-free commit can show a bug after the rebase if
other parts of the project changed (whereas with a merge, the bug
would be attributable to the merge)
I know those are all balanced by some advantages of rebasing, but I also
think they are things that can be troublesome for a user who does not
fully grok the rebase process. I'm just wondering if we should mention
both, but steer people towards merging as the safer alternative (you
might have ugly history, but you are less likely to create a mess with
duplicate commits or badly-resolved conflicts).
The really correct thing to do here is to encourage a feature branch
workflow, but in my experience people are happier to walk through a
rebase than to switch over to feature branches completely.
An alternative pull mode would be:
git reset --keep @{u}
git merge @{-1}
which gets a sensible history shape without any of your disadvantages
above. But that didn't go anywhere last time it came up [1] [2].
[1] http://article.gmane.org/gmane.comp.version-control.git/210246
[2] http://article.gmane.org/gmane.comp.version-control.git/210625
Fortunately there probably are very few of these users.
Maybe. I am not sure how one would measure.
If you are interested, I can ask the opinion of some of the GitHub
trainers. They see a lot of new users and have a sense of what kinds of
confusion come up most frequently, what kinds of workflows they tend to
see, etc. Their experience may be biased towards corporate-ish users,
though, because those are the people who pay for training.
I expect corporate environments are the ones in which this is relevant.
Open source projects that care about the shape of history can have one
person able to write to the central repository who can enforce the
policy they want. This tends to be more difficult in a corporate
environment, particularly one that was previously using a centralised
VCS.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] Teach git to change to a given directory using -C option
On Wed, Sep 4, 2013 at 8:20 AM, Nazri Ramliy ayieh...@gmail.com wrote: Subject: git: run in a directory given with -C option This is similar in spirit to to make -C dir ... and tar -C dir --- diff --git a/Documentation/git.txt b/Documentation/git.txt index 83edf30..ae049da 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -395,6 +395,20 @@ displayed. See linkgit:git-help[1] for more information, because `git --help ...` is converted internally into `git help ...`. +-C path:: + Run as if git was started in path instead of the current working + directory. When multiple -C options are given, each subsequent + non-absolute -C path is interpreted relative to the preceding -C + path. For consistency with existing formatting in git.txt, you may want to squash in the following fixes (sans gmail whitespace damage): --- 8 --- diff --git a/Documentation/git.txt b/Documentation/git.txt index ae049da..6622037 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -396,14 +396,14 @@ because `git --help ...` is converted internally into `git help ...`. -C path:: - Run as if git was started in path instead of the current working - directory. When multiple -C options are given, each subsequent - non-absolute -C path is interpreted relative to the preceding -C - path. + Run as if git was started in 'path' instead of the current working + directory. When multiple '-C' options are given, each subsequent + non-absolute `-C path` is interpreted relative to the preceding `-C + path`. + -This option affects options that expect path name like --git-dir and ---work-tree in that their interpretations of the path names would be -made relative to the working directory caused by the -C option. For +This option affects options that expect path name like '--git-dir' and +'--work-tree' in that their interpretations of the path names would be +made relative to the working directory caused by the '-C' option. For example the following invocations are equivalent: git --git-dir=a.git --work-tree=b -C c status --- 8 --- +This option affects options that expect path name like --git-dir and +--work-tree in that their interpretations of the path names would be +made relative to the working directory caused by the -C option. For +example the following invocations are equivalent: + +git --git-dir=a.git --work-tree=b -C c status +git --git-dir=c/a.git --work-tree=c/b status Is the interaction of -C with --work-tree and --git-dir desirable or useful? (I'm genuinely curious.) Do you have use-cases in mind? Would mentioning them in the commit message help to justify the interaction? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
From: Philip Oakley philipoak...@iee.org [2] http://dx.doi.org/10.%2F1467-8721.01235 Why People Fail to Recognize Their Own Incompetence. Oops, That's behind a paywall, and a more recent variant. Though the corollaries (People fail to recognise their own skills, and hence they/we mishandle our communications) are just as (IMHO more) important. I believe this is the on-line version of the original 1999 paper http://mastercodeprofessional.com/library_files/Kruger-Dunning---Unskilled_and_Unaware_of_It_(2009).pdf The section 5.1. The Burden of Expertise discusses my point above. they [Experts] fail to realize that their proficiency is not necessarily shared by their peers. Philip -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Doc: 'replace' merge and non-merge commits
Merges are often treated as special case objects so tell users that they are not special here. Signed-off-by: Philip Oakley philipoak...@iee.org --- This updates my in-line patch given in [PATCH v3 07/11] Documentation/replace: tell that -f option bypasses the type check ($gmane/233997 05 September 2013 23:20) and Junio's comments ($gmane/234001 06 September 2013 00:13) Applies on top of pu and Christian Couder's series. Philip --- Documentation/git-replace.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt index 414000e..f373ab4 100644 --- a/Documentation/git-replace.txt +++ b/Documentation/git-replace.txt @@ -26,6 +26,7 @@ This restriction can be bypassed using `-f`. Unless `-f` is given, the 'replace' reference must not yet exist. There is no other restriction on the replaced and replacement objects. +Merge commits can be replaced by non-merge commits and vice versa. Replacement references will be used by default by all Git commands except those doing reachability traversal (prune, pack transfer and -- 1.8.1.msysgit.1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bash completion doing full tree traversal?
I experienced a weird stall from git bash completion: $ git add pTAB I did some investigation and found this call trace: + __git_index_files '--others --modified' '' --- git ls-files --exclude-standard --others --modified This bash function captures output of the git call and strips all but the base directory or base filename. The results are limited to entries in the current directory. Appears to be from fea16b47b603e and first released in v1.8.2 (I have 1.8.4). Isaac -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 04/11] pack v4: add version argument to write_pack_header
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/pack-objects.c | 2 +-
bulk-checkin.c | 2 +-
pack-write.c | 7 +--
pack.h | 3 +--
4 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f069462..33faea8 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -735,7 +735,7 @@ static void write_pack_file(void)
else
f = create_tmp_packfile(pack_tmp_name);
- offset = write_pack_header(f, nr_remaining);
+ offset = write_pack_header(f, 2, nr_remaining);
if (!offset)
die_errno(unable to write pack header);
nr_written = 0;
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 6b0b6d4..9d8f0d0 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -176,7 +176,7 @@ static void prepare_to_stream(struct bulk_checkin_state
*state,
reset_pack_idx_option(state-pack_idx_opts);
/* Pretend we are going to write only one object */
- state-offset = write_pack_header(state-f, 1);
+ state-offset = write_pack_header(state-f, 2, 1);
if (!state-offset)
die_errno(unable to write pack header);
}
diff --git a/pack-write.c b/pack-write.c
index 631007e..88e4788 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -186,12 +186,15 @@ const char *write_idx_file(const char *index_name, struct
pack_idx_entry **objec
return index_name;
}
-off_t write_pack_header(struct sha1file *f, uint32_t nr_entries)
+off_t write_pack_header(struct sha1file *f,
+ int version, uint32_t nr_entries)
{
struct pack_header hdr;
hdr.hdr_signature = htonl(PACK_SIGNATURE);
- hdr.hdr_version = htonl(PACK_VERSION);
+ hdr.hdr_version = htonl(version);
+ if (!pack_version_ok(hdr.hdr_version))
+ die(_(pack version %d is not supported), version);
hdr.hdr_entries = htonl(nr_entries);
if (sha1write(f, hdr, sizeof(hdr)))
return 0;
diff --git a/pack.h b/pack.h
index aa6ee7d..855f6c6 100644
--- a/pack.h
+++ b/pack.h
@@ -8,7 +8,6 @@
* Packed object header
*/
#define PACK_SIGNATURE 0x5041434b /* PACK */
-#define PACK_VERSION 2
#define pack_version_ok(v) ((v) == htonl(2) || (v) == htonl(3))
struct pack_header {
uint32_t hdr_signature;
@@ -80,7 +79,7 @@ extern const char *write_idx_file(const char *index_name,
struct pack_idx_entry
extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs,
off_t offset, off_t len, unsigned int nr);
extern int verify_pack_index(struct packed_git *);
extern int verify_pack(struct packed_git *, verify_fn fn, struct progress *,
uint32_t);
-extern off_t write_pack_header(struct sha1file *f, uint32_t);
+extern off_t write_pack_header(struct sha1file *f, int, uint32_t);
extern void fixup_pack_header_footer(int, unsigned char *, const char *,
uint32_t, unsigned char *, off_t);
extern char *index_pack_lockfile(int fd);
extern int encode_in_pack_object_header(enum object_type, uintmax_t, unsigned
char *);
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 05/11] pack-write.c: add pv4_encode_in_pack_object_header
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
pack-write.c | 29 +
pack.h | 1 +
2 files changed, 30 insertions(+)
diff --git a/pack-write.c b/pack-write.c
index 88e4788..6f11104 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -1,6 +1,7 @@
#include cache.h
#include pack.h
#include csum-file.h
+#include varint.h
void reset_pack_idx_option(struct pack_idx_option *opts)
{
@@ -340,6 +341,34 @@ int encode_in_pack_object_header(enum object_type type,
uintmax_t size, unsigned
return n;
}
+/*
+ * The per-object header is a pretty dense thing, which is
+ * - first byte: low four bits are size, then three bits of type,
+ *and the high bit is size continues.
+ * - each byte afterwards: low seven bits are size continuation,
+ *with the high bit being size continues
+ */
+int pv4_encode_in_pack_object_header(enum object_type type,
+uintmax_t size, unsigned char *hdr)
+{
+ uintmax_t val;
+ if (type OBJ_COMMIT || type OBJ_PV4_TREE || type == OBJ_OFS_DELTA)
+ die(bad type %d, type);
+
+ /*
+* We allocate 4 bits in the LSB for the object type which
+* should be good for quite a while, given that we effectively
+* encodes only 5 object types: commit, tree, blob, delta,
+* tag.
+*/
+ val = size;
+ if (MSB(val, 4))
+ die(fixme: the code doesn't currently cope with big sizes);
+ val = 4;
+ val |= type;
+ return encode_varint(val, hdr);
+}
+
struct sha1file *create_tmp_packfile(char **pack_tmp_name)
{
char tmpname[PATH_MAX];
diff --git a/pack.h b/pack.h
index 855f6c6..38f869d 100644
--- a/pack.h
+++ b/pack.h
@@ -83,6 +83,7 @@ extern off_t write_pack_header(struct sha1file *f, int,
uint32_t);
extern void fixup_pack_header_footer(int, unsigned char *, const char *,
uint32_t, unsigned char *, off_t);
extern char *index_pack_lockfile(int fd);
extern int encode_in_pack_object_header(enum object_type, uintmax_t, unsigned
char *);
+extern int pv4_encode_in_pack_object_header(enum object_type, uintmax_t,
unsigned char *);
#define PH_ERROR_EOF (-1)
#define PH_ERROR_PACK_SIGNATURE(-2)
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 07/11] list-objects.c: add show_tree_entry callback to traverse_commit_list
This helps construct tree dictionary in pack v4.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/pack-objects.c | 2 +-
builtin/rev-list.c | 4 ++--
list-objects.c | 9 -
list-objects.h | 3 ++-
upload-pack.c | 2 +-
5 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ef68fc5..b38d3dc 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2380,7 +2380,7 @@ static void get_object_list(int ac, const char **av)
if (prepare_revision_walk(revs))
die(revision walk setup failed);
mark_edges_uninteresting(revs.commits, revs, show_edge);
- traverse_commit_list(revs, show_commit, show_object, NULL);
+ traverse_commit_list(revs, show_commit, NULL, show_object, NULL);
if (keep_unreachable)
add_objects_in_unpacked_packs(revs);
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index a5ec30d..b25f896 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -243,7 +243,7 @@ static int show_bisect_vars(struct rev_list_info *info, int
reaches, int all)
strcpy(hex, sha1_to_hex(revs-commits-item-object.sha1));
if (flags BISECT_SHOW_ALL) {
- traverse_commit_list(revs, show_commit, show_object, info);
+ traverse_commit_list(revs, show_commit, NULL, show_object,
info);
printf(--\n);
}
@@ -348,7 +348,7 @@ int cmd_rev_list(int argc, const char **argv, const char
*prefix)
return show_bisect_vars(info, reaches, all);
}
- traverse_commit_list(revs, show_commit, show_object, info);
+ traverse_commit_list(revs, show_commit, NULL, show_object, info);
if (revs.count) {
if (revs.left_right revs.cherry_mark)
diff --git a/list-objects.c b/list-objects.c
index 3dd4a96..6def897 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -61,6 +61,7 @@ static void process_gitlink(struct rev_info *revs,
static void process_tree(struct rev_info *revs,
struct tree *tree,
+show_tree_entry_fn show_tree_entry,
show_object_fn show,
struct name_path *path,
struct strbuf *base,
@@ -107,9 +108,13 @@ static void process_tree(struct rev_info *revs,
continue;
}
+ if (show_tree_entry)
+ show_tree_entry(entry, cb_data);
+
if (S_ISDIR(entry.mode))
process_tree(revs,
lookup_tree(entry.sha1),
+show_tree_entry,
show, me, base, entry.path,
cb_data);
else if (S_ISGITLINK(entry.mode))
@@ -167,6 +172,7 @@ static void add_pending_tree(struct rev_info *revs, struct
tree *tree)
void traverse_commit_list(struct rev_info *revs,
show_commit_fn show_commit,
+ show_tree_entry_fn show_tree_entry,
show_object_fn show_object,
void *data)
{
@@ -196,7 +202,8 @@ void traverse_commit_list(struct rev_info *revs,
continue;
}
if (obj-type == OBJ_TREE) {
- process_tree(revs, (struct tree *)obj, show_object,
+ process_tree(revs, (struct tree *)obj,
+show_tree_entry, show_object,
NULL, base, name, data);
continue;
}
diff --git a/list-objects.h b/list-objects.h
index 3db7bb6..297b2e0 100644
--- a/list-objects.h
+++ b/list-objects.h
@@ -2,8 +2,9 @@
#define LIST_OBJECTS_H
typedef void (*show_commit_fn)(struct commit *, void *);
+typedef void (*show_tree_entry_fn)(const struct name_entry *, void *);
typedef void (*show_object_fn)(struct object *, const struct name_path *,
const char *, void *);
-void traverse_commit_list(struct rev_info *, show_commit_fn, show_object_fn,
void *);
+void traverse_commit_list(struct rev_info *, show_commit_fn,
show_tree_entry_fn, show_object_fn, void *);
typedef void (*show_edge_fn)(struct commit *);
void mark_edges_uninteresting(struct commit_list *, struct rev_info *,
show_edge_fn);
diff --git a/upload-pack.c b/upload-pack.c
index 127e59a..ccf76d9 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -125,7 +125,7 @@ static int do_rev_list(int in, int out, void *user_data)
for (i = 0; i extra_edge_obj.nr; i++)
fprintf(pack_pipe, -%s\n, sha1_to_hex(
extra_edge_obj.objects[i].item-sha1));
- traverse_commit_list(revs, show_commit, show_object, NULL);
+ traverse_commit_list(revs,
[PATCH 00/11] pack v4 support in pack-objects
I can produce pack v4 on git.git with this and verify it with index-pack. I'm not familiar with pack-objects code and not really confident with my changes. Suggestions are welcome. Also I chose to keep packv4-create.c in libgit.a and move test code out to test-packv4.c. Not sure if it's good decision. The other option is to copy necessary code to pack-objects.c, then delete packv4-create.c in the end. Either way we have the same amount of code move. Thin pack support is not there yet, but it should be simple on pack-objects' end. Like the compatibility layer you added to sha1_file.c, this code does not take advantage of v4 as source packs (performance regressions entail) A lot of rooms for improvements. Nguyễn Thái Ngọc Duy (11): pack v4: allocate dicts from the beginning pack v4: stop using static/global variables in packv4-create.c pack v4: move packv4-create.c to libgit.a pack v4: add version argument to write_pack_header pack-write.c: add pv4_encode_in_pack_object_header pack-objects: add --version to specify written pack version list-objects.c: add show_tree_entry callback to traverse_commit_list pack-objects: create pack v4 tables pack-objects: do not cache delta for v4 trees pack-objects: exclude commits out of delta objects in v4 pack-objects: support writing pack v4 Makefile | 4 +- builtin/pack-objects.c | 187 +++-- builtin/rev-list.c | 4 +- bulk-checkin.c | 2 +- list-objects.c | 9 +- list-objects.h | 3 +- pack-write.c | 36 +++- pack.h | 6 +- packv4-create.c| 534 - packv4-create.h (new) | 50 + test-packv4.c (new)| 476 +++ upload-pack.c | 2 +- 12 files changed, 789 insertions(+), 524 deletions(-) create mode 100644 packv4-create.h create mode 100644 test-packv4.c -- 1.8.2.83.gc99314b -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/11] pack-objects: add --version to specify written pack version
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/pack-objects.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 33faea8..ef68fc5 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -81,6 +81,7 @@ static int num_preferred_base;
static struct progress *progress_state;
static int pack_compression_level = Z_DEFAULT_COMPRESSION;
static int pack_compression_seen;
+static int pack_version = 2;
static unsigned long delta_cache_size = 0;
static unsigned long max_delta_cache_size = 256 * 1024 * 1024;
@@ -735,7 +736,7 @@ static void write_pack_file(void)
else
f = create_tmp_packfile(pack_tmp_name);
- offset = write_pack_header(f, 2, nr_remaining);
+ offset = write_pack_header(f, pack_version, nr_remaining);
if (!offset)
die_errno(unable to write pack header);
nr_written = 0;
@@ -2455,6 +2456,7 @@ int cmd_pack_objects(int argc, const char **argv, const
char *prefix)
{ OPTION_CALLBACK, 0, index-version, NULL,
N_(version[,offset]),
N_(write the pack index file in the specified idx format
version),
0, option_parse_index_version },
+ OPT_INTEGER(0, version, pack_version, N_(pack version)),
OPT_ULONG(0, max-pack-size, pack_size_limit,
N_(maximum size of each output pack file)),
OPT_BOOL(0, local, local,
@@ -2525,6 +2527,8 @@ int cmd_pack_objects(int argc, const char **argv, const
char *prefix)
}
if (pack_to_stdout != !base_name || argc)
usage_with_options(pack_usage, pack_objects_options);
+ if (pack_version != 2)
+ die(_(pack version %d is not supported), pack_version);
rp_av[rp_ac++] = pack-objects;
if (thin) {
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 01/11] pack v4: allocate dicts from the beginning
commit_ident_table and tree_path_table are local to packv4-create.c
and test-packv4.c. Move them out of add_*_dict_entries so
add_*_dict_entries can be exported to pack-objects.c
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
packv4-create.c | 22 --
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/packv4-create.c b/packv4-create.c
index 38fa594..dbc2a03 100644
--- a/packv4-create.c
+++ b/packv4-create.c
@@ -181,14 +181,12 @@ static char *get_nameend_and_tz(char *from, int *tz_val)
return end;
}
-static int add_commit_dict_entries(void *buf, unsigned long size)
+int add_commit_dict_entries(struct dict_table *commit_ident_table,
+ void *buf, unsigned long size)
{
char *name, *end = NULL;
int tz_val;
- if (!commit_ident_table)
- commit_ident_table = create_dict_table();
-
/* parse and add author info */
name = strstr(buf, \nauthor );
if (name) {
@@ -212,14 +210,12 @@ static int add_commit_dict_entries(void *buf, unsigned
long size)
return 0;
}
-static int add_tree_dict_entries(void *buf, unsigned long size)
+static int add_tree_dict_entries(struct dict_table *tree_path_table,
+void *buf, unsigned long size)
{
struct tree_desc desc;
struct name_entry name_entry;
- if (!tree_path_table)
- tree_path_table = create_dict_table();
-
init_tree_desc(desc, buf, size);
while (tree_entry(desc, name_entry)) {
int pathlen = tree_entry_len(name_entry);
@@ -659,6 +655,9 @@ static int create_pack_dictionaries(struct packed_git *p,
struct progress *progress_state;
unsigned int i;
+ commit_ident_table = create_dict_table();
+ tree_path_table = create_dict_table();
+
progress_state = start_progress(Scanning objects, p-num_objects);
for (i = 0; i p-num_objects; i++) {
struct pack_idx_entry *obj = obj_list[i];
@@ -666,7 +665,8 @@ static int create_pack_dictionaries(struct packed_git *p,
enum object_type type;
unsigned long size;
struct object_info oi = {};
- int (*add_dict_entries)(void *, unsigned long);
+ int (*add_dict_entries)(struct dict_table *, void *, unsigned
long);
+ struct dict_table *dict;
display_progress(progress_state, i+1);
@@ -679,9 +679,11 @@ static int create_pack_dictionaries(struct packed_git *p,
switch (type) {
case OBJ_COMMIT:
add_dict_entries = add_commit_dict_entries;
+ dict = commit_ident_table;
break;
case OBJ_TREE:
add_dict_entries = add_tree_dict_entries;
+ dict = tree_path_table;
break;
default:
continue;
@@ -693,7 +695,7 @@ static int create_pack_dictionaries(struct packed_git *p,
if (check_sha1_signature(obj-sha1, data, size, typename(type)))
die(packed %s from %s is corrupt,
sha1_to_hex(obj-sha1), p-pack_name);
- if (add_dict_entries(data, size) 0)
+ if (add_dict_entries(dict, data, size) 0)
die(can't process %s object %s,
typename(type), sha1_to_hex(obj-sha1));
free(data);
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/11] pack v4: move packv4-create.c to libgit.a
git-packv4-create now becomes test-packv4. Code that will not be used
by pack-objects.c is moved to test-packv4.c. It may be removed when
the code transition to pack-objects completes.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Makefile| 4 +-
packv4-create.c | 491 +---
packv4-create.h | 39 +
test-packv4.c (new) | 476 ++
4 files changed, 525 insertions(+), 485 deletions(-)
create mode 100644 test-packv4.c
diff --git a/Makefile b/Makefile
index 22fc276..af2e3e3 100644
--- a/Makefile
+++ b/Makefile
@@ -550,7 +550,6 @@ PROGRAM_OBJS += shell.o
PROGRAM_OBJS += show-index.o
PROGRAM_OBJS += upload-pack.o
PROGRAM_OBJS += remote-testsvn.o
-PROGRAM_OBJS += packv4-create.o
# Binary suffix, set to .exe for Windows builds
X =
@@ -568,6 +567,7 @@ TEST_PROGRAMS_NEED_X += test-line-buffer
TEST_PROGRAMS_NEED_X += test-match-trees
TEST_PROGRAMS_NEED_X += test-mergesort
TEST_PROGRAMS_NEED_X += test-mktemp
+TEST_PROGRAMS_NEED_X += test-packv4
TEST_PROGRAMS_NEED_X += test-parse-options
TEST_PROGRAMS_NEED_X += test-path-utils
TEST_PROGRAMS_NEED_X += test-prio-queue
@@ -702,6 +702,7 @@ LIB_H += notes.h
LIB_H += object.h
LIB_H += pack-revindex.h
LIB_H += pack.h
+LIB_H += packv4-create.h
LIB_H += packv4-parse.h
LIB_H += parse-options.h
LIB_H += patch-ids.h
@@ -839,6 +840,7 @@ LIB_OBJS += object.o
LIB_OBJS += pack-check.o
LIB_OBJS += pack-revindex.o
LIB_OBJS += pack-write.o
+LIB_OBJS += packv4-create.o
LIB_OBJS += packv4-parse.o
LIB_OBJS += pager.o
LIB_OBJS += parse-options.o
diff --git a/packv4-create.c b/packv4-create.c
index 920a0b4..cdf82c0 100644
--- a/packv4-create.c
+++ b/packv4-create.c
@@ -18,9 +18,9 @@
#include packv4-create.h
-static int pack_compression_seen;
-static int pack_compression_level = Z_DEFAULT_COMPRESSION;
-static int min_tree_copy = 1;
+int pack_compression_seen;
+int pack_compression_level = Z_DEFAULT_COMPRESSION;
+int min_tree_copy = 1;
struct data_entry {
unsigned offset;
@@ -28,17 +28,6 @@ struct data_entry {
unsigned hits;
};
-struct dict_table {
- unsigned char *data;
- unsigned cur_offset;
- unsigned size;
- struct data_entry *entry;
- unsigned nb_entries;
- unsigned max_entries;
- unsigned *hash;
- unsigned hash_size;
-};
-
struct dict_table *create_dict_table(void)
{
return xcalloc(sizeof(struct dict_table), 1);
@@ -139,7 +128,7 @@ static int cmp_dict_entries(const void *a_, const void *b_)
return diff;
}
-static void sort_dict_entries_by_hits(struct dict_table *t)
+void sort_dict_entries_by_hits(struct dict_table *t)
{
qsort(t-entry, t-nb_entries, sizeof(*t-entry), cmp_dict_entries);
t-hash_size = (t-nb_entries * 4 / 3) / 2;
@@ -208,7 +197,7 @@ int add_commit_dict_entries(struct dict_table
*commit_ident_table,
return 0;
}
-static int add_tree_dict_entries(struct dict_table *tree_path_table,
+int add_tree_dict_entries(struct dict_table *tree_path_table,
void *buf, unsigned long size)
{
struct tree_desc desc;
@@ -224,7 +213,7 @@ static int add_tree_dict_entries(struct dict_table
*tree_path_table,
return 0;
}
-void dump_dict_table(struct dict_table *t)
+static void dump_dict_table(struct dict_table *t)
{
int i;
@@ -241,7 +230,7 @@ void dump_dict_table(struct dict_table *t)
}
}
-static void dict_dump(struct packv4_tables *v4)
+void dict_dump(struct packv4_tables *v4)
{
dump_dict_table(v4-commit_ident_table);
dump_dict_table(v4-tree_path_table);
@@ -611,103 +600,6 @@ void *pv4_encode_tree(const struct packv4_tables *v4,
return buffer;
}
-static struct pack_idx_entry *get_packed_object_list(struct packed_git *p)
-{
- unsigned i, nr_objects = p-num_objects;
- struct pack_idx_entry *objects;
-
- objects = xmalloc((nr_objects + 1) * sizeof(*objects));
- objects[nr_objects].offset = p-pack_size - 20;
- for (i = 0; i nr_objects; i++) {
- hashcpy(objects[i].sha1, nth_packed_object_sha1(p, i));
- objects[i].offset = nth_packed_object_offset(p, i);
- }
-
- return objects;
-}
-
-static int sort_by_offset(const void *e1, const void *e2)
-{
- const struct pack_idx_entry * const *entry1 = e1;
- const struct pack_idx_entry * const *entry2 = e2;
- if ((*entry1)-offset (*entry2)-offset)
- return -1;
- if ((*entry1)-offset (*entry2)-offset)
- return 1;
- return 0;
-}
-
-static struct pack_idx_entry **sort_objs_by_offset(struct pack_idx_entry *list,
- unsigned nr_objects)
-{
- unsigned i;
- struct pack_idx_entry **sorted;
-
- sorted = xmalloc((nr_objects + 1) * sizeof(*sorted));
- for (i = 0; i nr_objects + 1; i++)
-
[PATCH 02/11] pack v4: stop using static/global variables in packv4-create.c
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
packv4-create.c | 103 --
packv4-create.h (new) | 11 ++
2 files changed, 69 insertions(+), 45 deletions(-)
create mode 100644 packv4-create.h
diff --git a/packv4-create.c b/packv4-create.c
index dbc2a03..920a0b4 100644
--- a/packv4-create.c
+++ b/packv4-create.c
@@ -15,6 +15,7 @@
#include pack-revindex.h
#include progress.h
#include varint.h
+#include packv4-create.h
static int pack_compression_seen;
@@ -145,9 +146,6 @@ static void sort_dict_entries_by_hits(struct dict_table *t)
rehash_entries(t);
}
-static struct dict_table *commit_ident_table;
-static struct dict_table *tree_path_table;
-
/*
* Parse the author/committer line from a canonical commit object.
* The 'from' argument points right after the author or committer
@@ -243,10 +241,10 @@ void dump_dict_table(struct dict_table *t)
}
}
-static void dict_dump(void)
+static void dict_dump(struct packv4_tables *v4)
{
- dump_dict_table(commit_ident_table);
- dump_dict_table(tree_path_table);
+ dump_dict_table(v4-commit_ident_table);
+ dump_dict_table(v4-tree_path_table);
}
/*
@@ -254,10 +252,12 @@ static void dict_dump(void)
* pack SHA1 table incremented by 1, or the literal SHA1 value prefixed
* with a zero byte if the needed SHA1 is not available in the table.
*/
-static struct pack_idx_entry *all_objs;
-static unsigned all_objs_nr;
-static int encode_sha1ref(const unsigned char *sha1, unsigned char *buf)
+
+int encode_sha1ref(const struct packv4_tables *v4,
+ const unsigned char *sha1, unsigned char *buf)
{
+ unsigned all_objs_nr = v4-all_objs_nr;
+ struct pack_idx_entry *all_objs = v4-all_objs;
unsigned lo = 0, hi = all_objs_nr;
do {
@@ -284,7 +284,8 @@ static int encode_sha1ref(const unsigned char *sha1,
unsigned char *buf)
* strict so to ensure the canonical version may always be
* regenerated and produce the same hash.
*/
-void *pv4_encode_commit(void *buffer, unsigned long *sizep)
+void *pv4_encode_commit(const struct packv4_tables *v4,
+ void *buffer, unsigned long *sizep)
{
unsigned long size = *sizep;
char *in, *tail, *end;
@@ -310,7 +311,7 @@ void *pv4_encode_commit(void *buffer, unsigned long *sizep)
if (get_sha1_lowhex(in + 5, sha1) 0)
goto bad_data;
in += 46;
- out += encode_sha1ref(sha1, out);
+ out += encode_sha1ref(v4, sha1, out);
/* count how many parent lines */
nb_parents = 0;
@@ -325,7 +326,7 @@ void *pv4_encode_commit(void *buffer, unsigned long *sizep)
while (nb_parents--) {
if (get_sha1_lowhex(in + 7, sha1))
goto bad_data;
- out += encode_sha1ref(sha1, out);
+ out += encode_sha1ref(v4, sha1, out);
in += 48;
}
@@ -337,7 +338,7 @@ void *pv4_encode_commit(void *buffer, unsigned long *sizep)
end = get_nameend_and_tz(in, tz_val);
if (!end)
goto bad_data;
- author_index = dict_add_entry(commit_ident_table, tz_val, in, end - in);
+ author_index = dict_add_entry(v4-commit_ident_table, tz_val, in, end -
in);
if (author_index 0)
goto bad_dict;
author_time = strtoul(end, end, 10);
@@ -353,7 +354,7 @@ void *pv4_encode_commit(void *buffer, unsigned long *sizep)
end = get_nameend_and_tz(in, tz_val);
if (!end)
goto bad_data;
- commit_index = dict_add_entry(commit_ident_table, tz_val, in, end - in);
+ commit_index = dict_add_entry(v4-commit_ident_table, tz_val, in, end -
in);
if (commit_index 0)
goto bad_dict;
commit_time = strtoul(end, end, 10);
@@ -436,7 +437,8 @@ static int compare_tree_entries(struct name_entry *e1,
struct name_entry *e2)
* If a delta buffer is provided, we may encode multiple ranges of tree
* entries against that buffer.
*/
-void *pv4_encode_tree(void *_buffer, unsigned long *sizep,
+void *pv4_encode_tree(const struct packv4_tables *v4,
+ void *_buffer, unsigned long *sizep,
void *delta, unsigned long delta_size,
const unsigned char *delta_sha1)
{
@@ -551,7 +553,7 @@ void *pv4_encode_tree(void *_buffer, unsigned long *sizep,
cp += encode_varint(copy_start, cp);
cp += encode_varint(copy_count, cp);
if (first_delta)
- cp += encode_sha1ref(delta_sha1, cp);
+ cp += encode_sha1ref(v4, delta_sha1, cp);
/*
* Now let's make sure this is going to take less
@@ -577,7 +579,7 @@ void *pv4_encode_tree(void *_buffer, unsigned long *sizep,
}
pathlen =
[PATCH 09/11] pack-objects: do not cache delta for v4 trees
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/pack-objects.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 69a22c1..665853d 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1759,8 +1759,12 @@ static void find_deltas(struct object_entry **list,
unsigned *list_size,
* and therefore it is best to go to the write phase ASAP
* instead, as we can afford spending more time compressing
* between writes at that moment.
+*
+* For v4 trees we'll need to delta differently anyway
+* so no cache. v4 commits simply do not delta.
*/
- if (entry-delta_data !pack_to_stdout) {
+ if (entry-delta_data !pack_to_stdout
+ (pack_version 4 || entry-type == OBJ_BLOB)) {
entry-z_delta_size = do_compress(entry-delta_data,
entry-delta_size);
cache_lock();
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/11] pack-objects: exclude commits out of delta objects in v4
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/pack-objects.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 665853d..daa4349 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1332,7 +1332,8 @@ static void check_object(struct object_entry *entry)
break;
}
- if (base_ref (base_entry = locate_object_entry(base_ref))) {
+ if (base_ref (base_entry = locate_object_entry(base_ref))
+ (pack_version 4 || entry-type != OBJ_COMMIT)) {
/*
* If base_ref was set above that means we wish to
* reuse delta data, and we even found that base
@@ -1416,6 +1417,8 @@ static void get_object_details(void)
check_object(entry);
if (big_file_threshold entry-size)
entry-no_try_delta = 1;
+ if (pack_version == 4 entry-type == OBJ_COMMIT)
+ entry-no_try_delta = 1;
}
free(sorted_by_offset);
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 11/11] pack-objects: support writing pack v4
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/pack-objects.c | 85 --
pack.h | 2 +-
2 files changed, 76 insertions(+), 11 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index daa4349..f6586a1 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -254,8 +254,10 @@ static unsigned long write_no_reuse_object(struct sha1file
*f, struct object_ent
enum object_type type;
void *buf;
struct git_istream *st = NULL;
+ char *result = OK;
- if (!usable_delta) {
+ if (!usable_delta ||
+ (pack_version == 4 || entry-type == OBJ_TREE)) {
if (entry-type == OBJ_BLOB
entry-size big_file_threshold
(st = open_istream(entry-idx.sha1, type, size, NULL)) !=
NULL)
@@ -287,7 +289,37 @@ static unsigned long write_no_reuse_object(struct sha1file
*f, struct object_ent
if (st) /* large blob case, just assume we don't compress well */
datalen = size;
- else if (entry-z_delta_size)
+ else if (pack_version == 4 entry-type == OBJ_COMMIT) {
+ datalen = size;
+ result = pv4_encode_commit(v4, buf, datalen);
+ if (result) {
+ free(buf);
+ buf = result;
+ type = OBJ_PV4_COMMIT;
+ }
+ } else if (pack_version == 4 entry-type == OBJ_TREE) {
+ datalen = size;
+ if (usable_delta) {
+ unsigned long base_size;
+ char *base_buf;
+ base_buf = read_sha1_file(entry-delta-idx.sha1, type,
+ base_size);
+ if (!base_buf || type != OBJ_TREE)
+ die(unable to read %s,
+ sha1_to_hex(entry-delta-idx.sha1));
+ result = pv4_encode_tree(v4, buf, datalen,
+base_buf, base_size,
+entry-delta-idx.sha1);
+ free(base_buf);
+ } else
+ result = pv4_encode_tree(v4, buf, datalen,
+NULL, 0, NULL);
+ if (result) {
+ free(buf);
+ buf = result;
+ type = OBJ_PV4_TREE;
+ }
+ } else if (entry-z_delta_size)
datalen = entry-z_delta_size;
else
datalen = do_compress(buf, size);
@@ -296,7 +328,10 @@ static unsigned long write_no_reuse_object(struct sha1file
*f, struct object_ent
* The object header is a byte of 'type' followed by zero or
* more bytes of length.
*/
- hdrlen = encode_in_pack_object_header(type, size, header);
+ if (pack_version 4)
+ hdrlen = encode_in_pack_object_header(type, size, header);
+ else
+ hdrlen = pv4_encode_in_pack_object_header(type, size, header);
if (type == OBJ_OFS_DELTA) {
/*
@@ -318,7 +353,7 @@ static unsigned long write_no_reuse_object(struct sha1file
*f, struct object_ent
sha1write(f, header, hdrlen);
sha1write(f, dheader + pos, sizeof(dheader) - pos);
hdrlen += sizeof(dheader) - pos;
- } else if (type == OBJ_REF_DELTA) {
+ } else if (type == OBJ_REF_DELTA pack_version 4) {
/*
* Deltas with a base reference contain
* an additional 20 bytes for the base sha1.
@@ -332,6 +367,10 @@ static unsigned long write_no_reuse_object(struct sha1file
*f, struct object_ent
sha1write(f, header, hdrlen);
sha1write(f, entry-delta-idx.sha1, 20);
hdrlen += 20;
+ } else if (type == OBJ_REF_DELTA pack_version == 4) {
+ hdrlen += encode_sha1ref(v4, entry-delta-idx.sha1,
+ header + hdrlen);
+ sha1write(f, header, hdrlen);
} else {
if (limit hdrlen + datalen + 20 = limit) {
if (st)
@@ -341,14 +380,26 @@ static unsigned long write_no_reuse_object(struct
sha1file *f, struct object_ent
}
sha1write(f, header, hdrlen);
}
+
if (st) {
datalen = write_large_blob_data(st, f, entry-idx.sha1);
close_istream(st);
- } else {
- sha1write(f, buf, datalen);
- free(buf);
+ return hdrlen + datalen;
}
+ if (!result) {
+ warning(_(can't convert %s object %s),
+ typename(entry-type),
+ sha1_to_hex(entry-idx.sha1));
+
[PATCH 08/11] pack-objects: create pack v4 tables
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/pack-objects.c | 87 --
1 file changed, 85 insertions(+), 2 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index b38d3dc..69a22c1 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -18,6 +18,7 @@
#include refs.h
#include streaming.h
#include thread-utils.h
+#include packv4-create.h
static const char *pack_usage[] = {
N_(git pack-objects --stdout [options...] [ ref-list |
object-list]),
@@ -61,6 +62,8 @@ static struct object_entry *objects;
static struct pack_idx_entry **written_list;
static uint32_t nr_objects, nr_alloc, nr_result, nr_written;
+static struct packv4_tables v4;
+
static int non_empty;
static int reuse_delta = 1, reuse_object = 1;
static int keep_unreachable, unpack_unreachable, include_tag;
@@ -2039,12 +2042,42 @@ static int add_ref_tag(const char *path, const unsigned
char *sha1, int flag, vo
return 0;
}
+static int sha1_idx_sort(const void *a_, const void *b_)
+{
+ const struct pack_idx_entry *a = a_;
+ const struct pack_idx_entry *b = b_;
+ return hashcmp(a-sha1, b-sha1);
+}
+
+static void prepare_sha1_table(void)
+{
+ unsigned i;
+ /*
+* This table includes SHA-1s that may not be present in the
+* pack. One of the use of such SHA-1 is for completing thin
+* packs, where index-pack does not need to add SHA-1 to the
+* table at completion time.
+*/
+ v4.all_objs = xmalloc(nr_objects * sizeof(*v4.all_objs));
+ v4.all_objs_nr = nr_objects;
+ for (i = 0; i nr_objects; i++)
+ v4.all_objs[i] = objects[i].idx;
+ qsort(v4.all_objs, nr_objects, sizeof(*v4.all_objs),
+ sha1_idx_sort);
+}
+
static void prepare_pack(int window, int depth)
{
struct object_entry **delta_list;
uint32_t i, nr_deltas;
unsigned n;
+ if (pack_version == 4) {
+ sort_dict_entries_by_hits(v4.commit_ident_table);
+ sort_dict_entries_by_hits(v4.tree_path_table);
+ prepare_sha1_table();
+ }
+
get_object_details();
/*
@@ -2191,6 +2224,34 @@ static void read_object_list_from_stdin(void)
add_preferred_base_object(line+41);
add_object_entry(sha1, 0, line+41, 0);
+
+ if (pack_version == 4) {
+ void *data;
+ enum object_type type;
+ unsigned long size;
+ int (*add_dict_entries)(struct dict_table *, void *,
unsigned long);
+ struct dict_table *dict;
+
+ switch (sha1_object_info(sha1, size)) {
+ case OBJ_COMMIT:
+ add_dict_entries = add_commit_dict_entries;
+ dict = v4.commit_ident_table;
+ break;
+ case OBJ_TREE:
+ add_dict_entries = add_tree_dict_entries;
+ dict = v4.tree_path_table;
+ break;
+ default:
+ continue;
+ }
+ data = read_sha1_file(sha1, type, size);
+ if (!data)
+ die(cannot unpack %s, sha1_to_hex(sha1));
+ if (add_dict_entries(dict, data, size) 0)
+ die(can't process %s object %s,
+ typename(type), sha1_to_hex(sha1));
+ free(data);
+ }
}
}
@@ -2198,10 +2259,26 @@ static void read_object_list_from_stdin(void)
static void show_commit(struct commit *commit, void *data)
{
+ if (pack_version == 4) {
+ unsigned long size;
+ enum object_type type;
+ unsigned char *buf;
+
+ /* commit-buffer is NULL most of the time, don't bother */
+ buf = read_sha1_file(commit-object.sha1, type, size);
+ add_commit_dict_entries(v4.commit_ident_table, buf, size);
+ free(buf);
+ }
add_object_entry(commit-object.sha1, OBJ_COMMIT, NULL, 0);
commit-object.flags |= OBJECT_ADDED;
}
+static void show_tree_entry(const struct name_entry *entry, void *data)
+{
+ dict_add_entry(v4.tree_path_table, entry-mode, entry-path,
+ tree_entry_len(entry));
+}
+
static void show_object(struct object *obj,
const struct name_path *path, const char *last,
void *data)
@@ -2380,7 +2457,9 @@ static void get_object_list(int ac, const char **av)
if (prepare_revision_walk(revs))
die(revision walk setup failed);
mark_edges_uninteresting(revs.commits, revs,
[PATCH] pull: use $curr_branch_short more
One of the first things git-pull.sh does is setting $curr_branch to
the target of HEAD and $curr_branch_short to the same but with the
leading refs/heads/ removed. Simplify the code by using
$curr_branch_short instead of setting $curr_branch to the same
shortened value.
The only other use of $curr_branch in that function doesn't have to
be replaced with $curr_branch_short because it just checks if the
string is empty. That property is the same with or without the prefix
unless HEAD points to refs/heads/ alone, which is invalid.
Noticed-by: Felipe Contreras felipe.contre...@gmail.com
Signed-off-by: Rene Scharfe l@web.de
---
Replacement patch. Corrected the command in the summary and
changed the first part of the description slightly.
git-pull.sh | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/git-pull.sh b/git-pull.sh
index f0df41c..d8b2708 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -166,9 +166,8 @@ error_on_no_merge_candidates () {
op_prep=with
fi
- curr_branch=${curr_branch#refs/heads/}
- upstream=$(git config branch.$curr_branch.merge)
- remote=$(git config branch.$curr_branch.remote)
+ upstream=$(git config branch.$curr_branch_short.merge)
+ remote=$(git config branch.$curr_branch_short.remote)
if [ $# -gt 1 ]; then
if [ $rebase = true ]; then
-- 1.8.4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] branch: use $curr_branch_short more
Am 31.08.2013 19:20, schrieb Felipe Contreras: A summary should contain as much information that would allow me to skip the commit message as possible. If I can't tell from the summary if I can safely skip the commit message, the summary is not doing a good job. trivial simplification explains the what, and the why at the same time, and allows most people to skip the commit message, thus is a good summary. No patch should be skipped on the mailing list. As you wrote, trivial patches can still be wrong. When going through the history I can see that quickly recognizing insubstantial changes is useful, but if I see a summary twice then in my mind forms a big question mark -- why did the same thing had to be done yet again? As an example, both 0d12e59f (pull: replace unnecessary sed invocation) and bc2bbc45 (pull, rebase: simplify to use die()) could arguably have had the summary trivial simplification, but I'm glad the author went with something a bit more specific. I agree that some kind of tagging with keywords like trivial, typo and so on can be helpful, though. Again, triviality and correctness are two separate different things. The patch is trivial even if you can't judge it's correctness. Well, in terms of impact I agree. To me, what you are describing is an obvious patch, not a trivial one. An obvious patch is so obvious that you can judge it's correctness easily by looking at the diff, a trivial one is of little importance. That's one definition; I think I had the mathematical notion in mind that calls proofs trivial which are immediately evident. René -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dir: remove dead code
Am 08.09.2013 16:42, schrieb Ramkumar Ramachandra:
On Sun, Sep 8, 2013 at 6:00 PM, René Scharfe l@web.de
mailto:l@web.de wrote:
Am 08.09.2013 08:09, schrieb Ramkumar Ramachandra:
Remove dead code around remove_dir_recursively().
This basically reverts ae2f203e (clean: preserve nested git worktree
in subdirectories). t7300 still seems to pass, though. I wonder why.
t7300 has nothing to do with ae2f203e.
ae2f203e modified t/t7300-clean.sh.
dir.c | 21 -
1 file changed, 4 insertions(+), 17 deletions(-)
diff --git a/dir.c b/dir.c
index 910bfcd..2b31241 100644
--- a/dir.c
+++ b/dir.c
@@ -1464,11 +1464,11 @@ int is_empty_dir(const char *path)
return ret;
}
-static int remove_dir_recurse(struct strbuf *path, int flag,
int *kept_up)
+int remove_dir_recursively(struct strbuf *path, int flag)
{
DIR *dir;
struct dirent *e;
- int ret = 0, original_len = path-len, len, kept_down = 0;
+ int ret = 0, original_len = path-len, len;
int only_empty = (flag REMOVE_DIR_EMPTY_ONLY);
int keep_toplevel = (flag REMOVE_DIR_KEEP_TOPLEVEL);
unsigned char submodule_head[20];
@@ -1476,8 +1476,6 @@ static int remove_dir_recurse(struct
strbuf *path, int flag, int *kept_up)
if ((flag REMOVE_DIR_KEEP_NESTED_GIT)
!resolve_gitlink_ref(path-__buf, HEAD,
submodule_head)) {
/* Do not descend and nuke a nested git work
tree. */
- if (kept_up)
- *kept_up = 1;
return 0;
}
@@ -1504,7 +1502,7 @@ static int remove_dir_recurse(struct
strbuf *path, int flag, int *kept_up)
if (lstat(path-buf, st))
; /* fall thru */
else if (S_ISDIR(st.st_mode)) {
- if (!remove_dir_recurse(path, flag,
kept_down))
+ if (!remove_dir_recursively(path, flag))
kept_down could have been set to 1 here...
Not possible.
Why?
I guess the answer is that kept_down could have only been set if the
flag REMOVE_DIR_KEEP_NESTED_GIT is given, which only git clean uses,
which in turn has its own implementation of remove_dir_recursively()
named remove_dirs() since f538a91e (git-clean: Display more accurate
delete messages).
That probably means you can remove even more code from
remove_dir_recursively().
continue; /* happy */
} else if (!only_empty !unlink(path-buf))
continue; /* happy, too */
@@ -1516,22 +1514,11 @@ static int remove_dir_recurse(struct
strbuf *path, int flag, int *kept_up)
closedir(dir);
strbuf_setlen(path, original_len);
- if (!ret !keep_toplevel !kept_down)
+ if (!ret !keep_toplevel)
ret = rmdir(path-buf);
... and would have prevented the rmdir() call here.
Is the removed code really dead? And if not, why does t7300 still pass?
Yes, it clearly is. Shared secret.
What is the secret and who shares it?
Thanks,
René
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] l10n: de.po: use das Tag instead of der Tag
Use das Tag to avoid confusion with the German word Tag (day). Reported-by: Dirk Heinrichs dirk.heinri...@altum.de Signed-off-by: Ralf Thielow ralf.thie...@gmail.com --- po/de.po | 22 +++--- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/po/de.po b/po/de.po index 11dde11..f4076fb 100644 --- a/po/de.po +++ b/po/de.po @@ -4694,12 +4694,12 @@ msgstr git describe [Optionen] --dirty #: builtin/describe.c:237 #, c-format msgid annotated tag %s not available -msgstr annotierter Tag %s ist nicht verfügbar +msgstr annotiertes Tag %s ist nicht verfügbar #: builtin/describe.c:241 #, c-format msgid annotated tag %s has no embedded name -msgstr annotierter Tag %s hat keinen eingebetteten Namen +msgstr annotiertes Tag %s hat keinen eingebetteten Namen #: builtin/describe.c:243 #, c-format @@ -4765,7 +4765,7 @@ msgstr #: builtin/describe.c:409 msgid find the tag that comes after the commit -msgstr findet den Tag, die nach Commit kommt +msgstr findet das Tag, das nach Commit kommt #: builtin/describe.c:410 msgid debug search strategy on stderr @@ -4777,7 +4777,7 @@ msgstr verwendet alle Referenzen #: builtin/describe.c:412 msgid use any tag, even unannotated -msgstr verwendet jeden Tag, auch nicht-annotierte +msgstr verwendet jedes Tag, auch nicht-annotierte #: builtin/describe.c:413 msgid always use long format @@ -4880,7 +4880,7 @@ msgstr Importiert Kennzeichen von dieser Datei #: builtin/fast-export.c:678 msgid Fake a tagger when tags lack one -msgstr erzeugt künstlich einen Tag-Ersteller, wenn der Tag keinen hat +msgstr erzeugt künstlich einen Tag-Ersteller, wenn das Tag keinen hat #: builtin/fast-export.c:680 msgid Output full tree for each commit @@ -5013,7 +5013,7 @@ msgstr (kann lokale Referenz nicht aktualisieren) #: builtin/fetch.c:324 msgid [new tag] -msgstr [neuer Tag] +msgstr [neues Tag] #: builtin/fetch.c:327 msgid [new branch] @@ -7831,7 +7831,7 @@ msgstr #: builtin/push.c:257 msgid Updates were rejected because the tag already exists in the remote. msgstr -Aktualisierungen wurden zurückgewiesen, weil der Tag bereits\n +Aktualisierungen wurden zurückgewiesen, weil das Tag bereits\n im Remote-Repository existiert. #: builtin/push.c:260 @@ -9244,7 +9244,7 @@ msgstr Optionen für Erstellung von Tags #: builtin/tag.c:454 msgid annotated tag, needs a message -msgstr annotierter Tag, benötigt eine Beschreibung +msgstr annotiertes Tag, benötigt eine Beschreibung #: builtin/tag.c:456 msgid tag message @@ -9252,15 +9252,15 @@ msgstr Tag-Beschreibung #: builtin/tag.c:458 msgid annotated and GPG-signed tag -msgstr annotierter und GPG-signierter Tag +msgstr annotiertes und GPG-signiertes Tag #: builtin/tag.c:462 msgid use another key to sign the tag -msgstr verwendet einen anderen Schlüssel um den Tag zu signieren +msgstr verwendet einen anderen Schlüssel um das Tag zu signieren #: builtin/tag.c:463 msgid replace the tag if exists -msgstr ersetzt den Tag, wenn er existiert +msgstr ersetzt das Tag, wenn es existiert #: builtin/tag.c:464 msgid show tag list in columns -- 1.8.4.325.g5deb81b -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
On Sat, Sep 07, 2013 at 11:37:13PM -0500, Felipe Contreras wrote: On Sat, Sep 7, 2013 at 11:18 PM, Jeff King p...@peff.net wrote: By svn-like, I mean the people whose workflow is: $ hack hack hack $ git commit $ git push ;# oops, somebody else pushed in the meantime $ git pull $ git push It's possible that some teams at work may be using this workflow. It's more likely that there would be a rebase if the push failed, but some teams might do a merge. I don't know because we don't dictate workflow to individual teams for the reasons I get into below. Regardless, merges are our typical workflow, so forcing rebase mode all the time wouldn't be appropriate for us. $ hack hack hack $ svn commit ;# oops, somebody else committed in the meantime $ svn update $ svn commit Those people would now have to learn enough to choose between merge and rebase when running the git pull. But that's only if they don't care about the shape of history. In my experience the people that cling more to centralized VCS do not like merges, so they rebase everything to make it a straight line. That is much more svn-like. So chances are they are already doing 'git pull --rebase' (or similar), so their workflow wouldn't be affected. We end up squashing each project branch into one commit (usually using git reset --soft), so we don't care about the shape of history. Over the course of a project branch, in fact, there may be many merges from the main release branches (including other projects), so history is going to be very messy otherwise. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [PATCH 0/3] Unconfuse git clone when two branches at are HEAD.
Philip Oakley philipoak...@iee.org writes: What I observed was that all the clones had the same HEAD problem, which I think comes from clone.c: guess_remote_head(). Yes. They share having to guess property because their data source does not tell them. My quick look at clone.c suggested to me that there would be a lot of commonality between the bundle data stream and the transport streams (identical?), and it was just a case of adding into the bundle data the same HEAD symref indication that would solve the normal clone problem (including backward compatibility). Is that a reasonable assesssment? You need to find a hole in the existing readers to stick the new information in a way that do not break existing readers but allow updated readers to extract that information. That is exactly what we did when we added the protocol capability. I do not offhand think an equivalent hole exists in the bundle file format. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
Richard Hansen rhan...@bbn.com writes: On 2013-09-07 22:41, Felipe Contreras wrote: On Wed, Sep 4, 2013 at 5:59 PM, Junio C Hamano gits...@pobox.com wrote: Which can be solved by adding the above fail option, and then renaming them to pull.integrate and branch.name.integrate to clarify what these variables are about (it is no longer do you rebase or not---if you choose not to rebase, by definition you are going to merge, as there is a third choice to fail), while retaining pull.rebase and branch.name.rebase as a deprecated synonym. All these names are completely unintuitive. First of all, why integrate? Integrate what to what? And then, why fail? Fail on what circumstances? Always? My proposal that does: pull.mode = merge/rebase/merge-ff-only Is way more intuitive. +1 What about something like: pull.mergeoptions (defaults to --ff-only) pull.rebaseoptions (defaults to empty? --preserve-merges?) branch.name.pull.mergeoptions (defaults to pull.mergeoptions) branch.name.pull.rebaseoptions (defaults to pull.rebaseoptions) As pull has two distinct phases fetch and merge/rebase, your mergeoptions/rebaseoptions is much better than mode, which does not tell which phase of pull the mode refers to. It is clear that they apply to the process to integrate the history obtained from the other side and your own history into one history. But it does not help Philip's case, if I understand correctly, where running git pull on some branches is always a mistake and the user wants it to stop at fetch the history and objects needed to complete the history from the other side phase without proceeding to the then integrate the history from the other side and the history of your branch into one step, which may be done with either merge or rebase. Even if we ignore that always fail, do not do anything use case, your two seemingly independent mergeoptions and rebaseoptions do not tell us which one is preferred between merge and rebase. A single pull.someoption = rebase | merge [| always-fail] makes that choice in a clear way, I think. Regarding the verb integrate. We used to explain pull is a fetch followed by a merge. With more people using git pull --rebase, the word merge used in that explanation of pull stopped being generic enough. Simplarily the upstream branch of local branch X is what you fetch and merge to update the branch X but that 'merge' can be 'rebase'. We needed a verb to call the process of integrate the two histories into one. git pull --help since 153d7265 (pull: change the description to integrate changes, 2013-07-07) uses that verb [*1*]. And that is where the name of the single configuration to pick how to integrate the history obtained by the first phase of pull came from. [Footnote] *1* I suspect that there may still be places in the documentation that have not been updated since the days back when the only valid way to integrate two lines of histories was to merge, and updating them may be a low-hanging fruit. Hint, hint. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] http-backend: provide Allow header for 405
The HTTP 1.1 standard requires an Allow header for 405 Method Not Allowed:
The response MUST include an Allow header containing a list of valid methods
for the requested resource.
So provide such a header when we return a 405 to the user agent.
Signed-off-by: brian m. carlson sand...@crustytoothpaste.net
---
http-backend.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/http-backend.c b/http-backend.c
index 0324417..8c61084 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -594,8 +594,11 @@ int main(int argc, char **argv)
if (strcmp(method, c-method)) {
const char *proto = getenv(SERVER_PROTOCOL);
- if (proto !strcmp(proto, HTTP/1.1))
+ if (proto !strcmp(proto, HTTP/1.1)) {
http_status(405, Method Not Allowed);
+ hdr_str(Allow, !strcmp(GET,
c-method) ?
+ GET, HEAD : c-method);
+ }
else
http_status(400, Bad Request);
hdr_nocache();
--
1.8.4.rc3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
Jeff King p...@peff.net writes:
A(ny) sanely defined compare A with B function should yield the
result of subtracting B from A, i.e. cmp(A,B) should be like (A-B).
That is what you feed qsort() and bsearch() (it is not limited to C;
you see the same in sort { $a = $b }). The definition naturally
makes cmp(A,B) 0 like A B and cmp(A,B) 0 like A B.
---
Ah, you mean if you think that the compare function should behave like
C *_cmp functions, it should be A-B. Perhaps it is simply that I do not
think of the function in those terms, but more like show me the
differences from B to A.
Otherwise why would so many
existing test frameworks do it the other way?
Which many existing frameworks do it the other way?
John mentioned JUnit, NUnit, and PHPUnit earlier in the thread. I
believe that Ruby's Test::Unit::Assertions also has
assert_equal(expected, actual).
Especially the last one can be excused. is A and B equal is a
binary between yes and no. If A and B are equal, B and A are
equal, and it becomes more like which endianness is correct? as
you mentioned earlier.
I think the real cause of confusion is that cmp(1) is not a
comparison in that sense but is an equality check; test_cmp has a
dual purpose in that its primary use as did the previous step
produce correct result? is an equality check and the order does not
really matter, but its secondary purpose, to show how the actual
output deviated from the norm, has to be done by subtracting the
expected result from the actual result.
As I said, I am somewhat sympathetic to those who want to see such
subtraction spelled as cmp(Actual,Expect), but we are so used to the
order diff(1) takes expect and actual to do that subtraction in
that order, so using diff(Expect,Actual) order is not that wrong.
Calling the abstraction test_diff might have avoided the wasted
brain bandwidth in this thread, but I do not think renaming it in
test-lib-functions.sh is worth the trouble, either ;-)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
On 2013-09-08 14:10, Junio C Hamano wrote: Richard Hansen rhan...@bbn.com writes: What about something like: pull.mergeoptions (defaults to --ff-only) pull.rebaseoptions (defaults to empty? --preserve-merges?) branch.name.pull.mergeoptions (defaults to pull.mergeoptions) branch.name.pull.rebaseoptions (defaults to pull.rebaseoptions) [snip] But it does not help Philip's case, if I understand correctly, where running git pull on some branches is always a mistake and the user wants it to stop at fetch the history and objects needed to complete the history from the other side phase without proceeding to the then integrate the history from the other side and the history of your branch into one step, which may be done with either merge or rebase. How about: branch.name.pull.defaultIntegrationMode = merge | rebase | none If 'merge', pull acts like 'git pull --merge' by default, merging the other commits into this branch. If 'rebase', pull acts like 'git pull --rebase' by default, rebasing this branch onto the other commits. If 'none', pull acts like 'git fetch' by default. Default: whatever pull.defaultIntegrationMode is set to. branch.name.pull.mergeoptions Arguments to pass to 'git merge' during the merge phase of 'git pull --merge'. Default: whatever pull.mergeoptions is set to. branch.name.pull.rebaseoptions Arguments to pass to 'git rebase' during the rebase phase of 'git pull --rebase'. Default: whatever pull.rebaseoptions is set to. pull.defaultIntegrationMode = rebase | merge | none See branch.name.pull.defaultIntegrationMode. Default: merge pull.mergeoptions See branch.name.pull.mergeoptions. Default: empty, but warn that a future version will change this to --ff-only. pull.rebaseoptions See branch.name.pull.rebaseoptions. Default: empty, but warn that a future version will change this to --preserve-merges? There's probably a better alternative to the term 'defaultIntegrationMode'. We could even add a defaultIntegrationMode = merge-there that reverses the parent order (the other commits become the first parent, the current branch becomes the second parent). -Richard -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[BUG] rebase not recovering gracefully from repack error
(patches|REBASE 8/9)$ git rebase --continue Applying: Check for __unix__ instead of __linux Applying: Completely disable crash handler on archs other than i386 and amd64 Auto packing the repository for optimum performance. You may also run git gc manually. See git help gc for more information. error: Could not read a7d470051f53f4e4c9247df752583868a79ec70b fatal: Failed to traverse parents of commit e6d2f264969207e337953717c260d37daa0a8554 error: failed to run repack (patches|REBASE 10/9)$ cat .git/rebase-apply/next 10 (patches|REBASE 10/9)$ The last patch has been dealt with and I consider the rebase done. Rebase would be bombing out without cleaning behind it when the auto repack fails ? On another aspect, I find the repack error is suspect: git fsck --no-dangling has nothing to complain about, and the missing commit is the real ancestor of a grafted commit. I suppose it has been gc'd by a buggy git version, as I recall seeing such a fix on the list some time ago. (yes, I shouldn't be using grafts any more, but that particular one dates back to 2006 ;) -- Yann [v1.8.4] -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/11] pack-write.c: add pv4_encode_in_pack_object_header
On Sun, 8 Sep 2013, Nguyễn Thái Ngọc Duy wrote:
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
pack-write.c | 29 +
pack.h | 1 +
2 files changed, 30 insertions(+)
diff --git a/pack-write.c b/pack-write.c
index 88e4788..6f11104 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -1,6 +1,7 @@
#include cache.h
#include pack.h
#include csum-file.h
+#include varint.h
void reset_pack_idx_option(struct pack_idx_option *opts)
{
@@ -340,6 +341,34 @@ int encode_in_pack_object_header(enum object_type type,
uintmax_t size, unsigned
return n;
}
+/*
+ * The per-object header is a pretty dense thing, which is
+ * - first byte: low four bits are size, then three bits of type,
+ *and the high bit is size continues.
+ * - each byte afterwards: low seven bits are size continuation,
+ *with the high bit being size continues
+ */
This comment is a bit misleading. It looks almost like the pack v2
object header encoding which is not a varint encoded value like this one
is.
+int pv4_encode_in_pack_object_header(enum object_type type,
+ uintmax_t size, unsigned char *hdr)
Could we have a somewhat shorter function name?
pv4_encode_object_header() should be acceptable given pv4 already
implies a pack.
+{
+ uintmax_t val;
+ if (type OBJ_COMMIT || type OBJ_PV4_TREE || type == OBJ_OFS_DELTA)
+ die(bad type %d, type);
This test has holes, such as types 5 and 8.
I think this would be better as:
switch (type) {
case OBJ_COMMIT:
case OBJ_TREE:
case OBJ_BLOB:
case OBJ_TAG:
case OBJ_REF_DELTA:
case OBJ_PV4_COMMIT:
case OBJ_PV4_TREE:
break;
default:
die(bad type %d, type);
}
The compiler ought to be smart enough to optimize the contiguous case
range. And that makes it explicit and obvious what we test for.
+
+ /*
+ * We allocate 4 bits in the LSB for the object type which
+ * should be good for quite a while, given that we effectively
+ * encodes only 5 object types: commit, tree, blob, delta,
+ * tag.
+ */
+ val = size;
+ if (MSB(val, 4))
+ die(fixme: the code doesn't currently cope with big sizes);
+ val = 4;
+ val |= type;
+ return encode_varint(val, hdr);
+}
+
struct sha1file *create_tmp_packfile(char **pack_tmp_name)
{
char tmpname[PATH_MAX];
diff --git a/pack.h b/pack.h
index 855f6c6..38f869d 100644
--- a/pack.h
+++ b/pack.h
@@ -83,6 +83,7 @@ extern off_t write_pack_header(struct sha1file *f, int,
uint32_t);
extern void fixup_pack_header_footer(int, unsigned char *, const char *,
uint32_t, unsigned char *, off_t);
extern char *index_pack_lockfile(int fd);
extern int encode_in_pack_object_header(enum object_type, uintmax_t,
unsigned char *);
+extern int pv4_encode_in_pack_object_header(enum object_type, uintmax_t,
unsigned char *);
#define PH_ERROR_EOF (-1)
#define PH_ERROR_PACK_SIGNATURE (-2)
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] send-email: don't call methods on undefined values
If SSL verification is enabled in git send-email, we could attempt to call a
method on an undefined value if the verification failed, since $smtp would end
up being undef. Look up the error string in a way that will produce a helpful
error message and not cause further errors.
Signed-off-by: brian m. carlson sand...@crustytoothpaste.net
---
git-send-email.perl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 2162478..3782c3b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1234,7 +1234,7 @@ X-Mailer: git-send-email $gitversion
if ($smtp-code == 220) {
$smtp = Net::SMTP::SSL-start_SSL($smtp,
ssl_verify_params())
- or die STARTTLS failed!
.$smtp-message;
+ or die STARTTLS failed!
.IO::Socket::SSL::errstr();
$smtp_encryption = '';
# Send EHLO again to receive fresh
# supported commands
--
1.8.4.rc3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/11] pack v4: move packv4-create.c to libgit.a
On Sun, 8 Sep 2013, Nguyễn Thái Ngọc Duy wrote:
git-packv4-create now becomes test-packv4. Code that will not be used
by pack-objects.c is moved to test-packv4.c. It may be removed when
the code transition to pack-objects completes.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Makefile| 4 +-
packv4-create.c | 491
+---
packv4-create.h | 39 +
test-packv4.c (new) | 476 ++
4 files changed, 525 insertions(+), 485 deletions(-)
create mode 100644 test-packv4.c
diff --git a/Makefile b/Makefile
index 22fc276..af2e3e3 100644
--- a/Makefile
+++ b/Makefile
@@ -550,7 +550,6 @@ PROGRAM_OBJS += shell.o
PROGRAM_OBJS += show-index.o
PROGRAM_OBJS += upload-pack.o
PROGRAM_OBJS += remote-testsvn.o
-PROGRAM_OBJS += packv4-create.o
# Binary suffix, set to .exe for Windows builds
X =
@@ -568,6 +567,7 @@ TEST_PROGRAMS_NEED_X += test-line-buffer
TEST_PROGRAMS_NEED_X += test-match-trees
TEST_PROGRAMS_NEED_X += test-mergesort
TEST_PROGRAMS_NEED_X += test-mktemp
+TEST_PROGRAMS_NEED_X += test-packv4
TEST_PROGRAMS_NEED_X += test-parse-options
TEST_PROGRAMS_NEED_X += test-path-utils
TEST_PROGRAMS_NEED_X += test-prio-queue
@@ -702,6 +702,7 @@ LIB_H += notes.h
LIB_H += object.h
LIB_H += pack-revindex.h
LIB_H += pack.h
+LIB_H += packv4-create.h
LIB_H += packv4-parse.h
LIB_H += parse-options.h
LIB_H += patch-ids.h
@@ -839,6 +840,7 @@ LIB_OBJS += object.o
LIB_OBJS += pack-check.o
LIB_OBJS += pack-revindex.o
LIB_OBJS += pack-write.o
+LIB_OBJS += packv4-create.o
LIB_OBJS += packv4-parse.o
LIB_OBJS += pager.o
LIB_OBJS += parse-options.o
diff --git a/packv4-create.c b/packv4-create.c
index 920a0b4..cdf82c0 100644
--- a/packv4-create.c
+++ b/packv4-create.c
@@ -18,9 +18,9 @@
#include packv4-create.h
-static int pack_compression_seen;
-static int pack_compression_level = Z_DEFAULT_COMPRESSION;
-static int min_tree_copy = 1;
+int pack_compression_seen;
+int pack_compression_level = Z_DEFAULT_COMPRESSION;
+int min_tree_copy = 1;
struct data_entry {
unsigned offset;
@@ -28,17 +28,6 @@ struct data_entry {
unsigned hits;
};
-struct dict_table {
- unsigned char *data;
- unsigned cur_offset;
- unsigned size;
- struct data_entry *entry;
- unsigned nb_entries;
- unsigned max_entries;
- unsigned *hash;
- unsigned hash_size;
-};
It doesn't seem necessary to move this structure definition to the
header file. Only an opaque
struct dict_table;
should be needed in packv4-create.h. That would keep the dictionary
handling localized.
Nicolas
[PATCHv2 0/5] branch: Fix --track on a remote-tracking non-branch
Hi, Here is the second iteration of this series. Only one change from the first iteration: The first patch now also fixes some missing -chaining noticed by Junio in t2024. ...Johan Johan Herland (4): t2024: Fix -chaining and a couple of typos t3200: Minor fix when preparing for tracking failure Refer to branch.name.remote/merge when documenting --track t3200: Add test demonstrating minor regression in 41c21f2 Per Cederqvist (1): branch.c: Relax unnecessary requirement on upstream's remote ref name Documentation/git-branch.txt | 6 -- branch.c | 3 +-- t/t2024-checkout-dwim.sh | 6 +++--- t/t3200-branch.sh| 37 - 4 files changed, 44 insertions(+), 8 deletions(-) -- 1.8.3.GIT -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 1/5] t2024: Fix -chaining and a couple of typos
Improved-by: Junio C Hamano gits...@pobox.com Signed-off-by: Johan Herland jo...@herland.net --- t/t2024-checkout-dwim.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh index dee55e4..094b92e 100755 --- a/t/t2024-checkout-dwim.sh +++ b/t/t2024-checkout-dwim.sh @@ -104,7 +104,7 @@ test_expect_success 'setup more remotes with unconventional refspecs' ' cd repo_c test_commit c_master git checkout -b bar - test_commit c_bar + test_commit c_bar git checkout -b spam test_commit c_spam ) @@ -113,9 +113,9 @@ test_expect_success 'setup more remotes with unconventional refspecs' ' cd repo_d test_commit d_master git checkout -b baz - test_commit f_baz + test_commit d_baz git checkout -b eggs - test_commit c_eggs + test_commit d_eggs ) git remote add repo_c repo_c git config remote.repo_c.fetch \ -- 1.8.3.GIT -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 4/5] t3200: Add test demonstrating minor regression in 41c21f2
In 41c21f2 (branch.c: Validate tracking branches with refspecs instead of refs/remotes/*), we changed the rules for what is considered a valid tracking branch (a.k.a. upstream branch). We now use the configured remotes and their refspecs to determine whether a proposed tracking branch is in fact within the domain of a remote, and we then use that information to deduce the upstream configuration (branch.name.remote and branch.name.merge). However, with that change, we also check that - in addition to a matching refspec - the result of mapping the tracking branch through that refspec (i.e. the corresponding ref name in the remote repo) happens to start with refs/heads/. In other words, we require that a tracking branch refers to a _branch_ in the remote repo. Now, consider that you are e.g. setting up an automated building/testing infrastructure for a group of similar source repositories. The build/test infrastructure consists of a central scheduler, and a number of build/test slave machines that perform the actual build/test work. The scheduler monitors the group of similar repos for changes (e.g. with a periodic git fetch), and triggers builds/tests to be run on one or more slaves. Graphically the changes flow between the repos like this: Source #1 ---v Slave #1 / Source #2 - Scheduler - Slave #2 \ Source #3 ---^ Slave #3 ... ... The scheduler maintains a single Git repo with each of the source repos set up as distinct remotes. The slaves also need access to all the changes from all of the source repos, so they pull from the scheduler repo, but using the following custom refspec: remote.origin.fetch = +refs/remotes/*:refs/remotes/* This makes all of the scheduler's remote-tracking branches automatically available as identical remote-tracking branches in each of the slaves. Now, consider what happens if a slave tries to create a local branch with one of the remote-tracking branches as upstream: git branch local_branch --track refs/remotes/source-1/some_branch Git now looks at the configured remotes (in this case there is only origin, pointing to the scheduler's repo) and sees refs/remotes/source-1/some_branch matching origin's refspec. Mapping through that refspec we find that the corresponding remote ref name is refs/remotes/source-1/some_branch. However, since this remote ref name does not start with refs/heads/, we discard it as a suitable upstream, and the whole command fails. This patch adds a testcase demonstrating this failure by creating two source repos (a and b) that are forwarded through a scheduler (c) to a slave repo (d), that then tries create a local branch with an upstream. See the next patch in this series for the exciting conclusion to this story... Reported-by: Per Cederqvist ced...@opera.com Signed-off-by: Johan Herland jo...@herland.net --- t/t3200-branch.sh | 34 ++ 1 file changed, 34 insertions(+) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 8f6ab8e..4031693 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -871,4 +871,38 @@ test_expect_success '--merged catches invalid object names' ' test_must_fail git branch --merged ' +test_expect_failure 'tracking with unexpected .fetch refspec' ' + git init a + ( + cd a + test_commit a + ) + git init b + ( + cd b + test_commit b + ) + git init c + ( + cd c + test_commit c + git remote add a ../a + git remote add b ../b + git fetch --all + ) + git init d + ( + cd d + git remote add c ../c + git config remote.c.fetch +refs/remotes/*:refs/remotes/* + git fetch c + git branch --track local/a/master remotes/a/master + test $(git config branch.local/a/master.remote) = c + test $(git config branch.local/a/master.merge) = refs/remotes/a/master + git rev-parse --verify a expect + git rev-parse --verify local/a/master actual + test_cmp expect actual + ) +' + test_done -- 1.8.3.GIT -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 5/5] branch.c: Relax unnecessary requirement on upstream's remote ref name
From: Per Cederqvist ced...@opera.com When creating an upstream relationship, we use the configured remotes and their refspecs to determine the upstream configuration settings branch.name.remote and branch.name.merge. However, if the matching refspec does not have refs/heads/something on the remote side, we end up rejecting the match, and failing the upstream configuration. It could be argued that when we set up an branch's upstream, we want that upstream to also be a proper branch in the remote repo. Although this is typically the common case, there are cases (as demonstrated by the previous patch in this series) where this requirement prevents a useful upstream relationship from being formed. Furthermore: - We have fundamentally no say in how the remote repo have organized its branches. The remote repo may put branches (or branch-like constructs that are insteresting for downstreams to track) outside refs/heads/*. - The user may intentionally want to track a non-branch from a remote repo, by using a branch and configured upstream in the local repo. Relaxing the checking to only require a matching remote/refspec allows the testcase introduced in the previous patch to succeed, and has no negative effect on the rest of the test suite. This patch fixes a behavior (arguably a regression) first introduced in 41c21f2 (branch.c: Validate tracking branches with refspecs instead of refs/remotes/*) on 2013-04-21 (released in = v1.8.3.2). Signed-off-by: Johan Herland jo...@herland.net --- branch.c | 3 +-- t/t3200-branch.sh | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/branch.c b/branch.c index c5c6984..2d15c19 100644 --- a/branch.c +++ b/branch.c @@ -203,8 +203,7 @@ static int check_tracking_branch(struct remote *remote, void *cb_data) struct refspec query; memset(query, 0, sizeof(struct refspec)); query.dst = tracking_branch; - return !(remote_find_tracking(remote, query) || -prefixcmp(query.src, refs/heads/)); + return !remote_find_tracking(remote, query); } static int validate_remote_tracking_branch(char *ref) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 4031693..f010303 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -871,7 +871,7 @@ test_expect_success '--merged catches invalid object names' ' test_must_fail git branch --merged ' -test_expect_failure 'tracking with unexpected .fetch refspec' ' +test_expect_success 'tracking with unexpected .fetch refspec' ' git init a ( cd a -- 1.8.3.GIT -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 2/5] t3200: Minor fix when preparing for tracking failure
We're testing that trying to --track a ref that is not covered by any remote refspec should fail. For that, we want to have refs/remotes/local/master present, but we also want the remote.local.fetch refspec to NOT match refs/remotes/local/master (so that the tracking setup will fail, as intended). However, when doing git fetch local to ensure the existence of refs/remotes/local/master, we must not already have changed remote.local.fetch so as to cause refs/remotes/local/master not to be fetched. Therefore, set remote.local.fetch to refs/heads/*:refs/remotes/local/* BEFORE we fetch, and then reset it to refs/heads/s:refs/remotes/local/s AFTER we have fetched (but before we test --track). Signed-off-by: Johan Herland jo...@herland.net --- t/t3200-branch.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 44ec6a4..8f6ab8e 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -319,8 +319,9 @@ test_expect_success 'test tracking setup (non-wildcard, matching)' ' test_expect_success 'tracking setup fails on non-matching refspec' ' git config remote.local.url . - git config remote.local.fetch refs/heads/s:refs/remotes/local/s + git config remote.local.fetch refs/heads/*:refs/remotes/local/* (git show-ref -q refs/remotes/local/master || git fetch local) + git config remote.local.fetch refs/heads/s:refs/remotes/local/s test_must_fail git branch --track my5 local/master test_must_fail git config branch.my5.remote test_must_fail git config branch.my5.merge -- 1.8.3.GIT -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 3/5] Refer to branch.name.remote/merge when documenting --track
Make it easier for readers to find the actual config variables that implement the upstream relationship. Suggested-by: Per Cederqvist ced...@opera.com Signed-off-by: Johan Herland jo...@herland.net --- Documentation/git-branch.txt | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt index b7cb625..311b336 100644 --- a/Documentation/git-branch.txt +++ b/Documentation/git-branch.txt @@ -48,7 +48,8 @@ working tree to it; use git checkout newbranch to switch to the new branch. When a local branch is started off a remote-tracking branch, Git sets up the -branch so that 'git pull' will appropriately merge from +branch (specifically the `branch.name.remote` and `branch.name.merge` +configuration entries) so that 'git pull' will appropriately merge from the remote-tracking branch. This behavior may be changed via the global `branch.autosetupmerge` configuration flag. That setting can be overridden by using the `--track` and `--no-track` options, and @@ -156,7 +157,8 @@ This option is only applicable in non-verbose mode. -t:: --track:: - When creating a new branch, set up configuration to mark the + When creating a new branch, set up `branch.name.remote` and + `branch.name.merge` configuration entries to mark the start-point branch as upstream from the new branch. This configuration will tell git to show the relationship between the two branches in `git status` and `git branch -v`. Furthermore, -- 1.8.3.GIT -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Unconfuse git clone when two branches at are HEAD.
From: Junio C Hamano gits...@pobox.com Sent: Sunday, September 08, 2013 6:35 PM Philip Oakley philipoak...@iee.org writes: What I observed was that all the clones had the same HEAD problem, which I think comes from clone.c: guess_remote_head(). Yes. They share having to guess property because their data source does not tell them. My quick look at clone.c suggested to me that there would be a lot of commonality between the bundle data stream and the transport streams (identical?), and it was just a case of adding into the bundle data the same HEAD symref indication that would solve the normal clone problem (including backward compatibility). Is that a reasonable assesssment? You need to find a hole in the existing readers to stick the new information in a way that do not break existing readers but allow updated readers to extract that information. That is exactly what we did when we added the protocol capability. I do not offhand think an equivalent hole exists in the bundle file format. -- I've been rummaging about as to options. One is to extend the ref format such that sha1 refs/heads/Test:HEAD would be considered a valid indicator of a symref relationship (i.e. using the typical 'colon' style). It would be appended after the regular refs, so all the existing refs are still transported. The point is that while it produces an error, it doesn't stop the cloning, and the error message error: * Ignoring funny ref 'refs/remotes/origin/Test:HEAD' locally gives a pretty clear statement of intent to those with older versions of git. Another alternative is to add an additional name space (e.g.) sha1 refs/remotes/origin/HEAD/Test which would simply be an extra directory layer that reflects where the HEAD should have been. Though this namespace example has the D/F conflict. Philip -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
On Sun, Sep 8, 2013 at 12:26 PM, brian m. carlson sand...@crustytoothpaste.net wrote: On Sat, Sep 07, 2013 at 11:37:13PM -0500, Felipe Contreras wrote: On Sat, Sep 7, 2013 at 11:18 PM, Jeff King p...@peff.net wrote: $ hack hack hack $ svn commit ;# oops, somebody else committed in the meantime $ svn update $ svn commit Those people would now have to learn enough to choose between merge and rebase when running the git pull. But that's only if they don't care about the shape of history. In my experience the people that cling more to centralized VCS do not like merges, so they rebase everything to make it a straight line. That is much more svn-like. So chances are they are already doing 'git pull --rebase' (or similar), so their workflow wouldn't be affected. We end up squashing each project branch into one commit (usually using git reset --soft), so we don't care about the shape of history. Over the course of a project branch, in fact, there may be many merges from the main release branches (including other projects), so history is going to be very messy otherwise. Yeah, but the key question at hand in this discussion is; what happens when 'git pull' stops working for them, and they don't know what to do, will they choose 'git pull --rebase' by mistake? I say the answer is no, because: 1) As you say in your scenario, somebody is telling these guys what to do, so when 'git pull' fails, somebody will figure out that they were doing a merge, so 'git pull --merge' is what they want to type from now on. 2) Git itself would be warning them for months that a 'non fast-forward was found, and a merge will be done for them', so when the warning turns to an error, they'll know they want a merge, so they'll do 'git pull --merge', either because the warning told them that's git was doing all along, or because they figured that out by googling, or reading the man page, or whatever. Either way, it would not be a big deal for these people, their user-experience wouldn't be totally broken by this proposed change, and that is the important conclusion. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
From: Junio C Hamano gits...@pobox.com Sent: Sunday, September 08, 2013 7:10 PM Richard Hansen rhan...@bbn.com writes: On 2013-09-07 22:41, Felipe Contreras wrote: On Wed, Sep 4, 2013 at 5:59 PM, Junio C Hamano gits...@pobox.com wrote: Which can be solved by adding the above fail option, and then renaming them to pull.integrate and branch.name.integrate to clarify what these variables are about (it is no longer do you rebase or not---if you choose not to rebase, by definition you are going to merge, as there is a third choice to fail), while retaining pull.rebase and branch.name.rebase as a deprecated synonym. All these names are completely unintuitive. First of all, why integrate? Integrate what to what? And then, why fail? Fail on what circumstances? Always? My proposal that does: pull.mode = merge/rebase/merge-ff-only Is way more intuitive. +1 What about something like: pull.mergeoptions (defaults to --ff-only) pull.rebaseoptions (defaults to empty? --preserve-merges?) branch.name.pull.mergeoptions (defaults to pull.mergeoptions) branch.name.pull.rebaseoptions (defaults to pull.rebaseoptions) As pull has two distinct phases fetch and merge/rebase, your mergeoptions/rebaseoptions is much better than mode, which does not tell which phase of pull the mode refers to. It is clear that they apply to the process to integrate the history obtained from the other side and your own history into one history. But it does not help Philip's case, if I understand correctly, where running git pull on some branches is always a mistake Not quite always, it's when it won't fast forward and the user wants it to stop at fetch the history and objects needed to complete the history from the other side phase without proceeding to the then integrate the history from the other side and the history of your branch into one step, Yes, it/Git should stop and wait for instructions... which may be done with either merge or rebase. Here I would typically rebase onto an adjusted destination, e.g. onto pu, or maybe next, rather than master (or vice versa depending on expectations). That is its a feature branch that needs to decide what it's on top of (well, I need to decide ;-) Even if we ignore that always fail, do not do anything use case, your two seemingly independent mergeoptions and rebaseoptions do not tell us which one is preferred between merge and rebase. A single pull.someoption = rebase | merge [| always-fail] makes that choice in a clear way, I think. or 'fail on non-ff' (which may or may not be the users, or Git's default, as per the series title ;-) Regarding the verb integrate. We used to explain pull is a fetch followed by a merge. With more people using git pull --rebase, the word merge used in that explanation of pull stopped being generic enough. Simplarily the upstream branch of local branch X is what you fetch and merge to update the branch X but that 'merge' can be 'rebase'. We needed a verb to call the process of integrate the two histories into one. git pull --help since 153d7265 (pull: change the description to integrate changes, 2013-07-07) uses that verb [*1*]. And that is where the name of the single configuration to pick how to integrate the history obtained by the first phase of pull came from. [Footnote] *1* I suspect that there may still be places in the documentation that have not been updated since the days back when the only valid way to integrate two lines of histories was to merge, and updating them may be a low-hanging fruit. Hint, hint. -- Philip -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
On Sun, Sep 8, 2013 at 1:10 PM, Junio C Hamano gits...@pobox.com wrote: pull.someoption = rebase | merge [| always-fail] makes that choice in a clear way, I think. Regarding the verb integrate. I doubt anybody thinks of pull being an integration, and even if it is, it's still doesn't explain what 'integration = merge' means. To be human friendly you would need to say 'integration-type' or 'integration-kind', or 'integration-mode', then a human would understand, oh yeah, the mode I'm using to integrated is a merge, got ya'. But why bother with yet another useless concept the user has to learn? The user doesn't need to learn about this concept of integration, all the user wants is to map: git pull --rebase = pull.name = rebase git pull --merge pull.name = merge That's it. And my proposed name, 'mode' does the trick just fine. pull.mode = rebase | merge | merge-no-ff -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] sequencer: trivial cleanup
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- sequencer.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/sequencer.c b/sequencer.c index 351548f..8ed9f98 100644 --- a/sequencer.c +++ b/sequencer.c @@ -466,10 +466,7 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit) empty_commit = is_original_commit_empty(commit); if (empty_commit 0) return empty_commit; - if (!empty_commit) - return 0; - else - return 1; + return empty_commit ? 0 : 1; } static int do_pick_commit(struct commit *commit, struct replay_opts *opts) -- 1.8.4.100.gde18f6d.dirty -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git tag output order is incorrect (IMHO)
On Thu, Jul 18, 2013 at 10:27 AM, Rahul Bansal rahul.ban...@rtcamp.com wrote: I am posting here first time, so please excuse me if this is not right place to send something like this. Please check - http://stackoverflow.com/questions/6091306/can-i-make-git-print-x-y-z-style-tag-names-in-a-sensible-order And also - https://github.com/gitlabhq/gitlabhq/issues/4565 IMHO git tag is expected to show tag-list ordered by versions. It may be case, that people do not follow same version numbering convention. Most people after x.9.x increment major version (that is why they may not be affected because of this) Another option like git tag --date-asc can be added which will print tags by creation date. (As long as people do not create backdated tag, this will work). I completely agree, and there was a proposal to an option like this a long time ago: http://article.gmane.org/gmane.comp.version-control.git/111032 -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sequencer: trivial cleanup
On Sun, Sep 8, 2013 at 5:42 PM, Ramkumar Ramachandra artag...@gmail.com wrote: Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- sequencer.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/sequencer.c b/sequencer.c index 351548f..8ed9f98 100644 --- a/sequencer.c +++ b/sequencer.c @@ -466,10 +466,7 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit) empty_commit = is_original_commit_empty(commit); if (empty_commit 0) return empty_commit; - if (!empty_commit) - return 0; - else - return 1; + return empty_commit ? 0 : 1; } Isn't it the other way around? Moreover, 'return !!empty_commit;' would be simpler. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] reset: add --stage and --work options
On Sun, Sep 8, 2013 at 5:05 PM, Ramkumar Ramachandra artag...@gmail.com wrote:
Felipe Contreras wrote:
@@ -290,6 +294,22 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
hashcpy(sha1, tree-object.sha1);
}
+ if (stage = 0 || working_tree = 0) {
+ if (reset_type != NONE)
+ die(_(--{stage,work} are incompatible with
--{hard,mixed,soft,merge}));
+
+ if (working_tree == 1) {
+ if (stage == 0)
+ die(_(--no-stage doesn't make sense with
--work));
+ reset_type = HARD;
+ } else {
+ if (stage == 1)
+ reset_type = NONE;
+ else
+ reset_type = SOFT;
+ }
+ }
+
Not making sense at this point. Why does --stage set a reset_type?
Yeah, we would need another patch to cleanup the variable names, but
for now it's better to minimize the changes.
Either way it doesn't matter because Junio is determined to stand
alone versus the rest of the world in this issue.
--
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sequencer: trivial cleanup
Felipe Contreras wrote: sequencer.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/sequencer.c b/sequencer.c index 351548f..8ed9f98 100644 --- a/sequencer.c +++ b/sequencer.c @@ -466,10 +466,7 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit) empty_commit = is_original_commit_empty(commit); if (empty_commit 0) return empty_commit; - if (!empty_commit) - return 0; - else - return 1; + return empty_commit ? 0 : 1; } Isn't it the other way around? Moreover, 'return !!empty_commit;' would be simpler. Yeah, thanks for pointing out this grave stupidity. This seems to be inconsequential as far as the tests are concerned: have to do some major yak shaving. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
Felipe Contreras wrote: On Sun, Sep 8, 2013 at 1:10 PM, Junio C Hamano gits...@pobox.com wrote: pull.someoption = rebase | merge [| always-fail] makes that choice in a clear way, I think. The core issue is that users rarely want to merge locally: that's the maintainer's job. Users simply want to rebase, and develop on different branches that they will rebase onto origin. I like Felipe's idea for using a pull.mode. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] branch: use $curr_branch_short more
On Sun, Sep 8, 2013 at 10:21 AM, René Scharfe l@web.de wrote: Am 31.08.2013 19:20, schrieb Felipe Contreras: A summary should contain as much information that would allow me to skip the commit message as possible. If I can't tell from the summary if I can safely skip the commit message, the summary is not doing a good job. trivial simplification explains the what, and the why at the same time, and allows most people to skip the commit message, thus is a good summary. No patch should be skipped on the mailing list. As you wrote, trivial patches can still be wrong. What patches each persons skips is each person's own decision, don't be patronizing, if I want to skip a trivial patch, I will, I can't read each and every patch from the dozens of mailing lists I'm subscribed to, and there's no way each and every reader is going to read each and every patch. They should be prioritized, and trivial ones can be safely skipped by most people. Here's a good example from a simple summary that I didn't write: t2024: Fix inconsequential typos http://article.gmane.org/gmane.comp.version-control.git/234038 Do I have to read this patch? No. I know it's inconsequential, I can safely skip it, the key word being *I*. *Somebody* should read it, sure, and I'm sure at least Junio would, but I don't need to. When going through the history I can see that quickly recognizing insubstantial changes is useful, but if I see a summary twice then in my mind forms a big question mark -- why did the same thing had to be done yet again? As an example, both 0d12e59f (pull: replace unnecessary sed invocation) and bc2bbc45 (pull, rebase: simplify to use die()) could arguably have had the summary trivial simplification, but I'm glad the author went with something a bit more specific. Well I wont. Because it takes long to read, and after reading I still don't don't if they are trivial or not, I might actually have to read the commit message, but to be honest, I would probably go right into the diff itself, because judging from Git's history, chances are that somebody wrote a novel there with the important bit I'm looking for just at the end, to don't ruin the suspense. In the first commit, it's saying it's a single invocation, so I take it it's trivial, but what is it replaced with? Is the code simpler, is it more complex? I don't know, I'm still not being told *why* that patch is made. It says 'unnecessary' but why is it unnecessary? In the second commit, it's saying it's a simplification, but I don't know if it's just one instance, or a thousand, so I don't know what would be the impact of the patch. Either way I'm forced to read more just to know if it was safe for me to skip them, at which point the whole purpose of a summary is defeated. Again, triviality and correctness are two separate different things. The patch is trivial even if you can't judge it's correctness. Well, in terms of impact I agree. No, in all terms. A patch can be: Trivial and correct Trivial and incorrect Non-trivial and correct Non-trivial and incorrect To me, what you are describing is an obvious patch, not a trivial one. An obvious patch is so obvious that you can judge it's correctness easily by looking at the diff, a trivial one is of little importance. That's one definition; I think I had the mathematical notion in mind that calls proofs trivial which are immediately evident. We are not talking about mathematics, we are talking about the English human language. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
On Sun, Sep 8, 2013 at 1:33 PM, Junio C Hamano gits...@pobox.com wrote: Jeff King p...@peff.net writes: Calling the abstraction test_diff might have avoided the wasted brain bandwidth in this thread, but I do not think renaming it in test-lib-functions.sh is worth the trouble, either ;-) Yes, but then it wouldn't be clear what's the main purpose of test_diff(). Primarily what we want is to check that they are the same. The diff is secondary, and it's not actually *needed*. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
On Sun, Sep 8, 2013 at 12:02 AM, Jeff King p...@peff.net wrote: On Sat, Sep 07, 2013 at 11:52:10PM -0500, Felipe Contreras wrote: Ah, you mean if you think that the compare function should behave like C *_cmp functions, it should be A-B. Perhaps it is simply that I do not think of the function in those terms, but more like show me the differences from B to A. But that is the problem, you are unable to ignore the implementation. You don't see test_cmp(), you see 'diff -u'. Yes, I already said earlier in the thread: I certainly think of test_cmp A B as differences from A to B, and the order makes sense. IOW, the test_cmp is diff abstraction is leaky, and that is fine (if it were not leaky, then order would not matter at all, but it clearly does). Then I don't think you can give a fair and objective opinion about what should be the ideal ordering of the arguments. You would be forever biased to whatever is the order of 'diff -u'. And I do not think it is a problem. The point of the function is not to abstract away the idea of comparison. The point is to give a hook for people on systems without diff -u to run the test suite. The point according to whom? I say it's the other way around. John mentioned JUnit, NUnit, and PHPUnit earlier in the thread. I believe that Ruby's Test::Unit::Assertions also has assert_equal(expected, actual). That's because they all do first expect, then actual. assert_equal( expected, actual, failure_message = nil ) assert_not_equal( expected, actual, failure_message = nil ) That's why. I do not see any reason why not_equal would not also work as assert_not_equal(actual, expected). Maybe I am missing your point. All right, maybe it's because Ruby started in Japan, and their sentences have very different orderings, maybe it's legacy from another test framework, maybe there's no reason at all and somebody just randomly picked them. Either way the fact that others are doing it differently doesn't mean that's the best way, that would be argumentum ad populum, and mothers are keen to remind us that if other kids are jumping the bridge, that doesn't mean we should too. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Re: [PATCH 3/4] t: rev-parse-parents: avoid yoda conditions
On Sun, Sep 08, 2013 at 06:25:45PM -0500, Felipe Contreras wrote: And I do not think it is a problem. The point of the function is not to abstract away the idea of comparison. The point is to give a hook for people on systems without diff -u to run the test suite. The point according to whom? I say it's the other way around. The point according to 82ebb0b (add test_cmp function for test scripts, 2008-03-12). I wish I had simply called it test_diff back then, and then this conversation could have never occurred. Either way the fact that others are doing it differently doesn't mean that's the best way, that would be argumentum ad populum, and mothers are keen to remind us that if other kids are jumping the bridge, that doesn't mean we should too. Did I once say my way of thinking about it is the best way? No. I said I think it is a matter of preference. I mentioned other systems using a particular ordering to show that the set of people who prefer it the other way is non-zero. Feel free to respond, but I have no interest in discussing this any further with you. This thread has become a giant time sink, and I have nothing else to say on the matter that I have not already said. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
On Sun, Sep 08, 2013 at 05:38:50PM -0500, Felipe Contreras wrote: Yeah, but the key question at hand in this discussion is; what happens when 'git pull' stops working for them, and they don't know what to do, will they choose 'git pull --rebase' by mistake? I agree, they will not choose git pull --rebase by mistake. I say the answer is no, because: 1) As you say in your scenario, somebody is telling these guys what to do, so when 'git pull' fails, somebody will figure out that they were doing a merge, so 'git pull --merge' is what they want to type from now on. Yes, that would be me. My hesitance here is that as the one usually driving git updates (which so far have happened once a year), I will end up retraining forty developers. I don't think the current behavior is broken or really problematic at all: merging has always been the default, and people have come to expect that. People using workflows that don't want merge have always either needed to set a configuration option or use --rebase. As the man page says, --rebase is unsafe, and that's why it's not the default. I would be much less unhappy with your earlier change that did not affect uses with arguments. That would limit the number of use cases affected. 2) Git itself would be warning them for months that a 'non fast-forward was found, and a merge will be done for them', so when the warning turns to an error, they'll know they want a merge, so they'll do 'git pull --merge', either because the warning told them that's git was doing all along, or because they figured that out by googling, or reading the man page, or whatever. Again, you assume that git updates happen on a regular basis, and you assume that most developers really know what happens under the hood. I don't see a warning now; in fact, I see: vauxhall ok % git status # On branch master # Your branch and 'upstream/master' have diverged, # and have 1 and 128 different commits each, respectively. # (use git pull to merge the remote branch into yours) # The current behavior of git is to explicitly encourage this behavior, and now you want to make it not work. I think this change is a bad idea, and I think the number of changes required to the test suite indicates that. If there's going to be a change here, it should have a deprecation period with the above message changed and appropriate warnings, not a flag day; your patches don't do that. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [PATCH 1/3] reset: add --stage and --work options
Felipe Contreras wrote: Either way it doesn't matter because Junio is determined to stand alone versus the rest of the world in this issue. Which is why he's the maintainer. Send in incremental updates, and he should be fine. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sequencer: trivial cleanup
Hi, On Sun, Sep 08, 2013 at 05:53:19PM -0500, Felipe Contreras wrote: On Sun, Sep 8, 2013 at 5:42 PM, Ramkumar Ramachandra artag...@gmail.com wrote: Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- sequencer.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/sequencer.c b/sequencer.c index 351548f..8ed9f98 100644 --- a/sequencer.c +++ b/sequencer.c @@ -466,10 +466,7 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit) empty_commit = is_original_commit_empty(commit); if (empty_commit 0) return empty_commit; - if (!empty_commit) - return 0; - else - return 1; + return empty_commit ? 0 : 1; } Isn't it the other way around? Moreover, 'return !!empty_commit;' would be simpler. Considering the possible return values from is_original_commit_empty(), I think all this could be written as return is_original_commit_empty(commit); Best, Gábor -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
On Sun, Sep 8, 2013 at 7:01 PM, brian m. carlson sand...@crustytoothpaste.net wrote: On Sun, Sep 08, 2013 at 05:38:50PM -0500, Felipe Contreras wrote: Yeah, but the key question at hand in this discussion is; what happens when 'git pull' stops working for them, and they don't know what to do, will they choose 'git pull --rebase' by mistake? I agree, they will not choose git pull --rebase by mistake. I say the answer is no, because: 1) As you say in your scenario, somebody is telling these guys what to do, so when 'git pull' fails, somebody will figure out that they were doing a merge, so 'git pull --merge' is what they want to type from now on. Yes, that would be me. My hesitance here is that as the one usually driving git updates (which so far have happened once a year), I will end up retraining forty developers. I don't think the current behavior is broken or really problematic at all: merging has always been the default, and people have come to expect that. It may not be broken for you, but it is for other people. Would you be so egocentric as to ignore everybody else because it works for you? People using workflows that don't want merge have always either needed to set a configuration option or use --rebase. As the man page says, --rebase is unsafe, and that's why it's not the default. Yes, but the problem is that people using other workflows end up avoiding 'git pull' at all, so at the end of the day we have one core command that the majority of users avoid, that's not good. I would be much less unhappy with your earlier change that did not affect uses with arguments. That would limit the number of use cases affected. I have no problem with: git pull $remote $branch Allowing non-fast-forward merges. And: git pull $remote git pull Not allowing them by default. But the problem is that it's not easy to implement. Either way, I'll venture that you don't want 'git pull $remote' to change, so it would be a waste of the time to try to get the above to work. 2) Git itself would be warning them for months that a 'non fast-forward was found, and a merge will be done for them', so when the warning turns to an error, they'll know they want a merge, so they'll do 'git pull --merge', either because the warning told them that's git was doing all along, or because they figured that out by googling, or reading the man page, or whatever. Again, you assume that git updates happen on a regular basis, and you assume that most developers really know what happens under the hood. No. The developers don't have to know what happens under the hood, Git would be telling them WARNING: we are doing a merge, what else is the developer to think, but that 'git pull' is doing a merge? As for the updates, yes, I assume updates happen at least each three months. If your company updates each year, I don't see what much more we can do to you help you. Doing a single change per year is certainly going to hold the project back. Fortunately this was only point 2), there's still point 1); you can tell them to use 'git pull --merge' from now on, and since you update once every year, you can do it while you give the training for the year. Or there's another option: 3) Distribute Git in your company with /etc/gitconfig having pull.mode = merge. This way nothing will change. I think we are being very accommodating to your company's use-case which is very far from the norm. Even in the absolute worst case scenario, you would have to tell people to use 'git pull --merge' instead, is that really so horrible? Should we really halt Git's progress because you would have to tell people to type nine extra characters or run one configuration command? I don't see a warning now; in fact, I see: vauxhall ok % git status # On branch master # Your branch and 'upstream/master' have diverged, # and have 1 and 128 different commits each, respectively. # (use git pull to merge the remote branch into yours) # The current behavior of git is to explicitly encourage this behavior, and now you want to make it not work. Yes, that's why it's a change. I think this change is a bad idea, and I think the number of changes required to the test suite indicates that. If there's going to be a change here, it should have a deprecation period with the above message changed and appropriate warnings, not a flag day; your patches don't do that. My patches pretty much do nothing else but introduce a warning. Nothing is broken, nothing is changed in the test suite: http://article.gmane.org/gmane.comp.version-control.git/233669 You are confusing my proposal with Junio's one. Also, my proposal was to enable this behavior (pull.mode = merge-ff-only) only for Git v2.0, which might happen probably way later than a year from now, so you your users might actually see the warning after all. But yeah, that's _my_ proposal. -- Felipe Contreras -- To unsubscribe from this list: send the line
Re: [PATCH 3/5] config: set errno in numeric git_parse_* functions
On Sun, Sep 8, 2013 at 4:36 AM, Jeff King p...@peff.net wrote: When we are parsing an integer or unsigned long, we use the strto*max functions, which properly set errno to ERANGE if we get a large value. However, we also do further range checks after applying our multiplication factor, but do not set ERANGE. This means that a caller cannot tell if an error was caused by ERANGE or if the input was simply not a valid number. This patch teaches git_parse_signed and git_parse_unsigned reliably set ERANGE for range errors, and EINVAL for other Missing to: s/reliably/to reliably/ Or, if you don't like splitting the infinitive: s/reliably set ERANGE/to set ERANGE reliably/ errors. Signed-off-by: Jeff King p...@peff.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Reject non-ff pulls by default
On Sun, Sep 8, 2013 at 7:29 PM, Felipe Contreras felipe.contre...@gmail.com wrote: My patches pretty much do nothing else but introduce a warning. Nothing is broken, nothing is changed in the test suite: http://article.gmane.org/gmane.comp.version-control.git/233669 You are confusing my proposal with Junio's one. Actually my mistake. My patches don't even add a warning, so nothing is changed at all (unless you manually configure pull.mode = merge-ff-only). I only suggested to add the warning, but didn't actually implement it. I'll do that soon. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
