Re: What's cooking in git.git (Dec 2013, #05; Thu, 26)
On Thu, Dec 26, 2013 at 4:08 PM, Junio C Hamano gits...@pobox.com wrote: Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [New Topics] Would $gmane/239575 [1] be of interest for New Topics? [1]: http://article.gmane.org/gmane.comp.version-control.git/239575/ -- 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: What's cooking in git.git (Dec 2013, #05; Thu, 26)
On Thu, Jan 2, 2014 at 4:11 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: On Fri, Dec 27, 2013 at 5:13 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: On Thu, Dec 26, 2013 at 4:08 PM, Junio C Hamano gits...@pobox.com wrote: [New Topics] Would $gmane/239575 [1] be of interest for New Topics? [1]: http://article.gmane.org/gmane.comp.version-control.git/239575/ Actually I was planning to scoop it up directly to master but forgot to do so. Make sense. Running git diff maint pu -- name-hash.c shows that we have added a comment that mentions index_name_exists---that needs to be adjusted, too, by the way. Oops, yes, I had noticed that too when testing atop 'pu' but then forgot about it when preparing the patch for submission on 'master'. I'm not sure how to move forward with this now that kb/fast-hashmap, with which it has a textual conflict, has graduated to 'next'. Should this become a two-patch series with one for scooping directly to 'master' and one for 'next' to sit atop kb/fast-hashmap? (But how will the textual conflict be handled?) I have a feeling that a small unused helper function is not a huge breakage that needs to be immediately fixed, so a single patch as a clean-up on top of whatever is cooking on 'next' should be the best approach, I would think. Sounds good. Thanks. -- 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] name-hash: retire unused index_name_exists()
db5360f3f496 (name-hash: refactor polymorphic index_name_exists();
2013-09-17) split index_name_exists() into index_file_exists() and
index_dir_exists() but retained index_name_exists() as a thin wrapper
to avoid disturbing possible in-flight topics. Since this change
landed in 'master' some time ago and there are no in-flight topics
referencing index_name_exists(), retire it.
Signed-off-by: Eric Sunshine sunsh...@sunshineco.com
---
The only difference from v1 [1] is that a comment added by
kb/fast-hashmap in 'next' referencing obsolete index_name_exists()
is also adjusted.
[1]: http://article.gmane.org/gmane.comp.version-control.git/239575/
cache.h | 2 --
name-hash.c | 9 +
2 files changed, 1 insertion(+), 10 deletions(-)
diff --git a/cache.h b/cache.h
index 2a21fbc..d0d1f2b 100644
--- a/cache.h
+++ b/cache.h
@@ -316,7 +316,6 @@ extern void free_name_hash(struct index_state *istate);
#define ce_modified(ce, st, options) ie_modified(the_index, (ce), (st),
(options))
#define cache_dir_exists(name, namelen) index_dir_exists(the_index, (name),
(namelen))
#define cache_file_exists(name, namelen, igncase)
index_file_exists(the_index, (name), (namelen), (igncase))
-#define cache_name_exists(name, namelen, igncase)
index_name_exists(the_index, (name), (namelen), (igncase))
#define cache_name_is_other(name, namelen) index_name_is_other(the_index,
(name), (namelen))
#define resolve_undo_clear() resolve_undo_clear_index(the_index)
#define unmerge_cache_entry_at(at) unmerge_index_entry_at(the_index, at)
@@ -466,7 +465,6 @@ extern int unmerged_index(const struct index_state *);
extern int verify_path(const char *path);
extern struct cache_entry *index_dir_exists(struct index_state *istate, const
char *name, int namelen);
extern struct cache_entry *index_file_exists(struct index_state *istate, const
char *name, int namelen, int igncase);
-extern struct cache_entry *index_name_exists(struct index_state *istate, const
char *name, int namelen, int igncase);
extern int index_name_pos(const struct index_state *, const char *name, int
namelen);
#define ADD_CACHE_OK_TO_ADD 1 /* Ok to add */
#define ADD_CACHE_OK_TO_REPLACE 2 /* Ok to replace file/directory */
diff --git a/name-hash.c b/name-hash.c
index 9a3bd3f..97444d0 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -115,7 +115,7 @@ static int cache_entry_cmp(const struct cache_entry *ce1,
{
/*
* For remove_name_hash, find the exact entry (pointer equality); for
-* index_name_exists, find all entries with matching hash code and
+* index_file_exists, find all entries with matching hash code and
* decide whether the entry matches in same_name.
*/
return remove ? !(ce1 == ce2) : 0;
@@ -227,13 +227,6 @@ struct cache_entry *index_file_exists(struct index_state
*istate, const char *na
return NULL;
}
-struct cache_entry *index_name_exists(struct index_state *istate, const char
*name, int namelen, int icase)
-{
- if (namelen 0 name[namelen - 1] == '/')
- return index_dir_exists(istate, name, namelen - 1);
- return index_file_exists(istate, name, namelen, icase);
-}
-
void free_name_hash(struct index_state *istate)
{
if (!istate-name_hash_initialized)
--
1.8.3.2
--
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] git p4: work around p4 bug that causes empty symlinks
On Tue, Jan 21, 2014 at 6:16 PM, Pete Wyckoff p...@padd.com wrote: Damien Gérard highlights an interesting problem. Some p4 repositories end up with symlinks that have an empty target. It is not possible to create this with current p4, but they do indeed exist. The effect in git p4 is that p4 print on the symlink returns an empty string, confusing the curret symlink-handling code. Such broken repositories cause problems in p4 as well, even with no git involved. In p4, syncing to a change that includes a bogus symlink causes errors: //depot/empty-symlink - updating /home/me/p4/empty-symlink rename: /home/me/p4/empty-symlink: No such file or directory and leaves no symlink. In git, replicate the p4 behavior by ignoring these bad symlinks. If, in a later p4 revision, the symlink happens to point to something non-null, the symlink will be replaced properly. Add a big test for all this too. This happens to be a regression introduced by 1292df1 (git-p4: Fix occasional truncation of symlink contents., 2013-08-08) and appeared first in 1.8.5. But it only shows up only in p4 Redundant only. repositories of dubious character, so can wait for a proper release. Tested-by: Damien Gérard dam...@iwi.me Signed-off-by: Pete Wyckoff p...@padd.com -- 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 06/11] git p4 test: run as user author
On Tue, Jan 21, 2014 at 6:16 PM, Pete Wyckoff p...@padd.com wrote: The tests use aut...@example.com as the canonical submitter, but he does not have an entry in the p4 users database. This causes the generated change description to complain that the git and p4 users disagree. The complaint message is still valid, just isn't useful in tests. It was was s/was was/was/ introduced in 848de9c (git-p4: warn if git authorship won't be retained, 2011-05-13). Fix t9813 to use @example.com instead of @localhost due to change in p4_add_user(). Move the function into the git p4 test library so author can be added at initialization time. Signed-off-by: Pete Wyckoff p...@padd.com -- 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 6/9] Pass directory indicator to match_pathspec_item()
On Fri, Jan 24, 2014 at 8:40 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh index af5134b..167af53 100755 --- a/t/t4010-diff-pathspec.sh +++ b/t/t4010-diff-pathspec.sh @@ -110,4 +110,21 @@ test_expect_success 'diff-tree -r with wildcard' ' test_cmp expected result ' +test_expect_success 'setup submodules' ' + test_tick + git init submod + ( cd submod test_commit first; ) Unnecessary semicolon might confuse the reader into thinking something unusual is going on here. + git add submod + git commit -m first + ( cd submod test_commit second; ) Ditto. + git add submod + git commit -m second +' + +test_expect_success 'diff-cache ignores trailing slash on submodule path' ' + git diff --name-only HEAD^ submod expect + git diff --name-only HEAD^ submod/ actual + test_cmp expect actual +' + test_done -- 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] doc: remote author/documentation sections from more pages
On Sun, Jan 26, 2014 at 6:43 PM, Michael Haggerty mhag...@alum.mit.edu wrote: Subject: [PATCH] doc: remote author/documentation sections from more pages s/remote/remove/ We decided at 48bb914e (doc: drop author/documentation sections from most pages, 2011-03-11) to remove author and documentation sections from our documentation. Remove a few stragglers. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu -- 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 17/17] Documentation: add documentation for 'git interpret-trailers'
On Sun, Jan 26, 2014 at 12:00 PM, Christian Couder chrisc...@tuxfamily.org wrote: Signed-off-by: Christian Couder chrisc...@tuxfamily.org --- diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt new file mode 100644 index 000..f74843e --- /dev/null +++ b/Documentation/git-interpret-trailers.txt @@ -0,0 +1,137 @@ +git-interpret-trailers(1) += + +NAME + +git-interpret-trailers - help add stuctured information into commit messages + +SYNOPSIS + +[verse] +'git interpret-trailers' [--trim-empty] [--infile=file] [token[=value]...] Would it be more consistent with existing documentation to format this as so? [--infile=file] [token[=value]]... +DESCRIPTION +--- +Help add RFC 822 like headers, called 'trailers', at the end of the s/822 like/822-like/ Was the suggestion, early in the discussion, to use footer rather than trailer dismissed? +otherwise free-form part of a commit message. + +Unless `--infile=file` is used, this command is a filter. It reads the +standard input for a commit message and apply the `token` arguments, s/apply/applies/ +if any, to this message. The resulting message is emited on the +standard output. + +Some configuration variables control the way the `token` arguments are +applied to the message and the way any existing trailer in the message +is changed. They also make it possible to automatically add some +trailers. + +By default, a 'token=value' or 'token:value' argument will be added +only if no trailer with the same (token, value) pair is already in the +message. The 'token' and 'value' parts will be trimmed to remove +starting and trailing white spaces, and the resulting trimmed 'token' Other git documentation uniformly spells it as whitespace rather than white spaces. +and 'value' will appear in the message like this: + + +token: value + + +By default, if there are already trailers with the same 'token' the +trailer will appear just after the last trailer with the same It might be a bit clearer to say the _new_ trailer will appear +'token'. Otherwise it will appear at the end of the message. + +Note that 'trailers' do not follow and are not intended to follow many +rules that are in RFC 822. For example they do not follow the line +breaking rules, the encoding rules and probably many other rules. + +Trailers have become a de facto standard way to add helpful structured +information into commit messages. For example the well known +Signed-off-by: trailer is used by many projects like the Linux +kernel and Git. This justification paragraph might make more sense near or at the very the top of the Description section. +OPTIONS +--- +--trim-empty:: + If the 'value' part of any trailer contains onlywhite spaces, s/onlywhite spaces/only whitespace/ + the whole trailer will be removed from the resulting message. + +infile=file:: + Read the commit message from `file` instead of the standard + input. + +CONFIGURATION VARIABLES +--- + +trailer.token.key:: + This 'key' will be used instead of 'token' in the + trailer. After some alphanumeric characters, it can contain + some non alphanumeric characters like ':', '=' or '#' that will + be used instead of ':' to separate the token from the value in + the trailer, though the default ':' is more standard. + +trailer.token.where:: + This can be either `after`, which is the default, or + `before`. If it is `before`, then a trailer with the specified + token, will appear before, instead of after, other trailers + with the same token, or otherwise at the beginning, instead of + at the end, of all the trailers. + +trailer.token.ifexist:: + This option makes it possible to chose what action will be s/chose/choose/ + performed when there is already at least one trailer with the + same token in the message. ++ +The valid values for this option are: `addIfDifferent` (this is the +default), `addIfDifferentNeighbor`, `add`, `overwrite` or `doNothing`. ++ +With `addIfDifferent`, a new trailer will be added only if no trailer +with the same (token, value) pair is already in the message. ++ +With `addIfDifferentNeighbor`, a new trailer will be added only if no +trailer with the same (token, value) pair is above or below the line +where the new trailer will be added. ++ +With `add`, a new trailer will be added, even if some trailers with +the same (token, value) pair are already in the message. ++ +With `overwrite`, the new trailer will overwrite an existing trailer +with the same token. ++ +With `doNothing`, nothing will be done, that is no new trailer will be +added if there is already one with the same token in the
Re: [PATCH v5 1/4] submodule: Make 'checkout' update_module explicit
On Sun, Jan 26, 2014 at 3:45 PM, W. Trevor King wk...@tremily.us wrote: This avoids the current awkwardness of having either '' or 'checkout' for checkout-mode updates, which makes testing for checkout-mode updates (or non-checkout-mode updates) easier. Signed-off-by: W. Trevor King wk...@tremily.us --- git-submodule.sh | 27 +++ 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 5247f78..5e8776c 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -803,17 +803,10 @@ cmd_update() update_module=$update else update_module=$(git config submodule.$name.update) - case $update_module in - '') - ;; # Unset update mode - checkout | rebase | merge | none) - ;; # Known update modes - !*) - ;; # Custom update command - *) - die $(eval_gettext Invalid update mode '$update_module' for submodule '$name') - ;; - esac + if test -z $update_module + then + update_module=checkout Here, you (unnecessarily) quote 'checkout'... + fi fi displaypath=$(relative_path $prefix$sm_path) @@ -882,11 +875,16 @@ Maybe you want to use 'update --init'?) case ;$cloned_modules; in *;$name;*) # then there is no local change to integrate - update_module= ;; + update_module=checkout ;; But here you use bare (unquoted) 'checkout'. Bare is probably more idiomatic. esac must_die_on_failure= case $update_module in + checkout) + command=git checkout $subforce -q + die_msg=$(eval_gettext Unable to checkout '\$sha1' in submodule path '\$displaypath') + say_msg=$(eval_gettext Submodule path '\$displaypath': checked out '\$sha1') + ;; rebase) command=git rebase die_msg=$(eval_gettext Unable to rebase '\$sha1' in submodule path '\$displaypath') @@ -906,10 +904,7 @@ Maybe you want to use 'update --init'?) must_die_on_failure=yes ;; *) - command=git checkout $subforce -q - die_msg=$(eval_gettext Unable to checkout '\$sha1' in submodule path '\$displaypath') - say_msg=$(eval_gettext Submodule path '\$displaypath': checked out '\$sha1') - ;; + die $(eval_gettext Invalid update mode '$update_module' for submodule '$name') esac if (clear_local_git_env; cd $sm_path $command $sha1) -- 1.8.5.2.8.g0f6c0d1 -- 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 5/5] builtin/commit.c: reduce scope of variables
On Wed, Jan 29, 2014 at 8:48 AM, Elia Pinto gitter.spi...@gmail.com wrote:
diff --git a/builtin/commit.c b/builtin/commit.c
index 3767478..eea4421 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1510,7 +1511,6 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
struct ref_lock *ref_lock;
struct commit_list *parents = NULL, **pptr = parents;
struct stat statbuf;
- int allow_fast_forward = 1;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
@@ -1576,6 +1576,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
}
fclose(fp);
strbuf_release(m);
+ int allow_fast_forward = 1;
This introduces a declaration-after-statement, which is frowned upon
in this project.
if (!stat(git_path(MERGE_MODE), statbuf)) {
if (strbuf_read_file(sb, git_path(MERGE_MODE), 0)
0)
die_errno(_(could not read MERGE_MODE));
--
1.7.10.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 v4 01/17] Add data structures and basic functions for commit trailers
On Thu, Jan 30, 2014 at 1:49 AM, Christian Couder
chrisc...@tuxfamily.org wrote:
We will use a doubly linked list to store all information
about trailers and their configuration.
This way we can easily remove or add trailers to or from
trailer lists while traversing the lists in either direction.
Signed-off-by: Christian Couder chrisc...@tuxfamily.org
---
diff --git a/trailer.c b/trailer.c
new file mode 100644
index 000..aed25e1
--- /dev/null
+++ b/trailer.c
@@ -0,0 +1,48 @@
+#include cache.h
+/*
+ * Copyright (c) 2013 Christian Couder chrisc...@tuxfamily.org
+ */
+
+static int same_token(struct trailer_item *a, struct trailer_item *b, int
alnum_len)
+{
+ return !strncasecmp(a-token, b-token, alnum_len);
+}
Maybe these functions defined in the header should all be 'static
inline' rather than just 'static'? Making them inline would be
consistent with functions defined in other git headers.
+
+static int same_value(struct trailer_item *a, struct trailer_item *b)
+{
+ return !strcasecmp(a-value, b-value);
+}
+
+static int same_trailer(struct trailer_item *a, struct trailer_item *b, int
alnum_len)
+{
+ return same_token(a, b, alnum_len) same_value(a, b);
+}
+
+/* Get the length of buf from its beginning until its last alphanumeric
character */
+static size_t alnum_len(const char *buf, size_t len)
+{
+ while (--len = 0 !isalnum(buf[len]));
'len' has type size_t, which is unsigned, so the conditional '--len =
0' will always be true (which will result in a crash if 'buf' contains
no alphanumerics).
+ return len + 1;
+}
--
1.8.5.2.201.gacc5987
--
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] Ensure __BYTE_ORDER is always set
On Thu, Jan 30, 2014 at 4:50 PM, Jeff King p...@peff.net wrote:
I think we could do this with something like the patch below, which
checks two things:
1. When we expand the ewah, it has the same number of bits we claimed
in the on-disk header.
2. The ewah header matches the number of objects in the packfile.
The first catches a corruption in the ewah data itself, and the latter
when the header is corrupted. You can test either by breaking the
endian-swapping. :)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index ae0b57b..a31e529 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -130,6 +131,31 @@ static struct ewah_bitmap *read_bitmap_1(struct
bitmap_index *index)
return NULL;
}
+ /*
+* It's OK for us to have too fewer bits than objects, as the EWAH
s/fewer/few/
+* writer may have simply left off an ending that is all-zeroes.
+*
+* However it's not OK for us to have too many bits, as that would
+* entail touching objects that we don't have. We are careful
+* enough to avoid doing so in later code, but in the case of
+* nonsensical values, we would want to avoid even allocating
+* memory to hold the expanded bitmap.
+*
+* There is one exception: we may go over to round up to the next
+* 64-bit ewah word, since the storage comes in chunks of that size.
+*/
+ expected_bits = index-pack-num_objects;
+ if (expected_bits 63) {
+ expected_bits = ~63;
+ expected_bits += 64;
+ }
+ if (b-bit_size expected_bits) {
+ error(unexpected number of bits in bitmap: %PRIuMAX
%PRIuMAX,
+ (uintmax_t)b-bit_size, (uintmax_t)expected_bits);
+ ewah_pool_free(b);
+ return NULL;
+ }
+
index-map_pos += bitmap_size;
return b;
}
--
--
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 v4 05/17] strbuf: add strbuf_isspace()
On Thu, Jan 30, 2014 at 1:49 AM, Christian Couder
chrisc...@tuxfamily.org wrote:
This helper function checks if a strbuf
contains only space chars or not.
Signed-off-by: Christian Couder chrisc...@tuxfamily.org
---
diff --git a/strbuf.c b/strbuf.c
index 83caf4a..2124bb8 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -124,6 +124,13 @@ void strbuf_ltrim(struct strbuf *sb)
sb-buf[sb-len] = '\0';
}
+int strbuf_isspace(struct strbuf *sb)
+{
+ char *b;
+ for (b = sb-buf; *b isspace(*b); b++);
Quoting from the strbuf documentation:
... strbufs may have embedded NULs. An strbuf is NUL
terminated for convenience, but no function in the
strbuf API actually relies on the string being free of
NULs.
So, the termination condition (*b) of this loop is questionable.
Looping from 0 to sb-len makes more sense.
+ return !*b;
Ditto for the return. This will incorrectly return 'true' if an
embedded NUL is encountered.
+}
+
struct strbuf **strbuf_split_buf(const char *str, size_t slen,
int terminator, int max)
{
--
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 v4 12/17] strbuf: add strbuf_replace()
On Thu, Jan 30, 2014 at 1:49 AM, Christian Couder
chrisc...@tuxfamily.org wrote:
Signed-off-by: Christian Couder chrisc...@tuxfamily.org
---
diff --git a/strbuf.c b/strbuf.c
index 2124bb8..e45e513 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -197,6 +197,13 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t
len,
strbuf_setlen(sb, sb-len + dlen - len);
}
+void strbuf_replace(struct strbuf *sb, const char *a, const char *b)
+{
+ char *ptr = strstr(sb-buf, a);
This could be 'const char *'.
+ if (ptr)
+ strbuf_splice(sb, ptr - sb-buf, strlen(a), b, strlen(b));
+}
+
void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t
len)
{
strbuf_splice(sb, pos, 0, data, len);
diff --git a/strbuf.h b/strbuf.h
index 02bff3a..38faf70 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -111,6 +111,9 @@ extern void strbuf_remove(struct strbuf *, size_t pos,
size_t len);
extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,
const void *, size_t);
+/* first occurence of a replaced with b */
+extern void strbuf_replace(struct strbuf *, const char *a, const char *b);
Updating Documentation/technical/api-strbuf.txt to mention this new
function would be appropriate.
extern void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
size_t size);
extern void strbuf_add(struct strbuf *, const void *, size_t);
--
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 v4 02/17] trailer: process trailers from file and arguments
On Thu, Jan 30, 2014 at 1:49 AM, Christian Couder
chrisc...@tuxfamily.org wrote:
This patch implements the logic that process trailers
from file and arguments.
At the beginning trailers from file are in their own
infile_tok doubly linked list, and trailers from
arguments are in their own arg_tok doubly linked list.
The lists are traversed and when an arg_tok should be
applied, it is removed from its list and inserted
into the infile_tok list.
Signed-off-by: Christian Couder chrisc...@tuxfamily.org
---
diff --git a/trailer.c b/trailer.c
index aed25e1..e9ccfa5 100644
--- a/trailer.c
+++ b/trailer.c
@@ -46,3 +46,192 @@ static size_t alnum_len(const char *buf, size_t len)
+static void apply_arg_if_exist(struct trailer_item *infile_tok,
+ struct trailer_item *arg_tok,
+ int alnum_len)
+{
+ switch (arg_tok-conf-if_exist) {
+ case EXIST_DO_NOTHING:
+ free(arg_tok);
This is freeing arg_tok, but isn't it leaking arg_tok-conf, and
conf-name, conf-key, conf-command? Ditto for all the other
free(arg_tok) invocations elsewhere in the file.
+ break;
+ case EXIST_OVERWRITE:
+ free((char *)infile_tok-value);
+ infile_tok-value = xstrdup(arg_tok-value);
+ free(arg_tok);
+ break;
+ case EXIST_ADD:
+ add_arg_to_infile(infile_tok, arg_tok);
+ break;
+ case EXIST_ADD_IF_DIFFERENT:
+ if (check_if_different(infile_tok, arg_tok, alnum_len, 1))
+ add_arg_to_infile(infile_tok, arg_tok);
+ else
+ free(arg_tok);
+ break;
+ case EXIST_ADD_IF_DIFFERENT_NEIGHBOR:
+ if (check_if_different(infile_tok, arg_tok, alnum_len, 0))
+ add_arg_to_infile(infile_tok, arg_tok);
+ else
+ free(arg_tok);
+ break;
+ }
+}
+
+static void process_infile_tok(struct trailer_item *infile_tok,
+ struct trailer_item **arg_tok_first,
+ enum action_where where)
+{
+ struct trailer_item *arg_tok;
+ struct trailer_item *next_arg;
+
+ int tok_alnum_len = alnum_len(infile_tok-token,
strlen(infile_tok-token));
+ for (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {
+ next_arg = arg_tok-next;
+ if (same_token(infile_tok, arg_tok, tok_alnum_len)
+ arg_tok-conf-where == where) {
+ /* Remove arg_tok from list */
+ remove_from_list(arg_tok, arg_tok_first);
+ /* Apply arg */
+ apply_arg_if_exist(infile_tok, arg_tok,
tok_alnum_len);
Redundant comments (saying the same thing as the code) can make the
code slightly more difficult to read.
+ /*
+* If arg has been added to infile,
+* then we need to process it too now.
+*/
+ if ((where == WHERE_AFTER ? infile_tok-next :
infile_tok-previous) == arg_tok)
+ infile_tok = arg_tok;
+ }
+ }
+}
--
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 v4 04/17] trailer: process command line trailer arguments
On Thu, Jan 30, 2014 at 1:49 AM, Christian Couder
chrisc...@tuxfamily.org wrote:
This patch parses the trailer command line arguments
and put the result into an arg_tok doubly linked
list.
Signed-off-by: Christian Couder chrisc...@tuxfamily.org
---
diff --git a/trailer.c b/trailer.c
index d979a0f..f48fd94 100644
--- a/trailer.c
+++ b/trailer.c
@@ -362,3 +362,80 @@ static int git_trailer_config(const char *conf_key,
const char *value, void *cb)
}
return 0;
}
+
+static void parse_trailer(struct strbuf *tok, struct strbuf *val, const char
*trailer)
+{
+ char *end = strchr(trailer, '=');
This can be 'const char *'.
+ if (!end)
+ end = strchr(trailer, ':');
+ if (end) {
+ strbuf_add(tok, trailer, end - trailer);
+ strbuf_trim(tok);
+ strbuf_addstr(val, end + 1);
+ strbuf_trim(val);
+ } else {
+ strbuf_addstr(tok, trailer);
+ strbuf_trim(tok);
+ }
+}
+
+static struct trailer_item *create_trailer_item(const char *string)
+{
+ struct strbuf tok = STRBUF_INIT;
+ struct strbuf val = STRBUF_INIT;
+ struct trailer_item *new;
+ struct trailer_item *item;
+ int tok_alnum_len;
+
+ parse_trailer(tok, val, string);
+
+ tok_alnum_len = alnum_len(tok.buf, tok.len);
+
+ /* Lookup if the token matches something in the config */
+ for (item = first_conf_item; item; item = item-next) {
+ if (!strncasecmp(tok.buf, item-conf-key, tok_alnum_len) ||
+ !strncasecmp(tok.buf, item-conf-name, tok_alnum_len)) {
+ new = xcalloc(sizeof(struct trailer_item), 1);
sizeof(*new) would be more future-proof.
+ new-conf = item-conf;
+ new-token = xstrdup(item-conf-key);
+ new-value = strbuf_detach(val, NULL);
+ strbuf_release(tok);
+ return new;
+ }
+ }
+
+ new = xcalloc(sizeof(struct trailer_item), 1);
Ditto.
+ new-conf = xcalloc(sizeof(struct conf_info), 1);
+ new-token = strbuf_detach(tok, NULL);
+ new-value = strbuf_detach(val, NULL);
+
+ return new;
+}
--
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 v4 06/17] trailer: parse trailers from input file
On Thu, Jan 30, 2014 at 1:49 AM, Christian Couder
chrisc...@tuxfamily.org wrote:
This patch reads trailers from an input file, parses
them and puts the result into a doubly linked list.
Signed-off-by: Christian Couder chrisc...@tuxfamily.org
---
trailer.c | 62 ++
1 file changed, 62 insertions(+)
diff --git a/trailer.c b/trailer.c
index f48fd94..084b3e1 100644
--- a/trailer.c
+++ b/trailer.c
@@ -439,3 +439,65 @@ static struct trailer_item
*process_command_line_args(int argc, const char **arg
+static void process_input_file(const char *infile,
+ struct trailer_item **infile_tok_first,
+ struct trailer_item **infile_tok_last)
+{
+ struct strbuf **lines = read_input_file(infile);
+ int start = find_trailer_start(lines);
+ int i;
+
+ /* Print non trailer lines as is */
+ for (i = 0; lines[i] i start; i++) {
+ printf(%s, lines[i]-buf);
+ }
+
+ /* Parse trailer lines */
+ for (i = start; lines[i]; i++) {
+ struct trailer_item *new = create_trailer_item(lines[i]-buf);
+ add_trailer_item(infile_tok_first, infile_tok_last, new);
Leaking 'lines'. Perhaps you want to invoke strbuf_list_free() here.
+ }
+}
--
1.8.5.2.201.gacc5987
--
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 v4 10/17] trailer: if no input file is passed, read from stdin
On Thu, Jan 30, 2014 at 1:49 AM, Christian Couder
chrisc...@tuxfamily.org wrote:
It is simpler and more natural if the git interpret-trailers
is made a filter as its output already goes to sdtout.
Signed-off-by: Christian Couder chrisc...@tuxfamily.org
---
diff --git a/trailer.c b/trailer.c
index 8681aed..73a65e0 100644
--- a/trailer.c
+++ b/trailer.c
@@ -464,8 +464,13 @@ static struct strbuf **read_input_file(const char
*infile)
{
struct strbuf sb = STRBUF_INIT;
- if (strbuf_read_file(sb, infile, 0) 0)
- die_errno(_(could not read input file '%s'), infile);
+ if (infile) {
+ if (strbuf_read_file(sb, infile, 0) 0)
+ die_errno(_(could not read input file '%s'),
infile);
+ } else {
+ if (strbuf_read(sb, fileno(stdin), 0) 0)
strbuf_fread(), perhaps?
+ die_errno(_(could not read from stdin));
+ }
return strbuf_split(sb, '\n');
}
@@ -530,10 +535,8 @@ void process_trailers(const char *infile, int
trim_empty, int argc, const char *
git_config(git_trailer_config, NULL);
- /* Print the non trailer part of infile */
- if (infile) {
- process_input_file(infile, infile_tok_first,
infile_tok_last);
- }
+ /* Print the non trailer part of infile (or stdin if infile is NULL)
*/
+ process_input_file(infile, infile_tok_first, infile_tok_last);
arg_tok_first = process_command_line_args(argc, argv);
--
1.8.5.2.201.gacc5987
--
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 v4 11/17] trailer: add new_trailer_item() function
On Thu, Jan 30, 2014 at 1:49 AM, Christian Couder
chrisc...@tuxfamily.org wrote:
This is a small refactoring to prepare for the next steps.
Since this is all brand new code, wouldn't it make more sense to
structure it in this fashion in the first place when introduced in
patch 4/17? It's not clear why it should be introduced with poorer
structure and then later cleaned up.
Signed-off-by: Christian Couder chrisc...@tuxfamily.org
---
trailer.c | 31 +++
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/trailer.c b/trailer.c
index 73a65e0..430ff39 100644
--- a/trailer.c
+++ b/trailer.c
@@ -399,11 +399,27 @@ static void parse_trailer(struct strbuf *tok, struct
strbuf *val, const char *tr
}
}
+static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,
+const char* tok, const char* val)
+{
+ struct trailer_item *new = xcalloc(sizeof(struct trailer_item), 1);
+ new-value = val;
+
+ if (conf_item) {
+ new-conf = conf_item-conf;
+ new-token = xstrdup(conf_item-conf-key);
+ } else {
+ new-conf = xcalloc(sizeof(struct conf_info), 1);
+ new-token = tok;
+ }
+
+ return new;
+}
+
static struct trailer_item *create_trailer_item(const char *string)
{
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
- struct trailer_item *new;
struct trailer_item *item;
int tok_alnum_len;
@@ -415,21 +431,12 @@ static struct trailer_item *create_trailer_item(const
char *string)
for (item = first_conf_item; item; item = item-next) {
if (!strncasecmp(tok.buf, item-conf-key, tok_alnum_len) ||
!strncasecmp(tok.buf, item-conf-name, tok_alnum_len)) {
- new = xcalloc(sizeof(struct trailer_item), 1);
- new-conf = item-conf;
- new-token = xstrdup(item-conf-key);
- new-value = strbuf_detach(val, NULL);
strbuf_release(tok);
- return new;
+ return new_trailer_item(item, NULL,
strbuf_detach(val, NULL));
}
}
- new = xcalloc(sizeof(struct trailer_item), 1);
- new-conf = xcalloc(sizeof(struct conf_info), 1);
- new-token = strbuf_detach(tok, NULL);
- new-value = strbuf_detach(val, NULL);
-
- return new;
+ return new_trailer_item(NULL, strbuf_detach(tok, NULL),
strbuf_detach(val, NULL));;
}
static void add_trailer_item(struct trailer_item **first,
--
1.8.5.2.201.gacc5987
--
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 17/17] Documentation: add documentation for 'git interpret-trailers'
Patch 17/17 of v4 failed to arrive in my inbox for some reason, so I'll just reply to v3 since there's another error I noticed which is still present in v4, plus a comment specific to v4 (see below). On Mon, Jan 27, 2014 at 3:33 PM, Christian Couder chrisc...@tuxfamily.org wrote: From: Eric Sunshine sunsh...@sunshineco.com On Sun, Jan 26, 2014 at 12:00 PM, Christian Couder chrisc...@tuxfamily.org wrote: +By default, a 'token=value' or 'token:value' argument will be added +only if no trailer with the same (token, value) pair is already in the +message. The 'token' and 'value' parts will be trimmed to remove +starting and trailing white spaces, and the resulting trimmed 'token' Other git documentation uniformly spells it as whitespace rather than white spaces. You are right I will change that. The rest of git documentation consistently spells it whitespace, but v4 uses whitespaces. +infile=file:: + Read the commit message from `file` instead of the standard + input. I didn't notice this when reviewing v3, and the same text appears in v4. There are a couple extra hyphens before 'infile', and you want and around file, so: s/infile=file/--infile=file/ -- 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] gitk: refactor: separate generic hunk parsing out of find_hunk_blamespecs{}
On Mon, Feb 3, 2014 at 5:41 PM, Max Kirillov m...@max630.net wrote:
For requesting a region blame, it is necessary to parse a hunk and
find the region in the parent file corresponding to the selected region.
There is already hunk parsin functionality in the find_hunk_blamespec{},
s/parsin/parsing/
s/in the/in/
but returns only information for a single line.
s/but/but it/
The new function, resolve_hunk_lines{}, scans the hunk once and returns
for all hunk lines between $start_diffline and $end_diffline, in which parent
each of them exists and which is its number there.
Signed-off-by: Max Kirillov m...@max630.net
---
gitk | 93
++--
1 file changed, 57 insertions(+), 36 deletions(-)
diff --git a/gitk b/gitk
index dfac4fd..7699a66 100755
--- a/gitk
+++ b/gitk
@@ -3590,11 +3590,11 @@ proc external_diff {} {
}
}
-proc find_hunk_blamespec {base line} {
+proc resolve_hunk_lines {base start_diffline end_diffline} {
global ctext
# Find and parse the hunk header
-set s_lix [$ctext search -backwards -regexp ^@@ $line.0 lineend
$base.0]
+set s_lix [$ctext search -backwards -regexp ^@@ $start_diffline.0
lineend $base.0]
if {$s_lix eq {}} return
set s_line [$ctext get $s_lix $s_lix + 1 lines]
@@ -3614,49 +3614,70 @@ proc find_hunk_blamespec {base line} {
}
# Now scan the lines to determine offset within the hunk
-set max_parent [expr {[llength $base_lines]-2}]
-set dline 0
+set max_parent [expr {[llength $base_lines]-1}]
set s_lno [lindex [split $s_lix .] 0]
-# Determine if the line is removed
-set chunk [$ctext get $line.0 $line.1 + $max_parent chars]
-if {[string match {[-+ ]*} $chunk]} {
- set removed_idx [string first - $chunk]
- # Choose a parent index
- if {$removed_idx = 0} {
- set parent $removed_idx
+set commitlines_by_diffline {}
+array unset commit_lines
+for {set p 0} {$p = $max_parent} {incr p} {
+ set commit_lines($p) [expr [lindex $base_lines $p] - 1]
+}
+for {set diffline [expr $s_lno + 1]} {$diffline = $end_diffline} {incr
diffline} {
+ set chunk [$ctext get $diffline.0 $diffline.0 + $max_parent chars]
+ if {$chunk eq {} || [string match \[\n@\]* $chunk]} {
+ # region is larger than hunk
+ return {}
+ }
+ set is_removed [expr [string first - $chunk] = 0]
+ if {!$is_removed} {
+ incr commit_lines(0)
+ set commitlines [list [list 0 $commit_lines(0)]]
} else {
- set unchanged_idx [string first $chunk]
- if {$unchanged_idx = 0} {
- set parent $unchanged_idx
- } else {
- # blame the current commit
- set parent -1
- }
- }
- # then count other lines that belong to it
- for {set i $line} {[incr i -1] $s_lno} {} {
- set chunk [$ctext get $i.0 $i.1 + $max_parent chars]
- # Determine if the line is removed
- set removed_idx [string first - $chunk]
- if {$parent = 0} {
- set code [string index $chunk $parent]
- if {$code eq - || ($removed_idx 0 $code ne +)} {
- incr dline
+ set commitlines {}
+ }
+ for {set p 1} {$p = $max_parent} {incr p} {
+ switch -- [string index $chunk $p-1] {
+ + {
}
- } else {
- if {$removed_idx 0} {
- incr dline
+ - {
+ incr commit_lines($p)
+ lappend commitlines [list $p $commit_lines($p)]
+ }
+ {
+ if {!$is_removed} {
+ incr commit_lines($p)
+ lappend commitlines [list $p $commit_lines($p)]
+ }
+ }
+ default {
+ error_popup resolve_hunk_lines: unexpected diff
line($diffline): $chunk
+ break
}
}
}
- incr parent
-} else {
- set parent 0
+ if {$diffline = $start_diffline} {
+ lappend commitlines_by_diffline [list $diffline $commitlines]
+ }
}
+return $commitlines_by_diffline
+}
-incr dline [lindex $base_lines $parent]
-return [list $parent $dline]
+proc find_hunk_blamespec {base line} {
+foreach cl_spec [resolve_hunk_lines $base $line $line] {
+ if {[lindex $cl_spec 0] == $line} {
+ set commitlines [lindex $cl_spec 1]
+ if {[llength $commitlines] 0} {
+ if {[llength $commitlines] 1 [lindex $commitlines 0 0]
eq 0} {
+ return [lindex $commitlines 1]
+ } else {
+ return [lindex $commitlines 0]
+
Re: [PATCH 03/13] Makefile: introduce make-var helper function
On Wed, Feb 5, 2014 at 12:50 PM, Jeff King p...@peff.net wrote: It's a common pattern in our Makefile to echo some make variables into a file, but only if they are different from a previous run. This sentinel file can then be used as a dependency to trigger rebuilds when the make variable changes. The code to do this is a bit ugly and repetetive; let's s/repetetive/repetitive/ factor it out into a reusable function. Note that this relies on the call and eval functions of GNU make. We previously avoided using call, as explained in 39c015c (Fixes for ancient versions of GNU make, 2006-02-18). However, it has been 8 years since then, so perhaps its use is acceptable now. The call function dates back to GNU make 3.77.90 (1997-07-21). The eval function dates back to 3.80 (2002-07-08). If it's still a problem to use these functions, we can do similar meta-programming with something like: include magic.mak magic.mak: ./generate-magic-rules $@+ mv $@+ $@ where the rules are generated by a shell (or other) script. 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 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap
On Thu, Feb 6, 2014 at 10:10 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index b0fa738..fb11073 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -200,5 +200,29 @@ EOF ) ' +# This test is tricky. We need large enough haves that fetch-pack +# will put pkt-flush in between. Then we need a have the the server s/the the/that the/ +# does not have, it'll send ACK %s ready +test_expect_success 'add more commits' ' + ( + cd shallow + for i in $(seq 10); do Do you want to use test_seq here? + git checkout --orphan unrelated$i + test_commit unrelated$i /dev/null + git push -q $HTTPD_DOCUMENT_ROOT_PATH/repo.git refs/heads/unrelated$i:refs/heads/unrelated$i + git push -q ../clone/.git refs/heads/unrelated$i:refs/heads/unrelated$i + done + git checkout master + test_commit new + git push $HTTPD_DOCUMENT_ROOT_PATH/repo.git master + ) + ( + cd clone + git checkout --orphan newnew + test_commit new-too + git fetch --depth=2 + ) +' + stop_httpd test_done -- 1.8.5.2.240.g8478abd -- 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 v4 7/9] rebase: parse options in stuck-long mode
On Sun, Feb 9, 2014 at 8:03 PM, brian m. carlson sand...@crustytoothpaste.net wrote: From: Nicolas Vigier bo...@mars-attacks.org There is no functionnal change. The reason for this change is to be able s/functionnal/functional/ to add a new option taking an optional argument. Signed-off-by: Nicolas Vigier bo...@mars-attacks.org Signed-off-by: brian m. carlson sand...@crustytoothpaste.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 v5 14/14] Documentation: add documentation for 'git interpret-trailers'
On Thu, Feb 6, 2014 at 3:20 PM, Christian Couder chrisc...@tuxfamily.org wrote: Signed-off-by: Christian Couder chrisc...@tuxfamily.org --- diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt new file mode 100644 index 000..0617941 --- /dev/null +++ b/Documentation/git-interpret-trailers.txt @@ -0,0 +1,132 @@ +OPTIONS +--- +--trim-empty:: + If the 'value' part of any trailer contains only whitespace, + the whole trailer will be removed from the resulting message. + +infile=file:: + Read the commit message from `file` instead of the standard + input. s//--/ + +CONFIGURATION VARIABLES +--- -- 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 v5 01/14] Add data structures and basic functions for commit trailers
On Sun, Feb 9, 2014 at 8:48 AM, Christian Couder
chrisc...@tuxfamily.org wrote:
From: Junio C Hamano gits...@pobox.com
Christian Couder chrisc...@tuxfamily.org writes:
+static inline int same_trailer(struct trailer_item *a, struct trailer_item
*b, int alnum_len)
+{
+return same_token(a, b, alnum_len) same_value(a, b);
+}
All these inlines look premature optimization that can be
delegated to any decent compiler, don't they?
Yeah, but as Eric suggested to add them like in header files and you
did not reply to him, I thought you agreed with him.
I will remove them.
If these functions are used only by trailer.c, then it would make
sense to move them from trailer.h to trailer.c so that they don't get
defined unnecessarily by each .c file which includes trailer.h.
+/* Get the length of buf from its beginning until its last alphanumeric
character */
+static inline size_t alnum_len(const char *buf, int len)
+{
+while (--len = 0 !isalnum(buf[len]));
+return (size_t) len + 1;
This is somewhat unfortunate. if the caller wants to receive
size_t, perhaps it should be passing in size_t (or ssize_t) to the
function? Hard to guess without an actual caller, though.
Ok, I will make it return an int.
Why not adjust the loop condition slightly instead so that you can
continue to accept and return size_t?
--
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] notes: Disallow reusing non-blob as a note object
On Wed, Feb 12, 2014 at 4:54 AM, Johan Herland jo...@herland.net wrote:
Currently git notes add -C $object will read the raw bytes from $object,
and then copy those bytes into the note object, which is hardcoded to be
of type blob. This means that if the given $object is a non-blob (e.g.
tree or commit), the raw bytes from that object is copied into a blob
object. This is probably not useful, and certainly not what any sane
user would expect. So disallow it, by erroring out if the $object passed
to the -C option is not a blob.
The fix also applies to the -c option (in which the user is prompted to
edit/verify the note contents in a text editor), and also when -c/-C is
passed to git notes append (which appends the $object contents to an
existing note object). In both cases, passing a non-blob $object does not
make sense.
Also add a couple of tests demonstrating expected behavior.
Suggested-by: Junio C Hamano gits...@pobox.com
Signed-off-by: Johan Herland jo...@herland.net
---
builtin/notes.c | 6 +-
t/t3301-notes.sh | 27 +++
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 2b24d05..bb89930 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -269,7 +269,11 @@ static int parse_reuse_arg(const struct option *opt,
const char *arg, int unset)
die(_(Failed to resolve '%s' as a valid ref.), arg);
if (!(buf = read_sha1_file(object, type, len)) || !len) {
free(buf);
- die(_(Failed to read object '%s'.), arg);;
+ die(_(Failed to read object '%s'.), arg);
+ }
+ if (type != OBJ_BLOB) {
+ free(buf);
+ die(_(Cannot read note data from non-blob object '%s'.),
arg);
The way this diagnostic is worded, it sound as if the 'read' failed
rather than that the user specified an incorrect object type. Perhaps
Object is not a blob '%s' or Expected blob but '%s' has type '%s'
or something along those lines?
}
strbuf_add((msg-buf), buf, len);
free(buf);
--
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 3/3] read-cache: add index.version config variable
On Sat, Feb 15, 2014 at 2:23 PM, Thomas Gummerer t.gumme...@gmail.com wrote: Add a config variable that allows setting the default index version when initializing a new index file. Similar to the GIT_INDEX_VERSION environment variable this only affects new index files. Signed-off-by: Thomas Gummerer t.gumme...@gmail.com --- index 37fd84d..bf34985 100755 --- a/t/t1600-index.sh +++ b/t/t1600-index.sh @@ -21,4 +21,31 @@ test_expect_success 'out of bounds GIT_INDEX_VERSION issues warning' ' ) ' +test_expect_success 'out of bounds index.version issuses warning' ' s/issuses/issues/ + ( + unset GIT_INDEX_VERSION + rm .git/index + git config --add index.version 1 + git add a 21 | sed s/[0-9]// actual.err + sed -e s/ Z$/ / -\EOF expect.err + warning: index.version set, but the value is invalid. + Using version Z + EOF + test_i18ncmp expect.err actual.err + ) +' + +test_expect_success 'GIT_INDEX_VERSION takes precedence over config' ' + ( + rm .git/index + GIT_INDEX_VERSION=4 + export GIT_INDEX_VERSION + git config --add index.version 2 + git add a 21 + echo 4 expect + test-index-version .git/index actual + test_cmp expect actual + ) +' + test_done -- 1.8.5.2.300.ge613be6.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 3/3] config: teach git config --file - to read from the standard input
On Fri, Feb 14, 2014 at 6:37 PM, Kirill A. Shutemov kir...@shutemov.name wrote: The patch extends git config --file interface to allow read config from stdin. Editing stdin or setting value in stdin is an error. Include by absolute path is allowed in stdin config, but not by relative path. Signed-off-by: Kirill A. Shutemov kir...@shutemov.name --- diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh index a70707620f14..fda6555024c5 100755 --- a/t/t1305-config-include.sh +++ b/t/t1305-config-include.sh @@ -122,6 +122,20 @@ test_expect_success 'relative includes from command line fail' ' test_must_fail git -c include.path=one config test.one ' +test_expect_success 'absolute includes from stdin work' ' + echo [test]one = 1 one + echo 1 expect + echo [include]path=\$PWD/one\ | To be Windows-friendly, you may want to use $(pwd). Quoting from t/README: When a test checks for an absolute path that a git command generated, construct the expected value using $(pwd) rather than $PWD, $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference on Windows, where the shell (MSYS bash) mangles absolute path names. For details, see the commit message of 4114156ae9. + git config --file - test.one actual + test_cmp expect actual +' + +test_expect_success 'relative includes from stdin line fail' ' + echo [test]one = 1 one + echo [include]path=one | + test_must_fail git config --file - test.one +' + test_expect_success 'include cycles are detected' ' cat .gitconfig -\EOF [test]value = gitconfig -- 1.8.5.2 -- 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] reset: optionally setup worktree and refresh index on --mixed
On Sat, Feb 15, 2014 at 9:28 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
Refreshing index requires work tree. So we have to options: always set
s/to/two/
up work tree (and refuse to reset if failing to do so), or make
refreshing index optional.
As refreshing index is not the main task, it makes more sense to make
it optional.
Reported-by: Patrick Palka patr...@parcs.ath.cx
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/reset.c | 7 ---
t/t7102-reset.sh | 11 +++
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 6004803..a991344 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -320,7 +320,7 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
if (reset_type == NONE)
reset_type = MIXED; /* by default */
- if (reset_type != SOFT reset_type != MIXED)
+ if (reset_type != SOFT (reset_type != MIXED ||
get_git_work_tree()))
setup_work_tree();
if (reset_type == MIXED is_bare_repository())
@@ -340,8 +340,9 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
int flags = quiet ? REFRESH_QUIET :
REFRESH_IN_PORCELAIN;
if (read_from_tree(pathspec, sha1))
return 1;
- refresh_index(the_index, flags, NULL, NULL,
- _(Unstaged changes after reset:));
+ if (get_git_work_tree())
+ refresh_index(the_index, flags, NULL, NULL,
+ _(Unstaged changes after
reset:));
} else {
int err = reset_index(sha1, reset_type, quiet);
if (reset_type == KEEP !err)
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 8d4b50d..ee117e2 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -535,4 +535,15 @@ test_expect_success 'reset with paths accepts tree' '
git diff HEAD --exit-code
'
+test_expect_success 'reset --mixed sets up work tree' '
+ git init mixed_worktree
+ (
+ cd mixed_worktree
+ test_commit dummy
+ )
+ : expect
+ git --git-dir=mixed_worktree/.git --work-tree=mixed_worktree reset
actual
+ test_cmp expect actual
+'
+
test_done
--
1.8.5.2.240.g8478abd
--
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
--
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 3/3] commit: add --cleanup=scissors
On Sat, Feb 15, 2014 at 10:37 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: Since 1a72cfd (commit -v: strip diffs and submodule shortlogs from the commit message - 2013-12-05) we have a less fragile way to cut out git status at the end of a commit message but it's only enabled for stripping submodule shortlogs. Add new cleanup option that reuses the same mechanism for the entire git status without accidentally remove s/remove/removing/ lines starting with '#' Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt index 1a7616c..98f976a 100644 --- a/Documentation/git-commit.txt +++ b/Documentation/git-commit.txt @@ -176,7 +176,7 @@ OPTIONS --cleanup=mode:: This option determines how the supplied commit message should be cleaned up before committing. The 'mode' can be `strip`, - `whitespace`, `verbatim`, or `default`. + `whitespace`, `verbatim`, `scissors` or `default`. + -- strip:: @@ -186,6 +186,11 @@ whitespace:: Same as `strip` except #commentary is not removed. verbatim:: Do not change the message at all. +scissors:: + Same as `whitespace`, except that everything from the line + `# 8 ` Would it make sense to be more explicit and say that the 'cut' line is also removed? Same as `whitespace`, except that the line `# 8 ` and all lines following it are removed ... + is truncated if the message is to be edited. `#` could be s/could/can/ + customized with core.commentChar. default:: Same as `strip` if the message is to be edited. Otherwise `whitespace`. -- 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] web--browse: Use powershell on Windows
On Sun, Feb 16, 2014 at 2:22 AM, Steven Penny svnp...@gmail.com wrote:
On Windows you can have either MinGW or Cygwin. As has been shown in this
script
MinGW uses start while Cygwin uses cygstart. The cygstart command is
robust but the start command breaks on certain URLs
$ git web--browse 'http://wikipedia.org/wiki/Key__Peele'
'_Peele' is not recognized as an internal or external command,
operable program or batch file.
An alternative is to use PowerShell. PowerShell is a component of Windows and
will work with both MinGW and Cygwin.
Signed-off-by: Steven Penny svnp...@gmail.com
---
diff --git a/Documentation/git-web--browse.txt
b/Documentation/git-web--browse.txt
index 2de575f..02cccf9 100644
--- a/Documentation/git-web--browse.txt
+++ b/Documentation/git-web--browse.txt
@@ -33,8 +33,7 @@ The following browsers (or commands) are currently
supported:
* lynx
* dillo
* open (this is the default under Mac OS X GUI)
-* start (this is the default under MinGW)
-* cygstart (this is the default under Cygwin)
+* powershell (this is the default under Windows)
* xdg-open
Custom commands may also be specified.
diff --git a/git-web--browse.sh b/git-web--browse.sh
index ebdfba6..72fbe32 100755
--- a/git-web--browse.sh
+++ b/git-web--browse.sh
@@ -34,7 +34,7 @@ valid_tool() {
firefox | iceweasel | seamonkey | iceape | \
chrome | google-chrome | chromium | chromium-browser | \
konqueror | opera | w3m | elinks | links | lynx | dillo | open | \
- start | cygstart | xdg-open)
+ powershell | xdg-open)
;; # happy
*)
valid_custom_tool $1 || return 1
@@ -124,13 +124,10 @@ if test -z $browser ; then
then
browser_candidates=open $browser_candidates
fi
- # /bin/start indicates MinGW
- if test -x /bin/start; then
- browser_candidates=start $browser_candidates
- fi
- # /usr/bin/cygstart indicates Cygwin
- if test -x /usr/bin/cygstart; then
- browser_candidates=cygstart $browser_candidates
+ # OS indicates Windows
+ if test -n $OS
+ then
+ browser_candidates=powershell $browser_candidates
fi
Doesn't this penalize users who don't have powershell installed?
for i in $browser_candidates; do
@@ -179,11 +176,11 @@ konqueror)
;;
esac
;;
-w3m|elinks|links|lynx|open|cygstart|xdg-open)
+w3m|elinks|links|lynx|open|xdg-open)
$browser_path $@
;;
-start)
- exec $browser_path 'web-browse' $@
+powershell)
+ $browser_path saps '$@'
;;
opera|dillo)
$browser_path $@
--
1.8.5.3
--
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] tag: support --sort=version
On Wed, Feb 19, 2014 at 8:39 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: --sort=version sorts tags as versions. GNU extension's strverscmp is used and no real compat implementation is provided so this is Linux only. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt index 404257d..fc23dc0 100644 --- a/Documentation/git-tag.txt +++ b/Documentation/git-tag.txt @@ -95,6 +95,10 @@ OPTIONS using fnmatch(3)). Multiple patterns may be given; if any of them matches, the tag is shown. +--sort=type:: + Sort in a specific order. Supported type is version. Prepend + - to revert sort order. s/revert/reverse/ + --column[=options]:: --no-column:: Display tag listing in columns. See configuration variable -- 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 23/25] checkout: detach if the branch is already checked out elsewhere
On Tue, Feb 18, 2014 at 8:40 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
The normal rule is anything outside refs/heads/ is detached. This
strictens the rule a bit more: if the branch is checked out (either in
s/strictens/increases strictness of/
$GIT_COMMON_DIR/HEAD or any $GIT_DIR/repos/.../HEAD) then it's
detached as well.
A hint is given so the user knows where to go and do something there
if they still want to checkout undetached here.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f961604..7b86f2b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -433,6 +433,11 @@ struct branch_info {
const char *name; /* The short name used */
const char *path; /* The full name of a real branch */
struct commit *commit; /* The named commit */
+ /*
+* if not null the branch is detached because it's alrady
s/alrady/already/
+* checked out in this checkout
+*/
+ char *checkout;
};
+static void check_linked_checkouts(struct branch_info *new)
+{
+ struct strbuf path = STRBUF_INIT;
+ DIR *dir;
+ struct dirent *d;
+
+ strbuf_addf(path, %s/repos, get_git_common_dir());
+ if ((dir = opendir(path.buf)) == NULL)
strbuf_release(path);
+ return;
+
+ strbuf_reset(path);
+ strbuf_addf(path, %s/HEAD, get_git_common_dir());
+ /*
+* $GIT_COMMON_DIR/HEAD is practically outside
+* $GIT_DIR so resolve_ref_unsafe() won't work (it
+* uses git_path). Parse the ref ourselves.
+*/
+ if (check_linked_checkout(new, , path.buf)) {
+ strbuf_release(path);
+ closedir(dir);
+ return;
+ }
+
+ while ((d = readdir(dir)) != NULL) {
+ if (!strcmp(d-d_name, .) || !strcmp(d-d_name, ..))
+ continue;
+ strbuf_reset(path);
+ strbuf_addf(path, %s/repos/%s/HEAD,
+ get_git_common_dir(), d-d_name);
+ if (check_linked_checkout(new, d-d_name, path.buf))
+ break;
+ }
+ strbuf_release(path);
+ closedir(dir);
+}
--
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 24/25] prune: strategies for linked checkouts
On Tue, Feb 18, 2014 at 8:40 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
The pruning rules are:
- if $REPO/locked exists, repos/id is not supposed to be pruned.
- if $REPO/locked exists and $REPO/gitdir's mtimer is older than a
s/mtimer/mtime/
really long limit, warn about old unused repo.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 7b86f2b..c501e9c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -854,6 +854,17 @@ static void remove_junk_on_signal(int signo)
raise(signo);
}
+static dev_t get_device_or_die(const char *path)
+{
+ struct stat buf;
+ if (stat(path, buf))
+ die_errno(failed to stat '%s', path);
+ /* Ah Windows! Make different drives different partitions */
+ if (buf.st_dev == 0 has_dos_drive_prefix(c:\\))
+ buf.st_dev = toupper(real_path(path)[0]);
This invocation of has_dos_drive_prefix() with hardcoded c:\\ at
first looks like an error until the reader realizes that it's an
#ifdef-less check if the platforms is Windows. Would it make more
sense to encapsulate this anomaly and abstract it away by fixing
compat/mingw.c:do_lstat() to instead set 'st_dev' automatically like
you do here? In particular, this line in mingw.c:
buf-st_dev = buf-st_rdev = 0; /* not used by Git */
+ return buf.st_dev;
+}
--
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 24/25] prune: strategies for linked checkouts
On Wed, Feb 19, 2014 at 5:08 PM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Tue, Feb 18, 2014 at 8:40 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com
wrote:
+static dev_t get_device_or_die(const char *path)
+{
+ struct stat buf;
+ if (stat(path, buf))
+ die_errno(failed to stat '%s', path);
+ /* Ah Windows! Make different drives different partitions */
+ if (buf.st_dev == 0 has_dos_drive_prefix(c:\\))
+ buf.st_dev = toupper(real_path(path)[0]);
This invocation of has_dos_drive_prefix() with hardcoded c:\\ at
first looks like an error until the reader realizes that it's an
#ifdef-less check if the platforms is Windows. Would it make more
sense to encapsulate this anomaly and abstract it away by fixing
compat/mingw.c:do_lstat() to instead set 'st_dev' automatically like
you do here? In particular, this line in mingw.c:
buf-st_dev = buf-st_rdev = 0; /* not used by Git */
+ return buf.st_dev;
Or, if doing this in do_lstat() is too expensive for normal stat()
operations (which is likely), then a simple #ifdef might be easier to
read; or abstract it into a get_device() function which Windows/MinGW
can override, doing buf.st_dev = toupper(real_path(...)), thus also
making the code easier to understand.
--
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 14/25] setup.c: convert is_git_directory() to use strbuf
On Tue, Feb 18, 2014 at 8:40 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
index 73e80ce..aec9fdb 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -116,6 +116,10 @@ extern void strbuf_add(struct strbuf *, const void *,
size_t);
static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
strbuf_add(sb, s, strlen(s));
}
+static inline void strbuf_addstr_at(struct strbuf *sb, size_t len, const
char *s) {
+ strbuf_setlen(sb, len);
+ strbuf_add(sb, s, strlen(s));
+}
Update Documentation/technical/api-strbuf.txt?
static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf
*sb2) {
strbuf_grow(sb, sb2-len);
strbuf_add(sb, sb2-buf, sb2-len);
--
1.8.5.2.240.g8478abd
--
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 3/8] merge-recursive: -Xindex-only to leave worktree unchanged
On Sat, Feb 22, 2014 at 4:17 AM, Thomas Rast t...@thomasrast.ch wrote: Using the new no_worktree flag from the previous commit, we can teach merge-recursive to leave the worktree untouched. Expose this with a new strategy option so that scripts can use it. Signed-off-by: Junio C Hamano gits...@pobox.com --- diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt index fb6e593..2934e99 100644 --- a/Documentation/merge-strategies.txt +++ b/Documentation/merge-strategies.txt @@ -92,6 +92,10 @@ subtree[=path];; is prefixed (or stripped from the beginning) to make the shape of two trees to match. +index-only;; s/;;/::/ + Write the merge result only to the index; do not touch the + worktree. + octopus:: This resolves cases with more than two heads, but refuses to do a complex merge that needs manual resolution. It is -- 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 7/8] name-hash: allow dir hashing even when !ignore_case
On Sat, Feb 22, 2014 at 4:17 AM, Thomas Rast t...@thomasrast.ch wrote:
The directory hash (for fast checks if the index already has a
directory) was only used in ignore_case mode and so depended on that
flag.
Make it generally available on request.
Signed-off-by: Thomas Rast t...@thomasrast.ch
---
diff --git a/name-hash.c b/name-hash.c
index e5b6e1a..c8953be 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -141,16 +141,19 @@ static void hash_index_entry(struct index_state
*istate, struct cache_entry *ce)
*pos = ce;
}
- if (ignore_case !(ce-ce_flags CE_UNHASHED))
+ if (istate-has_dir_hash !(ce-ce_flags CE_UNHASHED))
add_dir_entry(istate, ce);
}
-static void lazy_init_name_hash(struct index_state *istate)
+void init_name_hash(struct index_state *istate, int force_dir_hash)
{
int nr;
if (istate-name_hash_initialized)
return;
+
+ istate-has_dir_hash = force_dir_hash || ignore_case;
This is getting a bit convoluted. Refactoring lazy_init_name_hash()
into two functions would eliminate the complexity. For instance:
void init_name_hash(struct index_state *istate)
{
...pure initialization code...
}
static void init_name_hash_if_needed(struct index_state *istate)
{
if (ignore_case !istate-name_hash_initialized)
init_name_hash(istate);
}
The two existing callers of lazy_init_name_hash() in name-hash.c,
which rely upon the lazy/ignore-case logic, would invoke the static
init_name_hash_if_needed().
The new caller in patch 8/8, which knows explicitly if and when it
wants the hash initialized can invoke the public init_name_hash().
if (istate-cache_nr)
preallocate_hash(istate-name_hash, istate-cache_nr);
for (nr = 0; nr istate-cache_nr; nr++)
@@ -161,7 +164,7 @@ static void lazy_init_name_hash(struct index_state
*istate)
void add_name_hash(struct index_state *istate, struct cache_entry *ce)
{
/* if already hashed, add reference to directory entries */
- if (ignore_case (ce-ce_flags CE_STATE_MASK) == CE_STATE_MASK)
+ if (istate-has_dir_hash (ce-ce_flags CE_STATE_MASK) ==
CE_STATE_MASK)
add_dir_entry(istate, ce);
ce-ce_flags = ~CE_UNHASHED;
@@ -181,7 +184,7 @@ void add_name_hash(struct index_state *istate, struct
cache_entry *ce)
void remove_name_hash(struct index_state *istate, struct cache_entry *ce)
{
/* if already hashed, release reference to directory entries */
- if (ignore_case (ce-ce_flags CE_STATE_MASK) == CE_HASHED)
+ if (istate-has_dir_hash (ce-ce_flags CE_STATE_MASK) ==
CE_HASHED)
remove_dir_entry(istate, ce);
ce-ce_flags |= CE_UNHASHED;
@@ -228,7 +231,7 @@ struct cache_entry *index_dir_exists(struct index_state
*istate, const char *nam
struct cache_entry *ce;
struct dir_entry *dir;
- lazy_init_name_hash(istate);
+ init_name_hash(istate, 0);
dir = find_dir_entry(istate, name, namelen);
if (dir dir-nr)
return dir-ce;
@@ -250,7 +253,7 @@ struct cache_entry *index_file_exists(struct index_state
*istate, const char *na
unsigned int hash = hash_name(name, namelen);
struct cache_entry *ce;
- lazy_init_name_hash(istate);
+ init_name_hash(istate, 0);
ce = lookup_hash(hash, istate-name_hash);
while (ce) {
@@ -286,9 +289,11 @@ void free_name_hash(struct index_state *istate)
if (!istate-name_hash_initialized)
return;
istate-name_hash_initialized = 0;
- if (ignore_case)
+ if (istate-has_dir_hash) {
/* free directory entries */
for_each_hash(istate-dir_hash, free_dir_entry, NULL);
+ istate-has_dir_hash = 0;
+ }
free_hash(istate-name_hash);
free_hash(istate-dir_hash);
--
1.9.0.313.g3d0a325
--
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 3/8] merge-recursive: -Xindex-only to leave worktree unchanged
On Sun, Feb 23, 2014 at 6:57 AM, Thomas Rast t...@thomasrast.ch wrote: Eric Sunshine sunsh...@sunshineco.com writes: On Sat, Feb 22, 2014 at 4:17 AM, Thomas Rast t...@thomasrast.ch wrote: Using the new no_worktree flag from the previous commit, we can teach merge-recursive to leave the worktree untouched. Expose this with a new strategy option so that scripts can use it. Signed-off-by: Junio C Hamano gits...@pobox.com --- diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt index fb6e593..2934e99 100644 --- a/Documentation/merge-strategies.txt +++ b/Documentation/merge-strategies.txt @@ -92,6 +92,10 @@ subtree[=path];; is prefixed (or stripped from the beginning) to make the shape of two trees to match. +index-only;; s/;;/::/ I think ;; is actually correct: this continues the sub-list of options to the recursive strategy. The :: level lists the available strategies. Make sense. Thanks for the explanation. + Write the merge result only to the index; do not touch the + worktree. + octopus:: This resolves cases with more than two heads, but refuses to do a complex merge that needs manual resolution. It is -- 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 -- Thomas Rast t...@thomasrast.ch -- 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 10/25] Add new environment variable $GIT_COMMON_DIR
On Tue, Feb 18, 2014 at 8:39 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: This variable is intended to support multiple working directories attached to a repository. Such a repository may have a main working directory, created by either git init or git clone and one or more linked working directories. These working directories and the main repository share the same repository directory. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- diff --git a/Documentation/git.txt b/Documentation/git.txt index 02bbc08..2c4a055 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -773,6 +773,14 @@ Git so take care if using Cogito etc. an explicit repository directory set via 'GIT_DIR' or on the command line. +'GIT_COMMON_DIR':: + If this variable is set to a path, non-worktree files that are + normally in $GIT_DIR will be taken from this path + instead. Worktree-specific files such as HEAD or index are + taken from $GIT_DIR. This variable has lower precedence than + other path variables such as GIT_INDEX_FILE, + GIT_OBJECT_DIRECTORY... For a person not familiar with git checkout --to or its underlying implementation, this description may be lacking. Such a reader may be left wondering about GIT_COMMON_DIR's overall purpose, and when and how it should be used. Perhaps it would make sense to talk a bit about git checkout --to here? Git Commits ~~~ 'GIT_AUTHOR_NAME':: -- 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 10/25] Add new environment variable $GIT_COMMON_DIR
On Wed, Feb 26, 2014 at 11:12 AM, Philip Oakley philipoak...@iee.org wrote: From: Duy Nguyen pclo...@gmail.com On Wed, Feb 26, 2014 at 8:24 AM, Eric Sunshine sunsh...@sunshineco.com wrote: +'GIT_COMMON_DIR':: + If this variable is set to a path, non-worktree files that are + normally in $GIT_DIR will be taken from this path + instead. Worktree-specific files such as HEAD or index are + taken from $GIT_DIR. This variable has lower precedence than + other path variables such as GIT_INDEX_FILE, + GIT_OBJECT_DIRECTORY... For a person not familiar with git checkout --to or its underlying implementation, this description may be lacking. Such a reader may be left wondering about GIT_COMMON_DIR's overall purpose, and when and how it should be used. Perhaps it would make sense to talk a bit about git checkout --to here? I don't want to repeat too much. Maybe mention about git checkout --to and point them to git-checkout man page? I've just looked at both https://www.kernel.org/pub/software/scm/git/docs/git-checkout.html and http://git-htmldocs.googlecode.com/git/git-checkout.html and neither appear to mention the --to option. Is it missing from the man page? Or is it me that's missing something? 'git checkout --to' is the new feature being introduced by this 25-patch series [1] from Duy (to which we are responding). [1]: http://thread.gmane.org/gmane.comp.version-control.git/242300 -- 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 10/25] Add new environment variable $GIT_COMMON_DIR
On Wed, Feb 26, 2014 at 5:55 AM, Duy Nguyen pclo...@gmail.com wrote: On Wed, Feb 26, 2014 at 8:24 AM, Eric Sunshine sunsh...@sunshineco.com wrote: +'GIT_COMMON_DIR':: + If this variable is set to a path, non-worktree files that are + normally in $GIT_DIR will be taken from this path + instead. Worktree-specific files such as HEAD or index are + taken from $GIT_DIR. This variable has lower precedence than + other path variables such as GIT_INDEX_FILE, + GIT_OBJECT_DIRECTORY... For a person not familiar with git checkout --to or its underlying implementation, this description may be lacking. Such a reader may be left wondering about GIT_COMMON_DIR's overall purpose, and when and how it should be used. Perhaps it would make sense to talk a bit about git checkout --to here? I don't want to repeat too much. Maybe mention about git checkout --to and point them to git-checkout man page? Yes, that might be sufficient. git checkout --to documentation points the reader at the MULTIPLE CHECKOUT MODE section which gives a more detailed explanation of GIT_COMMON_DIR, so a user wanting to understand GIT_COMMON_DIR better would have a way to find the information. -- 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 21/25] checkout: support checking out into a new working directory
On Tue, Feb 18, 2014 at 8:40 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
git checkout --to sets up a new working directory with a .git file
pointing to $GIT_DIR/repos/id. It then executes git checkout again
on the new worktree with the same arguments except --to is taken
out. The second checkout execution, which is not contaminated with any
info from the current repository, will actually check out and
everything that normal git checkout does.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0570e41..2b856a6 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -806,6 +814,74 @@ static int switch_branches(const struct checkout_opts
*opts,
return ret || writeout_error;
}
+static int prepare_linked_checkout(const struct checkout_opts *opts,
+ struct branch_info *new)
+{
+ struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;
+ struct strbuf sb = STRBUF_INIT;
+ const char *path = opts-new_worktree;
+ struct stat st;
+ const char *name;
+ struct child_process cp;
+ int counter = 0, len;
+
+ if (!new-commit)
+ die(_(no branch specified));
+
+ len = strlen(path);
+ if (!len || is_dir_sep(path[len - 1]))
+ die(_('--to' argument '%s' cannot end with a slash), path);
What is the purpose of this restriction?
+ for (name = path + len - 1; name path; name--)
+ if (is_dir_sep(*name)) {
+ name++;
+ break;
+ }
+ strbuf_addstr(sb_repo, git_path(repos/%s, name));
+ len = sb_repo.len;
+ if (safe_create_leading_directories_const(sb_repo.buf))
+ die_errno(_(could not create leading directories of '%s'),
+ sb_repo.buf);
+ while (!stat(sb_repo.buf, st)) {
+ counter++;
+ strbuf_setlen(sb_repo, len);
+ strbuf_addf(sb_repo, %d, counter);
+ }
+ name = sb_repo.buf + len - strlen(name);
+ if (mkdir(sb_repo.buf, 0777))
+ die_errno(_(could not create directory of '%s'),
sb_repo.buf);
+
+ strbuf_addf(sb_git, %s/.git, path);
+ if (safe_create_leading_directories_const(sb_git.buf))
+ die_errno(_(could not create leading directories of '%s'),
+ sb_git.buf);
+
+ write_file(sb_git.buf, 1, gitdir: %s/repos/%s\n,
+ real_path(get_git_dir()), name);
+ /*
+* This is to keep resolve_ref() happy. We need a valid HEAD
+* or is_git_directory() will reject the directory. Any valid
+* value would do because this value will be ignored and
+* replaced at the next (real) checkout.
+*/
+ strbuf_addf(sb, %s/HEAD, sb_repo.buf);
+ write_file(sb.buf, 1, %s\n, sha1_to_hex(new-commit-object.sha1));
+ strbuf_reset(sb);
+ strbuf_addf(sb, %s/commondir, sb_repo.buf);
+ write_file(sb.buf, 1, ../..\n);
+
+ if (!opts-quiet)
+ fprintf_ln(stderr, _(Enter %s (identifier %s)), path, name);
+
+ setenv(GIT_CHECKOUT_NEW_WORKTREE, 1, 1);
+ setenv(GIT_DIR_ENVIRONMENT, sb_git.buf, 1);
+ setenv(GIT_WORK_TREE_ENVIRONMENT, path, 1);
+ memset(cp, 0, sizeof(cp));
+ cp.git_cmd = 1;
+ cp.argv = opts-saved_argv;
+ return run_command(cp);
+}
+
--
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: Doc target fails to parse user-manual.xml
On Wed, Feb 26, 2014 at 11:13 AM, Leo R. Lundgren l...@finalresort.org wrote: I'm installing git 1.9.0 from source, on a freshly installed SLES 11 SP3. The git binaries work fine to compile and install, but `make doc` fails on some XML parsing errors. The system is fully updated with the latest stable packages in the SLES 11 SP3 distribution. What I've done is: - 8 - foo@bar:~ rpm -qa|grep xml libxml2-python-2.7.6-0.23.1 php53-xmlwriter-5.3.17-0.13.7 libxml2-2.7.6-0.23.1 libxml2-32bit-2.7.6-0.23.1 php53-xmlreader-5.3.17-0.13.7 xmlcharent-0.3-403.14 python-xml-2.6.8-0.15.1 yast2-xml-2.16.1-1.23 foo@bar:~ rpm -qa|grep doc docbook_4-4.5-111.14 pam-doc-1.1.5-0.10.17 perl-doc-5.10.0-64.67.52 readline-doc-5.2-147.17.30 docbook-xsl-stylesheets-1.78.1-0.7.17 apparmor-docs-2.5.1.r1445-55.59.1 asciidoc-8.2.7-29.21 PolicyKit-doc-0.9-14.39.2 nfs-doc-1.2.3-18.29.1 bash-doc-3.2-147.17.30 postgresql91-docs-9.1.9-0.3.1 Some of these packages are pretty old and possibly buggy. The documentation builds cleanly on my Linux and Mac boxes, but they are using newer versions of these tools. For instance, I have asciidoc 8.6.7 on Linux, and 8.6.9 on Mac, whereas your version is only at 8.2.7. Perhaps try updating your toolchain. foo@bar:~/git-1.9.0 make doc make -C Documentation all make[1]: Entering directory `/home/foo/git-1.9.0/Documentation' GEN mergetools-list.made GEN cmd-list.made GEN doc.dep make[2]: Entering directory `/home/foo/git-1.9.0' make[2]: `GIT-VERSION-FILE' is up to date. make[2]: Leaving directory `/home/foo/git-1.9.0' make[1]: Leaving directory `/home/foo/git-1.9.0/Documentation' make[1]: Entering directory `/home/foo/git-1.9.0/Documentation' make[2]: Entering directory `/home/foo/git-1.9.0' make[2]: `GIT-VERSION-FILE' is up to date. make[2]: Leaving directory `/home/foo/git-1.9.0' ASCIIDOC git-add.html ASCIIDOC git-am.html ASCIIDOC git-annotate.html ASCIIDOC git-apply.html ASCIIDOC git-archimport.html ASCIIDOC git-archive.html ASCIIDOC git-bisect.html ASCIIDOC git-blame.html ASCIIDOC git-branch.html ASCIIDOC git-bundle.html ASCIIDOC git-cat-file.html ASCIIDOC git-check-attr.html ASCIIDOC git-check-ignore.html ASCIIDOC git-check-mailmap.html ASCIIDOC git-checkout-index.html ASCIIDOC git-checkout.html ASCIIDOC git-check-ref-format.html ASCIIDOC git-cherry-pick.html ASCIIDOC git-cherry.html ASCIIDOC git-citool.html ASCIIDOC git-clean.html ASCIIDOC git-clone.html ASCIIDOC git-column.html ASCIIDOC git-commit-tree.html ASCIIDOC git-commit.html ASCIIDOC git-config.html ASCIIDOC git-count-objects.html ASCIIDOC git-credential-cache--daemon.html ASCIIDOC git-credential-cache.html ASCIIDOC git-credential-store.html ASCIIDOC git-credential.html ASCIIDOC git-cvsexportcommit.html ASCIIDOC git-cvsimport.html ASCIIDOC git-cvsserver.html ASCIIDOC git-daemon.html ASCIIDOC git-describe.html ASCIIDOC git-diff-files.html ASCIIDOC git-diff-index.html ASCIIDOC git-difftool.html ASCIIDOC git-diff-tree.html ASCIIDOC git-diff.html ASCIIDOC git-fast-export.html ASCIIDOC git-fast-import.html ASCIIDOC git-fetch-pack.html ASCIIDOC git-fetch.html ASCIIDOC git-filter-branch.html ASCIIDOC git-fmt-merge-msg.html ASCIIDOC git-for-each-ref.html ASCIIDOC git-format-patch.html ASCIIDOC git-fsck-objects.html ASCIIDOC git-fsck.html ASCIIDOC git-gc.html ASCIIDOC git-get-tar-commit-id.html ASCIIDOC git-grep.html ASCIIDOC git-gui.html ASCIIDOC git-hash-object.html ASCIIDOC git-help.html ASCIIDOC git-http-backend.html ASCIIDOC git-http-fetch.html ASCIIDOC git-http-push.html ASCIIDOC git-imap-send.html ASCIIDOC git-index-pack.html ASCIIDOC git-init-db.html ASCIIDOC git-init.html ASCIIDOC git-instaweb.html ASCIIDOC git-log.html ASCIIDOC git-ls-files.html ASCIIDOC git-ls-remote.html ASCIIDOC git-ls-tree.html ASCIIDOC git-mailinfo.html ASCIIDOC git-mailsplit.html ASCIIDOC git-merge-base.html ASCIIDOC git-merge-file.html ASCIIDOC git-merge-index.html ASCIIDOC git-merge-one-file.html ASCIIDOC git-mergetool--lib.html ASCIIDOC git-mergetool.html ASCIIDOC git-merge-tree.html ASCIIDOC git-merge.html ASCIIDOC git-mktag.html ASCIIDOC git-mktree.html ASCIIDOC git-mv.html ASCIIDOC git-name-rev.html ASCIIDOC git-notes.html ASCIIDOC git-p4.html ASCIIDOC git-pack-objects.html ASCIIDOC git-pack-redundant.html ASCIIDOC git-pack-refs.html ASCIIDOC git-parse-remote.html ASCIIDOC git-patch-id.html ASCIIDOC git-prune-packed.html ASCIIDOC git-prune.html ASCIIDOC git-pull.html ASCIIDOC git-push.html ASCIIDOC git-quiltimport.html
Re: [PATCH v3 21/25] checkout: support checking out into a new working directory
On Wed, Feb 26, 2014 at 6:19 PM, Duy Nguyen pclo...@gmail.com wrote:
On Thu, Feb 27, 2014 at 3:06 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
+ len = strlen(path);
+ if (!len || is_dir_sep(path[len - 1]))
+ die(_('--to' argument '%s' cannot end with a slash),
path);
What is the purpose of this restriction?
Laziness on my part :) Because the following loop searches backward to
get the `basename $path`, trailing slash would make it return empty
base name. I could have just removed the trailing slash here instead
of dying though. Thanks for catching.
Thanks for the explanation. I thought that that might be the case.
+ for (name = path + len - 1; name path; name--)
+ if (is_dir_sep(*name)) {
+ name++;
+ break;
+ }
--
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/2] fetch: add a failing test for prunning with overlapping refspecs
On Thu, Feb 27, 2014 at 4:00 AM, Carlos Martín Nieto c...@elego.de wrote: Subject: fetch: add a failing test for prunning with overlapping refspecs s/prunning/pruning/ Signed-off-by: Carlos Martín Nieto c...@elego.de --- diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 1f0f8e6..4949e3d 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -113,6 +113,26 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' ' git rev-parse origin/master ' +test_expect_failure 'fetch --prune handles overlapping refspecs' ' + cd $D + git update-ref refs/pull/42/head master + git clone . prune-overlapping + cd prune-overlapping + git config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* + + git fetch --prune origin + git rev-parse origin/master + git rev-parse origin/pr/42 + + git config --unset-all remote.origin.fetch Broken -chain. + git config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* + git config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* + + git fetch --prune origin + git rev-parse origin/master + git rev-parse origin/pr/42 +' + test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' ' cd $D git clone . prune-tags -- 1.9.0.rc3.244.g3497008 -- 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/2] fetch: add a failing test for prunning with overlapping refspecs
On Thu, Feb 27, 2014 at 4:00 AM, Carlos Martín Nieto c...@elego.de wrote: Subject: fetch: add a failing test for prunning with overlapping refspecs s/prunning/pruning/ Signed-off-by: Carlos Martín Nieto c...@elego.de --- diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 1f0f8e6..4949e3d 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -113,6 +113,26 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' ' git rev-parse origin/master ' +test_expect_failure 'fetch --prune handles overlapping refspecs' ' + cd $D + git update-ref refs/pull/42/head master + git clone . prune-overlapping + cd prune-overlapping + git config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* + + git fetch --prune origin + git rev-parse origin/master + git rev-parse origin/pr/42 + + git config --unset-all remote.origin.fetch Broken -chain. + git config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* + git config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* + + git fetch --prune origin + git rev-parse origin/master + git rev-parse origin/pr/42 +' + test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' ' cd $D git clone . prune-tags -- 1.9.0.rc3.244.g3497008 -- 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] Problem in bulk-checkin.c:finish_bulk_checkin() Unable to fix
On Thu, Feb 27, 2014 at 7:04 PM, Faiz Kothari faiz.of...@gmail.com wrote:
Signed-off-by: Faiz Kothari faiz.of...@gmail.com
---
Compiles without errors.
Fails in test t/t1050-large.sh ,fails 12/15 tests. Dumps memory map and
backtrace.
Somewhere its not able to free(): invalid pointer.
Please somone pointout where I am doing it wrong.
Help is really appreciated.
Thanks.
Hint: Pay close attention to the second sentence of the microproject
you've undertaken:
Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for
handling packname, and explain why this is useful. Also check if
the first argument of pack-write.c:finish_tmp_packfile() can be
made const.
It's the clue to understanding the crash.
bulk-checkin.c |8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..c76cd6b 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -23,7 +23,7 @@ static struct bulk_checkin_state {
static void finish_bulk_checkin(struct bulk_checkin_state *state)
{
unsigned char sha1[20];
- char packname[PATH_MAX];
+ struct strbuf packname = STRBUF_INIT;
int i;
if (!state-f)
@@ -42,9 +42,8 @@ static void finish_bulk_checkin(struct bulk_checkin_state
*state)
state-offset);
close(fd);
}
-
- sprintf(packname, %s/pack/pack-, get_object_directory());
- finish_tmp_packfile(packname, state-pack_tmp_name,
+ strbuf_addf(packname, %s/pack/pack-, get_object_directory());
+ finish_tmp_packfile(packname.buf, state-pack_tmp_name,
state-written, state-nr_written,
state-pack_idx_opts, sha1);
for (i = 0; i state-nr_written; i++)
@@ -53,6 +52,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state
*state)
clear_exit:
free(state-written);
memset(state, 0, sizeof(*state));
+ strbuf_release(packname);
/* Make objects we just wrote available to ourselves */
reprepare_packed_git();
--
1.7.9.5
--
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
--
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] rewrite bulk-checkin.c:finish_bulk_checkin() using strbuf
On Fri, Feb 28, 2014 at 2:58 AM, Faiz Kothari faiz.of...@gmail.com wrote:
Signed-off-by: Faiz Kothari faiz.of...@gmail.com
Notes:
I finally got what's happening, and why the errors were caused.
packname is supposed to contain the complete path to the .pack file.
Packs are stored as /path/to/SHA1.pack which I overlooked earlier.
After inspecting what is happening in pack-write.c:finish_tmp_packfile()
which indirectly modifies packname by appending the SHA1 and .pack to
packname
This is happening in these code snippets:
char *end_of_name_prefix = strrchr(name_buffer, 0);
and later
sprintf(end_of_name_prefix, %s.pack, sha1_to_hex(sha1));
name_buffer is packname.buf
Using const for the first argument of pack-write.c:finish_tmp_packfile()
doesnot raise any compile time warning or error and not any runtime
errors,
since the packname.buf is on heap and has extra space to which more char
can be written.
If this was not the case,
for e.g. passing a constant string and modifying it.
This will result in a segmentation fault.
---
This notes section is important to the ongoing email discussion,
however, it should be placed below the --- line so that it does not
become part of the recorded commit message when the patch is applied
via git am.
bulk-checkin.c |8 +---
pack-write.c |2 +-
pack.h |2 +-
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..bbdf1ec 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -23,7 +23,7 @@ static struct bulk_checkin_state {
static void finish_bulk_checkin(struct bulk_checkin_state *state)
{
unsigned char sha1[20];
- char packname[PATH_MAX];
+ struct strbuf packname = STRBUF_INIT;
int i;
if (!state-f)
@@ -42,9 +42,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state
*state)
state-offset);
close(fd);
}
+ strbuf_addf(packname, %s/pack/pack-, get_object_directory());
+ strbuf_grow(packname, 40 + 5);
There are several problems with this. First, magic numbers 40 and 5
convey no meaning to the reader. At the very least, they should be
named constants or a comment should explain them. More seriously,
though, this code is fragile since it has far too intimate knowledge
of the inner workings of finish_tmp_packfile(). If the implementation
of finish_tmp_packfile() changes in the future such that it writes
more than 45 additional characters to the incoming buffer, this will
break.
Rather than coupling finish_bulk_checkin() and finish_tmp_packfile()
so tightly, consider finish_tmp_packfile() a black box which just
does its job and then propose ways to make things work without
finish_bulk_checkin() having to know how that job is done.
- sprintf(packname, %s/pack/pack-, get_object_directory());
- finish_tmp_packfile(packname, state-pack_tmp_name,
+ finish_tmp_packfile(packname.buf, state-pack_tmp_name,
state-written, state-nr_written,
state-pack_idx_opts, sha1);
for (i = 0; i state-nr_written; i++)
diff --git a/pack-write.c b/pack-write.c
index 605d01b..ac38867 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)
return sha1fd(fd, *pack_tmp_name);
}
-void finish_tmp_packfile(char *name_buffer,
+void finish_tmp_packfile(const char *name_buffer,
This is misleading and fragile. By specifying 'const',
finish_tmp_packfile() promises not to modify the content of the
incoming name_buffer, yet it breaks this promise by modifying the
buffer through the non-const end_of_name_prefix variable (after
dropping the 'const' via strrchr()).
const char *pack_tmp_name,
struct pack_idx_entry **written_list,
uint32_t nr_written,
--
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] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname
On Fri, Feb 28, 2014 at 4:46 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Fri, Feb 28, 2014 at 3:28 AM, Sun He sunheeh...@gmail.com wrote:
Signed-off-by: Sun He sunheeh...@gmail.com
---
Nicely done.
Due to the necessary changes to finish_tmp_packfile(), the focus of
this patch has changed slightly from that suggested by the
microproject. It's really now about finish_tmp_packfile() rather than
finish_bulk_checkin(). As such, it may make sense to adjust the patch
subject to reflect this. For instance:
Subject: finish_tmp_packfile(): use strbuf for pathname construction
You may also want your commit message to explain why you chose this
approach over other legitimate approaches. For instance, your change
places extra burden on callers of finish_tmp_packfile() by leaking a
detail of its implementation: namely that it wants to manipulate
pathnames easily (via strbuf). An equally valid and more encapsulated
approach would be for finish_tmp_packfile() to accept a 'const char *'
and construct its own strbuf internally.
If the extra strbuf creation in the alternate approach is measurably
slower, then you could use that fact to justify your implementation
choice in the commit message. On the other hand, if it's not
measurably slower, then perhaps the more encapsulated approach with
cleaner API might be preferable.
More comments below.
] diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..72fb82b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -803,7 +803,7 @@ static void write_pack_file(void)
if (!pack_to_stdout) {
struct stat st;
- char tmpname[PATH_MAX];
+ struct strbuf tmpname = STRBUF_INIT;
/*
* Packs are runtime accessed in their mtime
@@ -823,26 +823,22 @@ static void write_pack_file(void)
utb.modtime = --last_mtime;
if (utime(pack_tmp_name, utb) 0)
warning(failed utime() on %s: %s,
- tmpname, strerror(errno));
+ pack_tmp_name,
strerror(errno));
Good catch finding this bug (as your commentary below states).
Ideally, each conceptual change should be presented distinctly from
others, so this bug should have its own patch (even though it's just a
one-liner).
}
- /* Enough space for -sha-1.pack? */
- if (sizeof(tmpname) = strlen(base_name) + 50)
- die(pack base name '%s' too long,
base_name);
- snprintf(tmpname, sizeof(tmpname), %s-, base_name);
+ strbuf_addf(tmpname, %s-, base_name);
if (write_bitmap_index) {
bitmap_writer_set_checksum(sha1);
bitmap_writer_build_type_index(written_list,
nr_written);
}
- finish_tmp_packfile(tmpname, pack_tmp_name,
+ finish_tmp_packfile(tmpname, pack_tmp_name,
written_list, nr_written,
pack_idx_opts, sha1);
if (write_bitmap_index) {
- char *end_of_name_prefix = strrchr(tmpname,
0);
- sprintf(end_of_name_prefix, %s.bitmap,
sha1_to_hex(sha1));
+ strbuf_addf(tmpname, %s.bitmap
,sha1_to_hex(sha1));
stop_progress(progress_state);
diff --git a/pack-write.c b/pack-write.c
index 9b8308b..2204aa9 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char
**pack_tmp_name)
return sha1fd(fd, *pack_tmp_name);
}
-void finish_tmp_packfile(char *name_buffer,
+void finish_tmp_packfile(struct strbuf *name_buffer,
const char *pack_tmp_name,
struct pack_idx_entry **written_list,
uint32_t nr_written,
@@ -344,7 +344,7 @@ void finish_tmp_packfile(char *name_buffer,
unsigned char sha1[])
{
const char *idx_tmp_name;
- char *end_of_name_prefix = strrchr(name_buffer, 0);
+ int pre_len = name_buffer-len;
Minor: Perhaps basename_len (or some such) would convey a bit more
meaning than pre_len.
if (adjust_shared_perm(pack_tmp_name))
die_errno(unable to make temporary pack file readable);
@@ -354,17 +354,21 @@ void finish_tmp_packfile(char *name_buffer,
if (adjust_shared_perm(idx_tmp_name))
die_errno(unable to make temporary index file readable);
- sprintf(end_of_name_prefix
Re: [PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname
On Fri, Feb 28, 2014 at 9:17 AM, 孙赫 sunheeh...@gmail.com wrote: 2014-02-28 21:12 GMT+08:00 Eric Sunshine [via git] ml-node+s661346n760450...@n2.nabble.com: On Fri, Feb 28, 2014 at 4:46 AM, Eric Sunshine [hidden email] wrote: On Fri, Feb 28, 2014 at 3:28 AM, Sun He [hidden email] wrote: Signed-off-by: Sun He [hidden email] --- Nicely done. Due to the necessary changes to finish_tmp_packfile(), the focus of this patch has changed slightly from that suggested by the microproject. It's really now about finish_tmp_packfile() rather than finish_bulk_checkin(). As such, it may make sense to adjust the patch subject to reflect this. For instance: Subject: finish_tmp_packfile(): use strbuf for pathname construction You may also want your commit message to explain why you chose this approach over other legitimate approaches. For instance, your change places extra burden on callers of finish_tmp_packfile() by leaking a detail of its implementation: namely that it wants to manipulate pathnames easily (via strbuf). An equally valid and more encapsulated approach would be for finish_tmp_packfile() to accept a 'const char *' and construct its own strbuf internally. If the extra strbuf creation in the alternate approach is measurably slower, then you could use that fact to justify your implementation choice in the commit message. On the other hand, if it's not measurably slower, then perhaps the more encapsulated approach with cleaner API might be preferable. Thank you for your explaination. I get the point. And I think if it is proved that the strbuf way is measurably slower. We should add a check for the length of string before we sprintf(). I'm not sure what you mean by checking the string length before sprintf(). My point was that if there are multiple ways of solving the same problem, it can be helpful for reviewers if your commit message explains why the solution you chose is better than the others. Slowness and/or cleanliness of API were just examples you might use in your commit message for justifying why you chose one approach over another. -- 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] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname
On Fri, Feb 28, 2014 at 10:54 AM, 孙赫 sunheeh...@gmail.com wrote:
2014-02-28 17:47 GMT+08:00 Eric Sunshine [via git]
ml-node+s661346n7604473...@n2.nabble.com:
On Fri, Feb 28, 2014 at 3:28 AM, Sun He [hidden email] wrote:
Signed-off-by: Sun He [hidden email]
---
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..72fb82b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -803,7 +803,7 @@ static void write_pack_file(void)
if (!pack_to_stdout) {
struct stat st;
- char tmpname[PATH_MAX];
+ struct strbuf tmpname = STRBUF_INIT;
/*
* Packs are runtime accessed in their mtime
@@ -823,26 +823,22 @@ static void write_pack_file(void)
utb.modtime = --last_mtime;
if (utime(pack_tmp_name, utb) 0)
warning(failed utime() on %s:
%s,
- tmpname, strerror(errno));
+ pack_tmp_name,
strerror(errno));
Good catch finding this bug (as your commentary below states).
Ideally, each conceptual change should be presented distinctly from
others, so this bug should have its own patch (even though it's just a
one-liner).
OK. Should I send this patch?
Yes, it is perfectly acceptable (and encouraged) to send this patch as
a submission distinct from your microproject.
--
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] rewrite bulk-checkin.c:finish_bulk_checkin() using strbuf
On Fri, Feb 28, 2014 at 1:27 PM, Faiz Kothari faiz.of...@gmail.com wrote: Thanks for the suggestions and remarks. [Administrivia: On this list, top-posting is frowned upon; inline responses are preferred.] I rewrote bulk-checkin.c:finish_bulk_checkin() using strbuf. But saw that Sun He has already implemented the same way I have done. Should I submit my implementation as a patch? Yes. The purpose of these micro-projects is to expose you to the Git project's development process so that you know what will be expected of you as a GSoC student, and to give the GSoC mentors an opportunity to evaluate your abilities and observe how you interact with the reviewers. Secondly, I tried implementing this WITHOUT changing the prototype of the function pack-write.c:finish_tmp_packfile(). For this I detached the buffer from strbuf in finish_bulk_checkin() using strbuf_detach() and passed it to finish_tmp_packfile(). Inside finish_tmp_packfile, I attached the same buffer to a local struct strbuf using strbuf_attach(). Now the problem is, two of the arguments to strbuf_attach() are 'alloc' and 'len' which are private members of the struct strbuf. But since I am just passing the detached buffer, the information of alloc and len is lost which is required at the time of attaching. I cannot think of any better way of using strbuf and NOT modify the prototype of finish_tmp_packfile() As a workaround, I can determine alloc = (strlen(buf) + 1) and len = strlen(buf) but AFAIK this is not always true and may break. Any suggestions? That's getting rather convoluted. You may want to ask yourself if it is really necessary for finish_tmp_packfile() to modify the buffer passed in by the caller or if finish_pack_file() should instead take responsibility for itself by allocating its own buffer (strbuf) in which to do path manipulation. -- 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] implemented strbuf_write_or_die()
On Sat, Mar 1, 2014 at 7:18 PM, Faiz Kothari faiz.of...@gmail.com wrote:
Subject: implemented strbuf_write_or_die()
Imperative tone is preferred. The commit message tells what it is
doing, not what it did.
Subject: introduce strbuf_write_or_die()
Signed-off-by: Faiz Kothari faiz.of...@gmail.com
---
Implementing this clearly distinguishes between writing a normal buffer
and writing a strbuf. Also, it provides an interface to write strbuf
directly without knowing the private members of strbuf, making strbuf
completely opaque. Also, makes the code more readable.
These three sentences which justify the change should go above the
--- line so they are recorded in the project history for future
readers to understand why the change was made. As for their actual
value, the first and third sentences are nebulous; the second sentence
may be suitable (although strbuf's buf and len are actually considered
public, so the justification not be supportable).
A couple other comments:
Justification is important because many topics are in-flight at any
given time. Changes like this one which touch an arbitrary number of
files may conflict with other in-flight topics, which places extra
burden on the project maintainer (Junio). The justification needs to
be strong enough to outweigh that added burden.
Introduction of a new function is conceptually distinct from the act
of updating existing callers to invoke it, thus this submission should
probably be decomposed into two or more patches where the first patch
introduces the function, and following patches convert existing
callers.
More below.
diff --git a/cache.h b/cache.h
index db223e8..8898bf4 100644
--- a/cache.h
+++ b/cache.h
@@ -1227,6 +1227,7 @@ extern int copy_fd(int ifd, int ofd);
extern int copy_file(const char *dst, const char *src, int mode);
extern int copy_file_with_time(const char *dst, const char *src, int mode);
extern void write_or_die(int fd, const void *buf, size_t count);
+extern void strbuf_write_or_die(const struct strbuf *sbuf, int fd);
You still have the layering violation here mentioned by both Johannes
and Michael. This declaration belongs in strbuf.h, not cache.h
Also, for bonus points, you should document this new function in
Documentation/technical/api-strbuf.txt.
extern int write_or_whine(int fd, const void *buf, size_t count, const char
*msg);
extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const
char *msg);
extern void fsync_or_die(int fd, const char *);
diff --git a/write_or_die.c b/write_or_die.c
index b50f99a..373450e 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -64,6 +64,16 @@ void write_or_die(int fd, const void *buf, size_t count)
}
}
+void strbuf_write_or_die(const struct strbuf *sbuf, int fd)
Ditto regarding the layering violation. This implementation belongs in strbuf.c.
+{
+ if(!sbuf)
+ fprintf(stderr, write error\n);
This is a programmer error, rather than a run-time I/O error. As such,
an assertion would be appropriate:
assert(sbuf);
+ else if (write_in_full(fd, sbuf-buf, sbuf-len) 0) {
+ check_pipe(errno);
+ die_errno(write error);
He Sun correctly pointed out in his review that you should simply
invoke the lower-level write_or_die() here rather than copying/pasting
its functionality. (You fixed it in an earlier version of the patch
but somehow reverted that fix when sending this version.)
+ }
+}
--
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] implemented strbuf_write_or_die()
On Sat, Mar 1, 2014 at 9:47 PM, Eric Sunshine sunsh...@sunshineco.com wrote: On Sat, Mar 1, 2014 at 7:18 PM, Faiz Kothari faiz.of...@gmail.com wrote: Implementing this clearly distinguishes between writing a normal buffer and writing a strbuf. Also, it provides an interface to write strbuf directly without knowing the private members of strbuf, making strbuf completely opaque. Also, makes the code more readable. These three sentences which justify the change should go above the --- line so they are recorded in the project history for future readers to understand why the change was made. As for their actual value, the first and third sentences are nebulous; the second sentence may be suitable (although strbuf's buf and len are actually considered public, so the justification not be supportable). ...justification *may* not be... -- 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] implemented strbuf_write_or_die()
On Sat, Mar 1, 2014 at 7:51 AM, He Sun sunheeh...@gmail.com wrote: 2014-03-01 19:21 GMT+08:00 Faiz Kothari faiz.of...@gmail.com: diff --git a/remote-curl.c b/remote-curl.c index 10cb011..dee8716 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads) if (start_command(client)) exit(1); if (preamble) - write_or_die(client.in, preamble-buf, preamble-len); + strbuf_write_or_die(client.in, preamble); if (heads) write_or_die(client.in, heads-buf, heads-len); This should be changed. May be you can use Ctrl-F to search write_or_die(). Or if you are using vim, use / and n to find all. It's not obvious from the patch fragment, but 'heads' is not a strbuf, so Faiz correctly left this invocation alone. -- 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] Replace tmpname with pack_tmp_name in warning. The developer mistook tmpname for pack_tmp_name.
On Sat, Mar 1, 2014 at 9:43 PM, Sun He sunheeh...@gmail.com wrote: Subject: Replace tmpname with pack_tmp_name in warning. The developer mistook tmpname for pack_tmp_name. The subject should be a short summary of the change, and the rest of the commit message before the --- line provides extra detail explaining the change. Signed-off-by: Sun He sunheeh...@gmail.com --- As tmpname is used without initialization, it should be a mistake. This is valid information for the commit message above the --- line. So, your full commit message might say something like this: Subject: write_pack_file: use correct variable in diagnostic 'pack_tmp_name' is the subject of the utime() check, so report it in the warning, not the uninitialized 'tmpname'. builtin/pack-objects.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index c733379..4922ce5 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -823,7 +823,7 @@ static void write_pack_file(void) utb.modtime = --last_mtime; if (utime(pack_tmp_name, utb) 0) warning(failed utime() on %s: %s, - tmpname, strerror(errno)); + pack_tmp_name, strerror(errno)); } /* Enough space for -sha-1.pack? */ -- 1.9.0.138.g2de3478.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] Place cache.h at the first place to match general rule
On Sat, Mar 1, 2014 at 9:18 PM, Sun He sunheeh...@gmail.com wrote:
Signed-off-by: Sun He sunheeh...@gmail.com
Helped-by: Duy Nguyen pclo...@gmail.com
Footers should follow a temporal order. For instance:
1. Duy helped you.
2. You revised your patch based upon his input.
3. You signed off before submitting the patch.
Hence, your Signed-off-by: should follow Helped-by:.
---
PATCH v2 Fix the spelling bug of general in subject as is suggested
by brain m.calson sand...@crustytoothpaste.net
There are two type of information you want to convey to readers:
1. Explanation and justification of the change itself. This is
recorded for all time in the project history as the commit message. It
is placed above the --- line.
2. Commentary related to this version / submission of the patch which
is not likely to be helpful or meaningful to people reading the
official project history via the commit messages. It is placed below
the --- line.
Explaining what you changed since the previous version of the patch,
as you do above, is a good thing. It's not meaningful once the patch
is officially accepted into the project; it's only meaningful to
people following the progression of the patch on the mailing list, so
it definitely belongs below the --- line, as you did here.
However...
The general rule is if cache.h or git-compat-util.h is included,
it is the first #include.
This information explains the patch's purpose, thus it is relevant to
the project history. It belongs above the --- line.
I parsed all the source files, and find many files start with builtin.h.
And git-compat-util.h is the first in it. So they don't need any change.
This could go either way. It tells how you arrived at this version of
the patch (relevant below ---), but also explains why the patch does
not have to touch additional files (relevant above ---). It's
probably okay to leave it below ---.
sigchain.c and test-sigchain.c are started with sigchain.h
I checked sigchain.h, and it didn't import any bug.
But to keep consistant with general rule, we should take this patch.
Commentary suitable for below ---.
Thanks.
sigchain.c | 2 +-
test-sigchain.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sigchain.c b/sigchain.c
index 1118b99..faa375d 100644
--- a/sigchain.c
+++ b/sigchain.c
@@ -1,5 +1,5 @@
-#include sigchain.h
#include cache.h
+#include sigchain.h
#define SIGCHAIN_MAX_SIGNALS 32
diff --git a/test-sigchain.c b/test-sigchain.c
index 42db234..e499fce 100644
--- a/test-sigchain.c
+++ b/test-sigchain.c
@@ -1,5 +1,5 @@
-#include sigchain.h
#include cache.h
+#include sigchain.h
#define X(f) \
static void f(int sig) { \
--
1.9.0.138.g2de3478.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][GSoC] git-compat-util.h:rewrite skip_prefix() as loop
Thanks for the submission. Minor comments below to give you a taste of
what it's like to contribute to this project...
On Sat, Mar 1, 2014 at 5:42 PM, kgeorgiou kyriakos.a.georg...@gmail.com wrote:
Subject: git-compat-util.h:rewrite skip_prefix() as loop
Space after colon. You might be able to provide more information in
the short summary of the subject. Perhaps something like this:
Subject: skip_prefix(): avoid scanning 'prefix' twice
Rewritten git-compat-util.h:skip_prefix() as a loop, so that it doesn't have
to
scan through the prefix string twice as a miniproject for GSoC 2014.
Good description. In this project, use imperative tone:
Rewrite skip_prefix() as loop...
(I've just noticed that this miniproject has already been tackled by another
contributor, if that's a problem I can pick something else.)
That's okay. The purpose of the miniprojects is for you to get a taste
of what it's like to contribute to this project and to understand what
will be expected of you as a GSoC student; and to give the GSoC
mentors a chance to judge your abilities and observe how you interact
with reviewers.
Looking forward to any kind of feedback.
- Kyriakos Georgiou
There are two types of information you want to convey to readers. (1)
Patch description, explanation, justification before the --- line
which is recorded in the permanent project history. (2) Commentary
below the --- line for readers of the patch but not of interest once
the patch is accepted officially. The as a miniproject for GSoC and
all lines following it are just commentary which should go below the
--- line.
Signed-off-by: kgeorgiou kyriakos.a.georg...@gmail.com
This project prefers real names, so:
Signed-off-by: Kyriakos Georgiou ...
---
git-compat-util.h | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 614a5e9..713f37a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -357,8 +357,11 @@ extern int suffixcmp(const char *str, const char
*suffix);
static inline const char *skip_prefix(const char *str, const char *prefix)
{
- size_t len = strlen(prefix);
- return strncmp(str, prefix, len) ? NULL : str + len;
+ while(*prefix *str == *prefix) {
Style nit: space after 'while'.
+ str++;
+ prefix++;
+ }
+ return *prefix ? NULL : str;
}
#if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
--
1.8.3.2
--
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] git-compat-util.h:rewrite skip_prefix() as loop
Thanks for the submission. Minor comments below to give you a taste of
what it's like to contribute to this project...
On Sat, Mar 1, 2014 at 8:32 AM, Siddharth Goel siddharth98...@gmail.com wrote:
Rewrote skip_prefix() function so that prefix is scanned once.
Good description. In this project, use imperative tone, so say
Rewrite skip_prefix()... as you did in the subject. In fact, this
description is short enough and conveys sufficient information that it
could just be placed in the subject as the entire commit message.
Subject: skip_prefix: rewrite so that prefix is scanned once
Signed-off-by: Siddharth Goel siddharth98...@gmail.com
---
git-compat-util.h | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 614a5e9..550dce3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -357,8 +357,11 @@ extern int suffixcmp(const char *str, const char
*suffix);
static inline const char *skip_prefix(const char *str, const char *prefix)
{
- size_t len = strlen(prefix);
- return strncmp(str, prefix, len) ? NULL : str + len;
+ while (*prefix != '\0' *str == *prefix) {
+ str++;
+ prefix++;
+ }
+ return (*prefix == '\0' ? str : NULL);
}
#if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
--
1.9.0.138.g2de3478.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] git-compat-util.h:rewrite skip_prefix() as loop
On Sat, Mar 1, 2014 at 10:22 PM, Siddharth Goel siddharth98...@gmail.com wrote: To my surprise, git format-patch had removed the Git Notes that I had put to my commit (regarding GSoC). I have written this patch as a part of the GSoC 2014 MicroProject for Git. You probably wanted to use the --notes option with format-patch. Going through the mail-chain I observed that many students have attempted this Microproject. So is it ok if I stick to this Microproject or should I go with another one? That's okay. The purpose of the miniprojects is for you to get a taste of what it's like to contribute to this project and to understand what will be expected of you as a GSoC student; and to give the GSoC mentors a chance to judge your abilities and observe how you interact with reviewers. -- 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 2/3] rebase: accept -number as another way of saying HEAD~number
On Sat, Mar 1, 2014 at 9:53 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: This is rev-list style, where people can do git rev-list -3 in addition to git rev-list HEAD~3. A lot of commands are driven by the revision machinery and also accept this form. This addition to rebase is just for convenience. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- diff --git a/git-rebase.sh b/git-rebase.sh index 5f6732b..33face1 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -342,6 +346,11 @@ do esac shift done +if [ -n $NUM ] With the exception of one errant if [...], git-rebase.sh uniformly uses if test +then + test $# -gt 0 usage + set -- $@ HEAD~$NUM +fi test $# -gt 2 usage if test -n $cmd diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 6d94b1f..11db7ea 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -215,4 +215,10 @@ test_expect_success 'rebase commit with an ancient timestamp' ' grep author .* 34567 +0600$ actual ' +test_expect_success 'rebase -number' ' + git reset --hard + test_must_fail git rebase -2 HEAD^^ + git rebase -2 +' + test_done -- 1.9.0.40.gaa8c3ea -- 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 2/3] rebase: accept -number as another way of saying HEAD~number
On Sat, Mar 1, 2014 at 9:53 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
This is rev-list style, where people can do git rev-list -3 in
addition to git rev-list HEAD~3. A lot of commands are driven by the
revision machinery and also accept this form. This addition to rebase
is just for convenience.
I'm seeing some pretty strange results with this. If I use -1, -2, or
-3 then it rebases the expected commits, however, -4 gives me 9
commits, and -5 rebases 35 commits. Am I misunderstanding how this
works?
I'm testing on a branch based on master with these three patches applied.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Documentation/git-rebase.txt | 3 +++
git-rebase.sh| 9 +
t/t3400-rebase.sh| 6 ++
3 files changed, 18 insertions(+)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 2a93c64..52c3561 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -223,6 +223,9 @@ As a special case, you may use A\...B as a shortcut for
the
merge base of A and B if there is exactly one merge base. You can
leave out at most one of A and B, in which case it defaults to HEAD.
+-number::
+ Specify upstream as HEAD~number.
+
upstream::
Upstream branch to compare against. May be any valid commit,
not just an existing branch name. Defaults to the configured
diff --git a/git-rebase.sh b/git-rebase.sh
index 5f6732b..33face1 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -43,6 +43,7 @@ continue! continue
abort! abort and check out the original branch
skip! skip current patch and continue
edit-todo! edit the todo list during an interactive rebase
+-NUM equivalent of HEAD~NUM
. git-sh-setup
. git-sh-i18n
@@ -335,6 +336,9 @@ do
--gpg-sign=*)
gpg_sign_opt=-S${1#--gpg-sign=}
;;
+ -NUM=*)
+ NUM=${1#-NUM=}
+ ;;
--)
shift
break
@@ -342,6 +346,11 @@ do
esac
shift
done
+if [ -n $NUM ]
+then
+ test $# -gt 0 usage
+ set -- $@ HEAD~$NUM
+fi
test $# -gt 2 usage
if test -n $cmd
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 6d94b1f..11db7ea 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -215,4 +215,10 @@ test_expect_success 'rebase commit with an ancient
timestamp' '
grep author .* 34567 +0600$ actual
'
+test_expect_success 'rebase -number' '
+ git reset --hard
+ test_must_fail git rebase -2 HEAD^^
+ git rebase -2
+'
+
test_done
--
1.9.0.40.gaa8c3ea
--
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
--
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 2/3] rebase: accept -number as another way of saying HEAD~number
On Sun, Mar 2, 2014 at 3:53 AM, Eric Sunshine sunsh...@sunshineco.com wrote: On Sat, Mar 1, 2014 at 9:53 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: This is rev-list style, where people can do git rev-list -3 in addition to git rev-list HEAD~3. A lot of commands are driven by the revision machinery and also accept this form. This addition to rebase is just for convenience. I'm seeing some pretty strange results with this. If I use -1, -2, or -3 then it rebases the expected commits, however, -4 gives me 9 commits, and -5 rebases 35 commits. Am I misunderstanding how this works? Nevermind. I wasn't paying attention to the fact that I was attempting to rebase merges. -- 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 3/3] rebase: new convenient option to edit a single commit
On Sat, Mar 1, 2014 at 9:53 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
git rebase -e XYZ is basically the same as
EDITOR=sed -i '1s/pick XYZ/edit XYZ/' $@ \
git rebase -i XYZ^
In English, it prepares the todo list for you to edit only commit XYZ
to save your time. The time saving is only significant when you edit a
lot of commits separately.
Should this accept multiple -e arguments? Based upon the above
justification, it sounds like it should, and I think that would be the
intuitive expectation (at least for me).
The current implementation, however, is broken with multiple -e arguments. With:
git rebase -i -e older -e newer
'newer' is ignored entirely. However, with:
git rebase -i -e newer -e older
it errors out when rewriting the todo list:
expected to find 'edit older' line but did not
An implementation supporting multiple -e arguments would need to
ensure that the todo list extends to the oldest rev specified by any
-e argument.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Documentation/git-rebase.txt | 4
git-rebase--interactive.sh | 17 ++---
git-rebase.sh| 10 ++
3 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 52c3561..b8c263d 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -359,6 +359,10 @@ unless the `--fork-point` option is specified.
user edit that list before rebasing. This mode can also be used to
split commits (see SPLITTING COMMITS below).
+-e::
+--edit-one::
+ Prepare the todo list to edit only the commit at upstream
+
-p::
--preserve-merges::
Instead of ignoring merges, try to recreate them.
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a1adae8..4762d57 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1040,9 +1040,20 @@ fi
has_action $todo ||
die_abort Nothing to do
-cp $todo $todo.backup
-git_sequence_editor $todo ||
- die_abort Could not execute editor
+if test -n $edit_one
+then
+ edit_one=`git rev-parse --short $edit_one`
+ sed 1s/pick $edit_one /edit $edit_one / $todo $todo.new ||
+ die_abort failed to update todo list
+ grep ^edit $edit_one $todo.new /dev/null ||
+ die_abort expected to find 'edit $edit_one' line but did not
+ mv $todo.new $todo ||
+ die_abort failed to update todo list
+else
+ cp $todo $todo.backup
+ git_sequence_editor $todo ||
+ die_abort Could not execute editor
+fi
has_action $todo ||
die_abort Nothing to do
diff --git a/git-rebase.sh b/git-rebase.sh
index 33face1..b8b6aa9 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -32,6 +32,7 @@ verify allow pre-rebase hook to run
rerere-autoupdate allow rerere to update index with resolved conflicts
root! rebase all reachable commits up to the root(s)
autosquash move commits that begin with squash!/fixup! under -i
+e,edit-one!generate todo list to edit this commit
committer-date-is-author-date! passed to 'git am'
ignore-date! passed to 'git am'
whitespace=! passed to 'git apply'
@@ -339,6 +340,10 @@ do
-NUM=*)
NUM=${1#-NUM=}
;;
+ --edit-one)
+ interactive_rebase=explicit
+ edit_one=t
+ ;;
--)
shift
break
@@ -463,6 +468,11 @@ then
;;
*) upstream_name=$1
shift
+ if test -n $edit_one
+ then
+ edit_one=$upstream_name
+ upstream_name=$upstream_name^
+ fi
;;
esac
upstream=$(peel_committish ${upstream_name}) ||
--
1.9.0.40.gaa8c3ea
--
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
--
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 3/3] rebase: new convenient option to edit a single commit
On Sun, Mar 2, 2014 at 4:04 AM, Eric Sunshine sunsh...@sunshineco.com wrote: On Sat, Mar 1, 2014 at 9:53 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: git rebase -e XYZ is basically the same as EDITOR=sed -i '1s/pick XYZ/edit XYZ/' $@ \ git rebase -i XYZ^ In English, it prepares the todo list for you to edit only commit XYZ to save your time. The time saving is only significant when you edit a lot of commits separately. Should this accept multiple -e arguments? Based upon the above justification, it sounds like it should, and I think that would be the intuitive expectation (at least for me). The current implementation, however, is broken with multiple -e arguments. With: git rebase -i -e older -e newer 'newer' is ignored entirely. However, with: git rebase -i -e newer -e older it errors out when rewriting the todo list: expected to find 'edit older' line but did not An implementation supporting multiple -e arguments would need to ensure that the todo list extends to the oldest rev specified by any -e argument. Of course, I'm misreading and abusing the intention of -e as if it is -e arg. -- 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] write_pack_file: use correct variable in diagnostic
On Sun, Mar 2, 2014 at 2:30 AM, Sun He sunheeh...@gmail.com wrote: 'pack_tmp_name' is the subject of the utime() check, so report it in the warning, not the uninitialized 'tmpname' Signed-off-by: Sun He sunheeh...@gmail.com --- Changing the subject and adding valid information as tutored by Eric Sunshine. Thanks to him. builtin/pack-objects.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index c733379..4922ce5 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -823,7 +823,7 @@ static void write_pack_file(void) utb.modtime = --last_mtime; if (utime(pack_tmp_name, utb) 0) warning(failed utime() on %s: %s, - tmpname, strerror(errno)); + pack_tmp_name, strerror(errno)); } /* Enough space for -sha-1.pack? */ -- 1.9.0.138.g2de3478.dirty Nicely done. Everything is where it ought to be. As this is an actual bug fix (not just a GSoC microproject): Reviewed-by: Eric Sunshine sunsh...@sunshineco.com -- 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] skip_prefix:rewrite so that prefix is scanned once
On Sun, Mar 2, 2014 at 10:03 AM, Siddharth Goel
siddharth98...@gmail.com wrote:
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Siddharth Goel siddharth98...@gmail.com
---
Thanks a lot Eric for your valuable comments. Please let me know if there is
anything else which needs to be modified in this patch.
Other than suggesting that you insert a space after the colon in the
subject, I don't have any further comments on this patch. That's
probably not a reason to resend, though.
As general commentary to any potential GSoC students reading this: do
what you can to ease the review process. One way to do so is to supply
commentary below the --- line (or in the cover letter) explaining
what changed in the present patch compared with the previous attempt.
Bonus points for providing a reference to the previous version of the
submission, like this [1].
[1]: http://thread.gmane.org/gmane.comp.version-control.git/243097
git-compat-util.h | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 614a5e9..550dce3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -357,8 +357,11 @@ extern int suffixcmp(const char *str, const char
*suffix);
static inline const char *skip_prefix(const char *str, const char *prefix)
{
- size_t len = strlen(prefix);
- return strncmp(str, prefix, len) ? NULL : str + len;
+ while (*prefix != '\0' *str == *prefix) {
+ str++;
+ prefix++;
+ }
+ return (*prefix == '\0' ? str : NULL);
}
#if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
--
1.9.0.138.g2de3478.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 v4 01/27] path.c: make get_pathname() return strbuf instead of static buffer
On Sat, Mar 1, 2014 at 7:12 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
We've been avoiding PATH_MAX whenever possible. This patch makes
get_pathname() return a strbuf and updates the callers to take
advantage of this. The code is simplified as we no longer need to
worry about buffer overflow.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
diff --git a/path.c b/path.c
index 24594c4..5346700 100644
--- a/path.c
+++ b/path.c
@@ -49,85 +60,70 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
return cleanup_path(buf);
}
-static char *vsnpath(char *buf, size_t n, const char *fmt, va_list args)
+static void vsnpath(struct strbuf *buf, const char *fmt, va_list args)
{
const char *git_dir = get_git_dir();
- size_t len;
-
- len = strlen(git_dir);
- if (n len + 1)
- goto bad;
- memcpy(buf, git_dir, len);
- if (len !is_dir_sep(git_dir[len-1]))
- buf[len++] = '/';
- len += vsnprintf(buf + len, n - len, fmt, args);
- if (len = n)
- goto bad;
- return cleanup_path(buf);
-bad:
- strlcpy(buf, bad_path, n);
- return buf;
+ strbuf_addstr(buf, git_dir);
+ if (buf-len !is_dir_sep(buf-buf[buf-len - 1]))
+ strbuf_addch(buf, '/');
+ strbuf_vaddf(buf, fmt, args);
+ strbuf_cleanup_path(buf);
}
There's a slight semantic change here. The old code overwrote 'buf',
but the new code appends to 'buf'. For someone familiar with
sprintf(), or typical va_list or variadic functions there may be an
intuitive expectation that 'buf' will be overwritten. Should this code
be doing strbuf_reset() as its first action (or should that be the
responsibility of the caller who is reusing 'buff')?
char *mkpath(const char *fmt, ...)
{
va_list args;
- unsigned len;
- char *pathname = get_pathname();
-
+ struct strbuf *pathname = get_pathname();
va_start(args, fmt);
- len = vsnprintf(pathname, PATH_MAX, fmt, args);
+ strbuf_vaddf(pathname, fmt, args);
va_end(args);
- if (len = PATH_MAX)
- return bad_path;
- return cleanup_path(pathname);
+ return cleanup_path(pathname-buf);
}
Prior to this change, it was possible (though probably not
recommended) for a caller to append gunk to the returned path up to
PATH_MAX without worrying about stomping memory. With the change, this
is no longer true. Should the function be changed to return 'const
char *' to enforce this restriction?
char *git_path(const char *fmt, ...)
{
- char *pathname = get_pathname();
+ struct strbuf *pathname = get_pathname();
va_list args;
- char *ret;
-
va_start(args, fmt);
- ret = vsnpath(pathname, PATH_MAX, fmt, args);
+ vsnpath(pathname, fmt, args);
va_end(args);
- return ret;
+ return pathname-buf;
}
Ditto.
void home_config_paths(char **global, char **xdg, char *file)
@@ -158,41 +154,27 @@ void home_config_paths(char **global, char **xdg, char
*file)
char *git_path_submodule(const char *path, const char *fmt, ...)
{
- char *pathname = get_pathname();
- struct strbuf buf = STRBUF_INIT;
+ struct strbuf *buf = get_pathname();
...
+ strbuf_cleanup_path(buf);
+ return buf-buf;
}
And here?
int validate_headref(const char *path)
--
1.9.0.40.gaa8c3ea
--
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 1/2] Introduce strbuf_write_or_die()
On Sun, Mar 2, 2014 at 2:34 AM, Faiz Kothari faiz.of...@gmail.com wrote:
Signed-off-by: Faiz Kothari faiz.of...@gmail.com
Place your sign off below the commit message.
Introduced a new function strbuf.c:strbuf_write_or_die()
to the strbuf family of functions. Now use this API instead
of write_or_die.c:write_or_die()
You want to explain what this patch is doing in imperative tone. Use
Introduce rather than Introduced. The first sentence correctly
states what the patch is doing, however, the second sentence explains
what the next patch is doing, so it doesn't belong here. So, your
commit message for this patch might become:
Subject: strbuf: introduce strbuf_write_or_die()
Add strbuf convenience wrapper around lower-level write_or_die().
---
Hi,
Thanks for the suggestions and feedbacks.
As Johannes Sixt pointed out, the function is now defined
in strbuf.c and prototype added to strbuf.h
Also, replaced if(!sbuf) with assert(sbuf) and split the patch into two
as pointed out by Eric Sunshine.
Good explanation of what changed since the last attempt.
As far as justification is concerned, I am not able to come up with
a satisfactory justification. Apart from, that it makes life of the
programmer a little easier and if we add a few more functions
to thestrbuf API, we can make strbuf completely opaque. I am open
to views and since I haven't used this API extensively, I cannot
comment for what is missing and what is required. But I am going through it.
Also, once this patch is OK, I'll add documentation for the API.
It's a good idea to add documentation when you add the function
itself, otherwise reviewers will have to wait yet another round to
review that addition. In this case, the documentation will likely be
one line, so it shouldn't be a particular burden to write it.
Thanks again for the feedback.
strbuf.c |6 ++
strbuf.h |1 +
2 files changed, 7 insertions(+)
diff --git a/strbuf.c b/strbuf.c
index 83caf4a..337a70c 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -477,6 +477,12 @@ int strbuf_read_file(struct strbuf *sb, const char
*path, size_t hint)
return len;
}
+void strbuf_write_or_die(const struct strbuf *sb, int fd)
+{
+ assert(sb);
+ write_or_die(fd, sb-buf, sb-len);
+}
Nice. Much better than previous versions of the patch.
void strbuf_add_lines(struct strbuf *out, const char *prefix,
const char *buf, size_t size)
{
diff --git a/strbuf.h b/strbuf.h
index 73e80ce..6aadb6d 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -156,6 +156,7 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE
*);
/* XXX: if read fails, any partial read is undone */
extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t
hint);
+extern void strbuf_write_or_die(const struct strbuf *sb, int fd);
extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
extern int strbuf_getwholeline(struct strbuf *, FILE *, int);
--
1.7.9.5
--
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 2/2] use strbuf_write_or_die()
On Sun, Mar 2, 2014 at 2:34 AM, Faiz Kothari faiz.of...@gmail.com wrote:
Signed-off-by: Faiz Kothari faiz.of...@gmail.com
Sign off below commit message.
Used strbuf.c:strbuf_write_or_die() instead of
write_or_die.c:write_or_die() at relevant places.
Imperative: Use strbuf...
Otherwise, the patch looks okay.
---
builtin/cat-file.c |2 +-
builtin/notes.c|6 +++---
builtin/receive-pack.c |2 +-
builtin/send-pack.c|2 +-
builtin/stripspace.c |2 +-
builtin/tag.c |2 +-
bundle.c |2 +-
credential-store.c |2 +-
fetch-pack.c |2 +-
http-backend.c |2 +-
remote-curl.c |6 +++---
11 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index d5a93e0..d07a0be 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -255,7 +255,7 @@ static int batch_one_object(const char *obj_name, struct
batch_options *opt,
strbuf_expand(buf, opt-format, expand_format, data);
strbuf_addch(buf, '\n');
- write_or_die(1, buf.buf, buf.len);
+ strbuf_write_or_die(buf, 1);
strbuf_release(buf);
if (opt-print_contents) {
diff --git a/builtin/notes.c b/builtin/notes.c
index 2b24d05..a208d56 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -140,7 +140,7 @@ static void write_commented_object(int fd, const unsigned
char *object)
if (strbuf_read(buf, show.out, 0) 0)
die_errno(_(could not read 'show' output));
strbuf_add_commented_lines(cbuf, buf.buf, buf.len);
- write_or_die(fd, cbuf.buf, cbuf.len);
+ strbuf_write_or_die(cbuf, fd);
strbuf_release(cbuf);
strbuf_release(buf);
@@ -167,14 +167,14 @@ static void create_note(const unsigned char *object,
struct msg_arg *msg,
die_errno(_(could not create file '%s'), path);
if (msg-given)
- write_or_die(fd, msg-buf.buf, msg-buf.len);
+ strbuf_write_or_die((msg-buf), fd);
else if (prev !append_only)
write_note_data(fd, prev);
strbuf_addch(buf, '\n');
strbuf_add_commented_lines(buf, note_template,
strlen(note_template));
strbuf_addch(buf, '\n');
- write_or_die(fd, buf.buf, buf.len);
+ strbuf_write_or_die(buf, fd);
write_commented_object(fd, object);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 85bba35..d590993 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1114,7 +1114,7 @@ static void report(struct command *commands, const char
*unpack_status)
if (use_sideband)
send_sideband(1, 1, buf.buf, buf.len, use_sideband);
else
- write_or_die(1, buf.buf, buf.len);
+ strbuf_write_or_die(buf, 1);
strbuf_release(buf);
}
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f420b74..f26ba21 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -86,7 +86,7 @@ static void print_helper_status(struct ref *ref)
}
strbuf_addch(buf, '\n');
- write_or_die(1, buf.buf, buf.len);
+ strbuf_write_or_die(buf, 1);
}
strbuf_release(buf);
}
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 1259ed7..33b7f85 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -115,7 +115,7 @@ int cmd_stripspace(int argc, const char **argv, const
char *prefix)
else
comment_lines(buf);
- write_or_die(1, buf.buf, buf.len);
+ strbuf_write_or_die(buf, 1);
strbuf_release(buf);
return 0;
}
diff --git a/builtin/tag.c b/builtin/tag.c
index 74d3780..53ab280 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -349,7 +349,7 @@ static void create_tag(const unsigned char *object, const
char *tag,
strbuf_commented_addf(buf, _(tag_template),
comment_line_char);
else
strbuf_commented_addf(buf,
_(tag_template_nocleanup), comment_line_char);
- write_or_die(fd, buf.buf, buf.len);
+ strbuf_write_or_die(buf, fd);
strbuf_release(buf);
}
close(fd);
diff --git a/bundle.c b/bundle.c
index e99065c..c8bddd8 100644
--- a/bundle.c
+++ b/bundle.c
@@ -279,7 +279,7 @@ int create_bundle(struct bundle_header *header, const
char *path,
while (strbuf_getwholeline(buf, rls_fout, '\n') != EOF) {
unsigned char sha1[20];
if (buf.len 0 buf.buf[0] == '-') {
- write_or_die(bundle_fd, buf.buf, buf.len);
+
Re: [PATCH] Replace memcpy with hashcpy when lengths defined
On Sun, Mar 2, 2014 at 2:19 PM, Alberto albco...@gmail.com wrote:
From: Alberto Corona albco...@gmail.com
Replaced memcpy with hashcpy where lengts in memcpy
are already defined.
This doesn't really explain what this patch is attempting to do. What
does lengths already defined mean? It's also misleading if taken
literally (as seen below).
Instead, you should say something about the purpose of the change. For
instance, explain that the change takes advantage of the abstraction
provided by hashcpy() rather than hardcoding knowledge about a
particular hash representation.
More below.
Signed-off-by: Alberto Corona albco...@gmail.com
---
bundle.c| 2 +-
grep.c | 2 +-
refs.c | 2 +-
sha1_name.c | 4 ++--
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/bundle.c b/bundle.c
index e99065c..7809fbb 100644
--- a/bundle.c
+++ b/bundle.c
@@ -19,7 +19,7 @@ static void add_to_ref_list(const unsigned char *sha1,
const char *name,
list-list = xrealloc(list-list,
list-alloc * sizeof(list-list[0]));
}
- memcpy(list-list[list-nr].sha1, sha1, 20);
+ hashcpy(list-list[list-nr].sha1, sha1);
list-list[list-nr].name = xstrdup(name);
list-nr++;
}
diff --git a/grep.c b/grep.c
index c668034..f5101f7 100644
--- a/grep.c
+++ b/grep.c
@@ -1650,7 +1650,7 @@ void grep_source_init(struct grep_source *gs, enum
grep_source_type type,
break;
case GREP_SOURCE_SHA1:
gs-identifier = xmalloc(20);
- memcpy(gs-identifier, identifier, 20);
+ hashcpy(gs-identifier, identifier);
break;
case GREP_SOURCE_BUF:
gs-identifier = NULL;
diff --git a/refs.c b/refs.c
index 89228e2..f90b7ea 100644
--- a/refs.c
+++ b/refs.c
@@ -1222,7 +1222,7 @@ static int resolve_gitlink_packed_ref(struct ref_cache
*refs,
if (ref == NULL)
return -1;
- memcpy(sha1, ref-u.value.sha1, 20);
+ hashcpy(sha1, ref-u.value.sha1);
return 0;
}
diff --git a/sha1_name.c b/sha1_name.c
index 6fca869..3f5010f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -111,7 +111,7 @@ static void find_short_object_filename(int len, const
char *hex_pfx, struct disa
continue;
if (memcmp(de-d_name, hex_pfx + 2, len - 2))
continue;
- memcpy(hex + 2, de-d_name, 38);
+ hashcpy(hex + 2, de-d_name);
This can't be correct. hashcpy() copies the binary representation of a
hash (which is currently 20 bytes, as seen in the implementation of
hashcpy() in cache.h). The fact that this particular memcpy() is
copying 38 bytes should be a clue that something is different here. In
fact, for this case, 'hex' is a 40-character textual representation of
the hash, thus not suitable for hashcpy().
if (!get_sha1_hex(hex, sha1))
update_candidates(ds, sha1);
}
@@ -373,7 +373,7 @@ const char *find_unique_abbrev(const unsigned char *sha1,
int len)
static char hex[41];
exists = has_sha1_file(sha1);
- memcpy(hex, sha1_to_hex(sha1), 40);
+ hashcpy(hex, sha1_to_hex(sha1));
Same as above.
if (len == 40 || !len)
return hex;
while (len 40) {
--
1.9.0.138.g2de3478.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] branch.c: change install_branch_config() to use skip_prefix()
Thanks for the submission. Comments below to give you a taste of the
Git review process...
On Sun, Mar 2, 2014 at 10:55 AM, Guanglin Xu mzguang...@gmail.com wrote:
Change install_branch_config() to use skip_prefix() and make it conform to
the usage of previous starts_with(). This is because the proper usage of
skip_prefix() overrides the functionality of starts_with(). Thorough
replacements may finally remove the starts_with() function and reduce code
redundency.
Justifying a change is certainly a good idea, however, the above
reasoning for this particular change is off mark. See below.
Also, wrap commit message lines to 65-70 characters or so.
Signed-off-by: Guanglin Xu mzguang...@gmail.com
---
branch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/branch.c b/branch.c
index 723a36b..ca4e824 100644
--- a/branch.c
+++ b/branch.c
@@ -50,7 +50,7 @@ static int should_setup_rebase(const char *origin)
void install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
{
const char *shortname = remote + 11;
- int remote_is_branch = starts_with(remote, refs/heads/);
+ int remote_is_branch = (NULL != skip_prefix(remote ,refs/heads/));
This actually makes the code more difficult to read and understand.
There's a more fundamental reason to use skip_prefix() here. See if
you can figure it out. (Hint: shortname)
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
--
1.9.0
Hi,
I am Guanglin Xu. I plan to apply for GSoC 2014.
This patch is in accordance with the idea#2 of GSoC2014 Microproject. Any
comments are welcomed.
This sort of commentary, which is appropriate to the email discussion
but not meant for permanent project history, should be placed
immediately below the --- line following your sign-off.
--
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 v4 02/27] Convert git_snpath() to strbuf_git_path()
On Sat, Mar 1, 2014 at 7:12 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
In the previous patch, git_snpath() is modified to allocate a new
strbuf buffer because vsnpath() needs that. But that makes it awkward
because git_snpath() receives a pre-allocated buffer from outside and
has to copy data back. Rename it to strbuf_git_path() and make it
receive strbuf directly.
The conversion from git_snpath() to git_path() in
update_refs_for_switch() is safe because that function does not keep
any pointer to the round-robin buffer pool allocated by
get_pathname().
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
diff --git a/refs.c b/refs.c
index 89228e2..434bd5e 100644
--- a/refs.c
+++ b/refs.c
@@ -2717,17 +2729,19 @@ static int copy_msg(char *buf, const char *msg)
return cp - buf;
}
-int log_ref_setup(const char *refname, char *logfile, int bufsize)
+int log_ref_setup(const char *refname, struct strbuf *sb_logfile)
{
int logfd, oflags = O_APPEND | O_WRONLY;
+ const char *logfile;
- git_snpath(logfile, bufsize, logs/%s, refname);
+ strbuf_git_path(sb_logfile, logs/%s, refname);
+ logfile = sb_logfile-buf;
if (log_all_ref_updates
(starts_with(refname, refs/heads/) ||
starts_with(refname, refs/remotes/) ||
starts_with(refname, refs/notes/) ||
!strcmp(refname, HEAD))) {
- if (safe_create_leading_directories(logfile) 0)
+ if (safe_create_leading_directories(sb_logfile-buf) 0)
At this point, 'logfile' is still 'sb_logfile-buf', so do you really
need this change?
return error(unable to create directory for %s,
logfile);
oflags |= O_CREAT;
@@ -2762,20 +2776,22 @@ static int log_ref_write(const char *refname, const
unsigned char *old_sha1,
int logfd, result, written, oflags = O_APPEND | O_WRONLY;
unsigned maxlen, len;
int msglen;
- char log_file[PATH_MAX];
+ struct strbuf sb_log_file = STRBUF_INIT;
+ const char *log_file;
char *logrec;
const char *committer;
if (log_all_ref_updates 0)
log_all_ref_updates = !is_bare_repository();
- result = log_ref_setup(refname, log_file, sizeof(log_file));
+ result = log_ref_setup(refname, sb_log_file);
if (result)
- return result;
+ goto done;
+ log_file = sb_log_file.buf;
logfd = open(log_file, oflags);
if (logfd 0)
- return 0;
+ goto done;
msglen = msg ? strlen(msg) : 0;
committer = git_committer_info(0);
maxlen = strlen(committer) + msglen + 100;
--
1.9.0.40.gaa8c3ea
--
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 v4 05/27] Make git_path() aware of file relocation in $GIT_DIR
On Sat, Mar 1, 2014 at 7:12 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
We allow the user to relocate certain paths out of $GIT_DIR via
environment variables, e.g. GIT_OBJECT_DIRECTORY, GIT_INDEX_FILE and
GIT_GRAFT_FILE. All callers are not supposed to use git_path() or
All callers are not is unusually difficult to understand. Changing
it to Callers are not simplifies.
git_pathdup() to get those paths. Instead they must use
get_object_directory(), get_index_file() and get_graft_file()
respectively. This is inconvenient and could be missed in review
(there's git_path(objects/info/alternates) somewhere in
(for example, there's... reads a bit better.
sha1_file.c).
This patch makes git_path() and git_pathdup() understand those
environment variables. So if you set GIT_OBJECT_DIRECTORY to /foo/bar,
git_path(objects/abc) should return /tmp/bar/abc. The same is done
I guess you mean it should return /foo/bar/abc.
for the two remaining env variables.
git rev-parse --git-path is the wrapper for script use.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Documentation/git-rev-parse.txt | 5 +
builtin/rev-parse.c | 7 +++
cache.h | 1 +
environment.c | 9 ++--
path.c | 46
+
t/t0060-path-utils.sh | 19 +
6 files changed, 85 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 0d2cdcd..33e4e90 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -232,6 +232,11 @@ print a message to stderr and exit with nonzero status.
repository. If path is a gitfile then the resolved path
to the real repository is printed.
+--git-path path::
+ Resolve $GIT_DIR/path and takes other path relocation
+ variables such as $GIT_OBJECT_DIRECTORY,
+ $GIT_INDEX_FILE... into account.
Would it help to add a quick illustration here?
For example, if GIT_OBJECT_DIRECTORY is /foo/bar,
then git rev-parse --git-path objects/abc returns /foo/bar/abc.
--show-cdup::
When the command is invoked from a subdirectory, show the
path of the top-level directory relative to the current
diff --git a/path.c b/path.c
index ccd7228..e020530 100644
--- a/path.c
+++ b/path.c
@@ -60,13 +60,59 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
return cleanup_path(buf);
}
static void do_git_path(struct strbuf *buf, const char *fmt, va_list args)
{
const char *git_dir = get_git_dir();
+ int gitdir_len;
strbuf_addstr(buf, git_dir);
Maybe simplify by dropping git_dir and invoking strbuf_addstr(buf,
get_git_dir())?
if (buf-len !is_dir_sep(buf-buf[buf-len - 1]))
strbuf_addch(buf, '/');
+ gitdir_len = buf-len;
strbuf_vaddf(buf, fmt, args);
+ adjust_git_path(buf, gitdir_len);
strbuf_cleanup_path(buf);
}
--
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 v4 10/27] Add new environment variable $GIT_COMMON_DIR
On Sat, Mar 1, 2014 at 7:12 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: This variable is intended to support multiple working directories attached to a repository. Such a repository may have a main working directory, created by either git init or git clone and one or more linked working directories. These working directories and the main repository share the same repository directory. --- diff --git a/Documentation/gitrepository-layout.txt b/Documentation/gitrepository-layout.txt index aa03882..10672a1 100644 --- a/Documentation/gitrepository-layout.txt +++ b/Documentation/gitrepository-layout.txt @@ -46,6 +46,9 @@ of incomplete object store is not suitable to be published for use with dumb transports but otherwise is OK as long as `objects/info/alternates` points at the object stores it borrows from. ++ +This directory is ignored $GIT_COMMON_DIR is set and s/ignored \$/ignored if $/g Note the /g since this error is repeated throughout the rest of the gitrepository-layout.txt patch. +$GIT_COMMON_DIR/objects will be used instead. objects/[0-9a-f][0-9a-f]:: A newly created object is stored in its own file. -- 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] finish_tmp_packfile():use strbuf for pathname construction
On Sat, Mar 1, 2014 at 9:29 PM, Sun He sunheeh...@gmail.com wrote:
Signed-off-by: Sun He sunheeh...@gmail.com
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Helped-by: Michael Haggerty mhag...@alum.mit.edu
---
This patch has assumed that you have already fix the bug of
tmpname in builtin/pack-objects.c:write_pack_file() warning()
builtin/pack-objects.c | 15 ++-
bulk-checkin.c | 8 +---
pack-write.c | 18 ++
pack.h | 2 +-
4 files changed, 22 insertions(+), 21 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..099d6ed 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -803,7 +803,7 @@ static void write_pack_file(void)
if (!pack_to_stdout) {
struct stat st;
- char tmpname[PATH_MAX];
+ struct strbuf tmpname = STRBUF_INIT;
/*
* Packs are runtime accessed in their mtime
@@ -826,23 +826,19 @@ static void write_pack_file(void)
tmpname, strerror(errno));
}
- /* Enough space for -sha-1.pack? */
- if (sizeof(tmpname) = strlen(base_name) + 50)
- die(pack base name '%s' too long,
base_name);
- snprintf(tmpname, sizeof(tmpname), %s-, base_name);
+ strbuf_addf(tmpname, %s-, base_name);
if (write_bitmap_index) {
bitmap_writer_set_checksum(sha1);
bitmap_writer_build_type_index(written_list,
nr_written);
}
- finish_tmp_packfile(tmpname, pack_tmp_name,
+ finish_tmp_packfile(tmpname, pack_tmp_name,
written_list, nr_written,
pack_idx_opts, sha1);
if (write_bitmap_index) {
- char *end_of_name_prefix = strrchr(tmpname,
0);
- sprintf(end_of_name_prefix, %s.bitmap,
sha1_to_hex(sha1));
+ strbuf_addf(tmpname, %s.bitmap
,sha1_to_hex(sha1));
Transpose the space and comma before the third argument.
Other than this, the patch looks reasonable.
stop_progress(progress_state);
@@ -851,10 +847,11 @@ static void write_pack_file(void)
bitmap_writer_select_commits(indexed_commits,
indexed_commits_nr, -1);
bitmap_writer_build(to_pack);
bitmap_writer_finish(written_list, nr_written,
-tmpname,
write_bitmap_options);
+tmpname.buf,
write_bitmap_options);
write_bitmap_index = 0;
}
+ strbuf_release(tmpname);
free(pack_tmp_name);
puts(sha1_to_hex(sha1));
}
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..98e651c 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -4,6 +4,7 @@
#include bulk-checkin.h
#include csum-file.h
#include pack.h
+#include strbuf.h
static int pack_compression_level = Z_DEFAULT_COMPRESSION;
@@ -23,7 +24,7 @@ static struct bulk_checkin_state {
static void finish_bulk_checkin(struct bulk_checkin_state *state)
{
unsigned char sha1[20];
- char packname[PATH_MAX];
+ struct strbuf packname = STRBUF_INIT;
int i;
if (!state-f)
@@ -43,8 +44,8 @@ static void finish_bulk_checkin(struct bulk_checkin_state
*state)
close(fd);
}
- sprintf(packname, %s/pack/pack-, get_object_directory());
- finish_tmp_packfile(packname, state-pack_tmp_name,
+ strbuf_addf(packname, %s/pack/pack-, get_object_directory());
+ finish_tmp_packfile(packname, state-pack_tmp_name,
state-written, state-nr_written,
state-pack_idx_opts, sha1);
for (i = 0; i state-nr_written; i++)
@@ -54,6 +55,7 @@ clear_exit:
free(state-written);
memset(state, 0, sizeof(*state));
+ strbuf_release(packname);
/* Make objects we just wrote available to ourselves */
reprepare_packed_git();
}
diff --git a/pack-write.c b/pack-write.c
index 9b8308b..9ccf804 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)
return sha1fd(fd, *pack_tmp_name);
}
-void finish_tmp_packfile(char *name_buffer,
+void finish_tmp_packfile(struct
Re: [PATCH v3] Place cache.h at the first place to match general rule
On Sun, Mar 2, 2014 at 3:31 AM, Sun He sunheeh...@gmail.com wrote:
The general rule is if cache.h or git-compat-util.h is included,
it is the first #include.
As builtin.h starts with git-compat-util.h, files that start with builtin.h
are not changed.
Minor: Odd one-space indentation on each line of commit message.
Helped-by: Duy Nguyen pclo...@gmail.com
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Sun He sunheeh...@gmail.com
---
PATCH v3 fix the position of information I want to convey to readers,
with the directions of Eric Sunshine.
sigchain.c and test-sigchain.c are started with sigchain.h
I checked sigchain.h, and it didn't import any bug.
But to keep consistant with general rule, we should take this patch.
sigchain.c | 2 +-
test-sigchain.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sigchain.c b/sigchain.c
index 1118b99..faa375d 100644
--- a/sigchain.c
+++ b/sigchain.c
@@ -1,5 +1,5 @@
-#include sigchain.h
#include cache.h
+#include sigchain.h
#define SIGCHAIN_MAX_SIGNALS 32
diff --git a/test-sigchain.c b/test-sigchain.c
index 42db234..e499fce 100644
--- a/test-sigchain.c
+++ b/test-sigchain.c
@@ -1,5 +1,5 @@
-#include sigchain.h
#include cache.h
+#include sigchain.h
#define X(f) \
static void f(int sig) { \
--
1.9.0.138.g2de3478.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] Replace memcpy with hashcpy when dealing hash copy globally
On Sun, Mar 2, 2014 at 4:37 AM, Sun He sunheeh...@gmail.com wrote: Replacing memcpy with hashcpy is more directly and elegant. A better explanation is that the change takes advantage of the abstraction provided by hashcpy() rather than hardcoding knowledge about a particular hash representation. Leave ppc/sha1.c alone, as it is an isolated component. Pull cache.h(actually ../cache.h) in just for one memcpy there is not proper. Strange one-space indentation on all lines of commit message. Other than that, the patch looks reasonable. Helped-by: Michael Haggerty mhag...@alum.mit.edu Helped-by: Duy Nguyen pclo...@gmail.com Signed-off-by: Sun He sunheeh...@gmail.com --- PATCH v2 leave ppc/sha1.c alone. The general rule is if cache.h or git-compat-util.h is included, it is the first #include, and system includes will be always in git-compat-tuil.h. via Duy Nguyen The change in PATCH v1 is not proper because I placed cache.h in the end. And adding it to the head is not a good way to achieve the goal, as is said above ---. Thanks to Duy Nguyen. Find the potential places with memcpy by the bash command: $ find . | xargs grep memcpy.*\(.*20.*\) ppc/sha1.c doesn't include cache.h and it cannot use So just leave memcpy(in ppc/sha1.c) alone bundle.c| 2 +- grep.c | 2 +- pack-bitmap-write.c | 2 +- reflog-walk.c | 4 ++-- refs.c | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/bundle.c b/bundle.c index e99065c..7809fbb 100644 --- a/bundle.c +++ b/bundle.c @@ -19,7 +19,7 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name, list-list = xrealloc(list-list, list-alloc * sizeof(list-list[0])); } - memcpy(list-list[list-nr].sha1, sha1, 20); + hashcpy(list-list[list-nr].sha1, sha1); list-list[list-nr].name = xstrdup(name); list-nr++; } diff --git a/grep.c b/grep.c index c668034..f5101f7 100644 --- a/grep.c +++ b/grep.c @@ -1650,7 +1650,7 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type, break; case GREP_SOURCE_SHA1: gs-identifier = xmalloc(20); - memcpy(gs-identifier, identifier, 20); + hashcpy(gs-identifier, identifier); break; case GREP_SOURCE_BUF: gs-identifier = NULL; diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c index 1218bef..5f1791a 100644 --- a/pack-bitmap-write.c +++ b/pack-bitmap-write.c @@ -530,7 +530,7 @@ void bitmap_writer_finish(struct pack_idx_entry **index, header.version = htons(default_version); header.options = htons(flags | options); header.entry_count = htonl(writer.selected_nr); - memcpy(header.checksum, writer.pack_checksum, 20); + hashcpy(header.checksum, writer.pack_checksum); sha1write(f, header, sizeof(header)); dump_bitmap(f, writer.commits); diff --git a/reflog-walk.c b/reflog-walk.c index b2fbdb2..d490f7d 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -32,8 +32,8 @@ static int read_one_reflog(unsigned char *osha1, unsigned char *nsha1, sizeof(struct reflog_info)); } item = array-items + array-nr; - memcpy(item-osha1, osha1, 20); - memcpy(item-nsha1, nsha1, 20); + hashcpy(item-osha1, osha1); + hashcpy(item-nsha1, nsha1); item-email = xstrdup(email); item-timestamp = timestamp; item-tz = tz; diff --git a/refs.c b/refs.c index 89228e2..f90b7ea 100644 --- a/refs.c +++ b/refs.c @@ -1222,7 +1222,7 @@ static int resolve_gitlink_packed_ref(struct ref_cache *refs, if (ref == NULL) return -1; - memcpy(sha1, ref-u.value.sha1, 20); + hashcpy(sha1, ref-u.value.sha1); return 0; } -- 1.9.0.138.g2de3478.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 -- 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 v4] branch.c: change install_branch_config() to use skip_prefix()
On Mon, Mar 3, 2014 at 1:36 AM, Guanglin Xu mzguang...@gmail.com wrote:
to avoid a magic code of 11.
Helped-by: Sun He sunheeh...@gmail.com
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Helped-by: Jacopo Notarstefano jaco...@gmail.com
Signed-off-by: Guanglin Xu mzguang...@gmail.com
---
This is an implementation of the idea#2 of GSoC 2014 microproject.
branch.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/branch.c b/branch.c
index 723a36b..4eec0ac 100644
--- a/branch.c
+++ b/branch.c
@@ -49,8 +49,12 @@ static int should_setup_rebase(const char *origin)
void install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
{
- const char *shortname = remote + 11;
- int remote_is_branch = starts_with(remote, refs/heads/);
+ const char *shortname = skip_prefix(remote ,refs/heads/);
+ int remote_is_branch;
+ if (NULL == shortname)
+ remote_is_branch = 0;
+ else
+ remote_is_branch = 1;
A bit verbose. Perhaps just:
int remote_is_branch = !!shortname;
which you will find to be idiomatic in this project.
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
--
1.9.0
--
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 00/11] Use ALLOC_GROW() instead of inline code
On Mon, Mar 3, 2014 at 2:18 AM, Dmitry S. Dolzhenko dmitrys.dolzhe...@yandex.ru wrote: Dmitry S. Dolzhenko (11): builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path() bundle.c: use ALLOC_GROW() in add_to_ref_list() cache-tree.c: use ALLOC_GROW() in find_subtree() commit.c: use ALLOC_GROW() in register_commit_graft() diff.c: use ALLOC_GROW() diffcore-rename.c: use ALLOC_GROW() patch-ids.c: use ALLOC_GROW() in add_commit() replace_object.c: use ALLOC_GROW() in register_replace_object() reflog-walk.c: use ALLOC_GROW() dir.c: use ALLOC_GROW() in create_simplify() attr.c: use ALLOC_GROW() in handle_attr_line() attr.c | 7 +-- builtin/pack-objects.c | 9 +++-- bundle.c | 6 +- cache-tree.c | 6 +- commit.c | 8 ++-- diff.c | 12 ++-- diffcore-rename.c | 12 ++-- dir.c | 5 + patch-ids.c| 5 + reflog-walk.c | 12 ++-- replace_object.c | 8 ++-- 11 files changed, 18 insertions(+), 72 deletions(-) -- 1.8.5.3 This version differs from previous only minor changes: - update commit messages - keep code lines within 80 columns Place this commentary at the top of the cover letter since that's where people look for it. You want to ease the reviewer's job as much as possible, so it helps to link to the previous submission, like this [1]. Likewise, you can help the reviewer by being more specific about how you updated the commit messages (and perhaps by linking to the relevant discussion points, like this [2][3]). [1]: http://thread.gmane.org/gmane.comp.version-control.git/242857 [2]: http://article.gmane.org/gmane.comp.version-control.git/243004 [3]: http://article.gmane.org/gmane.comp.version-control.git/243049 -- 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 v4 12/27] *.sh: avoid hardcoding $GIT_DIR/hooks/...
On Sat, Mar 1, 2014 at 7:12 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
If $GIT_COMMON_DIR is set, it should be $GIT_COMMON_DIR/hooks/, not
$GIT_DIR/hooks/. Just let rev-parse --git-path handle it.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
git-am.sh | 22 +++---
git-rebase--interactive.sh | 6 +++---
git-rebase--merge.sh | 6 ++
git-rebase.sh | 4 ++--
templates/hooks--applypatch-msg.sample | 4 ++--
templates/hooks--pre-applypatch.sample | 4 ++--
6 files changed, 22 insertions(+), 24 deletions(-)
diff --git a/git-am.sh b/git-am.sh
index bbea430..dfa0618 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -803,10 +803,10 @@ To restore the original branch and stop patching run
\\$cmdline --abort\.
continue
fi
- if test -x $GIT_DIR/hooks/applypatch-msg
+ hook=`git rev-parse --git-path hooks/applypatch-msg`
Did you want to use $(...) rather than `...`?
Same question for the remainder of the patch.
+ if test -x $hook
then
- $GIT_DIR/hooks/applypatch-msg $dotest/final-commit ||
- stop_here $this
+ $hook $dotest/final-commit || stop_here $this
fi
if test -f $dotest/final-commit
@@ -880,9 +880,10 @@ did you forget to use 'git add'?
stop_here_user_resolve $this
fi
- if test -x $GIT_DIR/hooks/pre-applypatch
+ hook=`git rev-parse --git-path hooks/pre-applypatch`
+ if test -x $hook
then
- $GIT_DIR/hooks/pre-applypatch || stop_here $this
+ $hook || stop_here $this
fi
tree=$(git write-tree)
@@ -908,18 +909,17 @@ did you forget to use 'git add'?
echo $(cat $dotest/original-commit) $commit
$dotest/rewritten
fi
- if test -x $GIT_DIR/hooks/post-applypatch
- then
- $GIT_DIR/hooks/post-applypatch
- fi
+ hook=`git rev-parse --git-path hooks/post-applypatch`
+ test -x $hook $hook
go_next
done
if test -s $dotest/rewritten; then
git notes copy --for-rewrite=rebase $dotest/rewritten
-if test -x $GIT_DIR/hooks/post-rewrite; then
- $GIT_DIR/hooks/post-rewrite rebase $dotest/rewritten
+hook=`git rev-parse --git-path hooks/post-rewrite`
+if test -x $hook; then
+ $hook rebase $dotest/rewritten
fi
fi
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 43c19e0..d741b04 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -632,9 +632,9 @@ do_next () {
git notes copy --for-rewrite=rebase $rewritten_list ||
true # we don't care if this copying failed
}
- if test -x $GIT_DIR/hooks/post-rewrite
- test -s $rewritten_list; then
- $GIT_DIR/hooks/post-rewrite rebase $rewritten_list
+ hook=`git rev-parse --git-path hooks/post-rewrite`
+ if test -x $hook test -s $rewritten_list; then
+ $hook rebase $rewritten_list
true # we don't care if this hook failed
fi
warn Successfully rebased and updated $head_name.
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index e7d96de..68f5d09 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -93,10 +93,8 @@ finish_rb_merge () {
if test -s $state_dir/rewritten
then
git notes copy --for-rewrite=rebase $state_dir/rewritten
- if test -x $GIT_DIR/hooks/post-rewrite
- then
- $GIT_DIR/hooks/post-rewrite rebase
$state_dir/rewritten
- fi
+ hook=`git rev-parse --git-path hooks/post-rewrite`
+ test -x $hook $hook rebase $state_dir/rewritten
fi
say All done.
}
diff --git a/git-rebase.sh b/git-rebase.sh
index 8a3efa2..1cf8dba 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -195,9 +195,9 @@ run_specific_rebase () {
run_pre_rebase_hook () {
if test -z $ok_to_skip_pre_rebase
- test -x $GIT_DIR/hooks/pre-rebase
+ test -x `git rev-parse --git-path hooks/pre-rebase`
then
- $GIT_DIR/hooks/pre-rebase ${1+$@} ||
+ `git rev-parse --git-path hooks/pre-rebase` ${1+$@} ||
die $(gettext The pre-rebase hook refused to rebase.)
fi
}
diff --git a/templates/hooks--applypatch-msg.sample
b/templates/hooks--applypatch-msg.sample
index 8b2a2fe..28b843b 100755
--- a/templates/hooks--applypatch-msg.sample
+++ b/templates/hooks--applypatch-msg.sample
@@ -10,6 +10,6 @@
# To enable this hook, rename this file to applypatch-msg.
. git-sh-setup
-test -x $GIT_DIR/hooks/commit-msg
- exec $GIT_DIR/hooks/commit-msg ${1+$@}
+commitmsg=`git
Re: [PATCH] commit.c: Replace starts_with() with skip_prefix()
Thanks for the submission. Some commentary below to expose you to the
review process on this project...
On Mon, Mar 3, 2014 at 2:47 AM, Karthik Nayak karthik@gmail.com wrote:
Replace with skip_prefix(), which uses the inbuilt function
strcmp() to compare.
Explaining the purpose of the change is indeed important, however,
this description misses the mark. See below.
Other Places were this can be implemented:
commit.c : line 1117
commit.c : line 1197
Bonus points if you actually follow through with this.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
commit.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/commit.c b/commit.c
index 6bf4fe0..1e181cf 100644
--- a/commit.c
+++ b/commit.c
@@ -552,6 +552,7 @@ static void record_author_date(struct author_date_slab
*author_date,
char *buffer = NULL;
struct ident_split ident;
char *date_end;
+ char *flag_sp; /* stores return value of skip_prefix() */
It's not clear why this variable is needed. It's only assigned (below)
but never referenced. Also, the name conveys little or no meaning. If
you need a comment to explain the purpose of the variable, that's
probably a good indication that it's poorly named.
unsigned long date;
if (!commit-buffer) {
@@ -566,7 +567,7 @@ static void record_author_date(struct author_date_slab
*author_date,
buf;
buf = line_end + 1) {
line_end = strchrnul(buf, '\n');
- if (!starts_with(buf, author )) {
+ if ((flag_sp = skip_prefix(buf, author )) == NULL) {
Unfortunately, this change makes the code more difficult to read.
Let's review the GSoC microproject:
Rewrite commit.c:record_author_date() to use skip_prefix().
Are there other places in this file where skip_prefix() would
be **more readable** than starts_with()?
Note the part I **highlighted**. This is a good clue that use of
skip_prefix() can be used to simplify the code. Examine
record_author_date() more carefully and see if you can identify how to
do so.
if (!line_end[0] || line_end[1] == '\n')
return; /* end of header */
continue;
--
1.9.0.138.g2de3478
--
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] implemented strbuf_write_or_die()
On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: On Sat, Mar 1, 2014 at 7:51 AM, He Sun sunheeh...@gmail.com wrote: 2014-03-01 19:21 GMT+08:00 Faiz Kothari faiz.of...@gmail.com: diff --git a/remote-curl.c b/remote-curl.c index 10cb011..dee8716 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads) if (start_command(client)) exit(1); if (preamble) - write_or_die(client.in, preamble-buf, preamble-len); + strbuf_write_or_die(client.in, preamble); if (heads) write_or_die(client.in, heads-buf, heads-len); This should be changed. May be you can use Ctrl-F to search write_or_die(). Or if you are using vim, use / and n to find all. It's not obvious from the patch fragment, but 'heads' is not a strbuf, so Faiz correctly left this invocation alone. That is a very good sign why this change is merely a code-churn and not an improvement, isn't it? We know (and any strbuf user should know) that -buf and -len are the ways to learn the pointer and the length the strbuf holds. Why anybody thinks it is benefitial to introduce another function that is _only_ for writing out strbuf and cannot be used to write out a plain buffer is simply beyond me. As a potential GSoC student and newcomer to the project, Faiz would not have known that this would be considered unwanted churn when he chose the task from the GSoC microproject page [1]. Perhaps it would be a good idea to retire this item from the list? On the other hand, it did expose Faiz to the iterative code review process on this project and gave him a taste of what would be expected of him as a GSoC student, so the microproject achieved that important goal, and thus wasn't an utter failure. [1]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md -- 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] commit.c:record_author_date() use skip_prefix() instead of starts_with()
On Mon, Mar 3, 2014 at 1:40 PM, Junio C Hamano gits...@pobox.com wrote:
Michael Haggerty mhag...@alum.mit.edu writes:
-if (!starts_with(buf, author )) {
+if (!skip_prefix(buf, author )) {
If this is the only change, there is not much point, is there? How does
this help? Perhaps there is some way to take advantage of the
difference between starts_with() and skip_prefix() to simplify the rest
of the function?
I admit I lost track, but wasn't there a discussion to use
starts_with/ends_with when appropriate (namely, the caller is
absolutely not interested in what the remainder of the string is
after skipping the prefix), moving away from skip_prefix()? Isn't
this change going in the wrong direction?
Yes, it would be going in the wrong direction if this was all there
was to it, but the particular GSoC microproject [1] which inspired
this (incomplete) submission expects that the potential student will
dig deeper and discover how skip_prefix() can be used to achieve
greater simplification in record_author_date() and in other places in
the same file.
[1]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md
--
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 3/3] rebase: new convenient option to edit a single commit
On Sat, Mar 1, 2014 at 9:53 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: git rebase -e XYZ is basically the same as EDITOR=sed -i '1s/pick XYZ/edit XYZ/' $@ \ git rebase -i XYZ^ In English, it prepares the todo list for you to edit only commit XYZ to save your time. The time saving is only significant when you edit a lot of commits separately. Is it correct to single out only edit for special treatment? If allowing edit on the command-line, then shouldn't command-line reword also be supported? I, for one, often need to reword a commit message (or two or three); far more frequently than I need to edit a commit. (This is a genuine question about perceived favoritism of edit, as opposed to a request to further bloat the interface.) -- 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] implemented strbuf_write_or_die()
On Mon, Mar 3, 2014 at 3:35 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: It's not obvious from the patch fragment, but 'heads' is not a strbuf, so Faiz correctly left this invocation alone. That is a very good sign why this change is merely a code-churn and not an improvement, isn't it? We know (and any strbuf user should know) that -buf and -len are the ways to learn the pointer and the length the strbuf holds. Why anybody thinks it is benefitial to introduce another function that is _only_ for writing out strbuf and cannot be used to write out a plain buffer is simply beyond me. As a potential GSoC student and newcomer to the project, Faiz would not have known that this would be considered unwanted churn when he chose the task from the GSoC microproject page [1]. Perhaps it would be a good idea to retire this item from the list? I don't think I saw this on the microproject suggestion page when I last looked at it, and assumed that this was on the student's own initiative. I also had not seen it earlier on the microprojects page and had the same reaction until I re-checked the page and found that it had been added [1]. The microprojects page already instructs students to indicate that a submission is for GSoC [2] (and many have followed the advice), but perhaps we can avoid this sort of misunderstanding in the future by making it more explicit: for instance, tell them to add [GSoC] to the Subject:. [1]: https://github.com/git/git.github.io/commit/f314120a2b5e831459673c612a3630ad953d9954 [2]: https://github.com/git/git.github.io/blame/master/SoC-2014-Microprojects.md#L83 On the other hand, it did expose Faiz to the iterative code review process on this project and gave him a taste of what would be expected of him as a GSoC student, so the microproject achieved that important goal, and thus wasn't an utter failure. [1]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md Surely. I would have to say that this is not a good sample exercise to suggest to new students and I'd encourage dropping it from the list. You could argue that it is an effective way to cull people with bad design taste to mix suggestions to make the codebase worse and see who picks them, but I do not think it is very fair ;-) Agreed. The item should be dropped from the list. -- 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: Help needed: Tests failed While replacing char array with strbuf in bulk-checkin.c
On Mon, Mar 3, 2014 at 4:11 PM, saikrishna.sripada saikrishna.srip...@students.iiit.ac.in wrote: I am trying do complete the microproject 4, inorder to apply to GSOC. I have made the below changes: https://gist.github.com/anhsirksai/9334565 Post my changes compilation is succes in the source directory. But when I ran the tests[make in t/ directory] my tests are failing saying free(): invalid pointer: 0x3630376532353636 *** === Backtrace: = /lib/x86_64-linux-gnu/libc.so.6(+0x7eb96)[0x2b5f3b540b96] /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x4fb829] /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x47d425] /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x4064ad] /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x405a04] /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x404cbd] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xed)[0x2b5f3b4e376d] /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x405109] Can some one please help me with the memory allacation and strbuf_release() Read the microproject text carefully and _fully_. It provides the clue you need to understand the problem. Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname, and explain why this is useful. Also check if the first argument of pack-write.c:finish_tmp_packfile() can be made const. If, after making a closer examination of the mentioned functions, the problem still eludes you, ask here again. -- 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/2] test-lib: tests skipped by GIT_SKIP_TESTS say so
On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr ilya.bo...@gmail.com wrote:
We used to show (missing ) next to tests skipped because they are
specified in GIT_SKIP_TESTS. Use (matched by GIT_SKIP_TESTS) instead.
Bikeshedding: That's pretty verbose. Perhaps just say (excluded)?
---
t/test-lib.sh | 13 -
1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1531c24..89a405b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -446,25 +446,28 @@ test_finish_ () {
test_skip () {
to_skip=
+ skipped_reason=
if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
then
to_skip=t
+ skipped_reason=matched GIT_SKIP_TESTS
fi
if test -z $to_skip test -n $test_prereq
! test_have_prereq $test_prereq
then
to_skip=t
- fi
- case $to_skip in
- t)
+
of_prereq=
if test $missing_prereq != $test_prereq
then
of_prereq= of $test_prereq
fi
-
+ skipped_reason=missing $missing_prereq${of_prereq}
+ fi
+ case $to_skip in
+ t)
say_color skip 3 skipping test: $@
- say_color skip ok $test_count # skip $1 (missing
$missing_prereq${of_prereq})
+ say_color skip ok $test_count # skip $1 ($skipped_reason)
: true
;;
*)
--
1.7.9
--
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
--
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 2/2] test-lib: GIT_TEST_ONLY to run only specific tests
On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr ilya.bo...@gmail.com wrote:
This is a counterpart to GIT_SKIP_TESTS. Mostly useful when debugging.
To be grammatically similar to GIT_SKIP_TESTS, perhaps name it GIT_RUN_TESTS?
---
t/README | 15 +++
t/test-lib.sh |8
2 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/t/README b/t/README
index caeeb9d..f939987 100644
--- a/t/README
+++ b/t/README
@@ -187,6 +187,21 @@ and either can match the t[0-9]{4} part to skip the
whole
test, or t[0-9]{4} followed by .$number to say which
particular test to skip.
+Sometimes the opposite is desired - ability to execute only one or
+several tests. Mostly while debugging tests. For that you can say
+
+$ GIT_TEST_ONLY=t9200.8 sh ./t9200-git-cvsexport-commit.sh
+
+or, similrary to GIT_SKIP_TESTS
+
+$ GIT_TEST_ONLY='t[0-4]??? t91?? t9200.8' make
+
+In additiona to matching against test suite number.test number
s/additiona/addition/
Plus the other typos already mentioned by Philip...
+GIT_TEST_ONLY is matched against just the test numbes. This comes
+handy when you are running only one test:
+
+$ GIT_TEST_ONLY='[0-8]' sh ./t9200-git-cvsexport-commit.sh
+
Note that some tests in the existing test suite rely on previous
test item, so you cannot arbitrarily disable one and expect the
remainder of test to check what the test originally was intended
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 89a405b..12bf436 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -464,6 +464,14 @@ test_skip () {
fi
skipped_reason=missing $missing_prereq${of_prereq}
fi
+ if test -z $to_skip test -n $GIT_TEST_ONLY
+ ! match_pattern_list $this_test.$test_count $GIT_TEST_ONLY
+ ! match_pattern_list $test_count $GIT_TEST_ONLY
+ then
+ to_skip=t
+ skipped_reason=not in GIT_TEST_ONLY
+ fi
+
case $to_skip in
t)
say_color skip 3 skipping test: $@
--
1.7.9
--
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
--
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] commit.c: Replace starts_with() with skip_prefix()
On Mon, Mar 3, 2014 at 10:23 AM, karthik nayak karthik@gmail.com wrote:
Hello Eric,
Thanks for Pointing out everything, i had a thorough look and fixed a
couple of things.
Here is an Updated Patch.
- Removed unnecessary code and variables.
- Replaced all instances of starts_with() with skip_prefix()
This commentary is important for the on-going email discussion but
does not belong in the commit message for the permanent project
history, so place it below the --- line just under your sign-off.
Explaining what changed since the last attempt, as you do here, is a
good thing. To further ease the review process, supply a link to the
previous attempt, like this [1].
[1]: http://thread.gmane.org/gmane.comp.version-control.git/243194
Replace starts_with() with skip_prefix() for better reading purposes.
Also to replace buf + strlen(author ) by skip_prefix(), which is
saved in a new const char variable buf_skipprefix.
The second sentence is really the meat of this change, and not at all
incidental as Also implies. You should use it (or a refinement of
it) as your commit message. The first sentence doesn't particularly
add much and can easily be dropped.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
commit.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/commit.c b/commit.c
index 6bf4fe0..e5dc2e2 100644
--- a/commit.c
+++ b/commit.c
@@ -552,6 +552,7 @@ static void record_author_date(struct
author_date_slab *author_date,
char *buffer = NULL;
struct ident_split ident;
char *date_end;
+ const char *buf_skipprefix;
Read this response by Junio [2] to another GSoC candidate which
explains why this is a poor variable name and how to craft a better
one.
[2]: http://thread.gmane.org/gmane.comp.version-control.git/243231/focus=243259
unsigned long date;
if (!commit-buffer) {
@@ -562,18 +563,20 @@ static void record_author_date(struct
author_date_slab *author_date,
return;
}
+ buf_skipprefix = skip_prefix(buf, author );
Unfortunately, your patch is badly whitespace-damaged as if it was
pasted into the mailer and mangled. (Your first submission did not
have this problem.) If you can, use git send-email to avoid such
corruption.
+
for (buf = commit-buffer ? commit-buffer : buffer;
buf;
buf = line_end + 1) {
line_end = strchrnul(buf, '\n');
- if (!starts_with(buf, author )) {
+ if (!buf_skipprefix) {
if (!line_end[0] || line_end[1] == '\n')
return; /* end of header */
continue;
}
if (split_ident_line(ident,
- buf + strlen(author ),
- line_end - (buf + strlen(author ))) ||
+ buf_skipprefix,
+ line_end - buf_skipprefix) ||
Looks reasonable (sans whitespace damage).
!ident.date_begin || !ident.date_end)
goto fail_exit; /* malformed author line */
break;
@@ -1113,7 +1116,7 @@ int parse_signed_commit(const unsigned char *sha1,
next = next ? next + 1 : tail;
if (in_signature line[0] == ' ')
sig = line + 1;
- else if (starts_with(line, gpg_sig_header)
+ else if (skip_prefix(line, gpg_sig_header)
line[gpg_sig_header_len] == ' ')
sig = line + gpg_sig_header_len + 1;
if (sig) {
@@ -1193,7 +1196,7 @@ static void parse_gpg_output(struct signature_check
*sigc)
for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
const char *found, *next;
- if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
+ if (skip_prefix(buf, sigcheck_gpg_status[i].check + 1)) {
/* At the very beginning of the buffer */
found = buf + strlen(sigcheck_gpg_status[i].check + 1);
} else {
For these two sets of changes, unless you are actually taking
advantage of the return value of skip_prefix(), the mere mechanical
replacement of starts_with() with skip_prefix() actually hurts
readability. Examine each of these cases more carefully to determine
whether skip_prefix() is in fact useful. If so, take advantage of it.
If not, explain in the patch commentary why skip_prefix() doesn't
help.
--
1.9.0.138.g2de3478
--
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 v4 19/27] wrapper.c: wrapper to open a file, fprintf then close
On Sat, Mar 1, 2014 at 12:11 PM, Torsten Bögershausen tbo...@web.de wrote:
On 2014-03-01 13.12, Nguyễn Thái Ngọc Duy wrote:
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
cache.h | 2 ++
wrapper.c | 31 +++
2 files changed, 33 insertions(+)
diff --git a/cache.h b/cache.h
index 98b5dd3..99b86d9 100644
--- a/cache.h
+++ b/cache.h
@@ -1239,6 +1239,8 @@ static inline ssize_t write_str_in_full(int fd, const
char *str)
{
return write_in_full(fd, str, strlen(str));
}
+__attribute__((format (printf,3,4)))
+extern int write_file(const char *path, int fatal, const char *fmt, ...);
/* pager.c */
extern void setup_pager(void);
diff --git a/wrapper.c b/wrapper.c
index 0cc5636..5ced50d 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -455,3 +455,34 @@ struct passwd *xgetpwuid_self(void)
errno ? strerror(errno) : _(no such user));
return pw;
}
+
+int write_file(const char *path, int fatal, const char *fmt, ...)
+{
+ struct strbuf sb = STRBUF_INIT;
+ int fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);
+ va_list params;
+ if (fd 0) {
Micro nit atop Torsten's micro nit:
It is 3% easier to understand the code if the check for open() failure
immediately follows the open() attempt:
va_list params;
int fd = open(...);
if (fd 0) {
+ if (fatal)
+ die_errno(_(could not open %s for writing), path);
+ return -1;
+ }
+ va_start(params, fmt);
+ strbuf_vaddf(sb, fmt, params);
+ va_end(params);
+ if (write_in_full(fd, sb.buf, sb.len) != sb.len) {
+ int err = errno;
+ close(fd);
+ errno = err;
+ strbuf_release(sb);
Micro nit:
Today we now what strbuf_release() is doing, but if we ever change the
implementation, it is 3% safer to keep err a little bit longer like this:
+ strbuf_release(sb);
+ errno = err;
--
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] [PATCH] commit.c: Replace starts_with() with skip_prefix()
Thanks for the resend. Etiquette on this list is to cc: people who
commented on previous versions of the submission. As Tanay already
mentioned, use [PATCH vN] in the subject where N is the version number
of this attempt. The -v option of git format-email can help.
More below.
On Tue, Mar 4, 2014 at 10:54 AM, Karthik Nayak karthik@gmail.com wrote:
In record_author_date() :
Replace buf + strlen(author ) by skip_prefix(), which is
saved in a new const char variable indent_line.
In parse_signed_commit() :
Replace line + gpg_sig_header_len by skip_prefix(), which
is saved in a new const char variable indent_line.
In parse_gpg_output() :
Replace buf + strlen(sigcheck_gpg_status[i].check + 1) by
skip_prefix(), which is saved in already available
variable found.
It's not necessary to explain in prose what the patch itself already
states more concisely and precisely. All of this text should be
dropped from the commit message. Instead, explain the purpose of the
patch, the problem it solves, etc. Please read the (2) Describe your
changes well section of Documentation/SubmittingPatches to get an
idea of what sort of information to include in the commit message.
A better commit message should say something about how the affected
functions want to know not only if the string has a prefix, but also
the text following the prefix, and that skip_prefix() can provide both
pieces of information.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
commit.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/commit.c b/commit.c
index 6bf4fe0..71a03e3 100644
--- a/commit.c
+++ b/commit.c
@@ -553,6 +553,7 @@ static void record_author_date(struct author_date_slab
*author_date,
struct ident_split ident;
char *date_end;
unsigned long date;
+ const char *indent_line;
Strange variable name. The code in question splits apart a person's
identification string (name, email, etc.). It has nothing to do with
indentation.
if (!commit-buffer) {
unsigned long size;
@@ -562,18 +563,19 @@ static void record_author_date(struct author_date_slab
*author_date,
return;
}
+ indent_line = skip_prefix(buf, author );
+
for (buf = commit-buffer ? commit-buffer : buffer;
buf;
buf = line_end + 1) {
line_end = strchrnul(buf, '\n');
- if (!starts_with(buf, author )) {
+ if (!indent_line) {
if (!line_end[0] || line_end[1] == '\n')
return; /* end of header */
continue;
}
- if (split_ident_line(ident,
-buf + strlen(author ),
-line_end - (buf + strlen(author ))) ||
+ if (split_ident_line(ident, indent_line,
+line_end -
indent_line) ||
Indent the second line (using tabs plus possible spaces) so that it
lines up with the ident in the line above. Be sure to set your editor
so tabs have width 8.
!ident.date_begin || !ident.date_end)
goto fail_exit; /* malformed author line */
break;
@@ -1098,6 +1100,7 @@ int parse_signed_commit(const unsigned char *sha1,
char *buffer = read_sha1_file(sha1, type, size);
int in_signature, saw_signature = -1;
char *line, *tail;
+ const char *indent_line;
if (!buffer || type != OBJ_COMMIT)
goto cleanup;
@@ -,11 +1114,11 @@ int parse_signed_commit(const unsigned char *sha1,
char *next = memchr(line, '\n', tail - line);
next = next ? next + 1 : tail;
+ indent_line = skip_prefix(line, gpg_sig_header);
Even stranger variable name for a GPG signature (which has nothing at
all to do with indentation).
if (in_signature line[0] == ' ')
sig = line + 1;
- else if (starts_with(line, gpg_sig_header)
-line[gpg_sig_header_len] == ' ')
- sig = line + gpg_sig_header_len + 1;
+ else if (indent_line indent_line[1] == ' ')
+ sig = indent_line + 2;
Why is this adding 2 rather than 1?
if (sig) {
strbuf_add(signature, sig, next - sig);
saw_signature = 1;
@@ -1193,10 +1196,8 @@ static void parse_gpg_output(struct signature_check
*sigc)
for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
const char *found, *next;
- if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
- /* At the very beginning of the buffer */
- found = buf +
Re: [PATCH] [PATCH] commit.c: Replace starts_with() with skip_prefix()
On Tue, Mar 4, 2014 at 12:58 PM, karthik nayak karthik@gmail.com wrote:
Hey Tanay,
1. Yes just getting used to git send-email now, should follow that from now
2. I thought it shouldn't be a part of the commit, so i put it after
the last ---
3. I did have a thought on your lines also , but concluded to it being
more advantageous, you might be right though
Minor etiquette note: On this list, respond inline rather than top-posting.
To understand why, look at how much scrolling up and down a person has
to do to relate your points 1, 2, and 3 to review statements made by
Tanay at the very bottom of the email.
Nice to hear from you.
Thanks
-Karthik Nayak
On Tue, Mar 4, 2014 at 11:01 PM, Tanay Abhra tanay...@gmail.com wrote:
Karthik Nayak karthik.188 at gmail.com writes:
In record_author_date() :
Replace buf + strlen(author ) by skip_prefix(), which is
saved in a new const char variable indent_line.
In parse_signed_commit() :
Replace line + gpg_sig_header_len by skip_prefix(), which
is saved in a new const char variable indent_line.
In parse_gpg_output() :
Replace buf + strlen(sigcheck_gpg_status[i].check + 1) by
skip_prefix(), which is saved in already available
variable found.
Signed-off-by: Karthik Nayak karthik.188 at gmail.com
---
commit.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/commit.c b/commit.c
index 6bf4fe0..71a03e3 100644
--- a/commit.c
+++ b/commit.c
at at -1098,6 +1100,7 at at int parse_signed_commit(const
unsigned char *sha1,
char *buffer = read_sha1_file(sha1, type, size);
int in_signature, saw_signature = -1;
char *line, *tail;
+ const char *indent_line;
if (!buffer || type != OBJ_COMMIT)
goto cleanup;
at at -,11 +1114,11 at at int parse_signed_commit(const
unsigned char *sha1,
char *next = memchr(line, '\n', tail - line);
next = next ? next + 1 : tail;
+ indent_line = skip_prefix(line, gpg_sig_header);
if (in_signature line[0] == ' ')
sig = line + 1;
- else if (starts_with(line, gpg_sig_header)
- line[gpg_sig_header_len] == ' ')
- sig = line + gpg_sig_header_len + 1;
+ else if (indent_line indent_line[1] == ' ')
+ sig = indent_line + 2;
if (sig) {
strbuf_add(signature, sig, next - sig);
saw_signature = 1;
Hi,
I was attempting the same microproject so I thought I would share some
points that were told to me earlier .The mail subject should have a
increasing order of submission numbers for each revision(for example here it
should be [PATCH v2]).
Also write the superfluous stuff which you have written in the bottom
after ---(the three dashes after the signed off statement) .
In the parse_signed_commit() function, gpg_sig_header_len and gpg_sig_header
variables are precomputed outside of the function, and also Junio said to
prefer starts_with(), whenever skip_prefix() advantages are not so important
in the context.So the replace may not be so advantageous ,I may be wrong in
this case.
Cheers,
Tanay Abhra.
--
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
--
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
--
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] [PATCH] commit.c: Replace starts_with() with skip_prefix()
On Tue, Mar 4, 2014 at 5:27 PM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Tue, Mar 4, 2014 at 10:54 AM, Karthik Nayak karthik@gmail.com wrote:
diff --git a/commit.c b/commit.c
index 6bf4fe0..71a03e3 100644
--- a/commit.c
+++ b/commit.c
@@ -1193,10 +1196,8 @@ static void parse_gpg_output(struct signature_check
*sigc)
for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
const char *found, *next;
- if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
- /* At the very beginning of the buffer */
- found = buf + strlen(sigcheck_gpg_status[i].check +
1);
- } else {
+ found = skip_prefix(buf, sigcheck_gpg_status[i].check +1);
Add a space after the plus sign.
+ if(!found) {
found = strstr(buf, sigcheck_gpg_status[i].check);
'found' is being computed again here, even though you already computed
it just above via skip_prefix(). This seems pretty wasteful.
Ignore this objection. I must have misread the code as I was composing
the email.
--
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] disable grafts during fetch/push/bundle
On Tue, Mar 4, 2014 at 12:48 PM, Jeff King p...@peff.net wrote:
diff --git a/commit.c b/commit.c
index 6bf4fe0..886dbfe 100644
--- a/commit.c
+++ b/commit.c
@@ -114,6 +114,11 @@ static unsigned long parse_commit_date(const char *buf,
const char *tail)
static struct commit_graft **commit_graft;
static int commit_graft_alloc, commit_graft_nr;
+int commit_grafts_loaded(void)
+{
+ return !!commit_graft_nr;
+}
Did you mean !!commit_graft ?
static int commit_graft_pos(const unsigned char *sha1)
{
int lo, hi;
--
1.8.5.2.500.g8060133
--
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] disable grafts during fetch/push/bundle
On Tue, Mar 4, 2014 at 7:37 PM, Jeff King p...@peff.net wrote:
On Tue, Mar 04, 2014 at 06:36:07PM -0500, Eric Sunshine wrote:
On Tue, Mar 4, 2014 at 12:48 PM, Jeff King p...@peff.net wrote:
diff --git a/commit.c b/commit.c
index 6bf4fe0..886dbfe 100644
--- a/commit.c
+++ b/commit.c
@@ -114,6 +114,11 @@ static unsigned long parse_commit_date(const char
*buf, const char *tail)
static struct commit_graft **commit_graft;
static int commit_graft_alloc, commit_graft_nr;
+int commit_grafts_loaded(void)
+{
+ return !!commit_graft_nr;
+}
Did you mean !!commit_graft ?
Shouldn't they produce the same results?
Yes they should, but the use of !! seemed to imply that you wanted to
apply it to the pointer value. (If you indeed intended to use
commit_graft_nr, then 'return commit_graft_nr', without !!, would have
been sufficient and idiomatic C.)
--
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] disable grafts during fetch/push/bundle
On Tue, Mar 4, 2014 at 8:05 PM, Jeff King p...@peff.net wrote:
On Tue, Mar 04, 2014 at 08:00:44PM -0500, Eric Sunshine wrote:
+int commit_grafts_loaded(void)
+{
+ return !!commit_graft_nr;
+}
Did you mean !!commit_graft ?
Shouldn't they produce the same results?
Yes they should, but the use of !! seemed to imply that you wanted to
apply it to the pointer value. (If you indeed intended to use
commit_graft_nr, then 'return commit_graft_nr', without !!, would have
been sufficient and idiomatic C.)
I just wanted to normalize the return value to a boolean 0/1. Even when
the input is an int, it eliminates surprises when you try to assign the
result to a bitfield or other smaller-width type.
Thanks for the explanation.
--
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 v4 27/27] count-objects: report unused files in $GIT_DIR/repos/...
On Sat, Mar 1, 2014 at 7:13 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
In linked checkouts, borrowed parts like config is taken from
$GIT_COMMON_DIR. $GIT_DIR/config is never used. Report them as
garbage.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/count-objects.c | 37 -
path.c | 4
2 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index a7f70cb..725cd5f 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -78,6 +78,39 @@ static void count_objects(DIR *d, char *path, int len, int
verbose,
}
}
+static void report_linked_checkout_garbage(void)
+{
+ /*
+* must be more or less in sync with * path.c:update_common_dir().
+*
+* logs is let slip because logs/HEAD is in $GIT_DIR but the
+* remaining in $GIT_COMMON_DIR. Probably not worth traversing
+* the entire logs directory for that.
+*
+* The same gc.pid for because it's a temporary file.
+*/
+ const char *list[] = {
+ branches, hooks, info, lost-found, modules,
+ objects, refs, remotes, rr-cache, svn,
+ config, packed-refs, shallow, NULL
+ };
+ struct strbuf sb = STRBUF_INIT;
+ const char **p;
+ int len;
+
+ if (!file_exists(git_path(commondir)))
+ return;
+ strbuf_addf(sb, %s/, get_git_dir());
+ len = sb.len;
+ for (p = list; *p; p++) {
+ strbuf_setlen(sb, len);
+ strbuf_addstr(sb, *p);
+ if (file_exists(sb.buf))
+ report_garbage(unused in linked checkout, sb.buf);
+ }
+ strbuf_release(sb);
+}
+
static char const * const count_objects_usage[] = {
N_(git count-objects [-v] [-H | --human-readable]),
NULL
@@ -102,8 +135,10 @@ int cmd_count_objects(int argc, const char **argv, const
char *prefix)
/* we do not take arguments other than flags for now */
if (argc)
usage_with_options(count_objects_usage, opts);
- if (verbose)
+ if (verbose) {
report_garbage = real_report_garbage;
+ report_linked_checkout_garbage();
+ }
memcpy(path, objdir, len);
if (len objdir[len-1] != '/')
path[len++] = '/';
diff --git a/path.c b/path.c
index 47383ff..2e6035d 100644
--- a/path.c
+++ b/path.c
@@ -92,6 +92,10 @@ static void replace_dir(struct strbuf *buf, int len, const
char *newdir)
static void update_common_dir(struct strbuf *buf, int git_dir_len)
{
+ /*
+* Remember to report_linked_checkout_garbage()
+* builtin/count-objects.c
+*/
I couldn't figure out why this comment was telling me to remember to
report linked checkout garbage until I realized that you omitted the
word update (as in remember to update). It might be clearer to say
something along these lines:
Keep synchronized with related list in
builtin/count-objects.c:report_linked_checkout_garbage().
Is it not possible or just too much of a hassle to maintain this list
in just one place, as in a header which is included by these two
files? The exceptions, such as 'log' and 'gc.pid', could be marked by
a special character in the entry (!gc.pid, for example) or any such
scheme.
const char *common_dir_list[] = {
branches, hooks, info, logs, lost-found, modules,
objects, refs, remotes, repos, rr-cache, svn,
--
1.9.0.40.gaa8c3ea
--
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
