[PATCH] Rewritten fsck.c:fsck_commit() through using starts_with() instead of memcmp()
Hi, I've done a microproject, number 15, "Rewrite fsck.c:fsck_commit() to use
starts_with() and/or skip_prefix()." I used only starts_with().
memcmp() returns 0 when both are equal, therefore when replacing with
starts_with() , I used "!" or deleted where appropriate.
I plan to apply for the GSoC 2014. I expect your feedbacks!
Signed-off-by: Oguzhan Unlu
---
fsck.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fsck.c b/fsck.c
index 64bf279..d43be98 100644
--- a/fsck.c
+++ b/fsck.c
@@ -290,12 +290,12 @@ static int fsck_commit(struct commit *commit, fsck_error
error_func)
int parents = 0;
int err;
- if (memcmp(buffer, "tree ", 5))
+ if (!starts_with(buffer, "tree "))
return error_func(&commit->object, FSCK_ERROR, "invalid format
- expected 'tree' line");
if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\n')
return error_func(&commit->object, FSCK_ERROR, "invalid 'tree'
line format - bad sha1");
buffer += 46;
- while (!memcmp(buffer, "parent ", 7)) {
+ while (starts_with(buffer, "parent ")) {
if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
return error_func(&commit->object, FSCK_ERROR, "invalid
'parent' line format - bad sha1");
buffer += 48;
@@ -322,13 +322,13 @@ static int fsck_commit(struct commit *commit, fsck_error
error_func)
if (p || parents)
return error_func(&commit->object, FSCK_ERROR, "parent
objects missing");
}
- if (memcmp(buffer, "author ", 7))
+ if (!starts_with(buffer, "author "))
return error_func(&commit->object, FSCK_ERROR, "invalid format
- expected 'author' line");
buffer += 7;
err = fsck_ident(&buffer, &commit->object, error_func);
if (err)
return err;
- if (memcmp(buffer, "committer ", strlen("committer ")))
+ if (!starts_with(buffer, "committer "))
return error_func(&commit->object, FSCK_ERROR, "invalid format
- expected 'committer' line");
buffer += strlen("committer ");
err = fsck_ident(&buffer, &commit->object, error_func);
--
1.9.1.286.g5172cb3
--
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 v3 2/2] log: add --show-linear-break to help see non-linear history
Option explanation is in rev-list-options.txt. The interaction with -z
is left undecided.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
* Revert back to the old option name --show-linear-break
* Get rid of saved_linear, use another flag in struct object instead
* Fix not showing the break bar after a root commit if the dag graph
has multiple roots
* Make it work with --graph (although I don't really see the point of
using both at the same time)
* Let the next contributor deal with -z
Documentation/rev-list-options.txt | 7 ++
log-tree.c | 9
object.h | 2 +-
revision.c | 45 +++---
revision.h | 9 +++-
5 files changed, 67 insertions(+), 5 deletions(-)
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index 9a3da36..b813961 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -758,6 +758,13 @@ This enables parent rewriting, see 'History
Simplification' below.
This implies the `--topo-order` option by default, but the
`--date-order` option may also be specified.
+--show-linear-break[=]::
+ When --graph is not used, all history branches are flattened
+ which can make it hard to see that the two consecutive commits
+ do not belong to a linear branch. This option puts a barrier
+ in between them in that case. If `` is specified, it
+ is the string that will be shown instead of the default one.
+
ifdef::git-rev-list[]
--count::
Print a number stating how many commits would have been
diff --git a/log-tree.c b/log-tree.c
index 5ce217d..c51a878 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -805,12 +805,21 @@ int log_tree_commit(struct rev_info *opt, struct commit
*commit)
if (opt->line_level_traverse)
return line_log_print(opt, commit);
+ if (opt->track_linear && !opt->linear && !opt->reverse_output_stage) {
+ if (opt->graph)
+ graph_show_padding(opt->graph);
+ else
+ puts("\n");
+ printf("%s\n", opt->break_bar);
+ }
shown = log_tree_diff(opt, commit, &log);
if (!shown && opt->loginfo && opt->always_show_header) {
log.parent = NULL;
show_log(opt);
shown = 1;
}
+ if (opt->track_linear && !opt->linear && opt->reverse_output_stage)
+ printf("\n%s\n", opt->break_bar);
opt->loginfo = NULL;
maybe_flush_or_die(stdout, "stdout");
return shown;
diff --git a/object.h b/object.h
index 768490b..9ee1959 100644
--- a/object.h
+++ b/object.h
@@ -28,7 +28,7 @@ struct object_array {
#define TYPE_BITS 3
/*
* object flag allocation:
- * revision.h: 0--10
+ * revision.h: 0--10 26
* fetch-pack.c:0---4
* walker.c:0-2
* upload-pack.c: 11-19
diff --git a/revision.c b/revision.c
index 78b5c3a..b9afdce 100644
--- a/revision.c
+++ b/revision.c
@@ -1831,6 +1831,18 @@ static int handle_revision_opt(struct rev_info *revs,
int argc, const char **arg
revs->notes_opt.use_default_notes = 1;
} else if (!strcmp(arg, "--show-signature")) {
revs->show_signature = 1;
+ } else if (!strcmp(arg, "--show-linear-break") ||
+ starts_with(arg, "--show-linear-break=")) {
+ if (starts_with(arg, "--show-linear-break="))
+ revs->break_bar = xstrdup(arg + 20);
+ else
+ revs->break_bar = "..";
+ revs->track_linear = 1;
+ /*
+* make track_linear() not a break bar before the
+* first shown commit.
+*/
+ commit_list_insert(NULL, &revs->previous_parents);
} else if (starts_with(arg, "--show-notes=") ||
starts_with(arg, "--notes=")) {
struct strbuf buf = STRBUF_INIT;
@@ -2896,6 +2908,22 @@ enum commit_action simplify_commit(struct rev_info
*revs, struct commit *commit)
return action;
}
+static void track_linear(struct rev_info *revs, struct commit *commit)
+{
+ struct commit_list *p;
+ for (p = revs->previous_parents; p; p = p->next)
+ if (p->item == NULL || /* first commit */
+ !hashcmp(p->item->object.sha1, commit->object.sha1))
+ break;
+ revs->linear = p != NULL;
+ if (revs->reverse) {
+ if (revs->linear)
+ commit->object.flags |= TRACK_LINEAR;
+ }
+ free_commit_list(revs->previous_parents);
+ revs->previous_parents = copy_commit_list(commit->parents);
+}
+
static struct commit *get_revision_1(struct rev_info *revs)
{
if (!revs->commits)
[PATCH v3 1/2] object.h: centralize object flag allocation
While the field "flags" is mainly used by the revision walker, it is
also used in many other places. Centralize the whole flag allocation to
one place for a better overview (and easier to move flags if we have
too).
Signed-off-by: Nguyễn Thái Ngọc Duy
---
The next step could be define (instead of document) all of them in
a single place. But that may result in naming conflicts and stuff
for little gain.
bisect.c| 3 +--
builtin/blame.c | 2 +-
bundle.c| 1 +
commit.c| 2 +-
fetch-pack.c| 1 +
http-push.c | 3 +--
object.h| 13 +
revision.h | 1 +
sha1_name.c | 2 ++
upload-pack.c | 2 +-
walker.c| 1 +
11 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/bisect.c b/bisect.c
index 8448d27..d6e851d 100644
--- a/bisect.c
+++ b/bisect.c
@@ -21,8 +21,7 @@ static const char *argv_checkout[] = {"checkout", "-q", NULL,
"--", NULL};
static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
static const char *argv_update_ref[] = {"update-ref", "--no-deref",
"BISECT_HEAD", NULL, NULL};
-/* bits #0-15 in revision.h */
-
+/* Remember to update object flag allocation in object.h */
#define COUNTED(1u<<16)
/*
diff --git a/builtin/blame.c b/builtin/blame.c
index e5b5d71..88cb799 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -74,7 +74,7 @@ static unsigned blame_copy_score;
#define BLAME_DEFAULT_MOVE_SCORE 20
#define BLAME_DEFAULT_COPY_SCORE 40
-/* bits #0..7 in revision.h, #8..11 used for merge_bases() in commit.c */
+/* Remember to update object flag allocation in object.h */
#define METAINFO_SHOWN (1u<<12)
#define MORE_THAN_ONE_PATH (1u<<13)
diff --git a/bundle.c b/bundle.c
index a85e0e4..1222952 100644
--- a/bundle.c
+++ b/bundle.c
@@ -120,6 +120,7 @@ static int list_refs(struct ref_list *r, int argc, const
char **argv)
return 0;
}
+/* Remember to update object flag allocation in object.h */
#define PREREQ_MARK (1u<<16)
int verify_bundle(struct bundle_header *header, int verbose)
diff --git a/commit.c b/commit.c
index 0f28902..f479331 100644
--- a/commit.c
+++ b/commit.c
@@ -721,7 +721,7 @@ void sort_in_topological_order(struct commit_list **list,
enum rev_sort_order so
/* merge-base stuff */
-/* bits #0..15 in revision.h */
+/* Remember to update object flag allocation in object.h */
#define PARENT1(1u<<16)
#define PARENT2(1u<<17)
#define STALE (1u<<18)
diff --git a/fetch-pack.c b/fetch-pack.c
index f061f1f..fd470e7 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -26,6 +26,7 @@ static int agent_supported;
static struct lock_file shallow_lock;
static const char *alternate_shallow_file;
+/* Remember to update object flag allocation in object.h */
#define COMPLETE (1U << 0)
#define COMMON (1U << 1)
#define COMMON_REF (1U << 2)
diff --git a/http-push.c b/http-push.c
index d4b40c9..f2c56c8 100644
--- a/http-push.c
+++ b/http-push.c
@@ -64,8 +64,7 @@ enum XML_Status {
#define LOCK_TIME 600
#define LOCK_REFRESH 30
-/* bits #0-15 in revision.h */
-
+/* Remember to update object flag allocation in object.h */
#define LOCAL(1u<<16)
#define REMOTE (1u<<17)
#define FETCHING (1u<<18)
diff --git a/object.h b/object.h
index 732bf4d..768490b 100644
--- a/object.h
+++ b/object.h
@@ -26,6 +26,19 @@ struct object_array {
#define OBJECT_ARRAY_INIT { 0, 0, NULL }
#define TYPE_BITS 3
+/*
+ * object flag allocation:
+ * revision.h: 0--10
+ * fetch-pack.c:0---4
+ * walker.c:0-2
+ * upload-pack.c: 11-19
+ * builtin/blame.c: 12-13
+ * bisect.c: 16
+ * bundle.c: 16
+ * http-push.c: 16-19
+ * commit.c: 16-19
+ * sha1_name.c:20
+ */
#define FLAG_BITS 27
/*
diff --git a/revision.h b/revision.h
index 1eb94c1..0262bbd 100644
--- a/revision.h
+++ b/revision.h
@@ -7,6 +7,7 @@
#include "commit.h"
#include "diff.h"
+/* Remember to update object flag allocation in object.h */
#define SEEN (1u<<0)
#define UNINTERESTING (1u<<1)
#define TREESAME (1u<<2)
diff --git a/sha1_name.c b/sha1_name.c
index 6fca869..2b6322f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -819,6 +819,8 @@ static int get_sha1_1(const char *name, int len, unsigned
char *sha1, unsigned l
* For future extension, ':/!' is reserved. If you want to match a message
* beginning with a '!', you have to repeat the exclamation mark.
*/
+
+/* Remember to update object flag allocation in object.h */
#define ONELINE_SEEN (1u<<20)
static int handle_one_ref(const char *path,
diff --git a/upload-pack.c b/upload-pack.c
index 9314c25..27df12f 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -17,7 +17,7 @@
static const char upload_pack_usage[] = "git upload-pack [--strict]
[--timeout=] ";
-/* bits #0..7 i
Re: git 1.9.1 tarball
On Wed, Mar 19, 2014 at 8:09 PM, 乙酸鋰 wrote: > > Hi, > > Where to find git 1.9.1 tarball? > It is not uploaded to google code. > -- You can download a tarball for 1.9.1 from GitHub: https://github.com/git/git/archive/v1.9.1.tar.gz Jason -- 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
[GSoC] Choosing a Project Proposal
Hi all, I'm Currently trying to decide on a project to work on in for Google Summer of Code, I'm stuck choosing between three which I find really interesting and I was wondering if any of them are particularly more pressing then the others. I would also love some comments on each of these three if possible expanding on them. the three projects I'm considering are, 1. Unifying git branch -l, git tag -l, and git for-each-ref 2. Refactor tempfile handling 3. Improve triangular workflow support Once again, I would appreciate all feedback on which of these are most important. Thanks for the Help, Brian Bourn -- 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] builtin/apply.c: fuzzy_matchlines:trying to fix some inefficiencies
Hi fellows,
I'm planning on applying on GSOC 2014...
I tried my luck with that kinda weird microproject about inefficiencies,
and I think I've discovered some.
(also on a totally different mood, there are some warning about empty format
strings during compilation that could easily be silenced with some #pragma
calls on "-Wformat-zero-length". Is there a way you're not adding this?)
The empty buffers check could happen at the beggining.
Leading whitespace check was unnecessary.
Some style changes
Thanks.
---
builtin/apply.c | 25 +
1 file changed, 9 insertions(+), 16 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index b0d0986..df2435f 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -294,20 +294,16 @@ static int fuzzy_matchlines(const char *s1, size_t n1,
const char *last2 = s2 + n2 - 1;
int result = 0;
+ /* early return if both lines are empty */
+ if ((s1 > last1) && (s2 > last2))
+ return 1;
+
/* ignore line endings */
while ((*last1 == '\r') || (*last1 == '\n'))
last1--;
while ((*last2 == '\r') || (*last2 == '\n'))
last2--;
- /* skip leading whitespace */
- while (isspace(*s1) && (s1 <= last1))
- s1++;
- while (isspace(*s2) && (s2 <= last2))
- s2++;
- /* early return if both lines are empty */
- if ((s1 > last1) && (s2 > last2))
- return 1;
while (!result) {
result = *s1++ - *s2++;
/*
@@ -315,18 +311,15 @@ static int fuzzy_matchlines(const char *s1, size_t n1,
* both buffers because we don't want "a b" to match
* "ab"
*/
- if (isspace(*s1) && isspace(*s2)) {
- while (isspace(*s1) && s1 <= last1)
- s1++;
- while (isspace(*s2) && s2 <= last2)
- s2++;
- }
+ while (isspace(*s1) && s1 <= last1)
+ s1++;
+ while (isspace(*s2) && s2 <= last2)
+ s2++;
/*
* If we reached the end on one side only,
* lines don't match
*/
- if (
- ((s2 > last2) && (s1 <= last1)) ||
+ if (((s2 > last2) && (s1 <= last1)) ||
((s1 > last1) && (s2 <= last2)))
return 0;
if ((s1 > last1) && (s2 > last2))
--
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 v2] Bump core.deltaBaseCacheLimit to 128MiB
On Thu, Mar 20, 2014 at 5:11 AM, Junio C Hamano wrote: > David Kastrup writes: > >> Junio C Hamano writes: >> >>> David Kastrup writes: >>> The default of 16MiB causes serious thrashing for large delta chains combined with large files. Signed-off-by: David Kastrup --- >>> >>> Is that a good argument? Wouldn't the default of 128MiB burden >>> smaller machines with bloated processes? >> >> The default file size before Git forgets about delta compression is >> 512MiB. Unpacking 500MiB files with 16MiB of delta storage is going to >> be uglier. >> >> ... >> >> Documentation/config.txt states: >> >> core.deltaBaseCacheLimit:: >> Maximum number of bytes to reserve for caching base objects >> that may be referenced by multiple deltified objects. By >> storing the >> entire decompressed base objects in a cache Git is able >> to avoid unpacking and decompressing frequently used base >> objects multiple times. >> + >> Default is 16 MiB on all platforms. This should be reasonable >> for all users/operating systems, except on the largest projects. >> You probably do not need to adjust this value. >> >> I've seen this seriously screwing performance in several projects of >> mine that don't really count as "largest projects". >> >> So the description in combination with the current setting is clearly wrong. > > That is a good material for proposed log message, and I think you > are onto something here. > > I know that the 512MiB default for the bitFileThreashold (aka > "forget about delta compression") came out of thin air. It was just > "1GB is always too huge for anybody, so let's cut it in half and > declare that value the initial version of a sane threashold", > nothing more. > > So it might be that the problem is 512MiB is still too big, relative > to the 16MiB of delta base cache, and the former may be what needs > to be tweaked. If a blob close to but below 512MiB is a problem for > 16MiB delta base cache, it would still be too big to cause the same > problem for 128MiB delta base cache---it would evict all the other > objects and then end up not being able to fit in the limit itself, > busting the limit immediately, no? > > I would understand if the change were to update the definition of > deltaBaseCacheLimit and link it to the value of bigFileThreashold, > for example. With the presented discussion, I am still not sure if > we can say that bumping deltaBaseCacheLimit is the right solution to > the "description with the current setting is clearly wrong" (which > is a real issue). I vote make big_file_threshold smaller. 512MB is already unfriendly for many smaller machines. I'm thinking somewhere around 32MB-64MB (and maybe increase delta cache base limit to match). The only downside I see is large blobs will be packed undeltified, which could increase pack size if you have lots of them. But maybe we could improve pack-objects/repack/gc to deltify large blobs anyway if they're old enough. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] gc --aggressive: three phase repacking
On Wed, Mar 19, 2014 at 9:14 PM, Stefan Beller wrote: > Maybe we should introduce another option --pack-for-archive > which features the characteristics of the old --aggressive. > And the new --aggressive would be a tradeoff between space and > time? > Thinking further we could add a couple of options there like > --developer-friendly which makes blaming fast > --hosting-friendly which makes it most efficient when upstream > --archival-friendly (the old --aggressive) > --... --aggressive-mode= would be a good option to select those friendly modes instead of a bunch of new options. Well, maby "gc --mode" is enough, we already have two modes: normal and aggressive (or although maybe it should be renamed archive). -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Enable index-pack threading in msysgit.
On Thu, Mar 20, 2014 at 4:35 AM, Stefan Zager wrote: > This adds a Windows implementation of pread. Note that it is NOT > safe to intersperse calls to read() and pread() on a file > descriptor. According to the ReadFile spec, using the 'overlapped' > argument should not affect the implicit position pointer of the > descriptor. Experiments have shown that this is, in fact, a lie. > > To accomodate that fact, this change also incorporates: > > http://article.gmane.org/gmane.comp.version-control.git/196042 > > ... which gives each index-pack thread its own file descriptor. If the problem is mixing read() and pread() then perhaps it's enough to do output_fd = dup(output_fd); after pack_fd is set in open_pack_file(), to make sure that fixup_pack_header_footer() has its own file handle. If that works, we don't need one pack_fd per thread. compat/mmap.c uses pread() and its bad interaction with read() could turn it into a nightmare. Fortunately Windows (except Cygwin) does not use this implementation. Not sure if we should make a note about this. It makes me wonder if sliding mmap window (like we do for pack access in sha1_file.c) would be better than pread(). index-pack used to do mmap() [1] in the past with poor performance but I don't think sliding window was mentioned. [1] http://thread.gmane.org/gmane.comp.version-control.git/34741/focus=34832 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -474,7 +474,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL) > endif > ifneq (,$(findstring MINGW,$(uname_S))) > pathsep = ; > - NO_PREAD = YesPlease > NEEDS_CRYPTO_WITH_SSL = YesPlease > NO_LIBGEN_H = YesPlease > NO_POLL = YesPlease What about the "ifeq ($(uname_S),Windows)" section? I think MSVC and MinGW builds share a lot of code. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib.sh: use printf instead of echo
Junio C Hamano wrote: > Jonathan Nieder writes: >> We currently use "echo" all over the place (e.g., 'echo "$path"' in >> git-sh-setup), and every time we fix it there is a chance of making >> mistakes. I wonder if it would make sense to add a helper to make the >> echo calls easier to replace: > > I agree that we would benefit from having a helper to print a single > line, which we very often do, without having to worry about the > boilerplate '%s\n' of printf or the portability gotcha of echo. > > I am a bit reluctant to name the helper "sane_echo" to declare "echo > that interprets backslashes in the string is insane", though. For > these "print a single line" uses, we are only interested in using a > subset of the features offered by 'echo', but that does not mean the > other features we do not want to trigger in our use is of no use to > any sane person. In a portable script, uncareful use of 'echo' is always insane. In a script tailored to an environment where echo behaves consistently it is perfectly reasonable to use 'echo', but that's a different story. In the context of git, saying "Here is the thing you should always use instead of echo" is a good thing, in my opinion. Hoping that clarifies, Jonathan -- 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] diff: optimise parse_dirstat_params() to only compare strings when necessary
I will send another version of this patch after review because there is an extra whitespace following the else statement. -- 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
git 1.9.1 tarball
Hi, Where to find git 1.9.1 tarball? It is not uploaded to google code. -- 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] diff: optimise parse_dirstat_params() to only compare strings when necessary
parse_dirstat_params() goes through a chain of if statements using
strcmp to parse parameters. When the parameter is a digit, the
value must go through all comparisons before the function realises
it is a digit. Optimise this logic by only going through the chain
of string compares when the parameter is not a digit.
Signed-off-by: Dragos Foianu
---
diff.c | 37 +++--
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/diff.c b/diff.c
index e343191..733764e 100644
--- a/diff.c
+++ b/diff.c
@@ -84,20 +84,25 @@ static int parse_dirstat_params(struct diff_options
*options, const char *params
string_list_split_in_place(¶ms, params_copy, ',', -1);
for (i = 0; i < params.nr; i++) {
const char *p = params.items[i].string;
- if (!strcmp(p, "changes")) {
- DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
- DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
- } else if (!strcmp(p, "lines")) {
- DIFF_OPT_SET(options, DIRSTAT_BY_LINE);
- DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
- } else if (!strcmp(p, "files")) {
- DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
- DIFF_OPT_SET(options, DIRSTAT_BY_FILE);
- } else if (!strcmp(p, "noncumulative")) {
- DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
- } else if (!strcmp(p, "cumulative")) {
- DIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);
- } else if (isdigit(*p)) {
+ if (!isdigit(*p)) {
+ if (!strcmp(p, "changes")) {
+ DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
+ DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
+ } else if (!strcmp(p, "lines")) {
+ DIFF_OPT_SET(options, DIRSTAT_BY_LINE);
+ DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
+ } else if (!strcmp(p, "files")) {
+ DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
+ DIFF_OPT_SET(options, DIRSTAT_BY_FILE);
+ } else if (!strcmp(p, "noncumulative")) {
+ DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
+ } else if (!strcmp(p, "cumulative")) {
+ DIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);
+ } else {
+ strbuf_addf(errmsg, _(" Unknown dirstat
parameter '%s'\n"), p);
+ ret++;
+ }
+ } else {
char *end;
int permille = strtoul(p, &end, 10) * 10;
if (*end == '.' && isdigit(*++end)) {
@@ -114,11 +119,7 @@ static int parse_dirstat_params(struct diff_options
*options, const char *params
p);
ret++;
}
- } else {
- strbuf_addf(errmsg, _(" Unknown dirstat parameter
'%s'\n"), p);
- ret++;
}
-
}
string_list_clear(¶ms, 0);
free(params_copy);
--
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: [PATCHv2] branch.c: simplify chain of if statements
Eric Sunshine sunshineco.com> writes: > > Other submissions have computed this value mathematically without need > for conditionals. For instance, we've seen: > > index = (!!origin << 0) + (!!remote_is_branch << 1) + (!!rebasing << 2) > > as, well as the equivalent: > > index = !!origin + !!remote_is_branch * 2 + !!rebasing * 4 > > Although this works, it does place greater cognitive demands on the > reader by requiring more effort to figure out what is going on and how > it relates to table position. The original (ungainly) chain of 'if' > statements in the original code does not suffer this problem. It > likewise is harder to understand than merely indexing into a > multi-dimension table where each variable is a key. I have seen other submissions using this logic, but I didn't think it accomplished the goal of the patch - simplifying the code. Not that my approach does either, but I found it a little easier to understand. Indexing into a table like this is always going to have this problem so this is probably not the right approach to accomplishing the microproject's goals. > It's possible to simplify this logic and have only a single > printf_ln() invocation. Hint: It's safe to pass in more arguments than > there are %s directives in the format string. Indeed. It's a habit of mine to pass the exact number of arguments to printf functions and I can't seem to get away from it. > You can use ARRAY_SIZE() in place of sizeof(...)/sizeof(...). > > Since an out-of-bound index would be a programmer bug, it would > probably be more appropriate to use an assert(), just after 'index' is > computed, rather than if+die(). The original code used die() because > it couldn't detect the error until the end of the if-chain. Thank you for this hint. Using already defined helpers in the project is better and will prevent the need to patch the constructs later on. > On Tue, Mar 18, 2014 at 6:31 PM, Eric Sunshine sunshineco.com> wrote: > > One other observation: You have a one-off error in your out-of-bounds > check. It should be 'index >= sizeof...' Well this is embarrasing. Thank you again for the feedback. It's incredibily helpful and I learned a lot from submitting these patches. Making the code simple is harder than it appears at first sight. I'm not sure it's worth pursuing the table approach further, especially since a solution has already been accepted and merged into the codebase. In this case, is it okay to try another microproject? I was thinking about trying #17 (finding bugs/inefficiencies in builtin/apply.c), but I've already had my one microproject. All the best, Dragos -- 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 v3][GSOC] fsck: use bitwise-or assignment operator to set flag
fsck_tree() has two different ways to set a flag, which are the followings: 1. Using a if-statement that guards assignment. 2. Using a bitwise-or assignment operator. Currently, many with the former way, and one with the latter way. In this patch, unify them to the latter way, because it makes the code shorter and easier to read, and it is brief and to the point. Signed-off-by: Hiroyuki Sano --- fsck.c | 18 ++ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/fsck.c b/fsck.c index b3022ad..abed62b 100644 --- a/fsck.c +++ b/fsck.c @@ -165,18 +165,12 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func) sha1 = tree_entry_extract(&desc, &name, &mode); - if (is_null_sha1(sha1)) - has_null_sha1 = 1; - if (strchr(name, '/')) - has_full_path = 1; - if (!*name) - has_empty_name = 1; - if (!strcmp(name, ".")) - has_dot = 1; - if (!strcmp(name, "..")) - has_dotdot = 1; - if (!strcmp(name, ".git")) - has_dotgit = 1; + has_null_sha1 |= is_null_sha1(sha1); + has_full_path |= !!strchr(name, '/'); + has_empty_name |= !*name; + has_dot |= !strcmp(name, "."); + has_dotdot |= !strcmp(name, ".."); + has_dotgit |= !strcmp(name, ".git"); has_zero_pad |= *(char *)desc.buffer == '0'; update_tree_entry(&desc); -- 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] disable grafts during fetch/push/bundle
Jeff King writes: > On Fri, Mar 07, 2014 at 08:08:37AM +0100, Christian Couder wrote: > >> > Be it graft or replace, I do not think we want to invite people to >> > use these mechansims too lightly to locally rewrite their history >> > willy-nilly without fixing their mistakes at the object layer with >> > "commit --amend", "rebase", "bfg", etc. in the longer term. So in >> > that sense, adding a command to make it easy is not something I am >> > enthusiastic about. >> > >> > On the other hand, if the user does need to use graft or replace >> > (perhaps to prepare for casting the fixed history in stone with >> > filter-branch), it would be good to help them avoid making mistakes >> > while doing so and tool support may be a way to do so. >> > >> > So, ... I am of two minds. > ... > I do not think the features we are talking about are significantly more > dangerous than "git replace" is in the first place. If we want to make > people aware of the dangers, perhaps git-replace.1 is the right place to > do it. Sure. So should we take the four-patch series for "git replace --edit"? -- 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: [BUG] Segfault on git describe
Sylvestre Ledru mozilla.com> writes: > > Hello, > > Trying to do some stats using the Firefox git repository > (https://github.com/mozilla/gecko-dev), I found a bug > on git describe. The following command will segfault: > git describe --contains a9ff31aebd6dbda82a3c733a72eeeaa0b0525b96 > > Please note that the Firefox history is a pretty long and this commit > date is 2001. > > I experience this issue with the git version, and Debian packages > (1.9.0-1 and 2.0~next.20140214-2) > > As attachment, the backtrace. I removed about 87250 calls to the > name_rev function. I guess that is a potential source of problem. > > Full is available here: > http://people.mozilla.org/~sledru/bt-git-on-ff.txt (21 MB) > > I am available to test patches if needed. > > Thanks, > Sylvestre > PS: I am not registered, please cc me. Hello, The name_rev function recursively calls itself which is why the backtrace is so big. Unfortunately, for repos with long histories it can lead to Stack Overflows. This is pretty much what happened in your case. I tested it on my computer and I get the same results. I managed to get it working by doubling my default stacksize: ulimit -S -s 16192 Considering your project is a very large one and merely allocating a few more resources fixes the problem, I'm not sure it warrants rewriting the function to use less stack. You will have to wait for one of the maintainers to give you a definitive answer. All the best, Dragos -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Make git more user-friendly during a merge conflict
Andrew Wong writes: > On Mon, Mar 17, 2014 at 7:04 PM, Junio C Hamano wrote: >> Has this series been tested with existing test suite? ... > I tested it during RFC, but missed it when I sent it as patch. > ... > I'll fix the problem. Sorry about that. Thanks. Will hold onto the topic branch lest I forget, but will keep it out of 'pu' in the meantime. -- 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] Enable index-pack threading in msysgit.
sza...@chromium.org (Stefan Zager) writes:
> This adds a Windows implementation of pread. Note that it is NOT
> safe to intersperse calls to read() and pread() on a file
> descriptor. According to the ReadFile spec, using the 'overlapped'
> argument should not affect the implicit position pointer of the
> descriptor. Experiments have shown that this is, in fact, a lie.
>
> To accomodate that fact, this change also incorporates:
>
> http://article.gmane.org/gmane.comp.version-control.git/196042
>
> ... which gives each index-pack thread its own file descriptor.
>
> Signed-off-by: Stefan Zager
> ---
I'll queue it on 'pu' until I hear from Windows folks.
There were a few things I tweaked while queuing, tho.
- the indentation of the new comment inside struct thread_local
declaration looked strange;
- there was one new if () statement whose block was opened on the
next line, not on the same line as if () itself.
Thanks.
> builtin/index-pack.c | 30 --
> compat/mingw.c | 37 -
> compat/mingw.h | 3 +++
> config.mak.uname | 1 -
> 4 files changed, 59 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 2f37a38..63b8b0e 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -40,17 +40,17 @@ struct base_data {
> int ofs_first, ofs_last;
> };
>
> -#if !defined(NO_PTHREADS) && defined(NO_THREAD_SAFE_PREAD)
> -/* pread() emulation is not thread-safe. Disable threading. */
> -#define NO_PTHREADS
> -#endif
> -
> struct thread_local {
> #ifndef NO_PTHREADS
> pthread_t thread;
> #endif
> struct base_data *base_cache;
> size_t base_cache_used;
> +/*
> + * To accomodate platforms that have pthreads, but don't have a
> + * thread-safe pread, give each thread its own file descriptor.
> + */
> + int pack_fd;
> };
>
> /*
> @@ -91,7 +91,8 @@ static off_t consumed_bytes;
> static unsigned deepest_delta;
> static git_SHA_CTX input_ctx;
> static uint32_t input_crc32;
> -static int input_fd, output_fd, pack_fd;
> +static const char *curr_pack;
> +static int input_fd, output_fd;
>
> #ifndef NO_PTHREADS
>
> @@ -134,6 +135,7 @@ static inline void unlock_mutex(pthread_mutex_t *mutex)
> */
> static void init_thread(void)
> {
> + int i;
> init_recursive_mutex(&read_mutex);
> pthread_mutex_init(&counter_mutex, NULL);
> pthread_mutex_init(&work_mutex, NULL);
> @@ -141,11 +143,17 @@ static void init_thread(void)
> pthread_mutex_init(&deepest_delta_mutex, NULL);
> pthread_key_create(&key, NULL);
> thread_data = xcalloc(nr_threads, sizeof(*thread_data));
> + for (i = 0; i < nr_threads; i++) {
> + thread_data[i].pack_fd = open(curr_pack, O_RDONLY);
> + if (thread_data[i].pack_fd == -1)
> + die_errno("unable to open %s", curr_pack);
> + }
> threads_active = 1;
> }
>
> static void cleanup_thread(void)
> {
> + int i;
> if (!threads_active)
> return;
> threads_active = 0;
> @@ -155,6 +163,8 @@ static void cleanup_thread(void)
> if (show_stat)
> pthread_mutex_destroy(&deepest_delta_mutex);
> pthread_key_delete(key);
> + for (i = 0; i < nr_threads; i++)
> + close(thread_data[i].pack_fd);
> free(thread_data);
> }
>
> @@ -288,13 +298,13 @@ static const char *open_pack_file(const char *pack_name)
> output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR,
> 0600);
> if (output_fd < 0)
> die_errno(_("unable to create '%s'"), pack_name);
> - pack_fd = output_fd;
> + nothread_data.pack_fd = output_fd;
> } else {
> input_fd = open(pack_name, O_RDONLY);
> if (input_fd < 0)
> die_errno(_("cannot open packfile '%s'"), pack_name);
> output_fd = -1;
> - pack_fd = input_fd;
> + nothread_data.pack_fd = input_fd;
> }
> git_SHA1_Init(&input_ctx);
> return pack_name;
> @@ -542,7 +552,7 @@ static void *unpack_data(struct object_entry *obj,
>
> do {
> ssize_t n = (len < 64*1024) ? len : 64*1024;
> - n = pread(pack_fd, inbuf, n, from);
> + n = pread(get_thread_data()->pack_fd, inbuf, n, from);
> if (n < 0)
> die_errno(_("cannot pread pack file"));
> if (!n)
> @@ -1490,7 +1500,7 @@ static void show_pack_info(int stat_only)
> int cmd_index_pack(int argc, const char **argv, const char *prefix)
> {
> int i, fix_thin_pack = 0, verify = 0, stat_only = 0;
> - const char *curr_pack, *curr_index;
> + const char *curr_index;
> const char *index_name = NULL, *pack_name = NULL;
> const char *keep_name = NULL, *keep_msg = NULL;
> char *index_name_buf = NULL,
Re: [PATCH v2] Bump core.deltaBaseCacheLimit to 128MiB
David Kastrup writes: > Junio C Hamano writes: > >> David Kastrup writes: >> >>> The default of 16MiB causes serious thrashing for large delta chains >>> combined with large files. >>> >>> Signed-off-by: David Kastrup >>> --- >> >> Is that a good argument? Wouldn't the default of 128MiB burden >> smaller machines with bloated processes? > > The default file size before Git forgets about delta compression is > 512MiB. Unpacking 500MiB files with 16MiB of delta storage is going to > be uglier. > > ... > > Documentation/config.txt states: > > core.deltaBaseCacheLimit:: > Maximum number of bytes to reserve for caching base objects > that may be referenced by multiple deltified objects. By storing > the > entire decompressed base objects in a cache Git is able > to avoid unpacking and decompressing frequently used base > objects multiple times. > + > Default is 16 MiB on all platforms. This should be reasonable > for all users/operating systems, except on the largest projects. > You probably do not need to adjust this value. > > I've seen this seriously screwing performance in several projects of > mine that don't really count as "largest projects". > > So the description in combination with the current setting is clearly wrong. That is a good material for proposed log message, and I think you are onto something here. I know that the 512MiB default for the bitFileThreashold (aka "forget about delta compression") came out of thin air. It was just "1GB is always too huge for anybody, so let's cut it in half and declare that value the initial version of a sane threashold", nothing more. So it might be that the problem is 512MiB is still too big, relative to the 16MiB of delta base cache, and the former may be what needs to be tweaked. If a blob close to but below 512MiB is a problem for 16MiB delta base cache, it would still be too big to cause the same problem for 128MiB delta base cache---it would evict all the other objects and then end up not being able to fit in the limit itself, busting the limit immediately, no? I would understand if the change were to update the definition of deltaBaseCacheLimit and link it to the value of bigFileThreashold, for example. With the presented discussion, I am still not sure if we can say that bumping deltaBaseCacheLimit is the right solution to the "description with the current setting is clearly wrong" (which is a real issue). -- 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] remote-hg: do not fail on invalid bookmarks
Max Horn writes:
> Mercurial can have bookmarks pointing to "nullid" (the empty root
> revision), while Git can not have references to it.
> When cloning or fetching from a Mercurial repository that has such a
> bookmark, the import will fail because git-remote-hg will not be able to
> create the corresponding reference.
>
> Warn the user about the invalid reference, and continue the import,
> instead of stopping right away.
The text above is identical to what Antoine wrote in e8d48743
(remote-hg: do not fail on invalid bookmarks, 2013-12-29); I'd
assume that this is to replace it.
But the code seems to do more, and I think that is related to the
detailed analysis you dug up from the archive earlier and then
summarised in your $gmane/20. Can you say a bit more about
these fake-bmark and bmark checking like you did in that original
3-patch series?
Thanks.
> Also add some test cases for this issue.
>
> Reported-by: Antoine Pelisse
> Signed-off-by: Max Horn
> ---
> contrib/remote-helpers/git-remote-hg | 6 +
> contrib/remote-helpers/test-hg.sh| 48
>
> 2 files changed, 54 insertions(+)
>
> diff --git a/contrib/remote-helpers/git-remote-hg
> b/contrib/remote-helpers/git-remote-hg
> index eb89ef6..49b2c2e 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -625,6 +625,12 @@ def list_head(repo, cur):
> def do_list(parser):
> repo = parser.repo
> for bmark, node in bookmarks.listbookmarks(repo).iteritems():
> +if node == '':
> +if fake_bmark == 'default' and bmark == 'master':
> +pass
> +else:
> +warn("Ignoring invalid bookmark '%s'", bmark)
> +continue
> bmarks[bmark] = repo[node]
>
> cur = repo.dirstate.branch()
> diff --git a/contrib/remote-helpers/test-hg.sh
> b/contrib/remote-helpers/test-hg.sh
> index a933b1e..8d01b32 100755
> --- a/contrib/remote-helpers/test-hg.sh
> +++ b/contrib/remote-helpers/test-hg.sh
> @@ -772,4 +772,52 @@ test_expect_success 'remote double failed push' '
> )
> '
>
> +test_expect_success 'clone remote with master null bookmark' '
> + test_when_finished "rm -rf gitrepo* hgrepo*" &&
> +
> + (
> + hg init hgrepo &&
> + cd hgrepo &&
> + echo a >a &&
> + hg add a &&
> + hg commit -m a &&
> + hg bookmark -r null master
> + ) &&
> +
> + git clone "hg::hgrepo" gitrepo &&
> + check gitrepo HEAD a
> +'
> +
> +test_expect_success 'clone remote with default null bookmark' '
> + test_when_finished "rm -rf gitrepo* hgrepo*" &&
> +
> + (
> + hg init hgrepo &&
> + cd hgrepo &&
> + echo a >a &&
> + hg add a &&
> + hg commit -m a &&
> + hg bookmark -r null -f default
> + ) &&
> +
> + git clone "hg::hgrepo" gitrepo &&
> + check gitrepo HEAD a
> +'
> +
> +test_expect_success 'clone remote with generic null bookmark' '
> + test_when_finished "rm -rf gitrepo* hgrepo*" &&
> +
> + (
> + hg init hgrepo &&
> + cd hgrepo &&
> + echo a >a &&
> + hg add a &&
> + hg commit -m a &&
> + hg bookmark -r null bmark
> + ) &&
> +
> + git clone "hg::hgrepo" gitrepo &&
> + check gitrepo HEAD a
> +'
> +
> 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] t5510: Do not use $(pwd) when fetching / pushing / pulling via rsync
Hi Sebastian, On Wed, 19 Mar 2014, Sebastian Schuberth wrote: > On MINGW, "pwd" is defined as "pwd -W" in test-lib.sh. This usually is the > right thing, but the absolute Windows path with a colon confuses rsync. We > could use $PWD in this case to work around the issue, but in fact there is > no need to use an absolute path in the first place, so get rid of it. > > This was discovered in the context of the mingwGitDevEnv project and only > did not surface before with msysgit because the latter does not ship > rsync. ACK Ciao, Dscho -- 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] remote-hg: do not fail on invalid bookmarks
Mercurial can have bookmarks pointing to "nullid" (the empty root
revision), while Git can not have references to it.
When cloning or fetching from a Mercurial repository that has such a
bookmark, the import will fail because git-remote-hg will not be able to
create the corresponding reference.
Warn the user about the invalid reference, and continue the import,
instead of stopping right away.
Also add some test cases for this issue.
Reported-by: Antoine Pelisse
Signed-off-by: Max Horn
---
contrib/remote-helpers/git-remote-hg | 6 +
contrib/remote-helpers/test-hg.sh| 48
2 files changed, 54 insertions(+)
diff --git a/contrib/remote-helpers/git-remote-hg
b/contrib/remote-helpers/git-remote-hg
index eb89ef6..49b2c2e 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -625,6 +625,12 @@ def list_head(repo, cur):
def do_list(parser):
repo = parser.repo
for bmark, node in bookmarks.listbookmarks(repo).iteritems():
+if node == '':
+if fake_bmark == 'default' and bmark == 'master':
+pass
+else:
+warn("Ignoring invalid bookmark '%s'", bmark)
+continue
bmarks[bmark] = repo[node]
cur = repo.dirstate.branch()
diff --git a/contrib/remote-helpers/test-hg.sh
b/contrib/remote-helpers/test-hg.sh
index a933b1e..8d01b32 100755
--- a/contrib/remote-helpers/test-hg.sh
+++ b/contrib/remote-helpers/test-hg.sh
@@ -772,4 +772,52 @@ test_expect_success 'remote double failed push' '
)
'
+test_expect_success 'clone remote with master null bookmark' '
+ test_when_finished "rm -rf gitrepo* hgrepo*" &&
+
+ (
+ hg init hgrepo &&
+ cd hgrepo &&
+ echo a >a &&
+ hg add a &&
+ hg commit -m a &&
+ hg bookmark -r null master
+ ) &&
+
+ git clone "hg::hgrepo" gitrepo &&
+ check gitrepo HEAD a
+'
+
+test_expect_success 'clone remote with default null bookmark' '
+ test_when_finished "rm -rf gitrepo* hgrepo*" &&
+
+ (
+ hg init hgrepo &&
+ cd hgrepo &&
+ echo a >a &&
+ hg add a &&
+ hg commit -m a &&
+ hg bookmark -r null -f default
+ ) &&
+
+ git clone "hg::hgrepo" gitrepo &&
+ check gitrepo HEAD a
+'
+
+test_expect_success 'clone remote with generic null bookmark' '
+ test_when_finished "rm -rf gitrepo* hgrepo*" &&
+
+ (
+ hg init hgrepo &&
+ cd hgrepo &&
+ echo a >a &&
+ hg add a &&
+ hg commit -m a &&
+ hg bookmark -r null bmark
+ ) &&
+
+ git clone "hg::hgrepo" gitrepo &&
+ check gitrepo HEAD a
+'
+
test_done
--
1.9.0.7.ga299b13
--
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] Enable index-pack threading in msysgit.
This adds a Windows implementation of pread. Note that it is NOT
safe to intersperse calls to read() and pread() on a file
descriptor. According to the ReadFile spec, using the 'overlapped'
argument should not affect the implicit position pointer of the
descriptor. Experiments have shown that this is, in fact, a lie.
To accomodate that fact, this change also incorporates:
http://article.gmane.org/gmane.comp.version-control.git/196042
... which gives each index-pack thread its own file descriptor.
Signed-off-by: Stefan Zager
---
builtin/index-pack.c | 30 --
compat/mingw.c | 37 -
compat/mingw.h | 3 +++
config.mak.uname | 1 -
4 files changed, 59 insertions(+), 12 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2f37a38..63b8b0e 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -40,17 +40,17 @@ struct base_data {
int ofs_first, ofs_last;
};
-#if !defined(NO_PTHREADS) && defined(NO_THREAD_SAFE_PREAD)
-/* pread() emulation is not thread-safe. Disable threading. */
-#define NO_PTHREADS
-#endif
-
struct thread_local {
#ifndef NO_PTHREADS
pthread_t thread;
#endif
struct base_data *base_cache;
size_t base_cache_used;
+/*
+ * To accomodate platforms that have pthreads, but don't have a
+ * thread-safe pread, give each thread its own file descriptor.
+ */
+ int pack_fd;
};
/*
@@ -91,7 +91,8 @@ static off_t consumed_bytes;
static unsigned deepest_delta;
static git_SHA_CTX input_ctx;
static uint32_t input_crc32;
-static int input_fd, output_fd, pack_fd;
+static const char *curr_pack;
+static int input_fd, output_fd;
#ifndef NO_PTHREADS
@@ -134,6 +135,7 @@ static inline void unlock_mutex(pthread_mutex_t *mutex)
*/
static void init_thread(void)
{
+ int i;
init_recursive_mutex(&read_mutex);
pthread_mutex_init(&counter_mutex, NULL);
pthread_mutex_init(&work_mutex, NULL);
@@ -141,11 +143,17 @@ static void init_thread(void)
pthread_mutex_init(&deepest_delta_mutex, NULL);
pthread_key_create(&key, NULL);
thread_data = xcalloc(nr_threads, sizeof(*thread_data));
+ for (i = 0; i < nr_threads; i++) {
+ thread_data[i].pack_fd = open(curr_pack, O_RDONLY);
+ if (thread_data[i].pack_fd == -1)
+ die_errno("unable to open %s", curr_pack);
+ }
threads_active = 1;
}
static void cleanup_thread(void)
{
+ int i;
if (!threads_active)
return;
threads_active = 0;
@@ -155,6 +163,8 @@ static void cleanup_thread(void)
if (show_stat)
pthread_mutex_destroy(&deepest_delta_mutex);
pthread_key_delete(key);
+ for (i = 0; i < nr_threads; i++)
+ close(thread_data[i].pack_fd);
free(thread_data);
}
@@ -288,13 +298,13 @@ static const char *open_pack_file(const char *pack_name)
output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR,
0600);
if (output_fd < 0)
die_errno(_("unable to create '%s'"), pack_name);
- pack_fd = output_fd;
+ nothread_data.pack_fd = output_fd;
} else {
input_fd = open(pack_name, O_RDONLY);
if (input_fd < 0)
die_errno(_("cannot open packfile '%s'"), pack_name);
output_fd = -1;
- pack_fd = input_fd;
+ nothread_data.pack_fd = input_fd;
}
git_SHA1_Init(&input_ctx);
return pack_name;
@@ -542,7 +552,7 @@ static void *unpack_data(struct object_entry *obj,
do {
ssize_t n = (len < 64*1024) ? len : 64*1024;
- n = pread(pack_fd, inbuf, n, from);
+ n = pread(get_thread_data()->pack_fd, inbuf, n, from);
if (n < 0)
die_errno(_("cannot pread pack file"));
if (!n)
@@ -1490,7 +1500,7 @@ static void show_pack_info(int stat_only)
int cmd_index_pack(int argc, const char **argv, const char *prefix)
{
int i, fix_thin_pack = 0, verify = 0, stat_only = 0;
- const char *curr_pack, *curr_index;
+ const char *curr_index;
const char *index_name = NULL, *pack_name = NULL;
const char *keep_name = NULL, *keep_msg = NULL;
char *index_name_buf = NULL, *keep_name_buf = NULL;
diff --git a/compat/mingw.c b/compat/mingw.c
index 383cafe..0efc570 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -329,7 +329,42 @@ int mingw_mkdir(const char *path, int mode)
return ret;
}
-int mingw_open (const char *filename, int oflags, ...)
+
+/*
+ * Warning: contrary to the specificiation, when ReadFile() is called
+ * with an 'overlapped' argument, it *will* modify the implict position
+ * pointer for the file descriptor. As a result, it is *not* safe to
+ * inters
Re: [RFC][GSoC] Calling for comments regarding rough draft of proposal
tanay abhra writes: > 2.Other things I should add to the proposal that I have left off?I am > getting confused what extra details I should add to the proposal. I > will add > the informal parts(my background, schedule for summer etc) of the > proposal later. I would not label the schedule and success criteria "informal"; without them how would one judge if the proposal has merits? Other things like your background and previous achievements would become relevant, after it is decided that the proposed project has merits, to see if you are a good fit to work on that project, so I agree with your message that it is sensible to defer them before the other parts of the proposal is ironed out. > #Proposed Improvements > > * Fix "git config --unset" to clean up detritus from sections that are > left empty. > > * Read the configuration from files once and cache the results in an > appropriate data structure in memory. > > * Change `git_config()` to iterate through the pre-read values in > memory rather than re-reading the configuration > files. > > * Add new API calls that allow the cache to be inquired easily and > efficiently. Rewrite other functions like > `git_config_int()` to be cache-aware. I think we already had a discussion to point out git_config_int() is not a good example for this bullet point (check the list archive). The approach seciton seems to use a more sensible example (point 2). > * Rewrite callers to use the new API wherever possible. > > * How to invalidate the cache correctly in the case that the > configuration is changed while `git` is executing. I wouldn't list this as an item of "list of improvements". It is merely a point you have to be careful about because you are doing other "improvements" based on "read all into memory first and do not re-read files" approach, no? In the current code, when somebody does git_config_set() and then later uses git_config() to grab the value of the variable set with the first call, we will read the value written to the file with the first call. With the proposed change, if you parse from the file upfront, callers to git_config_set() will need to somehow invalidate that stale copy in memory, either updating only the changed part (harder) or just discarding the cache (easy). > ##Changing the git_config api to retrieve values from memory > > Approach:- > > We parse the config file once, storing the raw values to records in > memory. After the whole config has been read, iterate through the records, > feeding the surviving values into the callback in the order they were > originally read > (minus deletions). > > Path to follow for the api conversion, > > 1. Convert the parser to read into an in-memory representation, but >leave git_config() as a wrapper which iterates over it. > > 2. Add query functions like config_string_get() which will inquire > cache for values efficiently. > > 3. Convert callbacks to query functions one by one. > > I propose two approaches for the format of the internal cache, > > 1.Using a hashmap to map keys to their values.This would bring as an > advantage, constant time lookups for the values.The implementation > will be similar to "dict" data structure in python, > > for example, section.subsection --mapped-to--> multi_value_string I have no idea what you wanted to illustrate with that "example" at all. > This approach loses the relative order of different config keys. As long as it keeps the order of multi-value elements, it should not be a problem. > 2.Another approach would be to actually represent the syntax tree of the > config file in memory. That would make lookups of individual keys more > expensive, but would enable other manipulation. E.g., if the syntax > tree included nodes for comments and other non-semantic constructs, then > we can use it for a complete rewrite. "for a complete rewrite" of what? > And "git config" becomes: > > 1. Read the tree. > > 2. Perform operations on the tree (add nodes, delete nodes, etc). > > 3. Write out the tree. > > and things like "remove the section header when the last item in the > section is removed" become trivial during step 2. Are you saying you will try both approaches during the summer? You should be able to look-up quickly *and* to preserve order at the same time within one approach, by either annotating the tree with a hash, or the other way around to annotate the hash with each node remembering where in the original file it came from (which you will need to keep in order to report errors anyway). > -- > ##Tidy configuration files > > When a configuration file is repeatedly modified, often garbage is > left behind. For example, after > > git config pull.rebase true > git config --unset pull.rebase > git config pull.rebase true > git config --unset pull.rebase > > the bottom of the configuration file is left with the useless lines > > [pull] >
Re: [PATCH v2] Bump core.deltaBaseCacheLimit to 128MiB
Junio C Hamano writes: > David Kastrup writes: > >> The default of 16MiB causes serious thrashing for large delta chains >> combined with large files. >> >> Signed-off-by: David Kastrup >> --- > > Is that a good argument? Wouldn't the default of 128MiB burden > smaller machines with bloated processes? The default file size before Git forgets about delta compression is 512MiB. Unpacking 500MiB files with 16MiB of delta storage is going to be uglier. >> Forgot the signoff. For the rationale of this patch and the 128MiB >> choice, see the original patch. > > "See the original patch", especially written after three-dash lines > without a reference, will not help future readers of "git log" who > later bisects to find that this change hurt their usage and want to > see why it was done unconditionally (as opposed to encouraging those > who benefit from this change to configure their Git to use larger > value for them, without hurting others). Documentation/config.txt states: core.deltaBaseCacheLimit:: Maximum number of bytes to reserve for caching base objects that may be referenced by multiple deltified objects. By storing the entire decompressed base objects in a cache Git is able to avoid unpacking and decompressing frequently used base objects multiple times. + Default is 16 MiB on all platforms. This should be reasonable for all users/operating systems, except on the largest projects. You probably do not need to adjust this value. I've seen this seriously screwing performance in several projects of mine that don't really count as "largest projects". So the description in combination with the current setting is clearly wrong. > While I can personally afford 128MiB, I do *not* think 16MiB was > chosen more scientifically than the choice of 128MiB this change > proposes to make, and my gut feeling is that this would not have too > big a negative impact to anybody, I would prefer to have a reason > better than gut feeling before accepting a default change. Shrug. I've set my private default anyway, so I don't have anything to gain from this change. If anybody wants to pick this up and put more science into the exact value: be my guest. -- David Kastrup -- 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] t5510: Do not use $(pwd) when fetching / pushing / pulling via rsync
Johannes Schindelin writes: > Hi Sebastian, > > On Wed, 19 Mar 2014, Sebastian Schuberth wrote: > >> On MINGW, "pwd" is defined as "pwd -W" in test-lib.sh. This usually is the >> right thing, but the absolute Windows path with a colon confuses rsync. We >> could use $PWD in this case to work around the issue, but in fact there is >> no need to use an absolute path in the first place, so get rid of it. >> >> This was discovered in the context of the mingwGitDevEnv project and only >> did not surface before with msysgit because the latter does not ship >> rsync. > > ACK > > Ciao, > Dscho 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
Re: [PATCH v2] Bump core.deltaBaseCacheLimit to 128MiB
David Kastrup writes: > The default of 16MiB causes serious thrashing for large delta chains > combined with large files. > > Signed-off-by: David Kastrup > --- Is that a good argument? Wouldn't the default of 128MiB burden smaller machines with bloated processes? > Forgot the signoff. For the rationale of this patch and the 128MiB > choice, see the original patch. "See the original patch", especially written after three-dash lines without a reference, will not help future readers of "git log" who later bisects to find that this change hurt their usage and want to see why it was done unconditionally (as opposed to encouraging those who benefit from this change to configure their Git to use larger value for them, without hurting others). While I can personally afford 128MiB, I do *not* think 16MiB was chosen more scientifically than the choice of 128MiB this change proposes to make, and my gut feeling is that this would not have too big a negative impact to anybody, I would prefer to have a reason better than gut feeling before accepting a default change. Thanks. > Documentation/config.txt | 2 +- > environment.c| 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 73c8973..1b6950a 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -484,7 +484,7 @@ core.deltaBaseCacheLimit:: > to avoid unpacking and decompressing frequently used base > objects multiple times. > + > -Default is 16 MiB on all platforms. This should be reasonable > +Default is 128 MiB on all platforms. This should be reasonable > for all users/operating systems, except on the largest projects. > You probably do not need to adjust this value. > + > diff --git a/environment.c b/environment.c > index c3c8606..73ed670 100644 > --- a/environment.c > +++ b/environment.c > @@ -37,7 +37,7 @@ int core_compression_seen; > int fsync_object_files; > size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE; > size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT; > -size_t delta_base_cache_limit = 16 * 1024 * 1024; > +size_t delta_base_cache_limit = 128 * 1024 * 1024; > unsigned long big_file_threshold = 512 * 1024 * 1024; > const char *pager_program; > int pager_use_color = 1; -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3][GSOC] diff: use is_dot_or_dotdot() instead of strcmp()
On Wed, Mar 19, 2014 at 7:23 AM, Hiroyuki Sano wrote:
> Subject: diff: use is_dot_or_dotdot() instead of strcmp()
You probably meant 'diff-no-index' rather than 'diff'.
You could make the subject a bit more explanatory by saying:
use is_dot_or_dotdot() instead of a manual "."/".." check
> The is_dot_or_dotdot() is used to check if the string is either "." or "..".
It's pretty obvious what this function does, so it's not necessary to
explain it.
> Include the "dir.h" header file to use is_dot_or_dotdot().
Including dir.h is a obvious requirement of using is_dot_or_dotdot(),
thus also does not require explanation.
Otherwise, the patch looks fine.
> Signed-off-by: Hiroyuki Sano
> ---
> diff-no-index.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 20b6a8a..8e642b3 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -11,6 +11,7 @@
> #include "tag.h"
> #include "diff.h"
> #include "diffcore.h"
> +#include "dir.h"
> #include "revision.h"
> #include "log-tree.h"
> #include "builtin.h"
> @@ -25,7 +26,7 @@ static int get_path_list(const char *path, struct
> string_list *list)
> return error("Could not open directory %s", path);
>
> while ((e = readdir(dir)))
> - if (strcmp(".", e->d_name) && strcmp("..", e->d_name))
> + if (!is_dot_or_dotdot(e->d_name))
> string_list_insert(list, e->d_name);
>
> closedir(dir);
> --
> 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 v2 1/3][GSOC] diff: rename read_directory() to get_path_list()
On Wed, Mar 19, 2014 at 7:23 AM, Hiroyuki Sano wrote:
> Subject: diff: rename read_directory() to get_path_list()
You probably mean 'diff-no-index' here rather than 'diff'.
> Including "dir.h" in "diff-no-index.c", it causes a compile error, because
> the same name function read_directory() is declared globally in "dir.h".
It might be a bit clearer to give a hint as to why dir.h will be a problem:
A subsequent patch will include dir.h in diff-no-index.c,
however, dir.h declares a read_directory() which is different
from the one defined statically by diff-no-index.c.
> This change is to avoid conflicts as above.
Good explanation, but write in imperative mood:
Rename the local read_directory() to avoid the conflict.
> Signed-off-by: Hiroyuki Sano
> ---
> diff-no-index.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 8e10bff..20b6a8a 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -16,7 +16,7 @@
> #include "builtin.h"
> #include "string-list.h"
>
> -static int read_directory(const char *path, struct string_list *list)
> +static int get_path_list(const char *path, struct string_list *list)
> {
> DIR *dir;
> struct dirent *e;
> @@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,
> int i1, i2, ret = 0;
> size_t len1 = 0, len2 = 0;
>
> - if (name1 && read_directory(name1, &p1))
> + if (name1 && get_path_list(name1, &p1))
> return -1;
> - if (name2 && read_directory(name2, &p2)) {
> + if (name2 && get_path_list(name2, &p2)) {
> string_list_clear(&p1, 0);
> return -1;
> }
> --
> 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: Rebasing merge commits
On Tue, Mar 18, 2014 at 01:11:06PM -0500, Robert Dailey wrote: > What's the general recommendation on rebasing after > creating a merge commit on my branch? Basically, rebase does not do anything magic. It just cherry-picks commits over a custom revision. You could do it manually: reset to the master and then cherry-pick all your commits (or whichever you would like to pick) from the older topic1. git rebase --onto would be useful to make it a bit less manual it there are many commits and few merges. I used to do it myself, but later I have made my own rebase which can somehow handle merges: https://github.com/max630/git-rebase2/ You could try it also. -- 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 v2] git-rebase: Teach rebase "-" shorthand.
John Keeping writes:
>> I thought your suggestion was:
>>
>> 'rebase @{-1}' says 'Fast-fowarded HEAD to @{-1}'. It should say
>> 'Fast-forwarded HEAD to 4f407407 (rebase: allow "-" short-hand
>> for the previous branch, 2014-03-19)' instead.
>>
>> Or it could be:
>>
>> 'rebase @{-1}' says 'Fast-fowarded HEAD to @{-1}'. It should say
>> 'Fast-forwarded HEAD to master' instead.
>>
>> In either case, it does not look like such a change is about
>> teaching "-" as a synonym to "@{-1}".
>
> My suggestion was specifically:
>
> 'rebase -' says 'Fast-forwarded HEAD to -'. It should say
> 'Fast-forwarded HEAD to master' instead.
OK, it was closer to the latter.
But why is it OK to leave @{-1}, which is just as "hmm, I do not
remember what the previous branch was myself" when the user says
"@{-1}" in the output while it not OK to leave "-" in the output?
I do not think of any sane reason, and that is why I think this
improvement is not part of "teaching rebase that '-' can be used in
place of @{-1}" topic.
--
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] Enable index-pack threading in msysgit.
sza...@chromium.org writes:
> This adds a Windows implementation of pread. Note that it is NOT
> safe to intersperse calls to read() and pread() on a file
> descriptor. According to the ReadFile spec, using the 'overlapped'
> argument should not affect the implicit position pointer of the
> descriptor. Experiments have shown that this is, in fact, a lie.
>
> To accomodate that fact, this change also incorporates:
>
> http://article.gmane.org/gmane.comp.version-control.git/196042
>
> ... which gives each index-pack thread its own file descriptor.
> ---
Sign-off?
The new "per-thread file descriptors to the same thing" in a generic
codepath is a bit of eyesore. For index-pack, keeping as many file
descritors open to the current pack as the worker threads are should
not be too bad, but could we have some comment next to the field
definition please (e.g. "Windows emulation of pread() needs separate
fd per thread; see $URL for details" or something)?
> builtin/index-pack.c | 21 -
> compat/mingw.c | 31 ++-
> compat/mingw.h | 3 +++
> config.mak.uname | 1 -
> 4 files changed, 49 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 2f37a38..c02dd4c 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -51,6 +51,7 @@ struct thread_local {
> #endif
> struct base_data *base_cache;
> size_t base_cache_used;
> + int pack_fd;
> };
>
> /*
> @@ -91,7 +92,8 @@ static off_t consumed_bytes;
> static unsigned deepest_delta;
> static git_SHA_CTX input_ctx;
> static uint32_t input_crc32;
> -static int input_fd, output_fd, pack_fd;
> +static const char *curr_pack;
> +static int input_fd, output_fd;
>
> #ifndef NO_PTHREADS
>
> @@ -134,6 +136,7 @@ static inline void unlock_mutex(pthread_mutex_t *mutex)
> */
> static void init_thread(void)
> {
> + int i;
> init_recursive_mutex(&read_mutex);
> pthread_mutex_init(&counter_mutex, NULL);
> pthread_mutex_init(&work_mutex, NULL);
> @@ -141,11 +144,17 @@ static void init_thread(void)
> pthread_mutex_init(&deepest_delta_mutex, NULL);
> pthread_key_create(&key, NULL);
> thread_data = xcalloc(nr_threads, sizeof(*thread_data));
> + for (i = 0; i < nr_threads; i++) {
> + thread_data[i].pack_fd = open(curr_pack, O_RDONLY);
> + if (thread_data[i].pack_fd == -1)
> + die_errno("unable to open %s", curr_pack);
> + }
> threads_active = 1;
> }
>
> static void cleanup_thread(void)
> {
> + int i;
> if (!threads_active)
> return;
> threads_active = 0;
> @@ -155,6 +164,8 @@ static void cleanup_thread(void)
> if (show_stat)
> pthread_mutex_destroy(&deepest_delta_mutex);
> pthread_key_delete(key);
> + for (i = 0; i < nr_threads; i++)
> + close(thread_data[i].pack_fd);
> free(thread_data);
> }
>
> @@ -288,13 +299,13 @@ static const char *open_pack_file(const char *pack_name)
> output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR,
> 0600);
> if (output_fd < 0)
> die_errno(_("unable to create '%s'"), pack_name);
> - pack_fd = output_fd;
> + nothread_data.pack_fd = output_fd;
> } else {
> input_fd = open(pack_name, O_RDONLY);
> if (input_fd < 0)
> die_errno(_("cannot open packfile '%s'"), pack_name);
> output_fd = -1;
> - pack_fd = input_fd;
> + nothread_data.pack_fd = input_fd;
> }
> git_SHA1_Init(&input_ctx);
> return pack_name;
> @@ -542,7 +553,7 @@ static void *unpack_data(struct object_entry *obj,
>
> do {
> ssize_t n = (len < 64*1024) ? len : 64*1024;
> - n = pread(pack_fd, inbuf, n, from);
> + n = pread(get_thread_data()->pack_fd, inbuf, n, from);
> if (n < 0)
> die_errno(_("cannot pread pack file"));
> if (!n)
> @@ -1490,7 +1501,7 @@ static void show_pack_info(int stat_only)
> int cmd_index_pack(int argc, const char **argv, const char *prefix)
> {
> int i, fix_thin_pack = 0, verify = 0, stat_only = 0;
> - const char *curr_pack, *curr_index;
> + const char *curr_index;
> const char *index_name = NULL, *pack_name = NULL;
> const char *keep_name = NULL, *keep_msg = NULL;
> char *index_name_buf = NULL, *keep_name_buf = NULL;
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 383cafe..6cc85d6 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -329,7 +329,36 @@ int mingw_mkdir(const char *path, int mode)
> return ret;
> }
>
> -int mingw_open (const char *filename, int oflags, ...)
> +
> +ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)
> +{
> + HANDLE hand = (HANDLE)_get_osfhandle(fd);
> +
Re: What's cooking in git.git (Mar 2014, #03; Fri, 14)
Max Horn writes: > On 17.03.2014, at 18:01, Junio C Hamano wrote: > >> Torsten Bögershausen writes: >> >>> On 2014-03-14 23.09, Junio C Hamano wrote: * ap/remote-hg-skip-null-bookmarks (2014-01-02) 1 commit - remote-hg: do not fail on invalid bookmarks Reported to break tests ($gmane/240005) Expecting a reroll. >>> I wonder what should happen here. >>> The change breaks all the tests in test-hg-hg-git.sh >>> (And the breakage may prevent us from detecting other breakages) >>> >>> The ideal situation would be to have an extra test case for the problem >>> which we try to fix with this patch. >>> >>> Antoine, is there any way to make your problem reproducable ? >>> And based on that, to make a patch which passes all test cases ? >> >> After re-reading the thread briefly (there're just five messages) >> >> http://thread.gmane.org/gmane.comp.version-control.git/239797/focus=240069 > > For some reason, that link does not contain all messages from that > conversation (unfortunately, I have seen GMane do that on multiple > occasions. I hence try not to rely on it for reviewing email > history -- I just don't trust it). In particular, it misses this > crucial post: [jc: please avoid overlong lines; I re-flowed above] > http://thread.gmane.org/gmane.comp.version-control.git/239830 Interesting. > The (or at least "a") root cause has actually been > discovered. Would a patch that adds an xfail test case for it be > acceptable? Do you mean a patch that only adds a new test that expects a failure to the current code, without touching the current code that has the bug it exposes? That would be a good place to start. > ... As a matter of fact, I a know a few more bugs in remote-hg for > which I could produce xfail test cases. Of course I'd prefer to > put them in together with a fix, but I don't know when I can get > to that, if ever. So, would such changes be welcome? Surely. That is to keep tabs on bugs in an actionable form; it is a better way of bug tracking than having a bug-tracker that is not actively maintained, I would think. -- 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] tests: use "env" to run commands with temporary env-var settings
David Tran writes: > Originally, we would use "VAR=VAL command" to execute a test command with > environment variable(s) only for that command. This does not work for commands > that are shell functions (most notably test functions like "test_must_fail"); > the result of the assignment is retained and affects later commands. > > To avoid this, we assigned and exported the environment variables and run > the test(s) in a subshell like this, > > ( > VAR=VAL && > export VAR > test_must_fail git command to be tested > ) > > Using the "env" utility, we should be able to say > > test_must_fail git command to be tested > > which is much short and easier to read. Looks familiar ;-) but it seems the changes from the original you took it from all look worsening, not improvements, to me. >>Isn't GIT_CONFIG here another way of saying: >> >> test_must_fail git config -f doesnotexist --list >> >>Perhaps that is shorter and more readable still (and there are a few >>similar cases in this patch. > I'll ignore this for now. If needed I can make another patch to resolve this. Yes, I think that is sensible. And it does not have to be done by you. > Hopefully this should be all of it. Seems to be well done. 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
Re: [PATCH v2] git-rebase: Teach rebase "-" shorthand.
On Wed, Mar 19, 2014 at 12:41:31PM -0700, Junio C Hamano wrote:
> John Keeping writes:
>
> > On Wed, Mar 19, 2014 at 12:02:01PM -0700, Junio C Hamano wrote:
> >> John Keeping writes:
> >>
> >> > On Wed, Mar 19, 2014 at 10:53:01AM -0700, Junio C Hamano wrote:
> >> >>"rebase -" with your change still says something like this:
> >> >>
> >> >> First, rewinding head to replay your work on top of it...
> >> >> Fast-forwarded HEAD to @{-1}.
> >> >>
> >> >>instead of "Fast-forwarded HEAD to -". Somebody may later want
> >> >>to "fix" this, making these two eye-candy output to be different
> >> >>from each other, and what your test expects will no longer hold
> >> >>(not that I think it is better to say "-" instead of @{-1}
> >> >>there).
> >> >
> >> > I don't think either of these is correct. When using "-" with the
> >> > commands that already support it, I have occasionally found that "-"
> >> > isn't what I thought it was.
> >> >
> >> > Can we use `git name-rev` to put the actual name here, so that people
> >> > who have not done what they intended can hopefully notice sooner?
> >>
> >> That sounds like a right thing to do. It however is totally
> >> orthogonal to the change we are discussing, and should be done as a
> >> separate patch.
> >
> > Is it not part of adding support for "-"?
>
> I thought your suggestion was:
>
> 'rebase @{-1}' says 'Fast-fowarded HEAD to @{-1}'. It should say
> 'Fast-forwarded HEAD to 4f407407 (rebase: allow "-" short-hand
> for the previous branch, 2014-03-19)' instead.
>
> Or it could be:
>
> 'rebase @{-1}' says 'Fast-fowarded HEAD to @{-1}'. It should say
> 'Fast-forwarded HEAD to master' instead.
>
> In either case, it does not look like such a change is about
> teaching "-" as a synonym to "@{-1}".
My suggestion was specifically:
'rebase -' says 'Fast-forwarded HEAD to -'. It should say
'Fast-forwarded HEAD to master' instead.
I'm not sure it's desirable to attempt to canonicalise whatever the user
writes on the command line, but since we're special-casing '-' I think
it is a good thing to print the branch name in that case.
--
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
[RFC][GSoC] Calling for comments regarding rough draft of proposal
Hi, I have already done the microproject, which has been merged into main last week. I have prepared a rough draft of my proposal for review, read all the previous mailing list threads about it.I am reading the codebase little by little. Please suggest improvements on the following topics, 1.I have read one-third of config.c and will complete reading it by tomorrow.Is there any other piece of code relevant to this proposal? 2.Other things I should add to the proposal that I have left off?I am getting confused what extra details I should add to the proposal. I will add the informal parts(my background, schedule for summer etc) of the proposal later. 3.Did I understand anything wrong or if my approach to solving problems is incorrect,if yes, I will redraft my proposal according to your suggestions. -- #GSoC Proposal : Git configuration API improvements --- #Proposed Improvements * Fix "git config --unset" to clean up detritus from sections that are left empty. * Read the configuration from files once and cache the results in an appropriate data structure in memory. * Change `git_config()` to iterate through the pre-read values in memory rather than re-reading the configuration files. * Add new API calls that allow the cache to be inquired easily and efficiently. Rewrite other functions like `git_config_int()` to be cache-aware. * Rewrite callers to use the new API wherever possible. * How to invalidate the cache correctly in the case that the configuration is changed while `git` is executing. #Future Improvements *Allow configuration values to be unset via a config file -- ##Changing the git_config api to retrieve values from memory Approach:- We parse the config file once, storing the raw values to records in memory. After the whole config has been read, iterate through the records, feeding the surviving values into the callback in the order they were originally read (minus deletions). Path to follow for the api conversion, 1. Convert the parser to read into an in-memory representation, but leave git_config() as a wrapper which iterates over it. 2. Add query functions like config_string_get() which will inquire cache for values efficiently. 3. Convert callbacks to query functions one by one. I propose two approaches for the format of the internal cache, 1.Using a hashmap to map keys to their values.This would bring as an advantage, constant time lookups for the values.The implementation will be similar to "dict" data structure in python, for example, section.subsection --mapped-to--> multi_value_string This approach loses the relative order of different config keys. 2.Another approach would be to actually represent the syntax tree of the config file in memory. That would make lookups of individual keys more expensive, but would enable other manipulation. E.g., if the syntax tree included nodes for comments and other non-semantic constructs, then we can use it for a complete rewrite. And "git config" becomes: 1. Read the tree. 2. Perform operations on the tree (add nodes, delete nodes, etc). 3. Write out the tree. and things like "remove the section header when the last item in the section is removed" become trivial during step 2. I still prefer the hashmap way of implementing the cache,as empty section headers are not so problematic(no processing pitfalls) and are sometimes annotated with comments which become redundant and confusing if the section header is removed.As for the aesthetic problem I propose a different solution for it below. -- ##Tidy configuration files When a configuration file is repeatedly modified, often garbage is left behind. For example, after git config pull.rebase true git config --unset pull.rebase git config pull.rebase true git config --unset pull.rebase the bottom of the configuration file is left with the useless lines [pull] [pull] Also,setting a config value, appends the key-value pair at the end of file without checking for empty main keys even if the main key(like [my]) is already present and empty.It works fine if the main key with an already present sub-key. for example:- git config pull.rebase true git config --unset pull.rebase git config pull.rebase true git config pull.option true gives [pull] [pull] rebase = true option = true Also, a possible detriment is presence of comments, For Example:- [my] # This section is for my own private settings Expected output: 1. When we delete the last key in a section, we should be able to delete the section header. 2. When we add a key into a section, we should be able to reus
[PATCH] t5510: Do not use $(pwd) when fetching / pushing / pulling via rsync
On MINGW, "pwd" is defined as "pwd -W" in test-lib.sh. This usually is the right thing, but the absolute Windows path with a colon confuses rsync. We could use $PWD in this case to work around the issue, but in fact there is no need to use an absolute path in the first place, so get rid of it. This was discovered in the context of the mingwGitDevEnv project and only did not surface before with msysgit because the latter does not ship rsync. Signed-off-by: Sebastian Schuberth --- t/t5510-fetch.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index ab28594..5acd753 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -301,7 +301,7 @@ test_expect_success 'fetch via rsync' ' mkdir rsynced && (cd rsynced && git init --bare && -git fetch "rsync:$(pwd)/../.git" master:refs/heads/master && +git fetch "rsync:../.git" master:refs/heads/master && git gc --prune && test $(git rev-parse master) = $(cd .. && git rev-parse master) && git fsck --full) @@ -312,7 +312,7 @@ test_expect_success 'push via rsync' ' (cd rsynced2 && git init) && (cd rsynced && -git push "rsync:$(pwd)/../rsynced2/.git" master) && +git push "rsync:../rsynced2/.git" master) && (cd rsynced2 && git gc --prune && test $(git rev-parse master) = $(cd .. && git rev-parse master) && @@ -323,7 +323,7 @@ test_expect_success 'push via rsync' ' mkdir rsynced3 && (cd rsynced3 && git init) && - git push --all "rsync:$(pwd)/rsynced3/.git" && + git push --all "rsync:rsynced3/.git" && (cd rsynced3 && test $(git rev-parse master) = $(cd .. && git rev-parse master) && git fsck --full) -- 1.8.5.2.msysgit.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 v2] git-rebase: Teach rebase "-" shorthand.
John Keeping writes:
> On Wed, Mar 19, 2014 at 12:02:01PM -0700, Junio C Hamano wrote:
>> John Keeping writes:
>>
>> > On Wed, Mar 19, 2014 at 10:53:01AM -0700, Junio C Hamano wrote:
>> >>"rebase -" with your change still says something like this:
>> >>
>> >> First, rewinding head to replay your work on top of it...
>> >> Fast-forwarded HEAD to @{-1}.
>> >>
>> >>instead of "Fast-forwarded HEAD to -". Somebody may later want
>> >>to "fix" this, making these two eye-candy output to be different
>> >>from each other, and what your test expects will no longer hold
>> >>(not that I think it is better to say "-" instead of @{-1}
>> >>there).
>> >
>> > I don't think either of these is correct. When using "-" with the
>> > commands that already support it, I have occasionally found that "-"
>> > isn't what I thought it was.
>> >
>> > Can we use `git name-rev` to put the actual name here, so that people
>> > who have not done what they intended can hopefully notice sooner?
>>
>> That sounds like a right thing to do. It however is totally
>> orthogonal to the change we are discussing, and should be done as a
>> separate patch.
>
> Is it not part of adding support for "-"?
I thought your suggestion was:
'rebase @{-1}' says 'Fast-fowarded HEAD to @{-1}'. It should say
'Fast-forwarded HEAD to 4f407407 (rebase: allow "-" short-hand
for the previous branch, 2014-03-19)' instead.
Or it could be:
'rebase @{-1}' says 'Fast-fowarded HEAD to @{-1}'. It should say
'Fast-forwarded HEAD to master' instead.
In either case, it does not look like such a change is about
teaching "-" as a synonym to "@{-1}".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib.sh: use printf instead of echo
David Kastrup writes: > Junio C Hamano writes: > >> Jonathan Nieder writes: >> >>> Junio C Hamano wrote: > Uwe Storbeck wrote: >>> >> +printf '%s\n' "$@" | sed -e 's/^/# /' This is wrong, isn't it? Why do we want one line per item here? >>> >>> Yes, Hannes caught the same, too. Sorry for the sloppiness. >>> >>> We currently use "echo" all over the place (e.g., 'echo "$path"' in >>> git-sh-setup), and every time we fix it there is a chance of making >>> mistakes. I wonder if it would make sense to add a helper to make the >>> echo calls easier to replace: >> >> I agree that we would benefit from having a helper to print a single >> line, which we very often do, without having to worry about the >> boilerplate '%s\n' of printf or the portability gotcha of echo. >> >> I am a bit reluctant to name the helper "sane_echo" to declare "echo >> that interprets backslashes in the string is insane", though. > > raw_echo Yeah, but the thing is, this is not even "raw" if you view it from the direction of knowing what "echo" does. That is why I repeated "helper to print a single line", which is a viewpoint from the user side. "We do not care how it is implemented, we just want a single line printed" is what we want to express, which "say" is perfectly in line with. "We use a subset semantics of 'echo' to implement it" is of secondary concern. -- 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] git-rebase: Teach rebase "-" shorthand.
On Wed, Mar 19, 2014 at 12:02:01PM -0700, Junio C Hamano wrote:
> John Keeping writes:
>
> > On Wed, Mar 19, 2014 at 10:53:01AM -0700, Junio C Hamano wrote:
> >>"rebase -" with your change still says something like this:
> >>
> >> First, rewinding head to replay your work on top of it...
> >> Fast-forwarded HEAD to @{-1}.
> >>
> >>instead of "Fast-forwarded HEAD to -". Somebody may later want
> >>to "fix" this, making these two eye-candy output to be different
> >>from each other, and what your test expects will no longer hold
> >>(not that I think it is better to say "-" instead of @{-1}
> >>there).
> >
> > I don't think either of these is correct. When using "-" with the
> > commands that already support it, I have occasionally found that "-"
> > isn't what I thought it was.
> >
> > Can we use `git name-rev` to put the actual name here, so that people
> > who have not done what they intended can hopefully notice sooner?
>
> That sounds like a right thing to do. It however is totally
> orthogonal to the change we are discussing, and should be done as a
> separate patch.
Is it not part of adding support for "-"?
I'm not arguing for a change to any existing functionality, just to the
behaviour introduced by this patch, which is basically a change from
"@{-1}" to "$(git name-rev --name-only @{-1})" in the patch. (The error
handling of name-rev appears not to be very useful here when the
previous branch has been deleted, so I don't think it's quite that
simple, but that's the principle.)
--
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] Enable index-pack threading in msysgit.
On Wed, Mar 19, 2014 at 9:57 AM, Stefan Zager wrote: >> >> I still don't understand how compat/pread.c does not work with pack_fd >> per thread. I don't have Windows to test, but I forced compat/pread.c >> on on Linux with similar pack_fd changes and it worked fine, helgrind >> only complained about progress.c. >> >> A pread() implementation that is thread-safe with condition sounds >> like an invite for trouble later. And I don't think converting read() >> to pread() is a good idea. Platforms that rely on pread() will hit >> first because of more use of compat/pread.c. read() seeks while >> pread() does not, so we have to audit more code.. > > Using one fd per thread is all well and good for something like > index-pack, which only accesses a single pack file. But using that > heuristic to add threading elsewhere is probably not going to work. > For example, I have a patch in progress to add threading to checkout, > and another one planned to add threading to status. In both cases, we > would need one fd per thread per pack file, which is pretty > ridiculous. > > There really aren't very many calls to read() in the code. I don't > think it would be very difficult to eliminate the remaining ones. The > more interesting question, I think is: what platforms still don't have > a thread-safe pread implementation? I don't want to go too deep down the rabbit hole here. We don't have to solve the read() vs. pread() issue once for all right now; that can wait for another day. The pread() implementation in this patch is certainly no worse than the one in compat/pread.c. Stefan -- 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][GSoC] diff-no-index: rename read_directory()
On Wed, Mar 19, 2014 at 2:31 PM, Junio C Hamano wrote:
> Brian Bourn writes:
>
>> It would be desirable to replace manual checking of "." or ".."
>> in diff-no-index.c with is_dot_or_dotdot(), which is defined
>> in dir.h, however, dir.h declares a read_directory() which conflicts
>> with a (different) static read_directory() defined
>> in diff-no-index.c. As a preparatory step, rename the local
>> read_directory() to avoid the collision.
>>
>> Signed-off-by: Brian Bourn
>> ---
>> Part 1 of my submission for GSoC
>> diff-no-index.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> Looks good to me. Doesn't Eric deserve a "Helped-by:" above?
Both patches look good to me too.
One final comment for future submissions (no need to resend this one):
As a courtesy to reviewers, below the "---" line after your sign-off,
provide a link to the previous attempt (like this [1]) and explain
what changed since the since the last version.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/244412
>> diff --git a/diff-no-index.c b/diff-no-index.c
>> index 8e10bff..ec51106 100644
>> --- a/diff-no-index.c
>> +++ b/diff-no-index.c
>> @@ -16,7 +16,7 @@
>> #include "builtin.h"
>> #include "string-list.h"
>>
>> -static int read_directory(const char *path, struct string_list *list)
>> +static int read_directory_contents(const char *path, struct string_list
>> *list)
>> {
>> DIR *dir;
>> struct dirent *e;
>> @@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,
>> int i1, i2, ret = 0;
>> size_t len1 = 0, len2 = 0;
>>
>> - if (name1 && read_directory(name1, &p1))
>> + if (name1 && read_directory_contents(name1, &p1))
>> return -1;
>> - if (name2 && read_directory(name2, &p2)) {
>> + if (name2 && read_directory_contents(name2, &p2)) {
>> string_list_clear(&p1, 0);
>> return -1;
>> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git-rebase: Teach rebase "-" shorthand.
John Keeping writes:
> On Wed, Mar 19, 2014 at 10:53:01AM -0700, Junio C Hamano wrote:
>>"rebase -" with your change still says something like this:
>>
>> First, rewinding head to replay your work on top of it...
>> Fast-forwarded HEAD to @{-1}.
>>
>>instead of "Fast-forwarded HEAD to -". Somebody may later want
>>to "fix" this, making these two eye-candy output to be different
>>from each other, and what your test expects will no longer hold
>>(not that I think it is better to say "-" instead of @{-1}
>>there).
>
> I don't think either of these is correct. When using "-" with the
> commands that already support it, I have occasionally found that "-"
> isn't what I thought it was.
>
> Can we use `git name-rev` to put the actual name here, so that people
> who have not done what they intended can hopefully notice sooner?
That sounds like a right thing to do. It however is totally
orthogonal to the change we are discussing, and should be done as a
separate patch.
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
Re: What's cooking in git.git (Mar 2014, #03; Fri, 14)
Max Horn writes: > So, one more silly (bikeshedding) question: should I do this as one big > patch adding multiple xfail tests - or one commit per test, with perhaps a > brief description of the issue at hand? Or should a code comment next to > the failing test explain things? Judging from the next paragraph, one patch per issue sounds like a good organization to help those who would want to fix these issues. > Actually, some of those bugs might require a lengthy background > explanation, so yet another variant would be to write an email here > With an explanation, then add a gmane ref to the commit message... Please first try to find a way that does not need any external references---not everybody is always online. A two-page description in the log message for a new five-line test_expect_fail piece is perfectly fine. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse --parseopt: option argument name hints
Ilya Bobyr writes: > I can not find this particular patch in the latest "What's cooking" email. > Is there something I can do? IIRC, I think I was waiting for the version with a new "Usage text" section to the documentation you alluded to in this exchange ($gmane/243924): Ilya Bobyr writes: > On 3/11/2014 12:10 PM, Junio C Hamano wrote: >> Documentation on the whole argument parsing is quite short, so,... > ... > I though that an example just to describe `argh' while useful would > look a bit disproportional, compared to the amount of text on > --parseopt. > > But now that I've added a "Usage text" section to looks quite in place. > > I just realized that the second patch I sent did not contain the > changes. Sorry about - I will resend it. > It does not seems like there is a lot of interest, so I am not sure > there will be a lot of discussion. > It is a minor fix and considering the number of the emails on the > list, I do not unexpected this kind of stuff to be very popular. > But it seems like a valid improvement to me. > Maybe I am missing something? You did the right thing by sending a reminder message with a pointer to help others locate the original (like the one I am responding to), as nobody can keep up with a busy list traffic. > Same questions about this one: > > [PATCH] gitk: replace SHA1 entry field on keyboard paste > http://www.mail-archive.com/git@vger.kernel.org/msg45040.html > > I think they are more or less similar, except that the second one is > just trivial. I do not remember if I forwarded the patch to the area maintainer Paul Mackerras , but if I didn't please do so yourself. The changes to gitk and git-gui come to me via their own project repositories. -- 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][GSOC] diff: rename read_directory() to get_directory_list()
Junio C Hamano writes:
>>> -static int read_directory(const char *path, struct string_list *list)
>>> +static int get_directory_list(const char *path, struct string_list *list)
>>
>> Renaming is a good idea but the new name sounds like you are
>> grabbing the names of directories, ignoring all the files, no?
>
> I am tempted to suggest, because this is an internal implementation
> detail only visible to this narrow corner of the system, calling
> this just
>
> static int ls(const char *path, struct string_list *result)
>
> ;-)
Scratch that one.
I'll take read_directory_contents() from Brian Bourn, which
essentially is the same patch.
Thanks.
-- >8 --
From: Brian Bourn
Date: Wed, 19 Mar 2014 11:58:21 -0400
Subject: [PATCH] diff-no-index: rename read_directory()
In the next patch, we will replace a manual checking of "." or ".."
with a call to is_dot_or_dotdot() defined in dir.h. The private
function read_directory() defined in this file will conflict with
the global function declared there when we do so.
As a preparatory step, rename the private read_directory() to avoid
the name collision.
Signed-off-by: Brian Bourn
Helped-by: Eric Sunshine
Signed-off-by: Junio C Hamano
---
diff-no-index.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index 33e5982..3e4f47e 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -16,7 +16,7 @@
#include "builtin.h"
#include "string-list.h"
-static int read_directory(const char *path, struct string_list *list)
+static int read_directory_contents(const char *path, struct string_list *list)
{
DIR *dir;
struct dirent *e;
@@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,
int i1, i2, ret = 0;
size_t len1 = 0, len2 = 0;
- if (name1 && read_directory(name1, &p1))
+ if (name1 && read_directory_contents(name1, &p1))
return -1;
- if (name2 && read_directory(name2, &p2)) {
+ if (name2 && read_directory_contents(name2, &p2)) {
string_list_clear(&p1, 0);
return -1;
}
--
1.9.1-423-g4596e3a
--
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/3][GSOC] fsck: replace if-statements to logical expressions
Hiroyuki Sano writes:
> There were two different ways to check flag values, one way is
> using if-statement, and the other way is using logical expression.
>
> To make sensible, replace if-statements to logical expressions in
> fsck_tree().
The change described by these two paragraphs makes sense to me, but
the "to make sensible" phrasing made me hiccup while reading it.
fsck_tree() uses two different ways to set flag values, many
with a simple if () condition that guards an assignment, and
one with an bitwise-or assignment operator.
Unify them to the latter, as it is shorter and easier to
read when the condition is short and to the point, which all
of them are.
or something?
> When checking "has_dot" and "has_dotdot", use is_dot_or_dotdot()
> instead of strcmp() to avoid hard coding.
I am not sure how this change is an improvement. Besides being
seemingly inefficient by checking name[1] twice (which is not a huge
objection, as a sensible compiler would notice and optimize), the
caller that checks name[1] already hardcodes its knowledge on what
is_dot_or_dotdot() does, e.g. when it returns true, name[0] is never
NUL, and name[1] is NUL only when it saw a dot and not a dotdot, so
the "to avoid hard coding" does not really justify this change.
I further wonder if
...
if (!name[0]) {
has_empty_name = 1;
} else if (name[0] == '.') {
has_dot |= !name[1];
has_dotdot |= name[1] == '.' && !name[2];
has_dotgit |= !strcmp(name + 1, "git");
}
...
may be an improvement (this is not a suggestion--when I say I
wonder, I usually do not know the answer). It defeats the "unify
the two styles" theme of this change, so...
> The is_dot_or_dotdot() is used to check if the string is
> either "." or "..".
> Include the "dir.h" header file to use is_dot_or_dotdot().
>
> Signed-off-by: Hiroyuki Sano
> ---
> fsck.c | 19 +++
> 1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index b3022ad..08f613d 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -6,6 +6,7 @@
> #include "commit.h"
> #include "tag.h"
> #include "fsck.h"
> +#include "dir.h"
>
> static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
> {
> @@ -165,18 +166,12 @@ static int fsck_tree(struct tree *item, int strict,
> fsck_error error_func)
>
> sha1 = tree_entry_extract(&desc, &name, &mode);
>
> - if (is_null_sha1(sha1))
> - has_null_sha1 = 1;
> - if (strchr(name, '/'))
> - has_full_path = 1;
> - if (!*name)
> - has_empty_name = 1;
> - if (!strcmp(name, "."))
> - has_dot = 1;
> - if (!strcmp(name, ".."))
> - has_dotdot = 1;
> - if (!strcmp(name, ".git"))
> - has_dotgit = 1;
> + has_null_sha1 |= is_null_sha1(sha1);
> + has_full_path |= !!strchr(name, '/');
> + has_empty_name |= !*name;
> + has_dot |= is_dot_or_dotdot(name) && !name[1];
> + has_dotdot |= is_dot_or_dotdot(name) && name[1];
> + has_dotgit |= !strcmp(name, ".git");
> has_zero_pad |= *(char *)desc.buffer == '0';
> update_tree_entry(&desc);
--
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][GSoC] diff-no-index: rename read_directory()
Brian Bourn writes:
> It would be desirable to replace manual checking of "." or ".."
> in diff-no-index.c with is_dot_or_dotdot(), which is defined
> in dir.h, however, dir.h declares a read_directory() which conflicts
> with a (different) static read_directory() defined
> in diff-no-index.c. As a preparatory step, rename the local
> read_directory() to avoid the collision.
>
> Signed-off-by: Brian Bourn
> ---
> Part 1 of my submission for GSoC
> diff-no-index.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Looks good to me. Doesn't Eric deserve a "Helped-by:" above?
Thanks.
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 8e10bff..ec51106 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -16,7 +16,7 @@
> #include "builtin.h"
> #include "string-list.h"
>
> -static int read_directory(const char *path, struct string_list *list)
> +static int read_directory_contents(const char *path, struct string_list
> *list)
> {
> DIR *dir;
> struct dirent *e;
> @@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,
> int i1, i2, ret = 0;
> size_t len1 = 0, len2 = 0;
>
> - if (name1 && read_directory(name1, &p1))
> + if (name1 && read_directory_contents(name1, &p1))
> return -1;
> - if (name2 && read_directory(name2, &p2)) {
> + if (name2 && read_directory_contents(name2, &p2)) {
> string_list_clear(&p1, 0);
> return -1;
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git-rebase: Teach rebase "-" shorthand.
Thank you for the feedback and tweaks! > Is the eye-candy output to the standard output what is the most > interesting during the execution of a rebase? Wouldn't we be > more interested to make sure that we did transplant the history > on the same commit between two cases? I agree. I'll consult the other tests to see how to write such a test. > Can we use `git name-rev` to put the actual name here, so that people > who have not done what they intended can hopefully notice sooner? This sounds like a great idea! Doing so would mirror how `git checkout` behaves; checkout informs the user of which branch they have switched to when using the "-" shorthand: "Switched to branch 'master'". Should I submit a new patch, or reroll this one? - Brian Gesiak -- 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] git-rebase: Teach rebase "-" shorthand.
On Wed, Mar 19, 2014 at 10:53:01AM -0700, Junio C Hamano wrote:
>"rebase -" with your change still says something like this:
>
> First, rewinding head to replay your work on top of it...
> Fast-forwarded HEAD to @{-1}.
>
>instead of "Fast-forwarded HEAD to -". Somebody may later want
>to "fix" this, making these two eye-candy output to be different
>from each other, and what your test expects will no longer hold
>(not that I think it is better to say "-" instead of @{-1}
>there).
I don't think either of these is correct. When using "-" with the
commands that already support it, I have occasionally found that "-"
isn't what I thought it was.
Can we use `git name-rev` to put the actual name here, so that people
who have not done what they intended can hopefully notice sooner?
--
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 (Mar 2014, #03; Fri, 14)
> Am 19.03.2014 um 18:04 schrieb Junio C Hamano : > > Max Horn writes: > >>> On 17.03.2014, at 18:01, Junio C Hamano wrote: >>> >>> Torsten Bögershausen writes: >>> > On 2014-03-14 23.09, Junio C Hamano wrote: > * ap/remote-hg-skip-null-bookmarks (2014-01-02) 1 commit > - remote-hg: do not fail on invalid bookmarks > > Reported to break tests ($gmane/240005) > Expecting a reroll. I wonder what should happen here. The change breaks all the tests in test-hg-hg-git.sh (And the breakage may prevent us from detecting other breakages) The ideal situation would be to have an extra test case for the problem which we try to fix with this patch. Antoine, is there any way to make your problem reproducable ? And based on that, to make a patch which passes all test cases ? >>> >>> After re-reading the thread briefly (there're just five messages) >>> >>> http://thread.gmane.org/gmane.comp.version-control.git/239797/focus=240069 >> >> For some reason, that link does not contain all messages from that >> conversation (unfortunately, I have seen GMane do that on multiple >> occasions. I hence try not to rely on it for reviewing email >> history -- I just don't trust it). In particular, it misses this >> crucial post: > > [jc: please avoid overlong lines; I re-flowed above] Sorry. If anybody knows a way to tech Mail.app to auto-wrap long lines, I'd appreciate a hint. > >> http://thread.gmane.org/gmane.comp.version-control.git/239830 > > Interesting. > >> The (or at least "a") root cause has actually been >> discovered. Would a patch that adds an xfail test case for it be >> acceptable? > > Do you mean a patch that only adds a new test that expects a failure > to the current code, without touching the current code that has the > bug it exposes? Exactly. > That would be a good place to start. Ok. > >> ... As a matter of fact, I a know a few more bugs in remote-hg for >> which I could produce xfail test cases. Of course I'd prefer to >> put them in together with a fix, but I don't know when I can get >> to that, if ever. So, would such changes be welcome? > > Surely. That is to keep tabs on bugs in an actionable form; it is a > better way of bug tracking than having a bug-tracker that is not > actively maintained, I would think. Yeah, makes sense. So, one more silly (bikeshedding) question: should I do this as one big patch adding multiple xfail tests - or one commit per test, with perhaps a brief description of the issue at hand? Or should a code comment next to the failing test explain things? Actually, some of those bugs might require a lengthy background explanation, so yet another variant would be to write an email here With an explanation, then add a gmane ref to the commit message... > > > > -- 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] fsck.c: fsck_tree() now use is_dot_or_dotdot().
Andrei Dinu writes:
> ---
> I try to find other sites that can use id_dot_or_dotdot() function.
This should also have been sent in the same series.
> @@ -171,10 +172,12 @@ static int fsck_tree(struct tree *item, int strict,
> fsck_error error_func)
> has_full_path = 1;
> if (!*name)
> has_empty_name = 1;
> - if (!strcmp(name, "."))
> - has_dot = 1;
> - if (!strcmp(name, ".."))
> - has_dotdot = 1;
> + if (is_dot_or_dotdot(name)) {
> +if (name[1] == '\0')
> + has_dot = 1;
> +else
> + has_dotdot = 1;
> +}
The indentation is wrong. Configure your text editor to show you tabs
and spaces differently (e.g. M-x whitespace-mode RET in Emacs). Git uses
tabs to indent, and only that.
I find the old code much clearer than the new one. This "name[1] ==
'\0'" looks weird to test if name is the string ".".
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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] git-rebase: Teach rebase "-" shorthand.
Brian Gesiak writes:
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 6d94b1f..6176754 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -88,6 +88,17 @@ test_expect_success 'rebase from ambiguous branch name' '
> git rebase master
> '
>
> +test_expect_success 'rebase using shorthand' '
> + git checkout master &&
> + git checkout -b shorthand HEAD^ &&
> + git rebase - 1>shorthand.stdout &&
> + git checkout master &&
> + git branch -D shorthand &&
> + git checkout -b shorthand HEAD^ &&
> + git rebase @{-1} 1>without_shorthand.stdout &&
> + test_i18ncmp without_shorthand.stdout shorthand.stdout
> +'
A handful of issues here:
* "1>target" looks unconventional and wastes readers' time, forcing
them to wonder if there is anything special going on, only to
realize there isn't anything noteworthy. Saying ">target" like
everybody else does avoids attracting unnecessary attention.
* "rebase using shorthand" is somewhat a myopic title; it assumes
that the only short-hand relevant to rebase will be that a "-"
stands for "@{-1}" to specify the branch we rebase the current
branch off of.
* The usual filename for the output from the command being tested
is 'actual', and the usual filename for the expected output is
'expect'. In this case, you are verifying that the output from
"rebase -" is the same as the output from "rebase @{-1}", so it
is more conventional to call the former 'actual' and the latter
'expect'.
* Is the eye-candy output to the standard output what is the most
interesting during the execution of a rebase? Wouldn't we be
more interested to make sure that we did transplant the history
on the same commit between two cases?
"rebase -" with your change still says something like this:
First, rewinding head to replay your work on top of it...
Fast-forwarded HEAD to @{-1}.
instead of "Fast-forwarded HEAD to -". Somebody may later want
to "fix" this, making these two eye-candy output to be different
from each other, and what your test expects will no longer hold
(not that I think it is better to say "-" instead of @{-1}
there).
I'll tentatively queue it with a minor tweak (see below).
Thanks.
-- >8 --
From: Brian Gesiak
Date: Wed, 19 Mar 2014 20:02:15 +0900
Subject: [PATCH] rebase: allow "-" short-hand for the previous branch
Teach rebase the same shorthand as checkout and merge to name the
branch to rebase the current branch on; that is, that "-" means "the
branch we were previously on".
Requested-by: Tim Chase
Signed-off-by: Brian Gesiak
Signed-off-by: Junio C Hamano
---
git-rebase.sh | 4
t/t3400-rebase.sh | 17 +
2 files changed, 21 insertions(+)
diff --git a/git-rebase.sh b/git-rebase.sh
index 8a3efa2..658c003 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -449,6 +449,10 @@ then
test "$fork_point" = auto && fork_point=t
;;
*) upstream_name="$1"
+ if test "$upstream_name" = "-"
+ then
+ upstream_name="@{-1}"
+ fi
shift
;;
esac
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 6d94b1f..80e0a95 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -88,6 +88,23 @@ test_expect_success 'rebase from ambiguous branch name' '
git rebase master
'
+test_expect_success 'rebase off of the previous branch using "-"' '
+ git checkout master &&
+ git checkout HEAD^ &&
+ git rebase @{-1} >expect.messages &&
+ git merge-base master HEAD >expect.forkpoint &&
+
+ git checkout master &&
+ git checkout HEAD^ &&
+ git rebase - >actual.messages &&
+ git merge-base master HEAD >actual.forkpoint &&
+
+ test_cmp expect.forkpoint actual.forkpoint &&
+ # the next one is dubious---we may want to say "-",
+ # instead of @{-1}, in the message
+ test_i18ncmp expect.messages actual.messages
+'
+
test_expect_success 'rebase a single mode change' '
git checkout master &&
git branch -D topic &&
--
1.9.1-423-g4596e3a
--
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
[GSoc 2014 Project Ideas].
Signed-off-by: Andrei Dinu --- Hello, I'm interested in "Improve triangular workflow support" project idea. Can you give me, please, further information about this project? Thank you very much! -- 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 1/2] diff-no-index.c: rename read_directory()
Andrei Dinu writes: > Signed-off-by: Andrei Dinu The commit message should explain why this is a good change. In general, the "why?" question is more important than the "what" (the reader can see from the patch that you renamed the function, but cannot guess why you did so). Also, when sending multiple patches, send them as a single thread. See for example: http://thread.gmane.org/gmane.comp.version-control.git/21 In practice, call "git send-email" once, not once per patch. Try sending the patches to yourself first to avoid disturbing the list in case of user error. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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] tests: set temp variables using 'env' in test function instead of subshell
Jeff King writes: > On Tue, Mar 18, 2014 at 03:16:27PM -0700, Junio C Hamano wrote: > >> > Isn't GIT_CONFIG here another way of saying: >> > >> > test_must_fail git config -f doesnotexist --list >> > >> > Perhaps that is shorter and more readable still (and there are a few >> > similar cases in this patch. >> >> Surely, but are we assuming that "git config" correctly honors the >> equivalence between GIT_CONFIG=file and -f file, or is that also >> something we are testing in these tests? > > I think we can assume that they are equivalent, and it is not worth > testing (and they are equivalent in code since 270a344 (config: stop > using config_exclusive_filename, 2012-02-16). > > My recollection is that GIT_CONFIG mostly exists as a historical > footnote. Recall that at one time it affected all commands, but that had > many problems and was done away with in dc87183 (Only use GIT_CONFIG in > "git config", not other programs, 2008-06-30). I think we left it in > place for git-config mostly for backward compatibility,... Thanks. Then I think it makes sense to do such a conversion but it probably should be done on top of this patch (we could do it before this patch), not as a part of this patch. -- 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][GSOC] diff: rename read_directory() to get_directory_list()
Junio C Hamano writes: > Hiroyuki Sano writes: > >> Including "dir.h" in "diff-no-index.c", it causes a compile error, because >> the same name function read_directory() is declared globally in "dir.h". >> >> This change is to avoid conflicts as above. >> >> Signed-off-by: Hiroyuki Sano >> --- >> diff-no-index.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/diff-no-index.c b/diff-no-index.c >> index 8e10bff..1ed5c9d 100644 >> --- a/diff-no-index.c >> +++ b/diff-no-index.c >> @@ -16,7 +16,7 @@ >> #include "builtin.h" >> #include "string-list.h" >> >> -static int read_directory(const char *path, struct string_list *list) >> +static int get_directory_list(const char *path, struct string_list *list) > > Renaming is a good idea but the new name sounds like you are > grabbing the names of directories, ignoring all the files, no? I am tempted to suggest, because this is an internal implementation detail only visible to this narrow corner of the system, calling this just static int ls(const char *path, struct string_list *result) ;-) -- 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] test-lib.sh: use printf instead of echo
Jonathan Nieder writes:
> Junio C Hamano wrote:
>>> Uwe Storbeck wrote:
>
+ printf '%s\n' "$@" | sed -e 's/^/# /'
>>
>> This is wrong, isn't it? Why do we want one line per item here?
>
> Yes, Hannes caught the same, too. Sorry for the sloppiness.
>
> We currently use "echo" all over the place (e.g., 'echo "$path"' in
> git-sh-setup), and every time we fix it there is a chance of making
> mistakes. I wonder if it would make sense to add a helper to make the
> echo calls easier to replace:
I agree that we would benefit from having a helper to print a single
line, which we very often do, without having to worry about the
boilerplate '%s\n' of printf or the portability gotcha of echo.
I am a bit reluctant to name the helper "sane_echo" to declare "echo
that interprets backslashes in the string is insane", though. For
these "print a single line" uses, we are only interested in using a
subset of the features offered by 'echo', but that does not mean the
other features we do not want to trigger in our use is of no use to
any sane person. It very different from "sane_unset" that works
around "unset" on an unset variable that can trigger an error when
nobody sane is interested in that error condition. If somebody is
interested if a variable is not yet set and behave differently,
there are more direct ways to see the "set-ness" of a variable, and
asking "unset" for that information is insane, hence I think the
name "sane_unset" is justified. I do not feel the same way for
"sane_echo".
I would have called it "say" if the name weren't taken.
> -- >8 --
> Subject: git-sh-setup: introduce sane_echo helper
>
> Using 'echo' with arguments that might contain backslashes or "-e" or
> "-n" can produce confusing results that vary from platform to platform
> (e.g., dash always interprets \ escape sequences and echoes "-e"
> verbatim, whereas bash does not interpret \ escapes unless "-e" is
> passed as an argument to echo and suppresses the "-e" from its
> output).
>
> Instead, we should use printf, which is more predictable:
>
> printf '%s\n' "$foo"; # Just prints $foo, on all platforms.
>
> Blindly replacing echo with "printf '%s\n'" would not be good enough
> because that printf prints each argument on its own line. Provide a
> sane_echo helper that prints its arguments, space-delimited, on a
> single line, to make this easier to remember, and tweak 'say'
> and 'die_with_status' to illustrate how it is used.
>
> No functional change intended.
>
> Signed-off-by: Jonathan Nieder
> ---
> git-sh-setup.sh | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git i/git-sh-setup.sh w/git-sh-setup.sh
> index 256c89a..f35b5b9 100644
> --- i/git-sh-setup.sh
> +++ w/git-sh-setup.sh
> @@ -43,6 +43,10 @@ git_broken_path_fix () {
>
> # @@BROKEN_PATH_FIX@@
>
> +sane_echo () {
> + printf '%s\n' "$*"
> +}
> +
> die () {
> die_with_status 1 "$@"
> }
> @@ -50,7 +54,7 @@ die () {
> die_with_status () {
> status=$1
> shift
> - printf >&2 '%s\n' "$*"
> + sane_echo >&2 "$*"
> exit "$status"
> }
>
> @@ -59,7 +63,7 @@ GIT_QUIET=
> say () {
> if test -z "$GIT_QUIET"
> then
> - printf '%s\n' "$*"
> + sane_echo "$*"
> fi
> }
>
--
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] test-lib.sh: use printf instead of echo
Junio C Hamano writes: > Jonathan Nieder writes: > >> Junio C Hamano wrote: Uwe Storbeck wrote: >> > + printf '%s\n' "$@" | sed -e 's/^/# /' >>> >>> This is wrong, isn't it? Why do we want one line per item here? >> >> Yes, Hannes caught the same, too. Sorry for the sloppiness. >> >> We currently use "echo" all over the place (e.g., 'echo "$path"' in >> git-sh-setup), and every time we fix it there is a chance of making >> mistakes. I wonder if it would make sense to add a helper to make the >> echo calls easier to replace: > > I agree that we would benefit from having a helper to print a single > line, which we very often do, without having to worry about the > boilerplate '%s\n' of printf or the portability gotcha of echo. > > I am a bit reluctant to name the helper "sane_echo" to declare "echo > that interprets backslashes in the string is insane", though. raw_echo -- David Kastrup -- 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 4/7] commit: fix patch hunk editing with "commit -p -m"
Torsten Bögershausen writes:
>> +int run_hook_with_custom_index(const char *index_file, const char *name,
>> ...)
>> +{
>> +const char *hook_env[3] = { NULL };
>> +char index[PATH_MAX];
> Sorry being late with the review.
>
> Recently some effort has been put to replace the
> "PATH_MAX/snprintf() combo" with code using strbuf.
>
> So I think for new developed code it could make sense to avoid
> PATH_MAX from the start.
Yes but because this is a compatibility wrapper for an existing
function that does the string[PATH_MAX] thing already, it would be
clearer to have such a conversion as a separate step.
--
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] Enable index-pack threading in msysgit.
On Wed, Mar 19, 2014 at 3:28 AM, Duy Nguyen wrote: > On Wed, Mar 19, 2014 at 2:50 PM, Stefan Zager wrote: >> >> I suppose it would be possible to fix the immediate problem just by >> using one fd per thread, without a new pread implementation. But it >> seems better overall to have a pread() implementation that is >> thread-safe as long as read() and pread() aren't interspersed; and >> then convert all existing read() calls to pread(). That would be a >> good follow-up patch... > > I still don't understand how compat/pread.c does not work with pack_fd > per thread. I don't have Windows to test, but I forced compat/pread.c > on on Linux with similar pack_fd changes and it worked fine, helgrind > only complained about progress.c. > > A pread() implementation that is thread-safe with condition sounds > like an invite for trouble later. And I don't think converting read() > to pread() is a good idea. Platforms that rely on pread() will hit > first because of more use of compat/pread.c. read() seeks while > pread() does not, so we have to audit more code.. Using one fd per thread is all well and good for something like index-pack, which only accesses a single pack file. But using that heuristic to add threading elsewhere is probably not going to work. For example, I have a patch in progress to add threading to checkout, and another one planned to add threading to status. In both cases, we would need one fd per thread per pack file, which is pretty ridiculous. There really aren't very many calls to read() in the code. I don't think it would be very difficult to eliminate the remaining ones. The more interesting question, I think is: what platforms still don't have a thread-safe pread implementation? -- 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 v5] fsck.c: fsck_tree() now use is_dot_or_dotdot().
Rewrite fsck_tree() to use is_dot_or_dotdot() from "dir.h".
Signed-off-by: Andrei Dinu
---
I try to find other sites that can use id_dot_or_dotdot() function.
fsck.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/fsck.c b/fsck.c
index 64bf279..82a55ab 100644
--- a/fsck.c
+++ b/fsck.c
@@ -6,6 +6,7 @@
#include "commit.h"
#include "tag.h"
#include "fsck.h"
+#include "dir.h"
static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
{
@@ -171,10 +172,12 @@ static int fsck_tree(struct tree *item, int strict,
fsck_error error_func)
has_full_path = 1;
if (!*name)
has_empty_name = 1;
- if (!strcmp(name, "."))
- has_dot = 1;
- if (!strcmp(name, ".."))
- has_dotdot = 1;
+ if (is_dot_or_dotdot(name)) {
+if (name[1] == '\0')
+ has_dot = 1;
+else
+ has_dotdot = 1;
+}
if (!strcmp(name, ".git"))
has_dotgit = 1;
has_zero_pad |= *(char *)desc.buffer == '0';
--
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
[PATCH v3 2/2][GSoC] diff-no-index: replace manual "."/".." check with is_dot_or_dotdot()
Signed-off-by: Brian Bourn
---
Part 2 of my submission for GSoC
diff-no-index.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index ec51106..c554691 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -15,6 +15,7 @@
#include "log-tree.h"
#include "builtin.h"
#include "string-list.h"
+#include "dir.h"
static int read_directory_contents(const char *path, struct string_list *list)
{
@@ -25,7 +26,7 @@ static int read_directory_contents(const char *path, struct
string_list *list)
return error("Could not open directory %s", path);
while ((e = readdir(dir)))
- if (strcmp(".", e->d_name) && strcmp("..", e->d_name))
+ if (!is_dot_or_dotdot(e->d_name))
string_list_insert(list, e->d_name);
closedir(dir);
--
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
[PATCH v3 1/2][GSoC] diff-no-index: rename read_directory()
It would be desirable to replace manual checking of "." or ".."
in diff-no-index.c with is_dot_or_dotdot(), which is defined
in dir.h, however, dir.h declares a read_directory() which conflicts
with a (different) static read_directory() defined
in diff-no-index.c. As a preparatory step, rename the local
read_directory() to avoid the collision.
Signed-off-by: Brian Bourn
---
Part 1 of my submission for GSoC
diff-no-index.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index 8e10bff..ec51106 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -16,7 +16,7 @@
#include "builtin.h"
#include "string-list.h"
-static int read_directory(const char *path, struct string_list *list)
+static int read_directory_contents(const char *path, struct string_list *list)
{
DIR *dir;
struct dirent *e;
@@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,
int i1, i2, ret = 0;
size_t len1 = 0, len2 = 0;
- if (name1 && read_directory(name1, &p1))
+ if (name1 && read_directory_contents(name1, &p1))
return -1;
- if (name2 && read_directory(name2, &p2)) {
+ if (name2 && read_directory_contents(name2, &p2)) {
string_list_clear(&p1, 0);
return -1;
}
--
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
[PATCH v4 2/2] diff-no-index.c: read_directory_path() use is_dot_or_dotdot().
Implement code so read_directory_path() use is_dot_or_dotdot() from "dir.h"
instead of strcmp().
Signed-off-by: Andrei Dinu
---
I plan on applying to GSoc 2014.
diff-no-index.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index 5e4a76c..2d1165f 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -15,6 +15,7 @@
#include "log-tree.h"
#include "builtin.h"
#include "string-list.h"
+#include "dir.h"
static int read_directory_path(const char *path, struct string_list *list)
{
@@ -25,7 +26,7 @@ static int read_directory_path(const char *path, struct
string_list *list)
return error("Could not open directory %s", path);
while ((e = readdir(dir)))
- if (strcmp(".", e->d_name) && strcmp("..", e->d_name))
+ if (!is_dot_or_dotdot(e->d_name))
string_list_insert(list, e->d_name);
closedir(dir);
--
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
[PATCH v4 1/2] diff-no-index.c: rename read_directory()
Signed-off-by: Andrei Dinu
---
I plan on applying to GSoc 2014
diff-no-index.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index 8e10bff..5e4a76c 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -16,7 +16,7 @@
#include "builtin.h"
#include "string-list.h"
-static int read_directory(const char *path, struct string_list *list)
+static int read_directory_path(const char *path, struct string_list *list)
{
DIR *dir;
struct dirent *e;
@@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,
int i1, i2, ret = 0;
size_t len1 = 0, len2 = 0;
- if (name1 && read_directory(name1, &p1))
+ if (name1 && read_directory_path(name1, &p1))
return -1;
- if (name2 && read_directory(name2, &p2)) {
+ if (name2 && read_directory_path(name2, &p2)) {
string_list_clear(&p1, 0);
return -1;
}
--
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 1/3] diff-no-index.c read_directory() use is_dot_or_dotdot().
> Subject: Re: [PATCH 1/3] ^^^ I guess you meant PATCH v3. 1/3 means that this is the first patch out of 3. Andrei Dinu writes: > Implement read_directory() to use is_dot_or_dotdot() function from dir.h > instead of strcmp(). > > Rename read_directory() in read_directory_path() to avoid conflicting with > read_directory() from dir.h. Ideally, these should be two distinct patches. One to rename read_directory (no real change), and the next one to use is_dot_or_dotdot. You may want to take this as an exercice to learn how to split a patch, but I won't insist on that. Other than that, the patch looks good. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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] remote-hg: allow invalid bookmarks in a few edge cases
On Wed, Mar 19, 2014 at 4:00 PM, Max Horn wrote: >> Thank you for working on this. >> I believe it would be fair that you forget about patch 1/3 as you fix >> it in this patch (2/3). >> Also, I think it would be best NOT to integrate a patch (mine) that >> breaks a test, as it >> would make bisect harder to use. > > OK, makes sense. I didn't want to step on anybodies feet by hijacking > previously made work (however small or big it might be -- I've been burned by > this before). Anyway, so I'll squash the first two commits together (or all > three even?), and edit the message. But I'd like to properly attribute that > you discovered the issue, so perhaps I can add something like "Reported-by: > Antoine Pelisse" or so? Yes, I think you can squash all three commits into one, and use the reported-by line that you mentioned. Thanks, Antoine -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] diff-no-index.c read_directory() use is_dot_or_dotdot().
Implement read_directory() to use is_dot_or_dotdot() function from dir.h
instead of strcmp().
Rename read_directory() in read_directory_path() to avoid conflicting with
read_directory() from dir.h.
Signed-off-by: Andrei Dinu
---
I plan on applying to GSoc 2014.
diff-no-index.c |9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index 8e10bff..2d1165f 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -15,8 +15,9 @@
#include "log-tree.h"
#include "builtin.h"
#include "string-list.h"
+#include "dir.h"
-static int read_directory(const char *path, struct string_list *list)
+static int read_directory_path(const char *path, struct string_list *list)
{
DIR *dir;
struct dirent *e;
@@ -25,7 +26,7 @@ static int read_directory(const char *path, struct
string_list *list)
return error("Could not open directory %s", path);
while ((e = readdir(dir)))
- if (strcmp(".", e->d_name) && strcmp("..", e->d_name))
+ if (!is_dot_or_dotdot(e->d_name))
string_list_insert(list, e->d_name);
closedir(dir);
@@ -107,9 +108,9 @@ static int queue_diff(struct diff_options *o,
int i1, i2, ret = 0;
size_t len1 = 0, len2 = 0;
- if (name1 && read_directory(name1, &p1))
+ if (name1 && read_directory_path(name1, &p1))
return -1;
- if (name2 && read_directory(name2, &p2)) {
+ if (name2 && read_directory_path(name2, &p2)) {
string_list_clear(&p1, 0);
return -1;
}
--
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 2/3] remote-hg: allow invalid bookmarks in a few edge cases
Hi Antoine,
On 19.03.2014, at 14:07, Antoine Pelisse wrote:
> Hi Max,
>
> Thank you for working on this.
> I believe it would be fair that you forget about patch 1/3 as you fix
> it in this patch (2/3).
> Also, I think it would be best NOT to integrate a patch (mine) that
> breaks a test, as it
> would make bisect harder to use.
OK, makes sense. I didn't want to step on anybodies feet by hijacking
previously made work (however small or big it might be -- I've been burned by
this before). Anyway, so I'll squash the first two commits together (or all
three even?), and edit the message. But I'd like to properly attribute that you
discovered the issue, so perhaps I can add something like "Reported-by: Antoine
Pelisse" or so?
Max
>
> Thanks,
> Antoine
>
> On Wed, Mar 19, 2014 at 1:33 PM, Max Horn wrote:
>> Fix the previous commit to workaround issues with edge cases: Specifically,
>> remote-hg inserts a fake 'master' branch, unless the cloned hg repository
>> already contains a 'master' bookmark. If that 'master' bookmark happens
>> to reference the 'null' commit, the preceding fix ignores it. This
>> would leave us in an inconsistent state. Avoid this by NOT ignoring
>> null bookmarks named 'master' or 'default' under suitable circumstances.
>>
>> Signed-off-by: Max Horn
>> ---
>> contrib/remote-helpers/git-remote-hg | 7 +--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/contrib/remote-helpers/git-remote-hg
>> b/contrib/remote-helpers/git-remote-hg
>> index 12d850e..49b2c2e 100755
>> --- a/contrib/remote-helpers/git-remote-hg
>> +++ b/contrib/remote-helpers/git-remote-hg
>> @@ -626,8 +626,11 @@ def do_list(parser):
>> repo = parser.repo
>> for bmark, node in bookmarks.listbookmarks(repo).iteritems():
>> if node == '':
>> -warn("Ignoring invalid bookmark '%s'", bmark)
>> -continue
>> +if fake_bmark == 'default' and bmark == 'master':
>> +pass
>> +else:
>> +warn("Ignoring invalid bookmark '%s'", bmark)
>> +continue
>> bmarks[bmark] = repo[node]
>>
>> cur = repo.dirstate.branch()
>> --
>> 1.9.0.7.ga299b13
>>
>
signature.asc
Description: Message signed with OpenPGP using GPGMail
Unexpected difference between core.autocrlf=true and .gitattributes text=auto
I have a file.txt which is stored with CRLF in the repository (I'm on Windows, but that should not matter). autocrlf has been set: $ git config core.autocrlf true Git considers the working tree clean: $ touch file.txt $ git status On branch master nothing to commit, working directory clean However, when adding .gitattributes with following content: * text=auto Git starts considering the file as modified: $ touch file.txt $ git status On branch master Changes not staged for commit: ... modified: file.txt I think it's correct to consider the file as modified, because when committing it, it should be converted to CRLF. But why doesn't it show up as modified in the first case? -Marc -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2] fsck.c:fsck_commit() use skip_prefix() and starts_with()
make buffer const char* because the content of the memory referenced by this
variable doesn't change in this function.
Add cast to buffer to match fsck_ident signature which is (char**,...)
use skip_prefix() instead of memcmp() to avoid using magic number.
Signed-off-by: Othman Darraz
---
I am planning to apply for GSOC2014
fsck.c | 27 ++-
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/fsck.c b/fsck.c
index 5eae856..128d426 100644
--- a/fsck.c
+++ b/fsck.c
@@ -284,21 +284,22 @@ static int fsck_ident(char **ident, struct object *obj,
fsck_error error_func)
static int fsck_commit(struct commit *commit, fsck_error error_func)
{
- char *buffer = commit->buffer;
+ const char *buffer = commit->buffer;
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
int parents = 0;
int err;
-
- if (starts_with(buffer, "tree "))
+ buffer = skip_prefix(buffer, "tree ");
+ if (!buffer)
return error_func(&commit->object, FSCK_ERROR, "invalid format
- expected 'tree' line");
- if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\n')()
+ if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
return error_func(&commit->object, FSCK_ERROR, "invalid 'tree'
line format - bad sha1");
- buffer += 46;
- while (!memcmp(buffer, "parent ", 7)) {
- if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
+ buffer += 41;
+ while (starts_with(buffer, "parent ")) {
+ buffer = skip_prefix(buffer, "parent ");
+ if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n')
return error_func(&commit->object, FSCK_ERROR, "invalid
'parent' line format - bad sha1");
- buffer += 48;
+ buffer += 41;
parents++;
}
graft = lookup_commit_graft(commit->object.sha1);
@@ -322,16 +323,16 @@ static int fsck_commit(struct commit *commit, fsck_error
error_func)
if (p || parents)
return error_func(&commit->object, FSCK_ERROR, "parent
objects missing");
}
- if (starts_with(buffer, "author "))
+ buffer = skip_prefix(buffer, "author ");
+ if (!buffer)
return error_func(&commit->object, FSCK_ERROR, "invalid format
- expected 'author' line");
- buffer += 7;
- err = fsck_ident(&buffer, &commit->object, error_func);
+ err = fsck_ident((char **)&buffer, &commit->object, error_func);
if (err)
return err;
- buffer = (char* )skip_prefix(buffer,"committer ");
+ buffer = skip_prefix(buffer,"committer ");
if (!buffer)
return error_func(&commit->object, FSCK_ERROR, "invalid format
- expected 'committer' line");
- err = fsck_ident(&buffer, &commit->object, error_func);
+ err = fsck_ident((char **)&buffer, &commit->object, error_func);
if (err)
return err;
if (!commit->tree)
--
1.9.0.258.g00eda23.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] diff-no-index.c : rewrite read_directory() to use is_dot_or_dotdot().
Andrei Dinu writes:
> replace manual "."/".." check with is_dot_or_dotdot().
This is not what the patch below does.
> choose to implement my own function because did't find the defined one.
That does not seem to be a good reason to me. Run
git grep is_dot_or_dotdot
in Git's source code to find it (or use your favorite code navigation
tool like ctags/etags/...).
> [1]: http://article.gmane.org/gmane.comp.version-control.git/244420
> diff-no-index.c |8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 83cdbf7..d91ea3b 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -19,10 +19,10 @@
> static int is_dot_or_dotdot(const char *path)
> {
> if (path[0] == '.' && path[1] == '\0')
> -return 0;
> +return 1;
> else if (path[0] == '.' && path[1] == '.' && path[2] == '\0')
> -return 0;
> -return 1;
> +return 1;
> +return 0;
> }
>
> static int read_directory(const char *path, struct string_list *list)
> @@ -34,7 +34,7 @@ static int read_directory(const char *path, struct
> string_list *list)
> return error("Could not open directory %s", path);
>
> while ((e = readdir(dir)))
> - if (is_dot_or_dotdot(e->d_name))
> + if (!is_dot_or_dotdot(e->d_name))
> string_list_insert(list, e->d_name);
This could come on top of your previous patch, but when you resend,
please sent a new patch, not a "fixup patch" like this. Git's history
should be clean, and the patch that will eventually be applied should
not reflect the trial and error iterations that led to the result.
Read about "git rebase -i" and "git commit --amend" for more information
about this.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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
Loan Offer
Good Day, I am a private lender i give out Guarantee Business Loans,Personal Loans,Automobile Purchase Loans,House Purchase Loans, Car Loans E.T.C i give out long term loan ranging from $2,000.00 to $1,000,000.00 from One to Fifty years maximum with 3% interest rate if interested reply with the below information filled. Name In Full:__ Gender:_ Country:__ State:__ Loan Amount:___ Loan Duration:_ Monthly Income:__ Occupation:___ Cell Phone:__ Mr James Scott. -- 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] diff-no-index.c : rewrite read_directory() to use is_dot_or_dotdot().
replace manual "."/".." check with is_dot_or_dotdot().
choose to implement my own function because did't find the defined one.
[1]: http://article.gmane.org/gmane.comp.version-control.git/244420
Signed-off-by: Andrei Dinu
---
I plan on applying to GSoc 2014
diff-no-index.c |8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index 83cdbf7..d91ea3b 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -19,10 +19,10 @@
static int is_dot_or_dotdot(const char *path)
{
if (path[0] == '.' && path[1] == '\0')
-return 0;
+return 1;
else if (path[0] == '.' && path[1] == '.' && path[2] == '\0')
-return 0;
-return 1;
+return 1;
+return 0;
}
static int read_directory(const char *path, struct string_list *list)
@@ -34,7 +34,7 @@ static int read_directory(const char *path, struct
string_list *list)
return error("Could not open directory %s", path);
while ((e = readdir(dir)))
- if (is_dot_or_dotdot(e->d_name))
+ if (!is_dot_or_dotdot(e->d_name))
string_list_insert(list, e->d_name);
closedir(dir);
--
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
Confirm Receipt
Hello, I am Mrs.Supini Thrunkul from Tai Yau Bank Hong Kong,I need your cooperation to transfer $47.3 million US Dollars to any trusted account within your control. Contact me for more details. Mrs.Supini Thrunkul Tel: +85 2580 848 65 -- 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] remote-hg: allow invalid bookmarks in a few edge cases
Hi Max,
Thank you for working on this.
I believe it would be fair that you forget about patch 1/3 as you fix
it in this patch (2/3).
Also, I think it would be best NOT to integrate a patch (mine) that
breaks a test, as it
would make bisect harder to use.
Thanks,
Antoine
On Wed, Mar 19, 2014 at 1:33 PM, Max Horn wrote:
> Fix the previous commit to workaround issues with edge cases: Specifically,
> remote-hg inserts a fake 'master' branch, unless the cloned hg repository
> already contains a 'master' bookmark. If that 'master' bookmark happens
> to reference the 'null' commit, the preceding fix ignores it. This
> would leave us in an inconsistent state. Avoid this by NOT ignoring
> null bookmarks named 'master' or 'default' under suitable circumstances.
>
> Signed-off-by: Max Horn
> ---
> contrib/remote-helpers/git-remote-hg | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/remote-helpers/git-remote-hg
> b/contrib/remote-helpers/git-remote-hg
> index 12d850e..49b2c2e 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -626,8 +626,11 @@ def do_list(parser):
> repo = parser.repo
> for bmark, node in bookmarks.listbookmarks(repo).iteritems():
> if node == '':
> -warn("Ignoring invalid bookmark '%s'", bmark)
> -continue
> +if fake_bmark == 'default' and bmark == 'master':
> +pass
> +else:
> +warn("Ignoring invalid bookmark '%s'", bmark)
> +continue
> bmarks[bmark] = repo[node]
>
> cur = repo.dirstate.branch()
> --
> 1.9.0.7.ga299b13
>
--
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] Bump core.deltaBaseCacheLimit to 128MiB
The default of 16MiB causes serious thrashing for large delta chains combined with large files. Signed-off-by: David Kastrup --- Forgot the signoff. For the rationale of this patch and the 128MiB choice, see the original patch. Documentation/config.txt | 2 +- environment.c| 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 73c8973..1b6950a 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -484,7 +484,7 @@ core.deltaBaseCacheLimit:: to avoid unpacking and decompressing frequently used base objects multiple times. + -Default is 16 MiB on all platforms. This should be reasonable +Default is 128 MiB on all platforms. This should be reasonable for all users/operating systems, except on the largest projects. You probably do not need to adjust this value. + diff --git a/environment.c b/environment.c index c3c8606..73ed670 100644 --- a/environment.c +++ b/environment.c @@ -37,7 +37,7 @@ int core_compression_seen; int fsync_object_files; size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE; size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT; -size_t delta_base_cache_limit = 16 * 1024 * 1024; +size_t delta_base_cache_limit = 128 * 1024 * 1024; unsigned long big_file_threshold = 512 * 1024 * 1024; const char *pager_program; int pager_use_color = 1; -- 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
[PATCH 1/3] remote-hg: do not fail on invalid bookmarks
From: Antoine Pelisse
Mercurial can have bookmarks pointing to "nullid" (the empty root
revision), while Git can not have references to it.
When cloning or fetching from a Mercurial repository that has such a
bookmark, the import will fail because git-remote-hg will not be able to
create the corresponding reference.
Warn the user about the invalid reference, and continue the import,
instead of stopping right away.
Signed-off-by: Antoine Pelisse
Signed-off-by: Max Horn
---
contrib/remote-helpers/git-remote-hg | 3 +++
1 file changed, 3 insertions(+)
diff --git a/contrib/remote-helpers/git-remote-hg
b/contrib/remote-helpers/git-remote-hg
index eb89ef6..12d850e 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -625,6 +625,9 @@ def list_head(repo, cur):
def do_list(parser):
repo = parser.repo
for bmark, node in bookmarks.listbookmarks(repo).iteritems():
+if node == '':
+warn("Ignoring invalid bookmark '%s'", bmark)
+continue
bmarks[bmark] = repo[node]
cur = repo.dirstate.branch()
--
1.9.0.7.ga299b13
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] remote-hg: allow invalid bookmarks in a few edge cases
Fix the previous commit to workaround issues with edge cases: Specifically,
remote-hg inserts a fake 'master' branch, unless the cloned hg repository
already contains a 'master' bookmark. If that 'master' bookmark happens
to reference the 'null' commit, the preceding fix ignores it. This
would leave us in an inconsistent state. Avoid this by NOT ignoring
null bookmarks named 'master' or 'default' under suitable circumstances.
Signed-off-by: Max Horn
---
contrib/remote-helpers/git-remote-hg | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/contrib/remote-helpers/git-remote-hg
b/contrib/remote-helpers/git-remote-hg
index 12d850e..49b2c2e 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -626,8 +626,11 @@ def do_list(parser):
repo = parser.repo
for bmark, node in bookmarks.listbookmarks(repo).iteritems():
if node == '':
-warn("Ignoring invalid bookmark '%s'", bmark)
-continue
+if fake_bmark == 'default' and bmark == 'master':
+pass
+else:
+warn("Ignoring invalid bookmark '%s'", bmark)
+continue
bmarks[bmark] = repo[node]
cur = repo.dirstate.branch()
--
1.9.0.7.ga299b13
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] remote-hg: add test cases for null bookmarks
Signed-off-by: Max Horn --- contrib/remote-helpers/test-hg.sh | 48 +++ 1 file changed, 48 insertions(+) diff --git a/contrib/remote-helpers/test-hg.sh b/contrib/remote-helpers/test-hg.sh index a933b1e..8d01b32 100755 --- a/contrib/remote-helpers/test-hg.sh +++ b/contrib/remote-helpers/test-hg.sh @@ -772,4 +772,52 @@ test_expect_success 'remote double failed push' ' ) ' +test_expect_success 'clone remote with master null bookmark' ' + test_when_finished "rm -rf gitrepo* hgrepo*" && + + ( + hg init hgrepo && + cd hgrepo && + echo a >a && + hg add a && + hg commit -m a && + hg bookmark -r null master + ) && + + git clone "hg::hgrepo" gitrepo && + check gitrepo HEAD a +' + +test_expect_success 'clone remote with default null bookmark' ' + test_when_finished "rm -rf gitrepo* hgrepo*" && + + ( + hg init hgrepo && + cd hgrepo && + echo a >a && + hg add a && + hg commit -m a && + hg bookmark -r null -f default + ) && + + git clone "hg::hgrepo" gitrepo && + check gitrepo HEAD a +' + +test_expect_success 'clone remote with generic null bookmark' ' + test_when_finished "rm -rf gitrepo* hgrepo*" && + + ( + hg init hgrepo && + cd hgrepo && + echo a >a && + hg add a && + hg commit -m a && + hg bookmark -r null bmark + ) && + + git clone "hg::hgrepo" gitrepo && + check gitrepo HEAD a +' + test_done -- 1.9.0.7.ga299b13 -- 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 (Mar 2014, #03; Fri, 14)
On 19.03.2014, at 11:53, Max Horn wrote: > > On 17.03.2014, at 18:01, Junio C Hamano wrote: > >> Torsten Bögershausen writes: >> >>> On 2014-03-14 23.09, Junio C Hamano wrote: * ap/remote-hg-skip-null-bookmarks (2014-01-02) 1 commit - remote-hg: do not fail on invalid bookmarks Reported to break tests ($gmane/240005) Expecting a reroll. >>> I wonder what should happen here. >>> The change breaks all the tests in test-hg-hg-git.sh >>> (And the breakage may prevent us from detecting other breakages) >>> >>> The ideal situation would be to have an extra test case for the problem >>> which we try to fix with this patch. > [...] > The (or at least "a") root cause has actually been discovered. Would a patch > that adds an xfail test case for it be acceptable? > > As to the why the proposed patch causes test failures: I think this is due to > the fact that remote-hg inserts a fake "master" branch (resp. "bookmark" in > the hg terminology). Now, in those test cases, a hg repository gets created > that actually contains a "null" bookmark named "master". When the proposed > fix for the problem is added, this bookmarks gets ignored. But at that point, > remote-hg already determined that there is a hg bookmark named "master", and > adjusts how it works accordingly -- when we then remove that bookmarks, > things go awry. > > But I might be wrong here, and in any case, did not yet have time to come up > with a proper fix. Actually, scratch that, I just came up with a fix, and also tests. Will submit shortly. I'd still like to know whether it is OK to submit further patches with (x?)failing test cases? signature.asc Description: Message signed with OpenPGP using GPGMail
[PATCH v2 1/3][GSOC] diff: rename read_directory() to get_path_list()
Including "dir.h" in "diff-no-index.c", it causes a compile error, because
the same name function read_directory() is declared globally in "dir.h".
This change is to avoid conflicts as above.
Signed-off-by: Hiroyuki Sano
---
diff-no-index.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index 8e10bff..20b6a8a 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -16,7 +16,7 @@
#include "builtin.h"
#include "string-list.h"
-static int read_directory(const char *path, struct string_list *list)
+static int get_path_list(const char *path, struct string_list *list)
{
DIR *dir;
struct dirent *e;
@@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,
int i1, i2, ret = 0;
size_t len1 = 0, len2 = 0;
- if (name1 && read_directory(name1, &p1))
+ if (name1 && get_path_list(name1, &p1))
return -1;
- if (name2 && read_directory(name2, &p2)) {
+ if (name2 && get_path_list(name2, &p2)) {
string_list_clear(&p1, 0);
return -1;
}
--
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
[PATCH v2 3/3][GSOC] fsck: replace if-statements to logical expressions
There were two different ways to check flag values,
one way is using if-statement, and the other way is
using logical expression.
To make sensible, replace if-statements to logical expressions
in fsck_tree().
When checking "has_dot" and "has_dotdot", use is_dot_or_dotdot()
instead of strcmp() to avoid hard coding.
The is_dot_or_dotdot() is used to check if the string is
either "." or "..".
Include the "dir.h" header file to use is_dot_or_dotdot().
Signed-off-by: Hiroyuki Sano
---
fsck.c | 19 +++
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/fsck.c b/fsck.c
index b3022ad..08f613d 100644
--- a/fsck.c
+++ b/fsck.c
@@ -6,6 +6,7 @@
#include "commit.h"
#include "tag.h"
#include "fsck.h"
+#include "dir.h"
static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
{
@@ -165,18 +166,12 @@ static int fsck_tree(struct tree *item, int strict,
fsck_error error_func)
sha1 = tree_entry_extract(&desc, &name, &mode);
- if (is_null_sha1(sha1))
- has_null_sha1 = 1;
- if (strchr(name, '/'))
- has_full_path = 1;
- if (!*name)
- has_empty_name = 1;
- if (!strcmp(name, "."))
- has_dot = 1;
- if (!strcmp(name, ".."))
- has_dotdot = 1;
- if (!strcmp(name, ".git"))
- has_dotgit = 1;
+ has_null_sha1 |= is_null_sha1(sha1);
+ has_full_path |= !!strchr(name, '/');
+ has_empty_name |= !*name;
+ has_dot |= is_dot_or_dotdot(name) && !name[1];
+ has_dotdot |= is_dot_or_dotdot(name) && name[1];
+ has_dotgit |= !strcmp(name, ".git");
has_zero_pad |= *(char *)desc.buffer == '0';
update_tree_entry(&desc);
--
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
[PATCH v2 2/3][GSOC] diff: use is_dot_or_dotdot() instead of strcmp()
The is_dot_or_dotdot() is used to check if the string is either "." or "..".
Include the "dir.h" header file to use is_dot_or_dotdot().
Signed-off-by: Hiroyuki Sano
---
diff-no-index.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index 20b6a8a..8e642b3 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -11,6 +11,7 @@
#include "tag.h"
#include "diff.h"
#include "diffcore.h"
+#include "dir.h"
#include "revision.h"
#include "log-tree.h"
#include "builtin.h"
@@ -25,7 +26,7 @@ static int get_path_list(const char *path, struct string_list
*list)
return error("Could not open directory %s", path);
while ((e = readdir(dir)))
- if (strcmp(".", e->d_name) && strcmp("..", e->d_name))
+ if (!is_dot_or_dotdot(e->d_name))
string_list_insert(list, e->d_name);
closedir(dir);
--
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
Financial Service / Loan Service
We offer all purpose loan at 3% interest rate. Contact Us for more details by Email:santanderfinancegr...@gmail.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
Configuring a third-party git hook
I have a bit of a weird question. Poking around with Google searches hasn't come up with any results, so I'm asking here :) Short version: What's the most appropriate way to configure a git hook? Long version: I have a git hook (handles prepare-commit-msg and commit-msg) and part of what it does can search 'git log' for a single file. It doesn't really care about the full history, and wants to be reasonably fast (as the user is waiting for it). It's just a convenience, so correctness isn't a huge issue. The easiest way to keep it moving through quickly is to limit the search: $ git log ...other options... HEAD~100 some-file.pike The problem with this is that it doesn't work if HEAD doesn't have 100 great-great-...-grandparents - plus, it's way too specific a number to hard-code. I might want it different on different repos (and the script is shared, and is available for other people to use). Now, if this were something in git core, I'd expect to set that value of 100 with 'git config', but this is my own script. Is it right to use 'git config' for something that isn't controlled by the core code of git? I've tentatively used "git config rosuav.log-search.limit" (with 0 or absence meaning "omit the argument" ie search the whole history), and am wondering if that's a really really bad idea. Here's the script in question: https://github.com/Rosuav/shed/blob/master/githook.pike#L36 Two parts to the question, then. Firstly, is it acceptable to use 'git config' for a hook like this? And secondly, either: Is there a naming convention to follow? or, what alternative would you recommend? Thanks in advance for any ideas/tips! ChrisA -- 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 4/4] gc --aggressive: three phase repacking
On Tue, Mar 18, 2014 at 12:00 PM, Duy Nguyen wrote: >>> size time >>> old aggr. 36MB 5m51 >>> new aggr. 37MB 6m13 >>> repack -adf 48MB 1m12 >> >> I am not clear on what these times mean. It looks like the new code is >> slower _and_ bigger. Can you explain them? > > That's right :) The upside is faster operations, which is complely > missed here. The good thing from those numbers is pack size does not > increase much (the upper limit would be repack -adf with default > settings). Something is not right. The performance numbers are against me. Still looking.. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] git-rebase: Teach rebase "-" shorthand.
Teach rebase the same shorthand as checkout and merge; that is, that "-"
means "the branch we were previously on".
Reported-by: Tim Chase
Signed-off-by: Brian Gesiak
---
git-rebase.sh | 4
t/t3400-rebase.sh | 11 +++
2 files changed, 15 insertions(+)
diff --git a/git-rebase.sh b/git-rebase.sh
index 5f6732b..2c75e9f 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -453,6 +453,10 @@ then
test "$fork_point" = auto && fork_point=t
;;
*) upstream_name="$1"
+ if test "$upstream_name" = "-"
+ then
+ upstream_name="@{-1}"
+ fi
shift
;;
esac
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 6d94b1f..6176754 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -88,6 +88,17 @@ test_expect_success 'rebase from ambiguous branch name' '
git rebase master
'
+test_expect_success 'rebase using shorthand' '
+ git checkout master &&
+ git checkout -b shorthand HEAD^ &&
+ git rebase - 1>shorthand.stdout &&
+ git checkout master &&
+ git branch -D shorthand &&
+ git checkout -b shorthand HEAD^ &&
+ git rebase @{-1} 1>without_shorthand.stdout &&
+ test_i18ncmp without_shorthand.stdout shorthand.stdout
+'
+
test_expect_success 'rebase a single mode change' '
git checkout master &&
git branch -D topic &&
--
1.8.5.2 (Apple Git-48)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[BUG] Segfault on git describe
Hello,
Trying to do some stats using the Firefox git repository
(https://github.com/mozilla/gecko-dev), I found a bug
on git describe. The following command will segfault:
git describe --contains a9ff31aebd6dbda82a3c733a72eeeaa0b0525b96
Please note that the Firefox history is a pretty long and this commit
date is 2001.
I experience this issue with the git version, and Debian packages
(1.9.0-1 and 2.0~next.20140214-2)
As attachment, the backtrace. I removed about 87250 calls to the
name_rev function. I guess that is a potential source of problem.
Full is available here:
http://people.mozilla.org/~sledru/bt-git-on-ff.txt (21 MB)
I am available to test patches if needed.
Thanks,
Sylvestre
PS: I am not registered, please cc me.
#0 inflate_table (type=type@entry=CODES, lens=0x178f230, codes=codes@entry=19,
table=0x178f228,
bits=bits@entry=0x178f210, work=work@entry=0x178f4b0) at inftrees.c:39
len = 0
sym =
min =
max =
root =
curr =
drop =
left =
used =
huff =
incr =
fill =
low =
mask =
here =
next =
base =
extra =
end =
count = {0, 0, 0, 0, 0, 0, 0, 0, 31046, 360, 0, 0, 62288, 376, 0, 0}
offs = {36858, 61615, 32767, 0, 0, 0, 0, 0, 127, 0, 0, 0, 65535, 65535,
0, 0}
lbase = {3, 4, 5, 6, 7, 8, 9, 10, 11, 13, 15, 17, 19, 23, 27, 31, 35,
43, 51, 59, 67, 83, 99, 115, 131,
163, 195, 227, 258, 0, 0}
lext = {16, 16, 16, 16, 16, 16, 16, 16, 17, 17, 17, 17, 18, 18, 18, 18,
19, 19, 19, 19, 20, 20, 20, 20, 21,
21, 21, 21, 16, 72, 78}
dbase = {1, 2, 3, 4, 5, 7, 9, 13, 17, 25, 33, 49, 65, 97, 129, 193,
257, 385, 513, 769, 1025, 1537, 2049,
3073, 4097, 6145, 8193, 12289, 16385, 24577, 0, 0}
dext = {16, 16, 16, 16, 17, 17, 18, 18, 19, 19, 20, 20, 21, 21, 22, 22,
23, 23, 24, 24, 25, 25, 26, 26, 27,
27, 28, 28, 29, 29, 64, 64}
#1 0x77bce6fd in inflate (strm=0x7f7ff190, flush=4) at
inflate.c:926
state = 0x178f1a0
next = 0x7fffb3b7e5b2
"?\305-LHU\355\070\315\271\002\006\220P\a\220\220\212\330\317\366%\265\232ĕ{\251\bOOڑ\221\177\270\341t\337w\277\024f\360hLͨu\323\006\252\254\213\232\235\363\326qh=\326\301\372M\353u\335T\352D\205G\001\364\206-\306\065\066M4&Zc\243%\215\036\071\022a0\270qƑ\v\212&9\344\002\303|\274\033\362O\352{Z\345\322\301ß\305\023\350\305\345*\264\065\302\375z\211\ny\030\222\b\377\vV\257\351\033\332屟:0\266v\306na:E\022>\303\363\324\335\330\345
_\375\222A\350\310@\361B\243Pǐ[\330}\276\277Y\240\061\302\313~\177U\r$\222\306n\245>H\302\001\374\f"...
put =
have = 1022863950
left = 343
hold = 59
bits = 7
in = 1022863961
out =
copy =
from =
here =
len =
ret =
hbuf = "\247U\b\003"
order = {16, 17, 18, 0, 8, 7, 9, 6, 10, 5, 11, 4, 12, 3, 13, 2, 14, 1,
15}
#2 0x00559b7d in git_inflate (strm=0x7f7ff190, flush=4) at
zlib.c:118
status = 0
#3 0x0052d73d in unpack_compressed_entry (p=0x850ac0,
w_curs=0x7f7ff8a8, curpos=50877863, size=342)
at sha1_file.c:1933
st = 0
stream = {z = {
next_in = 0x7fffb3b7e5a7
"x\234\235\216\261N\303\060\020\206w?\305-LHU\355\070\315\271\002\006\220P\a\220\220\212\330\317\366%\265\232ĕ{\251\bOOڑ\221\177\270\341t\337w\277\024f\360hLͨu\323\006\252\254\213\232\235\363\326qh=\326\301\372M\353u\335T\352D\205G\001\364\206-\306\065\066M4&Zc\243%\215\036\071\022a0\270qƑ\v\212&9\344\002\303|\274\033\362O\352{Z\345\322\301ß\305\023\350\305\345*\264\065\302\375z\211\ny\030\222\b\377\vV\257\351\033\332屟:0\266v\306na:E\022>\303\363\324\335\330\345
_\375\222A\350\310@\361B\243Pǐ[\330}\276\277Y\240\061\302\313~\177U\r"...,
avail_in = 1022863961, total_in = 0, next_out = 0x178f040 "\034", avail_out =
343, total_out = 0, msg = 0x0,
state = 0x178f1a0, zalloc = 0x77bd3000 , zfree =
0x77bd3010 , opaque = 0x0,
data_type = 0, adler = 1, reserved = 0}, avail_in = 1022863961,
avail_out = 343, total_in = 0,
total_out = 0,
next_in = 0x7fffb3b7e5a7
"x\234\235\216\261N\303\060\020\206w?\305-LHU\355\070\315\271\002\006\220P\a\220\220\212\330\317\366%\265\232ĕ{\251\bOOڑ\221\177\270\341t\337w\277\024f\360hLͨu\323\006\252\254\213\232\235\363\326qh=\326\301\372M\353u\335T\352D\205G\001\364\206-\306\065\066M4&Zc\243%\215\036\071\022a0\270qƑ\v\212&9\344\002\303|\274\033\362O\352{Z\345\322\301ß\305\023\350\305\345*\264\065\302\375z\211\ny\030\222\b\377\vV\257\351\033\332屟:0\266v\306na:E\022>\303\363\324\335\330\345
_\375\222A\350\310@\361B\243Pǐ[\330}\276\277Y\240\061\302\313~\177U\r"...,
next_out = 0x178f040 "\034"}
buffer = 0x178f040 "\034"
in = 0x7fffb3b7e5a7
"x\234\235\216\261N\303\060\020\206w?\305-LHU\355\070\315\271\002\006\220P\a\220\220\212\330\317\366%\26
Re: What's cooking in git.git (Mar 2014, #03; Fri, 14)
On 17.03.2014, at 18:01, Junio C Hamano wrote:
> Torsten Bögershausen writes:
>
>> On 2014-03-14 23.09, Junio C Hamano wrote:
>>> * ap/remote-hg-skip-null-bookmarks (2014-01-02) 1 commit
>>> - remote-hg: do not fail on invalid bookmarks
>>>
>>> Reported to break tests ($gmane/240005)
>>> Expecting a reroll.
>> I wonder what should happen here.
>> The change breaks all the tests in test-hg-hg-git.sh
>> (And the breakage may prevent us from detecting other breakages)
>>
>> The ideal situation would be to have an extra test case for the problem
>> which we try to fix with this patch.
>>
>> Antoine, is there any way to make your problem reproducable ?
>> And based on that, to make a patch which passes all test cases ?
>
> After re-reading the thread briefly (there're just five messages)
>
> http://thread.gmane.org/gmane.comp.version-control.git/239797/focus=240069
For some reason, that link does not contain all messages from that conversation
(unfortunately, I have seen GMane do that on multiple occasions. I hence try
not to rely on it for reviewing email history -- I just don't trust it). In
particular, it misses this crucial post:
http://thread.gmane.org/gmane.comp.version-control.git/239830
I call it crucial because it describes how to make a reproducible test cases
out of this, in which a legitimate hg repository leads to a remote-hg error
preventing the user from normal operation.
>
> I think the "breakage" the patch tries to fix seems to be of dubious
> nature in the first place ("I don't know how I ended-up with such a
> bookmark", Antoine says in $gmane/239800), and it has been in
> "Expecting a reroll" state in response to "I will try to come-up
> with an improved version" in $gmane/240069 but nothing has happened
> for a few months.
>
> At this point I think it would be OK for me to discard the topic
> (without prejudice); if the root cause of the issue (if there is
> one) and a proper fix is discovered in the future, the topic can
> come back with a fresh patch, but it appears to me that keeping the
> above patch in my tree would not help anybody.
The (or at least "a") root cause has actually been discovered. Would a patch
that adds an xfail test case for it be acceptable?
As to the why the proposed patch causes test failures: I think this is due to
the fact that remote-hg inserts a fake "master" branch (resp. "bookmark" in the
hg terminology). Now, in those test cases, a hg repository gets created that
actually contains a "null" bookmark named "master". When the proposed fix for
the problem is added, this bookmarks gets ignored. But at that point, remote-hg
already determined that there is a hg bookmark named "master", and adjusts how
it works accordingly -- when we then remove that bookmarks, things go awry.
But I might be wrong here, and in any case, did not yet have time to come up
with a proper fix. What I do have is a test case, that I could turn into an
xfail test. As a matter of fact, I a know a few more bugs in remote-hg for
which I could produce xfail test cases. Of course I'd prefer to put them in
together with a fix, but I don't know when I can get to that, if ever. So,
would such changes be welcome?
signature.asc
Description: Message signed with OpenPGP using GPGMail
Re: [PATCH] Enable index-pack threading in msysgit.
On Wed, Mar 19, 2014 at 2:50 PM, Stefan Zager wrote: > On Wed, Mar 19, 2014 at 12:30 AM, Duy Nguyen wrote: >> On Wed, Mar 19, 2014 at 7:46 AM, wrote: >>> This adds a Windows implementation of pread. Note that it is NOT >>> safe to intersperse calls to read() and pread() on a file >>> descriptor. According to the ReadFile spec, using the 'overlapped' >>> argument should not affect the implicit position pointer of the >>> descriptor. Experiments have shown that this is, in fact, a lie. >> >> If I understand it correctly, new pread() is added because >> compat/pread.c does not work because of some other read() in between? >> Where are those read() (I can only see one in index-pack.c, but there >> could be some hidden read()..) > > I *think* it's the call to fixup_pack_header_footer(), but I'm not 100% sure. > > I suppose it would be possible to fix the immediate problem just by > using one fd per thread, without a new pread implementation. But it > seems better overall to have a pread() implementation that is > thread-safe as long as read() and pread() aren't interspersed; and > then convert all existing read() calls to pread(). That would be a > good follow-up patch... I still don't understand how compat/pread.c does not work with pack_fd per thread. I don't have Windows to test, but I forced compat/pread.c on on Linux with similar pack_fd changes and it worked fine, helgrind only complained about progress.c. A pread() implementation that is thread-safe with condition sounds like an invite for trouble later. And I don't think converting read() to pread() is a good idea. Platforms that rely on pread() will hit first because of more use of compat/pread.c. read() seeks while pread() does not, so we have to audit more code.. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3][GSOC] diff: rename read_directory() to get_directory_list()
On Mon, Mar 17, 2014 at 5:30 PM, Hiroyuki Sano wrote:
> Including "dir.h" in "diff-no-index.c", it causes a compile error, because
> the same name function read_directory() is declared globally in "dir.h".
>
> This change is to avoid conflicts as above.
This explanation is slightly lacking. Since dir.h is not currently
included by this file, a person reading this patch may wonder why you
need to avoid conflict where there is none. Adding a short sentence
saying that a subsequent patch will include dir.h in order to access
is_dot_or_dotdot() can avoid such confusion.
> Signed-off-by: Hiroyuki Sano
> ---
> diff-no-index.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 8e10bff..1ed5c9d 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -16,7 +16,7 @@
> #include "builtin.h"
> #include "string-list.h"
>
> -static int read_directory(const char *path, struct string_list *list)
> +static int get_directory_list(const char *path, struct string_list *list)
> {
> DIR *dir;
> struct dirent *e;
> @@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,
> int i1, i2, ret = 0;
> size_t len1 = 0, len2 = 0;
>
> - if (name1 && read_directory(name1, &p1))
> + if (name1 && get_directory_list(name1, &p1))
> return -1;
> - if (name2 && read_directory(name2, &p2)) {
> + if (name2 && get_directory_list(name2, &p2)) {
> string_list_clear(&p1, 0);
> return -1;
> }
> --
> 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] diff-no-index.c: rewrote read_directory() to use is_dot_or_dotdot() function.
Thanks for the submission. Comments below to give you a taste of the
Git review process...
On Wed, Mar 19, 2014 at 4:31 AM, Andrei Dinu wrote:
> Subject: diff-no-index.c: rewrote read_directory() to use is_dot_or_dotdot()
> function.
Use imperative tone: "rewrite" instead of "rewrote". The subject is a
bit long. Try to keep it to 65-70 characters. You might instead say:
diff-no-index: replace manual "."/".." check with is_dot_or_dotdot()
> is_dot_or_dotdot() verifies if the name of the directory sent as parameter to
> this function is the same with '.' or '..' and returns 0 if that is true.
Wrap commit message to 65-70 characters.
> There is unnecessary to iterate each character of the char* argument and
> verify it with strcmp because if there is a match that is at the beginning of
> chars.
You may be able to drop most or all of this text. A subject such as
the one suggested above probably conveys enough information to explain
and justify the patch without having to say anything more.
> Signed-off-by: Andrei Dinu
>
> I plan on applying to GSoc 2014
This commentary about GSoC won't be interesting to people reading the
commit message months or years from now, so place it after the "---"
line just below.
> ---
> diff-no-index.c | 11 ++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 8e10bff..83cdbf7 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -16,6 +16,15 @@
> #include "builtin.h"
> #include "string-list.h"
>
> +static int is_dot_or_dotdot(const char *path)
> +{
> +if (path[0] == '.' && path[1] == '\0')
> +return 0;
> +else if (path[0] == '.' && path[1] == '.' && path[2] == '\0')
> +return 0;
> +return 1;
> +}
Git already defines an is_dot_or_dotdot() function. Is there a reason
you chose to implement your own?
It is very unusual for a function asking "is this true" to return
false when the condition is true, and vice versa. It is not a good
idea to break expectations in this fashion.
> static int read_directory(const char *path, struct string_list *list)
> {
> DIR *dir;
> @@ -25,7 +34,7 @@ static int read_directory(const char *path, struct
> string_list *list)
> return error("Could not open directory %s", path);
>
> while ((e = readdir(dir)))
> - if (strcmp(".", e->d_name) && strcmp("..", e->d_name))
> + if (is_dot_or_dotdot(e->d_name))
> string_list_insert(list, e->d_name);
>
> closedir(dir);
> --
> 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] fsck.c:fsck_commit() use starts_with() and skip_prefix()
On Wed, Mar 19, 2014 at 5:04 AM, Othman Darraz wrote: > 2014-03-19 0:12 GMT+01:00 Eric Sunshine : >> On Tue, Mar 18, 2014 at 7:09 PM, Eric Sunshine >> wrote: >> >> >> diff --git a/fsck.c b/fsck.c >> >> index 64bf279..5eae856 100644 >> >> --- a/fsck.c >> >> +++ b/fsck.c >> >> @@ -290,7 +290,7 @@ static int fsck_commit(struct commit *commit, >> >> fsck_error error_func) >> >> int parents = 0; >> >> int err; >> >> >> >> - if (memcmp(buffer, "tree ", 5)) >> >> + if (starts_with(buffer, "tree ")) >> >> return error_func(&commit->object, FSCK_ERROR, "invalid >> >> format - expected 'tree' line"); >> >> if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\n') >> > >> > One of the reasons for using starts_with() rather than memcmp() is >> > that it allows you to eliminate magic numbers, such as 5. However, if >> > you look closely at this code fragment, you will see that the magic >> > number is still present in the expression 'buffer+5'. starts_with(), >> > might be a better fit. >> >> Of course, I meant "skip_prefix() might be a better fit". > > Thank you for your review and feedbacks. > Actually I made a mistake because I misunderstood how to run the tests, I > was using the wrong Makefile. For quick initial testing, you can just run the single test script. For instance: (cd t && ./t1450-fsck.sh) Once you have that running correctly, then run the entire test suite to ensure that your changes didn't break anything else. > Secondly I think , as well, that skip_prefix() > is a better fit. Nevertheless, as the variable buffer change in this > function, using skip_prefix() implies the use of cast. Yes, the variable named 'buffer' changes, but does the content of the memory referenced by 'buffer' ever change? If not, then 'buffer' could be made 'const', thus eliminating the need for the cast. (Note that changing it to 'const' might involve a bit of extra work, but the question is still pertinent.) > I will make the > changes right now. > > Thank you for your time. > > Othman DARRAZ -- 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] Selection of the verbose message is replaced with generated message in install_branch_config()
Thanks for the resubmission. Comments below...
On Tue, Mar 18, 2014 at 10:33 AM, Aleksey Mokhovikov
wrote:
> Subject: [PATCH][GSOC] Selection of the verbose message is replaced with
> generated message in install_branch_config()
Say [PATCH v2] to indicate your second attempt.
This subject is quite long. Try to keep it short but informative.
Mention the module or function first. Use imperative voice. You might
say:
Subject: install_branch_config: replace if-chain with table lookup
> This patch replaces if chain with
> 2 dimensional array of format strings and arguments.
This _is_ a patch, so no need to say so explicitly. Use imperative
voice. You might say:
Replace if-chain, which selects message in verbose mode, with
table-driven approach.
> Signed-off-by: Aleksey Mokhovikov
> ---
> This patch is unrelated with previous one, but related to GSoC.
> So I don't know if I should create new thread for this patch.
Keeping it in the same thread provides continuity which can help
reviewers. Providing a link to the previous attempt, like this [1], is
also courteous to reviewers.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/244237
> Compare with original construction
> Pros:
> 1) Remove if chain.
> 2) Single table contains all messages with arguments in one construction.
> 3) Less code duplication.
> Cons:
> 1) Harder to associate the case with message.
> 2) Harder for compiler to warn the programmer about not
>enough arguments for format string.
> 3) Harder to add non-string argument to messages.
Nice summary. Do you draw any conclusions from it? Is the new code
better? Easier to understand? Would it be better merely to refactor
the 'if' statements a bit instead of using a table?
> If !!(x) is out of the coding guide, then message_id construction
> can be rewritten in several other ways.
> The guideline tells that !!(x) is not welcome and should not be
> unless needed. But looks like this is normal place for it.
Curious. !!x is indeed used regularly. Where did you read that it is
not welcome?
> P.S.
> Thanks to comments I got, so
> I've read more GNU gettext manual to get info
> about translation workflow. When I posted a first patch
> I've passed the same message format strings to gettext /*_()*/
> as in original, to save the context of the message. And they
> will be translated if every possible string combination
> will be marked separately for translation. Because of
> xgettext can extract string from source automatically,
> it ruins the idea to generate message from parts. Even if the
> exaclty same message format string is passed to gettext.
Indeed, it's a common and understandable misconception.
> branch.c | 53 ++---
> 1 file changed, 30 insertions(+), 23 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..51a5faf 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -47,12 +47,32 @@ static int should_setup_rebase(const char *origin)
> return 0;
> }
>
> +
Unnecessary blank line insertion. This adds noise to the patch which
distracts from the real changes.
> 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/");
> struct strbuf key = STRBUF_INIT;
> int rebasing = should_setup_rebase(origin);
> + int message_id = (!!remote_is_branch << 2) | (!!origin << 1) |
> (!!rebasing);
It works, but it's also a fairly magical incantation, placing greater
cognitive demands on the reader. You could achieve a similar result
with a multi-dimensional table which is indexed directly by
!!remote_is_branch, !!origin, and !!rebasing. (Whether you actually
want to do so is a different question.)
> + const char *message_table[][4] =
> + {{N_("Branch %s set up to track local ref %s."),
> + local, remote},
> +{N_("Branch %s set up to track local ref %s by rebasing."),
> + local, remote},
> +{N_("Branch %s set up to track remote ref %s."),
> + local, remote},
> +{N_("Branch %s set up to track remote ref %s by rebasing."),
> + local, remote},
> +{N_("Branch %s set up to track local branch %s."),
> + local, shortname},
> +{N_("Branch %s set up to track local branch %s by
> rebasing."),
> + local, shortname},
> +{N_("Branch %s set up to track remote branch %s from %s."),
> + local, shortname, origin},
> +{N_("Branch %s set up to track remote branch %s from %s by
> rebasing."),
> + local, shortname, origin}};
Indeed, this is a reasonably decent way to keep the arguments and
messages together and can simplify the code nicely. Unfortunately,
this project is still restricted prima
[PATCH] Bump core.deltaBaseCacheLimit to 128MiB
The default of 16MiB causes serious thrashing for large delta chains combined with large files. --- 128MiB seems like a big bump, but the limit for files not getting delta compressed at all is 512MiB and it would appear that they should be a bit comparable. It is hard to find good benchmarks here: basically the best benchmark I found was git blame with problematically large repositories compressed with the (old) git-gc --aggressive settings, using not-yet-committed but presented code (the current blame code spends so much time thrashing in other places that the benchmark differences are less obvious). Documentation/config.txt | 2 +- environment.c| 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 73c8973..1b6950a 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -484,7 +484,7 @@ core.deltaBaseCacheLimit:: to avoid unpacking and decompressing frequently used base objects multiple times. + -Default is 16 MiB on all platforms. This should be reasonable +Default is 128 MiB on all platforms. This should be reasonable for all users/operating systems, except on the largest projects. You probably do not need to adjust this value. + diff --git a/environment.c b/environment.c index c3c8606..73ed670 100644 --- a/environment.c +++ b/environment.c @@ -37,7 +37,7 @@ int core_compression_seen; int fsync_object_files; size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE; size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT; -size_t delta_base_cache_limit = 16 * 1024 * 1024; +size_t delta_base_cache_limit = 128 * 1024 * 1024; unsigned long big_file_threshold = 512 * 1024 * 1024; const char *pager_program; int pager_use_color = 1; -- 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] rev-parse --parseopt: option argument name hints
On 3/12/2014 9:59 AM, Junio C Hamano wrote: Ilya Bobyr writes: I though that an example just to describe `argh' while useful would look a bit disproportional, compared to the amount of text on --parseopt. But now that I've added a "Usage text" section to looks quite in place. Good thinking. I was also wondering about the possible next step(s). If you like the patch will you just take it from the maillist and it would appear in the next "What's cooking in git.git"? Or the process is different? It goes more like this: Thank you for all the details. - A topic that is in a good enough shape to be discussed and moved forward is given its own topic branch and then merged to 'pu', so that we do not forget. The topic enters "What's cooking" at this stage. I can not find this particular patch in the latest "What's cooking" email. Is there something I can do? It does not seems like there is a lot of interest, so I am not sure there will be a lot of discussion. It is a minor fix and considering the number of the emails on the list, I do not unexpected this kind of stuff to be very popular. But it seems like a valid improvement to me. Maybe I am missing something? Same questions about this one: [PATCH] gitk: replace SHA1 entry field on keyboard paste http://www.mail-archive.com/git@vger.kernel.org/msg45040.html I think they are more or less similar, except that the second one is just trivial. - Discussion on the topic continues on the list, and the topic can be replaced or built upon while it is still on 'pu' to polish it further. . We may see a grave issue with the change and may discard it from 'pu'. . We may see a period of inaction after issues are pointed out and/or improvements are suggested, which would cause the topic marked as stalled; this may cause it to be eventually discarded as "abandoned" if nobody cares deeply enough. - After a while, when it seems that we, collectively as the Git development circle, agree that we would eventually want that change in a released version in some future (not necessarily in the upcoming release), the topic is merged to 'next', which is the branch Git developers are expected to run in their daily lives. . We may see some updates that builds on the patches merged to 'next' so far to fix late issues discovered. . We may see a grave issue with the change and may have to revert & discard it from 'next'. - After a while, when the topic proves to be solid, it is merged to 'master', in preparation for the upcoming release. -- 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: [GSoC14][RFC] Is there any interest in adding a port of checkpatch.pl to contrib/?
On 18 March 2014 02:38, Jacopo Notarstefano wrote: > 3. As far as I can tell, checkpatch needs to be run from the root > folder of a linux repository clone. Cloning several hundred MBs for a > single perl script looks a little foolish to me. If that is your worry maybe you should upload the script to CPAN. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/" -- 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
